Skip to content

Fix server variables conflict#2045

Closed
acm19 wants to merge 1 commit into
oapi-codegen:mainfrom
acm19:fix-server-variables-conflict
Closed

Fix server variables conflict#2045
acm19 wants to merge 1 commit into
oapi-codegen:mainfrom
acm19:fix-server-variables-conflict

Conversation

@acm19

@acm19 acm19 commented Jul 25, 2025

Copy link
Copy Markdown

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: #2003

@acm19 acm19 requested a review from a team as a code owner July 25, 2025 11:10
@kusari-inspector

kusari-inspector Bot commented Jul 25, 2025

Copy link
Copy Markdown

Kusari Analysis Results

Analysis for commit: 961dc9c, performed at: 2025-08-12T09:17:46Z

@kusari-inspector rerun - Trigger a re-analysis of this PR

@kusari-inspector feedback [your message] - Send feedback to our AI and team


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!

@jamietanna

Copy link
Copy Markdown
Member

Nice, thank you!

Comment thread configuration-schema.json

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that's done. I did leave a typo fix.

@acm19 acm19 force-pushed the fix-server-variables-conflict branch from 524b078 to 4b52cc6 Compare July 25, 2025 18:19
@kusari-inspector

Copy link
Copy Markdown

Kusari PR Analysis rerun based on - 4b52cc6 performed at: 2025-07-25T18:19:32Z - link to updated analysis

@acm19 acm19 force-pushed the fix-server-variables-conflict branch from 4b52cc6 to cd507af Compare July 25, 2025 18:20
@kusari-inspector

Copy link
Copy Markdown

Kusari PR Analysis rerun based on - cd507af performed at: 2025-07-25T18:20:01Z - link to updated analysis

@acm19

acm19 commented Aug 12, 2025

Copy link
Copy Markdown
Author

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
@acm19 acm19 force-pushed the fix-server-variables-conflict branch from cd507af to 961dc9c Compare August 12, 2025 09:17
@kusari-inspector

Copy link
Copy Markdown

Kusari PR Analysis rerun based on - 961dc9c performed at: 2025-08-12T09:17:25Z - link to updated analysis

mromaszewicz added a commit that referenced this pull request May 1, 2026
* 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>
@mromaszewicz

Copy link
Copy Markdown
Member

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 08b30183.

Your diagnosis was correct: an enum value of default collides with the named default-pointer constant. Your PR fixed it by adding an opt-in Enum infix to enum-value identifiers; the merged fix instead renames the default-pointer constant from <Prefix>Default to <Prefix>DefaultValue only when the spec actually contains a colliding enum value, so adopters whose specs compile cleanly under the current codegen see no identifier change at all. Same root cause, different namespace separation — and yours pointed us at the right shape of solution.

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 Valid() method, #2007 default not in enum producing broken Go) that share the same root cause: the server-URLs template re-implemented its own type/enum emission instead of going through the generic codegen pipeline.

Apologies for the long wait. Thanks for the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Client Server URLs: support handling an enum with default as a value clashing with the default value constant

3 participants