From 23512308fbfa9ba2e1ead689faa9e78a0bd22903 Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Fri, 24 Jan 2025 10:13:10 -0700 Subject: [PATCH] MONGOID-5836 Don't repeat callbacks for embedded grandchildren (backport to 8.1-stable) (#5936) * 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 * prevent duplicate callbacks for these tests --- .github/workflows/test.yml | 7 + lib/mongoid/interceptable.rb | 11 +- spec/mongoid/interceptable_spec.rb | 80 +++++++++++ spec/mongoid/interceptable_spec_models.rb | 158 +++++++--------------- 4 files changed, 140 insertions(+), 116 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 9391380e09..5895eb4436 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -184,6 +184,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 fda2dab567..0940619ff9 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 @@ -219,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 diff --git a/spec/mongoid/interceptable_spec.rb b/spec/mongoid/interceptable_spec.rb index 5baf2c461b..da1fa19a87 100644 --- a/spec/mongoid/interceptable_spec.rb +++ b/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 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