diff --git a/datamodel/low/base/schema_hash.go b/datamodel/low/base/schema_hash.go index 04205304..887a4a33 100644 --- a/datamodel/low/base/schema_hash.go +++ b/datamodel/low/base/schema_hash.go @@ -10,7 +10,9 @@ import ( "strings" "sync" + "github.com/pb33f/go-yaml" "github.com/pb33f/libopenapi/datamodel/low" + "github.com/pb33f/libopenapi/index" "github.com/pb33f/libopenapi/orderedmap" ) @@ -412,26 +414,29 @@ func hashSchemaDependentRequired(h *maphash.Hash, label string, m *orderedmap.Ma } func (s *Schema) quickHashKey() string { - // The key identifies a node by file plus position, so the path must come from the file - // RootNode is in. GetIndex() may have been re-attributed to the file a $ref resolves to, - // which would pair a resolved-file path with a referring-file line and column: two - // different $refs at the same position in different files pointing into one shared file - // would then collide and return each other's hash. - // refIndex is assigned unconditionally in Build alongside Index, so it is nil only when - // the schema was never built, in which case Index is nil too and there is no path either way. - idx := s.refIndex + return quickHashKeyOf(s.refIndex, s.Index, s.RootNode) +} + +// quickHashKeyOf builds the quick-hash key for a schema whose root node is root, read from the file of +// refIndex, in the document of idx. The key identifies a node by file plus position, so the path must come +// from the file root is in. Index may have been re-attributed to the file a $ref resolves to, which would +// pair a resolved-file path with a referring-file line and column: two different $refs at the same position +// in different files pointing into one shared file would then collide and return each other's hash. +// refIndex is assigned unconditionally in Build alongside Index, so it is nil only when the schema was never +// built, in which case Index is nil too and there is no path either way. +func quickHashKeyOf(refIndex, idx *index.SpecIndex, root *yaml.Node) string { path := "" - if idx != nil { - path = idx.GetSpecAbsolutePath() + if refIndex != nil { + path = refIndex.GetSpecAbsolutePath() } cfID := "root" - if s.Index != nil { - if s.Index.GetRolodex() != nil { - if s.Index.GetRolodex().GetId() != "" { - cfID = s.Index.GetRolodex().GetId() + if idx != nil { + if idx.GetRolodex() != nil { + if idx.GetRolodex().GetId() != "" { + cfID = idx.GetRolodex().GetId() } } else { - cfID = s.Index.GetConfig().GetId() + cfID = idx.GetConfig().GetId() } } @@ -439,9 +444,9 @@ func (s *Schema) quickHashKey() string { keyBuf.Grow(len(path) + len(cfID) + 16) keyBuf.WriteString(path) keyBuf.WriteByte(':') - keyBuf.WriteString(strconv.Itoa(s.RootNode.Line)) + keyBuf.WriteString(strconv.Itoa(root.Line)) keyBuf.WriteByte(':') - keyBuf.WriteString(strconv.Itoa(s.RootNode.Column)) + keyBuf.WriteString(strconv.Itoa(root.Column)) keyBuf.WriteByte(':') keyBuf.WriteString(cfID) return keyBuf.String() diff --git a/datamodel/low/base/schema_proxy.go b/datamodel/low/base/schema_proxy.go index c2eae19f..66290012 100644 --- a/datamodel/low/base/schema_proxy.go +++ b/datamodel/low/base/schema_proxy.go @@ -267,7 +267,8 @@ func (sp *SchemaProxy) Schema() *Schema { // there were no errors during build, then nil will be returned. // // Thread safety: GetBuildError() is safe to call concurrently only after Schema() has been called at least once -// on this proxy (from any goroutine). All standard code paths (Hash(), high-level Schema()) call Schema() first. +// on this proxy (from any goroutine). The high-level Schema() calls Schema() first; Hash() may not, when a quick +// hash for the referenced schema is already cached. func (sp *SchemaProxy) GetBuildError() error { return sp.buildError } @@ -363,6 +364,10 @@ func (sp *SchemaProxy) computeHash() uint64 { cfg := sp.getSpecConfig() if cfg != nil && cfg.UseSchemaQuickHash { if !CheckSchemaProxyForCircularRefs(sp) { + // many references point at the same schema; reuse its quick hash before building another copy. + if hash, ok := sp.cachedQuickHash(); ok { + return hash + } sch = sp.Schema() if sch != nil { return sch.QuickHash() @@ -475,3 +480,26 @@ func (sp *SchemaProxy) attemptPropertyMerging(node *yaml.Node, config *datamodel return merged } + +// cachedQuickHash returns the quick hash already stored for the schema this reference would build, without +// building it. It only answers when the key the built schema would use can be worked out from the proxy: a node +// that is itself a $ref is re-attributed to another index while building (and is the only kind of node +// referenced-property merging changes), so that is left to Schema(). +func (sp *SchemaProxy) cachedQuickHash() (uint64, bool) { + if sp.vn == nil { + return 0, false + } + root := utils.ResolveMergeNodes(sp.vn) + if root == nil { + return 0, false + } + if isRef, _, _ := utils.IsNodeRefValue(root); isRef { + return 0, false + } + if v, ok := SchemaQuickHashMap.Load(quickHashKeyOf(sp.idx, sp.idx, root)); ok { + if hash, isHash := v.(uint64); isHash { + return hash, true + } + } + return 0, false +} diff --git a/datamodel/low/base/schema_proxy_test.go b/datamodel/low/base/schema_proxy_test.go index 577ffbad..99958f54 100644 --- a/datamodel/low/base/schema_proxy_test.go +++ b/datamodel/low/base/schema_proxy_test.go @@ -921,3 +921,80 @@ properties: recomputedHash := sch.Hash() assert.NotEqual(t, uint64(0), recomputedHash) } + +// quickHashRefProxy is a reference proxy as the model builds it: its value node is the schema it points to. +func quickHashRefProxy(t *testing.T, target string, cfg *index.SpecIndexConfig) *SchemaProxy { + t.Helper() + var node yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(target), &node)) + cfg.UseSchemaQuickHash = true + idx := index.NewSpecIndexWithConfig(&node, cfg) + rolodex := index.NewRolodex(cfg) + idx.SetRolodex(rolodex) + rolodex.SetRootIndex(idx) + + sp := new(SchemaProxy) + sp.vn = node.Content[0] + sp.idx = idx + sp.ctx = context.Background() + r := low.Reference{} + r.SetReference("#/components/schemas/Pet", &yaml.Node{}) + sp.Reference = r + return sp +} + +func TestSchemaProxy_QuickHash_ReusesCachedHashWithoutBuilding(t *testing.T) { + ClearSchemaQuickHashMap() + defer ClearSchemaQuickHashMap() + sp := quickHashRefProxy(t, "type: object\nproperties:\n name:\n type: string\n", &index.SpecIndexConfig{}) + + SchemaQuickHashMap.Store(quickHashKeyOf(sp.idx, sp.idx, sp.vn), uint64(42)) + + assert.Equal(t, uint64(42), sp.Hash()) + assert.Nil(t, sp.rendered.Load(), "the referenced schema should not be built") +} + +func TestSchemaProxy_QuickHash_BuildsOnMiss(t *testing.T) { + ClearSchemaQuickHashMap() + defer ClearSchemaQuickHashMap() + sp := quickHashRefProxy(t, "type: object\nproperties:\n name:\n type: string\n", &index.SpecIndexConfig{}) + + hash := sp.Hash() + + built := sp.rendered.Load() + require.NotNil(t, built) + assert.Equal(t, built.QuickHash(), hash) + // the key worked out from the proxy is the one the built schema uses. + assert.Equal(t, built.quickHashKey(), quickHashKeyOf(sp.idx, sp.idx, sp.vn)) +} + +func TestSchemaProxy_CachedQuickHash_Declines(t *testing.T) { + ClearSchemaQuickHashMap() + defer ClearSchemaQuickHashMap() + target := "type: object\nproperties:\n name:\n type: string\n" + + // a node that is itself a $ref is re-attributed while building, with or without property merging. + for _, cfg := range []*index.SpecIndexConfig{{}, {MergeReferencedProperties: true}} { + sp := quickHashRefProxy(t, "$ref: '#/components/schemas/Other'\ndescription: sibling\n", cfg) + SchemaQuickHashMap.Store(quickHashKeyOf(sp.idx, sp.idx, sp.vn), uint64(42)) + _, ok := sp.cachedQuickHash() + assert.False(t, ok) + } + + // nothing to build. + sp := new(SchemaProxy) + _, ok := sp.cachedQuickHash() + assert.False(t, ok) + + // an alias with nothing behind it. + sp = quickHashRefProxy(t, target, &index.SpecIndexConfig{}) + sp.vn = &yaml.Node{Kind: yaml.AliasNode} + _, ok = sp.cachedQuickHash() + assert.False(t, ok) + + // a value that isn't a hash. + sp = quickHashRefProxy(t, target, &index.SpecIndexConfig{}) + SchemaQuickHashMap.Store(quickHashKeyOf(sp.idx, sp.idx, sp.vn), "not a hash") + _, ok = sp.cachedQuickHash() + assert.False(t, ok) +} diff --git a/what-changed/what_changed_test.go b/what-changed/what_changed_test.go index 5d76f948..2654c678 100644 --- a/what-changed/what_changed_test.go +++ b/what-changed/what_changed_test.go @@ -12,6 +12,7 @@ import ( "github.com/pb33f/libopenapi/datamodel" v2 "github.com/pb33f/libopenapi/datamodel/low/v2" v3 "github.com/pb33f/libopenapi/datamodel/low/v3" + "github.com/pb33f/libopenapi/what-changed/model" "github.com/pb33f/testify/assert" "github.com/pb33f/testify/require" ) @@ -388,3 +389,68 @@ func TestCompareOpenAPIDocuments_CrossFileCircularReference(t *testing.T) { assert.Equal(t, 1, changes.TotalChanges()) assert.Equal(t, 1, changes.TotalBreakingChanges()) } + +// Many references to one schema in a document with a reference loop: with quick hash, references after the +// first reuse the cached hash of the schema they point to rather than building it again. The result must be the +// same as without quick hash. +func TestCompareOpenAPIDocuments_QuickHashCyclicSharedRefs(t *testing.T) { + spec := func(nameType string) []byte { + return []byte(`openapi: 3.1.0 +info: + title: shared refs + version: '1' +paths: {} +components: + schemas: + Pet: + type: object + properties: + name: + type: ` + nameType + ` + Owner: + type: object + properties: + pet: + $ref: '#/components/schemas/Pet' + pets: + type: array + items: + $ref: '#/components/schemas/Pet' + Shop: + type: object + properties: + featured: + $ref: '#/components/schemas/Pet' + owner: + $ref: '#/components/schemas/Owner' + Node: + type: object + properties: + next: + $ref: '#/components/schemas/Link' + Link: + type: object + properties: + node: + $ref: '#/components/schemas/Node'`) + } + compare := func(quick bool) *model.DocumentChanges { + config := datamodel.NewDocumentConfiguration() + config.UseSchemaQuickHash = quick + load := func(b []byte) *v3.Document { + info, err := datamodel.ExtractSpecInfo(b) + require.NoError(t, err) + doc, err := v3.CreateDocumentFromConfig(info, config) + require.NoError(t, err) + return doc + } + return CompareOpenAPIDocuments(load(spec("string")), load(spec("integer"))) + } + + withQuickHash, without := compare(true), compare(false) + require.NotNil(t, withQuickHash) + require.NotNil(t, without) + assert.Equal(t, without.TotalChanges(), withQuickHash.TotalChanges()) + assert.Equal(t, without.TotalBreakingChanges(), withQuickHash.TotalBreakingChanges()) + assert.Equal(t, 1, withQuickHash.TotalBreakingChanges()) // Pet.name: string -> integer +}