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.

8 files changed, 248 insertions, 107 deletions

diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG
index d1124801df..aca81b0077 100644
--- a/activerecord/CHANGELOG
+++ b/activerecord/CHANGELOG
@@ -1,5 +1,24 @@
 *Rails 3.1.0 (unreleased)*
 
+* Changed the behaviour of association.destroy for has_and_belongs_to_many and has_many :through.
+  From now on, 'destroy' or 'delete' on an association will be taken to mean 'get rid of the link',
+  not (necessarily) 'get rid of the associated records'.
+
+  Previously, has_and_belongs_to_many.destroy(*records) would destroy the records themselves. It
+  would not delete any records in the join table. Now, it deletes the records in the join table.
+
+  Previously, has_many_through.destroy(*records) would destroy the records themselves, and the
+  records in the join table. [Note: This has not always been the case; previous version of Rails
+  only deleted the records themselves.] Now, it destroys only the records in the join table.
+
+  Note that this change is backwards-incompatible to an extent, but there is unfortunately no
+  way to 'deprecate' it before changing it. The change is being made in order to have
+  consistency as to the meaning of 'destroy' or 'delete' across the different types of associations.
+
+  If you wish to destroy the records themselves, you can do records.association.each(&:destroy)
+
+  [Jon Leighton]
+
 * Add :bulk => true option to change_table to make all the schema changes defined in change_table block using a single ALTER statement. [Pratik Naik]
 
   Example:
diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb
index 2811f53424..9bd59132f5 100644
--- a/activerecord/lib/active_record/associations/association_collection.rb
+++ b/activerecord/lib/active_record/associations/association_collection.rb
@@ -192,7 +192,7 @@ module ActiveRecord
       def destroy(*records)
         records = find(records) if records.any? {|record| record.kind_of?(Fixnum) || record.kind_of?(String)}
         remove_records(records) do |_records, old_records|
-          old_records.each { |record| record.destroy }
+          delete_records(old_records, :destroy) if old_records.any?
         end
 
         load_target
@@ -462,6 +462,12 @@ module ActiveRecord
           end
         end
 
+        # Delete the given records from the association, using one of the methods :destroy,
+        # :delete_all or :nullify. The default method used is given by the :dependent option.
+        def delete_records(records, method = @reflection.options[:dependent])
+          raise NotImplementedError
+        end
+
         def callback(method, record)
           callbacks_for(method).each do |callback|
             case callback
diff --git a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
index 3329a4af8e..d70f326e55 100644
--- a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
@@ -40,7 +40,7 @@ module ActiveRecord
           load_target.size
         end
 
-        def delete_records(records)
+        def delete_records(records, method = nil)
           if sql = @reflection.options[:delete_sql]
             records.each { |record| @owner.connection.delete(interpolate_sql(sql, record)) }
           else
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index caefd14ee3..aff955dd5f 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -54,20 +54,20 @@ module ActiveRecord
         end
 
         # Deletes the records according to the <tt>:dependent</tt> option.
-        def delete_records(records)
-          case @reflection.options[:dependent]
-            when :destroy
-              records.each { |r| r.destroy }
-            when :delete_all
-              @reflection.klass.delete(records.map { |r| r.id })
-            else
-              updates    = { @reflection.foreign_key => nil }
-              conditions = { @reflection.association_primary_key => records.map { |r| r.id } }
+        def delete_records(records, method = @reflection.options[:dependent])
+          case method
+          when :destroy
+            records.each { |r| r.destroy }
+          when :delete_all
+            @reflection.klass.delete(records.map { |r| r.id })
+          else
+            updates    = { @reflection.foreign_key => nil }
+            conditions = { @reflection.association_primary_key => records.map { |r| r.id } }
 
-              scoped.where(conditions).update_all(updates)
+            scoped.where(conditions).update_all(updates)
           end
 
-          if has_cached_counter? && @reflection.options[:dependent] != :destroy
+          if has_cached_counter? && method != :destroy
             @owner.class.update_counters(@owner.id, cached_counter_attribute_name => -records.size)
           end
         end
diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb
index d5b901beff..3174ea6373 100644
--- a/activerecord/lib/active_record/associations/has_many_through_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -8,13 +8,6 @@ module ActiveRecord
 
       alias_method :new, :build
 
-      def destroy(*records)
-        transaction do
-          delete_records(records.flatten)
-          super
-        end
-      end
-
       # Returns the size of the collection by executing a SELECT COUNT(*) query if the collection hasn't been
       # loaded and calling collection.size if it has. If it's more likely than not that the collection does
       # have a size larger than zero, and you need to fetch that collection afterwards, it'll take one fewer
@@ -51,10 +44,18 @@ module ActiveRecord
         end
 
         # TODO - add dependent option support
-        def delete_records(records)
+        def delete_records(records, method = @reflection.options[:dependent])
           through_association = @owner.send(@reflection.through_reflection.name)
-          records.each do |associate|
-            through_association.where(construct_join_attributes(associate)).delete_all
+
+          case method
+          when :destroy
+            records.each do |record|
+              through_association.where(construct_join_attributes(record)).destroy_all
+            end
+          else
+            records.each do |record|
+              through_association.where(construct_join_attributes(record)).delete_all
+            end
           end
         end
 
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