Prevent double firing the before save callback of new object when the…

… parent association saved in the callback

Related rails#18155, rails#26661, 268a5bb, rails#27434, rails#27442, and rails#28599.

Originally rails#18155 was introduced for preventing double insertion caused
by the after save callback. But it was caused the before save issue
(rails#26661). 268a5bb fixed rails#26661, but it was caused the performance
regression (rails#27434). rails#27442 added new record to `target` before calling
callbacks for fixing rails#27434. But it was caused double firing before save
callback (rails#28599). We cannot add new object to `target` before saving
the object.

This is improving rails#18155 to only track callbacks after `save`.

Fixes rails#28599.

activerecord/lib/active_record/associations/collection_association.rb

@@ -280,35 +280,6 @@ def add_to_target(record, skip_callbacks = false, &block)
         replace_on_target(record, index, skip_callbacks, &block)
       end
 
-      def replace_on_target(record, index, skip_callbacks)
-        callback(:before_add, record) unless skip_callbacks
-
-        begin
-          if index
-            record_was = target[index]
-            target[index] = record
-          else
-            target << record
-          end
-
-          set_inverse_instance(record)
-
-          yield(record) if block_given?
-        rescue
-          if index
-            target[index] = record_was
-          else
-            target.delete(record)
-          end
-
-          raise
-        end
-
-        callback(:after_add, record) unless skip_callbacks
-
-        record
-      end
-
       def scope
         scope = super
         scope.none! if null_scope?
@@ -385,15 +356,19 @@ def _create_record(attributes, raise = false, &block)
             transaction do
               add_to_target(build_record(attributes)) do |record|
                 yield(record) if block_given?
-                insert_record(record, true, raise)
+                insert_record(record, true, raise) { @_was_loaded = loaded? }
               end
             end
           end
         end
 
         # Do the relevant stuff to insert the given record into the association collection.
-        def insert_record(record, validate = true, raise = false)
-          raise NotImplementedError
+        def insert_record(record, validate = true, raise = false, &block)
+          if raise
+            record.save!(validate: validate, &block)
+          else
+            record.save(validate: validate, &block)
+          end
         end
 
         def create_scope
@@ -448,19 +423,41 @@ def replace_common_records_in_memory(new_target, original_target)
           end
         end
 
-        def concat_records(records, should_raise = false)
+        def concat_records(records, raise = false)
           result = true
 
           records.each do |record|
             raise_on_type_mismatch!(record)
-            add_to_target(record) do |rec|
-              result &&= insert_record(rec, true, should_raise) unless owner.new_record?
+            add_to_target(record) do
+              result &&= insert_record(record, true, raise) { @_was_loaded = loaded? } unless owner.new_record?
             end
           end
 
           result && records
         end
 
+        def replace_on_target(record, index, skip_callbacks)
+          callback(:before_add, record) unless skip_callbacks
+
+          set_inverse_instance(record)
+
+          @_was_loaded = true
+
+          yield(record) if block_given?
+
+          if index
+            target[index] = record
+          elsif @_was_loaded || !loaded?
+            target << record
+          end
+
+          callback(:after_add, record) unless skip_callbacks
+
+          record
+        ensure
+          @_was_loaded = nil
+        end
+
         def callback(method, record)
           callbacks_for(method).each do |callback|
             callback.call(method, owner, record)

activerecord/lib/active_record/associations/has_many_association.rb

@@ -31,12 +31,7 @@ def handle_dependency
 
       def insert_record(record, validate = true, raise = false)
         set_owner_attributes(record)
-
-        if raise
-          record.save!(validate: validate)
-        else
-          record.save(validate: validate)
-        end
+        super
       end
 
       def empty?

activerecord/lib/active_record/associations/has_many_through_association.rb

@@ -39,11 +39,7 @@ def insert_record(record, validate = true, raise = false)
         ensure_not_nested
 
         if record.new_record? || record.has_changes_to_save?
-          if raise
-            record.save!(validate: validate)
-          else
-            return unless record.save(validate: validate)
-          end
+          return unless super
         end
 
         save_through_record(record)

activerecord/lib/active_record/persistence.rb

@@ -100,6 +100,10 @@ def persisted?
       !(@new_record || @destroyed)
     end
 
+    ##
+    # :call-seq:
+    #   save(*args)
+    #
     # Saves the model.
     #
     # If the model is new, a record gets created in the database, otherwise
@@ -121,12 +125,16 @@ def persisted?
     #
     # Attributes marked as readonly are silently ignored if the record is
     # being updated.
-    def save(*args)
-      create_or_update(*args)
+    def save(*args, &block)
+      create_or_update(*args, &block)
     rescue ActiveRecord::RecordInvalid
       false
     end
 
+    ##
+    # :call-seq:
+    #   save!(*args)
+    #
     # Saves the model.
     #
     # If the model is new, a record gets created in the database, otherwise
@@ -150,8 +158,8 @@ def save(*args)
     # being updated.
     #
     # Unless an error is raised, returns true.
-    def save!(*args)
-      create_or_update(*args) || raise(RecordNotSaved.new("Failed to save the record", self))
+    def save!(*args, &block)
+      create_or_update(*args, &block) || raise(RecordNotSaved.new("Failed to save the record", self))
     end
 
     # Deletes the record in the database and freezes this instance to
@@ -550,9 +558,9 @@ def relation_for_destroy
       self.class.unscoped.where(self.class.primary_key => id)
     end
 
-    def create_or_update(*args)
+    def create_or_update(*args, &block)
       _raise_readonly_record_error if readonly?
-      result = new_record? ? _create_record : _update_record(*args)
+      result = new_record? ? _create_record(&block) : _update_record(*args, &block)
       result != false
     end
 
@@ -567,6 +575,9 @@ def _update_record(attribute_names = self.attribute_names)
         rows_affected = self.class.unscoped._update_record attributes_values, id, id_in_database
         @_trigger_update_callback = rows_affected > 0
       end
+
+      yield(self) if block_given?
+
       rows_affected
     end
 
@@ -579,6 +590,9 @@ def _create_record(attribute_names = self.attribute_names)
       self.id ||= new_id if self.class.primary_key
 
       @new_record = false
+
+      yield(self) if block_given?
+
       id
     end
 

activerecord/test/cases/associations/has_many_associations_test.rb

@@ -2447,15 +2447,29 @@ def self.name
     assert_equal [first_bulb, second_bulb], car.bulbs
   end
 
-  test "double insertion of new object to association when same association used in the after create callback of a new object" do
+  test "prevent double insertion of new object when the parent association loaded in the after save callback" do
     reset_callbacks(:save, Bulb) do
       Bulb.after_save { |record| record.car.bulbs.load }
+
       car = Car.create!
       car.bulbs << Bulb.new
+
       assert_equal 1, car.bulbs.size
     end
   end
 
+  test "prevent double firing the before save callback of new object when the parent association saved in the callback" do
+    reset_callbacks(:save, Bulb) do
+      count = 0
+      Bulb.before_save { |record| record.car.save && count += 1 }
+
+      car = Car.create!
+      car.bulbs.create!
+
+      assert_equal 1, count
+    end
+  end
+
   class AuthorWithErrorDestroyingAssociation < ActiveRecord::Base
     self.table_name = "authors"
     has_many :posts_with_error_destroying,
@@ -2496,7 +2510,7 @@ def test_ids_reader_memoization
 
   def test_loading_association_in_validate_callback_doesnt_affect_persistence
     reset_callbacks(:validation, Bulb) do
-      Bulb.after_validation { |m| m.car.bulbs.load }
+      Bulb.after_validation { |record| record.car.bulbs.load }
 
       car = Car.create!(name: "Car")
       bulb = car.bulbs.create!