Make record.association.destroy(*records) on habtm and hm:t only delete records in the join table. This is to make the destroy method more consistent across the different types of associations. For more details see the CHANGELOG entry.

3 files changed, 199 insertions, 84 deletions

diff --git a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb
index 126b767d06..5dd2c9861e 100644
--- a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb
+++ b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb
@@ -372,27 +372,34 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase
 
   def test_destroying
     david = Developer.find(1)
-    active_record = Project.find(1)
+    project = Project.find(1)
     david.projects.reload
     assert_equal 2, david.projects.size
-    assert_equal 3, active_record.developers.size
+    assert_equal 3, project.developers.size
 
-    assert_difference "Project.count", -1 do
-      david.projects.destroy(active_record)
+    assert_no_difference "Project.count" do
+      david.projects.destroy(project)
     end
 
+    join_records = Developer.connection.select_all("SELECT * FROM developers_projects WHERE developer_id = #{david.id} AND project_id = #{project.id}")
+    assert join_records.empty?
+
     assert_equal 1, david.reload.projects.size
     assert_equal 1, david.projects(true).size
   end
 
-  def test_destroying_array
+  def test_destroying_many
     david = Developer.find(1)
     david.projects.reload
+    projects = Project.all
 
-    assert_difference "Project.count", -Project.count do
-      david.projects.destroy(Project.find(:all))
+    assert_no_difference "Project.count" do
+      david.projects.destroy(*projects)
     end
 
+    join_records = Developer.connection.select_all("SELECT * FROM developers_projects WHERE developer_id = #{david.id}")
+    assert join_records.empty?
+
     assert_equal 0, david.reload.projects.size
     assert_equal 0, david.projects(true).size
   end
@@ -401,7 +408,14 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase
     david = Developer.find(1)
     david.projects.reload
     assert !david.projects.empty?
-    david.projects.destroy_all
+
+    assert_no_difference "Project.count" do
+      david.projects.destroy_all
+    end
+
+    join_records = Developer.connection.select_all("SELECT * FROM developers_projects WHERE developer_id = #{david.id}")
+    assert join_records.empty?
+
     assert david.projects.empty?
     assert david.projects(true).empty?
   end
diff --git a/activerecord/test/cases/associations/has_many_through_associations_test.rb b/activerecord/test/cases/associations/has_many_through_associations_test.rb
index 96f4597726..6830478107 100644
--- a/activerecord/test/cases/associations/has_many_through_associations_test.rb
+++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb
@@ -128,8 +128,10 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
   end
 
   def test_destroy_association
-    assert_difference ["Person.count", "Reader.count"], -1 do
-      posts(:welcome).people.destroy(people(:michael))
+    assert_no_difference "Person.count" do
+      assert_difference "Reader.count", -1 do
+        posts(:welcome).people.destroy(people(:michael))
+      end
     end
 
     assert posts(:welcome).reload.people.empty?
@@ -137,8 +139,10 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
   end
 
   def test_destroy_all
-    assert_difference ["Person.count", "Reader.count"], -1 do
-      posts(:welcome).people.destroy_all
+    assert_no_difference "Person.count" do
+      assert_difference "Reader.count", -1 do
+        posts(:welcome).people.destroy_all
+      end
     end
 
     assert posts(:welcome).reload.people.empty?
diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb
index 11c0c5b0ef..c2f2609c9e 100644
--- a/activerecord/test/cases/autosave_association_test.rb
+++ b/activerecord/test/cases/autosave_association_test.rb
@@ -689,110 +689,207 @@ class TestDestroyAsPartOfAutosaveAssociation < ActiveRecord::TestCase
     assert_equal 'NewName', @parrot.reload.name
   end
 
-  # has_many & has_and_belongs_to
-  %w{ parrots birds }.each do |association_name|
-    define_method("test_should_destroy_#{association_name}_as_part_of_the_save_transaction_if_they_were_marked_for_destroyal") do
-      2.times { |i| @pirate.send(association_name).create!(:name => "#{association_name}_#{i}") }
+  def test_should_destroy_has_many_as_part_of_the_save_transaction_if_they_were_marked_for_destruction
+    2.times { |i| @pirate.birds.create!(:name => "birds_#{i}") }
 
-      assert !@pirate.send(association_name).any? { |child| child.marked_for_destruction? }
+    assert !@pirate.birds.any? { |child| child.marked_for_destruction? }
 
-      @pirate.send(association_name).each { |child| child.mark_for_destruction }
-      klass = @pirate.send(association_name).first.class
-      ids = @pirate.send(association_name).map(&:id)
+    @pirate.birds.each { |child| child.mark_for_destruction }
+    klass = @pirate.birds.first.class
+    ids = @pirate.birds.map(&:id)
 
-      assert @pirate.send(association_name).all? { |child| child.marked_for_destruction? }
-      ids.each { |id| assert klass.find_by_id(id) }
+    assert @pirate.birds.all? { |child| child.marked_for_destruction? }
+    ids.each { |id| assert klass.find_by_id(id) }
 
