Fix MONGOID-4982 Cannot update a field on embedded association member…

…, add another member and add a nested association to the first member in the same save call (#4934) * Fix MONGOID-4982 "conflicts within conflicts" issue * revert removed test code * fix the test * adjust line length * tweak the comment prose * note future work Co-authored-by: shields <[email protected]> Co-authored-by: Oleg Pudeyev <[email protected]>
mongodb · Dec 14, 2020 · 06d17ca · 06d17ca
1 parent 030aded
commit 06d17ca
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 7 deletions.
diff --git a/lib/mongoid/persistable/updatable.rb b/lib/mongoid/persistable/updatable.rb
@@ -137,8 +137,33 @@ def update_document(options = {})
             coll = collection(_root)
             selector = atomic_selector
             coll.find(selector).update_one(positionally(selector, updates), session: _session)
-            conflicts.each_pair do |key, value|
-              coll.find(selector).update_one(positionally(selector, { key => value }), session: _session)
+
+            # The following code applies updates which would cause
+            # path conflicts in MongoDB, for example when changing attributes
+            # of foo.0.bars while adding another foo. Each conflicting update
+            # is applied using its own write.
+            #
+            # TODO: MONGOID-5026: reduce the number of writes performed by
+            # more intelligently combining the writes such that there are
+            # fewer conflicts.
+            conflicts.each_pair do |modifier, changes|
+
+              # Group the changes according to their root key which is
+              # the top-level association name.
+              # This handles at least the cases described in MONGOID-4982.
+              conflicting_change_groups = changes.group_by do |key, _|
+                key.split(".", 2).first
+              end.values
+
+              # Apply changes in batches. Pop one change from each
+              # field-conflict group round-robin until all changes
+              # have been applied.
+              while batched_changes = conflicting_change_groups.map(&:pop).compact.to_h.presence
+                coll.find(selector).update_one(
+                  positionally(selector, modifier => batched_changes),
+                  session: _session,
+                )
+              end
             end
           end
         end

diff --git a/spec/mongoid/persistable/savable_spec.rb b/spec/mongoid/persistable/savable_spec.rb
@@ -293,6 +293,8 @@
           expect(truck.crates[1].volume).to eq 0.8
           expect(truck.crates[1].toys.size).to eq 0
 
+          # TODO: MONGOID-5026: combine the updates so that there are
+          # no conflicts.
           #expect(truck.atomic_updates[:conflicts]).to eq nil
 
           expect { truck.save! }.not_to raise_error
@@ -360,8 +362,6 @@
 
         context 'when also updating first embedded top level association' do
           it 'performs all writes' do
-            pending 'https://jira.mongodb.org/browse/MONGOID-4982'
-
             truck.crates.first.volume = 2
             truck.crates.first.toys.build(name: 'Bear')
             truck.crates.build
@@ -397,8 +397,6 @@
 
         context 'when embedded association embeds another association' do
           it 'persists the new documents' do
-            pending 'https://jira.mongodb.org/browse/MONGOID-4982'
-
             expect(truck.seats.size).to eq 1
             expect(truck.seats[0].rating).to eq 1
 
@@ -409,7 +407,7 @@
 
             _truck = Truck.find(truck.id)
             expect(_truck.seats.size).to eq 2
-            expect(_truck.seats[0].rating).to eq 1
+            expect(_truck.seats[0].rating).to eq 2
             expect(_truck.seats[0].armrests.length).to eq 1
             expect(_truck.seats[1].rating).to eq 100
           end