fix: compare the schema behind a changed $ref

index/rolodex.go

@@ -247,6 +247,11 @@ func (r *Rolodex) GetIndexingDuration() time.Duration {
 	return r.indexingDuration
 }
 
+// HasCheckedForCircularReferences returns true once the rolodex has been checked for circular references.
+func (r *Rolodex) HasCheckedForCircularReferences() bool {
+	return r.circChecked
+}
+
 // GetRootIndex returns the root index of the rolodex (the entry point, the main document)
 func (r *Rolodex) GetRootIndex() *SpecIndex {
 	return r.rootIndex

index/rolodex_test.go

@@ -3049,3 +3049,17 @@ func TestRolodex_Open_HttpPrefixedLocationWithoutScheme(t *testing.T) {
 	assert.Nil(t, rf)
 	assert.EqualError(t, err, "remote URL 'httpdocs/pet.yaml' has no scheme, unable to fetch it")
 }
+
+func TestRolodex_HasCheckedForCircularReferences(t *testing.T) {
+	var rootNode yaml.Node
+	_ = yaml.Unmarshal([]byte("openapi: 3.1.0"), &rootNode)
+	config := CreateOpenAPIIndexConfig()
+	config.AvoidCircularReferenceCheck = true
+	rolo := NewRolodex(config)
+	rolo.SetRootNode(&rootNode)
+	assert.NoError(t, rolo.IndexTheRolodex(context.Background()))
+	assert.False(t, rolo.HasCheckedForCircularReferences())
+
+	rolo.CheckForCircularReferences()
+	assert.True(t, rolo.HasCheckedForCircularReferences())
+}

what-changed/model/schema.go

@@ -16,6 +16,7 @@ import (
 	"github.com/pb33f/libopenapi/datamodel/low/base"
 	v3 "github.com/pb33f/libopenapi/datamodel/low/v3"
 	"github.com/pb33f/libopenapi/orderedmap"
+	"github.com/pb33f/libopenapi/utils"
 )
 
 // SchemaChanges represent all changes to a base.Schema OpenAPI object. These changes are represented
@@ -396,6 +397,24 @@ func (s *SchemaChanges) TotalBreakingChanges() int {
 	return t
 }
 
+// canCompareBehindRefChange reports whether the schemas either side of a changed $ref can be compared like any
+// other two schemas. Both must resolve, and neither may be on a reference loop, which can only be known once the
+// document has been checked for circular references. A 3.1 $ref with sibling keywords is held in an internal allOf
+// form that doesn't line up with the referenced schema field by field, so it stops at the $ref change too.
+func canCompareBehindRefChange(l, r *base.SchemaProxy) bool {
+	for _, proxy := range []*base.SchemaProxy{l, r} {
+		if proxy.Schema() == nil || proxy.IsTransformedRefWithSiblings() {
+			return false
+		}
+		idx := proxy.GetIndex()
+		if idx == nil || idx.GetRolodex() == nil || !idx.GetRolodex().HasCheckedForCircularReferences() ||
+			base.CheckSchemaProxyForCircularRefs(proxy) {
+			return false
+		}
+	}
+	return true
+}
+
 // CompareSchemas accepts a left and right SchemaProxy and checks for changes. If anything is found, returns
 // a pointer to SchemaChanges, otherwise returns nil
 func CompareSchemas(l, r *base.SchemaProxy) *SchemaChanges {
@@ -439,18 +458,15 @@ func CompareSchemas(l, r *base.SchemaProxy) *SchemaChanges {
 				// continue on because the external references are the same and we need to check things going forward.
 
 			} else {
-				// references are different, that's all we care to know.
+				// the reference points somewhere else: record that, then compare the schema it now points to.
 				CreateChange(&changes, Modified, v3.RefLabel,
 					l.GetValueNode().Content[1], r.GetValueNode().Content[1], BreakingModified(CompSchema, PropRef), l.GetReference(),
 					r.GetReference())
-				sc.PropertyChanges = NewPropertyChanges(changes)
 
-				// check if this is a circular ref.
-				if base.CheckSchemaProxyForCircularRefs(l) || base.CheckSchemaProxyForCircularRefs(r) {
-					// if we have a circular reference, we can't do any more work here.
-					return nil
+				if !canCompareBehindRefChange(l, r) || low.AreEqual(l.Schema(), r.Schema()) {
+					sc.PropertyChanges = NewPropertyChanges(changes)
+					return sc
 				}
-				return sc
 			}
 		}
 
@@ -463,14 +479,11 @@ func CompareSchemas(l, r *base.SchemaProxy) *SchemaChanges {
 			if lHash != rHash {
 				CreateChange(&changes, Modified, v3.RefLabel,
 					l.GetValueNode(), r.GetValueNode().Content[1], BreakingModified(CompSchema, PropRef), l, r.GetReference())
-				sc.PropertyChanges = NewPropertyChanges(changes)
 
-				// check if this is a circular ref.
-				if base.CheckSchemaProxyForCircularRefs(r) {
-					// if we have a circular reference, we can't do any more work here.
-					return nil
+				if !canCompareBehindRefChange(l, r) {
+					sc.PropertyChanges = NewPropertyChanges(changes)
+					return sc
 				}
-				return sc
 			}
 		}
 
@@ -483,14 +496,11 @@ func CompareSchemas(l, r *base.SchemaProxy) *SchemaChanges {
 			if lHash != rHash {
 				CreateChange(&changes, Modified, v3.RefLabel,
 					l.GetValueNode().Content[1], r.GetValueNode(), BreakingModified(CompSchema, PropRef), l.GetReference(), r)
-				sc.PropertyChanges = NewPropertyChanges(changes)
 
-				// check if this is a circular ref.
-				if base.CheckSchemaProxyForCircularRefs(l) {
-					// if we have a circular reference, we can't do any more work here.
-					return nil
+				if !canCompareBehindRefChange(l, r) {
+					sc.PropertyChanges = NewPropertyChanges(changes)
+					return sc
 				}
-				return sc
 			}
 		}
 
@@ -1804,7 +1814,7 @@ func schemaComparisonViewForSimpleAllOfObject(proxy *base.SchemaProxy, schema *b
 }
 
 func isSimpleAllOfObjectSchema(proxy *base.SchemaProxy, schema *base.Schema) bool {
-	if proxy == nil || schema == nil || proxy.IsReference() || len(schema.AllOf.Value) == 0 {
+	if proxy == nil || schema == nil || len(schema.AllOf.Value) == 0 {
 		return false
 	}
 	if len(schema.OneOf.Value) > 0 || len(schema.AnyOf.Value) > 0 || len(schema.PrefixItems.Value) > 0 || schema.Not.Value != nil {
@@ -2049,21 +2059,37 @@ func schemaPairUsesEquivalentSimpleScalarUnion(typeProxy, anyOfProxy *base.Schem
 }
 
 func isPureTypeArraySchema(proxy *base.SchemaProxy) bool {
-	if proxy == nil || proxy.IsReference() || !schemaNodeHasSingleKey(proxy.GetValueNode(), v3.TypeLabel) {
+	node := schemaDefinitionNode(proxy)
+	if !schemaNodeHasSingleKey(node, v3.TypeLabel) {
 		return false
 	}
-	typeNode := proxy.GetValueNode().Content[1]
+	typeNode := node.Content[1]
 	return typeNode.Kind == yaml.SequenceNode && len(typeNode.Content) > 0
 }
 
 func isPureAnyOfUnionSchema(proxy *base.SchemaProxy) bool {
-	if proxy == nil || proxy.IsReference() || !schemaNodeHasSingleKey(proxy.GetValueNode(), v3.AnyOfLabel) {
+	if !schemaNodeHasSingleKey(schemaDefinitionNode(proxy), v3.AnyOfLabel) {
 		return false
 	}
 	schema := proxy.Schema()
 	return schema != nil && len(schema.AnyOf.Value) > 0
 }
 
+// schemaDefinitionNode returns the node a schema is defined by, following a reference to the schema it points to.
+func schemaDefinitionNode(proxy *base.SchemaProxy) *yaml.Node {
+	if proxy == nil {
+		return nil
+	}
+	node := proxy.GetValueNode()
+	if isRef, _, _ := utils.IsNodeRefValue(node); isRef {
+		if proxy.GetIndex() == nil {
+			return nil
+		}
+		node, _, _, _ = low.LocateRefNodeWithContext(proxy.GetContext(), node, proxy.GetIndex())
+	}
+	return node
+}
+
 func schemaNodeHasSingleKey(node *yaml.Node, key string) bool {
 	return node != nil && node.Kind == yaml.MappingNode && len(node.Content) == 2 && node.Content[0].Value == key
 }
@@ -2081,10 +2107,11 @@ func schemaNodeHasOnlyAllowedKeys(node *yaml.Node, allowed map[string]struct{})
 }
 
 func extractTypeArraySet(proxy *base.SchemaProxy) (map[string]struct{}, bool) {
-	if proxy == nil || proxy.IsReference() || !schemaNodeHasSingleKey(proxy.GetValueNode(), v3.TypeLabel) {
+	node := schemaDefinitionNode(proxy)
+	if !schemaNodeHasSingleKey(node, v3.TypeLabel) {
 		return nil, false
 	}
-	typeNode := proxy.GetValueNode().Content[1]
+	typeNode := node.Content[1]
 	if typeNode.Kind != yaml.SequenceNode || len(typeNode.Content) == 0 {
 		return nil, false
 	}

what-changed/model/schema_composition_wrap.go

@@ -10,9 +10,10 @@ import (
 )
 
 // preservedObjectBranch returns a comparison view for an object moved into a
-// composition. Uncertain wrappers retain the normal structural comparison.
+// composition. Uncertain wrappers retain the normal structural comparison. The
+// old object may be a reference, which is compared as the schema it resolves to.
 func preservedObjectBranch(l, r *base.SchemaProxy, old, wrapper *base.Schema) *base.Schema {
-	if l.IsReference() || r.IsReference() || old == nil || wrapper == nil ||
+	if r.IsReference() || old == nil || wrapper == nil ||
 		old.Properties.Value == nil || old.Properties.Value.Len() == 0 || hasObjectComposition(old) {
 		return nil
 	}

what-changed/model/schema_composition_wrap_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/pb33f/go-yaml"
 	"github.com/pb33f/libopenapi/datamodel"
 	"github.com/pb33f/libopenapi/datamodel/low"
+	"github.com/pb33f/libopenapi/datamodel/low/base"
 	"github.com/pb33f/libopenapi/datamodel/low/v3"
 	"github.com/pb33f/testify/assert"
 	"github.com/pb33f/testify/require"
@@ -498,3 +499,61 @@ components:
 		CompareSchemas(lSchemaProxy, rSchemaProxy)
 	})
 }
+
+// https://github.com/pb33f/libopenapi/issues/663
+// A referenced object moved into a nullable composition is reported like the same inline refactor, plus the
+// non-breaking $ref change.
+func TestCompareSchemas_ReferencedObjectWrappedMatchesInline(t *testing.T) {
+	left := `openapi: 3.1.0
+components:
+  schemas:
+    Thing:
+      type: object
+      required: [id]
+      properties:
+        id:
+          type: string
+    Entry:
+      $ref: '#/components/schemas/Thing'
+    Inline:
+      type: object
+      required: [id]
+      properties:
+        id:
+          type: string`
+
+	right := `openapi: 3.1.0
+components:
+  schemas:
+    Thing:
+      type: object
+      required: [id]
+      properties:
+        id:
+          type: string
+    Entry:
+      anyOf:
+        - $ref: '#/components/schemas/Thing'
+        - type: 'null'
+    Inline:
+      anyOf:
+        - type: object
+          required: [id]
+          properties:
+            id:
+              type: string
+        - type: 'null'`
+
+	leftDoc, rightDoc := test_BuildDoc(left, right)
+	find := func(doc *v3.Document, name string) *base.SchemaProxy {
+		return doc.Components.Value.FindSchema(name).Value
+	}
+
+	changes := CompareSchemas(find(leftDoc, "Entry"), find(rightDoc, "Entry"))
+	inline := CompareSchemas(find(leftDoc, "Inline"), find(rightDoc, "Inline"))
+	require.NotNil(t, changes)
+	require.NotNil(t, inline)
+	assert.Zero(t, changes.TotalBreakingChanges())
+	assert.Equal(t, inline.TotalChanges()+1, changes.TotalChanges())
+	assert.Equal(t, changesOtherThanRef(inline), changesOtherThanRef(changes))
+}