diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb index aff955dd5f..732f63713e 100644 --- a/activerecord/lib/active_record/associations/has_many_association.rb +++ b/activerecord/lib/active_record/associations/has_many_association.rb @@ -45,30 +45,54 @@ module ActiveRecord [@reflection.options[:limit], count].compact.min end - def has_cached_counter? - @owner.attribute_present?(cached_counter_attribute_name) + def has_cached_counter?(reflection = @reflection) + @owner.attribute_present?(cached_counter_attribute_name(reflection)) end - def cached_counter_attribute_name - "#{@reflection.name}_count" + def cached_counter_attribute_name(reflection = @reflection) + "#{reflection.name}_count" + end + + def update_counter(difference, reflection = @reflection) + if has_cached_counter?(reflection) + counter = cached_counter_attribute_name(reflection) + @owner.class.update_counters(@owner.id, counter => difference) + @owner[counter] += difference + @owner.changed_attributes.delete(counter) # eww + end + end + + # This shit is nasty. We need to avoid the following situation: + # + # * An associated record is deleted via record.destroy + # * Hence the callbacks run, and they find a belongs_to on the record with a + # :counter_cache options which points back at our @owner. So they update the + # counter cache. + # * In which case, we must make sure to *not* update the counter cache, or else + # it will be decremented twice. + # + # Hence this method. + def inverse_updates_counter_cache?(reflection = @reflection) + counter_name = cached_counter_attribute_name(reflection) + reflection.klass.reflect_on_all_associations(:belongs_to).any? { |inverse_reflection| + inverse_reflection.counter_cache_column == counter_name + } end # Deletes the records according to the :dependent option. def delete_records(records, method = @reflection.options[:dependent]) - case method - when :destroy + if method == :destroy records.each { |r| r.destroy } - when :delete_all - @reflection.klass.delete(records.map { |r| r.id }) + update_counter(-records.length) unless inverse_updates_counter_cache? else - updates = { @reflection.foreign_key => nil } - conditions = { @reflection.association_primary_key => records.map { |r| r.id } } + keys = records.map { |r| r[@reflection.association_primary_key] } + scope = scoped.where(@reflection.association_primary_key => keys) - scoped.where(conditions).update_all(updates) - end - - if has_cached_counter? && method != :destroy - @owner.class.update_counters(@owner.id, cached_counter_attribute_name => -records.size) + if method == :delete_all + update_counter(-scope.delete_all) + else + update_counter(-scope.update_all(@reflection.foreign_key => nil)) + end end end 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 c98ac79dc0..040de7e98f 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -31,6 +31,9 @@ module ActiveRecord through_association = @owner.send(@reflection.through_reflection.name) through_association.create!(construct_join_attributes(record)) + + update_counter(1) + true end private @@ -43,19 +46,35 @@ module ActiveRecord end end - def deletion_scope(records) - @owner.send(@reflection.through_reflection.name).where(construct_join_attributes(*records)) + def update_through_counter?(method) + case method + when :destroy + !inverse_updates_counter_cache?(@reflection.through_reflection) + when :nullify + false + else + true + end end def delete_records(records, method = @reflection.options[:dependent]) + through = @owner.send(:association_proxy, @reflection.through_reflection.name) + scope = through.scoped.where(construct_join_attributes(*records)) + case method when :destroy - deletion_scope(records).destroy_all + count = scope.destroy_all.length when :nullify - deletion_scope(records).update_all(@reflection.source_reflection.foreign_key => nil) + count = scope.update_all(@reflection.source_reflection.foreign_key => nil) else - deletion_scope(records).delete_all + count = scope.delete_all end + + if @reflection.through_reflection.macro == :has_many && update_through_counter?(method) + update_counter(-count, @reflection.through_reflection) + end + + update_counter(-count) end def find_target diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index e36124a055..23b777ac6d 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -630,7 +630,7 @@ class HasManyAssociationsTest < ActiveRecord::TestCase assert_equal topic.replies.to_a.size, topic.replies_count end - def test_deleting_updates_counter_cache_without_dependent_destroy + def test_deleting_updates_counter_cache_without_dependent_option post = posts(:welcome) assert_difference "post.reload.taggings_count", -1 do @@ -640,16 +640,22 @@ class HasManyAssociationsTest < ActiveRecord::TestCase def test_deleting_updates_counter_cache_with_dependent_delete_all post = posts(:welcome) - - # Manually update the count as the tagging will have been added to the taggings association, - # rather than to the taggings_with_delete_all one (which is just a 'shadow' of the former) - post.update_attribute(:taggings_with_delete_all_count, post.taggings_with_delete_all.to_a.count) + post.update_attribute(:taggings_with_delete_all_count, post.taggings_count) assert_difference "post.reload.taggings_with_delete_all_count", -1 do post.taggings_with_delete_all.delete(post.taggings_with_delete_all.first) end end + def test_deleting_updates_counter_cache_with_dependent_destroy + post = posts(:welcome) + post.update_attribute(:taggings_with_destroy_count, post.taggings_count) + + assert_difference "post.reload.taggings_with_destroy_count", -1 do + post.taggings_with_destroy.delete(post.taggings_with_destroy.first) + end + end + def test_deleting_a_collection force_signal37_to_load_all_clients_of_firm companies(:first_firm).clients_of_firm.create("name" => "Another Client") @@ -701,9 +707,9 @@ class HasManyAssociationsTest < ActiveRecord::TestCase def test_clearing_updates_counter_cache topic = Topic.first - topic.replies.clear - topic.reload - assert_equal 0, topic.replies_count + assert_difference 'topic.reload.replies_count', -1 do + topic.replies.clear + end end def test_clearing_a_dependent_association_collection diff --git a/activerecord/test/cases/associations/has_many_through_associations_test.rb b/activerecord/test/cases/associations/has_many_through_associations_test.rb index 2aaf5750ba..c58068ef75 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -25,8 +25,8 @@ require 'models/membership' require 'models/club' class HasManyThroughAssociationsTest < ActiveRecord::TestCase - fixtures :posts, :readers, :people, :comments, :authors, :categories, - :owners, :pets, :toys, :jobs, :references, :companies, :members, + fixtures :posts, :readers, :people, :comments, :authors, :categories, :taggings, :tags, + :owners, :pets, :toys, :jobs, :references, :companies, :members, :author_addresses, :subscribers, :books, :subscriptions, :developers, :categorizations # Dummies to force column loads so query counts are clean. @@ -255,6 +255,37 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase end end + def test_update_counter_caches_on_delete + post = posts(:welcome) + tag = post.tags.create!(:name => 'doomed') + + assert_difference ['post.reload.taggings_count', 'post.reload.tags_count'], -1 do + posts(:welcome).tags.delete(tag) + end + end + + def test_update_counter_caches_on_delete_with_dependent_destroy + post = posts(:welcome) + tag = post.tags.create!(:name => 'doomed') + post.update_attribute(:tags_with_destroy_count, post.tags.count) + + assert_difference ['post.reload.taggings_count', 'post.reload.tags_with_destroy_count'], -1 do + posts(:welcome).tags_with_destroy.delete(tag) + end + end + + def test_update_counter_caches_on_delete_with_dependent_nullify + post = posts(:welcome) + tag = post.tags.create!(:name => 'doomed') + post.update_attribute(:tags_with_nullify_count, post.tags.count) + + assert_no_difference 'post.reload.taggings_count' do + assert_difference 'post.reload.tags_with_nullify_count', -1 do + posts(:welcome).tags_with_nullify.delete(tag) + end + end + end + def test_replace_association assert_queries(4){posts(:welcome);people(:david);people(:michael); posts(:welcome).people(true)} @@ -671,4 +702,13 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase assert_equal true, club.reload.membership.favourite end + + def test_deleting_from_has_many_through_a_belongs_to_should_not_try_to_update_counter + post = posts(:welcome) + address = author_addresses(:david_address) + + assert post.author_addresses.include?(address) + post.author_addresses.delete(address) + assert post[:author_count].nil? + end end diff --git a/activerecord/test/fixtures/posts.yml b/activerecord/test/fixtures/posts.yml index f817493190..07069a064f 100644 --- a/activerecord/test/fixtures/posts.yml +++ b/activerecord/test/fixtures/posts.yml @@ -5,6 +5,7 @@ welcome: body: Such a lovely day comments_count: 2 taggings_count: 1 + tags_count: 1 type: Post thinking: @@ -14,6 +15,7 @@ thinking: body: Like I hopefully always am comments_count: 1 taggings_count: 1 + tags_count: 1 type: SpecialPost authorless: diff --git a/activerecord/test/models/post.rb b/activerecord/test/models/post.rb index 1c95d30d6b..fd8cd2244a 100644 --- a/activerecord/test/models/post.rb +++ b/activerecord/test/models/post.rb @@ -39,6 +39,7 @@ class Post < ActiveRecord::Base has_many :author_favorites, :through => :author has_many :author_categorizations, :through => :author, :source => :categorizations + has_many :author_addresses, :through => :author has_one :very_special_comment has_one :very_special_comment_with_post, :class_name => "VerySpecialComment", :include => :post @@ -57,6 +58,10 @@ class Post < ActiveRecord::Base end has_many :taggings_with_delete_all, :class_name => 'Tagging', :as => :taggable, :dependent => :delete_all + has_many :taggings_with_destroy, :class_name => 'Tagging', :as => :taggable, :dependent => :destroy + + has_many :tags_with_destroy, :through => :taggings, :source => :tag, :dependent => :destroy + has_many :tags_with_nullify, :through => :taggings, :source => :tag, :dependent => :nullify has_many :misc_tags, :through => :taggings, :source => :tag, :conditions => "tags.name = 'Misc'" has_many :funky_tags, :through => :taggings, :source => :tag diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index 09c7b7ba63..665a4fe914 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -455,6 +455,10 @@ ActiveRecord::Schema.define do t.integer :comments_count, :default => 0 t.integer :taggings_count, :default => 0 t.integer :taggings_with_delete_all_count, :default => 0 + t.integer :taggings_with_destroy_count, :default => 0 + t.integer :tags_count, :default => 0 + t.integer :tags_with_destroy_count, :default => 0 + t.integer :tags_with_nullify_count, :default => 0 end create_table :price_estimates, :force => true do |t|