Permalink
Browse files

Correctly update counter caches on deletion for has_many :through [#2824

 state:resolved]. Also fixed a bunch of other counter cache bugs in the process, as once I fixed this one others started appearing like nobody's business.
  • Loading branch information...
jonleighton committed Feb 5, 2011
1 parent 05bcb8c commit 52f09eac5b3d297021ef726e04ec19f6011cb302
@@ -45,30 +45,54 @@ def count_records
[@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 <tt>:dependent</tt> 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 } }
scoped.where(conditions).update_all(updates)
end
keys = records.map { |r| r[@reflection.association_primary_key] }
scope = scoped.where(@reflection.association_primary_key => keys)
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
@@ -31,6 +31,9 @@ def insert_record(record, force = true, validate = true)
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 @@ def target_reflection_has_associated_record?
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
@@ -630,7 +630,7 @@ def test_deleting_updates_counter_cache
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 @@ def test_deleting_updates_counter_cache_without_dependent_destroy
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 @@ def test_clearing_an_association_collection
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
@@ -25,8 +25,8 @@
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 @@ def test_belongs_to_with_dependent_nullify
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 @@ def test_create_with_conditions_hash_on_through_association
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
@@ -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:
@@ -39,6 +39,7 @@ def find_most_recent
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 @@ def add_joins_and_select
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
@@ -455,6 +455,10 @@ def create_table(*args, &block)
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|

0 comments on commit 52f09ea

Please sign in to comment.