From d9b02587a95eb6f850ac165fe1a01ca4c3aa77fc 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 | 154 ++++++---------------- 4 files changed, 130 insertions(+), 120 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 73b62b7f3b..458d6c54b9 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -152,6 +152,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 3e946740be..9a3d39a503 100644 --- a/lib/mongoid/interceptable.rb +++ b/lib/mongoid/interceptable.rb @@ -152,9 +152,13 @@ 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) + _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 @@ -235,9 +239,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 f13b44040b..90c7ecb59f 100644 --- a/spec/mongoid/interceptable_spec.rb +++ b/spec/mongoid/interceptable_spec.rb @@ -389,6 +389,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 a6b87957e3..48b4fba536 100644 --- a/spec/mongoid/interceptable_spec_models.rb +++ b/spec/mongoid/interceptable_spec_models.rb @@ -1,11 +1,13 @@ # rubocop:todo all 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 @@ -16,6 +18,8 @@ module CallbackTracking extend ActiveSupport::Concern included do + field :name, type: String + %i( validation save create update ).each do |what| @@ -35,199 +39,110 @@ 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 - - attr_accessor :callback_registry - before_save :test_mongoid_state - include CallbackTracking - private # Helps test that cascading child callbacks have access to the Mongoid @@ -238,6 +153,15 @@ 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 class InterceptableBand