From b873701d9a3242f4a9959f6e71240cba12b44150 Mon Sep 17 00:00:00 2001 From: Joseph Axisa Date: Tue, 20 Oct 2026 14:33:41 -0700 Subject: [PATCH] fix: only treat schemas on a reference loop as circular CheckSchemaProxyForCircularRefs returned true for every schema proxy as soon as a document had one circular reference: the journey check kept a single dup map across all results, and every journey repeats its loop point. Inline schemas then hashed to 0 with UseSchemaQuickHash, so their changes were skipped, and CompareSchemas dropped $ref changes. Check membership of the loop instead: the part of each journey from the loop point on (all of it when the loop point only ends the journey, as at the resolver's depth limit), matched by the node the reference resolves to, since a relative reference such as 'b.yaml#/B' never matches the journey's absolute definitions. Inline schemas are not references and are never circular. Fixes #662 --- datamodel/low/base/circ_check.go | 53 ++++++- datamodel/low/base/circ_check_test.go | 141 ++++++++++++++++++ test_specs/circular_cross_file/mod/a.yaml | 5 + test_specs/circular_cross_file/mod/b.yaml | 5 + test_specs/circular_cross_file/mod/root.yaml | 26 ++++ test_specs/circular_cross_file/orig/a.yaml | 5 + test_specs/circular_cross_file/orig/b.yaml | 5 + test_specs/circular_cross_file/orig/root.yaml | 26 ++++ what-changed/model/schema_test.go | 98 ++++++++++++ what-changed/what_changed_test.go | 77 ++++++++++ 10 files changed, 434 insertions(+), 7 deletions(-) create mode 100644 test_specs/circular_cross_file/mod/a.yaml create mode 100644 test_specs/circular_cross_file/mod/b.yaml create mode 100644 test_specs/circular_cross_file/mod/root.yaml create mode 100644 test_specs/circular_cross_file/orig/a.yaml create mode 100644 test_specs/circular_cross_file/orig/b.yaml create mode 100644 test_specs/circular_cross_file/orig/root.yaml diff --git a/datamodel/low/base/circ_check.go b/datamodel/low/base/circ_check.go index 3f425224..099dfbda 100644 --- a/datamodel/low/base/circ_check.go +++ b/datamodel/low/base/circ_check.go @@ -3,7 +3,14 @@ package base -import "slices" +import ( + "slices" + + "github.com/pb33f/go-yaml" + "github.com/pb33f/libopenapi/datamodel/low" + "github.com/pb33f/libopenapi/index" + "github.com/pb33f/libopenapi/utils" +) // CheckSchemaProxyForCircularRefs checks if the provided SchemaProxy has any circular references, extracted from // The rolodex attached to the index. @@ -11,6 +18,9 @@ func CheckSchemaProxyForCircularRefs(s *SchemaProxy) bool { if s.GetIndex() == nil || s.GetIndex().GetRolodex() == nil { return false // no index or rolodex, so no circular references } + if !s.IsReference() { + return false // an inline schema is not a reference, so it cannot be a point on a reference loop. + } rolo := s.GetIndex().GetRolodex() allCircs := rolo.GetRootIndex().GetCircularReferences() safeCircularRefs := rolo.GetSafeCircularReferences() @@ -21,19 +31,48 @@ func CheckSchemaProxyForCircularRefs(s *SchemaProxy) bool { combinedCircularRefs := slices.Clone(safeCircularRefs) combinedCircularRefs = append(combinedCircularRefs, ignoredCircularRefs...) combinedCircularRefs = append(combinedCircularRefs, allCircs...) - dup := make(map[string]struct{}) + var target *yaml.Node + resolved := false for _, ref := range combinedCircularRefs { // hash the root node of the schema reference if ref.LoopPoint.FullDefinition == s.GetReference() || ref.LoopPoint.Definition == s.GetReference() { return true } - // check journey, if we have any duplicated - for _, ji := range ref.Journey { - if _, exists := dup[ji.FullDefinition]; exists { - return true // this has already been checked, it's a loop. + // check the loop itself: a journey starts with the path that leads into the loop, and the schemas on + // that path are not circular. a loop point that only appears at the end (the resolver's depth limit) + // leaves no way to tell the two apart, so the whole journey counts. + loop := ref.Journey + if start := slices.IndexFunc(loop, func(ji *index.Reference) bool { + return ji.FullDefinition == ref.LoopPoint.FullDefinition + }); start >= 0 && start < len(loop)-1 { + loop = loop[start:] + } + for _, ji := range loop { + if ji.FullDefinition == s.GetReference() { + return true + } + // journey entries hold absolute definitions, which a relative reference such as 'b.yaml#/B' never + // matches as written, so compare the node the reference resolves to. + if ji.Node == nil { + continue + } + if !resolved { + target, resolved = schemaProxyTarget(s), true + } + if target != nil && utils.NodeAlias(ji.Node) == target { + return true } - dup[ji.FullDefinition] = struct{}{} } } return false } + +// schemaProxyTarget returns the node the proxy's reference resolves to, or nil if it cannot be found. +func schemaProxyTarget(s *SchemaProxy) *yaml.Node { + node := s.GetValueNode() + if isRef, _, _ := utils.IsNodeRefValue(node); !isRef { + return utils.NodeAlias(node) // child schema proxies are built from the node their reference already resolved to. + } + node, _, _, _ = low.LocateRefNodeWithContext(s.GetContext(), node, s.GetIndex()) + return node +} diff --git a/datamodel/low/base/circ_check_test.go b/datamodel/low/base/circ_check_test.go index 84cc7671..365183cb 100644 --- a/datamodel/low/base/circ_check_test.go +++ b/datamodel/low/base/circ_check_test.go @@ -9,6 +9,7 @@ import ( "github.com/pb33f/libopenapi/datamodel/low" "github.com/pb33f/libopenapi/index" "github.com/pb33f/testify/assert" + "github.com/pb33f/testify/require" "testing" ) @@ -87,3 +88,143 @@ func TestCheckSchemaProxyForCircularRefs_JourneyCheck(t *testing.T) { assert.True(t, CheckSchemaProxyForCircularRefs(schema)) // no rolodex yet. } + +func TestCheckSchemaProxyForCircularRefs_JourneyCheck_LeadIn(t *testing.T) { + + rolo := index.NewRolodex(&index.SpecIndexConfig{}) + dummyNode := &yaml.Node{Content: []*yaml.Node{{Content: []*yaml.Node{}}}} + + ref := low.Reference{} + ref.SetReference("minty-fresh", dummyNode) + + schema := &SchemaProxy{ + Reference: ref, + } + rootIndex := index.NewSpecIndex(dummyNode) + _ = schema.Build(context.Background(), dummyNode, dummyNode, rootIndex) + + rootIndex.SetRolodex(rolo) + rolo.SetRootNode(dummyNode) + rolo.SetRootIndex(rootIndex) + rolo.SetSafeCircularReferences([]*index.CircularReferenceResult{ + { + LoopPoint: &index.Reference{ + FullDefinition: "tasty-burger", + }, + Journey: []*index.Reference{ + { + FullDefinition: "minty-fresh", + }, + { + FullDefinition: "tasty-burger", + }, + { + FullDefinition: "salty-fries", + }, + { + FullDefinition: "tasty-burger", + }, + }, + }, + }) + + assert.False(t, CheckSchemaProxyForCircularRefs(schema)) // only leads into the loop. +} + +func TestCheckSchemaProxyForCircularRefs_JourneyCheck_DepthLimit(t *testing.T) { + + rolo := index.NewRolodex(&index.SpecIndexConfig{}) + dummyNode := &yaml.Node{Content: []*yaml.Node{{Content: []*yaml.Node{}}}} + + ref := low.Reference{} + ref.SetReference("minty-fresh", dummyNode) + + schema := &SchemaProxy{ + Reference: ref, + } + rootIndex := index.NewSpecIndex(dummyNode) + _ = schema.Build(context.Background(), dummyNode, dummyNode, rootIndex) + + rootIndex.SetRolodex(rolo) + rolo.SetRootNode(dummyNode) + rolo.SetRootIndex(rootIndex) + rolo.SetSafeCircularReferences([]*index.CircularReferenceResult{ + { + LoopPoint: &index.Reference{ + FullDefinition: "tasty-burger", + }, + Journey: []*index.Reference{ + { + FullDefinition: "minty-fresh", + }, + { + FullDefinition: "tasty-burger", + }, + }, + }, + }) + + assert.True(t, CheckSchemaProxyForCircularRefs(schema)) // the loop point only ends the journey, so all of it counts. +} + +func TestCheckSchemaProxyForCircularRefs_ResolvesReference(t *testing.T) { + yml := `openapi: 3.1.0 +components: + schemas: + Pet: + type: object + A: + type: object + properties: + b: + $ref: '#/components/schemas/B' + B: + type: object + properties: + a: + $ref: '#/components/schemas/A'` + + var rootNode yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(yml), &rootNode)) + config := index.CreateOpenAPIIndexConfig() + config.SpecAbsolutePath = "/specs/root.yaml" // journey entries hold absolute definitions, as in a real document. + rolo := index.NewRolodex(config) + rolo.SetRootNode(&rootNode) + require.NoError(t, rolo.IndexTheRolodex(context.Background())) + rolo.CheckForCircularReferences() + idx := rolo.GetRootIndex() + require.NotEmpty(t, idx.GetCircularReferences()) + + // a proxy built from a reference node, as written in the document. + proxy := func(ref string) *SchemaProxy { + var node yaml.Node + _ = yaml.Unmarshal([]byte(`$ref: '`+ref+`'`), &node) + sp := new(SchemaProxy) + _ = sp.Build(context.Background(), nil, node.Content[0], idx) + return sp + } + + // whichever schema is the loop point, the other is found by resolving the reference. + assert.True(t, CheckSchemaProxyForCircularRefs(proxy("#/components/schemas/A"))) + assert.True(t, CheckSchemaProxyForCircularRefs(proxy("#/components/schemas/B"))) + assert.False(t, CheckSchemaProxyForCircularRefs(proxy("#/components/schemas/Pet"))) + assert.False(t, CheckSchemaProxyForCircularRefs(proxy("#/components/schemas/Missing"))) + + // a child schema proxy holds the node its reference already resolved to. + property := func(schema, name string) *SchemaProxy { + node := idx.FindComponent(context.Background(), "#/components/schemas/"+schema).Node + var sch Schema + _ = low.BuildModel(node, &sch) + _ = sch.Build(context.Background(), node, idx) + return sch.FindProperty(name).Value + } + assert.True(t, CheckSchemaProxyForCircularRefs(property("A", "b"))) + assert.True(t, CheckSchemaProxyForCircularRefs(property("B", "a"))) + + // an inline schema is not a reference. + var inline yaml.Node + _ = yaml.Unmarshal([]byte(`type: object`), &inline) + sp := new(SchemaProxy) + _ = sp.Build(context.Background(), nil, inline.Content[0], idx) + assert.False(t, CheckSchemaProxyForCircularRefs(sp)) +} diff --git a/test_specs/circular_cross_file/mod/a.yaml b/test_specs/circular_cross_file/mod/a.yaml new file mode 100644 index 00000000..4f9d6847 --- /dev/null +++ b/test_specs/circular_cross_file/mod/a.yaml @@ -0,0 +1,5 @@ +A: + type: object + properties: + b: + $ref: b.yaml#/B diff --git a/test_specs/circular_cross_file/mod/b.yaml b/test_specs/circular_cross_file/mod/b.yaml new file mode 100644 index 00000000..21b2bc4a --- /dev/null +++ b/test_specs/circular_cross_file/mod/b.yaml @@ -0,0 +1,5 @@ +B: + type: object + properties: + a: + $ref: a.yaml#/A diff --git a/test_specs/circular_cross_file/mod/root.yaml b/test_specs/circular_cross_file/mod/root.yaml new file mode 100644 index 00000000..237766e7 --- /dev/null +++ b/test_specs/circular_cross_file/mod/root.yaml @@ -0,0 +1,26 @@ +openapi: 3.1.0 +info: + title: Cross file circular references + version: 1.0.0 +paths: + /nodes: + get: + responses: + "200": + description: A node + content: + application/json: + schema: + $ref: a.yaml#/A + /pets: + get: + responses: + "200": + description: A pet + content: + application/json: + schema: + type: object + properties: + name: + type: integer diff --git a/test_specs/circular_cross_file/orig/a.yaml b/test_specs/circular_cross_file/orig/a.yaml new file mode 100644 index 00000000..4f9d6847 --- /dev/null +++ b/test_specs/circular_cross_file/orig/a.yaml @@ -0,0 +1,5 @@ +A: + type: object + properties: + b: + $ref: b.yaml#/B diff --git a/test_specs/circular_cross_file/orig/b.yaml b/test_specs/circular_cross_file/orig/b.yaml new file mode 100644 index 00000000..21b2bc4a --- /dev/null +++ b/test_specs/circular_cross_file/orig/b.yaml @@ -0,0 +1,5 @@ +B: + type: object + properties: + a: + $ref: a.yaml#/A diff --git a/test_specs/circular_cross_file/orig/root.yaml b/test_specs/circular_cross_file/orig/root.yaml new file mode 100644 index 00000000..7f03e4d0 --- /dev/null +++ b/test_specs/circular_cross_file/orig/root.yaml @@ -0,0 +1,26 @@ +openapi: 3.1.0 +info: + title: Cross file circular references + version: 1.0.0 +paths: + /nodes: + get: + responses: + "200": + description: A node + content: + application/json: + schema: + $ref: a.yaml#/A + /pets: + get: + responses: + "200": + description: A pet + content: + application/json: + schema: + type: object + properties: + name: + type: string diff --git a/what-changed/model/schema_test.go b/what-changed/model/schema_test.go index 9c16ff2e..f61968f9 100644 --- a/what-changed/model/schema_test.go +++ b/what-changed/model/schema_test.go @@ -7,6 +7,7 @@ import ( "context" "fmt" "reflect" + "strings" "sync" "sync/atomic" "testing" @@ -7421,3 +7422,100 @@ components: // TotalBreakingChanges should count the vocabulary change assert.Equal(t, 1, changes.TotalBreakingChanges()) } + +// https://github.com/pb33f/libopenapi/issues/662 +func TestCompareSchemas_RefToInline_UnrelatedCircularReference(t *testing.T) { + low.ClearHashCache() + left := `openapi: 3.0 +components: + schemas: + Pet: + type: object + properties: + name: + type: string + Owner: + type: object + properties: + pet: + $ref: '#/components/schemas/Pet' + A: + type: object + properties: + b: + $ref: '#/components/schemas/B' + B: + type: object + properties: + a: + $ref: '#/components/schemas/A'` + + right := strings.Replace(left, ` pet: + $ref: '#/components/schemas/Pet'`, ` pet: + type: object + properties: + name: + type: string + required: [name]`, 1) + + leftDoc, rightDoc := test_BuildDoc(left, right) + + lSchemaProxy := leftDoc.Components.Value.FindSchema("Owner").Value + rSchemaProxy := rightDoc.Components.Value.FindSchema("Owner").Value + + changes := CompareSchemas(lSchemaProxy, rSchemaProxy) + assert.NotNil(t, changes) + petChanges := changes.SchemaPropertyChanges["pet"] + assert.NotNil(t, petChanges) + assert.Equal(t, v3.RefLabel, petChanges.Changes[0].Property) + assert.Equal(t, "#/components/schemas/Pet", petChanges.Changes[0].OriginalObject) +} + +// https://github.com/pb33f/libopenapi/issues/662 +func TestCheckSchemaProxyForCircularRefs_OnlyLoopMembers(t *testing.T) { + low.ClearHashCache() + spec := `openapi: 3.0 +components: + schemas: + Pet: + type: object + Owner: + type: object + properties: + pet: + $ref: '#/components/schemas/Pet' + start: + $ref: '#/components/schemas/Start' + Start: + type: object + properties: + a: + $ref: '#/components/schemas/A' + A: + type: object + properties: + b: + $ref: '#/components/schemas/B' + B: + type: object + properties: + a: + $ref: '#/components/schemas/A'` + + doc, _ := test_BuildDoc(spec, spec) + property := func(schema, name string) *base.SchemaProxy { + return doc.Components.Value.FindSchema(schema).Value.Schema().FindProperty(name).Value + } + + // on the loop + assert.True(t, base.CheckSchemaProxyForCircularRefs(property("A", "b"))) + assert.True(t, base.CheckSchemaProxyForCircularRefs(property("B", "a"))) + assert.True(t, base.CheckSchemaProxyForCircularRefs(property("Start", "a"))) + + // not on the loop: unrelated, or only on the way into it + assert.False(t, base.CheckSchemaProxyForCircularRefs(property("Owner", "pet"))) + assert.False(t, base.CheckSchemaProxyForCircularRefs(property("Owner", "start"))) + + // inline schemas are not references + assert.False(t, base.CheckSchemaProxyForCircularRefs(doc.Components.Value.FindSchema("Owner").Value)) +} diff --git a/what-changed/what_changed_test.go b/what-changed/what_changed_test.go index 87c9d8e1..5d76f948 100644 --- a/what-changed/what_changed_test.go +++ b/what-changed/what_changed_test.go @@ -6,12 +6,14 @@ package what_changed import ( "fmt" "os" + "strings" "testing" "github.com/pb33f/libopenapi/datamodel" v2 "github.com/pb33f/libopenapi/datamodel/low/v2" v3 "github.com/pb33f/libopenapi/datamodel/low/v3" "github.com/pb33f/testify/assert" + "github.com/pb33f/testify/require" ) func TestCompareOpenAPIDocuments(t *testing.T) { @@ -311,3 +313,78 @@ func TestCheckExplodedFileCheck_IdenticalRefNames(t *testing.T) { assert.Len(t, allChanges, 1) } + +// https://github.com/pb33f/libopenapi/issues/662 +func TestCompareOpenAPIDocuments_UnrelatedCircularReference(t *testing.T) { + original := `openapi: 3.0.3 +info: + title: Pets + version: "1" +paths: {} +components: + schemas: + Owner: + type: object + properties: + pet: + type: object + properties: + name: + type: string + A: + type: object + properties: + b: + $ref: '#/components/schemas/B' + c: + $ref: '#/components/schemas/C' + B: + type: object + properties: + a: + $ref: '#/components/schemas/A' + C: + type: object + properties: + a: + $ref: '#/components/schemas/A'` + modified := strings.Replace(original, "name:\n type: string", "name:\n type: integer", 1) + + config := datamodel.NewDocumentConfiguration() + config.UseSchemaQuickHash = true + infoOrig, _ := datamodel.ExtractSpecInfo([]byte(original)) + infoMod, _ := datamodel.ExtractSpecInfo([]byte(modified)) + origDoc, err := v3.CreateDocumentFromConfig(infoOrig, config) + require.NoError(t, err) + modDoc, err := v3.CreateDocumentFromConfig(infoMod, config) + require.NoError(t, err) + + // two loops share A, and only one of them is recorded: hashing C still stops at A. + changes := CompareOpenAPIDocuments(origDoc, modDoc) + require.NotNil(t, changes) + assert.Equal(t, 1, changes.TotalChanges()) + assert.Equal(t, 1, changes.TotalBreakingChanges()) +} + +// https://github.com/pb33f/libopenapi/issues/662 +func TestCompareOpenAPIDocuments_CrossFileCircularReference(t *testing.T) { + load := func(dir string) *v3.Document { + spec, err := os.ReadFile(dir + "/root.yaml") + require.NoError(t, err) + info, err := datamodel.ExtractSpecInfo(spec) + require.NoError(t, err) + doc, err := v3.CreateDocumentFromConfig(info, &datamodel.DocumentConfiguration{ + BasePath: dir, + AllowFileReferences: true, + UseSchemaQuickHash: true, + }) + require.NoError(t, err) + return doc + } + + // hashing has to stop at the loop, although the relative references don't match the journey as written. + changes := CompareOpenAPIDocuments(load("../test_specs/circular_cross_file/orig"), load("../test_specs/circular_cross_file/mod")) + require.NotNil(t, changes) + assert.Equal(t, 1, changes.TotalChanges()) + assert.Equal(t, 1, changes.TotalBreakingChanges()) +}