diff options
6 files changed, 48 insertions, 9 deletions
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 2f4a0873b0..8dab1f3db0 100644 --- a/activerecord/CHANGELOG.md +++ b/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* diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb index 3e853454a8..0dace89dac 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(load_target).tap do + delete(:all).tap do reset loaded! end @@ -226,7 +226,17 @@ 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) - 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 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 a4cea99372..e8ae3e51b9 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 @@ -46,11 +46,17 @@ module ActiveRecord 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 diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb index 059e6c77bc..e631579087 100644 --- a/activerecord/lib/active_record/associations/has_many_association.rb +++ b/activerecord/lib/active_record/associations/has_many_association.rb @@ -89,8 +89,12 @@ module ActiveRecord 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) 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 53d49fef2e..86c665b5fe 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -126,6 +126,10 @@ 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 a28b8e6651..c128ede34f 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -1698,4 +1698,16 @@ 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 |