Don't validate records of an :autosave association if they're marked for...

Don't validate records of an :autosave association if they're marked for destruction. [#2064 status:resolved]
Signed-off-by: NDavid Heinemeier Hansson <david@loudthinking.com>

activerecord/lib/active_record/autosave_association.rb

@@ -243,13 +243,16 @@ def validate_collection_association(reflection)
     end
 
     # Returns whether or not the association is valid and applies any errors to
-    # the parent, <tt>self</tt>, if it wasn't.
+    # the parent, <tt>self</tt>, if it wasn't. Skips any <tt>:autosave</tt>
+    # enabled records if they're marked_for_destruction?.
     def association_valid?(reflection, association)
       unless valid = association.valid?
         if reflection.options[:autosave]
-          association.errors.each do |attribute, message|
-            attribute = "#{reflection.name}_#{attribute}"
-            errors.add(attribute, message) unless errors.on(attribute)
+          unless association.marked_for_destruction?
+            association.errors.each do |attribute, message|
+              attribute = "#{reflection.name}_#{attribute}"
+              errors.add(attribute, message) unless errors.on(attribute)
+            end
           end
         else
           errors.add(reflection.name)

activerecord/test/cases/autosave_association_test.rb

@@ -13,9 +13,6 @@
 require 'models/ship_part'
 require 'models/treasure'
 
-# TODO:
-# - add test case for new parent and children with invalid data and saving with validate = false
-
 class TestAutosaveAssociationsInGeneral < ActiveRecord::TestCase
   def test_autosave_should_be_a_valid_option_for_has_one
     assert base.valid_keys_for_has_one_association.include?(:autosave)
@@ -449,6 +446,14 @@ def test_should_destroy_a_child_association_as_part_of_the_save_transaction_if_i
     assert_nil Ship.find_by_id(id)
   end
 
+  def test_should_skip_validation_on_a_child_association_if_marked_for_destruction
+    @pirate.ship.name = ''
+    assert !@pirate.valid?
+
+    @pirate.ship.mark_for_destruction
+    assert_difference('Ship.count', -1) { @pirate.save! }
+  end
+
   def test_should_rollback_destructions_if_an_exception_occurred_while_saving_a_child
     # Stub the save method of the @pirate.ship instance to destroy and then raise an exception
     class << @pirate.ship
@@ -478,6 +483,14 @@ def test_should_destroy_a_parent_association_as_part_of_the_save_transaction_if_
     assert_nil Pirate.find_by_id(id)
   end
 
+  def test_should_skip_validation_on_a_parent_association_if_marked_for_destruction
+    @ship.pirate.catchphrase = ''
+    assert !@ship.valid?
+
+    @ship.pirate.mark_for_destruction
+    assert_difference('Pirate.count', -1) { @ship.save! }
+  end
+
   def test_should_rollback_destructions_if_an_exception_occurred_while_saving_a_parent
     # Stub the save method of the @ship.pirate instance to destroy and then raise an exception
     class << @ship.pirate
@@ -511,6 +524,17 @@ def save(*args)
       ids.each { |id| assert_nil klass.find_by_id(id) }
     end
 
+    define_method("test_should_skip_validation_on_the_#{association_name}_association_if_marked_for_destruction") do
+      2.times { |i| @pirate.send(association_name).create!(:name => "#{association_name}_#{i}") }
+      children = @pirate.send(association_name)
+
+      children.each { |child| child.name = '' }
+      assert !@pirate.valid?
+
+      children.each { |child| child.mark_for_destruction }
+      assert_difference("#{association_name.classify}.count", -2) { @pirate.save! }
+    end
+
     define_method("test_should_rollback_destructions_if_an_exception_occurred_while_saving_#{association_name}") do
       2.times { |i| @pirate.send(association_name).create!(:name => "#{association_name}_#{i}") }
       before = @pirate.send(association_name).map { |c| c }