diff options
author | Jon Leighton <j@jonathanleighton.com> | 2011-02-05 13:13:49 +0000 |
---|---|---|
committer | Jon Leighton <j@jonathanleighton.com> | 2011-02-07 23:35:05 +0000 |
commit | 52f09eac5b3d297021ef726e04ec19f6011cb302 (patch) | |
tree | c2b15313ce7bc00ade263d2e9030721c32381f3b /activerecord/lib/active_record | |
parent | 05bcb8cecc8573f28ad080839233b4bb9ace07be (diff) | |
download | rails-52f09eac5b3d297021ef726e04ec19f6011cb302.tar.gz rails-52f09eac5b3d297021ef726e04ec19f6011cb302.tar.bz2 rails-52f09eac5b3d297021ef726e04ec19f6011cb302.zip |
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.
Diffstat (limited to 'activerecord/lib/active_record')
-rw-r--r-- | activerecord/lib/active_record/associations/has_many_association.rb | 54 | ||||
-rw-r--r-- | activerecord/lib/active_record/associations/has_many_through_association.rb | 29 |
2 files changed, 63 insertions, 20 deletions
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 <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 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 |