-      @pirate.save
-      assert @pirate.reload.send(association_name).empty?
-      ids.each { |id| assert_nil klass.find_by_id(id) }
+    @pirate.save
+    assert @pirate.reload.birds.empty?
+    ids.each { |id| assert_nil klass.find_by_id(id) }
+  end
+
+  def test_should_skip_validation_on_has_many_if_marked_for_destruction
+    2.times { |i| @pirate.birds.create!(:name => "birds_#{i}") }
+
+    @pirate.birds.each { |bird| bird.name = '' }
+    assert !@pirate.valid?
+
+    @pirate.birds.each do |bird|
+      bird.mark_for_destruction
+      bird.expects(:valid?).never
     end
+    assert_difference("Bird.count", -2) { @pirate.save! }
+  end
+
+  def test_should_skip_validation_on_has_many_if_destroyed
+    @pirate.birds.create!(:name => "birds_1")
 
-    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)
+    @pirate.birds.each { |bird| bird.name = '' }
+    assert !@pirate.valid?
+
+    @pirate.birds.each { |bird| bird.destroy }
+    assert @pirate.valid?
+  end
 
-      children.each { |child| child.name = '' }
-      assert !@pirate.valid?
+  def test_a_child_marked_for_destruction_should_not_be_destroyed_twice_while_saving_has_many
+    @pirate.birds.create!(:name => "birds_1")
+
+    @pirate.birds.each { |bird| bird.mark_for_destruction }
+    assert @pirate.save
+
+    @pirate.birds.each { |bird| bird.expects(:destroy).never }
+    assert @pirate.save
+  end
 
-      children.each do |child|
-        child.mark_for_destruction
-        child.expects(:valid?).never
+  def test_should_rollback_destructions_if_an_exception_occurred_while_saving_has_many
+    2.times { |i| @pirate.birds.create!(:name => "birds_#{i}") }
+    before = @pirate.birds.map { |c| c.mark_for_destruction ; c }
+
+    # Stub the destroy method of the the second child to raise an exception
+    class << before.last
+      def destroy(*args)
+        super
+        raise 'Oh noes!'
       end
-      assert_difference("#{association_name.classify}.count", -2) { @pirate.save! }
     end
 
-    define_method("test_should_skip_validation_on_the_#{association_name}_association_if_destroyed") do
-      @pirate.send(association_name).create!(:name => "#{association_name}_1")
-      children = @pirate.send(association_name)
+    assert_raise(RuntimeError) { assert !@pirate.save }
+    assert_equal before, @pirate.reload.birds
+  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
+      association_name_with_callbacks = "birds_with_#{callback_type}_callbacks"
+
+      pirate = Pirate.new(:catchphrase => "Arr")
+      pirate.send(association_name_with_callbacks).build(:name => "Crowe the One-Eyed")
 
-      children.each { |child| child.name = '' }
-      assert !@pirate.valid?
+      expected = [
+        "before_adding_#{callback_type}_bird_<new>",
+        "after_adding_#{callback_type}_bird_<new>"
+      ]
 
-      children.each { |child| child.destroy }
-      assert @pirate.valid?
+      assert_equal expected, pirate.ship_log
     end
 
-    define_method("test_a_child_marked_for_destruction_should_not_be_destroyed_twice_while_saving_#{association_name}") do
-      @pirate.send(association_name).create!(:name => "#{association_name}_1")
-      children = @pirate.send(association_name)
+    define_method("test_should_run_remove_callback_#{callback_type}s_for_has_many") do
+      association_name_with_callbacks = "birds_with_#{callback_type}_callbacks"
 
-      children.each { |child| child.mark_for_destruction }
-      assert @pirate.save
-      children.each { |child| child.expects(:destroy).never }
-      assert @pirate.save
+      @pirate.send(association_name_with_callbacks).create!(:name => "Crowe the One-Eyed")
+      @pirate.send(association_name_with_callbacks).each { |c| c.mark_for_destruction }
+      child_id = @pirate.send(association_name_with_callbacks).first.id
+
+      @pirate.ship_log.clear
+      @pirate.save
+
+      expected = [
+        "before_removing_#{callback_type}_bird_#{child_id}",
+        "after_removing_#{callback_type}_bird_#{child_id}"
+      ]
+
+      assert_equal expected, @pirate.ship_log
     end
+  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.mark_for_destruction ; c }
+  def test_should_destroy_habtm_as_part_of_the_save_transaction_if_they_were_marked_for_destruction
+    2.times { |i| @pirate.parrots.create!(:name => "parrots_#{i}") }
 
-      # Stub the destroy method of the the second child to raise an exception
-      class << before.last
-        def destroy(*args)
-          super
-          raise 'Oh noes!'
-        end
-      end
+    assert !@pirate.parrots.any? { |parrot| parrot.marked_for_destruction? }
+    @pirate.parrots.each { |parrot| parrot.mark_for_destruction }
 
