5-0-stable: Prevent double firing the before save callback of new object when the parent association saved in the callback

activerecord/lib/active_record/associations/collection_association.rb

@@ -421,34 +421,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
-
-          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
-        set_inverse_instance(record)
-
-        record
-      end
-
       def scope
         scope = super
         scope.none! if null_scope?
@@ -522,15 +494,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
@@ -584,19 +560,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

@@ -40,13 +40,7 @@ def handle_dependency
 
       def insert_record(record, validate = true, raise = false)
         set_owner_attributes(record)
-        set_inverse_instance(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.changed?
-          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
@@ -148,8 +156,8 @@ def save(*args)
     #
     # Attributes marked as readonly are silently ignored if the record is
     # being updated.
-    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
@@ -535,9 +543,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 ReadOnlyRecord, "#{self.class} is marked as readonly" if readonly?
-      result = new_record? ? _create_record : _update_record(*args)
+      result = new_record? ? _create_record(&block) : _update_record(*args, &block)
       result != false
     end
 
@@ -546,10 +554,14 @@ def create_or_update(*args)
     def _update_record(attribute_names = self.attribute_names)
       attributes_values = arel_attributes_with_values_for_update(attribute_names)
       if attributes_values.empty?
-        0
+        rows_affected = 0
       else
-        self.class.unscoped._update_record attributes_values, id, id_was
+        rows_affected = self.class.unscoped._update_record attributes_values, id, id_was
       end
+
+      yield(self) if block_given?
+
+      rows_affected
     end
 
     # Creates a record with values matching those of the instance attributes
@@ -561,6 +573,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

@@ -586,20 +586,6 @@ def test_update_all_on_association_accessed_before_save
     assert_not_equal clients_proxy_id, firm.clients.object_id
   end
 
-  def test_update_all_on_association_accessed_before_save_with_explicit_foreign_key
-    # We can use the same cached proxy object because the id is available for the scope
-    firm = Firm.new(name: 'Firm', id: 100)
-    clients_proxy_id = firm.clients.object_id
-    firm.clients << Client.first
-    firm.save!
-    assert_equal firm.clients.count, firm.clients.update_all(description: 'Great!')
-    assert_equal clients_proxy_id, firm.clients.object_id
-    firm = Firm.new(name: "Firm")
-    firm.clients << Client.first
-    firm.save!
-    assert_equal firm.clients.count, firm.clients.update_all(description: "Great!")
-  end
-
   def test_update_all_on_association_accessed_before_save_with_explicit_foreign_key
     firm = Firm.new(name: "Firm", id: 100)
     firm.clients << Client.first
@@ -2401,15 +2387,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
+
   def test_association_force_reload_with_only_true_is_deprecated
     company = Company.find(1)
 
@@ -2456,7 +2456,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!