feat(extractor/gin): Gin extractor enhancement — response tracing, param types, tags, route exclusion#69
Merged
spencercjh merged 10 commits intomainfrom Apr 3, 2026
Merged
Conversation
…m() detection The Gin extractor's buildOperation only added path parameters when c.Param() calls were detected in handler code analysis. Handlers that didn't explicitly call c.Param() for every path segment would produce invalid OpenAPI specs missing required path parameters. Now path parameters are always extracted from the route path definition (the authoritative source of truth), matching the existing behavior for handlers without analysis results. Signed-off-by: spencercjh <spencercjh@gmail.com>
…ort aliases findHandlerDecl now resolves package references by examining imports in the route registration file, correctly handling both direct package names and import aliases (e.g., customapis "gin-crosspkg/apis"). It also filters out methods (fn.Recv != nil) to avoid matching non-handler functions with receivers. Adds resolvePkgRef and pkgNameForImport helpers, plus an integration test with a multi-package fixture that exercises name collisions across apis, adapter, and adapter method declarations. Signed-off-by: spencercjh <spencercjh@gmail.com>
…ase 2 implements detection of standalone response helper functions like done(c, data, err) and extracts the Go type from arguments using varTypeMap. For response types from known helpers names (done, respond, response, writeJSON, sendJSON, reply). and the error argument patterns. Signed-off-by: spencercjh <spencercjh@gmail.com>
Signed-off-by: spencercjh <spencercjh@gmail.com>
…rence, tags, route exclusion - Cross-function call tracking: auto-discover helper funcs taking *gin.Context, trace into their bodies to find c.JSON/c.XML calls for response type extraction - Parameter type inference: detect integer/boolean/number from strconv.Atoi/ParseBool, c.GetInt/c.GetBool methods, and == "true" boolean comparisons - Tags inference from route path prefix (e.g. /api/v1/projects → tag: Projects) - Clean operationId by stripping package prefix (apis.CreateProject → CreateProject) - Add --exclude-route and --exclude-route-prefix CLI flags for configurable route filtering - Use inferred GoType for query/header param schemas instead of hardcoded string Signed-off-by: spencercjh <spencercjh@gmail.com>
Regenerated golden files to reflect the new features: - Tags inferred from route path (e.g. /api/v1/users → tag "Users") - Query params page/size now correctly typed as integer (from struct field types) Signed-off-by: spencercjh <spencercjh@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds route exclusion flags/config, implements route filtering, introduces cross-function helper tracing and response extraction, promotes embedded struct fields into schemas, infers parameter types from code patterns, and updates generator integration and tests/fixtures accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (generate.go)
participant Config as Config (config.go)
participant Generator as Generator (generator.go)
participant Parser as ASTParser (ast_parser.go)
participant HandlerAnalyzer as HandlerAnalyzer (handler_analyzer.go)
participant HelperTracer as HelperTracer (helper_tracer.go)
participant ResponseAnalyzer as ResponseAnalyzer (response_analyzer.go)
participant SchemaExtractor as SchemaExtractor (schema_extractor.go)
CLI->>Config: read exclusion flags / config
Config-->>CLI: merged exclude lists
CLI->>Generator: run Generate with options (incl. excludes)
Generator->>Parser: ExtractRoutes(excludeRoutes, excludePrefixes)
Parser->>Parser: normalize paths & filter defaults/custom prefixes
Parser-->>Generator: filtered routes
Generator->>HandlerAnalyzer: AnalyzeHandler(fn)
HandlerAnalyzer->>HelperTracer: detect helper calls & lookup helpers
HelperTracer->>ResponseAnalyzer: trace helper -> extract c.JSON/XML/YAML responses
ResponseAnalyzer-->>HelperTracer: ResponseInfo{Status, GoType}
HelperTracer-->>HandlerAnalyzer: attach Responses to HandlerInfo
HandlerAnalyzer->>HandlerAnalyzer: inferParamTypes (strconv, comparisons)
HandlerAnalyzer-->>Generator: HandlerInfo (params, responses)
Generator->>SchemaExtractor: ExtractSchema(type)
SchemaExtractor->>SchemaExtractor: promote embedded fields, avoid cycles
SchemaExtractor-->>Generator: OpenAPI schema
Generator-->>CLI: assembled OpenAPI spec (with tags, normalized operationIds)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Review Summary by QodoGin extractor enhancement — response tracing, parameter types, tags, route exclusion, and cross-package resolution
WalkthroughsDescription• **Cross-function response tracing**: Auto-discover helper functions accepting *gin.Context and trace into their bodies to find c.JSON/c.XML calls for response type extraction, covering non-standard helpers beyond done/respond • **Parameter type inference**: Detect integer/boolean/number types from strconv conversions (Atoi, ParseInt, ParseBool, ParseFloat), c.GetInt()/c.GetBool() methods, and boolean comparisons (== "true") • **Tags inference**: Infer OpenAPI tags from route path prefixes (e.g., /api/v1/users → tag Users) • **Cross-package handler resolution**: Resolve handler references across packages using import analysis and handle name collisions with proper package matching • **Configurable route exclusion**: Add --exclude-route and --exclude-route-prefix CLI flags for custom route filtering with default exclusions (/swagger, /docs, /debug, /static, /public) • **Embedded struct field promotion**: Implement field promotion for embedded structs following Go's promotion rules with cycle detection • **operationId cleanup**: Strip package prefix from operationId (e.g., apis.CreateProject → CreateProject) • **Comprehensive test coverage**: New tests for helper analysis, parameter type inference, route filtering, embedded struct schemas, and cross-package handler resolution with integration tests Diagramflowchart LR
A["Route Handler"] -->|"cross-package resolution"| B["Handler Declaration"]
B -->|"trace helper calls"| C["Response Type Extraction"]
A -->|"parameter analysis"| D["Type Inference"]
D -->|"strconv/GetInt/GetBool"| E["Typed Parameters"]
A -->|"path prefix"| F["Tag Inference"]
A -->|"exclusion rules"| G["Filtered Routes"]
B -->|"embedded structs"| H["Schema with Promoted Fields"]
C --> I["OpenAPI Spec"]
E --> I
F --> I
H --> I
File Changes1. internal/extractor/gin/handler_analyzer.go
|
Code Review by Qodo
1.
|
spencercjh
added
enhancement
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enhances the Gin extractor’s AST-based OpenAPI generation to better match real-world Gin code patterns, especially around response discovery, parameter typing, tags/operationId formatting, and route filtering.
Changes:
- Add cross-function response helper tracing and improve response/type inference utilities.
- Improve parameter typing (query/header) and enrich operations with inferred tags + cleaned
operationId. - Add configurable route exclusion (
--exclude-route,--exclude-route-prefix) plus default non-API route filtering; extend schema extraction to promote embedded struct fields.
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/extractor/types.go | Extend GenerateOptions with exclude-route settings. |
| internal/extractor/gin/schema_extractor.go | Promote embedded struct fields into parent schemas with cycle detection. |
| internal/extractor/gin/schema_extractor_test.go | Add tests covering embedded/pointer/nested embedded struct promotion. |
| internal/extractor/gin/helper_analyzer.go | Introduce helper/type/status-code inference helpers shared by handler analysis. |
| internal/extractor/gin/helper_analyzer_test.go | Add tests for helper tracing and parameter type inference. |
| internal/extractor/gin/handler_analyzer.go | Discover helper funcs, trace helper responses, infer param types, and typed parameter extraction. |
| internal/extractor/gin/generator.go | Pass exclude rules into route extraction; improve cross-package handler resolution; infer tags; use typed schemas for query/header params. |
| internal/extractor/gin/ast_parser.go | Add default + user-configurable route filtering during extraction. |
| internal/extractor/gin/ast_parser_test.go | Add tests for default/custom route exclusion logic. |
| cmd/generate.go | Add CLI flags and plumb exclude rules into GenerateOptions. |
| integration-tests/gin/gin_test.go | Add e2e test for cross-package handler name collision resolution. |
| integration-tests/gin/fixtures/golden/* | Update golden snapshots for tags and typed query params. |
| integration-tests/gin/fixtures/gin-crosspkg/* | Add new fixture project for cross-package handler resolution testing. |
| docs/plans/2026-04-02-gin-extractor-enhancement.md | Add enhancement plan document describing goals/approach. |
Comments suppressed due to low confidence (1)
internal/extractor/gin/ast_parser.go:125
- Route filtering now always applies the hard-coded
defaultExcludePrefixes(e.g.,/docs,/debug,/static). This is a behavior change that can silently drop legitimate APIs living under these prefixes, and there’s currently no opt-out. Consider making the defaults configurable (e.g., only apply when user hasn’t provided any exclude rules, or add a flag/config to disable default excludes).
// Filter out non-API routes (swagger docs, static files, debug endpoints, etc.)
routes = filterRoutes(routes, excludeRoutes, excludePrefixes)
slog.Info("Extracted routes", "count", len(routes))
return routes, nil
}
// extractGroups extracts router group definitions from a file.
func (p *ASTParser) extractGroups(file *ast.File) map[string]*GroupInfo {
groups := make(map[string]*GroupInfo)
ast.Inspect(file, func(n ast.Node) bool {
if node, ok := n.(*ast.AssignStmt); ok {
if len(node.Lhs) == 1 && len(node.Rhs) == 1 {
if group := p.parseGroupAssignment(node.Rhs[0]); group != nil {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
internal/extractor/gin/schema_extractor_test.go (1)
835-851: Consider consolidating the duplicate helper functions.
getSchemaPropKeysandgetSchemaKeys(lines 836-842) have identical implementations sinceopenapi3.Schemasis the same type used for both component schemas and properties. Consider using a single helper.♻️ Suggested consolidation
// Helper function to get schema keys for error messages func getSchemaKeys(schemas openapi3.Schemas) []string { keys := make([]string, 0, len(schemas)) for k := range schemas { keys = append(keys, k) } return keys } - -// Helper function to get property keys for error messages -func getSchemaPropKeys(props openapi3.Schemas) []string { - keys := make([]string, 0, len(props)) - for k := range props { - keys = append(keys, k) - } - return keys -}Then update the call sites (lines 740, 744, 788, 830) to use
getSchemaKeysinstead ofgetSchemaPropKeys.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extractor/gin/schema_extractor_test.go` around lines 835 - 851, Consolidate the duplicate helpers by removing getSchemaPropKeys and using getSchemaKeys everywhere; update all call sites that currently call getSchemaPropKeys to call getSchemaKeys instead (the functions operate on the same openapi3.Schemas type), and delete the redundant getSchemaPropKeys declaration so only getSchemaKeys remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/plans/2026-04-02-gin-extractor-enhancement.md`:
- Around line 9-10: The plan uses a workstation-specific absolute path
`/home/caijh/codes/open/openapi` which prevents reproducibility; update the
docs/plans/2026-04-02-gin-extractor-enhancement.md content to replace that
hardcoded path (and any other occurrences around lines 314-319) with a neutral
placeholder like `<openapi-repo>` or a relative path (e.g., `./openapi`) so
examples and comparison steps are reproducible for other contributors.
In `@internal/extractor/gin/ast_parser.go`:
- Around line 344-358: The shouldExcludeRoute function is matching filters
verbatim and with raw HasPrefix which misses parameter syntax differences and
mis-matches boundaries; normalize both the input path and all filters
(defaultExcludePrefixes, extraPrefixes, extraExcludes) to the same OpenAPI style
(e.g., convert ":param" to "{param}" and collapse duplicate slashes) before
comparing, perform prefix checks on segment boundaries (ensure a prefix match
only when the next character is '/' or end-of-string to avoid "/public" matching
"/publications"), and then do the exact exclude check against the normalized
extraExcludes; update the logic around defaultExcludePrefixes, extraPrefixes and
extraExcludes in shouldExcludeRoute to use these normalized values.
In `@internal/extractor/gin/generator.go`:
- Around line 360-363: The current change switches path param sourcing to
route.FullPath but extractPathParamsFromRoute hardcodes every param schema to
string, losing earlier analyzer-inferred types (e.g. int for id); update the
merge logic so extractPathParamsFromRoute (or its caller) preserves any existing
inferred schema/type for a parameter instead of overriding with string: when
appending to operation.Parameters, check for an existing parameter by name (from
analyzer results or prior operation.Parameters) and reuse its Schema/Type; only
create a new string-typed schema for parameters that are truly absent. Ensure
the logic references extractPathParamsFromRoute, operation.Parameters and
route.FullPath so name matching and schema preservation occur during the
append/merge.
In `@internal/extractor/gin/handler_analyzer.go`:
- Around line 23-24: The helper discovery currently uses a single helperFuncs
map keyed only by function name causing collisions across packages; update
discoverHelperFuncs to store helpers scoped by package (e.g.
map[string]map[string]*ast.FuncDecl or map[pkgName+"#"+funcName]*ast.FuncDecl)
using the package identifier from each parsed ast.File, and change
parseHelperCall to resolve bare helper calls by first looking up the helper in
the caller's package scope (and still support selector/resolved-package lookups)
so functions with the same name in different packages do not overwrite each
other; update any code that reads helperFuncs (references in
discoverHelperFuncs, parseHelperCall and other locations around lines ~41-52 and
~669-670) to use the new scoped key or nested map lookup.
- Around line 705-708: The response resolution must recursively replace
identifiers inside helper AST nodes before extracting the type: update the call
site where resolvedArg is computed (currently using
resolveThroughParamMap(call.Args[1], paramArgMap) before calling
extractTypeFromResponseWithStatus) to run a recursive resolution that walks
composite literals, unary/paren/selector/index/field expressions and replaces
identifier nodes from paramArgMap (and apply the same fix at the other
occurrence around lines 751-757). Concretely, extend or replace
resolveThroughParamMap to traverse the AST node tree for call.Args elements,
resolving identifiers nested inside composite literal fields, slices, maps,
selector expressions, etc., then pass the fully-resolved AST node into
extractTypeFromResponseWithStatus so extractTypeFromResponseWithStatus and
varTypeMap receive the actual concrete argument nodes rather than top-level
wrappers.
- Around line 624-630: The code currently uses StatusCode: 0 to represent an
OpenAPI "default" response which gets serialized as the numeric "0"; instead
introduce an explicit sentinel and serialization path: either add a boolean
field (e.g., ResponseInfo.IsDefault) and set IsDefault: true in the isErrorType
branch, or define a clearly-named constant (e.g., DefaultStatusCode = -1) and
set StatusCode: DefaultStatusCode in that branch; then update the response
generator that consumes []ResponseInfo (the serialization path that reads
ResponseInfo.StatusCode) to emit the string "default" when IsDefault is true or
when StatusCode == DefaultStatusCode. Ensure you modify the isErrorType handling
(where info.Responses is appended) and the ResponseInfo type/serialization code
so no numeric 0 is produced.
- Around line 287-290: Remove the case branches that treat Gin's Context.GetInt,
GetInt64, and GetBool as query parameters: delete or stop calling
a.extractTypedParam for the "GetInt", "GetInt64", and "GetBool" cases (the
switch handling of call → a.extractTypedParam(call, info, goTypeInteger,
"query") and a.extractTypedParam(call, info, goTypeBoolean, "query")). These
methods read Context.Keys (middleware-populated state) and must not be
documented as client query inputs; ensure only query-string accessors (e.g.,
c.Query variants) are handled as "query" parameters in the same switch so that
query extraction remains correct.
In `@internal/extractor/gin/helper_analyzer.go`:
- Around line 87-105: The isErrorType heuristic is too permissive: remove the
hard-coded checks that treat an identifier named "e" and the literal "nil" as
errors; instead only treat an identifier as an error when varTypeMap indicates
its static type is "error" or "*errors.errorString". Update the isErrorType
function to drop the ident.Name == "e" branch and the nil-special-case branch
and rely on the varTypeMap lookup (and you may also remove or optionally keep
the ident.Name == "err" special-case in favor of the varTypeMap) so only
statically-typed errors are recognized.
In `@internal/extractor/gin/schema_extractor.go`:
- Around line 301-314: The embedded selector lookup currently loses package
context because resolveEmbeddedTypeName returns a pkg-qualified name but
e.findTypeSpec falls back to a short-name global scan, causing wrong-type
matches when two packages export the same type; update the lookup flow so that
the full pkg-qualified name from resolveEmbeddedTypeName is used to locate the
TypeSpec within the importing file's package/import context (for example by
adding a new finder like findTypeSpecWithPkgContext or extending findTypeSpec to
accept the originating file/package/import map) and use that to resolve the
embedded type; apply the same change to the identical path at the other
occurrence (around lines 350-357) so promotion/inlining always uses the
import-aware lookup rather than the global short-name scan.
- Around line 271-274: The embedded-field branch currently promotes fields
without checking struct tags; before calling e.promoteEmbeddedFields(field,
schema, visitedMap) invoke the same tag-processing used for named fields
(applyTags) on the embedded field and act on its result: if applyTags indicates
the field is skipped (e.g., json:"-") then continue and do not promote; if
applyTags returns an explicit name/rename (e.g., json:"name") treat the field as
a named field instead of promoting it (i.e., skip promotion and let the
named-field path handle it). Ensure you reference the same helpers used
elsewhere (applyTags, promoteEmbeddedFields, visitedMap, schema, field) so
tagged embedded fields are excluded or renamed consistently.
---
Nitpick comments:
In `@internal/extractor/gin/schema_extractor_test.go`:
- Around line 835-851: Consolidate the duplicate helpers by removing
getSchemaPropKeys and using getSchemaKeys everywhere; update all call sites that
currently call getSchemaPropKeys to call getSchemaKeys instead (the functions
operate on the same openapi3.Schemas type), and delete the redundant
getSchemaPropKeys declaration so only getSchemaKeys remains.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: defc8b49-c027-4ae6-a756-dff069fbdb63
⛔ Files ignored due to path filters (1)
integration-tests/gin/fixtures/gin-crosspkg/go.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
cmd/generate.godocs/plans/2026-04-02-gin-extractor-enhancement.mdintegration-tests/gin/fixtures/gin-crosspkg/adapter/project.gointegration-tests/gin/fixtures/gin-crosspkg/adapter/service.gointegration-tests/gin/fixtures/gin-crosspkg/apis/handlers.gointegration-tests/gin/fixtures/gin-crosspkg/go.modintegration-tests/gin/fixtures/gin-crosspkg/main.gointegration-tests/gin/fixtures/gin-crosspkg/models/types.gointegration-tests/gin/fixtures/golden/paths/api-v1-users-get.jsonintegration-tests/gin/fixtures/golden/paths/api-v1-users-id-get.jsonintegration-tests/gin/fixtures/golden/paths/api-v1-users-post.jsonintegration-tests/gin/gin_test.gointernal/extractor/gin/ast_parser.gointernal/extractor/gin/ast_parser_test.gointernal/extractor/gin/generator.gointernal/extractor/gin/handler_analyzer.gointernal/extractor/gin/helper_analyzer.gointernal/extractor/gin/helper_analyzer_test.gointernal/extractor/gin/schema_extractor.gointernal/extractor/gin/schema_extractor_test.gointernal/extractor/types.go
Signed-off-by: spencercjh <spencercjh@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/plans/2026-04-02-gin-extractor-enhancement.md`:
- Around line 56-60: The fenced code blocks showing the handler trace (e.g.,
"handler CreateProject" with "done(c, data, err)" and the "done() calls
c.JSON(200, Rsp{Data: fields[0]})" snippet) are missing language specifiers and
trigger MD040; update those fenced blocks to include an appropriate language
(e.g., ```go or ```text) so they get syntax highlighting; apply the same change
to the other similar block later in the file (the block around the second
occurrence of the handler trace).
- Around line 182-185: The CLI docs show exclusion examples that don't match
actual semantics: update the examples to use exact-path vs prefix flags and the
real config keys—explain that the flag/option exclude-route (and config key
extract.excludeRoutes) performs exact-path matching, while exclude-route-prefix
(or the config key extract.excludeRoutePrefixes) is for prefix-based filtering;
replace `gin.excludePrefixes` with `extract.excludeRoutes` and
`extract.excludeRoutePrefixes` and provide example commands demonstrating both
exact match (e.g., --exclude-route /swagger) and prefix match (e.g.,
--exclude-route-prefix /rjob) consistent with the logic implemented in
internal/config/config.go.
In `@internal/extractor/gin/handler_analyzer.go`:
- Around line 807-809: The type-inference misses header-based parameters because
findContextQueryParam only matches query/path accessors; detect header accessors
(e.g., c.GetHeader("X-Limit") and c.Request.Header.Get("X-Limit")) and feed
their param name into updateParamType. Concretely, extend findContextQueryParam
(or add a new helper like findContextHeaderParam) to recognize
GetHeader/Request.Header.Get call expressions and return the header key, then
call that helper at the same call sites where findContextQueryParam is used (the
invocation preceding updateParamType) so updateParamType can update HeaderParams
for GetHeader-based usages.
- Around line 667-693: traceHelperResponse currently only passes the caller's
varTypeMap into extractTypeFromResponseWithStatus, so helper-local wrapper types
(e.g., a local "rsp" constructed inside helperDecl) are unknown and resolve to
the identifier name; fix by merging the helper's local variable type mappings
into the map used for extraction: after building paramArgMap (using
buildParamArgMap) and before calling extractTypeFromResponseWithStatus,
construct/compute a helperLocalVarMap from helperDecl (scan helperDecl.Body for
local variable declarations/assignments or reuse the existing analyzer logic
that builds var type maps) and merge it into a new combinedVarTypeMap
(combinedVarTypeMap should prefer helper-local types for names declared in
helperDecl and fall back to the caller varTypeMap), then pass combinedVarTypeMap
into extractTypeFromResponseWithStatus so resolveThroughParamMap +
extractTypeFromResponseWithStatus can recognize helper-local wrapper types like
"rsp".
- Around line 736-749: resolveThroughParamMap currently mutates parsed AST nodes
in place (notably *ast.CompositeLit and *ast.KeyValueExpr), which corrupts the
shared helper body across handlers; instead, return new AST nodes and avoid
modifying the original nodes: when you see a *ast.CompositeLit, construct and
return a new &ast.CompositeLit{Type: e.Type, Elts: newElts} where each elt is
the recursively resolved result; when you see a *ast.KeyValueExpr, return a new
&ast.KeyValueExpr{Key: e.Key, Value: resolvedValue}; for *ast.Ident returning
paramToArg[e.Name] return a copy (or a freshly constructed node) rather than the
original if that node can be shared; ensure the default case returns the
original expr unchanged. Update resolveThroughParamMap to build and return new
nodes so HandlerAnalyzer (and its shared helper body) is never modified in
place.
In `@internal/extractor/gin/helper_analyzer.go`:
- Around line 136-156: The extractStatusCode function currently returns 200 for
any unrecognized status expressions, causing unknown http.Status* constants or
local variables to be treated as successful 200 responses; change
extractStatusCode (in helper_analyzer.go) to return a sentinel (e.g., -1) for
unknown/unparsable expressions instead of 200, and update callers such as
extractTypeFromResponseWithStatus to check for this sentinel (status < 0) and
treat it as "unknown" (i.e., do not unwrap as a success payload or assume
success/error branching) so real error-status codes remain on the error-wrapper
path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ff8e63e-6759-4224-a32f-658d2d246908
📒 Files selected for processing (10)
cmd/generate.godocs/plans/2026-04-02-gin-extractor-enhancement.mdinternal/config/config.gointernal/extractor/gin/ast_parser.gointernal/extractor/gin/generator.gointernal/extractor/gin/handler_analyzer.gointernal/extractor/gin/helper_analyzer.gointernal/extractor/gin/helper_analyzer_test.gointernal/extractor/gin/schema_extractor.gointernal/extractor/gin/schema_extractor_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/extractor/gin/helper_analyzer_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/extractor/gin/schema_extractor_test.go
- internal/extractor/gin/schema_extractor.go
- internal/extractor/gin/ast_parser.go
- internal/extractor/gin/generator.go
Signed-off-by: spencercjh <spencercjh@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
internal/extractor/gin/handler_analyzer.go (3)
712-735:⚠️ Potential issue | 🟠 MajorInclude helper-local variables in traced type resolution.
traceHelperResponse()still passes only the caller'svarTypeMapintoextractTypeFromResponseWithStatus(). If the helper doesrsp := Envelope{Data: data}; c.JSON(..., rsp), extraction degrades torspinstead ofEnvelope. Build a second map fromhelperDecl.Bodyand merge it before the type lookup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extractor/gin/handler_analyzer.go` around lines 712 - 735, The current scan uses only the caller's varTypeMap when calling extractTypeFromResponseWithStatus, so helper-local variable types (declared in helperDecl.Body) are not considered; update traceHelperResponse (or the surrounding logic where paramArgMap is built and extractTypeFromResponseWithStatus is invoked) to construct a helper-local var type map by analyzing helperDecl.Body (similar to how varTypeMap is built), merge that helper map with the existing varTypeMap (helper map taking precedence for names local to the helper), and pass the merged map into extractTypeFromResponseWithStatus so responses like rsp := Envelope{Data: data}; c.JSON(..., rsp) resolve to Envelope instead of rsp.
849-851:⚠️ Potential issue | 🟠 MajorHeader params still never participate in the retyping pass.
Both inference sites call
findContextQueryParam(), but that helper only matchesQuery,DefaultQuery, andParam.strconv.Atoi(c.GetHeader("X-Limit"))andc.GetHeader("X-Debug") == "true"therefore stay documented asstring.#!/bin/bash # Expected: inspect any matches; they currently won't be retyped by this pass. python - <<'PY' from pathlib import Path import re patterns = { "strconv+GetHeader": re.compile(r'strconv\.(?:Atoi|ParseInt|ParseBool|ParseFloat)\s*\(\s*.*?\.GetHeader\(', re.S), "bool-compare+GetHeader": re.compile(r'\.GetHeader\([^)]*\)\s*==\s*"(?:true|false)"', re.S), } for path in Path(".").rglob("*.go"): try: text = path.read_text() except Exception: continue for label, pattern in patterns.items(): for match in pattern.finditer(text): line = text.count("\n", 0, match.start()) + 1 print(f"{path}:{line}: {label}") PYAlso applies to: 890-892, 910-927
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extractor/gin/handler_analyzer.go` around lines 849 - 851, The retyping pass misses header-based inferences because findContextQueryParam() only recognizes Query/DefaultQuery/Param; update the analyzer to also detect headers (e.g., c.GetHeader("X-...") and c.Request.Header.Get or Header["X-..."]) and feed those names into updateParamType just like query params. Concretely, extend or add a helper (e.g., findContextHeaderParam) and call it alongside findContextQueryParam in the sites around paramName := a.findContextQueryParam(call.Args[0]) (and similar blocks at the other locations you noted) so that header-derived expressions like strconv.Atoi(c.GetHeader(...)) and c.GetHeader(...) == "true" produce appropriate retyping via updateParamType.
778-791:⚠️ Potential issue | 🔴 CriticalMake parameter substitution a pure deep-clone walk.
resolveThroughParamMap()mutates nodes from the shared parsed helper body and only descends intoCompositeLit/KeyValueExpr. That lets one handler's substitutions leak into later analyses, and wrappers like&datastill won't resolve to the caller argument. Return cloned wrapper nodes and keep descending instead of editingein place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extractor/gin/handler_analyzer.go` around lines 778 - 791, resolveThroughParamMap currently mutates nodes from the parsed helper body and only edits CompositeLit/KeyValueExpr in place, allowing substitutions to leak and failing to resolve wrappers like &data; change it to perform a pure deep-clone walk: when encountering an *ast.Ident that maps in paramToArg, return a cloned copy of the mapped arg (do not overwrite the original Ident), and for composite nodes (e.g. *ast.CompositeLit, *ast.KeyValueExpr, *ast.UnaryExpr for address operators, and other container nodes you use) allocate and return new node instances whose child fields are set to the result of recursively calling resolveThroughParamMap, instead of mutating e in place; ensure you preserve node positions/metadata as needed or zero them if not required.
🧹 Nitpick comments (1)
internal/extractor/gin/ast_parser.go (1)
329-337: Precompute normalized excludes/prefixes once per filtering pass.
shouldExcludeRoutere-normalizesextraExcludesfor every route, which adds repeated allocations and work. Move normalization outside the loop infilterRoutesand pass precomputed values.Also applies to: 360-364
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extractor/gin/ast_parser.go` around lines 329 - 337, The loop in filterRoutes re-normalizes extraExcludes/extraPrefixes per route via shouldExcludeRoute, causing repeated allocations; move normalization out of the loop by computing normalizedExtraExcludes and normalizedExtraPrefixes once at the start of filterRoutes (using the existing normalization helper), then call shouldExcludeRoute with those precomputed normalized slices (or add a new shouldExcludeRouteNormalized that accepts already-normalized values) and remove per-call normalization inside shouldExcludeRoute; make the same change for the other occurrence that currently normalizes in-loop (the block around the other filter usage).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/extractor/gin/ast_parser.go`:
- Around line 373-375: normalizeRoutePath only replaces Gin ':param' syntax via
routeParamReplacer but misses Gin wildcard syntax ('*param'), so filters like
'/files/*path' won't normalize to the same format as convertPathFormat; update
normalizeRoutePath to also replace wildcard segments (e.g., use a second regexp
or extend routeParamReplacer to match `\*([A-Za-z0-9_]+)` and replace with
`{$1}`) so that `*name` is normalized to `{name}`, keeping references to
normalizeRoutePath and routeParamReplacer (or a new routeWildcardReplacer) and
ensuring behavior matches convertPathFormat.
In `@internal/extractor/gin/handler_analyzer.go`:
- Around line 690-698: The current logic returns to a.extractHelperResponse
whenever ident.Name matches the isKnownResponseHelper heuristic (e.g.,
"done"/"respond"/"reply"), preventing lookupHelper from preferring a discovered
helper declaration; change the flow so lookupHelper(a.callerPkg, ident.Name) is
attempted first and, if it finds a helper, call a.traceHelperResponse(helper,
call.Args, info, varTypeMap); only if no helper is found fall back to
isKnownResponseHelper and call a.extractHelperResponse. This keeps
a.lookupHelper, a.traceHelperResponse, a.extractHelperResponse, ident.Name and
call.Args as the referenced symbols to update and ensures wrappers with real
response bodies/statuses are traced from the helper body.
- Around line 43-58: The discoverHelperFuncs loop currently uses
isGinContextType which only recognizes the hardcoded "gin" name; update
discovery to resolve per-file import aliases and pass them to the type check so
imports like "g", "." or other aliases are recognized. Concretely, in
discoverHelperFuncs (and any caller of isGinContextType) build a map of import
aliases from the file's AST (examining file.Imports for alias or default names,
treating dot-imports as matching Context type and resolving package paths for
"github.com/gin-gonic/gin"), then update isGinContextType to accept that alias
map or replace it with a new helper (e.g., isContextTypeWithAliases) that checks
if the parameter type matches any alias (or dot-import) for gin.Context; use the
file-specific alias resolution when deciding to add fn into a.helperFuncs.
In `@internal/extractor/gin/schema_extractor.go`:
- Around line 299-306: The ref built for cross-package embedded types uses the
full qualified name from resolveEmbeddedTypeName (e.g., "models.User") which
yields refs like "#/components/schemas/models.User" that mismatch
fieldToSchemaRef's short-name refs; update the embed-handling in
schema_extractor.go (the block using resolveEmbeddedTypeName and assigning
schema.Properties[parts[0]]) to extract the short type name (substring after the
last '.') from embeddedTypeName before building the ref so it uses
"#/components/schemas/User", matching fieldToSchemaRef and avoiding broken
references.
---
Duplicate comments:
In `@internal/extractor/gin/handler_analyzer.go`:
- Around line 712-735: The current scan uses only the caller's varTypeMap when
calling extractTypeFromResponseWithStatus, so helper-local variable types
(declared in helperDecl.Body) are not considered; update traceHelperResponse (or
the surrounding logic where paramArgMap is built and
extractTypeFromResponseWithStatus is invoked) to construct a helper-local var
type map by analyzing helperDecl.Body (similar to how varTypeMap is built),
merge that helper map with the existing varTypeMap (helper map taking precedence
for names local to the helper), and pass the merged map into
extractTypeFromResponseWithStatus so responses like rsp := Envelope{Data: data};
c.JSON(..., rsp) resolve to Envelope instead of rsp.
- Around line 849-851: The retyping pass misses header-based inferences because
findContextQueryParam() only recognizes Query/DefaultQuery/Param; update the
analyzer to also detect headers (e.g., c.GetHeader("X-...") and
c.Request.Header.Get or Header["X-..."]) and feed those names into
updateParamType just like query params. Concretely, extend or add a helper
(e.g., findContextHeaderParam) and call it alongside findContextQueryParam in
the sites around paramName := a.findContextQueryParam(call.Args[0]) (and similar
blocks at the other locations you noted) so that header-derived expressions like
strconv.Atoi(c.GetHeader(...)) and c.GetHeader(...) == "true" produce
appropriate retyping via updateParamType.
- Around line 778-791: resolveThroughParamMap currently mutates nodes from the
parsed helper body and only edits CompositeLit/KeyValueExpr in place, allowing
substitutions to leak and failing to resolve wrappers like &data; change it to
perform a pure deep-clone walk: when encountering an *ast.Ident that maps in
paramToArg, return a cloned copy of the mapped arg (do not overwrite the
original Ident), and for composite nodes (e.g. *ast.CompositeLit,
*ast.KeyValueExpr, *ast.UnaryExpr for address operators, and other container
nodes you use) allocate and return new node instances whose child fields are set
to the result of recursively calling resolveThroughParamMap, instead of mutating
e in place; ensure you preserve node positions/metadata as needed or zero them
if not required.
---
Nitpick comments:
In `@internal/extractor/gin/ast_parser.go`:
- Around line 329-337: The loop in filterRoutes re-normalizes
extraExcludes/extraPrefixes per route via shouldExcludeRoute, causing repeated
allocations; move normalization out of the loop by computing
normalizedExtraExcludes and normalizedExtraPrefixes once at the start of
filterRoutes (using the existing normalization helper), then call
shouldExcludeRoute with those precomputed normalized slices (or add a new
shouldExcludeRouteNormalized that accepts already-normalized values) and remove
per-call normalization inside shouldExcludeRoute; make the same change for the
other occurrence that currently normalizes in-loop (the block around the other
filter usage).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad3ea46d-f68c-42f7-9b08-8b1cfeb19700
📒 Files selected for processing (3)
internal/extractor/gin/ast_parser.gointernal/extractor/gin/handler_analyzer.gointernal/extractor/gin/schema_extractor.go
Signed-off-by: spencercjh <spencercjh@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
internal/extractor/gin/response_analyzer.go (1)
116-166:⚠️ Potential issue | 🟠 MajorUnknown status expressions default to 200, altering response semantics.
When
extractStatusCodeencounters unrecognizedhttp.Status*constants (e.g.,http.StatusConflict,http.StatusUnprocessableEntity) or local variables, it returns 200. This causesextractTypeFromResponseWithStatusto treat these as success responses and unwrapDatafields instead of returning the wrapper type.♻️ Proposed fix to return sentinel for unknown status
// extractStatusCode extracts HTTP status code from expression. func extractStatusCode(expr ast.Expr) int { // Integer literal if lit, ok := expr.(*ast.BasicLit); ok { if lit.Kind == token.INT { var code int if _, err := fmt.Sscanf(lit.Value, "%d", &code); err == nil { return code } } } // http.StatusOK reference if sel, ok := expr.(*ast.SelectorExpr); ok { if x, ok := sel.X.(*ast.Ident); ok && x.Name == "http" { - return statusCodeFromName(sel.Sel.Name) + code := statusCodeFromName(sel.Sel.Name) + if code != -1 { + return code + } } } - return 200 // Default + return -1 // Unknown } // statusCodeFromName converts http.StatusXxx to code. func statusCodeFromName(name string) int { switch name { // ... existing cases ... } - return 200 + return -1 // Unknown constant }Then in
extractTypeFromResponseInternal, treatstatusCode < 0as "unknown" and skip success/error-specific unwrapping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extractor/gin/response_analyzer.go` around lines 116 - 166, The function extractStatusCode currently returns 200 for any unknown/unsupported expressions which misclassifies responses; modify extractStatusCode to return a sentinel negative value (e.g., -1) for unrecognized expressions (including unknown http.Status* names and non-literal identifiers) and update statusCodeFromName to return that same sentinel for unmatched names; then update extractTypeFromResponseInternal to treat statusCode < 0 as "unknown" and skip the success/error-specific unwrapping logic so wrapper types are preserved for unknown statuses.
🧹 Nitpick comments (3)
internal/extractor/gin/response_analyzer.go (1)
140-166: Consider adding common HTTP status codes.
statusCodeFromNameis missing several frequently-used status codes likeStatusConflict(409),StatusUnprocessableEntity(422),StatusTooManyRequests(429), andStatusGone(410). These will fall through to the default return value.♻️ Suggested additions
case "StatusServiceUnavailable": return 503 + case "StatusConflict": + return 409 + case "StatusGone": + return 410 + case "StatusUnprocessableEntity": + return 422 + case "StatusTooManyRequests": + return 429 + case "StatusMethodNotAllowed": + return 405 + case "StatusNotAcceptable": + return 406 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extractor/gin/response_analyzer.go` around lines 140 - 166, statusCodeFromName currently omits several common HTTP statuses and falls back to 200; add explicit cases in the switch inside statusCodeFromName for the missing symbols: "StatusConflict" => 409, "StatusGone" => 410, "StatusUnprocessableEntity" => 422, and "StatusTooManyRequests" => 429 so they return the correct numeric codes instead of defaulting to 200; keep the existing default return behavior for unknown names.internal/extractor/gin/type_inference.go (2)
112-124: Variable-to-param name matching may produce false positives.The logic assumes the variable name equals the query param name. If a variable named
enabledis compared to"true"but originates fromc.Query("is_enabled"), this won't update the correct param. Conversely, if there's an unrelatedenabledvariable and a query param also namedenabled, it would incorrectly mark that param as boolean.This is a minor concern since it's a best-effort heuristic, but consider adding a comment documenting this limitation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extractor/gin/type_inference.go` around lines 112 - 124, The heuristic in the comparison branch that checks if exprSide is an *ast.Ident and matches ident.Name against info.QueryParams (in the code around exprSide, ident, and info.QueryParams) can produce false positives because variable names may not map to the original query param name; add a concise comment near this block explaining the limitation (that variable-to-param name matching is a best-effort heuristic and can misidentify types if the variable was assigned from a differently-named c.Query/c.Param or is an unrelated local variable) so future readers understand why this approach is used and its potential inaccuracies.
208-237:inferTypeFromVarNamecapitalizes unknown variable names, which may not be valid types.For unknown variable names like
myData, the function returnsMyDatawhich may not correspond to any actual type. This could pollute the type map with non-existent types.♻️ Suggested improvement
func inferTypeFromVarName(varName string) string { if varName == "" { return "" } // Common variable name -> type mappings mappings := map[string]string{ "user": "User", "users": "User", "req": "", "result": "", "data": "", "item": "", "items": "", } if typ, ok := mappings[varName]; ok { return typ } - // Try to capitalize first letter (heuristic) - // e.g., "pageResult" -> "PageResult" - if varName[0] >= 'a' && varName[0] <= 'z' { - return string(varName[0]-'a'+'A') + varName[1:] - } - - return varName + // Don't guess types for unknown variable names + return "" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extractor/gin/type_inference.go` around lines 208 - 237, inferTypeFromVarName currently capitalizes arbitrary unknown variable names (e.g., "myData" -> "MyData"), which can introduce non-existent types; change the logic in inferTypeFromVarName to only return types from the explicit mappings map (user/users -> "User", etc.) and otherwise return an empty string instead of applying the capitalization heuristic; update/remove the heuristic branch that checks varName[0] and ensure callers handle an empty string result, referencing the inferTypeFromVarName function and the mappings variable to locate and modify the behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/extractor/gin/helper_tracer_test.go`:
- Around line 176-182: After locating the handler function with the loop that
assigns to fn (the ast.FuncDecl found by comparing f.Name.Name == "GetUsers"),
add a nil check immediately after the loop in each test
(TestParseHelperCall_DoneWithCompositeLit, TestParseHelperCall_NotHelper,
TestParseHelperCall_MethodCallIgnored) to prevent panics — e.g., if fn == nil
then fail the test with t.Fatalf or t.Fatal and a clear message that the
GetUsers handler was not found so subsequent use of fn is safe.
In `@internal/extractor/gin/helper_tracer.go`:
- Around line 115-161: traceHelperResponse currently only uses the caller's
varTypeMap so helper-local variables (e.g., rsp := Wrapper{...}) are invisible;
fix by collecting helper-local variable type mappings from helperDecl.Body (scan
for short/var assignments and composite literals/typed returns to build a
localVarMap), merge localVarMap with the incoming varTypeMap into a
combinedVarMap (local overrides or vice versa as appropriate), and pass
combinedVarMap into extractTypeFromResponseWithStatus instead of varTypeMap;
keep using paramArgMap for resolving call args (resolveThroughParamMap) and
preserve the existing logic that finds c.JSON/c.XML calls in
traceHelperResponse.
- Around line 192-206: resolveThroughParamMap currently mutates AST nodes in
place (modifying CompositeLit.Elts and KeyValueExpr.Value) which corrupts shared
helper declarations; change it to produce and return new AST nodes instead of
mutating originals: for *ast.CompositeLit, build a new slice of elements by
calling resolveThroughParamMap on each elt and return a new
&ast.CompositeLit{Type: e.Type, Elts: newElts} (preserving other fields), for
*ast.KeyValueExpr return a new &ast.KeyValueExpr{Key: e.Key, Value:
resolvedValue} where resolvedValue is the recursive call result, and for
*ast.Ident continue to return the mapped arg; ensure the function always returns
the newly constructed node when a replacement occurs so the caller receives a
non-mutated AST.
- Around line 104-113: The current branch prioritizes isKnownResponseHelper over
a.lookupHelper causing real helper bodies to be ignored; change the order so you
first call a.lookupHelper(a.callerPkg, ident.Name) and if it returns non-nil
invoke a.traceHelperResponse(helper, call.Args, info, varTypeMap) and return,
otherwise fall back to isKnownResponseHelper to call
a.extractHelperResponse(call.Args, info, varTypeMap); keep the same return
behavior and argument variables (ident.Name, call.Args, info, varTypeMap) when
moving the checks.
In `@internal/extractor/gin/type_inference_test.go`:
- Around line 102-127: The test locates the handler function 'getItems' into the
local variable fn but doesn't verify fn is non-nil before calling
analyzer.AnalyzeHandler(fn); add a nil-check after the loop (e.g., if fn == nil)
and call t.Fatalf with a clear message that the target function wasn't found so
AnalyzeHandler is never invoked with a nil 'fn' and the test fails cleanly.
In `@internal/extractor/gin/type_inference.go`:
- Around line 126-148: findContextQueryParam currently only recognizes Query,
DefaultQuery and Param calls so header parameters like c.GetHeader("X-Limit")
(possibly wrapped in strconv.Atoi) are missed; update findContextQueryParam
(method on HandlerAnalyzer) to also handle the "GetHeader" selector and return
the extracted string literal via extractStringLiteral, and consider renaming the
function to findContextParamName if you want broader semantics; ensure
updateParamType's existing HeaderParams lookup will then be reachable for
header-derived names.
---
Duplicate comments:
In `@internal/extractor/gin/response_analyzer.go`:
- Around line 116-166: The function extractStatusCode currently returns 200 for
any unknown/unsupported expressions which misclassifies responses; modify
extractStatusCode to return a sentinel negative value (e.g., -1) for
unrecognized expressions (including unknown http.Status* names and non-literal
identifiers) and update statusCodeFromName to return that same sentinel for
unmatched names; then update extractTypeFromResponseInternal to treat statusCode
< 0 as "unknown" and skip the success/error-specific unwrapping logic so wrapper
types are preserved for unknown statuses.
---
Nitpick comments:
In `@internal/extractor/gin/response_analyzer.go`:
- Around line 140-166: statusCodeFromName currently omits several common HTTP
statuses and falls back to 200; add explicit cases in the switch inside
statusCodeFromName for the missing symbols: "StatusConflict" => 409,
"StatusGone" => 410, "StatusUnprocessableEntity" => 422, and
"StatusTooManyRequests" => 429 so they return the correct numeric codes instead
of defaulting to 200; keep the existing default return behavior for unknown
names.
In `@internal/extractor/gin/type_inference.go`:
- Around line 112-124: The heuristic in the comparison branch that checks if
exprSide is an *ast.Ident and matches ident.Name against info.QueryParams (in
the code around exprSide, ident, and info.QueryParams) can produce false
positives because variable names may not map to the original query param name;
add a concise comment near this block explaining the limitation (that
variable-to-param name matching is a best-effort heuristic and can misidentify
types if the variable was assigned from a differently-named c.Query/c.Param or
is an unrelated local variable) so future readers understand why this approach
is used and its potential inaccuracies.
- Around line 208-237: inferTypeFromVarName currently capitalizes arbitrary
unknown variable names (e.g., "myData" -> "MyData"), which can introduce
non-existent types; change the logic in inferTypeFromVarName to only return
types from the explicit mappings map (user/users -> "User", etc.) and otherwise
return an empty string instead of applying the capitalization heuristic;
update/remove the heuristic branch that checks varName[0] and ensure callers
handle an empty string result, referencing the inferTypeFromVarName function and
the mappings variable to locate and modify the behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f2b1d68a-111c-4e7c-b889-7877461e5e1f
📒 Files selected for processing (8)
internal/extractor/gin/handler_analyzer.gointernal/extractor/gin/handler_analyzer_test.gointernal/extractor/gin/helper_tracer.gointernal/extractor/gin/helper_tracer_test.gointernal/extractor/gin/response_analyzer.gointernal/extractor/gin/response_analyzer_test.gointernal/extractor/gin/type_inference.gointernal/extractor/gin/type_inference_test.go
💤 Files with no reviewable changes (1)
- internal/extractor/gin/handler_analyzer_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/extractor/gin/response_analyzer_test.go
Comment on lines
+115
to
+161
| // traceHelperResponse traces into a helper function's body to find c.JSON/c.XML | ||
| // response calls and extracts response types by mapping call-site arguments to | ||
| // the helper's parameters. | ||
| func (a *HandlerAnalyzer) traceHelperResponse(helperDecl *ast.FuncDecl, callArgs []ast.Expr, info *HandlerInfo, varTypeMap map[string]string) { | ||
| if helperDecl.Body == nil { | ||
| return | ||
| } | ||
|
|
||
| // Build param-to-argument mapping for the helper function | ||
| // e.g., helper(c *gin.Context, data any, err error) called as helper(c, project, nil) | ||
| // maps: data → project (with type from varTypeMap), err → nil | ||
| paramArgMap := a.buildParamArgMap(helperDecl, callArgs, varTypeMap) | ||
|
|
||
| // Scan helper body for c.JSON/c.XML calls | ||
| ast.Inspect(helperDecl.Body, func(n ast.Node) bool { | ||
| call, ok := n.(*ast.CallExpr) | ||
| if !ok { | ||
| return true | ||
| } | ||
| sel, ok := call.Fun.(*ast.SelectorExpr) | ||
| if !ok { | ||
| return true | ||
| } | ||
| if _, ok = sel.X.(*ast.Ident); !ok { | ||
| return true | ||
| } | ||
|
|
||
| method := sel.Sel.Name | ||
| switch method { | ||
| case "JSON", "XML", "YAML": | ||
| if len(call.Args) >= 2 { | ||
| // Resolve the response argument through param mapping | ||
| resolvedArg := a.resolveThroughParamMap(call.Args[1], paramArgMap) | ||
| statusCode := extractStatusCode(call.Args[0]) | ||
| goType := extractTypeFromResponseWithStatus(resolvedArg, varTypeMap, statusCode) | ||
| if goType != "" { | ||
| info.Responses = append(info.Responses, ResponseInfo{ | ||
| StatusCode: statusCode, | ||
| GoType: goType, | ||
| }) | ||
| slog.Debug("Traced helper response", "helper", helperDecl.Name.Name, "status", statusCode, "type", goType) | ||
| } | ||
| } | ||
| } | ||
| return true | ||
| }) | ||
| } |
There was a problem hiding this comment.
Helper-local variables are invisible to type extraction.
traceHelperResponse only passes the caller's varTypeMap. If the helper declares local variables like rsp := Wrapper{Data: data}; c.JSON(200, rsp), the variable rsp won't be found in varTypeMap, degrading extraction to the identifier name rather than the actual type.
♻️ Proposed fix to merge helper-local types
func (a *HandlerAnalyzer) traceHelperResponse(helperDecl *ast.FuncDecl, callArgs []ast.Expr, info *HandlerInfo, varTypeMap map[string]string) {
if helperDecl.Body == nil {
return
}
+ // Build combined varTypeMap with helper-local variables
+ combinedVarMap := make(map[string]string)
+ for k, v := range varTypeMap {
+ combinedVarMap[k] = v
+ }
+ // Add helper-local variable types (caller types take precedence via param mapping)
+ helperLocalMap := a.buildVarTypeMap(helperDecl.Body)
+ for k, v := range helperLocalMap {
+ if _, exists := combinedVarMap[k]; !exists {
+ combinedVarMap[k] = v
+ }
+ }
// Build param-to-argument mapping for the helper function
paramArgMap := a.buildParamArgMap(helperDecl, callArgs, varTypeMap)Then use combinedVarMap instead of varTypeMap in the extractTypeFromResponseWithStatus call.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/extractor/gin/helper_tracer.go` around lines 115 - 161,
traceHelperResponse currently only uses the caller's varTypeMap so helper-local
variables (e.g., rsp := Wrapper{...}) are invisible; fix by collecting
helper-local variable type mappings from helperDecl.Body (scan for short/var
assignments and composite literals/typed returns to build a localVarMap), merge
localVarMap with the incoming varTypeMap into a combinedVarMap (local overrides
or vice versa as appropriate), and pass combinedVarMap into
extractTypeFromResponseWithStatus instead of varTypeMap; keep using paramArgMap
for resolving call args (resolveThroughParamMap) and preserve the existing logic
that finds c.JSON/c.XML calls in traceHelperResponse.
…priority, type inference - resolveThroughParamMap now returns new AST nodes instead of mutating shared helper declarations, preventing cross-handler corruption - parseHelperCall prefers lookupHelper over isKnownResponseHelper so traced bodies take priority over name-based heuristics - findContextQueryParam recognizes GetHeader for header param type inference - Cross-package embedded type refs strip package prefix to match short-name schema component keys (e.g. models.User → User) - normalizeRoutePath reuses convertPathFormat for wildcard *param handling - Add nil checks in all test handler lookups to prevent silent panics Signed-off-by: spencercjh <spencercjh@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+175
to
+185
| params := helperDecl.Type.Params.List | ||
| for i, param := range params { | ||
| if i >= len(callArgs) { | ||
| break | ||
| } | ||
| // Skip the first param (*gin.Context) — it's always the context | ||
| if i == 0 { | ||
| continue | ||
| } | ||
| for _, name := range param.Names { | ||
| paramArgMap[name.Name] = callArgs[i] |
There was a problem hiding this comment.
buildParamArgMap indexes helperDecl.Type.Params.List by field index (i) and uses that to pick callArgs[i]. This breaks for grouped parameters in a single field (e.g., func helper(c *gin.Context, data, err any)), because both names in that field will be mapped to the same argument, shifting subsequent args and producing incorrect response type resolution. Consider iterating args with a separate parameter-position counter over each individual name (skipping the first *gin.Context param), rather than using the params.List index directly.
Suggested change
| params := helperDecl.Type.Params.List | |
| for i, param := range params { | |
| if i >= len(callArgs) { | |
| break | |
| } | |
| // Skip the first param (*gin.Context) — it's always the context | |
| if i == 0 { | |
| continue | |
| } | |
| for _, name := range param.Names { | |
| paramArgMap[name.Name] = callArgs[i] | |
| argPos := 0 | |
| skippedContext := false | |
| for _, param := range helperDecl.Type.Params.List { | |
| if len(param.Names) == 0 { | |
| if !skippedContext { | |
| skippedContext = true | |
| } else { | |
| argPos++ | |
| } | |
| continue | |
| } | |
| for _, name := range param.Names { | |
| // Skip the first param (*gin.Context) — it's always the context. | |
| if !skippedContext { | |
| skippedContext = true | |
| argPos++ | |
| continue | |
| } | |
| if argPos >= len(callArgs) { | |
| return paramArgMap | |
| } | |
| paramArgMap[name.Name] = callArgs[argPos] | |
| argPos++ |
Comment on lines
+136
to
+142
| sel, ok := call.Fun.(*ast.SelectorExpr) | ||
| if !ok { | ||
| return true | ||
| } | ||
| if _, ok = sel.X.(*ast.Ident); !ok { | ||
| return true | ||
| } |
There was a problem hiding this comment.
traceHelperResponse currently treats any selector call with method JSON/XML/YAML as a Gin response call as long as the receiver is an *ast.Ident. This can misattribute responses if the helper contains other identifiers with JSON/XML/YAML methods. Since the helper’s first parameter is known to be the *gin.Context, it would be more accurate to only match calls where the selector receiver name equals that first parameter name.
Comment on lines
+233
to
+237
| // Phase 1: If we have a package prefix, only search files from that package | ||
| if pkgPrefix != "" { | ||
| for _, file := range e.files { | ||
| if file.Name.Name != pkgPrefix { | ||
| continue |
There was a problem hiding this comment.
findTypeSpec tries to constrain cross-package lookups by comparing the type’s package prefix (e.g., models.User → models) to file.Name.Name. This only works when the imported identifier matches the target package clause; it fails for aliased imports (e.g., alias.User) or packages whose declared name differs from the import name, and then falls back to a global short-name search (potentially reintroducing collisions). Consider resolving pkgPrefix via the importing file’s import table (alias → actual package name) or tracking types by import path/package directory instead of file.Name.Name.
Comment on lines
+596
to
+603
| // Skip common API prefixes | ||
| lower := strings.ToLower(p) | ||
| if lower == "api" || strings.HasPrefix(lower, "v") && len(lower) > 1 { | ||
| // Check if it's a version like v1, v2, v3 | ||
| if lower == "api" || (len(lower) == 2 && lower[1] >= '0' && lower[1] <= '9') { | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
inferTag’s version-prefix skipping only ignores v<single-digit> (len==2). Common patterns like /api/v10/... or /api/v1.2/... will not be treated as version segments and will instead become tags (e.g., V10). Consider expanding the check to skip v followed by one-or-more digits (and possibly v<digits>.<digits>), so tag inference remains stable across versioning schemes.
Comment on lines
+251
to
+275
| // pkgNameForImport resolves an import path to its actual package name by finding | ||
| // parsed files whose directory matches the import path suffix. | ||
| func pkgNameForImport(importPath string, files map[string]*ast.File) string { | ||
| // Extract the directory suffix from the import path | ||
| // e.g., "gin-crosspkg/apis" → "apis" | ||
| lastSlash := strings.LastIndex(importPath, "/") | ||
| if lastSlash == -1 { | ||
| return "" | ||
| } | ||
| dirSuffix := "/" + importPath[lastSlash+1:] + "/" | ||
|
|
||
| for path, file := range files { | ||
| // Check if the file is in a directory matching the import path suffix | ||
| // e.g., "/home/user/project/apis/handlers.go" contains "/apis/" | ||
| lastDirSlash := strings.LastIndex(path, "/") | ||
| if lastDirSlash == -1 { | ||
| continue | ||
| } | ||
| dir := path[:lastDirSlash] | ||
| // Check if the directory ends with the expected suffix | ||
| if strings.HasSuffix(dir, dirSuffix[:len(dirSuffix)-1]) { | ||
| return file.Name.Name | ||
| } | ||
| } | ||
| return "" |
There was a problem hiding this comment.
pkgNameForImport attempts to resolve an aliased import by matching only the last path segment (directory suffix) against parsed file paths and then returns the first match from iterating a map. If multiple parsed directories share that suffix (e.g., multiple /apis folders), the chosen package becomes nondeterministic across runs. Consider using the full import path (module-aware) to locate the correct directory, or deterministically selecting among matches (e.g., by sorting candidate paths).
Comment on lines
+10
to
+20
| // inferParamTypes scans the handler body for strconv conversions and conditional | ||
| // comparisons that reveal the true type of query/path/header parameters previously | ||
| // extracted as string. | ||
| // | ||
| // Detected patterns: | ||
| // - strconv.Atoi(c.Query("offset")) → integer | ||
| // - strconv.ParseInt(c.Query("id"), ...) → integer | ||
| // - strconv.ParseBool(c.Query("verbose")) → boolean | ||
| // - strconv.ParseFloat(c.Query("rate"), ...) → number | ||
| // - c.Query("x") == "true" → boolean | ||
| func (a *HandlerAnalyzer) inferParamTypes(body *ast.BlockStmt, info *HandlerInfo) { |
There was a problem hiding this comment.
PR description mentions parameter type inference via c.GetInt() / c.GetBool() methods, but the implementation in this PR only infers types from strconv conversions and == "true"/"false" comparisons (no GetInt/GetBool handling found in internal/extractor/gin). Either update the description to match the implemented behavior, or add the missing inference patterns so the feature matches the stated scope.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/extractor/gin/schema_extractor.go (1)
376-386:⚠️ Potential issue | 🟡 MinorRequired fields from embedded schema may incorrectly include shadowed properties.
Line 385 unconditionally appends all required field names from the embedded schema, but line 379 skips property promotion for names that already exist in the parent. This mismatch can cause:
- Incorrect required marking: If parent has
"id"(optional) and embedded has"id"(required), the parent's property remains but gets incorrectly marked required.- Duplicate entries: If both schemas have the same required field, the name appears twice in the Required array.
🛠️ Proposed fix to filter required fields
// Promote properties from embedded schema into parent if embeddedSchema.Properties != nil { for name, propRef := range embeddedSchema.Properties { if _, exists := parentSchema.Properties[name]; !exists { parentSchema.Properties[name] = propRef } } } - parentSchema.Required = append(parentSchema.Required, embeddedSchema.Required...) + // Only include required fields that were actually promoted (not shadowed) + for _, req := range embeddedSchema.Required { + if _, existsInParent := parentSchema.Properties[req]; existsInParent { + // Check if this property came from the embedded schema (was promoted) + if _, existedBefore := parentSchema.Properties[req]; !existedBefore { + continue // Skip - parent already had this property + } + } + // Avoid duplicates + alreadyRequired := false + for _, existing := range parentSchema.Required { + if existing == req { + alreadyRequired = true + break + } + } + if !alreadyRequired { + parentSchema.Required = append(parentSchema.Required, req) + } + }A cleaner approach would track which properties were actually promoted:
// Promote properties from embedded schema into parent + promoted := make(map[string]bool) if embeddedSchema.Properties != nil { for name, propRef := range embeddedSchema.Properties { if _, exists := parentSchema.Properties[name]; !exists { parentSchema.Properties[name] = propRef + promoted[name] = true } } } - parentSchema.Required = append(parentSchema.Required, embeddedSchema.Required...) + for _, req := range embeddedSchema.Required { + if promoted[req] { + parentSchema.Required = append(parentSchema.Required, req) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extractor/gin/schema_extractor.go` around lines 376 - 386, The code appends all names from embeddedSchema.Required into parentSchema.Required even when some properties were not promoted or already exist, causing incorrect required markings and duplicates; modify the promotion block that iterates embeddedSchema.Properties to record which property names were actually added (e.g., collect promotedNames when parentSchema.Properties[name] did not exist and you assign propRef) and then only append those promotedNames (filtering out any that already exist in parentSchema.Required to avoid duplicates) to parentSchema.Required; ensure you reference embeddedSchema.Properties, parentSchema.Properties and parentSchema.Required when implementing the fix.
🧹 Nitpick comments (3)
internal/extractor/gin/ast_parser.go (2)
315-325: Consider semantic clarity:/favicon.icois an exact path, not a prefix.This works correctly due to
hasPathSegmentPrefixhandling exact matches, but mixing full paths in a "prefixes" list could be confusing for maintainers. Consider either renaming todefaultExcludePatternsor documenting that the list handles both prefixes and exact paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extractor/gin/ast_parser.go` around lines 315 - 325, Rename or clarify the meaning of defaultExcludePrefixes to avoid confusion between prefixes and exact paths: either rename the symbol (e.g., defaultExcludePatterns or defaultExcludePathsAndPrefixes) and update all references, or keep the name but add a comment noting that hasPathSegmentPrefix handles both prefix and exact path matches; ensure references to defaultExcludePrefixes and the helper hasPathSegmentPrefix are updated/annotated so maintainers understand exact-path entries like "/favicon.ico" are supported.
343-364: Consider hoistingnormalizedExcludescomputation to avoid repeated allocation.The
normalizedExcludesslice is created for every route. When filtering 100 routes with 10 custom excludes, this creates 100 slices. Moving the normalization tofilterRoutesand passing pre-normalized slices would be more efficient.♻️ Proposed optimization
In
filterRoutes, pre-normalize the excludes:func filterRoutes(routes []Route, extraExcludes, extraPrefixes []string) []Route { + normalizedExcludes := make([]string, len(extraExcludes)) + for i, exc := range extraExcludes { + normalizedExcludes[i] = normalizeRoutePath(exc) + } + normalizedPrefixes := make([]string, len(extraPrefixes)) + for i, p := range extraPrefixes { + normalizedPrefixes[i] = normalizeRoutePath(p) + } var filtered []Route for _, route := range routes { - if shouldExcludeRoute(route.FullPath, extraExcludes, extraPrefixes) { + if shouldExcludeRoute(route.FullPath, normalizedExcludes, normalizedPrefixes) {Then update
shouldExcludeRouteto skip redundant normalization of the filter inputs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extractor/gin/ast_parser.go` around lines 343 - 364, The per-route allocation of normalizedExcludes in shouldExcludeRoute causes repeated work; pre-normalize the exclude list once in filterRoutes and change shouldExcludeRoute to accept the already-normalized excludes/prefixes (e.g., update its signature used by filterRoutes and other callers to take normalizedExcludes and normalizedPrefixes or pass normalizedPrefix strings), remove the normalization loop inside shouldExcludeRoute, and update all call sites (notably filterRoutes) to compute normalizedExcludes/normalizedPrefixes via normalizeRoutePath once and pass them in to avoid allocating a new slice per route.internal/extractor/gin/type_inference.go (1)
112-123: Variable-name-to-param-name matching is a heuristic with edge cases.The logic at lines 115-123 assumes that if a variable name matches a query param name and is compared to
"true"/"false", the param should be typed as boolean. This could produce false positives in unusual cases:page := c.Query("page") // Actually numeric if page == "true" { ... } // Bug in user code, but inference marks "page" as booleanSince this is best-effort inference and such patterns are rare, this is acceptable, but consider adding a debug log when this heuristic triggers to aid troubleshooting:
slog.Debug("Inferred boolean from variable comparison heuristic", "var", ident.Name, "param", info.QueryParams[i].Name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extractor/gin/type_inference.go` around lines 112 - 123, The heuristic in the exprSide == *ast.Ident branch (checking ident.Name against info.QueryParams[*].Name) can yield false positives; add an explicit debug log when this heuristic triggers to make such inferences visible for troubleshooting by calling slog.Debug with a clear message like "Inferred boolean from variable comparison heuristic" and include fields for the variable name (ident.Name) and the matched param name (info.QueryParams[i].Name), then keep the existing assignment info.QueryParams[i].GoType = goTypeBoolean and the existing return.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/extractor/gin/helper_tracer.go`:
- Around line 165-189: The buildParamArgMap function incorrectly uses the loop
index i for both params.List and callArgs, causing grouped parameter names
(param.Names) to map to the same callArg; update buildParamArgMap in
HandlerAnalyzer to use a separate argument index (e.g., argIdx) that increments
for each parameter group entry but is advanced per name within that group so
each name in param.Names consumes a distinct callArgs[argIdx], skip the first
context param as before, and stop when argIdx >= len(callArgs); ensure
param.Names are each mapped to the corresponding callArgs element rather than
reusing callArgs[i].
---
Duplicate comments:
In `@internal/extractor/gin/schema_extractor.go`:
- Around line 376-386: The code appends all names from embeddedSchema.Required
into parentSchema.Required even when some properties were not promoted or
already exist, causing incorrect required markings and duplicates; modify the
promotion block that iterates embeddedSchema.Properties to record which property
names were actually added (e.g., collect promotedNames when
parentSchema.Properties[name] did not exist and you assign propRef) and then
only append those promotedNames (filtering out any that already exist in
parentSchema.Required to avoid duplicates) to parentSchema.Required; ensure you
reference embeddedSchema.Properties, parentSchema.Properties and
parentSchema.Required when implementing the fix.
---
Nitpick comments:
In `@internal/extractor/gin/ast_parser.go`:
- Around line 315-325: Rename or clarify the meaning of defaultExcludePrefixes
to avoid confusion between prefixes and exact paths: either rename the symbol
(e.g., defaultExcludePatterns or defaultExcludePathsAndPrefixes) and update all
references, or keep the name but add a comment noting that hasPathSegmentPrefix
handles both prefix and exact path matches; ensure references to
defaultExcludePrefixes and the helper hasPathSegmentPrefix are updated/annotated
so maintainers understand exact-path entries like "/favicon.ico" are supported.
- Around line 343-364: The per-route allocation of normalizedExcludes in
shouldExcludeRoute causes repeated work; pre-normalize the exclude list once in
filterRoutes and change shouldExcludeRoute to accept the already-normalized
excludes/prefixes (e.g., update its signature used by filterRoutes and other
callers to take normalizedExcludes and normalizedPrefixes or pass
normalizedPrefix strings), remove the normalization loop inside
shouldExcludeRoute, and update all call sites (notably filterRoutes) to compute
normalizedExcludes/normalizedPrefixes via normalizeRoutePath once and pass them
in to avoid allocating a new slice per route.
In `@internal/extractor/gin/type_inference.go`:
- Around line 112-123: The heuristic in the exprSide == *ast.Ident branch
(checking ident.Name against info.QueryParams[*].Name) can yield false
positives; add an explicit debug log when this heuristic triggers to make such
inferences visible for troubleshooting by calling slog.Debug with a clear
message like "Inferred boolean from variable comparison heuristic" and include
fields for the variable name (ident.Name) and the matched param name
(info.QueryParams[i].Name), then keep the existing assignment
info.QueryParams[i].GoType = goTypeBoolean and the existing return.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 952bef27-6f7d-42d0-b4d6-c6bb4b08a31c
📒 Files selected for processing (8)
internal/extractor/gin/ast_parser.gointernal/extractor/gin/handler_analyzer_test.gointernal/extractor/gin/helper_tracer.gointernal/extractor/gin/helper_tracer_test.gointernal/extractor/gin/response_analyzer_test.gointernal/extractor/gin/schema_extractor.gointernal/extractor/gin/type_inference.gointernal/extractor/gin/type_inference_test.go
✅ Files skipped from review due to trivial changes (2)
- internal/extractor/gin/type_inference_test.go
- internal/extractor/gin/response_analyzer_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/extractor/gin/handler_analyzer_test.go
Comment on lines
+165
to
+189
| // buildParamArgMap maps helper function parameter names to their call-site arguments. | ||
| // e.g., for helper(c *gin.Context, data any, err error) called as helper(c, project, nil): | ||
| // | ||
| // "data" → *ast.Ident{Name: "project"}, "err" → *ast.Ident{Name: "nil"} | ||
| func (a *HandlerAnalyzer) buildParamArgMap(helperDecl *ast.FuncDecl, callArgs []ast.Expr, _ map[string]string) map[string]ast.Expr { | ||
| paramArgMap := make(map[string]ast.Expr) | ||
| if helperDecl.Type.Params == nil { | ||
| return paramArgMap | ||
| } | ||
|
|
||
| params := helperDecl.Type.Params.List | ||
| for i, param := range params { | ||
| if i >= len(callArgs) { | ||
| break | ||
| } | ||
| // Skip the first param (*gin.Context) — it's always the context | ||
| if i == 0 { | ||
| continue | ||
| } | ||
| for _, name := range param.Names { | ||
| paramArgMap[name.Name] = callArgs[i] | ||
| } | ||
| } | ||
| return paramArgMap | ||
| } |
There was a problem hiding this comment.
Incorrect parameter-to-argument mapping when parameters are grouped.
When Go function parameters are grouped (e.g., func helper(c *gin.Context, data, err any)), the params.List entry at index 1 contains both data and err in its Names slice. The current loop uses i as the index into both params and callArgs, incorrectly mapping both names to the same argument.
For helper(c, proj, nil):
- Expected:
data → proj,err → nil - Actual:
data → proj,err → proj(both map tocallArgs[1])
🐛 Proposed fix to track argument index separately
func (a *HandlerAnalyzer) buildParamArgMap(helperDecl *ast.FuncDecl, callArgs []ast.Expr, _ map[string]string) map[string]ast.Expr {
paramArgMap := make(map[string]ast.Expr)
if helperDecl.Type.Params == nil {
return paramArgMap
}
params := helperDecl.Type.Params.List
- for i, param := range params {
- if i >= len(callArgs) {
- break
- }
- // Skip the first param (*gin.Context) — it's always the context
- if i == 0 {
- continue
- }
- for _, name := range param.Names {
- paramArgMap[name.Name] = callArgs[i]
+ argIdx := 0 // Track actual argument position
+ for _, param := range params {
+ for _, name := range param.Names {
+ if argIdx >= len(callArgs) {
+ return paramArgMap
+ }
+ // Skip the first arg (*gin.Context) — it's always the context
+ if argIdx == 0 {
+ argIdx++
+ continue
+ }
+ paramArgMap[name.Name] = callArgs[argIdx]
+ argIdx++
}
}
return paramArgMap
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/extractor/gin/helper_tracer.go` around lines 165 - 189, The
buildParamArgMap function incorrectly uses the loop index i for both params.List
and callArgs, causing grouped parameter names (param.Names) to map to the same
callArg; update buildParamArgMap in HandlerAnalyzer to use a separate argument
index (e.g., argIdx) that increments for each parameter group entry but is
advanced per name within that group so each name in param.Names consumes a
distinct callArgs[argIdx], skip the first context param as before, and stop when
argIdx >= len(callArgs); ensure param.Names are each mapped to the corresponding
callArgs element rather than reusing callArgs[i].
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
*gin.Context, trace into their bodies to findc.JSON/c.XMLcalls for response type extraction (covers non-standard helpers beyonddone/respond)integer/boolean/numberfromstrconv.Atoi/ParseBool/ParseFloat,c.GetInt()/c.GetBool()methods, and== "true"boolean comparisons/api/v1/projects/{id}→ tagProjects)apis.CreateProject→CreateProject)--exclude-routeand--exclude-route-prefixCLI flags for custom route filteringstringTest plan
go test ./...)make test-e2e)make lint)🤖 Generated with Claude Code