-      assert_raise(RuntimeError) { assert !@pirate.save }
-      assert_equal before, @pirate.reload.send(association_name)
+    assert_no_difference "Parrot.count" do
+      @pirate.save
     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_#{association_name}") do
-        association_name_with_callbacks = "#{association_name}_with_#{callback_type}_callbacks"
+    assert @pirate.reload.parrots.empty?
 
-        pirate = Pirate.new(:catchphrase => "Arr")
-        pirate.send(association_name_with_callbacks).build(:name => "Crowe the One-Eyed")
+    join_records = Pirate.connection.select_all("SELECT * FROM parrots_pirates WHERE pirate_id = #{@pirate.id}")
+    assert join_records.empty?
+  end
 
-        expected = [
-          "before_adding_#{callback_type}_#{association_name.singularize}_<new>",
-          "after_adding_#{callback_type}_#{association_name.singularize}_<new>"
-        ]
+  def test_should_skip_validation_on_habtm_if_marked_for_destruction
+    2.times { |i| @pirate.parrots.create!(:name => "parrots_#{i}") }
 
-        assert_equal expected, pirate.ship_log
-      end
+    @pirate.parrots.each { |parrot| parrot.name = '' }
+    assert !@pirate.valid?
+
+    @pirate.parrots.each do |parrot|
+      parrot.mark_for_destruction
+      parrot.expects(:valid?).never
+    end
 
-      define_method("test_should_run_remove_callback_#{callback_type}s_for_#{association_name}") do
-        association_name_with_callbacks = "#{association_name}_with_#{callback_type}_callbacks"
+    @pirate.save!
+    assert @pirate.reload.parrots.empty?
+  end
+
+  def test_should_skip_validation_on_habtm_if_destroyed
+    @pirate.parrots.create!(:name => "parrots_1")
 
-        @pirate.send(association_name_with_callbacks).create!(:name => "Crowe the One-Eyed")
-        @pirate.send(association_name_with_callbacks).each { |c| c.mark_for_destruction }
-        child_id = @pirate.send(association_name_with_callbacks).first.id
+    @pirate.parrots.each { |parrot| parrot.name = '' }
+    assert !@pirate.valid?
 
-        @pirate.ship_log.clear
-        @pirate.save
+    @pirate.parrots.each { |parrot| parrot.destroy }
+    assert @pirate.valid?
+  end
+
+  def test_a_child_marked_for_destruction_should_not_be_destroyed_twice_while_saving_habtm
+    @pirate.parrots.create!(:name => "parrots_1")
+
+    @pirate.parrots.each { |parrot| parrot.mark_for_destruction }
+    assert @pirate.save
+
+    assert_queries(1) do
+      assert @pirate.save
+    end
+  end
 
-        expected = [
-          "before_removing_#{callback_type}_#{association_name.singularize}_#{child_id}",
-          "after_removing_#{callback_type}_#{association_name.singularize}_#{child_id}"
-        ]
+  def test_should_rollback_destructions_if_an_exception_occurred_while_saving_habtm
+    2.times { |i| @pirate.parrots.create!(:name => "parrots_#{i}") }
+    before = @pirate.parrots.map { |c| c.mark_for_destruction ; c }
 
-        assert_equal expected, @pirate.ship_log
+    class << @pirate.parrots
+      def destroy(*args)
+        super
+        raise 'Oh noes!'
       end
     end
+
+    assert_raise(RuntimeError) { assert !@pirate.save }
+    assert_equal before, @pirate.reload.parrots
+  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_habtm") do
+      association_name_with_callbacks = "parrots_with_#{callback_type}_callbacks"
+
+      pirate = Pirate.new(:catchphrase => "Arr")
+      pirate.send(association_name_with_callbacks).build(:name => "Crowe the One-Eyed")
+
+      expected = [
+        "before_adding_#{callback_type}_parrot_<new>",
+        "after_adding_#{callback_type}_parrot_<new>"
+      ]
+
+      assert_equal expected, pirate.ship_log
+    end
+
+    define_method("test_should_run_remove_callback_#{callback_type}s_for_habtm") do
+      association_name_with_callbacks = "parrots_with_#{callback_type}_callbacks"
+
+      @pirate.send(association_name_with_callbacks).create!(:name => "Crowe the One-Eyed")
+      @pirate.send(association_name_with_callbacks).each { |c| c.mark_for_destruction }
+      child_id = @pirate.send(association_name_with_callbacks).first.id
+
+      @pirate.ship_log.clear
+      @pirate.save
+
+      expected = [
+        "before_removing_#{callback_type}_parrot_#{child_id}",
+        "after_removing_#{callback_type}_parrot_#{child_id}"
+      ]
+
+      assert_equal expected, @pirate.ship_log
+    end
   end
 end