Skip to content
4 changes: 4 additions & 0 deletions internal/test/issues/issue-1373/config.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
package: issue1373
generate:
models: true
output: issue.gen.go
3 changes: 3 additions & 0 deletions internal/test/issues/issue-1373/generate.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
package issue1373

//go:generate go run github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen --config=config.yaml spec.yaml
15 changes: 15 additions & 0 deletions internal/test/issues/issue-1373/issue.gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

32 changes: 32 additions & 0 deletions internal/test/issues/issue-1373/spec.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
openapi: 3.0.2
info:
version: '0.0.1'
title: example
description: |
Make sure that recursive $ref in allOf are handled properly
paths:
/example:
get:
operationId: exampleGet
responses:
'200':
description: "OK"
content:
'application/json':
schema:
$ref: '#/components/schemas/RecursiveObject'

components:
schemas:
RecursiveObject:
allOf:
- $ref: "#/components/schemas/NonRecursiveObject"
- $ref: "#/components/schemas/RecursiveObject"
- properties:
FieldInRecursive::
type: string

NonRecursiveObject:
properties:
FieldInNonRecursive:
type: string
23 changes: 17 additions & 6 deletions pkg/codegen/merge_schemas.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,12 @@ func mergeSchemas(allOf []*openapi3.SchemaRef, path []string) (Schema, error) {
if err != nil {
return Schema{}, err
}
schema, err = mergeOpenapiSchemas(schema, oneOfSchema, true)

seenSchemaRef := make(map[string]bool)
if allOf[i].Ref != "" {
seenSchemaRef[allOf[i].Ref] = true
}
schema, err = mergeOpenapiSchemas(schema, oneOfSchema, true, seenSchemaRef)
Comment thread
mromaszewicz marked this conversation as resolved.
Comment on lines +42 to +46

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 allOf[0] self-ref not seeded into seenSchemaRef

allOf[0]'s ref is never added to any seenSchemaRef map before the loop. Inside mergeOpenapiSchemas, s1 is derived from allOf[0].Value, and if s1.AllOf contains a ref back to the same schema (i.e., the recursive self-ref is placed first in allOf), mergeAllOf(s1.AllOf, seenSchemaRef) won't detect the cycle — the ref isn't in the map — and will stack overflow again.

Concretely, a spec like the following would still crash:

RecursiveObject:
  allOf:
    - $ref: "#/components/schemas/RecursiveObject"   # self-ref is allOf[0]
    - $ref: "#/components/schemas/NonRecursiveObject"

Seeding allOf[0].Ref into an initial map before iterating would close this gap:

seenTopLevel := make(map[string]bool)
if allOf[0].Ref != "" {
    seenTopLevel[allOf[0].Ref] = true
}
for i := 1; i < n; i++ {
    seenSchemaRef := make(map[string]bool)
    for k, v := range seenTopLevel { seenSchemaRef[k] = v }
    if allOf[i].Ref != "" {
        seenSchemaRef[allOf[i].Ref] = true
        seenTopLevel[allOf[i].Ref] = true
    }
    ...
}

if err != nil {
return Schema{}, fmt.Errorf("error merging schemas for AllOf: %w", err)
}
Expand Down Expand Up @@ -71,11 +76,17 @@ func valueWithPropagatedRef(ref *openapi3.SchemaRef) (openapi3.Schema, error) {
return schema, nil
}

func mergeAllOf(allOf []*openapi3.SchemaRef) (openapi3.Schema, error) {
func mergeAllOf(allOf []*openapi3.SchemaRef, seenSchemaRef map[string]bool) (openapi3.Schema, error) {
var schema openapi3.Schema
for _, schemaRef := range allOf {
var err error
schema, err = mergeOpenapiSchemas(schema, *schemaRef.Value, true)
if schemaRef.Ref != "" && seenSchemaRef[schemaRef.Ref] {
continue
}
if schemaRef.Ref != "" {
seenSchemaRef[schemaRef.Ref] = true
}
schema, err = mergeOpenapiSchemas(schema, *schemaRef.Value, true, seenSchemaRef)
if err != nil {
return openapi3.Schema{}, fmt.Errorf("error merging schemas for AllOf: %w", err)
}
Expand All @@ -85,7 +96,7 @@ func mergeAllOf(allOf []*openapi3.SchemaRef) (openapi3.Schema, error) {

// mergeOpenapiSchemas merges two openAPI schemas and returns the schema
// all of whose fields are composed.
func mergeOpenapiSchemas(s1, s2 openapi3.Schema, allOf bool) (openapi3.Schema, error) {
func mergeOpenapiSchemas(s1, s2 openapi3.Schema, allOf bool, seenSchemaRef map[string]bool) (openapi3.Schema, error) {
var result openapi3.Schema

result.Extensions = make(map[string]any, len(s1.Extensions)+len(s2.Extensions))
Expand All @@ -100,15 +111,15 @@ func mergeOpenapiSchemas(s1, s2 openapi3.Schema, allOf bool) (openapi3.Schema, e
var err error
if s1.AllOf != nil {
var merged openapi3.Schema
merged, err = mergeAllOf(s1.AllOf)
merged, err = mergeAllOf(s1.AllOf, seenSchemaRef)
if err != nil {
return openapi3.Schema{}, fmt.Errorf("error transitive merging AllOf on schema 1")
}
s1 = merged
}
if s2.AllOf != nil {
var merged openapi3.Schema
merged, err = mergeAllOf(s2.AllOf)
merged, err = mergeAllOf(s2.AllOf, seenSchemaRef)
if err != nil {
return openapi3.Schema{}, fmt.Errorf("error transitive merging AllOf on schema 2")
}
Expand Down
12 changes: 6 additions & 6 deletions pkg/codegen/merge_schemas_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ func TestMergeOpenapiSchemas_DiscriminatorPropagation(t *testing.T) {
s1 := openapi3.Schema{Discriminator: disc}
s2 := openapi3.Schema{}

result, err := mergeOpenapiSchemas(s1, s2, true)
result, err := mergeOpenapiSchemas(s1, s2, true, make(map[string]bool))
require.NoError(t, err)
assert.Equal(t, disc, result.Discriminator)
})
Expand All @@ -26,7 +26,7 @@ func TestMergeOpenapiSchemas_DiscriminatorPropagation(t *testing.T) {
s1 := openapi3.Schema{}
s2 := openapi3.Schema{Discriminator: disc}

result, err := mergeOpenapiSchemas(s1, s2, true)
result, err := mergeOpenapiSchemas(s1, s2, true, make(map[string]bool))
require.NoError(t, err)
assert.Equal(t, disc, result.Discriminator)
})
Expand All @@ -36,7 +36,7 @@ func TestMergeOpenapiSchemas_DiscriminatorPropagation(t *testing.T) {
s1 := openapi3.Schema{Discriminator: disc}
s2 := openapi3.Schema{Discriminator: disc2}

_, err := mergeOpenapiSchemas(s1, s2, true)
_, err := mergeOpenapiSchemas(s1, s2, true, make(map[string]bool))
require.Error(t, err)
assert.Contains(t, err.Error(), "discriminators")
})
Expand All @@ -45,7 +45,7 @@ func TestMergeOpenapiSchemas_DiscriminatorPropagation(t *testing.T) {
s1 := openapi3.Schema{}
s2 := openapi3.Schema{}

result, err := mergeOpenapiSchemas(s1, s2, true)
result, err := mergeOpenapiSchemas(s1, s2, true, make(map[string]bool))
require.NoError(t, err)
assert.Nil(t, result.Discriminator)
})
Expand All @@ -54,15 +54,15 @@ func TestMergeOpenapiSchemas_DiscriminatorPropagation(t *testing.T) {
s1 := openapi3.Schema{Discriminator: disc}
s2 := openapi3.Schema{}

_, err := mergeOpenapiSchemas(s1, s2, false)
_, err := mergeOpenapiSchemas(s1, s2, false, make(map[string]bool))
require.Error(t, err)
})

t.Run("non-allOf with discriminator on s2 errors", func(t *testing.T) {
s1 := openapi3.Schema{}
s2 := openapi3.Schema{Discriminator: disc}

_, err := mergeOpenapiSchemas(s1, s2, false)
_, err := mergeOpenapiSchemas(s1, s2, false, make(map[string]bool))
require.Error(t, err)
})
}
Loading