Merge pull request rails#49847 from joshuay03/duplicate-callbacks-when-child-autosaves-parent

[Fix rails#48688] Duplicate callback execution when child autosaves parent with `has_one` and `belongs_to`

Author: byroot

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

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 <tt>:validate</tt> 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)
-              end
+          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)
 
-              saved = record.save(validate: !autosave)
-              raise ActiveRecord::Rollback if !saved && autosave
-              saved
+              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)
             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
 

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

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
-
-  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
-
-  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
-  }
-
-  def set_iris_with_read_only_foreign_key_color_to_blue
-    iris_with_read_only_foreign_key.color = "blue"
-  end
+  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_save
+      (@after_save_callbacks_stack ||= []) << iris.has_changes_to_save?
+    end
+    alias trace_after_save2 trace_after_save
+
+  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
+    }
+
+  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