Fix allOf/nullable detection and recursion#2473
Merged
Merged
Conversation
Closes: oapi-codegen#1898 The OpenAPI 3.0 idiom for making a $ref nullable is to wrap it in an allOf alongside an inline `{nullable: true}`, since 3.0 forbids siblings next to $ref. This failed generation with "error merging schemas for AllOf: merging two schemas with different Nullable". Two problems: 1. mergeOpenapiSchemas errored whenever two allOf members disagreed on nullability. kin-openapi models Nullable as a plain bool, so an unset value is indistinguishable from an explicit `nullable: false`, which made this widely-used idiom unusable. Union the flag instead (if any member is nullable, the merged schema is nullable) — consistent with how Required is already merged. 2. schemaIsNullable only read the schema's own Nullable flag. For the allOf idiom the flag lives on a member, so the field was generated as non-nullable (no pointer / nullable.Nullable[T] wrapping). Descend transitively into allOf members, matching the transitive allOf flattening in mergeOpenapiSchemas, with a lazily-allocated cycle guard so a self-referential allOf cannot cause unbounded recursion. Adds unit tests for the nullable union and an end-to-end fixture in internal/test/schemas/nullable exercising the decorated-$ref idiom under nullable-type:true for both required and optional properties. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Greptile SummaryThis PR fixes the OpenAPI 3.0 pattern of making a
Confidence Score: 5/5Safe to merge. Both the merge logic and the nullability detection are correct, the cycle guard in schemaIsNullableRec is sound, and the end-to-end fixture confirms the generated types are right. Both code paths are logically correct: nil-map reads in Go are safe (returning the zero value), the lazily-allocated seen map is shared across recursive frames via Go reference semantics, and unioning nullability rather than requiring equality is the right semantic for the allOf decorator idiom. The regression test covers required/optional variants and JSON marshalling round-trips. Generated output shows only the expected new types with no unrelated drift. No files require special attention.
|
| Filename | Overview |
|---|---|
| pkg/codegen/merge_schemas.go | Replaces the error-on-mismatch nullability check with a union: if any allOf member is nullable, the merged schema is nullable. Logically correct and consistent with how Required is already merged. |
| pkg/codegen/schema.go | Adds schemaIsNullableRec that descends transitively into allOf members with a lazily-allocated pointer-keyed cycle guard. Logic is correct: nil-map reads in Go return the zero value safely, and the seen map is shared across recursive calls via reference semantics. |
| pkg/codegen/merge_schemas_test.go | Adds four table-driven sub-tests covering nullable union: one/both/neither nullable combinations, all passing. |
| internal/test/schemas/nullable/spec.yaml | Adds DecoratedRef and DecoratableValue schemas exercising the allOf-wraps-$ref-with-nullable idiom for both required and optional fields, with provenance comment for issue #1898. |
| internal/test/schemas/nullable/nullable.gen.go | Adds DecoratableValue type alias and DecoratedRef struct with nullable.Nullable[string] fields as expected; no unrelated drift. |
| internal/test/schemas/nullable/nullable_test.go | Adds TestDecoratedRef_AllOfNullable with compile-time type assertions and JSON marshalling round-trip tests for both required and optional nullable refs. |
Reviews (1): Last reviewed commit: "Fix allOf/nullable detection and recursi..." | Re-trigger Greptile
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes: #1898
The OpenAPI 3.0 idiom for making a $ref nullable is to wrap it in an allOf alongside an inline
{nullable: true}, since 3.0 forbids siblings next to $ref. This failed generation with "error merging schemas for AllOf: merging two schemas with different Nullable".Two problems:
mergeOpenapiSchemas errored whenever two allOf members disagreed on nullability. kin-openapi models Nullable as a plain bool, so an unset value is indistinguishable from an explicit
nullable: false, which made this widely-used idiom unusable. Union the flag instead (if any member is nullable, the merged schema is nullable) — consistent with how Required is already merged.schemaIsNullable only read the schema's own Nullable flag. For the allOf idiom the flag lives on a member, so the field was generated as non-nullable (no pointer / nullable.Nullable[T] wrapping). Descend transitively into allOf members, matching the transitive allOf flattening in mergeOpenapiSchemas, with a lazily-allocated cycle guard so a self-referential allOf cannot cause unbounded recursion.
Adds unit tests for the nullable union and an end-to-end fixture in internal/test/schemas/nullable exercising the decorated-$ref idiom under nullable-type:true for both required and optional properties.