From 30f8e6a4630161ba2535c56de04bc8c08b7b372d Mon Sep 17 00:00:00 2001 From: Hittrich Date: Wed, 20 May 2026 19:29:12 +0200 Subject: [PATCH 1/3] Harden allOf merging and default-response handling --- pkg/codegen/naming.go | 12 +- pkg/codegen/schema_merge.go | 236 ++++++++++++++---- pkg/codegen/schema_merge_test.go | 172 +++++++++++++ pkg/codegen/schema_object.go | 7 +- .../merge-nested-allof-no-type-wrapper.yml | 39 +++ pkg/codegen/typedef_responses.go | 190 ++++++++------ pkg/codegen/typedef_responses_test.go | 230 +++++++++++++++++ 7 files changed, 764 insertions(+), 122 deletions(-) create mode 100644 pkg/codegen/testdata/merge-nested-allof-no-type-wrapper.yml create mode 100644 pkg/codegen/typedef_responses_test.go diff --git a/pkg/codegen/naming.go b/pkg/codegen/naming.go index c3e8e319..c957ee61 100644 --- a/pkg/codegen/naming.go +++ b/pkg/codegen/naming.go @@ -676,5 +676,15 @@ func isMediaTypeJson(mediaType string) bool { if err != nil { return false } - return parsed == "application/json" || strings.HasSuffix(parsed, "+json") + + if parsed == "application/json" || strings.HasSuffix(parsed, "+json") { + return true + } + + switch parsed { + case "application/x-ndjson", "application/ndjson", + "application/jsonl", "application/x-jsonlines": + return true + } + return false } diff --git a/pkg/codegen/schema_merge.go b/pkg/codegen/schema_merge.go index 517f6dbb..d0a6cd47 100644 --- a/pkg/codegen/schema_merge.go +++ b/pkg/codegen/schema_merge.go @@ -14,6 +14,7 @@ import ( "fmt" "reflect" "slices" + "strings" "github.com/pb33f/libopenapi/datamodel/high/base" "github.com/pb33f/libopenapi/orderedmap" @@ -80,9 +81,16 @@ func createFromCombinator(schema *base.Schema, options ParseOptions) (GoSchema, } // If the allOf resulted in a simple type (no properties), return it directly - // This handles cases like allOf with a description-only schema and a $ref + // This handles cases like allOf with a description-only schema and a $ref. + // Exception: when the parent schema declares its own object type, the + // outer type wins over a conflicting primitive from allOf (a malformed + // spec pattern: `type: object, nullable: true, allOf: [SomeEnum]`). + // Falling through lets createObjectSchema produce an object the + // validator accepts; the allOf becomes metadata-only. if len(allOfSchema.Properties) == 0 && !hasAnyOf && !hasOneOf { - return allOfSchema, nil + if !parentTypeConflictsWithAllOf(schema, allOfSchema) { + return allOfSchema, nil + } } out.Properties = append(out.Properties, allOfSchema.Properties...) @@ -304,6 +312,35 @@ func isMetadataOnlySchema(schema *base.Schema) bool { return true } +// parentTypeConflictsWithAllOf reports whether the parent schema declares +// `type: object` while the merged allOf result resolves to a non-object +// primitive (e.g. an integer enum). In that case the allOf must not be +// returned as the schema's effective type — the outer object wins. Only +// the object/primitive direction is checked here; primitive-vs-primitive +// mismatches are rejected by mergeOpenapiSchemas already. +func parentTypeConflictsWithAllOf(parent *base.Schema, allOfSchema GoSchema) bool { + if parent == nil || !slices.Contains(parent.Type, "object") { + return false + } + if allOfSchema.GoType == "" { + return false + } + if strings.HasPrefix(allOfSchema.GoType, "map[") || strings.HasPrefix(allOfSchema.GoType, "[]") { + return false + } + if isPrimitiveType(allOfSchema.GoType) || len(allOfSchema.EnumValues) > 0 { + return true + } + if allOfSchema.OpenAPISchema != nil { + for _, t := range allOfSchema.OpenAPISchema.Type { + if t != "object" && t != "null" { + return true + } + } + } + return false +} + // mergeAllOfSchemas merges all allOf elements into a single GoSchema. // When hasSiblingProperties is true, the parent schema has its own properties // alongside allOf, so the single-ref alias optimization is skipped to ensure @@ -558,23 +595,18 @@ func mergeAllOfSchemas(allOf []*base.SchemaProxy, options ParseOptions, hasSibli return out, nil } -// flattenAllOf transitively resolves a schema's allOf while preserving the -// schema's own structural fields (properties, required, extensions, nullable, -// additionalProperties). Returns s unchanged when it has no allOf. +// flattenAllOf resolves s.AllOf and merges back the parent's structural +// fields. Annotation fields (format, description, title) are excluded to +// avoid merge conflicts with the allOf elements. func flattenAllOf(s *base.Schema) *base.Schema { if s == nil || s.AllOf == nil { return s } merged, err := mergeAllOf(s.AllOf) if err != nil { - // Fall back to original schema on merge errors return s } - // Build a schema with only the structural fields from s that should be - // preserved alongside allOf. Annotation fields like format, description, - // and title are intentionally excluded to avoid merge conflicts with the - // allOf elements. ownFields := &base.Schema{ Type: s.Type, Properties: s.Properties, @@ -590,9 +622,6 @@ func flattenAllOf(s *base.Schema) *base.Schema { result, err := mergeOpenapiSchemas(merged, ownFields) if err != nil { - // If own fields conflict with the allOf result (e.g., incompatible - // types), fall back to the allOf-only merge to avoid breaking - // generation for otherwise valid specs. return s } return result @@ -618,6 +647,12 @@ func mergeOpenapiSchemas(s1, s2 *base.Schema) (*base.Schema, error) { return s2, nil } + // Must flatten before the type-presence checks. A bare allOf wrapper + // has no own `type`; without flattening, the `len(t1) == 0` branch + // below would discard its contents and return s2 alone. + s1 = flattenAllOf(s1) + s2 = flattenAllOf(s2) + result := &base.Schema{} t1 := getSchemaType(s1) @@ -651,9 +686,6 @@ func mergeOpenapiSchemas(s1, s2 *base.Schema) (*base.Schema, error) { // additionalProperties, nullable), merge them into the allOf result so // they are not lost. Annotation-only fields (format, description, title) // are not merged because they can conflict with the allOf elements. - s1 = flattenAllOf(s1) - s2 = flattenAllOf(s2) - result.AllOf = append(s1.AllOf, s2.AllOf...) result.Type = t1 @@ -700,31 +732,22 @@ func mergeOpenapiSchemas(s1, s2 *base.Schema) (*base.Schema, error) { // This allows merging schemas where one specifies nullable and another doesn't result.Nullable = mergeNullable(s1.Nullable, s2.Nullable) - if !ptrEqual(s1.ReadOnly, s2.ReadOnly) { - return nil, ErrMergingSchemasWithDifferentReadOnly - } - result.ReadOnly = s1.ReadOnly - - if !ptrEqual(s1.WriteOnly, s2.WriteOnly) { - return nil, ErrMergingSchemasWithDifferentWriteOnly - } - result.WriteOnly = s1.WriteOnly + // ReadOnly/WriteOnly are annotations, not constraints — the validator + // doesn't enforce per-branch agreement. Take the union (true if any + // branch is true): if one branch marks the property response-only, the + // merged property is response-only. + result.ReadOnly = mergeBoolPtrOr(s1.ReadOnly, s2.ReadOnly) + result.WriteOnly = mergeBoolPtrOr(s1.WriteOnly, s2.WriteOnly) // 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, 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 - // code reads GoLow().GetReference() to attribute properties back to - // the spec, and a synthetic proxy has no low-level backing. + // On property collision, an annotation-only override (just + // `example` or `description`) must not overwrite a sibling that + // shapes the Go type. Otherwise s2 wins, matching prior behavior. + // We keep the original SchemaProxy rather than fabricating one via + // CreateSchemaProxy: downstream reads GoLow().GetReference() to + // attribute properties back to the spec. for k, v := range s1.Properties.FromOldest() { if result.Properties == nil { result.Properties = orderedmap.New[string, *base.SchemaProxy]() @@ -740,10 +763,19 @@ func mergeOpenapiSchemas(s1, s2 *base.Schema) (*base.Schema, error) { if isAnnotationOnlySchema(v.Schema()) && !isAnnotationOnlySchema(existing.Schema()) { continue } + if merged, ok := mergePropertyProxies(existing, v); ok { + result.Properties.Set(k, merged) + continue + } } result.Properties.Set(k, v) } + // Merge array Items. Without this, two array schemas with `items` on + // both sides produce a result with no Items, and downstream type + // inference falls back to string. + result.Items = mergeItems(s1.Items, s2.Items) + if isAdditionalPropertiesExplicitFalse(s1) || isAdditionalPropertiesExplicitFalse(s2) { result.AdditionalProperties = &base.DynamicValue[*base.SchemaProxy, bool]{ A: nil, @@ -778,6 +810,60 @@ func mergeOpenapiSchemas(s1, s2 *base.Schema) (*base.Schema, error) { return result, nil } +// mergePropertyProxies merges two colliding property schemas so that +// constraints from either side (enum, pattern, format, etc.) survive +// the allOf merge instead of being clobbered by the second declaration. +// Reports ok=false if the merge isn't safe (e.g. incompatible types); +// caller should fall back to s2-wins. Returned proxy is synthetic +// (no GoLow().GetReference()), which is acceptable for property +// collisions where the type itself stays inline. +func mergePropertyProxies(a, b *base.SchemaProxy) (*base.SchemaProxy, bool) { + sa := a.Schema() + sb := b.Schema() + if sa == nil || sb == nil { + return nil, false + } + + saCopy := *sa + sbCopy := *sb + + // Propagate type when one side has it and the other doesn't, so the + // `len(tX) == 0` short-circuits in mergeOpenapiSchemas don't drop + // the typeless side's constraints (enum, pattern, ...) on the floor. + if len(saCopy.Type) > 0 && len(sbCopy.Type) == 0 { + sbCopy.Type = saCopy.Type + } else if len(sbCopy.Type) > 0 && len(saCopy.Type) == 0 { + saCopy.Type = sbCopy.Type + } + + // Relax flags that mergeOpenapiSchemas rejects outright. Property + // overrides commonly omit readOnly/writeOnly/exclusive*; treating + // the mismatch as "inherit from the typed side" is safer than + // dropping the override entirely. + sbCopy.ReadOnly = saCopy.ReadOnly + sbCopy.WriteOnly = saCopy.WriteOnly + sbCopy.UniqueItems = saCopy.UniqueItems + sbCopy.ExclusiveMinimum = saCopy.ExclusiveMinimum + sbCopy.ExclusiveMaximum = saCopy.ExclusiveMaximum + if sbCopy.Format == "" { + sbCopy.Format = saCopy.Format + } + + merged, err := mergeOpenapiSchemas(&saCopy, &sbCopy) + if err != nil || merged == nil { + return nil, false + } + + if sa.Example != nil && merged.Example == nil { + merged.Example = sa.Example + } + if sb.Example != nil && merged.Example == nil { + merged.Example = sb.Example + } + + return base.CreateSchemaProxy(merged), true +} + // isAdditionalPropertiesExplicitFalse determines whether an Schema is explicitly defined as `additionalProperties: false` func isAdditionalPropertiesExplicitFalse(s *base.Schema) bool { if s.AdditionalProperties == nil { @@ -807,11 +893,8 @@ 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. +// isAnnotationOnlySchema reports whether a schema only has metadata +// (description, example) and no fields that shape the generated type. func isAnnotationOnlySchema(s *base.Schema) bool { if s == nil { return true @@ -850,11 +933,62 @@ func isAnnotationOnlySchema(s *base.Schema) bool { // 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. func mergeNullable(a, b *bool) *bool { - // If either is explicitly true, result is true + return mergeBoolPtrOr(a, b) +} + +// mergeItems combines the array `items` constraint from two allOf branches. +// In 3.1, `items` can be either a schema or `false` (forbidding items); if +// either side forbids items, the merged result does too. For schema items, +// the later branch wins, mirroring the property-override semantics in +// `mergeOpenapiSchemas`: a later allOf branch specializes the items +// declaration from an earlier branch. We don't attempt true element-wise +// composition because one side can be a wrapper like `anyOf: [...]` with +// no own type, which `mergeOpenapiSchemas` would short-circuit and drop. +func mergeItems(a, b *base.DynamicValue[*base.SchemaProxy, bool]) *base.DynamicValue[*base.SchemaProxy, bool] { + if a == nil { + return b + } + if b == nil { + return a + } + if !a.IsA() { + if !a.B { + return a + } + return b + } + if !b.IsA() { + if !b.B { + return b + } + return a + } + + // Annotation-only override (no shape, just description/example) must + // not clobber a typed sibling - same rule as property collisions. + if a.A != nil && b.A != nil { + sa := a.A.Schema() + sb := b.A.Schema() + if sa != nil && sb != nil { + if isAnnotationOnlySchema(sb) && !isAnnotationOnlySchema(sa) { + return a + } + if isAnnotationOnlySchema(sa) && !isAnnotationOnlySchema(sb) { + return b + } + } + } + return b +} + +// mergeBoolPtrOr returns the logical OR of two optional booleans, preserving +// the "unset" state (nil) when both inputs are nil or false. Used to merge +// annotation flags (Nullable, ReadOnly, WriteOnly) across allOf branches: +// if any branch sets the flag, the merged result has it set. +func mergeBoolPtrOr(a, b *bool) *bool { if (a != nil && *a) || (b != nil && *b) { return ptr(true) } - // If both are nil or false, return nil (default behavior) return nil } @@ -875,14 +1009,24 @@ func isDiscriminatedUnionWithChild(schema *base.Schema, childRef string) bool { return false } - // Check if any oneOf element references the child for _, element := range schema.OneOf { if element.GetReference() == childRef { return true } } + for _, element := range schema.AnyOf { + if element.GetReference() == childRef { + return true + } + } + + // Discriminator + mapping without oneOf/anyOf describes an + // inheritance base, not a union. The child's allOf inherits from + // the base and should NOT be filtered. + if len(schema.OneOf) == 0 && len(schema.AnyOf) == 0 { + return false + } - // Also check the discriminator mapping if schema.Discriminator.Mapping != nil { for _, mappedRef := range schema.Discriminator.Mapping.FromOldest() { if mappedRef == childRef { diff --git a/pkg/codegen/schema_merge_test.go b/pkg/codegen/schema_merge_test.go index 74263a5e..4e55a62d 100644 --- a/pkg/codegen/schema_merge_test.go +++ b/pkg/codegen/schema_merge_test.go @@ -119,6 +119,120 @@ func TestMergeOpenapiSchemas(t *testing.T) { assert.NotEmpty(t, code) }) + t.Run("nested allOf inside a no-type wrapper preserves properties", func(t *testing.T) { + // When an allOf entry is itself an allOf wrapper without an + // explicit `type`, the merge has to flatten the nested allOf + // before checking types: otherwise the type-presence + // short-circuit drops the wrapper entirely and the composed + // schema ends up with only the second branch's properties. + contents, err := os.ReadFile("testdata/merge-nested-allof-no-type-wrapper.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 ContactDetails struct", + "ContactDetails should be a struct") + // Properties from the nested allOf wrapper must survive. + assert.Regexp(t, `(?m)ContactID\s+\*?string`, combined, + "ContactID must come through from the nested allOf wrapper") + assert.Regexp(t, `(?m)Note\s+\*?string`, combined, + "Note must come through from the nested allOf wrapper") + // And the outer branch's own properties stay. + assert.Regexp(t, `(?m)Tags\s+\*?\[\]string`, combined, + "Tags must stay alongside the inherited fields") + }) + + t.Run("allOf with colliding array items keeps the later branch", func(t *testing.T) { + // Regression: when two allOf branches both declare `items`, + // mergeItems used to pick the first branch and silently drop + // the second, losing any anyOf/oneOf union inside it. allOf + // composition refines an earlier declaration, so the later + // branch should win - same rule as property collisions. + contents := []byte(` +openapi: "3.0.0" +info: + version: 1.0.0 + title: Test +paths: + /test: + post: + operationId: testEndpoint + requestBody: + required: true + content: + application/json: + schema: + $ref: '#/components/schemas/CombinedError' + responses: + '200': + description: ok +components: + schemas: + BaseError: + type: object + properties: + issues: + type: array + items: + type: object + properties: + field: + type: string + issue: + type: string + SpecificError: + properties: + issues: + type: array + items: + anyOf: + - title: ERROR_TYPE_A + properties: + issue: + type: string + enum: + - ERROR_A + - title: ERROR_TYPE_B + properties: + issue: + type: string + enum: + - ERROR_B + CombinedError: + allOf: + - $ref: '#/components/schemas/BaseError' + - $ref: '#/components/schemas/SpecificError' +`) + opts := Configuration{ + PackageName: "testpkg", + Output: &Output{UseSingleFile: true}, + } + + code, err := Generate(contents, opts) + require.NoError(t, err) + combined := code.GetCombined() + + // The anyOf branches from SpecificError must survive the + // merge - the previous bug collapsed CombinedError items to + // just BaseError's {field, issue} object and dropped the + // ERROR_A/ERROR_B enums entirely. + assert.Contains(t, combined, "ERRORA", + "ERROR_A enum from SpecificError's anyOf must survive the merge") + assert.Contains(t, combined, "ERRORB", + "ERROR_B enum from SpecificError's anyOf must survive the merge") + assert.Contains(t, combined, "CombinedError_Issues_AnyOf", + "the anyOf union type on items must be generated for CombinedError") + }) + 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 @@ -808,6 +922,64 @@ components: result := isDiscriminatedUnionWithChild(nil, "#/components/schemas/Child") assert.False(t, result) }) + + t.Run("returns false for inheritance base with discriminator but no oneOf", func(t *testing.T) { + contents := []byte(` +openapi: "3.0.0" +info: + version: 1.0.0 + title: Test +paths: {} +components: + schemas: + Parent: + type: object + properties: + kind: + type: string + discriminator: + propertyName: kind + mapping: + CHILD: '#/components/schemas/Child' + Child: + allOf: + - $ref: '#/components/schemas/Parent' + - type: object + properties: + extra: + type: string +`) + doc := loadDoc(t, contents) + parentSchema := doc.Model.Components.Schemas.GetOrZero("Parent").Schema() + result := isDiscriminatedUnionWithChild(parentSchema, "#/components/schemas/Child") + assert.False(t, result) + }) + + t.Run("returns true for anyOf with child ref", func(t *testing.T) { + contents := []byte(` +openapi: "3.0.0" +info: + version: 1.0.0 + title: Test +paths: {} +components: + schemas: + Parent: + discriminator: + propertyName: type + anyOf: + - $ref: '#/components/schemas/Child' + Child: + type: object + properties: + name: + type: string +`) + doc := loadDoc(t, contents) + parentSchema := doc.Model.Components.Schemas.GetOrZero("Parent").Schema() + result := isDiscriminatedUnionWithChild(parentSchema, "#/components/schemas/Child") + assert.True(t, result) + }) } func TestIfThenElse(t *testing.T) { diff --git a/pkg/codegen/schema_object.go b/pkg/codegen/schema_object.go index bce9a783..34de118c 100644 --- a/pkg/codegen/schema_object.go +++ b/pkg/codegen/schema_object.go @@ -111,7 +111,12 @@ func createObjectSchema(schema *base.Schema, options ParseOptions) (GoSchema, er if schema != nil && schema.Properties != nil { for pName, p := range schema.Properties.FromOldest() { propertyPath := append(path, pName) - pRef := p.GoLow().GetReference() + + pRef := "" + if low := p.GoLow(); low != nil { + pRef = low.GetReference() + } + opts := options.WithReference(pRef).WithPath(propertyPath) pSchema, err := GenerateGoSchema(p, opts) if err != nil { diff --git a/pkg/codegen/testdata/merge-nested-allof-no-type-wrapper.yml b/pkg/codegen/testdata/merge-nested-allof-no-type-wrapper.yml new file mode 100644 index 00000000..95d10385 --- /dev/null +++ b/pkg/codegen/testdata/merge-nested-allof-no-type-wrapper.yml @@ -0,0 +1,39 @@ +openapi: 3.0.0 +info: + title: Nested AllOf inside a no-type wrapper test + version: 1.0.0 +paths: + /contacts/{id}: + get: + parameters: + - in: path + name: id + required: true + schema: {type: string} + responses: + '200': + description: ok + content: + application/json: + schema: + $ref: '#/components/schemas/ContactDetails' +components: + schemas: + ContactBase: + allOf: + - properties: + contactId: {type: string} + name: {type: string} + type: object + - properties: + note: {type: string} + type: object + ContactDetails: + type: object + allOf: + - $ref: '#/components/schemas/ContactBase' + - properties: + tags: + type: array + items: {type: string} + type: object diff --git a/pkg/codegen/typedef_responses.go b/pkg/codegen/typedef_responses.go index 714b404a..6df1b0ac 100644 --- a/pkg/codegen/typedef_responses.go +++ b/pkg/codegen/typedef_responses.go @@ -180,6 +180,10 @@ func getOperationResponses(operationID string, responses *v3high.Responses, opti ResponseName: "struct{}", StatusCode: status, Headers: headers, + // Preserve the declared media type even when the schema + // is empty (e.g. `text/html: {}`); strict response + // validators rely on it. + ContentType: contentType, } all[status] = successDefinition } @@ -413,6 +417,25 @@ func getOperationResponses(operationID string, responses *v3high.Responses, opti all[status] = rcd } + // When no explicit success is documented but `default` has content, treat + // `default` as the success at 200. OpenAPI validators interpret `default` + // as covering any unlisted status code (including 2xx), so the spec's + // schema is what they check against. Fabricating a 204 struct{} here + // would make mocks return content that doesn't satisfy the real schema. + defaultAsSuccess := false + if successCode == 0 && defaultResponse != nil && defaultResponse.Content != nil && defaultResponse.Content.First() != nil { + rcd, tds, err := buildDefaultResponseDefinition(operationID, defaultResponse, 200, true, options) + if err != nil { + return nil, nil, err + } + if rcd != nil { + successCode = 200 + all[200] = rcd + typeDefinitions = append(typeDefinitions, tds...) + defaultAsSuccess = true + } + } + if successCode == 0 { successCode = 204 successDefinition := &ResponseContentDefinition{ @@ -425,96 +448,115 @@ func getOperationResponses(operationID string, responses *v3high.Responses, opti all[successCode] = successDefinition } - if errorCode == 0 && defaultResponse != nil { - errorCode = 500 - fstErrorCode = 500 - typeSuffix := "ErrorResponse" - content := defaultResponse.Content.First() + if errorCode == 0 && defaultResponse != nil && !defaultAsSuccess { + rcd, tds, err := buildDefaultResponseDefinition(operationID, defaultResponse, 500, false, options) + if err != nil { + return nil, nil, err + } + if rcd != nil { + fstErrorCode = 500 + all[500] = rcd + typeDefinitions = append(typeDefinitions, tds...) + } + } - ref := "" - contentType := "application/json" - var ( - contentSchema GoSchema - err error - refType string - contentVal *v3high.MediaType - ) + successes, errs := partitionResponses(all) + res := &ResponseDefinition{ + SuccessStatusCode: successCode, + Success: all[successCode], + Error: all[fstErrorCode], + All: all, + Successes: successes, + Errors: errs, + } - if content != nil { - contentType, contentVal = content.Key(), content.Value() - if contentVal.Schema != nil { - ref = contentVal.Schema.GetReference() + return res, typeDefinitions, nil +} - opts := options.WithReference(ref).WithPath([]string{operationID, typeSuffix}) - contentSchema, err = GenerateGoSchema(contentVal.Schema, opts) - if err != nil { - return nil, nil, fmt.Errorf("error generating request body definition: %w", err) - } - } - } +// buildDefaultResponseDefinition turns a `default` response into a +// ResponseContentDefinition installed at the given status. Used both when no +// explicit success is documented (install as 200 success) and when no +// explicit error is documented (install as 500 error). +func buildDefaultResponseDefinition(operationID string, defaultResponse *v3high.Response, status int, isSuccess bool, options ParseOptions) (*ResponseContentDefinition, []TypeDefinition, error) { + typeSuffix := "ErrorResponse" + if isSuccess { + typeSuffix = "Response" + } + + content := defaultResponse.Content.First() + ref := "" + contentType := "application/json" + var ( + contentSchema GoSchema + err error + refType string + contentVal *v3high.MediaType + ) + + if content != nil { + contentType, contentVal = content.Key(), content.Value() + if contentVal.Schema != nil { + ref = contentVal.Schema.GetReference() - if ref != "" { - refType, err = refPathToGoType(ref) + opts := options.WithReference(ref).WithPath([]string{operationID, typeSuffix}) + contentSchema, err = GenerateGoSchema(contentVal.Schema, opts) if err != nil { - return nil, nil, fmt.Errorf("error turning reference (%s) into a Go type: %w", ref, err) + return nil, nil, fmt.Errorf("error generating request body definition: %w", err) } } + } - if !contentSchema.IsZero() { - if refType != "" { - contentSchema.RefType = refType - } - responseName := operationID + typeSuffix - if contentSchema.ArrayType != nil { - contentSchema, _ = replaceInlineTypes(contentSchema, options) - } - td := TypeDefinition{ - Name: responseName, - Schema: contentSchema, - SpecLocation: SpecLocationResponse, - NeedsMarshaler: needsMarshaler(contentSchema), - } - options.typeTracker.register(td, "") - typeDefinitions = append(typeDefinitions, td) + if ref != "" { + refType, err = refPathToGoType(ref) + if err != nil { + return nil, nil, fmt.Errorf("error turning reference (%s) into a Go type: %w", ref, err) + } + } - // Filter out AdditionalTypes that already exist in the type tracker - for _, additionalType := range contentSchema.AdditionalTypes { - if _, exists := options.typeTracker.LookupByName(additionalType.Name); !exists { - typeDefinitions = append(typeDefinitions, additionalType) - options.typeTracker.register(additionalType, "") - } - } + if contentSchema.IsZero() { + return nil, nil, nil + } - errHeaders, err := generateResponseHeadersSchema(defaultResponse.Headers.FromOldest(), operationID, options) - if err != nil { - return nil, nil, fmt.Errorf("error generating response headers schema: %w", err) - } + if refType != "" { + contentSchema.RefType = refType + } + responseName := operationID + typeSuffix + if contentSchema.ArrayType != nil { + contentSchema, _ = replaceInlineTypes(contentSchema, options) + } + var typeDefinitions []TypeDefinition + td := TypeDefinition{ + Name: responseName, + Schema: contentSchema, + SpecLocation: SpecLocationResponse, + NeedsMarshaler: needsMarshaler(contentSchema), + } + options.typeTracker.register(td, "") + typeDefinitions = append(typeDefinitions, td) - errorDefinition := &ResponseContentDefinition{ - ResponseName: responseName, - IsSuccess: false, - Description: defaultResponse.Description, - Schema: contentSchema, - Ref: refType, - ContentType: contentType, - StatusCode: errorCode, - Headers: errHeaders, - } - all[errorCode] = errorDefinition + for _, additionalType := range contentSchema.AdditionalTypes { + if _, exists := options.typeTracker.LookupByName(additionalType.Name); !exists { + typeDefinitions = append(typeDefinitions, additionalType) + options.typeTracker.register(additionalType, "") } } - successes, errs := partitionResponses(all) - res := &ResponseDefinition{ - SuccessStatusCode: successCode, - Success: all[successCode], - Error: all[fstErrorCode], - All: all, - Successes: successes, - Errors: errs, + headers, err := generateResponseHeadersSchema(defaultResponse.Headers.FromOldest(), operationID, options) + if err != nil { + return nil, nil, fmt.Errorf("error generating response headers schema: %w", err) } - return res, typeDefinitions, nil + rcd := &ResponseContentDefinition{ + ResponseName: responseName, + IsSuccess: isSuccess, + Description: defaultResponse.Description, + Schema: contentSchema, + Ref: refType, + ContentType: contentType, + StatusCode: status, + Headers: headers, + } + return rcd, typeDefinitions, nil } // partitionResponses splits an `all` map of every documented response into diff --git a/pkg/codegen/typedef_responses_test.go b/pkg/codegen/typedef_responses_test.go new file mode 100644 index 00000000..237758de --- /dev/null +++ b/pkg/codegen/typedef_responses_test.go @@ -0,0 +1,230 @@ +// Copyright 2025 DoorDash, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. + +package codegen + +import ( + "testing" + + "github.com/pb33f/libopenapi" + v3 "github.com/pb33f/libopenapi/datamodel/high/v3" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGetOperationResponses(t *testing.T) { + loadOp := func(t *testing.T, contents []byte, path, method string) (*v3.Operation, ParseOptions) { + t.Helper() + doc, err := libopenapi.NewDocument(contents) + require.NoError(t, err) + model, errs := doc.BuildV3Model() + require.Empty(t, errs) + + item := model.Model.Paths.PathItems.GetOrZero(path) + require.NotNil(t, item, "path %s not found", path) + + var op *v3.Operation + switch method { + case "get": + op = item.Get + case "post": + op = item.Post + case "put": + op = item.Put + case "delete": + op = item.Delete + } + require.NotNil(t, op, "operation %s %s not found", method, path) + + opts := ParseOptions{ + typeTracker: newTypeTracker(), + visited: map[string]bool{}, + model: &model.Model, + } + return op, opts + } + + t.Run("default response with content becomes 200 success when no explicit success documented", func(t *testing.T) { + contents := []byte(` +openapi: "3.0.0" +info: + version: 1.0.0 + title: Test +paths: + /ping: + get: + operationId: ping + responses: + default: + description: any + content: + application/json: + schema: + type: object + properties: + message: + type: string +`) + op, opts := loadOp(t, contents, "/ping", "get") + res, _, err := getOperationResponses("ping", op.Responses, opts) + require.NoError(t, err) + require.NotNil(t, res) + + assert.Equal(t, 200, res.SuccessStatusCode, + "default with content must be installed as 200 success, not a fabricated 204") + require.NotNil(t, res.Success) + assert.True(t, res.Success.IsSuccess) + assert.Equal(t, "application/json", res.Success.ContentType) + assert.False(t, res.Success.Schema.IsZero(), + "the default response's schema must drive the success type") + + // Default must NOT also be installed as a 500 error in this case. + _, has500 := res.All[500] + assert.False(t, has500, "default was promoted to success; it must not also appear as 500") + + // No fabricated 204. + _, has204 := res.All[204] + assert.False(t, has204) + }) + + t.Run("default response stays as 500 error when explicit success is documented", func(t *testing.T) { + contents := []byte(` +openapi: "3.0.0" +info: + version: 1.0.0 + title: Test +paths: + /ping: + get: + operationId: ping + responses: + '200': + description: ok + content: + application/json: + schema: + type: object + properties: + ok: + type: boolean + default: + description: error + content: + application/json: + schema: + type: object + properties: + message: + type: string +`) + op, opts := loadOp(t, contents, "/ping", "get") + res, _, err := getOperationResponses("ping", op.Responses, opts) + require.NoError(t, err) + + assert.Equal(t, 200, res.SuccessStatusCode) + require.NotNil(t, res.Success) + assert.True(t, res.Success.IsSuccess) + + require.NotNil(t, res.Error, "default must be installed as the fallback error") + assert.Equal(t, 500, res.Error.StatusCode) + assert.False(t, res.Error.IsSuccess) + }) + + t.Run("default response without content falls back to fabricated 204 success", func(t *testing.T) { + contents := []byte(` +openapi: "3.0.0" +info: + version: 1.0.0 + title: Test +paths: + /ping: + get: + operationId: ping + responses: + default: + description: any +`) + op, opts := loadOp(t, contents, "/ping", "get") + res, _, err := getOperationResponses("ping", op.Responses, opts) + require.NoError(t, err) + + // No content on default means there's nothing to promote to success; + // the 204 fallback kicks in. + assert.Equal(t, 204, res.SuccessStatusCode) + require.NotNil(t, res.Success) + assert.Equal(t, "struct{}", res.Success.ResponseName) + }) + + t.Run("empty success body preserves declared ContentType", func(t *testing.T) { + contents := []byte(` +openapi: "3.0.0" +info: + version: 1.0.0 + title: Test +paths: + /page: + get: + operationId: getPage + responses: + '200': + description: html + content: + text/html: {} +`) + op, opts := loadOp(t, contents, "/page", "get") + res, _, err := getOperationResponses("getPage", op.Responses, opts) + require.NoError(t, err) + + require.NotNil(t, res.Success) + assert.Equal(t, 200, res.Success.StatusCode) + assert.Equal(t, "struct{}", res.Success.ResponseName, + "no schema = no decoded body, so the response is struct{}") + assert.Equal(t, "text/html", res.Success.ContentType, + "the declared media type must survive even when the body schema is empty") + }) + + t.Run("default with content promoted to success generates a Response type, not an ErrorResponse", func(t *testing.T) { + contents := []byte(` +openapi: "3.0.0" +info: + version: 1.0.0 + title: Test +paths: + /ping: + get: + operationId: ping + responses: + default: + description: any + content: + application/json: + schema: + type: object + properties: + message: + type: string +`) + op, opts := loadOp(t, contents, "/ping", "get") + res, typeDefs, err := getOperationResponses("ping", op.Responses, opts) + require.NoError(t, err) + require.NotNil(t, res.Success) + + // The success-side name suffix is "Response", not "ErrorResponse". + assert.Equal(t, "pingResponse", res.Success.ResponseName) + + var names []string + for _, td := range typeDefs { + names = append(names, td.Name) + } + assert.Contains(t, names, "pingResponse") + assert.NotContains(t, names, "pingErrorResponse", + "when default is promoted to success, no parallel ErrorResponse type should be emitted") + }) +} From 8d4500c021f7bbf16e3e30ffbd85de04780c8d0e Mon Sep 17 00:00:00 2001 From: Hittrich Date: Wed, 20 May 2026 21:05:15 +0200 Subject: [PATCH 2/3] adapter template --- pkg/codegen/templates/handler/adapter.tmpl | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/codegen/templates/handler/adapter.tmpl b/pkg/codegen/templates/handler/adapter.tmpl index 3f731822..84442f3e 100644 --- a/pkg/codegen/templates/handler/adapter.tmpl +++ b/pkg/codegen/templates/handler/adapter.tmpl @@ -546,7 +546,10 @@ func (a *HTTPAdapter) {{ $op.ID | ucFirst }}(w http.ResponseWriter, r *http.Requ if w.Header().Get("Content-Type") == "" { w.Header().Set("Content-Type", "{{ escapeGoString $op.Response.Success.ContentType }}") } - {{- if or (eq $op.Response.Success.ContentType "application/json") (hasPrefix $op.Response.Success.ContentType "application/json;") (hasSuffix $op.Response.Success.ContentType "+json") (contains $op.Response.Success.ContentType "+json;") }} + {{- if eq $op.Response.Success.ResponseName "struct{}" }} + {{/* Schema-less body (e.g. `text/html: {}`): declare the content type but write no body. */}} + w.WriteHeader(status) + {{- else if or (eq $op.Response.Success.ContentType "application/json") (hasPrefix $op.Response.Success.ContentType "application/json;") (hasSuffix $op.Response.Success.ContentType "+json") (contains $op.Response.Success.ContentType "+json;") }} w.WriteHeader(status) if resp != nil && resp.Body != nil { _ = json.NewEncoder(w).Encode(resp.Body) From 0326f7cd2ef5cbc00c3f487f0c8a3ac7fc9875d7 Mon Sep 17 00:00:00 2001 From: Hittrich Date: Wed, 20 May 2026 21:44:31 +0200 Subject: [PATCH 3/3] raw content type --- pkg/codegen/templates/handler/service.tmpl | 12 +++++++- pkg/codegen/typedef_responses.go | 35 ++++++++++++++++++++-- 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/pkg/codegen/templates/handler/service.tmpl b/pkg/codegen/templates/handler/service.tmpl index 21e8c73e..cb8a112a 100644 --- a/pkg/codegen/templates/handler/service.tmpl +++ b/pkg/codegen/templates/handler/service.tmpl @@ -78,7 +78,17 @@ func ({{ $receiver }} *{{ $serviceName }}) {{ $op.ID }}(ctx context.Context, opt func ({{ $receiver }} *{{ $serviceName }}) {{ $op.ID }}(ctx context.Context) ({{ if $op.Response.Success }}*{{ $modelsPrefix }}{{ $op.ID | ucFirst }}ResponseData, error{{ else }}error{{ end }}) { // TODO: Implement your business logic here {{- if $op.Response.Success }} - return {{ $modelsPrefix }}New{{ $op.ID | ucFirst }}ResponseData(new({{ $modelsPrefix }}{{ $op.Response.Success.ResponseName }})), nil + {{- if $op.Response.Success.IsRaw }} + return {{ $modelsPrefix }}New{{ $op.ID | ucFirst }}ResponseData([]byte("TODO: marshal response")), nil + {{- else }} + {{- $bodyType := $op.Response.Success.ResponseName -}} + {{- if eq $bodyType "struct{}" }} + return {{ $modelsPrefix }}New{{ $op.ID | ucFirst }}ResponseData(nil), nil + {{- else }} + {{- if $modelsPrefix }}{{ $bodyType = printf "%s%s" $modelsPrefix $bodyType }}{{ end }} + return {{ $modelsPrefix }}New{{ $op.ID | ucFirst }}ResponseData(new({{ $bodyType }})), nil + {{- end }} + {{- end }} {{- else }} return nil {{- end }} diff --git a/pkg/codegen/typedef_responses.go b/pkg/codegen/typedef_responses.go index 6df1b0ac..29995e74 100644 --- a/pkg/codegen/typedef_responses.go +++ b/pkg/codegen/typedef_responses.go @@ -517,6 +517,20 @@ func buildDefaultResponseDefinition(operationID string, defaultResponse *v3high. return nil, nil, nil } + // Coerce raw content types (XML, CSV, */*, etc.) to []byte. Mirrors the + // per-status path; without it, a `default` response with a non-JSON + // content type would emit a typed Body that the handler template's + // "unknown content type" branch then tries to w.Write as []byte. + isRaw := isRawContentType(contentType) + if isRaw { + contentSchema = GoSchema{ + GoType: "[]byte", + DefineViaAlias: true, + Description: contentSchema.Description, + } + refType = "" + } + if refType != "" { contentSchema.RefType = refType } @@ -524,6 +538,16 @@ func buildDefaultResponseDefinition(operationID string, defaultResponse *v3high. if contentSchema.ArrayType != nil { contentSchema, _ = replaceInlineTypes(contentSchema, options) } + // When the default schema $refs a component (e.g. an error/event struct) + // AND the content type forces us to []byte, the operation-level alias + // name can collide with the component type. The component is already + // registered as a struct; registering a []byte alias under the same + // name silently drops it and the client template still ends up with + // `result := SomeStruct(bodyBytes)`. Disambiguate the alias name so + // both types co-exist. + if isRaw && options.typeTracker.Exists(responseName) { + responseName = options.typeTracker.generateUniqueName(responseName) + } var typeDefinitions []TypeDefinition td := TypeDefinition{ Name: responseName, @@ -555,6 +579,7 @@ func buildDefaultResponseDefinition(operationID string, defaultResponse *v3high. ContentType: contentType, StatusCode: status, Headers: headers, + IsRaw: isRaw, } return rcd, typeDefinitions, nil } @@ -597,12 +622,18 @@ func generateResponseHeadersSchema(headers iter.Seq2[string, *v3high.Header], op } // isRawContentType returns true for content types that require manual marshaling -// (XML, YAML, etc.) and should use []byte as the response type. +// (XML, YAML, ndjson, etc.) and should use []byte as the response type. +// +// Note: only `application/json` and `*+json` are JSON-encodable as a single +// object. ndjson/jsonl variants are recognized by `isMediaTypeJson` for +// naming/tagging purposes, but they need raw byte framing on the wire, so +// they fall through to []byte here. func isRawContentType(contentType string) bool { return contentType != "" && contentType != "application/json" && !strings.HasPrefix(contentType, "application/json;") && - !isMediaTypeJson(contentType) && + !strings.HasSuffix(contentType, "+json") && + !strings.Contains(contentType, "+json;") && contentType != "text/plain" && !strings.HasPrefix(contentType, "text/plain;") && contentType != "text/html" &&