fix: only treat schemas on a reference loop as circular

datamodel/low/base/circ_check.go

@@ -3,14 +3,24 @@
 
 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.
 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
+}

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

test_specs/circular_cross_file/mod/a.yaml

@@ -0,0 +1,5 @@
+A:
+  type: object
+  properties:
+    b:
+      $ref: b.yaml#/B

test_specs/circular_cross_file/mod/b.yaml

@@ -0,0 +1,5 @@
+B:
+  type: object
+  properties:
+    a:
+      $ref: a.yaml#/A

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

test_specs/circular_cross_file/orig/a.yaml

@@ -0,0 +1,5 @@
+A:
+  type: object
+  properties:
+    b:
+      $ref: b.yaml#/B

test_specs/circular_cross_file/orig/b.yaml

@@ -0,0 +1,5 @@
+B:
+  type: object
+  properties:
+    a:
+      $ref: a.yaml#/A

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