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.
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.