Merge pull request #27442 from kamipo/fix_27434

Add a record to target before any callbacks loads the record
rails · Dec 27, 2016 · fb6ac0c · fb6ac0c
2 parents 4d5ced3 + 9eee782
commit fb6ac0c
Show file tree

Hide file tree

Showing 3 changed files with 17 additions and 14 deletions.
diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb
@@ -299,12 +299,23 @@ def add_to_target(record, skip_callbacks = false, &block)
       def replace_on_target(record, index, skip_callbacks)
         callback(:before_add, record) unless skip_callbacks
 
-        yield(record) if block_given?
+        begin
+          if index
+            record_was = target[index]
+            target[index] = record
+          else
+            target << record
+          end
 
-        if index
-          @target[index] = record
-        else
-          append_record(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
@@ -502,10 +513,6 @@ def find_by_scan(*args)
             load_target.select { |r| ids.include?(r.id.to_s) }
           end
         end
-
-        def append_record(record)
-          @target << record unless @target.include?(record)
-        end
     end
   end
 end
diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -206,10 +206,6 @@ def find_target
         def invertible_for?(record)
           false
         end
-
-        def append_record(record)
-          @target << record
-        end
     end
   end
 end
diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb
@@ -2475,7 +2475,7 @@ def self.name
 
   test "double insertion of new object to association when same association used in the after create callback of a new object" do
     reset_callbacks(:save, Bulb) do
-      Bulb.after_save { |record| record.car.bulbs.to_a }
+      Bulb.after_save { |record| record.car.bulbs.load }
       car = Car.create!
       car.bulbs << Bulb.new
       assert_equal 1, car.bulbs.size