From a47e882c8f000aa646c78402158fa6817e18856b Mon Sep 17 00:00:00 2001 From: Steven WdV Date: Wed, 16 Jul 2025 16:00:16 +0200 Subject: [PATCH] Flag for fixed list merge key traverse override behavior, and fix traversing map with merge key that would override local key (completes #2110 fix) --- .../operators/anchor-and-alias-operators.md | 32 +++++++++++ pkg/yqlib/doc/operators/traverse-read.md | 12 ++-- pkg/yqlib/operator_anchors_aliases.go | 14 +++-- pkg/yqlib/operator_anchors_aliases_test.go | 10 ++++ pkg/yqlib/operator_traverse_path.go | 37 ++++++++++-- pkg/yqlib/operator_traverse_path_test.go | 57 ++++++++++++++----- 6 files changed, 135 insertions(+), 27 deletions(-) diff --git a/pkg/yqlib/doc/operators/anchor-and-alias-operators.md b/pkg/yqlib/doc/operators/anchor-and-alias-operators.md index d624b88b..e1f05b52 100644 --- a/pkg/yqlib/doc/operators/anchor-and-alias-operators.md +++ b/pkg/yqlib/doc/operators/anchor-and-alias-operators.md @@ -96,6 +96,38 @@ y: 2 x: 1 ``` +## Override with local key +like https://yaml.org/type/merge.html, but with x: 1 before the merge key. This is legacy behavior, see --yaml-fix-merge-anchor-to-spec + +Given a sample.yml file of: +```yaml +- &CENTER + x: 1 + y: 2 +- &LEFT + x: 0 + y: 2 +- &BIG + r: 10 +- &SMALL + r: 1 +- x: 1 + !!merge <<: + - *BIG + - *LEFT + - *SMALL +``` +then +```bash +yq '.[4] | explode(.)' sample.yml +``` +will output +```yaml +x: 0 +r: 10 +y: 2 +``` + ## Get anchor Given a sample.yml file of: ```yaml diff --git a/pkg/yqlib/doc/operators/traverse-read.md b/pkg/yqlib/doc/operators/traverse-read.md index af5c965b..3957f4c4 100644 --- a/pkg/yqlib/doc/operators/traverse-read.md +++ b/pkg/yqlib/doc/operators/traverse-read.md @@ -304,6 +304,8 @@ foo_a ``` ## Traversing merge anchors with override +This is legacy behavior, see --yaml-fix-merge-anchor-to-spec + Given a sample.yml file of: ```yaml foo: &foo @@ -399,7 +401,7 @@ foobar_thing ``` ## Traversing merge anchor lists -Note that the keys earlier in the merge anchors sequence override later ones +Note that the later merge anchors override previous, but this is legacy behavior, see --yaml-fix-merge-anchor-to-spec Given a sample.yml file of: ```yaml @@ -428,10 +430,12 @@ yq '.foobarList.thing' sample.yml ``` will output ```yaml -foo_thing +bar_thing ``` ## Splatting merge anchor lists +With legacy override behavior, see --yaml-fix-merge-anchor-to-spec + Given a sample.yml file of: ```yaml foo: &foo @@ -460,9 +464,9 @@ yq '.foobarList[]' sample.yml will output ```yaml bar_b -foo_thing -foobarList_c foo_a +bar_thing +foobarList_c ``` ## Select multiple indices diff --git a/pkg/yqlib/operator_anchors_aliases.go b/pkg/yqlib/operator_anchors_aliases.go index 1fa8ecca..937eed52 100644 --- a/pkg/yqlib/operator_anchors_aliases.go +++ b/pkg/yqlib/operator_anchors_aliases.go @@ -3,6 +3,7 @@ package yqlib import ( "container/list" "fmt" + "slices" ) func assignAliasOperator(d *dataTreeNavigator, context Context, expressionNode *ExpressionNode) (Context, error) { @@ -237,9 +238,13 @@ func applyMergeAnchor(node *CandidateNode, merge *CandidateNode, mergeIndex int, return applyMergeAnchorMap(node, merge, mergeIndex, inline, newContent) case SequenceNode: log.Debugf("a merge list!") - // Earlier keys take precedence - for index := len(merge.Content) - 1; index >= 0; index = index - 1 { - childValue := merge.Content[index] + // With FixMergeAnchorToSpec, we rely on overrideEntry to reject duplicates + content := slices.All(merge.Content) + if !ConfiguredYamlPreferences.FixMergeAnchorToSpec { + // Even without FixMergeAnchorToSpec, this already gave preference to earlier keys + content = slices.Backward(merge.Content) + } + for _, childValue := range content { childInline := inline if childValue.Kind == AliasNode { childInline = false @@ -303,8 +308,9 @@ func overrideEntry(node *CandidateNode, key *CandidateNode, value *CandidateNode keyNode := newEl.Value.(*CandidateNode) log.Debugf("checking new content %v:%v", keyNode.Value, valueEl.Value.(*CandidateNode).Value) if keyNode.Value == key.Value && keyNode.Alias == nil && key.Alias == nil { - log.Debugf("overridign new content") + log.Debugf("overriding new content") if !ConfiguredYamlPreferences.FixMergeAnchorToSpec { + //TODO This also fires in when an earlier element in a list merge anchor overwrites a later element, which *is* to the spec log.Warning("--yaml-fix-merge-anchor-to-spec is false; causing the merge anchor to override the existing value at %v which isn't to the yaml spec. This flag will default to true in late 2025.", keyNode.GetNicePath()) valueEl.Value = value } diff --git a/pkg/yqlib/operator_anchors_aliases_test.go b/pkg/yqlib/operator_anchors_aliases_test.go index ec54fe6e..5f6dfacc 100644 --- a/pkg/yqlib/operator_anchors_aliases_test.go +++ b/pkg/yqlib/operator_anchors_aliases_test.go @@ -122,6 +122,7 @@ var anchorOperatorScenarios = []expressionScenario{ expression: ".[4] | explode(.)", expected: []string{"D0, P[4], (!!map)::r: 10\nx: 1\ny: 2\n"}, }, + //TODO The following 2 tests warn about overwriting [3].r not being to spec while they shouldn't { description: "Override", subdescription: "see https://yaml.org/type/merge.html", @@ -129,6 +130,15 @@ var anchorOperatorScenarios = []expressionScenario{ expression: ".[4] | explode(.)", expected: []string{"D0, P[4], (!!map)::r: 10\ny: 2\nx: 1\n"}, }, + // Correctly warns about overwriting [4].x + { + description: "Override with local key", + subdescription: "like https://yaml.org/type/merge.html, but with x: 1 before the merge key. " + + "This is legacy behavior, see --yaml-fix-merge-anchor-to-spec", + document: specDocument + "- x: 1\n << : [ *BIG, *LEFT, *SMALL ]\n", + expression: ".[4] | explode(.)", + expected: []string{"D0, P[4], (!!map)::x: 0\nr: 10\ny: 2\n"}, + }, { description: "Get anchor", document: `a: &billyBob cat`, diff --git a/pkg/yqlib/operator_traverse_path.go b/pkg/yqlib/operator_traverse_path.go index ecdeafbd..d0010b42 100644 --- a/pkg/yqlib/operator_traverse_path.go +++ b/pkg/yqlib/operator_traverse_path.go @@ -3,7 +3,6 @@ package yqlib import ( "container/list" "fmt" - "github.com/elliotchance/orderedmap" ) @@ -280,11 +279,39 @@ func doTraverseMap(newMatches *orderedmap.OrderedMap, node *CandidateNode, wante log.Debug("MATCHED") if prefs.IncludeMapKeys { log.Debug("including key") - newMatches.Set(key.GetKey(), key) + keyName := key.GetKey() + if newMatches.Has(keyName) { + if ConfiguredYamlPreferences.FixMergeAnchorToSpec { + log.Debug("not overwriting existing key") + } else { + log.Warning( + "--yaml-fix-merge-anchor-to-spec is false; "+ + "causing the merge anchor to override the existing key at %v which isn't to the yaml spec. "+ + "This flag will default to true in late 2025.", key.GetNicePath()) + log.Debug("overwriting existing key") + newMatches.Set(keyName, key) + } + } else { + newMatches.Set(keyName, key) + } } if !prefs.DontIncludeMapValues { log.Debug("including value") - newMatches.Set(value.GetKey(), value) + valueName := value.GetKey() + if newMatches.Has(valueName) { + if ConfiguredYamlPreferences.FixMergeAnchorToSpec { + log.Debug("not overwriting existing value") + } else { + log.Warning( + "--yaml-fix-merge-anchor-to-spec is false; "+ + "causing the merge anchor to override the existing value at %v which isn't to the yaml spec. "+ + "This flag will default to true in late 2025.", key.GetNicePath()) + log.Debug("overwriting existing value") + newMatches.Set(valueName, value) + } + } else { + newMatches.Set(valueName, value) + } } } } @@ -300,9 +327,7 @@ func traverseMergeAnchor(newMatches *orderedmap.OrderedMap, merge *CandidateNode case MappingNode: return doTraverseMap(newMatches, merge, wantedKey, prefs, splat) case SequenceNode: - // Earlier keys take precedence - for index := len(merge.Content) - 1; index >= 0; index = index - 1 { - childValue := merge.Content[index] + for _, childValue := range merge.Content { if childValue.Kind == AliasNode { childValue = childValue.Alias } diff --git a/pkg/yqlib/operator_traverse_path_test.go b/pkg/yqlib/operator_traverse_path_test.go index 9c83fa13..fadbf819 100644 --- a/pkg/yqlib/operator_traverse_path_test.go +++ b/pkg/yqlib/operator_traverse_path_test.go @@ -25,6 +25,26 @@ foobar: thing: foobar_thing ` +var fixedTraversePathOperatorScenarios = []expressionScenario{ + { + description: "Traversing merge anchor lists", + subdescription: "Note that the keys earlier in the merge anchors sequence override later ones", + document: mergeDocSample, + expression: `.foobarList.thing`, + expected: []string{ + "D0, P[foo thing], (!!str)::foo_thing\n", + }, + }, + { + description: "Traversing merge anchors with override", + document: mergeDocSample, + expression: `.foobar.c`, + expected: []string{ + "D0, P[foobar c], (!!str)::foobar_c\n", + }, + }, +} + var traversePathOperatorScenarios = []expressionScenario{ { skipDoc: true, @@ -400,9 +420,10 @@ var traversePathOperatorScenarios = []expressionScenario{ }, }, { - description: "Traversing merge anchors with override", - document: mergeDocSample, - expression: `.foobar.c`, + description: "Traversing merge anchors with override", + subdescription: "This is legacy behavior, see --yaml-fix-merge-anchor-to-spec", + document: mergeDocSample, + expression: `.foobar.c`, expected: []string{ "D0, P[foo c], (!!str)::foo_c\n", }, @@ -442,12 +463,13 @@ var traversePathOperatorScenarios = []expressionScenario{ }, }, { - description: "Traversing merge anchor lists", - subdescription: "Note that the keys earlier in the merge anchors sequence override later ones", - document: mergeDocSample, - expression: `.foobarList.thing`, + description: "Traversing merge anchor lists", + subdescription: "Note that the later merge anchors override previous, " + + "but this is legacy behavior, see --yaml-fix-merge-anchor-to-spec", + document: mergeDocSample, + expression: `.foobarList.thing`, expected: []string{ - "D0, P[foo thing], (!!str)::foo_thing\n", + "D0, P[bar thing], (!!str)::bar_thing\n", }, }, { @@ -467,14 +489,15 @@ var traversePathOperatorScenarios = []expressionScenario{ }, }, { - description: "Splatting merge anchor lists", - document: mergeDocSample, - expression: `.foobarList[]`, + description: "Splatting merge anchor lists", + subdescription: "With legacy override behavior, see --yaml-fix-merge-anchor-to-spec", + document: mergeDocSample, + expression: `.foobarList[]`, expected: []string{ "D0, P[bar b], (!!str)::bar_b\n", - "D0, P[foo thing], (!!str)::foo_thing\n", - "D0, P[foobarList c], (!!str)::foobarList_c\n", "D0, P[foo a], (!!str)::foo_a\n", + "D0, P[bar thing], (!!str)::bar_thing\n", + "D0, P[foobarList c], (!!str)::foobarList_c\n", }, }, { @@ -575,3 +598,11 @@ func TestTraversePathOperatorScenarios(t *testing.T) { } documentOperatorScenarios(t, "traverse-read", traversePathOperatorScenarios) } + +func TestTraversePathOperatorAlignedToSpecScenarios(t *testing.T) { + ConfiguredYamlPreferences.FixMergeAnchorToSpec = true + for _, tt := range fixedTraversePathOperatorScenarios { + testScenario(t, &tt) + } + ConfiguredYamlPreferences.FixMergeAnchorToSpec = false +}