Prevent double firing the before save callback of new object when the parent association saved in the callback

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

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

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

Fixes #28599.

4 files changed, 54 insertions, 52 deletions

diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb
index 77282e6463..62c944fce3 100644
--- a/activerecord/lib/active_record/associations/collection_association.rb
+++ b/activerecord/lib/active_record/associations/collection_association.rb
@@ -280,35 +280,6 @@ module ActiveRecord
         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 @@ module ActiveRecord
             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 @@ module ActiveRecord
           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)
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index b413eb3f9c..10ca0e47ff 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -31,12 +31,7 @@ module ActiveRecord
 
       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?
diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb
index c4a7fe4432..53ffb3b68d 100644
--- a/activerecord/lib/active_record/associations/has_many_through_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -39,11 +39,7 @@ module ActiveRecord
         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)
diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb
index 7ceb7d1a55..f652c7c3a1 100644
--- a/activerecord/lib/active_record/persistence.rb
+++ b/activerecord/lib/active_record/persistence.rb
@@ -100,6 +100,10 @@ module ActiveRecord
       !(@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 @@ module ActiveRecord
     #
     # 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 @@ module ActiveRecord
     # 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 @@ module ActiveRecord
       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 @@ module ActiveRecord
         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 @@ module ActiveRecord
       self.id ||= new_id if self.class.primary_key
 
       @new_record = false
+
+      yield(self) if block_given?
+
       id
     end