MONGOID-5836 Don't repeat callbacks for embedded grandchildren. (back…

…port to 8.0-stable) (#5937)

* MONGOID-5836 Don't repeat callbacks for embedded grandchildren. (#5933)

* MONGOID-5836 don't explicitly process grandchildren

Grandchildren are already accounted for when this is invoked,
because if called when callbacks are cascading, the input is
the full list of all children and grandchildren. Processing
grandchildren recursively here causes them to be processed twice.

* specify python version to address failing mongo-orchestration

* make sure we don't break update callbacks

* don't recursively process children here

the list of children already includes all descendants

* MONGOID-5542 Prevent multiple calls of embedded docs callbacks (#5621) (#5626)

* Add test

* Fix double callback call

* Change test that expects the old behavior

* Add config option

* Flip the flag

---------

Co-authored-by: Dmitry Rybakov <[email protected]>

.github/workflows/test.yml

@@ -293,6 +293,13 @@ jobs:
       uses: actions/checkout@v2
       with:
         submodules: recursive
+
+    # the default python 3.8 doesn't cut it, and causes mongo-orchestration
+    # to fail in mongodb-labs/drivers-evergreen-tools.
+    - uses: actions/setup-python@v5
+      with:
+        python-version: '3.13'
+
     - id: start-mongodb
       name: start mongodb
       uses: mongodb-labs/drivers-evergreen-tools@master

lib/mongoid/config.rb

@@ -142,6 +142,16 @@ module Config
       end
     end
 
+    # When this flag is true, callbacks for every embedded document will be
+    # called only once, even if the embedded document is embedded in multiple
+    # documents in the root document's dependencies graph.
+    # This will be the default in 9.0. Setting this flag to false restores the
+    # pre-9.0 behavior, where callbacks are called for every occurrence of an
+    # embedded document. The pre-9.0 behavior leads to a problem that for multi
+    # level nested documents callbacks are called multiple times.
+    # See https://jira.mongodb.org/browse/MONGOID-5542
+    option :prevent_multiple_calls_of_embedded_callbacks, default: false
+
     # When this flag is true, callbacks for embedded documents will not be
     # called. This is the default in 8.x, but will be changed to false in 9.0.
     #

lib/mongoid/interceptable.rb

@@ -141,9 +141,13 @@ def run_callbacks(kind, with_children: true, &block)
     # @api private
     def _mongoid_run_child_callbacks(kind, children: nil, &block)
       if Mongoid::Config.around_callbacks_for_embeds
-        _mongoid_run_child_callbacks_with_around(kind, children: children, &block)
+        _mongoid_run_child_callbacks_with_around(kind,
+                                                 children: children,
+                                                 &block)
       else
-        _mongoid_run_child_callbacks_without_around(kind, children: children, &block)
+        _mongoid_run_child_callbacks_without_around(kind,
+                                                    children: children,
+                                                    &block)
       end
     end
 
@@ -163,13 +167,14 @@ def _mongoid_run_child_callbacks(kind, children: nil, &block)
     #  @api private
     def _mongoid_run_child_callbacks_with_around(kind, children: nil, &block)
       child, *tail = (children || cascadable_children(kind))
+      with_children = !Mongoid::Config.prevent_multiple_calls_of_embedded_callbacks
       if child.nil?
         block&.call
       elsif tail.empty?
-        child.run_callbacks(child_callback_type(kind, child), &block)
+        child.run_callbacks(child_callback_type(kind, child), with_children: with_children, &block)
       else
-        child.run_callbacks(child_callback_type(kind, child)) do
-          _mongoid_run_child_callbacks_with_around(kind, children: tail, &block)
+        child.run_callbacks(child_callback_type(kind, child), with_children: with_children) do
+          _mongoid_run_child_callbacks(kind, children: tail, &block)
         end
       end
     end
@@ -218,9 +223,6 @@ def _mongoid_run_child_before_callbacks(kind, children: [], callback_list: [])
         return false if env.halted
         env.value = !env.halted
         callback_list << [next_sequence, env]
-        if (grandchildren = child.send(:cascadable_children, kind))
-          _mongoid_run_child_before_callbacks(kind, children: grandchildren, callback_list: callback_list)
-        end
       end
       callback_list
     end

spec/integration/callbacks_models.rb

@@ -153,3 +153,40 @@ class Building
 
   has_and_belongs_to_many :architects, dependent: :nullify
 end
+
+class Root
+  include Mongoid::Document
+  embeds_many :embedded_once, cascade_callbacks: true
+  after_save :trace
+
+  attr_accessor :logger
+
+  def trace
+    logger << :root
+  end
+end
+
+class EmbeddedOnce
+  include Mongoid::Document
+  embeds_many :embedded_twice, cascade_callbacks: true
+  embedded_in :root
+  after_save :trace
+
+  attr_accessor :logger
+
+  def trace
+    logger << :embedded_once
+  end
+end
+
+class EmbeddedTwice
+  include Mongoid::Document
+  embedded_in :embedded_once
+  after_save :trace
+
+  attr_accessor :logger
+
+  def trace
+    logger << :embedded_twice
+  end
+end

spec/integration/callbacks_spec.rb

@@ -467,4 +467,31 @@ class PreviouslyPersistedPerson
       expect { book.save! }.not_to raise_error(SystemStackError)
     end
   end
+
+  context 'nested embedded documents' do
+    config_override :prevent_multiple_calls_of_embedded_callbacks, true
+
+    let(:logger) { Array.new }
+
+    let(:root) do
+      Root.new(
+        embedded_once: [
+          EmbeddedOnce.new(
+            embedded_twice: [EmbeddedTwice.new]
+          )
+        ]
+      )
+    end
+
+    before(:each) do
+      root.logger = logger
+      root.embedded_once.first.logger = logger
+      root.embedded_once.first.embedded_twice.first.logger = logger
+    end
+
+    it 'runs callbacks in the correct order' do
+      root.save!
+      expect(logger).to eq(%i[embedded_twice embedded_once root])
+    end
+  end
 end

spec/mongoid/interceptable_spec.rb

@@ -388,6 +388,86 @@ class TestClass
         end
       end
     end
+
+    context 'with embedded grandchildren' do
+      IS = InterceptableSpec
+
+      config_override :prevent_multiple_calls_of_embedded_callbacks, true
+
+      context 'when creating' do
+        let(:registry) { IS::CallbackRegistry.new(only: %i[ before_save ]) }
+
+        let(:expected_calls) do
+          [
+            # the parent
+            [ IS::CbParent, :before_save ],
+
+            # the immediate child of the parent
+            [ IS::CbCascadedNode, :before_save ],
+
+            # the grandchild of the parent
+            [ IS::CbCascadedNode, :before_save ],
+          ]
+        end
+
+        let!(:parent) do
+          parent = IS::CbParent.new(registry)
+          child = IS::CbCascadedNode.new(registry)
+          grandchild = IS::CbCascadedNode.new(registry)
+
+          child.cb_cascaded_nodes = [ grandchild ]
+          parent.cb_cascaded_nodes = [ child ]
+
+          parent.tap(&:save)
+        end
+
+        it 'should cascade callbacks to grandchildren' do
+          expect(registry.calls).to be == expected_calls
+        end
+      end
+
+      context 'when updating' do
+        let(:registry) { IS::CallbackRegistry.new(only: %i[ before_update ]) }
+
+        let(:expected_calls) do
+          [
+            # the parent
+            [ IS::CbParent, :before_update ],
+
+            # the immediate child of the parent
+            [ IS::CbCascadedNode, :before_update ],
+
+            # the grandchild of the parent
+            [ IS::CbCascadedNode, :before_update ],
+          ]
+        end
+
+        let!(:parent) do
+          parent = IS::CbParent.new(nil)
+          child = IS::CbCascadedNode.new(nil)
+          grandchild = IS::CbCascadedNode.new(nil)
+
+          child.cb_cascaded_nodes = [ grandchild ]
+          parent.cb_cascaded_nodes = [ child ]
+
+          parent.save
+
+          parent.callback_registry = registry
+          child.callback_registry = registry
+          grandchild.callback_registry = registry
+
+          parent.name = 'updated'
+          child.name = 'updated'
+          grandchild.name = 'updated'
+
+          parent.tap(&:save)
+        end
+
+        it 'should cascade callbacks to grandchildren' do
+          expect(registry.calls).to be == expected_calls
+        end
+      end
+    end
   end
 
   describe ".before_destroy" do
@@ -578,6 +658,26 @@ class TestClass
           band.notes.push(note)
           record.notes.push(note)
         end
+
+        context "when saving the root" do
+          context 'with prevent_multiple_calls_of_embedded_callbacks enabled' do
+            config_override :prevent_multiple_calls_of_embedded_callbacks, true
+
+            it "executes the callbacks only once for each document" do
+              expect(note).to receive(:update_saved).once
+              band.save!
+            end
+          end
+
+          context 'with prevent_multiple_calls_of_embedded_callbacks disabled' do
+            config_override :prevent_multiple_calls_of_embedded_callbacks, false
+
+            it "executes the callbacks once for each ember" do
+              expect(note).to receive(:update_saved).twice
+              band.save!
+            end
+          end
+        end
       end
     end