| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
OpenAPI 3.1 allows `type` to be a list, so a value may be any one of several types. The primitive-type dispatch uses `Types.Is(...)`, which only matches a single-element list, so every multi-type union fell through to `unhandled Schema type` and failed generation. Go has no type matching that constraint, so map a union to `any`, the same permissive mapping a bare `type: "null"` already gets (oapi-codegen#2430), rather than rejecting an otherwise valid spec. The mapping is narrow on purpose, so the existing error still catches malformed input. A list-valued `type` is 3.1-only syntax, so a 3.0 document carrying one keeps failing. Every entry must name a JSON Schema type, so a misspelled one (`type: [strng, number]`) keeps failing too. A union carrying an `enum` is excluded from enum codegen for the reason `type: array` already is: `const X any = ...` is not a valid Go constant. It generates the plain `any` the union maps to.
Greptile SummaryThis PR adds OpenAPI 3.1 multi-type union support by mapping unions to any and passing union member metadata to runtime parameter binders.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (2): Last reviewed commit: "Extend OpenAPI 3.1 union type binding to..." | Re-trigger Greptile |
Sorry, something went wrong.
There was a problem hiding this comment.
@mromaszewicz I'd expect that we'd treat this as a oneOf in this case?
Sorry, something went wrong.
You know, I'm not sure. This isn't like oneOf because that can handle any schema definition, however, this is much more limited, because it's just top level types. I do think any, like in this PR, is a better fit. If someone wants something that is [string, array, number], it's on them to disentangle what this is. any will have the json marshaler parse this as it sees fit, while if we make it something like json.RawMessage, then the user can decide how to parse it, so to me, it's a choice between any and json.RawMessage. I'm leaning towards any. |
Sorry, something went wrong.
|
Thanks for the PR — the core mapping is well-guarded and the writeup is unusually thorough. We ran a deep analysis with Claude on this change (probing generated output for the interaction cases against a compiled runtime), and it surfaced a few issues that make the change incomplete as-is. Findings below, roughly in order of severity. 1. Parameter positions generate code that always fails at request timeA multi-type union in a parameter now generates successfully, but the endpoint is permanently broken: parameters:
- name: id
in: path
required: true
schema:
type: [string, integer]The generated handler binds into an any destination, and runtime.BindStyledParameterWithOptions / BindQueryParameter unconditionally reject that: error binding string parameter: can not bind to destination of type: interface So a path param typed this way 400s on every request, and a query param 400s whenever it's supplied. Before this PR the spec failed loudly at generation time; after it, the failure moves to runtime. Unlike type: "null", a string-or-integer ID is a plausible real spec, and this is the one position where any isn't merely permissive but non-functional. We'd want parameter schemas to either keep erroring at generation, or bind the raw string — not silently generate a dead endpoint. Relatedly, the PR body says generation is exercised across "property, additionalProperties, parameter, request body, response body, array items, and allOf positions", but the committed suite only covers property, additionalProperties, a named component, and the enum interaction. If you tested the other positions locally, could you commit those tests? The parameter case above is exactly the kind of thing that coverage would have caught. 2. type list + oneOf/anyOf siblings: the composition is silently discardedUnionWithOneOf:
type: [string, number]
oneOf:
- type: string
- type: numbergenerates type UnionWithOneOf = any with no union accessors, because generateUnion only runs inside the object/typeless branch of GenerateGoSchema. Not a regression (this errored before), but it's a legal 3.1 conjunction that now silently loses the As…/From… machinery a bare oneOf gets. At minimum this deserves a line in the README note. 3. allOf ordering now swallows unions silentlyAllOfUnionMember:
allOf:
- properties:
name:
type: string
- type: [string, number]generates a plain struct — the union member's type vanishes, because MergeSchemas does result.Type = s1.Type and discards s2's type when s1 has none. Reverse the member order and you get any with the properties dropped instead. The order-dependency is a pre-existing MergeSchemas bug (single types behave the same on main), so it's not this PR's fault — but the PR converts some of these from hard errors into silent, order-dependent output. Worth either a guard or a tracking issue. Confirmed as fine
Overall: the schema-position mapping looks merge-worthy, but we'd like the parameter story resolved (and the position tests committed) before this lands. |
Sorry, something went wrong.
|
Update on the review findings above: the runtime side of finding 1 has shipped. runtime v1.7.0 adds a Types []string field to BindStyledParameterOptions / BindQueryParameterOptions, consulted only when the destination is an any: the value binds to the first union member that parses, in specificity order (boolean, integer, number, string) with JSON-grammar numeric detection, so union-typed parameters can now bind correctly instead of 400ing on every request. What that leaves on the codegen side to complete the parameter story here:
We're happy to either take these as a follow-up PR on our side, or you're welcome to fold them into this one — whichever you prefer. If you'd rather keep this PR at its current scope, we'll handle the plumbing after merge so both land in the same release. On the remaining findings: having dug further, finding 2 (type list + oneOf/anyOf siblings) turns out to be mostly theoretical — in any coherent spec the sibling type list is redundant with the branch types (validators intersect; linters flag it), disagreement between them makes members unsatisfiable, and every such spec fails generation on main today anyway, so there's nobody to regress. We're treating it as a non-issue. Finding 3 (the MergeSchemas order dependency) is pre-existing and we'll take care of it, along with the strict-server any-response compile issue, on our side — none of that needs to block this PR. |
Sorry, something went wrong.
A union-typed parameter (type: [string, integer]) lowers to `any`, which
the runtime's destination-driven binder rejected on every request, so
generation succeeded but the endpoint never worked. runtime v1.7.0 added a
Types option carrying the union's member list for exactly this case: the
value binds to the first member that parses (boolean, integer, number,
string — JSON number grammar), only when the destination is an `any`.
Codegen now emits that option. ParameterDefinition and
ResponseHeaderDefinition gain SchemaTypes(), returning the member list
with the "null" nullability marker stripped for genuine unions and nil
otherwise; all 31 BindStyledParameterOptions/BindQueryParameterOptions
literals across the stdlib, echo, gin, fiber and iris templates populate
Types unconditionally (via the existing toStringArray helper), rendering
[]string{} for single-type parameters. Generated code therefore requires
runtime >= v1.7.0 from the next release on — a deliberate choice over a
conditional-emission shim, which added template complexity for a
compatibility window we don't intend to support.
SchemaType() previously returned the first entry of the type list
verbatim: "string" for [string, integer], and "null" for
["null", "string"]. It now strips the null marker and returns "" for
unions. Type is unread by every released runtime, so no generated
behavior changes.
ZeroValueIsNil() now reports true for `any` parameters, so the generated
client wraps optional union parameters in the existing nil check instead
of passing a nil interface to StyleParamWithOptions, which panics. This
also fixes the same latent panic for optional typeless (schema: {}) and
type: "null" parameters.
The new internal/test/openapi31params suite exercises the full loop —
generated std-http server and client, path/query/header positions, a
nullable union, member selection, and typed round-trips — against
runtime v1.7.0. The sibling openapi31 suite stays models-only per its
charter. internal/test and examples bump runtime v1.5.0 -> v1.7.0, and
every committed .gen.go with bind calls regenerates to carry the Types
field.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Sorry, something went wrong.
|
Ok, @bendrucker , what do you think of this change. I added a commit which makes it much larger, since I had to change every template to pass through the OpenAPI 3.1 types array to the runtime package. |
Sorry, something went wrong.
The previous commit populated Types unconditionally in every generated bind call, which regenerated 54 committed .gen.go files and made all generated code — including code from plain 3.0 specs — require runtime v1.7.0. Emit the field only for multi-type union parameters instead, via the unionTypes template helper (rendering through the existing toStringArray). Single-type parameters emit no Types field at all, so code generated from specs without union parameters is byte-identical to previous releases and keeps compiling against older runtimes; only specs that actually use union parameters pick up the v1.7.0 requirement, and every previously-churned .gen.go reverts to its committed state. The examples and internal/test modules both build against runtime v1.7.0 for consistency — that is a choice of what we test against, not a requirement the generated code imposes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Just getting back to this, definitely didn't anticipate those edge cases, thanks for finishing this one for me! Will take a look back through and see if there are any follow ups, might tackle #2525 as well. |
Sorry, something went wrong.
|
Revising my earlier comment: #2525 looks like the same bug as #1328, and #2514 already fixes it with the struct wrapper suggested in the issue. I reviewed that branch merged onto current main and verified it handles every shape from #2525, including the 3.1 unions from this PR. Details on the issue. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
OpenAPI 3.1 allows type to be a list, so a value may be any one of several types. The primitive-type dispatch in pkg/codegen/schema.go uses Types.Is(...), which only matches a single-element list, so every multi-type union fell through to unhandled Schema type and failed generation. Closes #2521.
Go has no type meaning "one of these", so a union maps to any. That follows the bare type: "null" branch directly above it, added for #2430, on the same reasoning: prefer the permissive mapping over rejecting an otherwise valid spec.
The mapping is deliberately narrow so the existing error keeps catching malformed input instead of quietly widening it. #1977 asks for a better error here, and swallowing more cases would be the wrong direction. A list-valued type is 3.1-only syntax, so a 3.0 document carrying one still fails. Every entry must name a JSON Schema type, so type: [strng, number] still fails on the typo.
One interaction needed handling. A union carrying an enum reached the enum branch, which emits const X T = ... against a type that is now any. That is not a valid Go constant, so generation succeeded but the output did not compile. Unions are now excluded from enum codegen for the reason type: array already is, and generate the plain any instead.
Nullable objects are unaffected. schemaPrimaryType strips "null" before the dispatch, so type: [object, "null"] still generates its struct. type: [object, string] does collapse to any and drops its declared properties. That is inherent to the mapping, since no Go type is either a struct or a string.
Generation is exercised across property, additionalProperties, parameter, request body, response body, array items, and allOf positions. One rough edge worth naming: a response body typed any under strict-server emits a method on a named interface type, which does not compile. That reproduces on main today for both schema: {} and type: "null", so this PR routes one more spec shape into it rather than causing it. Happy to fix that separately if you'd like it tracked.
For motivation, Honeycomb's published 3.1 spec carries two of these for genuinely polymorphic event and query-result values. The only workaround today is an overlay that strips the type keyword before generation.