diff --git a/datamodel/low/base/schema.go b/datamodel/low/base/schema.go index 1ddff501c..f94fe4909 100644 --- a/datamodel/low/base/schema.go +++ b/datamodel/low/base/schema.go @@ -162,6 +162,10 @@ func (s *Schema) GetContext() context.Context { // Hash will calculate a SHA256 hash from the values of the schema, This allows equality checking against // Schemas defined inside an OpenAPI document. The only way to know if a schema has changed, is to hash it. func (s *Schema) Hash() [32]byte { + // a nil schema (e.g. from a SchemaProxy that failed to resolve) hashes like an empty one. + if s == nil { + return sha256.Sum256([]byte{}) + } // calculate a hash from every property in the schema. var d []string if !s.SchemaTypeRef.IsEmpty() { diff --git a/datamodel/low/base/schema_test.go b/datamodel/low/base/schema_test.go index e65e4a9b1..cd67198de 100644 --- a/datamodel/low/base/schema_test.go +++ b/datamodel/low/base/schema_test.go @@ -1970,3 +1970,11 @@ func TestExtractSchema_CheckExampleNodesExtracted(t *testing.T) { t.Fail() } } + +func TestSchema_Hash_NilReceiver(t *testing.T) { + // a nil schema (e.g. from a SchemaProxy that failed to resolve) must not panic + // and hashes like an empty schema. + var s *Schema + empty := &Schema{} + assert.Equal(t, empty.Hash(), s.Hash()) +} diff --git a/datamodel/low/v3/create_document.go b/datamodel/low/v3/create_document.go index 80929b3f7..4d75696b6 100644 --- a/datamodel/low/v3/create_document.go +++ b/datamodel/low/v3/create_document.go @@ -57,7 +57,7 @@ func createDocument(info *datamodel.SpecInfo, config *datamodel.DocumentConfigur doc.Rolodex = rolodex // If basePath is provided, add a local filesystem to the rolodex. - if idxConfig.BasePath != "" || config.AllowFileReferences { + if idxConfig.BasePath != "" && config.AllowFileReferences { var cwd string cwd, _ = filepath.Abs(config.BasePath) // if a supplied local filesystem is provided, add it to the rolodex. diff --git a/index/rolodex.go b/index/rolodex.go index 2d0cdfd97..cc9d440d8 100644 --- a/index/rolodex.go +++ b/index/rolodex.go @@ -357,7 +357,7 @@ func (r *Rolodex) IndexTheRolodex() error { // which does not exist, but is used to formulate the absolute path to root references correctly. if r.indexConfig.BasePath != "" && r.indexConfig.BaseURL == nil { basePath := r.indexConfig.BasePath - if !filepath.IsAbs(basePath) { + if !filepath.IsAbs(basePath) && r.indexConfig.AllowFileLookup { basePath, _ = filepath.Abs(basePath) } diff --git a/what-changed/model/schema.go b/what-changed/model/schema.go index a4528683d..6bf65c581 100644 --- a/what-changed/model/schema.go +++ b/what-changed/model/schema.go @@ -304,6 +304,15 @@ func (s *SchemaChanges) TotalBreakingChanges() int { return t } +// schemasProvablyEqual checks that both proxies resolve and hash identically. If either proxy +// cannot be resolved (e.g. an unresolvable circular reference), equality cannot be proven, so a +// change should be reported instead of hashing a nil schema. +func schemasProvablyEqual(l, r *base.SchemaProxy) bool { + lSchema := l.Schema() + rSchema := r.Schema() + return lSchema != nil && rSchema != nil && lSchema.Hash() == rSchema.Hash() +} + // 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 { @@ -346,11 +355,13 @@ func CompareSchemas(l, r *base.SchemaProxy) *SchemaChanges { if !l.IsReference() && r.IsReference() { // check if the referenced schema matches or not // https://github.com/pb33f/libopenapi/issues/218 - lHash := l.Schema().Hash() - rHash := r.Schema().Hash() - if lHash != rHash { + if !schemasProvablyEqual(l, r) { + var rContentNode *yaml.Node = r.GetValueNode() + if len(r.GetValueNode().Content) > 1 { + rContentNode = r.GetValueNode().Content[1] + } CreateChange(&changes, Modified, v3.RefLabel, - l.GetValueNode(), r.GetValueNode().Content[1], true, l, r.GetReference()) + l.GetValueNode(), rContentNode, true, l, r.GetReference()) sc.PropertyChanges = NewPropertyChanges(changes) return sc // we're done here } @@ -360,11 +371,13 @@ func CompareSchemas(l, r *base.SchemaProxy) *SchemaChanges { if l.IsReference() && !r.IsReference() { // check if the referenced schema matches or not // https://github.com/pb33f/libopenapi/issues/218 - lHash := l.Schema().Hash() - rHash := r.Schema().Hash() - if lHash != rHash { + if !schemasProvablyEqual(l, r) { + var lContentNode *yaml.Node = l.GetValueNode() + if len(l.GetValueNode().Content) > 1 { + lContentNode = l.GetValueNode().Content[1] + } CreateChange(&changes, Modified, v3.RefLabel, - l.GetValueNode().Content[1], r.GetValueNode(), true, l.GetReference(), r) + lContentNode, r.GetValueNode(), true, l.GetReference(), r) sc.PropertyChanges = NewPropertyChanges(changes) return sc // done, nothing else to do. } @@ -373,6 +386,18 @@ func CompareSchemas(l, r *base.SchemaProxy) *SchemaChanges { lSchema := l.Schema() rSchema := r.Schema() + // if either proxy cannot be resolved (e.g. an unresolvable circular reference), + // the schemas cannot be walked. Report a modification if only one side is nil. + if lSchema == nil || rSchema == nil { + if lSchema != rSchema { + CreateChange(&changes, Modified, v3.SchemaLabel, + l.GetValueNode(), r.GetValueNode(), true, l, r) + sc.PropertyChanges = NewPropertyChanges(changes) + return sc + } + return nil + } + if low.AreEqual(lSchema, rSchema) { // there is no point going on, we know nothing changed! return nil diff --git a/what-changed/model/schema_test.go b/what-changed/model/schema_test.go index 2095c544e..dc2fe9907 100644 --- a/what-changed/model/schema_test.go +++ b/what-changed/model/schema_test.go @@ -3232,3 +3232,67 @@ components: assert.Equal(t, 1, changes.TotalChanges()) } + +func TestCompareSchemas_UnresolvableCircularAliasRef(t *testing.T) { + // a property that changes between an inline schema and a $ref pointing at a + // ref-to-ref alias chain inside a reference cycle. The alias chain cannot be + // resolved by the proxy (circular lookup), so SchemaProxy.Schema() returns nil; + // comparing must report a change, not panic hashing a nil schema. + inline := `openapi: 3.1.0 +components: + schemas: + VendorCredentialSummary: + $ref: "#/components/schemas/VendorCredentialFile" + VendorCredentialFile: + $ref: "#/components/schemas/VendorCredentialFileImpl" + VendorCredentialFileImpl: + type: object + properties: + summary: + $ref: "#/components/schemas/VendorCredentialSummary" + VendorCredentialDataRequest: + type: object + properties: + summary: + allOf: + - type: object + additionalProperties: true +` + + ref := `openapi: 3.1.0 +components: + schemas: + VendorCredentialSummary: + $ref: "#/components/schemas/VendorCredentialFile" + VendorCredentialFile: + $ref: "#/components/schemas/VendorCredentialFileImpl" + VendorCredentialFileImpl: + type: object + properties: + summary: + $ref: "#/components/schemas/VendorCredentialSummary" + VendorCredentialDataRequest: + type: object + properties: + summary: + $ref: "#/components/schemas/VendorCredentialSummary" +` + + leftDoc, rightDoc := test_BuildDoc(inline, ref) + + // inline -> ref + lSchemaProxy := leftDoc.Components.Value.FindSchema("VendorCredentialDataRequest").Value + rSchemaProxy := rightDoc.Components.Value.FindSchema("VendorCredentialDataRequest").Value + changes := CompareSchemas(lSchemaProxy, rSchemaProxy) + assert.NotNil(t, changes) + assert.Equal(t, 1, changes.TotalChanges()) + assert.Equal(t, Modified, changes.GetAllChanges()[0].ChangeType) + assert.Equal(t, v3.RefLabel, changes.GetAllChanges()[0].Property) + + // ref -> inline + changes = CompareSchemas(rSchemaProxy, lSchemaProxy) + assert.NotNil(t, changes) + assert.Equal(t, 1, changes.TotalChanges()) + assert.Equal(t, Modified, changes.GetAllChanges()[0].ChangeType) + assert.Equal(t, v3.RefLabel, changes.GetAllChanges()[0].Property) +}