MONGOID-5658 Fix callbacks for embedded (#5727)

* 5658

* 5658

* 5658

* Document the config option

* Add warning

* Add docstrings

* Update for ActiveSupport 7.1

* Add test case

* Add release notes

* Update specs

Author: comandeo-mongo

docs/release-notes/mongoid-9.0.txt

@@ -18,6 +18,23 @@ please consult GitHub releases for detailed release notes and JIRA for
 the complete list of issues fixed in each release, including bug fixes.
 
 
+``around_*`` callbacks for embedded documents are now ignored
+-------------------------------------------------------------
+
+Mongoid 8.x and older allows user to define ``around_*`` callbacks for embedded
+documents. Starting from 9.0 these callbacks are ignored and will not be executed.
+A warning will be printed to the console if such callbacks are defined.
+
+If you want to restore the old behavior, you can set
+``Mongoid.around_embedded_document_callbacks`` to true in your application.
+
+.. note::
+  Enabling ``around_*`` callbacks for embedded documents is not recommended
+  as it may cause ``SystemStackError`` exceptions when a document has many
+  embedded documents. See `MONGOID-5658 <https://jira.mongodb.org/browse/MONGOID-5658>`_
+  for more details.
+
+
 ``for_js`` method is deprecated
 -------------------------------
 

lib/mongoid/config.rb

@@ -137,6 +137,16 @@ module Config
     # See https://jira.mongodb.org/browse/MONGOID-5542
     option :prevent_multiple_calls_of_embedded_callbacks, default: true
 
+    # When this flag is false, callbacks for embedded documents will not be
+    # called. This is the default in 9.0.
+    #
+    # Setting this flag to true restores the pre-9.0 behavior, where callbacks
+    # for embedded documents are called. This may lead to stack overflow errors
+    # if there are more than cicrca 1000 embedded documents in the root
+    # document's dependencies graph.
+    # See https://jira.mongodb.org/browse/MONGOID-5658 for more details.
+    option :around_callbacks_for_embeds, default: false
+
     # Returns the Config singleton, for use in the configure DSL.
     #
     # @return [ self ] The Config singleton.

lib/mongoid/interceptable.rb

@@ -149,6 +149,28 @@ def run_callbacks(kind, with_children: true, skip_if: nil, &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)
+      else
+        _mongoid_run_child_callbacks_without_around(kind, children: children, &block)
+      end
+    end
+
+    # Execute the callbacks of given kind for embedded documents including
+    # around callbacks.
+    #
+    # @note This method is prone to stack overflow errors if the document
+    #   has a large number of embedded documents. It is recommended to avoid
+    #   using around callbacks for embedded documents until a proper solution
+    #   is implemented.
+    #
+    # @param [ Symbol ] kind The type of callback to execute.
+    # @param [ Array<Document> ] children Children to execute callbacks on. If
+    #  nil, callbacks will be executed on all cascadable children of
+    #  the document.
+    #
+    #  @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?
@@ -157,11 +179,73 @@ def _mongoid_run_child_callbacks(kind, children: nil, &block)
         child.run_callbacks(child_callback_type(kind, child), with_children: with_children, &block)
       else
         child.run_callbacks(child_callback_type(kind, child), with_children: with_children) do
-          _mongoid_run_child_callbacks(kind, children: tail, &block)
+          _mongoid_run_child_callbacks_with_around(kind, children: tail, &block)
         end
       end
     end
 
+    # Execute the callbacks of given kind for embedded documents without
+    # around callbacks.
+    #
+    # @param [ Symbol ] kind The type of callback to execute.
+    # @param [ Array<Document> ] children Children to execute callbacks on. If
+    #   nil, callbacks will be executed on all cascadable children of
+    #   the document.
+    #
+    # @api private
+    def _mongoid_run_child_callbacks_without_around(kind, children: nil, &block)
+      children = (children || cascadable_children(kind))
+      callback_list = _mongoid_run_child_before_callbacks(kind, children: children)
+      return false if callback_list == false
+      value = block&.call
+      callback_list.each do |_next_sequence, env|
+        env.value &&= value
+      end
+      return false if _mongoid_run_child_after_callbacks(callback_list: callback_list) == false
+
+      value
+    end
+
+    # Execute the before callbacks of given kind for embedded documents.
+    #
+    # @param [ Symbol ] kind The type of callback to execute.
+    # @param [ Array<Document> ] children Children to execute callbacks on.
+    # @param [ Array<ActiveSupport::Callbacks::CallbackSequence, ActiveSupport::Callbacks::Filters::Environment> ] callback_list List of
+    #   pairs of callback sequence and environment. This list will be later used
+    #   to execute after callbacks in reverse order.
+    #
+    # @api private
+    def _mongoid_run_child_before_callbacks(kind, children: [], callback_list: [])
+      children.each do |child|
+        chain = child.__callbacks[child_callback_type(kind, child)]
+        env = ActiveSupport::Callbacks::Filters::Environment.new(child, false, nil)
+        next_sequence = compile_callbacks(chain)
+        unless next_sequence.final?
+          Mongoid.logger.warn("Around callbacks are disabled for embedded documents. Skipping around callbacks for #{child.class.name}.")
+          Mongoid.logger.warn("To enable around callbacks for embedded documents, set Mongoid::Config.around_callbacks_for_embeds to true.")
+        end
+        next_sequence.invoke_before(env)
+        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
+
+    # Execute the after callbacks.
+    #
+    # @param [ Array<ActiveSupport::Callbacks::CallbackSequence, ActiveSupport::Callbacks::Filters::Environment> ] callback_list List of
+    #   pairs of callback sequence and environment.
+    def _mongoid_run_child_after_callbacks(callback_list: [])
+      callback_list.reverse_each do |next_sequence, env|
+        next_sequence.invoke_after(env)
+        return false if env.halted
+      end
+    end
+
     # Returns the stored callbacks to be executed later.
     #
     # @return [ Array<Symbol> ] Method symbols of the stored pending callbacks.
@@ -314,13 +398,7 @@ def run_targeted_callbacks(place, kind)
         end
         self.class.send :define_method, name do
           env = ActiveSupport::Callbacks::Filters::Environment.new(self, false, nil)
-          sequence = if chain.method(:compile).arity == 0
-                       # ActiveSupport < 7.1
-                       chain.compile
-                     else
-                       # ActiveSupport >= 7.1
-                       chain.compile(nil)
-                     end
+          sequence = compile_callbacks(chain)
           sequence.invoke_before(env)
           env.value = !env.halted
           sequence.invoke_after(env)
@@ -330,5 +408,24 @@ def run_targeted_callbacks(place, kind)
       end
       send(name)
     end
+
+    # Compile the callback chain.
+    #
+    # This method hides the differences between ActiveSupport implementations
+    # before and after 7.1.
+    #
+    # @param [ ActiveSupport::Callbacks::CallbackChain ] chain The callback chain.
+    # @param [ Symbol | nil ] type The type of callback chain to compile.
+    #
+    # @return [ ActiveSupport::Callbacks::CallbackSequence ] The compiled callback sequence.
+    def compile_callbacks(chain, type = nil)
+      if chain.method(:compile).arity == 0
+        # ActiveSupport < 7.1
+        chain.compile
+      else
+        # ActiveSupport >= 7.1
+        chain.compile(type)
+      end
+    end
   end
 end

spec/integration/callbacks_spec.rb

@@ -558,6 +558,7 @@ def will_save_change_to_attribute_values_before
 
   context 'nested embedded documents' do
     config_override :prevent_multiple_calls_of_embedded_callbacks, true
+    config_override :around_callbacks_for_embeds, true
 
     let(:logger) { Array.new }
 
@@ -582,4 +583,23 @@ def will_save_change_to_attribute_values_before
       expect(logger).to eq(%i[embedded_twice embedded_once root])
     end
   end
+
+  context 'cascade callbacks' do
+    ruby_version_gte '3.0'
+
+    let(:book) do
+      Book.new
+    end
+
+    before do
+      1500.times do
+        book.pages.build
+      end
+    end
+
+    # https://jira.mongodb.org/browse/MONGOID-5658
+    it 'does not raise SystemStackError' do
+      expect { book.save! }.not_to raise_error(SystemStackError)
+    end
+  end
 end