Fix server variables conflict#2045
Conversation
Kusari Analysis ResultsAnalysis for commit: 961dc9c, performed at: 2025-08-12T09:17:46Z • • Recommendation✅ PROCEED with this Pull Request Summary✅ No Flagged Issues Detected All values appear to be within acceptable risk parameters. No pinned version dependency changes, code issues or exposed secrets detected! Found this helpful? Give it a 👍 or 👎 reaction! |
|
Nice, thank you! |
There was a problem hiding this comment.
Can we please revert the unrelated changes? Although those full stops at the end may be reasonable, I'd prefer to remove them from this PR to keep it atomic. Thanks!
There was a problem hiding this comment.
Sure, that's done. I did leave a typo fix.
524b078 to
4b52cc6
Compare
|
Kusari PR Analysis rerun based on - 4b52cc6 performed at: 2025-07-25T18:19:32Z - link to updated analysis |
4b52cc6 to
cd507af
Compare
|
Kusari PR Analysis rerun based on - cd507af performed at: 2025-07-25T18:20:01Z - link to updated analysis |
|
Hey @jamietanna, bumping this in your inbox so it doesn't get forgotten. I've replied to all your comments so far. |
Adds a property to opt-in avoiding the default value colliding with the value `default` for enum server variables. When set to true `Enum` is added to the variable name so that it doesn't clash with the default variable name. This behaviour is opt-in to avoid backward compatibility, but the default is actually a bug, so it make sense to change that default in the next major release. Resolves: oapi-codegen#2003
cd507af to
961dc9c
Compare
|
Kusari PR Analysis rerun based on - 961dc9c performed at: 2025-08-12T09:17:25Z - link to updated analysis |
* Route server enums through general enums codegen
The server-URL codegen feature (`generate.server-urls: true`) re-implemented
its own enum emission inside server-urls.tmpl rather than going through the
generic GenerateEnums / GenerateTypes pipeline used by every other
enum-bearing schema in the codebase. That self-contained path quietly
accumulated five distinct bugs:
* Two `const ...VariableDefault` declarations whenever an enum value's
`ucFirst` form was the literal string `Default` (e.g. `enum: [default]`),
producing uncompilable Go (#2003).
* No `Valid()` method on enum-typed variables, with a literal "TODO ...
will validate that the value is part of the ... enum" comment emitted
in the generated function body (#2006).
* Variables declared in `variables:` but absent from the URL still produced
a type, constant, function parameter, and a no-op `strings.ReplaceAll`
call (#2004).
* `{name}` placeholders in the URL with no entry in `variables:` were left
in the URL after substitution and tripped the trailing `{`/`}` runtime
check on every call, making the generated function permanently unusable
(#2005).
* When `default` was set to a value not in `enum` (which OpenAPI 3 declares
invalid), the template emitted `const FooDefault Foo = FooDevelopment`
where `FooDevelopment` was never declared, surfacing the spec error as a
confusing Go compile failure (#2007).
All five share one root cause: server-URL variables didn't reach the type
and enum codegen passes that already exist for every other schema.
Changes:
- Add `ForceEnumPrefix` to TypeDefinition. GenerateEnums OR's it into
PrefixTypeName so synthesized server-URL enum types keep the existing
always-prefixed identifier shape (`<Prefix><Value>` rather than the bare
`<Value>` the generic path would otherwise emit when no conflict is
detected).
- Add BuildServerURLTypeDefinitions in pkg/codegen/server_urls.go. It
extracts enum-typed used variables from spec.Servers, synthesizes a
TypeDefinition per variable, and validates `default ∈ enum` at codegen
time so #2007 spec violations are caught with a clear error rather than
emitted as broken Go.
- Wire BuildServerURLTypeDefinitions into the `generate.server-urls` gate
so the synthesized types flow through GenerateTypes (typedef.tmpl) and
GenerateEnums (constants.tmpl) regardless of `generate.models`. This
gives server-URL enum variables the same `type X string`, `const ( ... )`
block, and `Valid()` method as any other enum-bearing schema.
- Drop type/const emission for enum-typed variables from server-urls.tmpl;
it now emits only the per-server function (which calls `.Valid()` on
each enum-typed parameter) and any non-enum variable scaffolding.
- Add ServerObjectDefinition.UsedVariables and UndeclaredPlaceholders
helpers + extract serverObjectDefinitions, the name-deconfliction pass
shared by both GenerateServerURLs and BuildServerURLTypeDefinitions, so
the type pipeline and the function-body pipeline see the same
identifiers.
- Extend genServerURLWithVariablesFunctionParams to take undeclared
placeholders, emitting them as plain `string` parameters alongside the
declared variables (sorted together for stable output).
User-visible naming change: for enum-typed server-URL variables only, the
default-pointer constant is renamed from `<Prefix>Default` to
`<Prefix>DefaultValue`. This is what makes the #2003 collision impossible
permanently — the enum-value namespace and the default-pointer namespace
are distinct regardless of what string values appear in the spec. Non-enum
variables keep their existing `<Prefix>Default` naming. Enum-value
identifiers also pick up an `N` prefix for digit-leading values (e.g. an
enum value `8443` becomes `<Prefix>N8443`) since they now flow through
SchemaNameToTypeName, matching how every other enum in the codegen names
digit-leading values.
The example fixture at examples/generate/serverurls/api.yaml gains two new
servers: one with `enum: [default, 443]` to lock in the #2003 fix
end-to-end, and one with an undeclared `{tenant}` placeholder for #2005.
The existing `noDefault: {}` declared-but-unused variable now demonstrates
the #2004 filtering. Three previously-identical localhost URLs are given
unique ports (80/81/82) to comply with OpenAPI's URL-uniqueness
requirement.
Closes #2003
Closes #2004
Closes #2005
Closes #2006
Closes #2007
Supersedes #2045
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Address PR #2358 review: minimize identifier churn
Greptile's review flagged that the original PR shipped several
unconditional behaviour changes to the generated API surface, none of
which were intrinsic to the bug fixes:
* `genServerURLWithVariablesFunctionParams` gained a third argument,
breaking any user-supplied custom `server-urls.tmpl` override that
called it with the previous two-argument form.
* Digit-leading enum values acquired an `N` prefix (e.g. `Variable8443`
became `VariableN8443`), an incidental side-effect of routing through
`SchemaNameToTypeName` even though the enclosing type prefix already
provided a leading letter.
* The default-pointer constant was unconditionally renamed from
`<Prefix>Default` to `<Prefix>DefaultValue` for every enum-typed
variable, breaking adopters whose specs had no collision and whose
code compiled cleanly under the old codegen.
Greptile also identified a latent bug: when two enum values fold to the
same identifier suffix (e.g. `enum: ["foo", "Foo"]`, both `ucFirst` to
`Foo`), `SanitizeEnumNames`' numeric-suffix dedup produced
`<Prefix>Foo` and `<Prefix>Foo1`, but the template's default-pointer
emitted `{{ $v.Default | schemaNameToTypeName | sanitizeGoIdentity }}`
without the suffix, so the pointer referenced an undeclared identifier.
Changes:
- Revert `genServerURLWithVariablesFunctionParams` to its original
two-argument signature. A new `ServerObjectDefinition.NewServerFunctionParams`
method now returns the full parameter list (typed declared variables
plus plain-`string` undeclared placeholders, sorted together
alphabetically), and the template calls `{{ .NewServerFunctionParams }}`
instead of the helper. Any user-supplied custom template that calls
`genServerURLWithVariablesFunctionParams` directly keeps working.
- Replace `SanitizeEnumNames` (which forces digit-leading values
through `SchemaNameToTypeName` and adds the `N` prefix) with a small
in-package helper `serverURLEnumKeys` that uses
`UppercaseFirstCharacter` directly, plus the same
`<key>`/`<key>1`/`<key>2` numeric-suffix dedup. Digit-leading values
stay digit-leading; identifiers for non-colliding specs are byte-for-byte
identical to what the pre-PR template produced.
- Make the `<Prefix>Default` to `<Prefix>DefaultValue` rename
asymmetric. A new `ServerObjectDefinition.EnumDefaultPointers` method
pre-computes each enum-typed variable's default-pointer info; it
switches to `<Prefix>DefaultValue` only when an enum value's
identifier suffix is the literal string "Default" (i.e. exactly the
spec pattern that produced #2003's duplicate-const compile error
before this fix). Adopters whose specs compiled cleanly under the
old codegen keep their `<Prefix>Default` constant.
- The same `EnumDefaultPointers` data carries the post-dedup target
identifier, so the template emits
`const <Prefix>Default <Type> = <Prefix>Foo1` rather than recomputing
via `schemaNameToTypeName | sanitizeGoIdentity`. The pointer always
agrees with whatever name the const-block actually produced.
- Drop the `schemaNameToTypeName` and `sanitizeGoIdentity` calls from
`server-urls.tmpl` accordingly. The template now iterates pre-computed
`.EnumDefaultPointers` and `.NewServerFunctionParams` rather than
reconstructing identifier names itself.
Test coverage:
- `pkg/codegen/server_urls_test.go` gains four `TestEnumDefaultPointers`
subtests pinning the asymmetric-rename behaviour, the post-dedup
reference, and the absence of the `N` prefix.
- `examples/generate/serverurls/api.yaml` adds a server with
`enum: ["foo", "Foo"]` exercising the dedup case end-to-end.
- `examples/generate/serverurls/gen_test.go` gains
`TestServerUrlCaseOnlyEnumCollision` confirming the generated code
compiles, the post-dedup constants are distinct, and the
default-pointer references the right one.
Net effect for adopters of the original `fix/enums` PR vs. `main`:
* Helper signature unchanged.
* Enum value identifiers unchanged for non-colliding specs.
* Default-pointer constant name unchanged for non-colliding specs.
* `noDefault`-style declared-but-unused parameters are still
dropped (the #2004 fix; the parameter was a no-op).
* Function calls `.Valid()` on enum-typed parameters (the #2006 fix).
* Specs that previously failed to compile (#2003 collision,
case-only-different enum values) now compile.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Thank you for submitting this PR, @acm19 — but we need to close it because the underlying bug (#2003) was just fixed via a different approach in #2358, merged as commit Your diagnosis was correct: an enum value of The merged PR also picks up four other bugs in the server-URLs feature (#2004 declared-but-unused variables generating dead parameters, #2005 undeclared placeholders making the function permanently error, #2006 missing Apologies for the long wait. Thanks for the contribution. |
Adds a property to opt-in avoiding the default value colliding with the value
defaultfor enum server variables. When set to trueEnumis added to the variable name so that it doesn't clash with the default variable name. This behaviour is opt-in to avoid backward compatibility, but the default is actually a bug, so it make sense to change that default in the next major release.Resolves: #2003