From ffb70f048dba6f029fa9ffd5bda81dd3c8db3954 Mon Sep 17 00:00:00 2001 From: Joseph Axisa Date: Tue, 20 Oct 2026 14:46:17 -0700 Subject: [PATCH] fix: compare the schema behind a changed $ref When a schema $ref was replaced by an inline schema, an inline schema by a $ref, or a $ref pointed somewhere else, CompareSchemas recorded a non-breaking $ref change and returned, so changes in the new schema were never reported. A $ref to a schema with type: int becoming an inline type: string was a single non-breaking change. Keep the $ref change, then compare the two schemas as it would compare two inline schemas. It still stops at the $ref change, now keeping that change instead of returning nil, when either side doesn't resolve, is on a reference loop, or comes from a document that was not checked for circular references (a loop can't be recognised there), and when a 3.1 $ref with sibling keywords is involved, since its internal allOf form doesn't line up with the referenced schema field by field. Rolodex gains HasCheckedForCircularReferences for that check. The helpers that recognise equivalent forms now look through a reference, so a change made through a $ref is reported like the same inline change: the object composition handling from #650 ($ref: Thing becoming anyOf: [$ref: Thing, type: 'null']), the simple allOf view, and a type array matching an anyOf of scalar types. Fixes #663 --- index/rolodex.go | 5 + index/rolodex_test.go | 14 + what-changed/model/schema.go | 77 ++-- what-changed/model/schema_composition_wrap.go | 5 +- .../model/schema_composition_wrap_test.go | 59 +++ what-changed/model/schema_test.go | 423 +++++++++++++++++- what-changed/what_changed_test.go | 9 +- 7 files changed, 548 insertions(+), 44 deletions(-) diff --git a/index/rolodex.go b/index/rolodex.go index 232f4680..1125a25f 100644 --- a/index/rolodex.go +++ b/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 diff --git a/index/rolodex_test.go b/index/rolodex_test.go index 455b929b..d257d612 100644 --- a/index/rolodex_test.go +++ b/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()) +} diff --git a/what-changed/model/schema.go b/what-changed/model/schema.go index 5ff42aa6..317815db 100644 --- a/what-changed/model/schema.go +++ b/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 } diff --git a/what-changed/model/schema_composition_wrap.go b/what-changed/model/schema_composition_wrap.go index 00d6e7b5..b847c194 100644 --- a/what-changed/model/schema_composition_wrap.go +++ b/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 } diff --git a/what-changed/model/schema_composition_wrap_test.go b/what-changed/model/schema_composition_wrap_test.go index ac08df46..2dd3639c 100644 --- a/what-changed/model/schema_composition_wrap_test.go +++ b/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)) +} diff --git a/what-changed/model/schema_test.go b/what-changed/model/schema_test.go index f61968f9..2db0dbf4 100644 --- a/what-changed/model/schema_test.go +++ b/what-changed/model/schema_test.go @@ -7,6 +7,7 @@ import ( "context" "fmt" "reflect" + "sort" "strings" "sync" "sync/atomic" @@ -24,6 +25,7 @@ import ( "github.com/pb33f/libopenapi/index" "github.com/pb33f/libopenapi/orderedmap" "github.com/pb33f/testify/assert" + "github.com/pb33f/testify/require" ) // These tests require full documents to be tested properly. schemas are perhaps the most complex @@ -336,10 +338,12 @@ components: changes := CompareSchemas(lSchemaProxy, rSchemaProxy) assert.NotNil(t, changes) - assert.Len(t, changes.Changes, 1) - assert.Len(t, changes.GetAllChanges(), 1) + assert.Len(t, changes.Changes, 2) + assert.Len(t, changes.GetAllChanges(), 2) assert.Equal(t, Modified, changes.Changes[0].ChangeType) assert.Equal(t, "#/components/schemas/Yo", changes.Changes[0].New) + assert.Equal(t, v3.TypeLabel, changes.Changes[1].Property) + assert.True(t, changes.Changes[1].Breaking) // the schema behind the reference is compared too. } func TestCompareSchemas_RefToInline(t *testing.T) { @@ -369,11 +373,13 @@ components: changes := CompareSchemas(lSchemaProxy, rSchemaProxy) assert.NotNil(t, changes) - assert.Len(t, changes.Changes, 1) - assert.Len(t, changes.GetAllChanges(), 1) + assert.Len(t, changes.Changes, 2) + assert.Len(t, changes.GetAllChanges(), 2) assert.Equal(t, Modified, changes.Changes[0].ChangeType) assert.Equal(t, v3.RefLabel, changes.Changes[0].Property) assert.Equal(t, "#/components/schemas/Yo", changes.Changes[0].Original) + assert.Equal(t, v3.TypeLabel, changes.Changes[1].Property) + assert.True(t, changes.Changes[1].Breaking) // the schema behind the reference is compared too. } func TestCompareSchemas_InlineToRef(t *testing.T) { @@ -403,11 +409,13 @@ components: changes := CompareSchemas(rSchemaProxy, lSchemaProxy) assert.NotNil(t, changes) - assert.Len(t, changes.Changes, 1) - assert.Len(t, changes.GetAllChanges(), 1) + assert.Len(t, changes.Changes, 2) + assert.Len(t, changes.GetAllChanges(), 2) assert.Equal(t, Modified, changes.Changes[0].ChangeType) assert.Equal(t, v3.RefLabel, changes.Changes[0].Property) assert.Equal(t, "#/components/schemas/Yo", changes.Changes[0].New) + assert.Equal(t, v3.TypeLabel, changes.Changes[1].Property) + assert.True(t, changes.Changes[1].Breaking) // the schema behind the reference is compared too. } func TestCompareSchemas_Identical(t *testing.T) { @@ -3452,7 +3460,8 @@ components: changes := CompareSchemas(lSchemaProxy, rSchemaProxy) assert.NotNil(t, changes) - assert.Equal(t, 1, changes.TotalChanges()) + assert.Equal(t, 2, changes.TotalChanges()) // the $ref change, and int becoming string behind it. + assert.Equal(t, 1, changes.TotalBreakingChanges()) } // https://github.com/pb33f/openapi-changes/issues/104 @@ -5405,7 +5414,9 @@ components: rSchemaProxy := rightDoc.Components.Value.FindSchema("OK").Value changes := CompareSchemas(lSchemaProxy, rSchemaProxy) - assert.Nil(t, changes) + // the reference is circular, so it isn't followed, but the $ref change itself is kept. + assert.Len(t, changes.GetAllChanges(), 1) + assert.Equal(t, v3.RefLabel, changes.Changes[0].Property) } func TestCompareSchemas_CheckRefChangeCircular_HackIndex(t *testing.T) { @@ -5472,15 +5483,15 @@ components: //rSchemaProxy.GetIndex().SetAbsolutePath("/something/else.yaml") changes := CompareSchemas(lSchemaProxy, rSchemaProxy) - assert.Len(t, changes.GetAllChanges(), 1) + assert.Len(t, changes.GetAllChanges(), 2) rSchemaProxy.GetIndex().SetAbsolutePath("/something/else.yaml") changes = CompareSchemas(lSchemaProxy, rSchemaProxy) - assert.Len(t, changes.GetAllChanges(), 1) + assert.Len(t, changes.GetAllChanges(), 2) rSchemaProxy.GetIndex().SetAbsolutePath("") changes = CompareSchemas(rSchemaProxy, lSchemaProxy) // flip them back - assert.Len(t, changes.GetAllChanges(), 1) + assert.Len(t, changes.GetAllChanges(), 2) } @@ -5512,7 +5523,9 @@ components: //rSchemaProxy.GetIndex().SetAbsolutePath("/something/else.yaml") changes := CompareSchemas(lSchemaProxy, rSchemaProxy) - assert.Nil(t, changes) + // the reference is circular, so it isn't followed, but the $ref change itself is kept. + assert.Len(t, changes.GetAllChanges(), 1) + assert.Equal(t, v3.RefLabel, changes.Changes[0].Property) } func TestCompareSchemas_CheckRefChange_HackIndex_RightToLeft(t *testing.T) { @@ -5543,11 +5556,11 @@ components: //rSchemaProxy.GetIndex().SetAbsolutePath("/something/else.yaml") changes := CompareSchemas(lSchemaProxy, rSchemaProxy) - assert.Len(t, changes.GetAllChanges(), 1) + assert.Len(t, changes.GetAllChanges(), 2) rSchemaProxy.GetIndex().SetAbsolutePath("/something/else.yaml") changes = CompareSchemas(lSchemaProxy, rSchemaProxy) - assert.Len(t, changes.GetAllChanges(), 1) + assert.Len(t, changes.GetAllChanges(), 2) } func TestCompareSchemas_CheckRefChangeCircular_HackIndex_RightToLeft(t *testing.T) { @@ -5578,7 +5591,9 @@ components: //rSchemaProxy.GetIndex().SetAbsolutePath("/something/else.yaml") changes := CompareSchemas(lSchemaProxy, rSchemaProxy) - assert.Nil(t, changes) + // the reference is circular, so it isn't followed, but the $ref change itself is kept. + assert.Len(t, changes.GetAllChanges(), 1) + assert.Equal(t, v3.RefLabel, changes.Changes[0].Property) } @@ -7423,6 +7438,384 @@ components: assert.Equal(t, 1, changes.TotalBreakingChanges()) } +// changesOtherThanRef summarises every change except the $ref change, for comparing a change made behind a +// reference with the same change made inline. +func changesOtherThanRef(changes *SchemaChanges) []string { + var summary []string + for _, c := range changes.GetAllChanges() { + if c.Property != v3.RefLabel { + summary = append(summary, fmt.Sprintf("%s %d %v", c.Property, c.ChangeType, c.Breaking)) + } + } + sort.Strings(summary) + return summary +} + +// https://github.com/pb33f/libopenapi/issues/663 +func TestCompareSchemas_RefToInline_RequiredAdded(t *testing.T) { + low.ClearHashCache() + left := `openapi: 3.1.0 +components: + schemas: + Pet: + type: object + properties: + name: + type: string + OK: + $ref: '#/components/schemas/Pet' + Inline: + type: object + properties: + name: + type: string` + + right := `openapi: 3.1.0 +components: + schemas: + Pet: + type: object + properties: + name: + type: string + OK: + type: object + properties: + name: + type: string + required: [name] + Inline: + type: object + properties: + name: + type: string + required: [name]` + + 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, "OK"), find(rightDoc, "OK")) + assert.NotNil(t, changes) + assert.Equal(t, 2, changes.TotalChanges()) + assert.Equal(t, 1, changes.TotalBreakingChanges()) + assert.Equal(t, v3.RefLabel, changes.Changes[0].Property) + assert.False(t, changes.Changes[0].Breaking) + + // reported the way the same change is reported between two inline schemas. + inline := CompareSchemas(find(leftDoc, "Inline"), find(rightDoc, "Inline")) + assert.Equal(t, changesOtherThanRef(inline), changesOtherThanRef(changes)) + + // and in the other direction. + changes = CompareSchemas(find(rightDoc, "OK"), find(leftDoc, "OK")) + inline = CompareSchemas(find(rightDoc, "Inline"), find(leftDoc, "Inline")) + assert.Equal(t, 2, changes.TotalChanges()) + assert.Equal(t, changesOtherThanRef(inline), changesOtherThanRef(changes)) +} + +// https://github.com/pb33f/libopenapi/issues/663 +func TestCompareSchemas_RefRetargeted_RequiredAdded(t *testing.T) { + low.ClearHashCache() + spec := `openapi: 3.1.0 +components: + schemas: + Pet: + type: object + properties: + name: + type: string + NewPet: + type: object + properties: + name: + type: string + required: [name] + OK: + $ref: '#/components/schemas/Pet'` + + leftDoc, rightDoc := test_BuildDoc(spec, strings.Replace(spec, "$ref: '#/components/schemas/Pet'", "$ref: '#/components/schemas/NewPet'", 1)) + + changes := CompareSchemas(leftDoc.Components.Value.FindSchema("OK").Value, rightDoc.Components.Value.FindSchema("OK").Value) + assert.NotNil(t, changes) + assert.Equal(t, 2, changes.TotalChanges()) + assert.Equal(t, 1, changes.TotalBreakingChanges()) + assert.Equal(t, "#/components/schemas/NewPet", changes.Changes[0].New) + assert.Equal(t, v3.RequiredLabel, changes.Changes[1].Property) +} + +// https://github.com/pb33f/libopenapi/issues/663 +func TestCompareSchemas_RefToInline_IdenticalCopy(t *testing.T) { + low.ClearHashCache() + left := `openapi: 3.1.0 +components: + schemas: + Pet: + type: object + properties: + name: + type: string + OK: + $ref: '#/components/schemas/Pet'` + + right := `openapi: 3.1.0 +components: + schemas: + Pet: + type: object + properties: + name: + type: string + OK: + type: object + properties: + name: + type: string` + + leftDoc, rightDoc := test_BuildDoc(left, right) + assert.Nil(t, CompareSchemas(leftDoc.Components.Value.FindSchema("OK").Value, rightDoc.Components.Value.FindSchema("OK").Value)) +} + +// https://github.com/pb33f/libopenapi/issues/663 +func TestCompareSchemas_RefToInline_SimpleAllOf(t *testing.T) { + low.ClearHashCache() + left := `openapi: 3.1.0 +components: + schemas: + Pet: + allOf: + - type: object + properties: + name: + type: string + OK: + $ref: '#/components/schemas/Pet' + Inline: + allOf: + - type: object + properties: + name: + type: string` + + right := `openapi: 3.1.0 +components: + schemas: + OK: + allOf: + - type: object + properties: + name: + type: string + required: [name] + Inline: + allOf: + - type: object + properties: + name: + type: string + required: [name]` + + leftDoc, rightDoc := test_BuildDoc(left, right) + find := func(doc *v3.Document, name string) *base.SchemaProxy { + return doc.Components.Value.FindSchema(name).Value + } + + // the referenced allOf is compared through the same merged view as an inline one. + changes := CompareSchemas(find(leftDoc, "OK"), find(rightDoc, "OK")) + inline := CompareSchemas(find(leftDoc, "Inline"), find(rightDoc, "Inline")) + assert.NotNil(t, changes) + assert.NotEmpty(t, changesOtherThanRef(changes)) + assert.Equal(t, changesOtherThanRef(inline), changesOtherThanRef(changes)) +} + +// https://github.com/pb33f/libopenapi/issues/663 +func TestCompareSchemas_RefWithSiblings_StopsAtRef(t *testing.T) { + low.ClearHashCache() + left := `openapi: 3.1.0 +components: + schemas: + Pet: + type: object + properties: + name: + type: string + OK: + $ref: '#/components/schemas/Pet'` + + right := `openapi: 3.1.0 +components: + schemas: + Pet: + type: object + properties: + name: + type: string + OK: + $ref: '#/components/schemas/Pet' + required: [name]` + + leftDoc, rightDoc := test_BuildDoc(left, right) + l := leftDoc.Components.Value.FindSchema("OK").Value + r := rightDoc.Components.Value.FindSchema("OK").Value + assert.True(t, r.IsTransformedRefWithSiblings()) + + // a $ref with siblings is held as an allOf, which doesn't line up with the referenced schema field by field. + changes := CompareSchemas(l, r) + assert.Len(t, changes.GetAllChanges(), 1) + assert.Equal(t, v3.RefLabel, changes.Changes[0].Property) + + changes = CompareSchemas(r, l) + assert.Len(t, changes.GetAllChanges(), 1) + assert.Equal(t, v3.RefLabel, changes.Changes[0].Property) +} + +// https://github.com/pb33f/libopenapi/issues/663 +func TestCompareSchemas_RefToInline_CircularStopsAtRef(t *testing.T) { + low.ClearHashCache() + left := `openapi: 3.1.0 +components: + schemas: + Node: + type: object + properties: + next: + $ref: '#/components/schemas/Node' + OK: + $ref: '#/components/schemas/Node'` + + right := `openapi: 3.1.0 +components: + schemas: + Node: + type: object + properties: + next: + $ref: '#/components/schemas/Node' + OK: + type: object + required: [next]` + + leftDoc, rightDoc := test_BuildDoc(left, right) + l := leftDoc.Components.Value.FindSchema("OK").Value + r := rightDoc.Components.Value.FindSchema("OK").Value + + changes := CompareSchemas(l, r) + require.NotNil(t, changes) + assert.Len(t, changes.GetAllChanges(), 1) + assert.Equal(t, v3.RefLabel, changes.Changes[0].Property) + + changes = CompareSchemas(r, l) + require.NotNil(t, changes) + assert.Len(t, changes.GetAllChanges(), 1) + assert.Equal(t, v3.RefLabel, changes.Changes[0].Property) +} + +// https://github.com/pb33f/libopenapi/issues/663 +func TestCompareSchemas_RefRetargeted_Unresolved(t *testing.T) { + low.ClearHashCache() + spec := `openapi: 3.1.0 +components: + schemas: + Pet: + type: object + required: [name] + properties: + name: + type: string + OK: + $ref: '#/components/schemas/Pet'` + + cases := map[string]struct { + ref string + config *datamodel.DocumentConfiguration + }{ + "missing": {"#/components/schemas/Missing", datamodel.NewDocumentConfiguration()}, + "external, not resolved": {"other.yaml#/Pet", &datamodel.DocumentConfiguration{SkipExternalRefResolution: true}}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + leftInfo, _ := datamodel.ExtractSpecInfo([]byte(spec)) + rightInfo, _ := datamodel.ExtractSpecInfo([]byte(strings.Replace(spec, "#/components/schemas/Pet'", tc.ref+"'", 1))) + leftDoc, _ := v3.CreateDocumentFromConfig(leftInfo, tc.config) + rightDoc, _ := v3.CreateDocumentFromConfig(rightInfo, tc.config) + + // there's nothing to compare the old schema with, so only the $ref change is reported. + changes := CompareSchemas(leftDoc.Components.Value.FindSchema("OK").Value, rightDoc.Components.Value.FindSchema("OK").Value) + require.NotNil(t, changes) + assert.Len(t, changes.GetAllChanges(), 1) + assert.Equal(t, v3.RefLabel, changes.Changes[0].Property) + }) + } +} + +// https://github.com/pb33f/libopenapi/issues/663 +func TestCompareSchemas_RefRetargeted_CircularCheckSkipped(t *testing.T) { + low.ClearHashCache() + left := `openapi: 3.1.0 +components: + schemas: + A: + type: object + properties: + b: + $ref: '#/components/schemas/B' + B: + type: object + properties: + a: + $ref: '#/components/schemas/A' + OK: + $ref: '#/components/schemas/A'` + right := strings.NewReplacer("schemas/A", "schemas/X", "schemas/B", "schemas/Y", " A:", " X:", " B:", " Y:").Replace(left) + + config := datamodel.NewDocumentConfiguration() + config.SkipCircularReferenceCheck = true + leftInfo, _ := datamodel.ExtractSpecInfo([]byte(left)) + rightInfo, _ := datamodel.ExtractSpecInfo([]byte(strings.Replace(right, "type: object\n properties:\n b:", "type: object\n description: changed\n properties:\n b:", 1))) + leftDoc, _ := v3.CreateDocumentFromConfig(leftInfo, config) + rightDoc, _ := v3.CreateDocumentFromConfig(rightInfo, config) + + // without the circular check a loop can't be recognised, so the reference is not followed. + changes := CompareSchemas(leftDoc.Components.Value.FindSchema("OK").Value, rightDoc.Components.Value.FindSchema("OK").Value) + require.NotNil(t, changes) + assert.Len(t, changes.GetAllChanges(), 1) + assert.Equal(t, v3.RefLabel, changes.Changes[0].Property) +} + +// https://github.com/pb33f/libopenapi/issues/663 +func TestCompareSchemas_RefToInline_EquivalentScalarUnion(t *testing.T) { + low.ClearHashCache() + left := `openapi: 3.1.0 +components: + schemas: + NullableString: + type: [string, 'null'] + OK: + $ref: '#/components/schemas/NullableString'` + + right := `openapi: 3.1.0 +components: + schemas: + OK: + anyOf: + - type: string + - type: 'null'` + + leftDoc, rightDoc := test_BuildDoc(left, right) + l := leftDoc.Components.Value.FindSchema("OK").Value + r := rightDoc.Components.Value.FindSchema("OK").Value + + // a type array and the equivalent anyOf are the same schema, whichever side is a reference. + changes := CompareSchemas(l, r) + require.NotNil(t, changes) + assert.Len(t, changes.GetAllChanges(), 1) + assert.Equal(t, v3.RefLabel, changes.Changes[0].Property) + + changes = CompareSchemas(r, l) + require.NotNil(t, changes) + assert.Len(t, changes.GetAllChanges(), 1) +} + // https://github.com/pb33f/libopenapi/issues/662 func TestCompareSchemas_RefToInline_UnrelatedCircularReference(t *testing.T) { low.ClearHashCache() diff --git a/what-changed/what_changed_test.go b/what-changed/what_changed_test.go index 5d76f948..ba42f65a 100644 --- a/what-changed/what_changed_test.go +++ b/what-changed/what_changed_test.go @@ -278,13 +278,18 @@ func TestCheckExplodedFileCheck(t *testing.T) { modDoc, _ := v3.CreateDocumentFromConfig(infoMod, config) changes := CompareOpenAPIDocuments(origDoc, modDoc) - assert.Equal(t, 1, changes.TotalChanges()) + assert.Equal(t, 3, changes.TotalChanges()) assert.Equal(t, 0, changes.TotalBreakingChanges()) + // both references changed, and the files they now point to are compared: c-alt.yaml has a new description. allChanges := changes.GetAllChanges() - assert.Len(t, allChanges, 1) + assert.Len(t, allChanges, 3) assert.Equal(t, "b.yaml#/components/schemas/SchemaB", allChanges[0].OriginalObject) assert.Equal(t, "b-alt.yaml#/components/schemas/SchemaB", allChanges[0].NewObject) + assert.Equal(t, "c.yaml", allChanges[1].OriginalObject) + assert.Equal(t, "c-alt.yaml", allChanges[1].NewObject) + assert.Equal(t, "description", allChanges[2].Property) + assert.Equal(t, "object C, but this has a changed description", allChanges[2].New) }