Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Two moderate review comments remain unresolved in the regression spec.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes MongoDB path conflicts when inserting an embeds_many child alongside pending touch updates.
Changes:
- Detects conflicting touch paths before merging updates.
- Defers conflicting touches to the callback round-trip.
- Adds regression coverage for MONGOID-5972.
File summaries
| File | Summary | Review comments |
|---|---|---|
spec/mongoid/persistable/creatable_5972_spec.rb |
Adds regression coverage for sibling insertion and touch persistence. | Moderate (2 votes): deregister dynamically defined models during teardown. Moderate (1 vote): assert the sibling’s updated_at after reload. |
lib/mongoid/persistable/creatable.rb |
Adds equal, ancestor, and descendant touch-path conflict detection. | No comments. |
Review details
Suppressed comments (1)
spec/mongoid/persistable/creatable_5972_spec.rb:59
- Could this regression test also assert the first item's
updated_atafter reload? The new branch deliberately skips merging the$set, so the key behavior is that the after-save fallback still persists the touch; the currentqty/archivedassertions would pass even if that deferred touch were lost.
it 'persists the updated first item' do
first = parent.reload.m5972_items.first
expect(first.qty).to eq(2)
expect(first.archived).to be(false)
end
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Inserting an embeds_many child while a sibling had a pending touch update merged items.0.updated_at into the same $push update_one, which MongoDB rejected with error 40. Skip the touch merge when a touch path would conflict with a path the insert targets, deferring the touches to the after_save callback's separate round-trip.
jamis
force-pushed
the
5972-embeds-many
branch
from
September 11, 2026 21:56
cf6d8a2 to
1d5054f
Compare
jamis
requested review from
comandeo-mongo
and removed request for
JamesKovacs
September 11, 2026 21:58
jamis
marked this pull request as draft
September 11, 2026 22:26
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
MONGOID-5972: Inserting an
embeds_manychild while another child of the same array has a pending touch update raisesMongo::Error::OperationFailure [40].Description
Fixes a regression introduced by MONGOID-5867 (Merge touch updates into embedded document insert).
insert_as_embeddedmerged the parent's pending touch updates into the sameupdate_onethat performed the embedded insert. Forembeds_many, the insert produces$pushon the array while a dirty sibling can make the touches include a path inside that array (e.g.items.0.updated_at). MongoDB rejects a single update whose operators target both a path and one of its ancestors or descendants, so the write failed with error 40. Before 9.1.0 the touch was persisted in its ownupdate_oneand the two never shared an update document.The merge is now skipped when any touch path conflicts (equal, ancestor, or descendant) with a path the insert already targets. Such touches are deferred to the
after_savecallback's separate round-trip, matching pre-9.1 behavior. The single-round-trip optimization in MONGOID-5867 is preserved for the common case.Verification
describe 'touch merged with embedded insert'inspec/mongoid/touchable_spec.rb(the embedded push/create touch-merge coverage). It fails on master with error 40 and passes with the fix.