diff --git a/compilers/openapi/internal/annotation/readers_internal_test.go b/compilers/openapi/internal/annotation/readers_internal_test.go index 4ffaf5f1..210d7cfa 100644 --- a/compilers/openapi/internal/annotation/readers_internal_test.go +++ b/compilers/openapi/internal/annotation/readers_internal_test.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/require" yaml "gopkg.in/yaml.v3" + "github.com/dexpace/morphic/compilers/openapi/internal/nodeview" "github.com/dexpace/morphic/ir" ) @@ -447,6 +448,63 @@ func TestRawChildNode_ReadsOnlyAMappingChild(t *testing.T) { assert.Nil(t, RawChildNode(&yaml.Node{Kind: yaml.DocumentNode}, "a"), "nor an empty document") } +// TestRawChildNode_IsNotTheMergeAwareView pins the difference between this +// reader and nodeview's, which is the reason the two exist side by side: what a +// keyword is preserved *as* is what the source spelled at it, while what a +// pointer or a $ref *resolves to* is what the parser will see. +// +// The three cases are the three ways the trees diverge, and each is a keyword +// this package would preserve verbatim. Answering a raw read through the view +// would silently rewrite all of them — a merged keyword would appear at a +// schema that never wrote it, an alias would be replaced by its target, and a +// key written twice would change which of the two survives. +// +// It reaches across packages because that is where the mistake would be made: +// nothing inside either reader can see that the other answers differently. +func TestRawChildNode_IsNotTheMergeAwareView(t *testing.T) { + t.Parallel() + + // use is the mapping under test in each case; the raw read of a top-level + // key is unambiguous, so it is safe to navigate with. + useOf := func(t *testing.T, src string) *yaml.Node { + t.Helper() + use := RawChildNode(yamlNode(t, src), "use") + require.NotNil(t, use, "the fixture must declare a `use` mapping") + return use + } + + t.Run("a merge key contributes nothing to the raw read", func(t *testing.T) { + t.Parallel() + use := useOf(t, "base: &b {title: merged}\nuse:\n <<: *b\n") + + assert.Nil(t, RawChildNode(use, "title"), + "the source wrote `<<`, not `title`, so nothing is preserved at title") + merged := nodeview.New().ChildByToken(use, "title") + require.NotNil(t, merged, "the parser, however, does see it") + assert.Equal(t, "merged", merged.Value) + }) + + t.Run("an aliased value is not dereferenced by the raw read", func(t *testing.T) { + t.Parallel() + use := useOf(t, "base: &b anchored\nuse: {title: *b}\n") + + raw := RawChildNode(use, "title") + require.NotNil(t, raw) + assert.Equal(t, yaml.AliasNode, raw.Kind, "the raw tree keeps the alias the source wrote") + assert.Equal(t, "anchored", nodeview.New().ChildByToken(use, "title").Value, + "where the view stands the anchor in its place") + }) + + t.Run("a repeated key resolves to opposite ends", func(t *testing.T) { + t.Parallel() + use := useOf(t, "use: {title: first, title: last}\n") + + assert.Equal(t, "first", RawChildNode(use, "title").Value, "first matching key wins") + assert.Equal(t, "last", nodeview.New().ChildByToken(use, "title").Value, + "where the view follows the parser and takes the last") + }) +} + // TestRawPropertyNode_NilSchemaReadsNothing pins the nil guard on the schema // side of the same reader, which every caller relies on to ask about a position // that may have no body written at it. diff --git a/compilers/openapi/internal/nodeview/nodeview.go b/compilers/openapi/internal/nodeview/nodeview.go index dc1e0660..a3b5e384 100644 --- a/compilers/openapi/internal/nodeview/nodeview.go +++ b/compilers/openapi/internal/nodeview/nodeview.go @@ -47,6 +47,10 @@ const MergeDepthLimit = 64 // expansion depth, this one caps a document with many merged mappings. Past the // budget the view still answers correctly — it just stops memoizing, trading a // cache hit for a recomputation. +// +// Both of the view's memos are charged to it: a mapping's expanded pairs, and +// the key index keyIndex projects from them. One bound covering both is what +// keeps a second memo from doubling the memory the first one was capped at. const maxCachedPairs = 1 << 21 // DocumentRoot returns the effective root node to scan: the content of a @@ -87,8 +91,13 @@ type Pair struct { // that first reached it. MergeDepthLimit and maxCachedPairs bound the chain // depth and cache size respectively, so unlimited memoization can't trade the // crash for exhausted memory instead. +// +// It memoizes one thing more, for the walk rather than the expansion: keyIndex +// projects a memoized mapping into a key map, so descending a JSON pointer costs +// a map read per token instead of a scan of every pair at each one. type View struct { pairs map[*yaml.Node][]Pair + keys map[*yaml.Node]map[string]*yaml.Node cachedPairs int inFlight map[*yaml.Node]bool exhausted bool @@ -105,6 +114,7 @@ func (v *View) Exhausted() bool { return v.exhausted } func New() *View { return &View{ pairs: map[*yaml.Node][]Pair{}, + keys: map[*yaml.Node]map[string]*yaml.Node{}, inFlight: map[*yaml.Node]bool{}, } } @@ -398,11 +408,7 @@ func (v *View) ChildByToken(n *yaml.Node, token string) *yaml.Node { } switch n.Kind { case yaml.MappingNode: - for _, p := range v.MappingPairs(n) { - if p.Key == token { - return p.Val - } - } + return v.mappingChild(n, token) case yaml.SequenceNode: idx, err := strconv.Atoi(token) if err != nil || idx < 0 || idx >= len(n.Content) { @@ -413,6 +419,56 @@ func (v *View) ChildByToken(n *yaml.Node, token string) *yaml.Node { return nil } +// mappingChild answers one key of a mapping through the key index, falling back +// to a scan of its pairs for a mapping the index declines to cover. +// +// n is known to be a mapping node here, so it is its own Deref and keys the +// index under the same node MappingPairs memoizes the pairs under. +func (v *View) mappingChild(n *yaml.Node, token string) *yaml.Node { + pairs := v.MappingPairs(n) + if index := v.keyIndex(n, pairs); index != nil { + return index[token] + } + for _, p := range pairs { + if p.Key == token { + return p.Val + } + } + return nil +} + +// keyIndex returns n's expansion as a key map, building it on first use, or nil +// when the view holds no memo to project. +// +// It is what stops a pointer walk rescanning the mappings it descends through. +// Resolving R references into a components mapping of M entries scans R×M pairs +// without it — quadratic in a document's own size, since both grow together — +// where an index makes each hop a map read. A key map cannot answer differently +// from the scan it replaces: expandContent yields each key once, so the pairs it +// is built from hold no duplicate for a first-match scan to prefer. +// +// The index is charged to the pair budget and gated on it by the same test +// memoize applies, which is what makes one bound cover both memos — and, since +// cachedPairs only ever grows, what makes the index cover exactly the mappings +// whose pairs the view retained: a mapping memoize declined fails this test too, +// so there is no expansion the index keeps and the pairs do not. +func (v *View) keyIndex(n *yaml.Node, pairs []Pair) map[string]*yaml.Node { + if index, built := v.keys[n]; built { + return index + } + if v.cachedPairs+len(pairs) > maxCachedPairs { + return nil + } + + index := make(map[string]*yaml.Node, len(pairs)) + for _, p := range pairs { + index[p.Key] = p.Val + } + v.keys[n] = index + v.cachedPairs += len(pairs) + return index +} + // Deref follows AliasNode links to the anchored node, bounded against an alias // chain that loops (the anchor-cycle detector reports those separately). func Deref(n *yaml.Node) *yaml.Node { diff --git a/compilers/openapi/internal/nodeview/nodeview_internal_test.go b/compilers/openapi/internal/nodeview/nodeview_internal_test.go index de8affb1..cc79a1f9 100644 --- a/compilers/openapi/internal/nodeview/nodeview_internal_test.go +++ b/compilers/openapi/internal/nodeview/nodeview_internal_test.go @@ -505,3 +505,97 @@ func TestPointerPath_SegmentCapStopsTheWalk(t *testing.T) { assert.Len(t, path, maxPointerSegments+1, "the walk stops at the cap: the root plus one node per followed token") } + +// TestChildByToken_IndexAgreesWithTheScanItReplaces holds the key index to the +// scan it stands in for, over the mappings whose effective pairs are not their +// literal ones: a merge source, an alias standing in for a whole mapping, and a +// key written twice. +// +// A map answers by key where a scan answers by position, so the two agree only +// because expandContent yields each key once. That is the property under test — +// asserting the index against MappingPairs itself, key by key, is what would +// redden if a duplicate ever survived into an expansion. +// +// Each mapping is read twice through one view, because the two reads take +// different paths: the first builds the index, the second reads it back. +func TestChildByToken_IndexAgreesWithTheScanItReplaces(t *testing.T) { + t.Parallel() + base := ymap(yscalar("a"), yscalar("1"), yscalar("b"), yscalar("2")) + tests := []struct { + name string + n *yaml.Node + }{ + {name: "explicit keys", n: ymap(yscalar("a"), yscalar("1"))}, + {name: "merged keys", n: ymap(ymerge(), yalias(base), yscalar("c"), yscalar("3"))}, + {name: "explicit beats merged", n: ymap(ymerge(), yalias(base), yscalar("a"), yscalar("9"))}, + {name: "alias for the whole value", n: ymap(yscalar("a"), yalias(yscalar("1")))}, + {name: "a key written twice", n: ymap(yscalar("a"), yscalar("1"), yscalar("a"), yscalar("2"))}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + v := New() + pairs := v.MappingPairs(tc.n) + require.NotEmpty(t, pairs, "the fixture must expand to something to compare") + + for _, read := range []string{"builds the index", "reads it back"} { + for _, p := range pairs { + assert.Same(t, p.Val, v.ChildByToken(tc.n, p.Key), "%s: key %q", read, p.Key) + } + assert.Nil(t, v.ChildByToken(tc.n, "absent"), "%s: an unwritten key names nothing", read) + } + }) + } +} + +// TestKeyIndex_IsChargedToThePairBudget pins the index to the bound that already +// covers the pairs it projects. A memo added outside that budget would double +// the memory maxCachedPairs was set to cap. +func TestKeyIndex_IsChargedToThePairBudget(t *testing.T) { + t.Parallel() + n := ymap(yscalar("a"), yscalar("1"), yscalar("b"), yscalar("2")) + v := New() + + require.Len(t, v.MappingPairs(n), 2) + require.Equal(t, 2, v.cachedPairs, "the expansion is charged") + require.Same(t, n.Content[1], v.ChildByToken(n, "a")) + assert.Equal(t, 4, v.cachedPairs, "the index charges its own entries too") + + require.Same(t, n.Content[3], v.ChildByToken(n, "b")) + assert.Equal(t, 4, v.cachedPairs, "a second read builds nothing and charges nothing") +} + +// TestKeyIndex_PastTheBudgetTheScanStillAnswers covers the gate that makes the +// index optional, from both states a read can reach it in: a mapping whose pairs +// the budget also declined, and one memoized while the budget still allowed it. +// +// The second is the case the gate exists for. The first is the case that makes +// the gate sufficient on its own: cachedPairs never falls, so a mapping memoize +// turned away fails the identical test here, and the index cannot end up holding +// an expansion the pairs do not. +func TestKeyIndex_PastTheBudgetTheScanStillAnswers(t *testing.T) { + t.Parallel() + tests := []struct { + name string + expandCold bool + }{ + {name: "the pairs were declined too", expandCold: false}, + {name: "the pairs were memoized first", expandCold: true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + n := ymap(yscalar("a"), yscalar("1")) + v := New() + if tc.expandCold { + require.Len(t, v.MappingPairs(n), 1) + } + v.cachedPairs = maxCachedPairs + require.Equal(t, tc.expandCold, len(v.pairs) == 1, "the fixture must set up the state it names") + + assert.Same(t, n.Content[1], v.ChildByToken(n, "a"), "the scan still answers") + assert.Nil(t, v.ChildByToken(n, "absent")) + assert.Empty(t, v.keys, "and nothing was indexed") + }) + } +} diff --git a/compilers/openapi/internal/nodeview/pointerpath_bench_test.go b/compilers/openapi/internal/nodeview/pointerpath_bench_test.go new file mode 100644 index 00000000..ba5ee122 --- /dev/null +++ b/compilers/openapi/internal/nodeview/pointerpath_bench_test.go @@ -0,0 +1,61 @@ +package nodeview + +import ( + "fmt" + "testing" + + yaml "gopkg.in/yaml.v3" +) + +// componentsDoc builds `{components: {schemas: {S0..Sn-1: {type: object}}}}`, +// the shape every internal $ref in an OpenAPI document points into. +func componentsDoc(n int) *yaml.Node { + schemas := &yaml.Node{Kind: yaml.MappingNode} + for i := range n { + schemas.Content = append(schemas.Content, + &yaml.Node{Kind: yaml.ScalarNode, Value: fmt.Sprintf("S%d", i)}, + &yaml.Node{Kind: yaml.MappingNode, Content: []*yaml.Node{ + {Kind: yaml.ScalarNode, Value: "type"}, + {Kind: yaml.ScalarNode, Value: "object"}, + }}) + } + components := &yaml.Node{Kind: yaml.MappingNode, Content: []*yaml.Node{ + {Kind: yaml.ScalarNode, Value: "schemas"}, schemas, + }} + return &yaml.Node{Kind: yaml.MappingNode, Content: []*yaml.Node{ + {Kind: yaml.ScalarNode, Value: "components"}, components, + }} +} + +// BenchmarkPointerPath_IntoAWideMapping resolves one pointer per component of a +// components mapping, which is what the reference scan does to a document whose +// every schema is referenced once. +// +// It guards a shape rather than a number. The walk descends the same mapping +// once per reference, so the pairs it reads grow as references × components +// without keyIndex — and those two grow together in a real document, making the +// scan quadratic in the document's own size. Each width here does n times the +// work of a single resolution, so the *per-component* cost is what to read: +// divide by n and compare across widths. It should stay flat, and a run where it +// grows with n is the index no longer being reached. +func BenchmarkPointerPath_IntoAWideMapping(b *testing.B) { + for _, n := range []int{64, 256, 1024} { + root := componentsDoc(n) + pointers := make([]string, n) + for i := range pointers { + pointers[i] = fmt.Sprintf("/components/schemas/S%d", i) + } + + b.Run(fmt.Sprintf("components%d", n), func(b *testing.B) { + b.ReportAllocs() + for b.Loop() { + v := New() // one view per pass: a view never outlives its compile + for _, p := range pointers { + if _, complete := v.PointerPath(root, p); !complete { + b.Fatalf("pointer %s must resolve", p) + } + } + } + }) + } +}