Detect partial enum idiom#2418
Conversation
Closes: oapi-codegen#2412 An anyOf/oneOf of a bare primitive and that same primitive constrained by an `enum` ("a value from this enum, or any other value of the same type") previously rendered as a JSON union -- or, for a top-level component, as nothing at all -- discarding the enum's known values. Detect this idiom and instead resolve the schema to the bare primitive (kept as a type alias, so any value stays assignable without a conversion) while still emitting the enum's known values as untyped constants. No distinct enum type and no Valid() method are generated, since any value of the primitive type is legal. Detection is deliberately strict: it fires only on a oneOf/anyOf with exactly two inline primitive members of the same type and format, where exactly one carries an enum. Anything else -- a $ref member, three or more members, mismatched type/format, or allOf -- falls through to the existing union/merge behavior unchanged. Implemented in GenerateGoSchema via untypedEnumUnion(); enum-value construction is factored into buildEnumValues() and shared with the existing enum branch. EnumDefinition gains ConstType and SkipValidate so constants.tmpl can emit primitive-typed constants and skip Valid() for this case. Adds the issue-2412 regression fixture. What's broken: this is the same idiom oapi-codegen#1189 fixed. At the property level, current releases generate a union wrapper (As*/From*/Merge* accessors); collapsing to a plain string removes that API, which is a breaking change for existing users. It is additive only for top-level components (which generate nothing today). For that reason the behavior is not yet gated -- it likely needs an opt-in output-options flag (default off) to preserve the oapi-codegen#1189 union representation, to be revisited. See the discussion on oapi-codegen#2412. The issue-1189 golden file is regenerated to reflect the new output. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@greptileai review |
Greptile SummaryThis PR adds special handling for partial-enum primitive unions in generated Go models. The main changes are:
Confidence Score: 3/5These issues should be fixed before merging.
pkg/codegen/schema.go
|
| Filename | Overview |
|---|---|
| pkg/codegen/schema.go | Adds partial-enum union detection and shared enum-value construction; the detector misses formatted-string and nullable edge cases. |
| pkg/codegen/codegen.go | Uses primitive constant types and skips validation for untyped enums, relying on schema detection to pass only constant-capable Go types. |
| pkg/codegen/templates/constants.tmpl | Renders enum constants with the new constant type and conditional validation method. |
Reviews (1): Last reviewed commit: "Detect partial enum idiom" | Re-trigger Greptile
| if !isPrimitiveOAPISchema(a.Value) || !isPrimitiveOAPISchema(b.Value) { | ||
| return nil, false | ||
| } | ||
| // Same primitive type and format. | ||
| if !slices.Equal(a.Value.Type.Slice(), b.Value.Type.Slice()) || a.Value.Format != b.Value.Format { |
There was a problem hiding this comment.
This detector accepts every type: string member as a primitive, but some string formats resolve to Go types that cannot be constants. For an anyOf like format: byte plus format: byte with an enum value, this branch calls oapiSchemaToGoType and can set the untyped enum's Go type to []byte. GenerateEnums only quotes values when the Go type is exactly string, so the generated constant is typed as a slice and its value is emitted as an unquoted identifier. That generated code cannot compile. The special case should reject string formats whose resolved Go type is not constant-capable, or only enable this path when the resolved Go type stays string.
| // Same primitive type and format. | ||
| if !slices.Equal(a.Value.Type.Slice(), b.Value.Type.Slice()) || a.Value.Format != b.Value.Format { | ||
| return nil, false | ||
| } | ||
| // Exactly one member carries the enum constraint. | ||
| aHasEnum := len(a.Value.Enum) > 0 | ||
| bHasEnum := len(b.Value.Enum) > 0 | ||
| if aHasEnum == bHasEnum { | ||
| return nil, false | ||
| } | ||
| if aHasEnum { | ||
| return a, true | ||
| } | ||
| return b, true |
There was a problem hiding this comment.
This comparison only checks member type, format, and enum presence, so it collapses unions where the bare primitive branch is nullable: true and the enum branch is not. That schema accepts JSON null through the nullable branch, but this path returns the schema resolved from the enum-bearing member only, so the generated type becomes a plain non-nullable primitive. A required field or top-level schema using that union can no longer represent a valid null value. The detector should require matching nullable flags and preserve them, or reject nullable members from this special case.
Closes: #2412
An anyOf/oneOf of a bare primitive and that same primitive constrained by an
enum("a value from this enum, or any other value of the same type") previously rendered as a JSON union -- or, for a top-level component, as nothing at all -- discarding the enum's known values.Detect this idiom and instead resolve the schema to the bare primitive (kept as a type alias, so any value stays assignable without a conversion) while still emitting the enum's known values as untyped constants. No distinct enum type and no Valid() method are generated, since any value of the primitive type is legal.
Detection is deliberately strict: it fires only on a oneOf/anyOf with exactly two inline primitive members of the same type and format, where exactly one carries an enum. Anything else -- a $ref member, three or more members, mismatched type/format, or allOf -- falls through to the existing union/merge behavior unchanged.
Implemented in GenerateGoSchema via untypedEnumUnion(); enum-value construction is factored into buildEnumValues() and shared with the existing enum branch. EnumDefinition gains ConstType and SkipValidate so constants.tmpl can emit primitive-typed constants and skip Valid() for this case. Adds the issue-2412 regression fixture.
What's broken: this is the same idiom #1189 fixed. At the property level, current releases generate a union wrapper (As*/From*/Merge* accessors); collapsing to a plain string removes that API, which is a breaking change for existing users. It is additive only for top-level components (which generate nothing today). For that reason the behavior is not yet gated -- it likely needs an opt-in output-options flag (default off) to preserve the #1189 union representation, to be revisited. See the discussion on #2412. The issue-1189 golden file is regenerated to reflect the new output.