-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
MONGOID-5228 disallow _id to be updated on persisted documents #5542
Conversation
Previously, updating _id on any document (top-level or embedded) would silently ignore the _id attribute, making it appear as though the operation succeeded. This was misleading, since the _id didn't actually change. This change makes it so that _id is explicitly made immutable once a document has been persisted. The same behavior is intentionally enforced regardless of whether the document is embedded or not. If you find yourself needing to update the _id field in an embedded document, you'll need to use the lower-level driver methods to accomplish this. We believe modifying the _id field (even in embedded documents) is an anti-pattern.
This seems reasonable. I've never encountered a use case to change the _id of a persisted doc, embedded or otherwise. |
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.
I agree with the changes, very well done.
However, I believe the changes are breaking; therefore, we should reflect them in the changelog, and probably hide behind a feature flag. I can imagine these changes may break some apps.
lib/mongoid/persistable/updatable.rb
Outdated
# has been persisted. | ||
def enforce_immutability_of_id_field! | ||
if _id_changed? && persisted? | ||
raise Errors::ReadonlyAttribute.new(:_id, _id) |
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.
ReadonlyAttribute
error message contains the following wordings:
summary: "Attributes flagged as readonly via Model.attr_readonly
can only have values set when the document is a new record."
resolution: "Don't define '%{name}' as readonly, or do not attempt
to update its value after the document is persisted."
This is misleading in case of _id
field, since it is not explicitly flagged as readonly. I suggest raising a dedicated error here.
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.
Yeah, good point. I've updated the PR with a new ImmutableAttribute
exception.
@@ -32,6 +32,8 @@ def build(parent) | |||
parent.send(association.setter, Factory.build(@class_name, attributes)) | |||
elsif delete? | |||
parent.send(association.setter, nil) | |||
else | |||
check_for_id_violation! |
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.
Maybe rename to check_for_id_mutation!
or check_for_id_change!
? "Violation" in this context sounds like a malformed (i.e. non-BSON) ID. Similar with acceptable_id
ID below.
@jamis looks like tests are broken on master. See failing Github Actions runs in this PR. Raised https://jira.mongodb.org/browse/MONGOID-5565 to track. |
…) to master (#5561) * MONGOID-5334 ensure localized demongoization works with symbol keys (#5416) * MONGOID-5334 ensure localized demongoization works with symbol keys * MONGOID-5334 add type check * MONGOID-5334 change back type check * MONGOID-5437 extract fallback enabling logic into macro (#5428) * MONGOID-5438 initial changes from Johnny's PR * MONGOID-5438 additional changes from johnny's PR * MONGOID-5437 fix tests * MONGOID-5437 fix uniqueness test * MONGOID-5334 enforce Hash type in localized demongoize (#5430) * MONGOID-5334 add type check * MONGOID-5334 change back type check * MONGOID-5334 enforce Hash type in localized demongoize * MONGOID-5456 cast castable values for integer/float/big decimal (#5431) * MONGOID-5456 cast castable values for integer/float/big decimal * MONGOID-5456 assert duration * MONGOID-4403 Support Dirty Tracking changed from/to (#5432) * MONGOID-4403 Support Dirty Tracking changed from/to * Apply suggestions from code review Co-authored-by: Oleg Pudeyev <[email protected]> * MONGOID-4403 add false example Co-authored-by: Oleg Pudeyev <[email protected]> * MONGOID-5456 Add documentation for responding to to_* (#5434) * MONGOID-5456 Add documentation for responding to to_* * Apply suggestions from code review Co-authored-by: Oleg Pudeyev <[email protected]> Co-authored-by: Oleg Pudeyev <[email protected]> * MONGOID-5461 Change custom field options example (#5438) * MONGOID-5461 Change custom field options example * Apply suggestions from code review Co-authored-by: Oleg Pudeyev <[email protected]> Co-authored-by: Oleg Pudeyev <[email protected]> * MONGOID-5458 Document fallbacks option on localized fields (#5439) * MONGOID-5460 Update custom field type protocol documentation (#5441) * MONGOID-5457 Document + example of non-String field types in localized fields (#5444) * MONGOID-4698 update warning when overriding criteria methods in scope (#5442) * MONGOID-4698 update warning when overriding criteria methods in scope * Apply suggestions from code review Co-authored-by: Oleg Pudeyev <[email protected]> Co-authored-by: Oleg Pudeyev <[email protected]> * MONGOID-5417 Fix memory leak when call 'with' (#5409) * MONGOID-4528 Add more dirty methods (#5440) Co-authored-by: Oleg Pudeyev <[email protected]> * MONGOID-4698 Correct warning to point out that klass.band doesnt conflict (#5450) * MONGOID-5226 Allow setting "store_in collection" on document subclass (#5449) * MONGOID-5226 Allow setting "store_in collection" on document subclass * remove comment * MONGOID-5226 add docs and testing * Apply suggestions from code review Co-authored-by: Oleg Pudeyev <[email protected]> * MONGOID-5226 answer comments Co-authored-by: Oleg Pudeyev <[email protected]> * MONGOID-5293 put legacy_attributes in 7.5 defaults (#5425) * MONGOID-5293 put legacy_attributes in 7.5 * switch order * MONGOID-5293 fix defaults tests * MONGOID-5438 Implement local override for i18n parameters (#5427) * MONGOID-5438 Implement local override for i18n parameters * MONGOID-5438 get rid of enfore_available_locale changes * MONGOID-5438 get rid of before alls * MONGOID-5438 fix tests caused by rspec precedence * MONGOID-5438 attempt to fix tests when fallbacks disabled * MONGOID-5438 make fallbacks default to return [self] * MONGOID-5438 reset fallbacks * MONGOID-5438 attempt to fix tests * MONGOID-5438 retry_test * MONGOID-5438 undefine fallback methods after fallback tests * MONGOID-5438 add back defaults * MONGOID-5438 use new macros * MONGOID-5438 use before and after suite * MONGOID-5438 update mrss * MONGOID-5438 remove suite checks * MONGOID-5370 Add collection options support (#5452) Co-authored-by: Oleg Pudeyev <[email protected]> * MONGOID-5474 add readonly! and raise on save/update (#5455) * MONGOID-5474 add readonly! and raise on save/update * MONGOID-5474 flip flag for 9.0 and add default * MONGOID-5474 fix error tests * Update spec/mongoid/config_spec.rb * Apply suggestions from code review Co-authored-by: Oleg Pudeyev <[email protected]> * Update lib/mongoid/config.rb Co-authored-by: Oleg Pudeyev <[email protected]> Co-authored-by: Oleg Pudeyev <[email protected]> * MONGOID-5474/MONGOID-5478 add docs and release notes (#5457) * MONGOID-5474 add docs and release notes * MONGOID-5474 use read-only * MONGOID-5474 add configuration docs * MOGOID-5474 switch order of flag docs * add MONGOID-5477 use case * MONGOID-5477 / MONGOID-5474 / MONGOID-5478 add user test (#5459) * MONGOID-5474 add user test * MONGOID-5474 change to read-only * MOGOID-5474 switch order of flag docs * MONGOID-5476 update 8.1 installation version (#5464) * MONGOID-5474 flip the feature flag in 8.1 (#5456) * MONGOID-5474 flip the feature flag in 8.1 * MONGOID-5474 flip flag in configuration * MONGOID-5474 update config test * MONGOID-5227 Add test and adjust docs for dotted attribute assignment in default scope (#5474) * MONGOID-5227 Add test and adjust docs for dotted attribute assignment in default scope * MONGOID-5227 adjust docs * MONGOID-5227 clarify the docs and add tests * Update docs/reference/queries.txt Co-authored-by: Oleg Pudeyev <[email protected]> Co-authored-by: Oleg Pudeyev <[email protected]> * MONGOID-5493: Update Gem::Specification team info (#5476) * MONGOID-5312 Document attributes method in reference (#5477) * MONGOID-5312 Document attributes method in reference * Apply suggestions from code review Co-authored-by: Oleg Pudeyev <[email protected]> * MONGOID-5312 update languagw Co-authored-by: Oleg Pudeyev <[email protected]> * MONGOID-5082 Create contributing guidelines (#5472) * MONGOID-5082 Create contributing guidelines * MONGOID-5082 propose test coverage * MONGOID-5082 whitespace * MONGOID-5082 update language * add intro * MONGOID-5445 Add load_async to criteria (#5454) Co-authored-by: Neil Shweky <[email protected]> Co-authored-by: Oleg Pudeyev <[email protected]> * MONGOID-5291 Clarify whether :touch option is applicable for all updates (#5481) * MONGOID-5261 Use config_override when a test requires a certain configuration option value (#5479) * MONGOID-5261 Use config_override when a test requires a certain configuration option value * fix syntax error * MONGOID-5261 a little more cleanup * MONGOID-5261 more cleanup * more clean up * MONGOID-5261 add persistence context override * MONGOID-5261 add comment * MONGOID-5100 allow selector arguments for .exists? (#5466) * MONGOID-5100 allow selector arguments for .exists? * MONGOID-5100 add examples to the docstrings * MONGOID-5100 add docs and release notes * Empty-Commit * MONGOID-5100 allow false to be passed * MONGOID-5100 update docs and docstrings * MONGOID-5100 remove false as a param * MONGOID-5100 put back nil/false * MONGOID-5100 fix memory tests and fix limit == 0 * MONGOID-5262 Implement local time zone override for tests (#5480) * MONGOID-5506 Update 8.x docs to document unscoped behavior changing in 9.0 (#5483) * MONGOID-5506 Update 8.x docs to document unscoped behavior changing in 9.0 * Update lib/mongoid/scopable.rb * RUBY-2942: Update documentation links (#5482) * MONGOID-5363 Change #upsert to perform updating upsert rather than replacing upsert (#5492) * MONGOID-5363 add :replace option to upsert method * MONGOID-5363 add docs, release notes for 8.1/9.0 * MONGOID-5363 switch default in 8.1 (#5493) * MONGOID-5363 update docstrings, flag default, and tests for 8.1 * specify default in mongoid 9 * MONGOID-5331 Document hash assignment to associations (#5494) * MONGOID-5331 change buildable docs * MONGOID-5331 add proxy tests and fix embedded_in proxy * MONGOID-5331 update models * Update spec/mongoid/association/embedded/embedded_in/proxy_spec.rb * MONGOID-5331 update doc strings in batchable * MONGOID-5331 add docs * MONGOID-5331 add docs/release notes * Apply suggestions from code review * Update spec/mongoid/association/embedded/embedded_in/proxy_spec.rb * RUBY-3097 add/use monotonic time for deadlines (#5491) * RUBY-3097 add/use monotonic time for deadlines * remove license and fix tests * MONGOID-3834 Document association availability in custom getters and setters (#5503) * MONGOID-3834 Document association availability in custom getters and setters * add callbacks note * MONGOID-5496: Update Getting Started (Rails) tutorial for Rails 7 (#5509) * Clone Rails Tutorial and create variation for Rails 7 * Update Existing Application docs for Rails 7 * MONGOID-5532: Fix path to comments partial (#5511) * MONGOID-5533: Unnecessary edits to Comment model in Rails Getting Started (#5512) * Add note about workaround for MONGOID-4885 in Rails 6 docs * Fix whitespace * MONGOID-5521 migrate Mongoid docs to snooty (#5513) * fix code blocks * MONGOID-5521 fix all parse errors * build docs with github action * Revert "build docs with github action" This reverts commit 9ffda44. * MONGOID-5496: Update Rails 7 Tutorial (#5515) * Remove `--skip-bundle` from _Optionally Skip Tests_ section * MONGOID-5518: Add Puma dependency to Sinatra tutorial (#5516) * MONGOID-5453 Add .none_of query method (#5524) * "none_of" query method * update docs and release notes * fix underline for header * MONGOID-5539: Fix example in aggregation reference documentation (#5536) * Update aggregation.txt * Update aggregation.txt * MONGOID-5228 disallow _id to be updated on persisted documents (ported to 8.1-stable) (#5545) * port #5542 to 8.1-stable * test tweak * update 8.1 release notes to mention immutable_ids * MONGOID-5490: Deprecate :use_activesupport_time_zone (#5488) * Deprecate the use_activesupport_time_zone_deprecated config option. * Typo * Update config.rb * Update mongoid-8.1.txt --------- Co-authored-by: shields <[email protected]> * MONGOID-5509: Deprecate all feature flags which were introduced in Mongoid 7.x (#5489) * Deprecate the use_activesupport_time_zone_deprecated config option. * Typo * Mongoid 8.1 should deprecate all feature flags which were introduced in the 7.x series, with intent to remove them in Mongoid 9.0. --------- Co-authored-by: shields <[email protected]> * Remove :use_activesupport_time_zone option + improve readme around timezone usage given the change * Remove deprecated :broken_aggregables config option * Remove deprecated :broken_alias_handling config option * Remove deprecated :broken_and config option * Remove deprecated :broken_scoping config option * Remove deprecated :broken_updates config option * Remove deprecated :compare_time_by_ms config option * Remove deprecated :legacy_attributes config option * Remove deprecated :legacy_pluck_distinct config option * Remove deprecated :legacy_triple_equals config option * Remove deprecated :object_id_as_json_oid config option * Remove deprecated :overwrite_chained_operators config option * Drop support for config.load_defaults versions "7.3", "7.4", "7.5", as their options are no longer available. * Fix broken spec in Mongoid 8.1 --------- Co-authored-by: Neil Shweky <[email protected]> Co-authored-by: Oleg Pudeyev <[email protected]> Co-authored-by: Dmitry Rybakov <[email protected]> Co-authored-by: Alex Bevilacqua <[email protected]> Co-authored-by: Jamis Buck <[email protected]> Co-authored-by: Jamis Buck <[email protected]> Co-authored-by: shields <[email protected]>
Previously, updating
_id
on any document (top-level or embedded) would behave inconsistently. In some cases, the_id
would be ignored, and the other updates would proceed. In another case, the entire update would be ignored. And in yet other cases, the update would succeed.The specific cases were:
_id
of a top-level document via either#save
or#update
: the new_id
was ignored, and the rest of the update (if any) succeeded._id
of an embedded document (either#embeds_one
or#embeds_many
) via#save
or#update
: the new_id
was accepted and persisted to the database._id
of an embedded document (#embeds_one
) via an update to the parent (e.g.parent.update(child: { _id: ... })
): the entire update to the child was ignored, including both the_id
field and any other attributes being updated.This change makes it so that
_id
is explicitly made immutable once a document has been persisted. The same behavior is intentionally enforced regardless of whether the document is embedded or not. If you find yourself needing to update the_id
field in an embedded document, you'll need to use the lower-level driver methods to accomplish this. We believe modifying the_id
field (even in embedded documents) is an anti-pattern.For legacy purposes, if you have an app that currently relies on updating the
_id
field of embedded documents, a new feature flag has been introduced:The default is
true
, which enforces the immutability of the_id
field on all documents. Setting this feature flag tofalse
will restore the legacy behavior (as described above).