Merge pull request #10681 from jholton/3-2-stable-fix_association_auto_save

autosave_association issue that occurs when table has unique index (3.2.x backport)

3 files changed, 48 insertions, 31 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index f8c75c0fba..ebd1a3424a 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,5 +1,14 @@
 ## unreleased ##
 
+*   fixes bug introduced by #3329.  Now, when autosaving associations,
+    deletions happen before inserts and saves.  This prevents a 'duplicate
+    unique value' database error that would occur if a record being created had
+    the same value on a unique indexed field as that of a record being destroyed.
+
+    Backport of #10417
+
+    *Johnny Holton*
+
 *   Fix that under some conditions, Active Record could produce invalid SQL of the sort:
     "SELECT DISTINCT DISTINCT".
 
diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb
index e1499fc3b0..7bc103043e 100644
--- a/activerecord/lib/active_record/autosave_association.rb
+++ b/activerecord/lib/active_record/autosave_association.rb
@@ -331,41 +331,33 @@ module ActiveRecord
         autosave = reflection.options[:autosave]
 
         if records = associated_records_to_validate_or_save(association, @new_record_before_save, autosave)
-          begin
-            records_to_destroy = []
-
-            records.each do |record|
-              next if record.destroyed?
-
-              saved = true
-
-              if autosave && record.marked_for_destruction?
-                records_to_destroy << record
-              elsif autosave != false && (@new_record_before_save || record.new_record?)
-                if autosave
-                  saved = association.insert_record(record, false)
-                else
-                  association.insert_record(record) unless reflection.nested?
-                end
-              elsif autosave
-                saved = record.save(:validate => false)
-              end
 
-              raise ActiveRecord::Rollback unless saved
-            end
+          if autosave
+            records_to_destroy = records.select(&:marked_for_destruction?)
+            records_to_destroy.each { |record| association.proxy.destroy(record) }
+            records -= records_to_destroy
+          end
+
+          records.each do |record|
 
-            records_to_destroy.each do |record|
-              association.proxy.destroy(record)
+            saved = true
+
+            if autosave != false && (@new_record_before_save || record.new_record?)
+              if autosave
+                saved = association.insert_record(record, false)
+              else
+                association.insert_record(record) unless reflection.nested?
+              end
+            elsif autosave
+              saved = record.save(:validate => false)
             end
-          rescue
-            records.each {|x| IdentityMap.remove(x) } if IdentityMap.enabled?
-            raise
-          end
 
+            raise ActiveRecord::Rollback unless saved
+          end
         end
 
         # reconstruct the scope now that we know the owner's id
-        association.send(:reset_scope) if association.respond_to?(:reset_scope)
+        association.reset_scope if association.respond_to?(:reset_scope)
       end
     end
 
diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb
index e6b881389b..bb07cf4185 100644
--- a/activerecord/test/cases/autosave_association_test.rb
+++ b/activerecord/test/cases/autosave_association_test.rb
@@ -583,7 +583,7 @@ class TestDefaultAutosaveAssociationOnNewRecord < ActiveRecord::TestCase
 end
 
 class TestDestroyAsPartOfAutosaveAssociation < ActiveRecord::TestCase
-  self.use_transactional_fixtures = false unless supports_savepoints?
+  self.use_transactional_fixtures = false
 
   def setup
     @pirate = Pirate.create(:catchphrase => "Don' botharrr talkin' like one, savvy?")
@@ -780,6 +780,20 @@ class TestDestroyAsPartOfAutosaveAssociation < ActiveRecord::TestCase
     assert_equal 2, @pirate.birds.reload.length
   end
 
+  def test_should_save_new_record_that_has_same_value_as_existing_record_marked_for_destruction_on_field_that_has_unique_index
+    Bird.connection.add_index :birds, :name, unique: true
+
+    3.times { |i| @pirate.birds.create(name: "unique_birds_#{i}") }
+
+    @pirate.birds[0].mark_for_destruction
+    @pirate.birds.build(name: @pirate.birds[0].name)
+    @pirate.save!
+
+    assert_equal 3, @pirate.birds.reload.length
+  ensure
+    Bird.connection.remove_index :birds, column: :name
+  end
+
   # Add and remove callbacks tests for association collections.
   %w{ method proc }.each do |callback_type|
     define_method("test_should_run_add_callback_#{callback_type}s_for_has_many") do
@@ -862,8 +876,10 @@ class TestDestroyAsPartOfAutosaveAssociation < ActiveRecord::TestCase
     @pirate.parrots.each { |parrot| parrot.mark_for_destruction }
     assert @pirate.save
 
-    assert_queries(0) do
-      assert @pirate.save
+    Pirate.transaction do
+      assert_queries(0) do
+        assert @pirate.save
+      end
     end
   end