rails#7347: Destroy marked child records before new child records in…

…serted
jetthoughts · Mar 18, 2013 · 70ae8d7 · 70ae8d7
1 parent c4a7c31
commit 70ae8d7
Show file tree

Hide file tree

Showing 3 changed files with 25 additions and 10 deletions.
diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb
@@ -320,15 +320,20 @@ def save_collection_association(reflection)
         autosave = reflection.options[:autosave]
 
         if records = associated_records_to_validate_or_save(association, @new_record_before_save, autosave)
-          records_to_destroy = []
-          records.each do |record|
-            next if record.destroyed?
+          not_destroyed_records = records.delete_if(&:destroyed?)
 
+          records_to_destroy, records_to_save = not_destroyed_records.partition do |record|
+            autosave && record.marked_for_destruction?
+          end
+
+          records_to_destroy.each do |record|
+            association.destroy(record)
+          end
+
+          records_to_save.each do |record|
             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
@@ -340,10 +345,6 @@ def save_collection_association(reflection)
 
             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
@@ -754,6 +754,18 @@ def destroy(*args)
     assert_equal before, @pirate.reload.birds
   end
 
+  def test_should_destroy_marked_childs_before_new_child_savings
+    @pirate.birds.create! name: 'Parrot', color: 'Colorful'
+    @pirate.birds.create! name: 'Sparrow', color: 'Asphalt'
+
+    @pirate.birds.each(&:mark_for_destruction)
+
+    @pirate.birds.build name: 'Parrot', color: 'Colorful'
+    @pirate.birds.build name: 'Sparrow', color: 'Asphalt'
+
+    @pirate.save!
+  end
+
   def test_when_new_record_a_child_marked_for_destruction_should_not_affect_other_records_from_saving
     @pirate = @ship.build_pirate(:catchphrase => "Arr' now I shall keep me eye on you matey!") # new record
 

diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb
@@ -89,6 +89,8 @@ def create_table(*args, &block)
     t.integer :pirate_id
   end
 
+  add_index :birds, [:name, :color], :unique => true, :name => 'unique_birds_index'
+
   create_table :books, :force => true do |t|
     t.integer :author_id
     t.column :name, :string