diff --git a/internal/test/schemas/nullable/nullable.gen.go b/internal/test/schemas/nullable/nullable.gen.go index 940611a65..9a45381fc 100644 --- a/internal/test/schemas/nullable/nullable.gen.go +++ b/internal/test/schemas/nullable/nullable.gen.go @@ -37,6 +37,15 @@ type Container struct { MayBeNull []nullable.Nullable[string] `json:"may-be-null"` } +// DecoratableValue defines model for DecoratableValue. +type DecoratableValue = string + +// DecoratedRef defines model for DecoratedRef. +type DecoratedRef struct { + OptionalNullableRef nullable.Nullable[string] `json:"optional_nullable_ref,omitempty"` + RequiredNullableRef nullable.Nullable[string] `json:"required_nullable_ref"` +} + // PatchRequest A request to patch an existing user object. type PatchRequest struct { // ComplexOptionalNullable Complex, optional and nullable diff --git a/internal/test/schemas/nullable/nullable_test.go b/internal/test/schemas/nullable/nullable_test.go index ca9f7f2e9..e8aa4d562 100644 --- a/internal/test/schemas/nullable/nullable_test.go +++ b/internal/test/schemas/nullable/nullable_test.go @@ -219,6 +219,46 @@ func TestContainer_UsesNullableType(t *testing.T) { require.True(t, c.MayBeNull[0].IsNull()) } +// ---- issue #1898: allOf decorating a $ref with `nullable: true` ---- + +// TestDecoratedRef_AllOfNullable covers the OpenAPI 3.0 idiom of making a +// referenced schema nullable by wrapping it in an allOf next to an inline +// `{nullable: true}` (issue #1898). Before the fix this failed generation +// with "merging two schemas with different Nullable"; now the decorated +// $ref is generated as a nullable.Nullable[T] field under nullable-type:true, +// for both required and optional properties. +func TestDecoratedRef_AllOfNullable(t *testing.T) { + // Compile-time proof of the field types: these only assign if both + // fields are nullable.Nullable[string]. + d := DecoratedRef{ + RequiredNullableRef: nullable.NewNullableWithValue("present"), + OptionalNullableRef: nullable.NewNullNullable[string](), + } + + val, err := d.RequiredNullableRef.Get() + require.NoError(t, err) + assert.Equal(t, "present", val) + assert.True(t, d.OptionalNullableRef.IsNull()) + + // Explicit null on the required field marshals to JSON null; the + // optional-but-unset field is omitted. + d = DecoratedRef{ + RequiredNullableRef: nullable.NewNullNullable[string](), + } + actual, err := json.Marshal(d) + require.NoError(t, err) + assert.JSONEq(t, `{"required_nullable_ref":null}`, string(actual)) + + // A value on the required field round-trips. + d = DecoratedRef{ + RequiredNullableRef: nullable.NewNullableWithValue("hello"), + OptionalNullableRef: nullable.NewNullableWithValue("world"), + } + actual, err = json.Marshal(d) + require.NoError(t, err) + assert.JSONEq(t, `{"required_nullable_ref":"hello","optional_nullable_ref":"world"}`, string(actual)) +} + // ---- openapi31_nullable: 3.0 vs 3.1 nullable idiom equivalence ---- // TestNicknameIsPointer_3_0 asserts that the 3.0 spec's `nullable: true` diff --git a/internal/test/schemas/nullable/spec.yaml b/internal/test/schemas/nullable/spec.yaml index d3c2742d9..1e3995d25 100644 --- a/internal/test/schemas/nullable/spec.yaml +++ b/internal/test/schemas/nullable/spec.yaml @@ -96,3 +96,27 @@ components: items: type: string nullable: true + + # issue #1898: decorate a $ref with `nullable: true` via allOf. OpenAPI 3.0 + # forbids siblings next to $ref, so allOf is the only way to add nullability + # to a referenced schema. Before the fix this errored with "merging two + # schemas with different Nullable". The inline member carries no structural + # content, so the merged type is the referenced value made nullable. + DecoratedRef: + type: object + required: + - required_nullable_ref + properties: + required_nullable_ref: + # required + nullable(via allOf) -> nullable.Nullable[...] + allOf: + - $ref: "#/components/schemas/DecoratableValue" + - nullable: true + optional_nullable_ref: + # optional + nullable(via allOf) -> nullable.Nullable[...] + allOf: + - $ref: "#/components/schemas/DecoratableValue" + - nullable: true + + DecoratableValue: + type: string diff --git a/pkg/codegen/merge_schemas.go b/pkg/codegen/merge_schemas.go index 537737913..0585044a0 100644 --- a/pkg/codegen/merge_schemas.go +++ b/pkg/codegen/merge_schemas.go @@ -356,11 +356,24 @@ func mergeOpenapiSchemas(s1, s2 openapi3.Schema, allOf bool, seenSchemaRef map[s // without needing to touch result.Nullable. The result.Nullable copy // below is a no-op in 3.1 (s1.Nullable is always false there) but kept // for 3.0 correctness, where Nullable is the only nullability carrier. - if schemaIsNullable(&s1) != schemaIsNullable(&s2) { - return openapi3.Schema{}, errors.New("merging two schemas with different Nullable") - + // + // Nullability is UNIONed rather than required to match: if any member + // is nullable, the merged schema is nullable. This supports the common + // OpenAPI 3.0 idiom of decorating a $ref with nullability, which is only + // expressible through allOf because 3.0 forbids siblings next to $ref + // (issue #1898): + // + // allOf: + // - $ref: "#/components/schemas/user" + // - nullable: true + // + // kin-openapi represents Nullable as a plain bool, so an unset value is + // indistinguishable from an explicit `nullable: false`; erroring on a + // mismatch made this widely-used idiom unusable. Union is also + // consistent with how Required is merged below. + if schemaIsNullable(&s1) || schemaIsNullable(&s2) { + result.Nullable = true } - result.Nullable = s1.Nullable if s1.ReadOnly != s2.ReadOnly { return openapi3.Schema{}, errors.New("merging two schemas with different ReadOnly") diff --git a/pkg/codegen/merge_schemas_test.go b/pkg/codegen/merge_schemas_test.go index 61fcbef12..15d2b1364 100644 --- a/pkg/codegen/merge_schemas_test.go +++ b/pkg/codegen/merge_schemas_test.go @@ -66,3 +66,44 @@ func TestMergeOpenapiSchemas_DiscriminatorPropagation(t *testing.T) { require.Error(t, err) }) } + +// TestMergeOpenapiSchemas_NullableUnion covers the OpenAPI 3.0 idiom of +// decorating a $ref with `nullable: true` via allOf (issue #1898). Members +// disagreeing on nullability must merge (union) rather than error. +func TestMergeOpenapiSchemas_NullableUnion(t *testing.T) { + t.Run("nullable on s2 propagates to result", func(t *testing.T) { + s1 := openapi3.Schema{} + s2 := openapi3.Schema{Nullable: true} + + result, err := mergeOpenapiSchemas(s1, s2, true, make(map[string]bool)) + require.NoError(t, err) + assert.True(t, result.Nullable) + }) + + t.Run("nullable on s1 propagates to result", func(t *testing.T) { + s1 := openapi3.Schema{Nullable: true} + s2 := openapi3.Schema{} + + result, err := mergeOpenapiSchemas(s1, s2, true, make(map[string]bool)) + require.NoError(t, err) + assert.True(t, result.Nullable) + }) + + t.Run("both nullable stays nullable", func(t *testing.T) { + s1 := openapi3.Schema{Nullable: true} + s2 := openapi3.Schema{Nullable: true} + + result, err := mergeOpenapiSchemas(s1, s2, true, make(map[string]bool)) + require.NoError(t, err) + assert.True(t, result.Nullable) + }) + + t.Run("neither nullable stays non-nullable", func(t *testing.T) { + s1 := openapi3.Schema{} + s2 := openapi3.Schema{} + + result, err := mergeOpenapiSchemas(s1, s2, true, make(map[string]bool)) + require.NoError(t, err) + assert.False(t, result.Nullable) + }) +} diff --git a/pkg/codegen/schema.go b/pkg/codegen/schema.go index 5d517eb26..0972159cc 100644 --- a/pkg/codegen/schema.go +++ b/pkg/codegen/schema.go @@ -468,9 +468,41 @@ func PropertiesEqual(a, b Property) bool { // using `type: [..., "null"]`) is documented as invalid input -- this // helper trusts its input to use the version-appropriate idiom. func schemaIsNullable(s *openapi3.Schema) bool { + return schemaIsNullableRec(s, nil) +} + +// schemaIsNullableRec is schemaIsNullable's implementation, carrying a +// `seen` set of already-visited schema values so that a cyclic allOf (a +// $ref member resolving back to an ancestor — the same cycles +// mergeOpenapiSchemas guards against) cannot cause unbounded recursion. +// The set is allocated lazily: the common case (no allOf) never touches it. +func schemaIsNullableRec(s *openapi3.Schema, seen map[*openapi3.Schema]bool) bool { if s == nil { return false } + // A nullable member inside an allOf makes the whole schema nullable. + // In OpenAPI 3.0, allOf is the only place a sibling (`nullable: true`) + // may sit next to a $ref, so wrapping a $ref in allOf is the idiomatic + // way to decorate a referenced schema as nullable (issue #1898). This + // is where the flag lives — the outer schema's own Nullable is unset — + // so descend into the members. Descent is transitive to match the + // transitive allOf flattening in mergeOpenapiSchemas, and equally valid + // under 3.1's type-array idiom, hence checked before the version + // branch. mergeOpenapiSchemas unions nullability into the merged type; + // this surfaces it at the use site so the field is wrapped in a pointer + // / nullable.Nullable[T]. + for _, member := range s.AllOf { + if member == nil || member.Value == nil || seen[member.Value] { + continue + } + if seen == nil { + seen = make(map[*openapi3.Schema]bool) + } + seen[member.Value] = true + if schemaIsNullableRec(member.Value, seen) { + return true + } + } if globalState.is31 { if s.Type != nil && s.Type.Includes("null") { return true