Skip to content

fix: correct key order after add, flatten, keys, sort, reverse and shuffle #2388

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

antoinedeschenes
Copy link
Contributor

@antoinedeschenes antoinedeschenes commented Jun 2, 2025

Fixes issue #2387 where the wrong item is being deleted by del() after doing a add, flatten, sort, reverse or shuffle operation on a sequence.

What it does:

  1. Always set the key on CandidateNode.AddChild
  2. Ensure shuffle swaps the node keys when shuffling 2 items.
  3. Ensure key is set on result arrays from keys operator
  4. Adds tests validating the proper items are deleted.

Versions impacted: this issue started in v4.40.1

Comparison between yq 4.35.2, 4.45.4 and the proposed PR changes.

+ ./yq4.35.2 -n '.foo = ["lhs"] + ["rhs"] | del(.foo[1])'
foo:
  - lhs
+ ./yq4.45.4 -n '.foo = ["lhs"] + ["rhs"] | del(.foo[1])'
foo:
  - rhs
+ go run . -n '.foo = ["lhs"] + ["rhs"] | del(.foo[1])'
foo:
  - lhs
--------------------
+ ./yq4.35.2 -n '[3,2,1] | sort | del(.[2])'
- 1
- 2
+ ./yq4.45.4 -n '[3,2,1] | sort | del(.[2])'
- 2
- 3
+ go run . -n '[3,2,1] | sort | del(.[2])'
- 1
- 2
--------------------
+ ./yq4.35.2 -n '[3,[2],[[1]]] | flatten | del(.[2])'
- 3
- 2
+ ./yq4.45.4 -n '[3,[2],[[1]]] | flatten | del(.[2])'
- 2
- 1
+ go run . -n '[3,[2],[[1]]] | flatten | del(.[2])'
- 3
- 2
--------------------
+ ./yq4.35.2 -n '[3,2,1] | reverse | del(.[2])'
- 1
- 2
+ ./yq4.45.4 -n '[3,2,1] | reverse | del(.[2])'
- 2
- 3
+ go run . -n '[3,2,1] | reverse | del(.[2])'
- 1
- 2
--------------------
+ ./yq4.35.2 -n '{"a": 1, "b": 2, "c": 3} | keys | del(.[] | select(.=="b"))'
- a
- c
+ ./yq4.45.4 -n '{"a": 1, "b": 2, "c": 3} | keys | del(.[] | select(.=="b"))'
- a
- b
- c
+ go run . -n '{"a": 1, "b": 2, "c": 3} | keys | del(.[] | select(.=="b"))'
- a
- c
--------------------

@antoinedeschenes antoinedeschenes force-pushed the fix/add-sequence-clear-rhs-keys branch from 5aeeeb1 to 4eeda65 Compare June 2, 2025 21:38
@antoinedeschenes antoinedeschenes marked this pull request as draft June 3, 2025 20:40
@antoinedeschenes antoinedeschenes force-pushed the fix/add-sequence-clear-rhs-keys branch from 4eeda65 to 7d53f62 Compare June 3, 2025 23:04
@antoinedeschenes antoinedeschenes changed the title fix: addSequence: clear rhs node Keys to ensure index is assigned fix: correct key order after add, flatten, sort, reverse and shuffle Jun 3, 2025
@antoinedeschenes antoinedeschenes marked this pull request as ready for review June 3, 2025 23:25
@antoinedeschenes antoinedeschenes force-pushed the fix/add-sequence-clear-rhs-keys branch from 7d53f62 to 75173f5 Compare June 3, 2025 23:57
@antoinedeschenes antoinedeschenes changed the title fix: correct key order after add, flatten, sort, reverse and shuffle fix: correct key order after add, flatten, keys, sort, reverse and shuffle Jun 3, 2025
@antoinedeschenes antoinedeschenes force-pushed the fix/add-sequence-clear-rhs-keys branch from 75173f5 to 7f3370d Compare June 4, 2025 00:03
@nvanheuverzwijn
Copy link

That fixes so much stuff

@mikefarah
Copy link
Owner

This looks really good 👍🏼 👍🏼 I'm about to go on an extended holiday - so reluctant to merge it an atm as I'll be away if anything happens. Will def review when I get back :)

@mikefarah mikefarah merged commit b15ce77 into mikefarah:master Jun 7, 2025
3 checks passed
@mikefarah
Copy link
Owner

Thinking more, I'll merge it in but won't release till I get back 👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants