Skip to content
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

Fix mongoize update all array operators master #5823

Merged

Conversation

JohnMaguir
Copy link
Contributor

@JohnMaguir JohnMaguir commented May 20, 2024

Issue when using $pull or $pop with update_all
Atomic operators that deal with arrays or sets: $addToSet, $push, $pull, $pop, $pullAll
Only $addToSet and $push are covered by current tests and $pull, $pop functionality is broken as Mongoid::AtomicUpdatePreparer.mongoize_for returns nil for query such as update_all("$pull" => { genres: "electronic" })

This became an issue after fixes from #5814 as correct key is now being passed to Mongoid::AtomicUpdatePreparer.mongoize_for

Fix is to mongoize value the same way it is for $addToSet or $push.
Also added tests for $pullAll even though it is currently working but good to have all operators covered.

Corresponding fix for 8.1 branch - #5824

depeche_mode.update_attribute(:genres, ["pop", "electronic", "dance" ])
new_order.update_attribute(:genres, ["electronic", "pop", "dance"])
context.update_all("$pullAll" => { genres: ["pop", "electronic"] })
end
Copy link
Contributor

@dem dem May 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JohnMaguir for $pullAll it is better to prepare multiple entries in the data to be pulled from the array, just to cover it fully:

              depeche_mode.update_attribute(:genres, ["pop", "electronic", "dance", "pop" ])
              new_order.update_attribute(:genres, ["electronic", "pop", "electronic", "dance"])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @dem updated now

Copy link
Contributor

@jamis jamis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Thank you 🙇

@jamis jamis merged commit 47c844c into mongodb:master May 20, 2024
15 of 16 checks passed
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