New :fallback option to handle nil for singular associations#6149
Conversation
c92b19f to
98bb51a
Compare
|
Wow! Nice feature! 🚀 |
jamis
left a comment
There was a problem hiding this comment.
@cperezabo thank you for this PR -- it looks like a convenient feature.
However, there are some subtle potential side-effects that I'm a bit worried about:
- Associations with
dependent:set may cause problems. If you look at any of the_dependent_*helper methods (inassociation/depending.rb, you'll see they callsend(association.name)with nowithout_autobuildwrapper. This leaves them vulnerable to returning the fallback object, if one is defined---which could result inNoMethodErrorbeing raised whendestroyornullify(etc.) is called on that fallback. - Relying on
association.relation_classis a bit fraught. If you look at the comment inRelatable#relation_class, you'll seecalling this method on a polymorphic association will generally fail with a NameError or produce misleading results. Polymorphic associations may need to be treated specially in the guard you added indefine_setter!. - Nested attributes (specifically in
association/nested/one.rb) callparent.send(association.name), which may return the fallback object. Looking in thebuildmethod in that file, there are multiple opportunities forNoMethodErrorexceptions to be raised when processing a nested attribute update.
| end | ||
|
|
||
| if @options.key?(:autobuild) | ||
| raise Errors::InvalidRelationOption.new(@owner_class, name, :fallback, self.class::VALID_OPTIONS) |
There was a problem hiding this comment.
InvalidRelationOption is mostly intended for reporting an option that is not recognized. It might be better to simply raise ArgumentError here? Because the message with this exception won't say anything about :autobuild being invalid when :fallback is specified, which is the actual issue here.
|
Hi @jamis, I've addressed each of your points. I also scanned the repository with Claude to look for any other spots with the same problem, and it found two — I've added tests for those as well. There's a commit per point so they're easy to review. You can squash the branch or leave it as is; I don't mind either way! Just to clarify, I always follow the TDD approach, so the tests were added one by one, along with the production code. |
Introduces a :fallback option on belongs_to, has_one, and embeds_one that takes a Proc invoked whenever the association resolves to nil, returning a null object stand-in. Direct assignment of an instance that does not match the association's class is treated as nil so the null object never reaches the database.
The _dependent_*! helpers read the association with a bare send(name), so an association with a :fallback returned the null object instead of nil when no real document existed. Destroying the owner then raised NoMethodError (destroy/delete_all/nullify) or a false DeleteRestriction and aborted the destroy (restrict_with_*). Read through without_autobuild in all five helpers so the cascade sees the real value (nil when absent), matching how the rest of Mongoid's internals read associations.
The association setter guarded the :fallback null object with object.is_a?(relation_class). On a polymorphic belongs_to, relation_class cannot resolve a target class from the association name and raised NameError on assignment. Check against Mongoid::Document for polymorphic associations (any document is a valid target) and keep the relation_class check for the rest. A non-document assignment such as the null object is treated as nil so it never reaches the database.
Association::Nested::One#build reads the existing association with parent.send(name) to decide whether to update, replace, or delete the nested document. With a :fallback that returned the null object instead of nil, so build never took the replace branch and called document methods (_id, using_object_ids?, ...) on the null object, raising NoMethodError. When the association has a :fallback, read the real value through without_autobuild so build sees nil and builds the nested document. Autobuilding associations are left untouched.
Combining :fallback with :autobuild previously raised InvalidRelationOption, whose message lists the valid options and does not explain that the real problem is the combination. Raise ArgumentError with a message naming both options instead, matching the ArgumentError already raised for a non-callable :fallback.
serializable_hash serializes included associations through serialize_relations, which read the association with a bare send(name) outside the without_autobuild block that already wraps field serialization. With a :fallback that returned the null object, and serializable_hash(include: ...) then called serializable_hash on it, raising NoMethodError. When the association has a :fallback, read the value through without_autobuild so a nil association is omitted from the output. Autobuilding associations are left untouched.
The counter cache after_create/after_update/before_destroy callbacks read the parent with __send__(name) to adjust its counter. With a :fallback that returned the null object when no parent was set, and the callbacks then indexed into it (record[cache_column]), raising NoMethodError on create, update, or destroy. Read through without_autobuild so the callbacks operate on the real parent and no-op when there is none, matching how the cascade helpers in Depending read associations.
jamis
left a comment
There was a problem hiding this comment.
Thank you! This looks really good 👍
|
Thanks @jamis, regarding the documentation, I visited https://github.com/mongodb/docs-mongoid to make the changes. However, it states that the documentation is now internally managed. Is there any way for me to contribute? |
Thank you so much for being willing to contribute documentation! Your PR description was wonderfully descriptive, and I'll include it in the release notes for the next version (I made a few tweaks to make it appropriate for that audience). As for the docs at https://www.mongodb.com/docs -- those are all internally managed to maintain a consistent voice and presentation across all the different driver languages that we support. I don't even have direct access to those docs; it's managed by a separate documentation team. However, if there is anything specific you'd like highlighted in the docs (which do not need to be in the release notes), feel free to edit the PR description and add another header at the same level as the "Summary" header; I'll point the docs team at this PR when they ask how to document the feature. (The "Summary" header and its associated contents are used to build our release notes.) |
Summary
A
:fallbackoption has been added tobelongs_to,has_one, andembeds_one.This option accepts a
Proc, and whenever the association would benil, Mongoid returns whatever the proc gives back (i.e. it follows the "null object" pattern).It behaves similarly with
has_oneandembeds_one.Things to note:
nil— whether via setter or constructor.:autobuild(they want opposite things). RaisesInvalidRelationOption.Credit to @cperezabo for this feature.