From 3350ba144f9057e707e5216f755bb107eedf8e11 Mon Sep 17 00:00:00 2001 From: Adam Talbot Date: Sat, 1 Aug 2026 12:36:15 +0100 Subject: [PATCH] Add YAML merge key (<<) support to the values walk Previously "<<" was walked like a normal alias key, which produced bogus paths (e.g. serviceA.<<.replicaCount) and meant a shared anchor's non-scalar content was only ever documented at the first merge site, due to the existing alias-cycle guard. mappingEntries now resolves "<<" merge keys (including sequences of mappings) and flattens their entries into the parent mapping before walking, following the conventional merge key precedence: explicit keys win over merged ones, and among multiple merge sources the earliest wins on conflict. A recursion guard prevents a self-referential merge from looping forever. The walk's alias/merge-cycle guard previously stopped descending into any non-end node it had already visited once, which also meant that any non-scalar value shared across more than one alias or merge site (not just the merge key's own direct entries) was silently documented only at the first site. This is replaced with proper memoization: the properties found under a shared node's subtree are cached (relative to where it was first reached) and replayed at every later site, while a separate in-progress marker still catches genuine self-reference without recursing forever. Signed-off-by: Adam Talbot --- parser/document.go | 208 +++++++++++++++++++++++++++++++++++----- parser/document_test.go | 165 +++++++++++++++++++++++++++++-- 2 files changed, 341 insertions(+), 32 deletions(-) diff --git a/parser/document.go b/parser/document.go index e86b0ac..d80508d 100644 --- a/parser/document.go +++ b/parser/document.go @@ -92,6 +92,18 @@ type Node struct { RawNode *yaml.Node } +// sharedNodeFrame accumulates the properties documented while a shared +// (non-end) node's subtree is being walked for the first time, relative to +// that node's own path. Once the walk of that subtree finishes, the frame's +// properties are cached so a later encounter of the same *yaml.Node — via a +// different alias or "<<" merge site — can replay them at its own path +// instead of either silently dropping them or re-walking the whole subtree +// again. +type sharedNodeFrame struct { + root paths.Path + properties []Property +} + func Load(filename string, includeHidden bool) (*Document, error) { file, err := os.Open(filename) if err != nil { @@ -109,10 +121,42 @@ func Load(filename string, includeHidden bool) (*Document, error) { HeadComments: parseComments(root.HeadComment), FootComment: parseComments(root.FootComment), } - // visited prevents alias cycles (CWE-674 stack overflow) and alias fan-out - // (OOM) by stopping re-descent into any non-end node already on this walk. - visited := map[*yaml.Node]bool{} - err = walk(node, func(node Node) (bool, error) { + + // memo caches the properties documented under a shared node's subtree, + // relative to the path it was first reached at, keyed by the underlying + // *yaml.Node so later encounters (via aliases or "<<" merge keys) can + // replay them rebased onto their own path. + memo := map[*yaml.Node][]Property{} + // inProgress guards against a node that, directly or transitively, + // refers to itself (CWE-674 stack overflow) — it is only ever true while + // that node's subtree is actively being walked for the first time. + inProgress := map[*yaml.Node]bool{} + // frames is a stack of shared nodes currently being walked for the first + // time, so any property found anywhere below them gets recorded + // (relative to each) for later replay. Diamond-shaped reuse (the same + // node reached from several sibling sites) is fine and expected; only a + // true cycle — re-entering a node already in this stack — is refused. + var frames []*sharedNodeFrame + + // record appends a property to the document's current section and, for + // every shared node currently being walked for the first time, also + // records it (relative to that node's own path) for replay at future + // encounters. + record := func(prop Property) { + sectionIdx := len(document.Sections) - 1 + document.Sections[sectionIdx].Properties = append(document.Sections[sectionIdx].Properties, prop) + + for _, f := range frames { + if len(prop.Path) < len(f.root) { + continue + } + rel := prop + rel.Path = append(paths.Path{}, prop.Path[len(f.root):]...) + f.properties = append(f.properties, rel) + } + } + + err = walk(node, func(node Node) (bool, func(), error) { comment := pop(&node.HeadComments) parseCommentsOntoDocument(node.Path.Parent(), &document, node.HeadComments) @@ -120,12 +164,12 @@ func Load(filename string, includeHidden bool) (*Document, error) { // If we have a comment instructing us to skip this node, obey it if comment.Tags.GetBool(TagIgnore) { - return true, nil + return true, nil, nil } // If we have a comment instructing us to hide this node, obey it if we are not including hidden nodes if comment.Tags.GetBool(TagHidden) && !includeHidden { - return true, nil + return true, nil, nil } // An end node is a node we find a property at, this is usually a scalar @@ -133,24 +177,46 @@ func Load(filename string, includeHidden bool) (*Document, error) { // +docs:property tag (or if they have no values). if !isEndNode(node, comment) { parseCommentsOntoDocument(node.Path.Parent(), &document, []Comment{comment}) - // Only guard recursion into children — end nodes are always safe to - // visit multiple times (scalars have no children to cycle through). - if visited[node.RawNode] { - return true, nil + + // We've fully documented this exact node before, via a different + // alias or "<<" merge site — replay what we found there instead + // of silently dropping it. + if cached, ok := memo[node.RawNode]; ok { + for _, p := range cached { + p.Path = append(append(paths.Path{}, node.Path...), p.Path...) + record(p) + } + return true, nil, nil + } + + // This node is already being walked further up the current call + // stack — a genuine cycle (e.g. a mapping merging itself). + // Stop here rather than recursing forever. + if inProgress[node.RawNode] { + return true, nil, nil } - visited[node.RawNode] = true - return false, nil + + inProgress[node.RawNode] = true + frame := &sharedNodeFrame{root: node.Path} + frames = append(frames, frame) + + after := func() { + memo[node.RawNode] = frame.properties + delete(inProgress, node.RawNode) + frames = frames[:len(frames)-1] + } + + return false, after, nil } - sectionIdx := len(document.Sections) - 1 - document.Sections[sectionIdx].Properties = append(document.Sections[sectionIdx].Properties, Property{ + record(Property{ Path: node.Path, Description: comment, Type: getTypeOf(node, comment), Default: getDefaultValue(node, comment), }) - return true, nil + return true, nil, nil }) return &document, err @@ -243,14 +309,24 @@ func parseCommentsOntoDocument(path paths.Path, document *Document, comments []C } } -func walk(root Node, fn func(node Node) (bool, error)) error { +// walk performs a depth-first traversal of a yaml node tree, calling fn for +// every node encountered. fn reports whether to stop descending into this +// node's children and may optionally return an "after" function, which is +// called once this node and all of its descendants have been fully visited — +// this lets the caller pair up per-node setup (e.g. entering a subtree for +// the first time) with a matching cleanup step. +func walk(root Node, fn func(node Node) (stop bool, after func(), err error)) error { // Call the function for every node, we the method can decide to stop // walking this branch as part of this call - stop, err := fn(root) + stop, after, err := fn(root) if err != nil { return err } + if after != nil { + defer after() + } + if stop { return nil } @@ -271,15 +347,12 @@ func walk(root Node, fn func(node Node) (bool, error)) error { } } case yaml.MappingNode: - for i := 0; i < len(root.RawNode.Content); i += 2 { - keyNode := root.RawNode.Content[i] - valueNode := root.RawNode.Content[i+1] - + for _, entry := range mappingEntries(root.RawNode, map[*yaml.Node]bool{}) { n := Node{ - Path: root.Path.WithProperty(keyNode.Value), - HeadComments: parseComments(keyNode.HeadComment), - FootComment: parseComments(keyNode.FootComment), - RawNode: valueNode, + Path: root.Path.WithProperty(entry.Key.Value), + HeadComments: parseComments(entry.Key.HeadComment), + FootComment: parseComments(entry.Key.FootComment), + RawNode: entry.Value, } if err := walk(n, fn); err != nil { @@ -315,6 +388,91 @@ func walk(root Node, fn func(node Node) (bool, error)) error { return nil } +// mappingEntry is a resolved key/value pair from a mapping node, after +// expanding any "<<" merge keys. +type mappingEntry struct { + Key *yaml.Node + Value *yaml.Node +} + +// isMergeKey returns true if n is a "<<" merge key, per the YAML merge key +// convention (not part of the core YAML spec, but widely supported). +func isMergeKey(n *yaml.Node) bool { + return n.Kind == yaml.ScalarNode && n.Value == "<<" && (n.Tag == "" || n.Tag == "!" || n.ShortTag() == "!!merge") +} + +// resolveMergeTargets expands a merge key's value into the mapping nodes it +// refers to. The value may be a single mapping, an alias to one, or a +// sequence of either (for merging in more than one mapping at once). +func resolveMergeTargets(n *yaml.Node) []*yaml.Node { + switch n.Kind { + case yaml.AliasNode: + return resolveMergeTargets(n.Alias) + case yaml.MappingNode: + return []*yaml.Node{n} + case yaml.SequenceNode: + var out []*yaml.Node + for _, item := range n.Content { + out = append(out, resolveMergeTargets(item)...) + } + return out + default: + // Not a valid merge target, e.g. an alias to a scalar. Ignore it + // rather than failing the whole document. + return nil + } +} + +// mappingEntries returns a mapping node's effective key/value pairs with any +// "<<" merge keys expanded in place. Explicit keys always take precedence +// over merged-in ones regardless of declaration order, and where more than +// one merge source defines the same key, the earliest one wins — both match +// the YAML merge key convention. +// +// inProgress guards against a merge target that (directly or transitively) +// merges itself, which would otherwise recurse forever. +func mappingEntries(n *yaml.Node, inProgress map[*yaml.Node]bool) []mappingEntry { + if inProgress[n] { + return nil + } + inProgress[n] = true + defer delete(inProgress, n) + + explicit := map[string]bool{} + for i := 0; i < len(n.Content); i += 2 { + if key := n.Content[i]; !isMergeKey(key) { + explicit[key.Value] = true + } + } + + seen := map[string]bool{} + var entries []mappingEntry + + for i := 0; i < len(n.Content); i += 2 { + key, value := n.Content[i], n.Content[i+1] + + if !isMergeKey(key) { + if !seen[key.Value] { + seen[key.Value] = true + entries = append(entries, mappingEntry{Key: key, Value: value}) + } + continue + } + + for _, target := range resolveMergeTargets(value) { + for _, entry := range mappingEntries(target, inProgress) { + if explicit[entry.Key.Value] || seen[entry.Key.Value] { + continue + } + seen[entry.Key.Value] = true + entries = append(entries, entry) + } + } + } + + return entries +} + // isEndNode returns true if the yaml node is considered one that should // be documented as a parameter. // diff --git a/parser/document_test.go b/parser/document_test.go index 508e79f..765d482 100644 --- a/parser/document_test.go +++ b/parser/document_test.go @@ -44,7 +44,10 @@ func TestLoad_SelfReferentialAlias(t *testing.T) { _ = err } -// Scaled-down billion-laughs must not OOM. +// Scaled-down billion-laughs must not OOM, and — since a shared node's +// properties are now memoized and replayed rather than just walked once — +// must still produce the exact (bounded, polynomial) number of properties +// implied by the fan-out, not an exponential blow-up. // Uses 5 levels (10^5 = 100 000 virtual nodes) instead of 9 to keep the test // fast while still exercising the fan-out path. func TestLoad_BillionLaughs(t *testing.T) { @@ -56,14 +59,20 @@ d: &d [*c,*c,*c,*c,*c,*c,*c,*c,*c,*c] e: [*d,*d,*d,*d,*d,*d,*d,*d,*d,*d] ` path := writeTemp(t, yaml) - _, err := Load(path, false) - _ = err + doc, err := Load(path, false) + require.NoError(t, err) + + total := 0 + for _, s := range doc.Sections { + total += len(s.Properties) + } + // a:10 + b:10*10 + c:10*100 + d:10*1000 + e:10*10000 = 111110 + assert.Equal(t, 111110, total) } -// Shared anchors must not panic. Note: the visited-set means a non-scalar -// anchor's subtree is walked only at the first alias reference, so serviceB's -// properties are not documented — that trade-off is intentional. -func TestLoad_SharedAnchorDoesNotPanic(t *testing.T) { +// A mapping merged into multiple mappings via "<<" must surface its +// properties under each merge site, not just the first. +func TestLoad_MergeKeySharedAcrossMappings(t *testing.T) { yaml := ` defaults: &defaults replicaCount: 1 @@ -81,6 +90,148 @@ serviceB: doc, err := Load(path, false) require.NoError(t, err) require.NotNil(t, doc) + + var paths []string + for _, s := range doc.Sections { + for _, p := range s.Properties { + paths = append(paths, p.Path.String()) + } + } + assert.Contains(t, paths, "serviceA.replicaCount") + assert.Contains(t, paths, "serviceA.image") + assert.Contains(t, paths, "serviceA.port") + assert.Contains(t, paths, "serviceB.replicaCount") + assert.Contains(t, paths, "serviceB.image") + assert.Contains(t, paths, "serviceB.port") + assert.NotContains(t, paths, "serviceA.<<") + assert.NotContains(t, paths, "serviceB.<<") +} + +// An explicit key must override a merged-in key of the same name regardless +// of whether it's declared before or after the "<<" merge key. +func TestLoad_MergeKeyExplicitOverridesMerged(t *testing.T) { + yaml := ` +defaults: &defaults + # -- from defaults + replicaCount: 1 + +service: + <<: *defaults + # -- overridden + replicaCount: 3 +` + path := writeTemp(t, yaml) + doc, err := Load(path, false) + require.NoError(t, err) + + var found *Property + for _, s := range doc.Sections { + for i, p := range s.Properties { + if p.Path.String() == "service.replicaCount" { + found = &s.Properties[i] + } + } + } + require.NotNil(t, found, "expected service.replicaCount to be documented") + assert.Equal(t, "3", found.Default) +} + +// Merging multiple mappings via a sequence must resolve conflicting keys in +// favour of the earliest mapping in the sequence. +func TestLoad_MergeKeySequencePrecedence(t *testing.T) { + yaml := ` +a: &a + value: from-a +b: &b + value: from-b + +service: + <<: [*a, *b] +` + path := writeTemp(t, yaml) + doc, err := Load(path, false) + require.NoError(t, err) + + var found *Property + for _, s := range doc.Sections { + for i, p := range s.Properties { + if p.Path.String() == "service.value" { + found = &s.Properties[i] + } + } + } + require.NotNil(t, found, "expected service.value to be documented") + assert.Equal(t, "from-a", found.Default) +} + +// A mapping that merges itself (directly or transitively) must not cause a +// stack overflow. +func TestLoad_MergeKeySelfReferential(t *testing.T) { + yaml := "a: &a\n <<: *a\n b: 1\n" + path := writeTemp(t, yaml) + _, err := Load(path, false) + // The call must return (possibly with an error) — a stack overflow is + // fatal and would kill the test process before we reach this line. + _ = err +} + +// A non-scalar value nested inside a mapping merged at more than one site +// must be documented at every site, not just the first. Before memoization +// was added, only the first site to reach the shared "nested" node got its +// properties — later sites silently got nothing, because the cycle/fan-out +// guard treated every repeat encounter as already handled. +func TestLoad_MergeKeyNestedSharedSubtreeDocumentedAtEverySite(t *testing.T) { + yaml := ` +template: &template + nested: + # -- from the shared template + value: shared-default + +svcA: + <<: *template + +svcB: + <<: *template +` + path := writeTemp(t, yaml) + doc, err := Load(path, false) + require.NoError(t, err) + + var paths []string + for _, s := range doc.Sections { + for _, p := range s.Properties { + paths = append(paths, p.Path.String()) + } + } + assert.Contains(t, paths, "svcA.nested.value") + assert.Contains(t, paths, "svcB.nested.value") +} + +// The same nested-sharing fix must also apply to plain alias reuse, not just +// "<<" merge keys — a nested non-scalar value reachable from two different +// alias sites must be documented at both. +func TestLoad_AliasNestedSharedSubtreeDocumentedAtEverySite(t *testing.T) { + yaml := ` +template: &template + nested: + # -- from the shared template + value: shared-default + +svcA: *template +svcB: *template +` + path := writeTemp(t, yaml) + doc, err := Load(path, false) + require.NoError(t, err) + + var paths []string + for _, s := range doc.Sections { + for _, p := range s.Properties { + paths = append(paths, p.Path.String()) + } + } + assert.Contains(t, paths, "svcA.nested.value") + assert.Contains(t, paths, "svcB.nested.value") } // A plain acyclic values file must parse correctly and surface its properties.