← back to Cli Printing Press
fix(cli): extend flag-identifier dedup to request body fields (#288)
15529383018a26747632e65e65e90803edee2ac5 · 2026-04-25 12:35:14 -0700 · Trevin Chow
Files touched
A internal/generator/body_collision_test.goM internal/generator/flag_collision.go
Diff
commit 15529383018a26747632e65e65e90803edee2ac5
Author: Trevin Chow <trevin@trevinchow.com>
Date: Sat Apr 25 12:35:14 2026 -0700
fix(cli): extend flag-identifier dedup to request body fields (#288)
---
internal/generator/body_collision_test.go | 191 ++++++++++++++++++++++++++++++
internal/generator/flag_collision.go | 105 +++++++++++-----
2 files changed, 266 insertions(+), 30 deletions(-)
diff --git a/internal/generator/body_collision_test.go b/internal/generator/body_collision_test.go
new file mode 100644
index 00000000..67895208
--- /dev/null
+++ b/internal/generator/body_collision_test.go
@@ -0,0 +1,191 @@
+package generator
+
+import (
+ "go/ast"
+ "go/parser"
+ "go/token"
+ "os"
+ "path/filepath"
+ "strings"
+ "testing"
+
+ "github.com/mvanhorn/cli-printing-press/internal/spec"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+// TestGenerateDeduplicatesCamelCollidingBodyFields covers issue #287, the
+// body-field analogue of #275 F-2. Two body fields whose Go identifiers
+// collapse to the same `body<Camel>` after camelization (e.g., `start_time`
+// and `StartTime` both yield `bodyStartTime`) currently produce duplicate
+// `var body<X>` declarations and refuse to compile. The fix mirrors F-2:
+// extend the dedup pass to walk Endpoint.Body and uniquify IdentName when
+// body fields would collide.
+func TestGenerateDeduplicatesCamelCollidingBodyFields(t *testing.T) {
+ t.Parallel()
+
+ apiSpec := minimalSpec("collide-body")
+ apiSpec.Resources["events"] = spec.Resource{
+ Description: "Events",
+ Endpoints: map[string]spec.Endpoint{
+ "create": {
+ Method: "POST",
+ Path: "/events",
+ Description: "Create an event with a custom timestamp",
+ Body: []spec.Param{
+ {Name: "start_time", Type: "string", Description: "Snake-case form"},
+ {Name: "StartTime", Type: "string", Description: "PascalCase form"},
+ },
+ },
+ "get": {
+ Method: "GET",
+ Path: "/events/{id}",
+ Description: "Get one event",
+ },
+ },
+ }
+
+ outputDir := filepath.Join(t.TempDir(), "collide-body-pp-cli")
+ require.NoError(t, New(apiSpec, outputDir).Generate())
+
+ bodyVars, flagBindings := parseBodyDeclarations(t,
+ filepath.Join(outputDir, "internal", "cli", "events_create.go"))
+
+ assertNoDuplicates(t, bodyVars,
+ "each body field must produce a distinct Go identifier")
+ assertNoDuplicates(t, flagBindings,
+ "each body field must register a distinct cobra flag name")
+ require.Len(t, bodyVars, 2,
+ "both body fields must still be represented after dedup")
+}
+
+// TestGenerateRenamesBodyFieldCollidingWithQueryParam guards the cross-
+// namespace cobra flag collision: a body field and a query param can each
+// register a cobra flag with the same name, and cobra rejects the second
+// registration at runtime. The dedup pass must rename one side so the CLI
+// flags stay distinct.
+func TestGenerateRenamesBodyFieldCollidingWithQueryParam(t *testing.T) {
+ t.Parallel()
+
+ apiSpec := minimalSpec("collide-cross")
+ apiSpec.Resources["posts"] = spec.Resource{
+ Description: "Posts",
+ Endpoints: map[string]spec.Endpoint{
+ "create": {
+ Method: "POST",
+ Path: "/posts",
+ Description: "Create a post; the dry-run query param shares a name with a body field",
+ Params: []spec.Param{
+ {Name: "tags", Type: "string", Description: "Query filter for tags"},
+ },
+ Body: []spec.Param{
+ {Name: "tags", Type: "string", Description: "Tags to set on the post"},
+ },
+ },
+ "get": {
+ Method: "GET",
+ Path: "/posts/{id}",
+ Description: "Get one post",
+ },
+ },
+ }
+
+ outputDir := filepath.Join(t.TempDir(), "collide-cross-pp-cli")
+ require.NoError(t, New(apiSpec, outputDir).Generate())
+
+ _, flagBindings := parseBodyDeclarations(t,
+ filepath.Join(outputDir, "internal", "cli", "posts_create.go"))
+
+ assertNoDuplicates(t, flagBindings,
+ "--tags from a body field must not collide with --tags from a query param")
+ assert.Contains(t, flagBindings, "tags",
+ "the first registrant keeps the canonical flag name")
+}
+
+// TestGenerateRenamesBodyFieldCollidingWithStdin guards against a body field
+// literally named `stdin` colliding with the `--stdin` flag the template
+// emits for POST/PUT/PATCH endpoints (command_endpoint.go.tmpl:525).
+func TestGenerateRenamesBodyFieldCollidingWithStdin(t *testing.T) {
+ t.Parallel()
+
+ apiSpec := minimalSpec("collide-stdin")
+ apiSpec.Resources["uploads"] = spec.Resource{
+ Description: "Uploads",
+ Endpoints: map[string]spec.Endpoint{
+ "create": {
+ Method: "POST",
+ Path: "/uploads",
+ Description: "Create an upload",
+ Body: []spec.Param{
+ {Name: "stdin", Type: "string", Description: "A field unfortunately named stdin"},
+ },
+ },
+ "get": {
+ Method: "GET",
+ Path: "/uploads/{id}",
+ Description: "Get one upload",
+ },
+ },
+ }
+
+ outputDir := filepath.Join(t.TempDir(), "collide-stdin-pp-cli")
+ require.NoError(t, New(apiSpec, outputDir).Generate())
+
+ _, flagBindings := parseBodyDeclarations(t,
+ filepath.Join(outputDir, "internal", "cli", "uploads_create.go"))
+
+ assertNoDuplicates(t, flagBindings,
+ "the body field named 'stdin' must not collide with the template's --stdin flag")
+}
+
+// parseBodyDeclarations returns the names of all `var bodyXxx` declarations
+// and the literal cobra flag names registered. Cobra registrations may come
+// from either flag<X> or body<X> Go identifiers, so the flag-binding return
+// covers the full namespace.
+func parseBodyDeclarations(t *testing.T, path string) (vars, bindings []string) {
+ t.Helper()
+ src, err := os.ReadFile(path)
+ require.NoError(t, err, "read generated file")
+
+ fset := token.NewFileSet()
+ file, err := parser.ParseFile(fset, path, src, 0)
+ require.NoError(t, err, "generated file must parse as Go")
+
+ ast.Inspect(file, func(n ast.Node) bool {
+ switch decl := n.(type) {
+ case *ast.GenDecl:
+ if decl.Tok != token.VAR {
+ return true
+ }
+ for _, sp := range decl.Specs {
+ vs, ok := sp.(*ast.ValueSpec)
+ if !ok {
+ continue
+ }
+ for _, name := range vs.Names {
+ // Match body<Suffix> declarations only; the bare `body`
+ // variable is the request-body map the template uses
+ // to assemble the JSON payload, not a per-field var.
+ if len(name.Name) > 4 && strings.HasPrefix(name.Name, "body") {
+ vars = append(vars, name.Name)
+ }
+ }
+ }
+ case *ast.CallExpr:
+ sel, ok := decl.Fun.(*ast.SelectorExpr)
+ if !ok || !strings.HasSuffix(sel.Sel.Name, "Var") {
+ return true
+ }
+ if len(decl.Args) < 2 {
+ return true
+ }
+ lit, ok := decl.Args[1].(*ast.BasicLit)
+ if !ok || lit.Kind != token.STRING {
+ return true
+ }
+ bindings = append(bindings, strings.Trim(lit.Value, `"`))
+ }
+ return true
+ })
+ return vars, bindings
+}
diff --git a/internal/generator/flag_collision.go b/internal/generator/flag_collision.go
index 806dd966..41750aec 100644
--- a/internal/generator/flag_collision.go
+++ b/internal/generator/flag_collision.go
@@ -6,27 +6,29 @@ import (
"github.com/mvanhorn/cli-printing-press/internal/spec"
)
-// dedupeFlagIdentifiers ensures that no two non-positional params on a single
-// endpoint share a Go identifier (flag<Camel>) or cobra flag name after
-// camelization or kebab-casing, and that no param collides with a reserved
-// generator-introduced identifier (pagination's flagAll; async's flagWait,
-// flagWaitTimeout, flagWaitInterval).
+// dedupeFlagIdentifiers ensures that no two non-positional params or body
+// fields on a single endpoint share a Go identifier (flag<Camel> /
+// body<Camel>) or cobra flag name after camelization or kebab-casing, and
+// that no entry collides with a reserved generator-introduced identifier
+// (pagination's flagAll, async's flagWait*, mutating endpoints' --stdin).
//
-// Conflicting params have Name suffixed with _2, _3, ... until the identifier
-// and flag name are both unique. Without this, specs that use date-range
-// filter conventions (e.g., Twilio's StartTime, StartTime>, StartTime< all
-// camelize to "StartTime") or expose a literal "all" parameter on a paginated
-// endpoint (e.g., GitHub notifications colliding with pagination's --all)
-// produce duplicate `var flagX` declarations and refuse to compile.
+// Conflicting entries have IdentName populated to Name with _2, _3, ...
+// suffixed until the Go identifier and cobra flag name are both unique
+// relative to other entries on the same endpoint and to reserved generator
+// names. Without this, specs that use date-range filter conventions (e.g.,
+// Twilio's StartTime, StartTime>, StartTime< all camelize to "StartTime"),
+// expose a literal "all" parameter on a paginated endpoint (e.g., GitHub
+// notifications colliding with pagination's --all), or have query params
+// and body fields that share names produce duplicate `var flagX` / `var
+// bodyX` declarations and refuse to compile, or register the same cobra
+// flag twice and fail at runtime.
func (g *Generator) dedupeFlagIdentifiers() {
if g.Spec == nil {
return
}
for resName, res := range g.Spec.Resources {
for epName, ep := range res.Endpoints {
- idents, flags := reservedFlagNamesForEndpoint(resName, epName, ep, g.AsyncJobs)
- ep.Params = uniquifyParamNames(ep.Params, idents, flags)
- res.Endpoints[epName] = ep
+ res.Endpoints[epName] = dedupeEndpointIdentifiers(resName, epName, ep, g.AsyncJobs)
}
for subName, sub := range res.SubResources {
// Sub-resource async lookups elsewhere in the generator (see
@@ -36,9 +38,7 @@ func (g *Generator) dedupeFlagIdentifiers() {
// correctly. DetectAsyncJobs does not currently walk
// sub-resources, so this lookup is a no-op today.
for epName, ep := range sub.Endpoints {
- idents, flags := reservedFlagNamesForEndpoint(subName, epName, ep, g.AsyncJobs)
- ep.Params = uniquifyParamNames(ep.Params, idents, flags)
- sub.Endpoints[epName] = ep
+ sub.Endpoints[epName] = dedupeEndpointIdentifiers(subName, epName, ep, g.AsyncJobs)
}
res.SubResources[subName] = sub
}
@@ -46,9 +46,40 @@ func (g *Generator) dedupeFlagIdentifiers() {
}
}
-// reservedFlagNamesForEndpoint returns identifiers and flag names that the
-// command templates emit themselves and that user params therefore must not
-// shadow.
+// dedupeEndpointIdentifiers runs the param-then-body uniquification for one
+// endpoint, sharing the cobra flag-name namespace across both passes. Body
+// fields and query/path params each emit `cmd.Flags().*Var(..., flagName, ...)`
+// against the same cobra command, so collisions across the two lists must be
+// detected together.
+func dedupeEndpointIdentifiers(resKey, epName string, ep spec.Endpoint, asyncJobs map[string]AsyncJobInfo) spec.Endpoint {
+ flagIdents, flagNames := reservedFlagNamesForEndpoint(resKey, epName, ep, asyncJobs)
+
+ // Pass 1: query/path params populate the flag<Camel> namespace.
+ ep.Params = uniquifyIdentifiers(ep.Params, "flag", flagIdents, flagNames)
+
+ // Pass 2: body fields populate the body<Camel> namespace, but their cobra
+ // flag names share the namespace with everything we just registered.
+ bodyFlagNames := make(map[string]struct{}, len(flagNames)+len(ep.Params))
+ for k := range flagNames {
+ bodyFlagNames[k] = struct{}{}
+ }
+ for _, p := range ep.Params {
+ if !p.Positional {
+ bodyFlagNames[flagName(paramIdent(p))] = struct{}{}
+ }
+ }
+ ep.Body = uniquifyIdentifiers(ep.Body, "body", nil, bodyFlagNames)
+
+ return ep
+}
+
+// reservedFlagNamesForEndpoint returns identifiers and cobra flag names that
+// the command templates emit themselves and that user params or body fields
+// therefore must not shadow. The returned `idents` set is in the flag<Camel>
+// namespace (params); body<Camel> body-namespace identifiers carry no
+// reserved entries because the generator-introduced helpers (stdinBody) use
+// a different naming pattern. The `flags` set covers cobra flag names, which
+// params and body fields share.
func reservedFlagNamesForEndpoint(resKey, epName string, ep spec.Endpoint, asyncJobs map[string]AsyncJobInfo) (idents, flags map[string]struct{}) {
idents = map[string]struct{}{}
flags = map[string]struct{}{}
@@ -64,17 +95,31 @@ func reservedFlagNamesForEndpoint(resKey, epName string, ep spec.Endpoint, async
flags["wait-timeout"] = struct{}{}
flags["wait-interval"] = struct{}{}
}
+ switch ep.Method {
+ case "POST", "PUT", "PATCH":
+ // command_endpoint.go.tmpl:525 emits cmd.Flags().BoolVar(&stdinBody,
+ // "stdin", ...) for mutating methods. stdinBody as a Go identifier
+ // does not pattern-match flag<X> or body<X>, so no ident reservation
+ // is needed; only the cobra flag name is shared.
+ flags["stdin"] = struct{}{}
+ }
return idents, flags
}
-// uniquifyParamNames returns params with IdentName populated whenever a
-// param's Go identifier or cobra flag name would otherwise collide with
-// another param earlier in the list or with a reserved generator name. The
-// first occurrence of each colliding pattern keeps IdentName empty (templates
-// fall back to Name); subsequent ones get IdentName set to Name with _2, _3,
-// ... appended. Wire-side serialization always reads from Name and is never
-// mutated. Positional params are not flagged and pass through.
-func uniquifyParamNames(params []spec.Param, reservedIdents, reservedFlags map[string]struct{}) []spec.Param {
+// uniquifyIdentifiers returns params with IdentName populated whenever an
+// entry's Go identifier (identPrefix + Camel(.Name)) or cobra flag name would
+// otherwise collide with another entry earlier in the list or with a reserved
+// generator name. The first occurrence of each colliding pattern keeps
+// IdentName empty (templates fall back to Name); subsequent ones get
+// IdentName set to Name with _2, _3, ... appended. Wire-side serialization
+// always reads from Name and is never mutated. Positional params are not
+// flagged and pass through.
+//
+// identPrefix is "flag" for query/path params and "body" for request body
+// fields; the prefix selects the Go-identifier namespace. The cobra flag-name
+// namespace is shared across both prefixes, so callers seed reservedFlags
+// with names already registered by an earlier pass.
+func uniquifyIdentifiers(params []spec.Param, identPrefix string, reservedIdents, reservedFlags map[string]struct{}) []spec.Param {
if len(params) == 0 {
return params
}
@@ -93,7 +138,7 @@ func uniquifyParamNames(params []spec.Param, reservedIdents, reservedFlags map[s
out[i] = p
continue
}
- ident := "flag" + toCamel(p.Name)
+ ident := identPrefix + toCamel(p.Name)
flag := flagName(p.Name)
if _, identTaken := usedIdents[ident]; !identTaken {
if _, flagTaken := usedFlags[flag]; !flagTaken {
@@ -105,7 +150,7 @@ func uniquifyParamNames(params []spec.Param, reservedIdents, reservedFlags map[s
}
for n := 2; ; n++ {
candidate := fmt.Sprintf("%s_%d", p.Name, n)
- ident = "flag" + toCamel(candidate)
+ ident = identPrefix + toCamel(candidate)
flag = flagName(candidate)
_, identTaken := usedIdents[ident]
_, flagTaken := usedFlags[flag]
← ee6bd881 fix(cli): refresh stale catalog entries and reject non-spec
·
back to Cli Printing Press
·
chore(main): release 2.3.3 (#277) 5f68784d →