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