From 27c7acb5915fbb85cbe2d186b5ea3dc9cbaed53f Mon Sep 17 00:00:00 2001 From: Hittrich Date: Mon, 18 May 2026 23:29:16 +0200 Subject: [PATCH 1/2] Preserve typed property when allOf branches overlap --- pkg/codegen/schema_merge.go | 22 ++++++++++-- pkg/codegen/schema_merge_test.go | 29 ++++++++++++++++ .../merge-overlapping-property-typed-wins.yml | 34 +++++++++++++++++++ 3 files changed, 83 insertions(+), 2 deletions(-) create mode 100644 pkg/codegen/testdata/merge-overlapping-property-typed-wins.yml diff --git a/pkg/codegen/schema_merge.go b/pkg/codegen/schema_merge.go index 616ff2d3..6b8ae819 100644 --- a/pkg/codegen/schema_merge.go +++ b/pkg/codegen/schema_merge.go @@ -713,7 +713,16 @@ func mergeOpenapiSchemas(s1, s2 *base.Schema) (*base.Schema, error) { // Required. We merge these. result.Required = append(s1.Required, s2.Required...) - // We merge all properties + // We merge all properties. When both schemas declare a property with + // the same name, the typed declaration wins over an untyped one + // (which usually only carries annotations like example or + // description). Without this, the later schema silently overwrote + // the earlier one and any type/enum/constraint it carried was lost. + // + // We deliberately pick one of the original SchemaProxies rather + // than fabricating a merged proxy via CreateSchemaProxy: downstream + // code reads GoLow().GetReference() to attribute properties back to + // the spec, and a synthetic proxy has no low-level backing. for k, v := range s1.Properties.FromOldest() { if result.Properties == nil { result.Properties = orderedmap.New[string, *base.SchemaProxy]() @@ -721,10 +730,19 @@ func mergeOpenapiSchemas(s1, s2 *base.Schema) (*base.Schema, error) { result.Properties.Set(k, v) } for k, v := range s2.Properties.FromOldest() { - // TODO: detect conflicts if result.Properties == nil { result.Properties = orderedmap.New[string, *base.SchemaProxy]() } + + if existing, exists := result.Properties.Get(k); exists { + if len(getSchemaType(existing.Schema())) == 0 && len(getSchemaType(v.Schema())) > 0 { + result.Properties.Set(k, v) + } + // Otherwise the existing (s1's) property keeps its place; + // either it already has a type and we don't want to drop it, + // or neither side has a type and the choice is moot. + continue + } result.Properties.Set(k, v) } diff --git a/pkg/codegen/schema_merge_test.go b/pkg/codegen/schema_merge_test.go index 3794b555..74263a5e 100644 --- a/pkg/codegen/schema_merge_test.go +++ b/pkg/codegen/schema_merge_test.go @@ -119,6 +119,35 @@ func TestMergeOpenapiSchemas(t *testing.T) { assert.NotEmpty(t, code) }) + t.Run("overlapping property keeps typed declaration", func(t *testing.T) { + // When two allOf branches declare the same property and only + // one of them gives it a type (the other only attaches an + // annotation such as `example`), the typed declaration must + // survive the merge. Previously the second branch silently + // overwrote the first, turning a `status: string` into an + // empty struct. + contents, err := os.ReadFile("testdata/merge-overlapping-property-typed-wins.yml") + require.NoError(t, err) + + opts := Configuration{ + PackageName: "testpkg", + Output: &Output{ + UseSingleFile: true, + }, + } + + code, err := Generate(contents, opts) + require.NoError(t, err) + combined := code.GetCombined() + + assert.Contains(t, combined, "type InvoiceCancellation struct", + "InvoiceCancellation should be a struct") + assert.Regexp(t, `(?m)Status\s+\*?string`, combined, + "Status should keep the string type from the referenced Invoice schema, not collapse to an empty struct from the annotation-only override") + assert.NotRegexp(t, `(?m)Status\s+\*?struct\{\}`, combined, + "Status must not be generated as struct{}; that would mean the annotation-only override dropped the typed declaration") + }) + t.Run("allOf with sibling properties preserves all fields", func(t *testing.T) { contents, err := os.ReadFile("testdata/allof-with-sibling-properties.yml") require.NoError(t, err) diff --git a/pkg/codegen/testdata/merge-overlapping-property-typed-wins.yml b/pkg/codegen/testdata/merge-overlapping-property-typed-wins.yml new file mode 100644 index 00000000..e8429043 --- /dev/null +++ b/pkg/codegen/testdata/merge-overlapping-property-typed-wins.yml @@ -0,0 +1,34 @@ +openapi: 3.0.0 +info: + title: Overlapping property in allOf preserves the typed declaration + version: 1.0.0 +paths: + /test: + get: + responses: + '200': + description: OK + content: + application/json: + schema: + $ref: '#/components/schemas/InvoiceCancellation' +components: + schemas: + Invoice: + type: object + properties: + id: + type: string + status: + type: string + enum: + - open + - paid + - cancelled + InvoiceCancellation: + allOf: + - $ref: '#/components/schemas/Invoice' + - type: object + properties: + status: + example: cancelled From 02e1c2949d9330fcc168372918f316f841be1695 Mon Sep 17 00:00:00 2001 From: Hittrich Date: Mon, 18 May 2026 23:39:56 +0200 Subject: [PATCH 2/2] regen --- pkg/codegen/schema_merge.go | 59 ++++++++++++++++++++++++++++++------- 1 file changed, 48 insertions(+), 11 deletions(-) diff --git a/pkg/codegen/schema_merge.go b/pkg/codegen/schema_merge.go index 6b8ae819..517f6dbb 100644 --- a/pkg/codegen/schema_merge.go +++ b/pkg/codegen/schema_merge.go @@ -713,11 +713,13 @@ func mergeOpenapiSchemas(s1, s2 *base.Schema) (*base.Schema, error) { // Required. We merge these. result.Required = append(s1.Required, s2.Required...) - // We merge all properties. When both schemas declare a property with - // the same name, the typed declaration wins over an untyped one - // (which usually only carries annotations like example or - // description). Without this, the later schema silently overwrote - // the earlier one and any type/enum/constraint it carried was lost. + // We merge all properties. When both schemas declare a property + // with the same name, an annotation-only override (carrying only + // fields like `example` or `description`) must not overwrite a + // sibling that actually shapes the Go type. Otherwise we keep the + // previous "s2 wins" behavior, since both sides genuinely shape + // the type and the second declaration is the more specific one in + // an allOf chain. // // We deliberately pick one of the original SchemaProxies rather // than fabricating a merged proxy via CreateSchemaProxy: downstream @@ -735,13 +737,9 @@ func mergeOpenapiSchemas(s1, s2 *base.Schema) (*base.Schema, error) { } if existing, exists := result.Properties.Get(k); exists { - if len(getSchemaType(existing.Schema())) == 0 && len(getSchemaType(v.Schema())) > 0 { - result.Properties.Set(k, v) + if isAnnotationOnlySchema(v.Schema()) && !isAnnotationOnlySchema(existing.Schema()) { + continue } - // Otherwise the existing (s1's) property keeps its place; - // either it already has a type and we don't want to drop it, - // or neither side has a type and the choice is moot. - continue } result.Properties.Set(k, v) } @@ -809,6 +807,45 @@ func getSchemaType(schema *base.Schema) []string { return nil } +// isAnnotationOnlySchema reports whether a schema has no fields that +// influence the generated Go type. Schemas like `{ example: cancelled }` +// or `{ description: "..." }` are annotation-only and must not be +// allowed to overwrite a sibling declaration that actually carries a +// type, enum, or sub-schema in an allOf merge. +func isAnnotationOnlySchema(s *base.Schema) bool { + if s == nil { + return true + } + if len(s.Type) > 0 { + return false + } + if len(s.AllOf) > 0 || len(s.OneOf) > 0 || len(s.AnyOf) > 0 { + return false + } + if s.If != nil || s.Then != nil || s.Else != nil || s.Not != nil { + return false + } + if s.Properties != nil && s.Properties.Len() > 0 { + return false + } + if s.PatternProperties != nil && s.PatternProperties.Len() > 0 { + return false + } + if s.Items != nil { + return false + } + if len(s.Enum) > 0 { + return false + } + if s.AdditionalProperties != nil { + return false + } + if s.Const != nil { + return false + } + return true +} + // mergeNullable merges two nullable pointers using a union (more permissive) approach. // If either is true, the result is true. If both are false or nil, the result is nil. // This allows merging schemas where one specifies nullable and another doesn't.