-
Notifications
You must be signed in to change notification settings - Fork 1.4k
MONGOID-5900 Fix #pluck on associations #6067
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes several bugs in the #pluck implementation for associations in Mongoid. It adds proper support for localized fields (respecting current locale and fallbacks), handles both loaded and unloaded association records correctly, and properly resolves field aliases.
Key changes:
- Extracts common pluck functionality into a new
Pluckablemodule for code reuse - Updates
has_manyandembeds_manyassociations to properly handle plucking from mixed loaded/unloaded/added documents - Adds comprehensive test coverage for localized fields, aliased fields, and embedded documents
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/mongoid/pluckable.rb | New shared module providing pluck functionality with support for field aliases, localization, and demongoization |
| lib/mongoid/contextual/mongo.rb | Refactored to use the new Pluckable module, removing duplicated code |
| lib/mongoid/association/referenced/has_many/enumerable.rb | Added pluck implementation that handles loaded, unloaded, and added documents |
| lib/mongoid/association/referenced/has_many/proxy.rb | Added pluck delegation to the enumerable target |
| lib/mongoid/association/embedded/embeds_many/proxy.rb | Added pluck delegation to criteria alongside existing find delegation |
| spec/support/models/*.rb | Added test models with localized and aliased fields to support comprehensive testing |
| spec/mongoid/association/referenced/has_many/enumerable_spec.rb | Added extensive test coverage for pluck functionality including localization, aliases, and embedded documents |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
spec/mongoid/association/referenced/has_many/enumerable_spec.rb
Outdated
Show resolved
Hide resolved
|
Hi Jamis, great, thank you for handling this. I noticed you extracted out a Pluckable concern. After merging this PR, I would encourage you to go a step further and consider merging my
|
* use the specialized implementation of pluck * use a custom pluck implementation * new file * incorporate Johnny's changes from mongodb#6044 * use same submodules as master * Make changes suggested by copilot review * rubocop appeasement * fix broken refactoring
* use the specialized implementation of pluck * use a custom pluck implementation * new file * incorporate Johnny's changes from #6044 * use same submodules as master * Make changes suggested by copilot review * rubocop appeasement * fix broken refactoring
This addresses a few bugs with the implementation of
#pluckon associations.Tests from #6044 (from @johnnyshields) are included in this PR (thank you, Johnny!).