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()) +}