Fix some issues around merge key/anchor handling #2400
Conversation
- Allow inline maps instead of just aliases - Disallow nested sequences - Disallow other types Closes mikefarah#2386
|
Wait, this actually doesn't fix |
- Allow inline maps instead of just aliases - Allow aliased sequences - Disallow other types - Use tag `!!merge` instead of key `<<` - Fix insertion index for sequence merge Closes mikefarah#2386
stevenwdv
changed the title
stevenwdv
changed the title
|
EDIT: nvm fixed it Ok, the excessive exploding fix I mentioned above may actually cause problems of its own: However, I don't really know how to solve this, because it depends on if the mapping node to which the merge anchor refers is also being exploded, e.g. with And apparently this issue is already present when partially exploding: |
|
Minor comment, need more time to review this and consider if there are any side effects - thanks for the contribution though! |
| document: mergeDocSample, | ||
| expression: `.foobarList.thing`, | ||
| expected: []string{ | ||
| "D0, P[bar thing], (!!str)::bar_thing\n", | ||
| "D0, P[foo thing], (!!str)::foo_thing\n", |
There was a problem hiding this comment.
Oooh nice find - this isn't to the YAML spec and it's works opposite to explode 🤦🏼 but this is a breaking change... and will cause all sorts of issues for people :/ Hmmmm
There was a problem hiding this comment.
I'm happy to go through with this PR if either; this fix is not included, or it's controlled via a new flag, maybe --yaml-correct-merge-order or something, defaulting to false. Should have a warning in there if the flag is not set and the logic is executed.
Then later with enough notice given, I can update the default flag value to true.
There was a problem hiding this comment.
A flag may be a good idea, I'll have a look at it later. Maybe the same could be done for #2110.
There was a problem hiding this comment.
I think it could be done under the same flag, I'm happy to work on 2110 and adding a flag, then perhaps you can add the if check into this PR?
There was a problem hiding this comment.
I've added a flag --yaml-fix-merge-anchor-to-spec available as a check in code as ConfiguredYamlPreferences.FixMergeAnchorToSpec. If you can add this for the fix you've done in traverse that'd be ace 👍🏼 You can see how I've tested true/false in the operator_anchor_aliases_test.go
There was a problem hiding this comment.
Nice! I put the fixed overriding behind the flag. You missed fixing #2110 for traversing (only exploding), but I could fix it both for local keys (#2110) & list merge keys at once anyway.
I also noticed that after 6e8cc00 it'll warn about overriding behavior with list merge anchor keys as well (e.g. {<<: [{a: 1}, {a: 2}]}), while this behavior is and was already correct (explode(.).a = 1), see the TODOs I added. Maybe you know of a good fix? Or add 'unless...' to the warning..?
There was a problem hiding this comment.
Oh, I just remembered, in the main branch the flag currently actually breaks list merge anchors for explode... 🙃
Edit: added a testcase for this
There was a problem hiding this comment.
I just realized that the way I fixed #2110 for operator_traverse_path actually breaks overriding keys in regular maps... :( I added a failing test.
I really wonder what the rationale behind the ordering in the merge keys spec was..
and fix traversing map with merge key that would override local key (completes mikefarah#2110 fix)
The one in fixedTraversePathOperatorScenarios still fails
This PR fixes multiple issues related to merge keys (
!!merge <<):{<<: {a: 42}},{<<: [{a: 42}]}), both for traversing and exploding (closes Merge key with inline map broken #2386){s: &s [{a: 42}], m: {<<: *s}}), both for traversing and exploding (closes Explode with merge aliases throw an error #2178){<<: [[{a: 42}]]}{<<: 42}{a: &a 42, m: {<<: *a}}were reported but cases like{m: {<<: 42}}were not. I chose to ignore all errors because otherwise traversing such a map (e.g. to correct types) becomes impossible.explode. Previously explode would just leave out invalid merge keys. Another potential solution would be to leave them as-is.echo '{b: &b {a: &a 42}, r: *a, c: {<<: *b}}' | yq 'explode(.c)'would yield{b: &b {a: 42}, r: *a, c: {a: 42}}, makingr: *aa dangling reference. Now it yields{b: &b {a: &a 42}, r: *a, c: {a: &a 42}}.!!merge) rather than value (<<).I do still have a question: I saw in
operator_traverse_path_test.gothat goccy would reject at least some invalid merge types at parse time, which might make using custom tags like<<: !include file.yaml(see #2386) impossible. Am I right about that? And does that mean that this usage will not be supported in the future? If so, that would be a bit sad. See also the note above about ignoring invalid merge keys.