From 52f09eac5b3d297021ef726e04ec19f6011cb302 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sat, 5 Feb 2011 13:13:49 +0000 Subject: 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. --- .../associations/has_many_association.rb | 54 ++++++++++++++++------ .../associations/has_many_through_association.rb | 29 ++++++++++-- 2 files changed, 63 insertions(+), 20 deletions(-) (limited to 'activerecord/lib') 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 } } - - 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 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 -- cgit v1.2.3