From 483c301e0a132a6415355be2296fb932d07bd1c5 Mon Sep 17 00:00:00 2001
From: Johnny Holton <johnny@holton.co>
Date: Wed, 10 Apr 2013 23:29:19 -0400
Subject: destroys association records before saving/inserting new association
 records

fixes bug introduced by  #3329
These are the conditions necessary to reproduce the bug:
- For an association, autosave => true.
- An association record is being destroyed
- A new association record is being created.
- There is a unique index one of the association's fields.
- The record being created has the same value as the record being
destroyed on the indexed field.

Before, the deletion of records was postponed until after all
insertions/saves.  Therefore the new record with the identical value in
the indexed field caused a non-unique value error to be thrown at the database
level.

With this fix, the deletions happen first, before the insertions/saves.
Therefore the record with the duplicate value is gone from the database
before the new record is created, thereby avoiding the non-uniuqe value
error.
---
 activerecord/CHANGELOG.md                          |  7 +++++++
 .../lib/active_record/autosave_association.rb      | 17 ++++++++---------
 .../test/cases/autosave_association_test.rb        | 22 +++++++++++++++++++---
 3 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index c1b849ce68..8175033c6f 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,10 @@
+*   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.
+
+    *Johnny Holton*
+
 *   Handle aliased attributes in ActiveRecord::Relation.
 
     When using symbol keys, ActiveRecord will now translate aliased attribute names to the actual column name used in the database:
diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb
index b0bd78ad46..87d4daa6d9 100644
--- a/activerecord/lib/active_record/autosave_association.rb
+++ b/activerecord/lib/active_record/autosave_association.rb
@@ -335,15 +335,18 @@ module ActiveRecord
         autosave = reflection.options[:autosave]
 
         if records = associated_records_to_validate_or_save(association, @new_record_before_save, autosave)
-          records_to_destroy = []
+
+          if autosave
+            records_to_destroy = records.select(&:marked_for_destruction?)
+            records_to_destroy.each { |record| association.destroy(record) }
+            records -= records_to_destroy
+          end
+
           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 != false && (@new_record_before_save || record.new_record?)
               if autosave
                 saved = association.insert_record(record, false)
               else
@@ -355,10 +358,6 @@ module ActiveRecord
 
             raise ActiveRecord::Rollback unless saved
           end
-
-          records_to_destroy.each do |record|
-            association.destroy(record)
-          end
         end
 
         # reconstruct the scope now that we know the owner's id
diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb
index 536ff4882c..580aa96ecd 100644
--- a/activerecord/test/cases/autosave_association_test.rb
+++ b/activerecord/test/cases/autosave_association_test.rb
@@ -566,7 +566,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
     super
@@ -764,6 +764,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
@@ -846,8 +860,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
 
-- 
cgit v1.2.3