Revert "Perf: Don't load the association for #delete_all."

This reverts commit b98d1e21635d8776de8893cc09bd86c71f6c78f0.

Closes #6609

Conflicts:

	activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb

6 files changed, 21 insertions, 45 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 0bd7685f9b..71050efbc5 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,15 @@
+## Rails 3.2.6 (unreleased) ##
+
+*   Revert earlier 'perf fix' (see 3.2.4 changelog / GH #6289). This
+    change introduced a regression (GH #6609). assoc.clear and
+    assoc.delete_all have loaded the association before doing the delete
+    since at least Rails 2.3. Doing the delete without loading the
+    records means that the `before_remove` and `after_remove` callbacks do
+    not get invoked. Therefore, this change was less a fix a more an
+    optimisation, which should only have gone into master.
+
+    *Jon Leighton*
+
 ## Rails 3.2.5 (Jun 1, 2012) ##
 
 *   Restore behavior of Active Record 3.2.3 scopes.
diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb
index 4c58094cfd..8ef6b594fd 100644
--- a/activerecord/lib/active_record/associations/collection_association.rb
+++ b/activerecord/lib/active_record/associations/collection_association.rb
@@ -154,7 +154,7 @@ module ActiveRecord
       #
       # See delete for more info.
       def delete_all
-        delete(:all).tap do
+        delete(load_target).tap do
           reset
           loaded!
         end
@@ -226,17 +226,7 @@ module ActiveRecord
       # are actually removed from the database, that depends precisely on
       # +delete_records+. They are in any case removed from the collection.
       def delete(*records)
-        dependent = options[:dependent]
-
-        if records.first == :all
-          if loaded? || dependent == :destroy
-            delete_or_destroy(load_target, dependent)
-          else
-            delete_records(:all, dependent)
-          end
-        else
-          delete_or_destroy(records, dependent)
-        end
+        delete_or_destroy(records, options[:dependent])
       end
 
       # Destroy +records+ and remove them from this association calling
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 e5da0d585c..8c6821323f 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
@@ -47,17 +47,11 @@ module ActiveRecord
             records = load_target if records == :all
             records.each { |record| owner.connection.delete(interpolate(sql, record)) }
           else
-            relation  = join_table
-            condition = relation[reflection.foreign_key].eq(owner.id)
-
-            unless records == :all
-              condition = condition.and(
-                relation[reflection.association_foreign_key].
-                  in(records.map { |x| x.id }.compact)
-              )
-            end
-
-            owner.connection.delete(relation.where(condition).compile_delete)
+            relation = join_table
+            stmt = relation.where(relation[reflection.foreign_key].eq(owner.id).
+              and(relation[reflection.association_foreign_key].in(records.map { |x| x.id }.compact))
+            ).compile_delete
+            owner.connection.delete stmt
           end
         end
 
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index e631579087..059e6c77bc 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -89,12 +89,8 @@ module ActiveRecord
             records.each { |r| r.destroy }
             update_counter(-records.length) unless inverse_updates_counter_cache?
           else
-            if records == :all
-              scope = scoped
-            else
-              keys  = records.map { |r| r[reflection.association_primary_key] }
-              scope = scoped.where(reflection.association_primary_key => keys)
-            end
+            keys  = records.map { |r| r[reflection.association_primary_key] }
+            scope = scoped.where(reflection.association_primary_key => keys)
 
             if method == :delete_all
               update_counter(-scope.delete_all)
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 86c665b5fe..53d49fef2e 100644
--- a/activerecord/lib/active_record/associations/has_many_through_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -126,10 +126,6 @@ module ActiveRecord
         def delete_records(records, method)
           ensure_not_nested
 
-          # This is unoptimised; it will load all the target records
-          # even when we just want to delete everything.
-          records = load_target if records == :all
-
           scope = through_association.scoped.where(construct_join_attributes(*records))
 
           case method
diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb
index 4f50c97147..a51ce26117 100644
--- a/activerecord/test/cases/associations/has_many_associations_test.rb
+++ b/activerecord/test/cases/associations/has_many_associations_test.rb
@@ -1698,16 +1698,4 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
     assert_equal [bulb1, bulb3], car.bulbs
     assert_equal [bulb1, bulb3], result
   end
-
-  test "delete_all, when not loaded, doesn't load the records" do
-    post = posts(:welcome)
-
-    assert post.taggings_with_delete_all.count > 0
-    assert !post.taggings_with_delete_all.loaded?
-
-    # 2 queries: one DELETE and another to update the counter cache
-    assert_queries(2) do
-      post.taggings_with_delete_all.delete_all
-    end
-  end
 end