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

Bug rails#6289

Conflicts:

	activerecord/test/cases/associations/has_many_associations_test.rb

Author: jonleighton

activerecord/CHANGELOG.md

@@ -1,5 +1,8 @@
 ## Rails 3.2.4 (unreleased) ##
 
+*   Perf fix: Don't load the records when doing assoc.delete_all.
+    GH #6289. *Jon Leighton*
+
 *   Association preloading shouldn't be affected by the current scoping.
     This could cause infinite recursion and potentially other problems.
     See GH #5667. *Jon Leighton*

activerecord/lib/active_record/associations/collection_association.rb

@@ -154,7 +154,7 @@ def transaction(*args)
       #
       # See delete for more info.
       def delete_all
-        delete(load_target).tap do
+        delete(:all).tap do
           reset
           loaded!
         end
@@ -226,7 +226,17 @@ def count(column_name = nil, count_options = {})
       # are actually removed from the database, that depends precisely on
       # +delete_records+. They are in any case removed from the collection.
       def delete(*records)
-        delete_or_destroy(records, options[:dependent])
+        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
       end
 
       # Destroy +records+ and remove them from this association calling

activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb

@@ -46,11 +46,17 @@ def delete_records(records, method)
           if sql = options[:delete_sql]
             records.each { |record| owner.connection.delete(interpolate(sql, record)) }
           else
-            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
+            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)
           end
         end
 

activerecord/lib/active_record/associations/has_many_association.rb

@@ -89,8 +89,12 @@ def delete_records(records, method)
             records.each { |r| r.destroy }
             update_counter(-records.length) unless inverse_updates_counter_cache?
           else
-            keys  = records.map { |r| r[reflection.association_primary_key] }
-            scope = scoped.where(reflection.association_primary_key => keys)
+            if records == :all
+              scope = scoped
+            else
+              keys  = records.map { |r| r[reflection.association_primary_key] }
+              scope = scoped.where(reflection.association_primary_key => keys)
+            end
 
             if method == :delete_all
               update_counter(-scope.delete_all)

activerecord/lib/active_record/associations/has_many_through_association.rb

@@ -126,6 +126,10 @@ def update_through_counter?(method)
         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

activerecord/test/cases/associations/has_many_associations_test.rb

@@ -1698,4 +1698,16 @@ def test_replace_returns_new_target
     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