From 09189cb19a97e517a68afb39582702ceaf886d73 Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Wed, 15 Jan 2025 09:39:12 -0700 Subject: [PATCH] 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 --- .github/workflows/test.yml | 7 + lib/mongoid/interceptable.rb | 11 +- spec/mongoid/interceptable_spec.rb | 78 +++++++++++ spec/mongoid/interceptable_spec_models.rb | 158 +++++++--------------- 4 files changed, 138 insertions(+), 116 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index a1e69ecfeb..eb49be24e7 100644 --- a/.github/workflows/test.yml +++ b/.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 diff --git a/lib/mongoid/interceptable.rb b/lib/mongoid/interceptable.rb index 34fb684940..fbd0e5246e 100644 --- a/lib/mongoid/interceptable.rb +++ b/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 @@ -218,9 +222,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 diff --git a/spec/mongoid/interceptable_spec.rb b/spec/mongoid/interceptable_spec.rb index 1c75b0c94c..ab2ed5574f 100644 --- a/spec/mongoid/interceptable_spec.rb +++ b/spec/mongoid/interceptable_spec.rb @@ -388,6 +388,84 @@ class TestClass end end end + + context 'with embedded grandchildren' do + IS = InterceptableSpec + + 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 diff --git a/spec/mongoid/interceptable_spec_models.rb b/spec/mongoid/interceptable_spec_models.rb index 71e7680899..7b72afee8b 100644 --- a/spec/mongoid/interceptable_spec_models.rb +++ b/spec/mongoid/interceptable_spec_models.rb @@ -1,10 +1,12 @@ module InterceptableSpec class CallbackRegistry - def initialize + def initialize(only: []) @calls = [] + @only = only end def record_call(cls, cb) + return unless @only.empty? || @only.any? { |pat| pat == cb } @calls << [cls, cb] end @@ -15,6 +17,8 @@ module CallbackTracking extend ActiveSupport::Concern included do + field :name, type: String + %i( validation save create update ).each do |what| @@ -34,196 +38,128 @@ module CallbackTracking end end end - end - class CbHasOneParent - include Mongoid::Document - - has_one :child, autosave: true, class_name: "CbHasOneChild", inverse_of: :parent + attr_accessor :callback_registry - def initialize(callback_registry) + def initialize(callback_registry, *args, **kwargs) @callback_registry = callback_registry - super() + super(*args, **kwargs) end + end - attr_accessor :callback_registry - + module RootInsertable def insert_as_root @callback_registry&.record_call(self.class, :insert_into_database) super end + end + class CbHasOneParent + include Mongoid::Document include CallbackTracking + include RootInsertable + + has_one :child, autosave: true, class_name: "CbHasOneChild", inverse_of: :parent end class CbHasOneChild include Mongoid::Document + include CallbackTracking belongs_to :parent, class_name: "CbHasOneParent", inverse_of: :child - - def initialize(callback_registry) - @callback_registry = callback_registry - super() - end - - attr_accessor :callback_registry - - include CallbackTracking end class CbHasManyParent include Mongoid::Document + include CallbackTracking + include RootInsertable has_many :children, autosave: true, class_name: "CbHasManyChild", inverse_of: :parent - - def initialize(callback_registry) - @callback_registry = callback_registry - super() - end - - attr_accessor :callback_registry - - def insert_as_root - @callback_registry&.record_call(self.class, :insert_into_database) - super - end - - include CallbackTracking end class CbHasManyChild include Mongoid::Document + include CallbackTracking belongs_to :parent, class_name: "CbHasManyParent", inverse_of: :children - - def initialize(callback_registry) - @callback_registry = callback_registry - super() - end - - attr_accessor :callback_registry - - include CallbackTracking end class CbEmbedsOneParent include Mongoid::Document + include CallbackTracking + include RootInsertable field :name embeds_one :child, cascade_callbacks: true, class_name: "CbEmbedsOneChild", inverse_of: :parent - - def initialize(callback_registry) - @callback_registry = callback_registry - super() - end - - attr_accessor :callback_registry - - def insert_as_root - @callback_registry&.record_call(self.class, :insert_into_database) - super - end - - include CallbackTracking end class CbEmbedsOneChild include Mongoid::Document + include CallbackTracking field :age embedded_in :parent, class_name: "CbEmbedsOneParent", inverse_of: :child - - def initialize(callback_registry) - @callback_registry = callback_registry - super() - end - - attr_accessor :callback_registry - - include CallbackTracking end class CbEmbedsManyParent include Mongoid::Document + include CallbackTracking + include RootInsertable embeds_many :children, cascade_callbacks: true, class_name: "CbEmbedsManyChild", inverse_of: :parent - - def initialize(callback_registry) - @callback_registry = callback_registry - super() - end - - attr_accessor :callback_registry - - def insert_as_root - @callback_registry&.record_call(self.class, :insert_into_database) - super - end - - include CallbackTracking end class CbEmbedsManyChild include Mongoid::Document + include CallbackTracking embedded_in :parent, class_name: "CbEmbedsManyParent", inverse_of: :children - - def initialize(callback_registry) - @callback_registry = callback_registry - super() - end - - attr_accessor :callback_registry - - include CallbackTracking end class CbParent include Mongoid::Document - - def initialize(callback_registry) - @callback_registry = callback_registry - super() - end - - attr_accessor :callback_registry + include CallbackTracking embeds_many :cb_children embeds_many :cb_cascaded_children, cascade_callbacks: true - - include CallbackTracking + embeds_many :cb_cascaded_nodes, cascade_callbacks: true, as: :parent end class CbChild include Mongoid::Document + include CallbackTracking embedded_in :cb_parent - - def initialize(callback_registry, options) - @callback_registry = callback_registry - super(options) - end - - attr_accessor :callback_registry - - include CallbackTracking end class CbCascadedChild include Mongoid::Document + include CallbackTracking embedded_in :cb_parent - def initialize(callback_registry, options) - @callback_registry = callback_registry - super(options) - end + before_save :test_mongoid_state - attr_accessor :callback_registry + private + + # Helps test that cascading child callbacks have access to the Mongoid + # state objects; if the implementation uses fiber-local (instead of truly + # thread-local) variables, the related tests will fail because the + # cascading child callbacks use fibers to linearize the recursion. + def test_mongoid_state + Mongoid::Threaded.stack('interceptable').push(self) + end + end + class CbCascadedNode + include Mongoid::Document include CallbackTracking + + embedded_in :parent, polymorphic: true + + embeds_many :cb_cascaded_nodes, cascade_callbacks: true, as: :parent end end