Avoid corrupt documents (#248)

mongoid · Nov 27, 2020 · dfed5c3 · dfed5c3
1 parent 2935da0
commit dfed5c3
Show file tree

Hide file tree

Showing 5 changed files with 110 additions and 70 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,7 @@
 * [#239](https://github.com/mongoid/mongoid-history/pull/239): Optimize `modified_attributes_for_create` 6-7x - [@getaroom](https://github.com/getaroom).
 * [#240](https://github.com/mongoid/mongoid-history/pull/240): `Mongoid::History.disable` and `disable_tracking` now restore the original state - [@getaroom](https://github.com/getaroom).
 * [#240](https://github.com/mongoid/mongoid-history/pull/240): Added `Mongoid::History.enable`, `Mongoid::History.enable!`, `Mongoid::History.disable!`, `enable_tracking`, `enable_tracking!`, and `disable_tracking!` - [@getaroom](https://github.com/getaroom).
+* [#248](https://github.com/mongoid/mongoid-history/pull/248): Don't update version on embedded documents if an ancestor is being destroyed in the same operation - [@getaroom](https://github.com/getaroom).
 
 ### 0.8.2 (2019/12/02)
 

diff --git a/lib/mongoid/history/trackable.rb b/lib/mongoid/history/trackable.rb
@@ -139,6 +139,10 @@ def _create_relation(name, value)
 
         private
 
+        def ancestor_flagged_for_destroy?(doc)
+          doc && (doc.flagged_for_destroy? || ancestor_flagged_for_destroy?(doc._parent))
+        end
+
         def get_versions_criteria(options_or_version)
           if options_or_version.is_a? Hash
             options = options_or_version
@@ -183,9 +187,7 @@ def related_scope
         end
 
         def traverse_association_chain(node = self)
-          list = node._parent ? traverse_association_chain(node._parent) : []
-          list << association_hash(node)
-          list
+          (node._parent ? traverse_association_chain(node._parent) : []).tap { |list| list << association_hash(node) }
         end
 
         def association_hash(node = self)
@@ -269,10 +271,7 @@ def track_destroy(&block)
         end
 
         def clear_trackable_memoization
-          @history_tracker_attributes = nil
-          @modified_attributes_for_create = nil
-          @modified_attributes_for_update = nil
-          @history_tracks = nil
+          @history_tracker_attributes = @modified_attributes_for_create = @modified_attributes_for_update = @history_tracks = nil
         end
 
         # Transform hash of pair of changes into an `original` and `modified` hash
@@ -312,10 +311,12 @@ def expand_nested_document_key_value(document_key, value)
           expanded_key
         end
 
+        def next_version
+          (send(history_trackable_options[:version_field]) || 0) + 1
+        end
+
         def increment_current_version
-          current_version = (send(history_trackable_options[:version_field]) || 0) + 1
-          send("#{history_trackable_options[:version_field]}=", current_version)
-          current_version
+          next_version.tap { |version| send("#{history_trackable_options[:version_field]}=", version) }
         end
 
         protected
@@ -326,7 +327,7 @@ def track_history_for_action?(action)
 
         def track_history_for_action(action)
           if track_history_for_action?(action)
-            current_version = increment_current_version
+            current_version = ancestor_flagged_for_destroy?(_parent) ? next_version : increment_current_version
             last_track = self.class.tracker_class.create!(
               history_tracker_attributes(action.to_sym)
               .merge(version: current_version, action: action.to_s, trackable: self)

diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
@@ -27,4 +27,6 @@
   config.before :each do
     Mongoid::History.reset!
   end
+
+  config.include ErrorHelpers
 end
diff --git a/spec/support/error_helpers.rb b/spec/support/error_helpers.rb
@@ -0,0 +1,7 @@
+module ErrorHelpers
+  def ignore_errors
+    yield
+  rescue StandardError => e
+    Mongoid.logger.debug "ignored error #{e}"
+  end
+end
diff --git a/spec/unit/trackable_spec.rb b/spec/unit/trackable_spec.rb
@@ -15,6 +15,26 @@ class MyDynamicModel
       include Mongoid::Attributes::Dynamic unless Mongoid::Compatibility::Version.mongoid3?
     end
 
+    class MyDeeplyNestedModel
+      include Mongoid::Document
+      include Mongoid::History::Trackable
+
+      embeds_many :children, class_name: 'MyNestableModel', cascade_callbacks: true # The problem only occurs if callbacks are cascaded
+      accepts_nested_attributes_for :children, allow_destroy: true
+      track_history modifier_field: nil
+    end
+
+    class MyNestableModel
+      include Mongoid::Document
+      include Mongoid::History::Trackable
+
+      embedded_in :parent, class_name: 'MyDeeplyNestedModel'
+      embeds_many :children, class_name: 'MyNestableModel', cascade_callbacks: true
+      accepts_nested_attributes_for :children, allow_destroy: true
+      field :name, type: String
+      track_history modifier_field: nil
+    end
+
     class HistoryTracker
       include Mongoid::History::Tracker
     end
@@ -29,6 +49,8 @@ class User
     Object.send(:remove_const, :MyDynamicModel)
     Object.send(:remove_const, :HistoryTracker)
     Object.send(:remove_const, :User)
+    Object.send(:remove_const, :MyDeeplyNestedModel)
+    Object.send(:remove_const, :MyNestableModel)
   end
 
   let(:user) { User.create! }
@@ -322,24 +344,14 @@ class MySubModel < MyModel
           end
 
           it 'should be rescued if an exception occurs in disable_tracking' do
-            begin
-              MyModel.disable_tracking do
-                raise 'exception'
-              end
-            rescue
-            end
+            ignore_errors { MyModel.disable_tracking { raise 'exception' } }
             expect(Mongoid::History.enabled?).to eq(true)
             expect(MyModel.new.track_history?).to eq(true)
           end
 
           it 'should be rescued if an exception occurs in enable_tracking' do
             MyModel.disable_tracking do
-              begin
-                MyModel.enable_tracking do
-                  raise 'exception'
-                end
-              rescue
-              end
+              ignore_errors { MyModel.enable_tracking { raise 'exception' } }
               expect(Mongoid::History.enabled?).to eq(true)
               expect(MyModel.new.track_history?).to eq(false)
             end
@@ -436,25 +448,15 @@ class MyModel2
 
           it 'should be rescued if an exception occurs in disable' do
             Mongoid::History.disable do
-              begin
-                MyModel.disable_tracking do
-                  raise 'exception'
-                end
-              rescue
-              end
+              ignore_errors { MyModel.disable_tracking { raise 'exception' } }
               expect(Mongoid::History.enabled?).to eq(false)
               expect(MyModel.new.track_history?).to eq(false)
             end
           end
 
           it 'should be rescued if an exception occurs in enable' do
             Mongoid::History.disable do
-              begin
-                Mongoid::History.enable do
-                  raise 'exception'
-                end
-              rescue
-              end
+              ignore_errors { Mongoid::History.enable { raise 'exception' } }
               expect(Mongoid::History.enabled?).to eq(false)
               expect(MyModel.new.track_history?).to eq(false)
             end
@@ -500,14 +502,7 @@ class MyModel2
         end
 
         it 'should rescue errors through both local and global tracking scopes' do
-          begin
-            Mongoid::History.disable do
-              MyModel.disable_tracking do
-                raise 'exception'
-              end
-            end
-          rescue
-          end
+          ignore_errors { Mongoid::History.disable { MyModel.disable_tracking { raise 'exception' } } }
           expect(Mongoid::History.enabled?).to eq(true)
           expect(MyModel.new.track_history?).to eq(true)
         end
@@ -698,13 +693,9 @@ class ModelTwo
         end
       end
 
-      after :each do
-        Object.send(:remove_const, :ModelTwo)
-      end
+      after(:each) { Object.send(:remove_const, :ModelTwo) }
 
-      before :each do
-        ModelTwo.history_settings paranoia_field: :neglected_at
-      end
+      before(:each) { ModelTwo.history_settings paranoia_field: :neglected_at }
 
       it { expect(Mongoid::History.trackable_settings[:ModelTwo]).to eq(paranoia_field: 'nglt') }
     end
@@ -716,9 +707,7 @@ class MyTrackerClass
       end
     end
 
-    after :each do
-      Object.send(:remove_const, :MyTrackerClass)
-    end
+    after(:each) { Object.send(:remove_const, :MyTrackerClass) }
 
     context 'when options contain tracker_class_name' do
       context 'when underscored' do
@@ -767,9 +756,7 @@ class EmbOne
         Object.send(:remove_const, :EmbOne)
       end
 
-      before :each do
-        model_one.save!
-      end
+      before(:each) { model_one.save! }
 
       let(:model_one) { ModelOne.new(foo: 'Foo') }
       let(:changes) { {} }
@@ -832,9 +819,7 @@ class EmbOne
   end
 
   describe '#track_update' do
-    before :each do
-      MyModel.track_history(on: :foo, track_update: true)
-    end
+    before(:each) { MyModel.track_history(on: :foo, track_update: true) }
 
     let!(:m) { MyModel.create!(foo: 'bar', modifier: user) }
 
@@ -851,9 +836,7 @@ class EmbOne
   end
 
   describe '#track_destroy' do
-    before :each do
-      MyModel.track_history(on: :foo, track_destroy: true)
-    end
+    before(:each) { MyModel.track_history(on: :foo, track_destroy: true) }
 
     let!(:m) { MyModel.create!(foo: 'bar', modifier: user) }
 
@@ -867,6 +850,58 @@ class EmbOne
         expect { m.destroy }.to raise_error(StandardError)
       end.to change(Tracker, :count).by(0)
     end
+
+    context 'with a deeply nested model' do
+      let(:m) do
+        MyDeeplyNestedModel.create!(
+          children: [
+            MyNestableModel.new(
+              name: 'grandparent',
+              children: [
+                MyNestableModel.new(name: 'parent 1', children: [MyNestableModel.new(name: 'child 1')]),
+                MyNestableModel.new(name: 'parent 2', children: [MyNestableModel.new(name: 'child 2')])
+              ]
+            )
+          ]
+        )
+      end
+      let(:attributes) do
+        {
+          'children_attributes' => [
+            {
+              'id' =>  m.children[0].id,
+              'children_attributes' => [
+                { 'id' => m.children[0].children[0].id, '_destroy' => '0' },
+                { 'id' => m.children[0].children[1].id, '_destroy' => '1' }
+              ]
+            }
+          ]
+        }
+      end
+
+      subject(:updated) do
+        m.update_attributes attributes
+        m.reload
+      end
+
+      let(:names_of_destroyed) do
+        MyDeeplyNestedModel.tracker_class
+                           .where('association_chain.id' => updated.id, 'action' => 'destroy')
+                           .map { |track| track.original['name'] }
+      end
+
+      it 'does not corrupt embedded models' do
+        expect(updated.children[0].children.count).to eq 1 # When the problem occurs, the 2nd child will continue to be present, but will only contain the version attribute
+      end
+
+      it 'creates a history track for the doc explicitly destroyed' do
+        expect(names_of_destroyed).to include 'parent 2'
+      end
+
+      it 'creates a history track for the doc implicitly destroyed' do
+        expect(names_of_destroyed).to include 'child 2'
+      end
+    end
   end
 
   describe '#track_create' do
@@ -879,9 +914,7 @@ class MyModelWithNoModifier
       end
     end
 
-    after :each do
-      Object.send(:remove_const, :MyModelWithNoModifier)
-    end
+    after(:each) { Object.send(:remove_const, :MyModelWithNoModifier) }
 
     before :each do
       MyModel.track_history(on: :foo, track_create: true)
@@ -919,9 +952,7 @@ class Fish
       end
     end
 
-    after :each do
-      Object.send(:remove_const, :Fish)
-    end
+    after(:each) { Object.send(:remove_const, :Fish) }
 
     it 'should track history' do
       expect do
@@ -947,9 +978,7 @@ def my_changes
       MyModel.history_trackable_options
     end
 
-    after :each do
-      Object.send(:remove_const, :CustomTracker)
-    end
+    after(:each) { Object.send(:remove_const, :CustomTracker) }
 
     it 'should not override in parent class' do
       expect(MyModel.history_trackable_options[:changes_method]).to eq :changes