GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||
GeezerVison is showing github.com — Register handlers in spec order by mromaszewicz · Pull Request #2465 · oapi-codegen/oapi-codegen Conversation
Member
Closes: #1887 Route registration was emitted in sorted path order, so reordering paths in the spec had no effect. On routers that match in registration order (Fiber, Gorilla/mux) this left users no way to disambiguate overlapping paths — e.g. a request for kin-openapi stores paths in a map, losing declaration order at parse time, so enable its origin tracking ( Model and interface generation are intentionally left in the old sorted order to preserve any existing ordering behavior — in particular the order-dependent fallback in name-collision resolution. Only handler registration follows spec order.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Closes: oapi-codegen#1887 Route registration was emitted in sorted path order, so reordering paths in the spec had no effect. On routers that match in registration order (Fiber, Gorilla/mux) this left users no way to disambiguate overlapping paths — e.g. a request for `/templates/privates/shortcuts` always went to `/templates/privates/{id}` rather than `/templates/{visibility}/shortcuts`. kin-openapi stores paths in a map, losing declaration order at parse time, so enable its origin tracking (`Loader.IncludeOrigin`) to recover each path's source line onto the OperationDefinition (SpecOrder). Each GenerateXServer now renders its registration template with operations sorted by SpecOrder, via a new GenerateTemplatesIntoBuffer that composes passes into one buffer (GenerateTemplates wraps it). Model and interface generation are intentionally left in the old sorted order to preserve any existing ordering behavior — in particular the order-dependent fallback in name-collision resolution. Only handler registration follows spec order. Co-Authored-By: Claude Fable 5 <[email protected]>
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes. --- ### Issue 1 of 2 pkg/codegen/operations.go:1069-1074 **Misplaced godoc comment** The line `// OperationDefinitions returns all operations for a swagger definition.` (originally the godoc for `OperationDefinitions`) is now the *first line* of `pathItemSourceLine`'s doc comment block. As a result, `go doc` will show `pathItemSourceLine` as starting with "OperationDefinitions returns all operations for a swagger definition.", and `OperationDefinitions` itself (line 1081) has no godoc at all. The `OperationDefinitions` comment line should be moved back to immediately precede `OperationDefinitions`, with a blank line separating the two function doc blocks. ### Issue 2 of 2 pkg/codegen/operations.go:1917-1924 **Unconditional behavior change — consider a compat flag** Route-registration order now follows spec declaration order for all backends and all users, with no opt-out. The project's stated practice is that behavior-changing features be opt-in via a configuration flag (`compatibility` or `output-options`) rather than silent breaking changes. Users on order-dependent routers (Fiber, Gorilla/mux) who previously worked around the alphabetical ordering by arranging their spec paths alphabetically will silently get different runtime routing on the next code-gen run. It is worth asking whether this should be gated behind a flag (e.g. `compatibility.register-routes-in-spec-order`) so the change is opt-in rather than unconditional. Reviews (1): Last reviewed commit: "Register handlers in spec order" | Re-trigger Greptile All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page.
|
Uh oh!
There was an error while loading. Please reload this page.
|
IMO, I think this may be an option we want a compatibility config option for @mromaszewicz, especially if it changes intent for everyone's specs |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
This only affects fiber and gorilla, and only in the case where a parameterized shallow path comes before a deeper path. For example, if you have:
The first is going to shadow the second. In every other router, path registration is order independent, so it's a no-op change, just a bit of generated code churn. I think I want this default-on, but I can add an opt out flag (even though spec editing can accomplish the same thing) |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
…tion PR oapi-codegen#2465 (issue oapi-codegen#1887) changed generated servers to register route handlers in spec-declaration order, so that routers which match in registration order (Fiber, Gorilla/mux) can disambiguate overlapping paths by ordering them in the spec. This makes that behavior opt-out: the new sort-handler-registrations compatibility flag restores the historical lexicographic (by path, then method) registration order for users who relied on it. The dispatch is a single seam -- operationsInRegistrationOrder picks sortOperationsLexicographically when the flag is set, otherwise the existing sortOperationsBySpecOrder -- and all nine GenerateXServer functions route through it. The lexicographic sort reproduces the historical gather order (SortedMapKeys over paths then methods), so with the flag unset the default output is unchanged. Adds TestSortHandlerRegistrations and documents the flag in configuration-schema.json. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…tion (#2477) PR #2465 (issue #1887) changed generated servers to register route handlers in spec-declaration order, so that routers which match in registration order (Fiber, Gorilla/mux) can disambiguate overlapping paths by ordering them in the spec. This makes that behavior opt-out: the new sort-handler-registrations compatibility flag restores the historical lexicographic (by path, then method) registration order for users who relied on it. The dispatch is a single seam -- operationsInRegistrationOrder picks sortOperationsLexicographically when the flag is set, otherwise the existing sortOperationsBySpecOrder -- and all nine GenerateXServer functions route through it. The lexicographic sort reproduces the historical gather order (SortedMapKeys over paths then methods), so with the flag unset the default output is unchanged. Adds TestSortHandlerRegistrations and documents the flag in configuration-schema.json. Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
…ons (#2601) * fix: ignore kin-openapi source locations when comparing type definitions Since #2465 the loader sets IncludeOrigin, so every schema records the line it was declared on. TypeDefinitionsEquivalent compared schemas with reflect.DeepEqual, so two identical schemas with the same x-go-type-name on different lines no longer matched, and generation failed with "duplicate typename". Compare the schemas like reflect.DeepEqual does, but treat any two *openapi3.Origin values as equal. Comparing the JSON encodings instead would drop resolved $ref targets and OpenAPI 3.1 $ref siblings, so different schemas could be merged silently. Fixes #2599 Co-authored-by: Claude Opus 5.5 <[email protected]> Co-authored-by: Marcin Romaszewicz <[email protected]>
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Re-order generated routesUh oh!
There was an error while loading. Please reload this page.
2 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.