From 154f4a4ba96b8f22e87084c2a82192e51c3a2362 Mon Sep 17 00:00:00 2001 From: Joshua Young Date: Mon, 30 Oct 2023 18:45:35 +0530 Subject: [PATCH] [Fix 48688] Duplicate callback execution when child autosaves parent with has_one and belongs_to --- activerecord/CHANGELOG.md | 9 ++ .../lib/active_record/autosave_association.rb | 56 +++++++----- .../test/cases/autosave_association_test.rb | 20 +++++ activerecord/test/models/eye.rb | 90 ++++++++++++++----- 4 files changed, 133 insertions(+), 42 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 7eaa825f05..2d3e6723ad 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,12 @@ +* Fix duplicate callback execution when child autosaves parent with `has_one` and `belongs_to`. + + Before, persisting a new child record with a new associated parent record would run `before_validation`, + `after_validation`, `before_save` and `after_save` callbacks twice. + + Now, these callbacks are only executed once as expected. + + *Joshua Young* + * `ActiveRecord::Encryption::Encryptor` now supports a `:compressor` option to customize the compression algorithm used. ```ruby diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index b6a178f139..a0926471b5 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -317,7 +317,12 @@ def nested_records_changed_for_autosave? def validate_single_association(reflection) association = association_instance_get(reflection.name) record = association && association.reader - association_valid?(association, record) if record && (record.changed_for_autosave? || custom_validation_context?) + return unless record && (record.changed_for_autosave? || custom_validation_context?) + + inverse_belongs_to_association = inverse_belongs_to_association_for(reflection, record) + return if inverse_belongs_to_association && inverse_belongs_to_association.updated? + + association_valid?(association, record) end # Validate the associated records if :validate or @@ -429,36 +434,44 @@ def save_collection_association(reflection) def save_has_one_association(reflection) association = association_instance_get(reflection.name) record = association && association.load_target + return unless record && !record.destroyed? - if record && !record.destroyed? - autosave = reflection.options[:autosave] + inverse_belongs_to_association = inverse_belongs_to_association_for(reflection, record) + return if inverse_belongs_to_association && inverse_belongs_to_association.updated? - if autosave && record.marked_for_destruction? - record.destroy - elsif autosave != false - primary_key = Array(compute_primary_key(reflection, self)).map(&:to_s) - primary_key_value = primary_key.map { |key| _read_attribute(key) } + autosave = reflection.options[:autosave] - if (autosave && record.changed_for_autosave?) || _record_changed?(reflection, record, primary_key_value) - unless reflection.through_reflection - foreign_key = Array(reflection.foreign_key) - primary_key_foreign_key_pairs = primary_key.zip(foreign_key) + if autosave && record.marked_for_destruction? + record.destroy + elsif autosave != false + primary_key = Array(compute_primary_key(reflection, self)).map(&:to_s) + primary_key_value = primary_key.map { |key| _read_attribute(key) } - primary_key_foreign_key_pairs.each do |primary_key, foreign_key| - association_id = _read_attribute(primary_key) - record[foreign_key] = association_id unless record[foreign_key] == association_id - end - association.set_inverse_instance(record) + if (autosave && record.changed_for_autosave?) || _record_changed?(reflection, record, primary_key_value) + unless reflection.through_reflection + foreign_key = Array(reflection.foreign_key) + primary_key_foreign_key_pairs = primary_key.zip(foreign_key) + + primary_key_foreign_key_pairs.each do |primary_key, foreign_key| + association_id = _read_attribute(primary_key) + record[foreign_key] = association_id unless record[foreign_key] == association_id end - - saved = record.save(validate: !autosave) - raise ActiveRecord::Rollback if !saved && autosave - saved + association.set_inverse_instance(record) end + + saved = record.save(validate: !autosave) + raise ActiveRecord::Rollback if !saved && autosave + saved end end end + def inverse_belongs_to_association_for(reflection, record) + reflection.inverse_of && + reflection.inverse_of.belongs_to? && + record.association(reflection.inverse_of.name) + end + # If the record is new or it has changed, returns true. def _record_changed?(reflection, record, key) record.new_record? || @@ -480,7 +493,6 @@ def inverse_polymorphic_association_changed?(reflection, record) return false unless reflection.inverse_of&.polymorphic? class_name = record._read_attribute(reflection.inverse_of.foreign_type) - reflection.active_record != record.class.polymorphic_class_for(class_name) end diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index 31d4ba753a..2f99d067f1 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -304,6 +304,26 @@ def test_callbacks_firing_order_on_save assert_equal [false, false, false, false], eye.after_save_callbacks_stack end + def test_callbacks_on_child_when_parent_autosaves_child + eye = Eye.create(iris: Iris.new) + assert_equal 1, eye.iris.before_validation_callbacks_counter + assert_equal 1, eye.iris.before_create_callbacks_counter + assert_equal 1, eye.iris.before_save_callbacks_counter + assert_equal 1, eye.iris.after_validation_callbacks_counter + assert_equal 1, eye.iris.after_create_callbacks_counter + assert_equal 1, eye.iris.after_save_callbacks_counter + end + + def test_callbacks_on_child_when_child_autosaves_parent + iris = Iris.create(eye: Eye.new) + assert_equal 1, iris.before_validation_callbacks_counter + assert_equal 1, iris.before_create_callbacks_counter + assert_equal 1, iris.before_save_callbacks_counter + assert_equal 1, iris.after_validation_callbacks_counter + assert_equal 1, iris.after_create_callbacks_counter + assert_equal 1, iris.after_save_callbacks_counter + end + def test_foreign_key_attribute_is_not_set_unless_changed eye = Eye.create!(iris_with_read_only_foreign_key_attributes: { color: "honey" }) assert_nothing_raised do diff --git a/activerecord/test/models/eye.rb b/activerecord/test/models/eye.rb index 626d32c1ed..8e22c29215 100644 --- a/activerecord/test/models/eye.rb +++ b/activerecord/test/models/eye.rb @@ -19,35 +19,85 @@ class Eye < ActiveRecord::Base after_update :trace_after_update2, if: :iris after_save :trace_after_save2, if: :iris - def trace_after_create - (@after_create_callbacks_stack ||= []) << !iris.persisted? - end - alias trace_after_create2 trace_after_create + private + def trace_after_create + (@after_create_callbacks_stack ||= []) << !iris.persisted? + end + alias trace_after_create2 trace_after_create - def trace_after_update - (@after_update_callbacks_stack ||= []) << iris.has_changes_to_save? - end - alias trace_after_update2 trace_after_update + def trace_after_update + (@after_update_callbacks_stack ||= []) << iris.has_changes_to_save? + end + alias trace_after_update2 trace_after_update - def trace_after_save - (@after_save_callbacks_stack ||= []) << iris.has_changes_to_save? - end - alias trace_after_save2 trace_after_save + def trace_after_save + (@after_save_callbacks_stack ||= []) << iris.has_changes_to_save? + end + alias trace_after_save2 trace_after_save - has_one :iris_with_read_only_foreign_key, class_name: "IrisWithReadOnlyForeignKey", foreign_key: :eye_id - accepts_nested_attributes_for :iris_with_read_only_foreign_key + public + has_one :iris_with_read_only_foreign_key, class_name: "IrisWithReadOnlyForeignKey", foreign_key: :eye_id + accepts_nested_attributes_for :iris_with_read_only_foreign_key - before_save :set_iris_with_read_only_foreign_key_color_to_blue, if: -> { - iris_with_read_only_foreign_key && @override_iris_with_read_only_foreign_key_color - } + before_save :set_iris_with_read_only_foreign_key_color_to_blue, if: -> { + iris_with_read_only_foreign_key && @override_iris_with_read_only_foreign_key_color + } - def set_iris_with_read_only_foreign_key_color_to_blue - iris_with_read_only_foreign_key.color = "blue" - end + private + def set_iris_with_read_only_foreign_key_color_to_blue + iris_with_read_only_foreign_key.color = "blue" + end end class Iris < ActiveRecord::Base + attr_reader :before_validation_callbacks_counter + attr_reader :before_create_callbacks_counter + attr_reader :before_save_callbacks_counter + + attr_reader :after_validation_callbacks_counter + attr_reader :after_create_callbacks_counter + attr_reader :after_save_callbacks_counter + belongs_to :eye + + before_validation :update_before_validation_counter + before_create :update_before_create_counter + before_save :update_before_save_counter + + after_validation :update_after_validation_counter + after_create :update_after_create_counter + after_save :update_after_save_counter + + private + def update_before_validation_counter + @before_validation_callbacks_counter ||= 0 + @before_validation_callbacks_counter += 1 + end + + def update_before_create_counter + @before_create_callbacks_counter ||= 0 + @before_create_callbacks_counter += 1 + end + + def update_before_save_counter + @before_save_callbacks_counter ||= 0 + @before_save_callbacks_counter += 1 + end + + def update_after_validation_counter + @after_validation_callbacks_counter ||= 0 + @after_validation_callbacks_counter += 1 + end + + def update_after_create_counter + @after_create_callbacks_counter ||= 0 + @after_create_callbacks_counter += 1 + end + + def update_after_save_counter + @after_save_callbacks_counter ||= 0 + @after_save_callbacks_counter += 1 + end end class IrisWithReadOnlyForeignKey < Iris