Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

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

Bug #6289

Conflicts:

	activerecord/test/cases/associations/has_many_associations_test.rb
  • Loading branch information...
commit b98d1e21635d8776de8893cc09bd86c71f6c78f0 1 parent 2802ad0
@jonleighton jonleighton authored
View
3  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*
View
14 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
View
16 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
View
8 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)
View
4 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
View
12 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
Please sign in to comment.
Something went wrong with that request. Please try again.