From 959fb8ea651fa6638aaa7caced20d921ca2ea5c1 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Thu, 7 Jun 2012 20:02:49 +0100 Subject: 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 --- activerecord/CHANGELOG.md | 12 ++++++++++++ .../active_record/associations/collection_association.rb | 14 ++------------ .../associations/has_and_belongs_to_many_association.rb | 16 +++++----------- .../active_record/associations/has_many_association.rb | 8 ++------ .../associations/has_many_through_association.rb | 4 ---- .../cases/associations/has_many_associations_test.rb | 12 ------------ 6 files changed, 21 insertions(+), 45 deletions(-) (limited to 'activerecord') 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 -- cgit v1.2.3