From 66679c8ecd9e916cbd96745b853603bc2fed7639 Mon Sep 17 00:00:00 2001 From: Ian Young Date: Mon, 17 Sep 2012 14:57:13 -0700 Subject: Update other counter caches on destroy --- activerecord/CHANGELOG.md | 5 +++++ .../lib/active_record/associations/builder/belongs_to.rb | 2 +- .../lib/active_record/associations/has_many_association.rb | 2 +- activerecord/lib/active_record/autosave_association.rb | 14 ++++++++++++++ activerecord/lib/active_record/core.rb | 1 + activerecord/test/cases/counter_cache_test.rb | 8 ++++++++ activerecord/test/fixtures/dog_lovers.yml | 3 +++ activerecord/test/fixtures/dogs.yml | 1 + activerecord/test/models/dog.rb | 5 +++-- activerecord/test/models/dog_lover.rb | 5 +++-- activerecord/test/schema/schema.rb | 8 +++++--- 11 files changed, 45 insertions(+), 9 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 3e368407f2..771f1333f0 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,5 +1,10 @@ ## Rails 4.0.0 (unreleased) ## +* Models with multiple counter cache associations now update correctly on destroy. + See #7706. + + *Ian Young* + * If inverse_of is true on an association, then when one calls +find()+ on the association, ActiveRecord will first look through the in-memory objects in the association for a particular id. Then, it will go to the DB if it diff --git a/activerecord/lib/active_record/associations/builder/belongs_to.rb b/activerecord/lib/active_record/associations/builder/belongs_to.rb index fbcb21118d..9ac561b997 100644 --- a/activerecord/lib/active_record/associations/builder/belongs_to.rb +++ b/activerecord/lib/active_record/associations/builder/belongs_to.rb @@ -31,7 +31,7 @@ module ActiveRecord::Associations::Builder end def belongs_to_counter_cache_before_destroy_for_#{name} - unless marked_for_destruction? + unless destroyed_by_association && destroyed_by_association.foreign_key.to_sym == #{foreign_key.to_sym.inspect} record = #{name} record.class.decrement_counter(:#{cache_column}, record.id) unless record.nil? end diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb index b7b4d7e3ae..29fae809da 100644 --- a/activerecord/lib/active_record/associations/has_many_association.rb +++ b/activerecord/lib/active_record/associations/has_many_association.rb @@ -22,7 +22,7 @@ module ActiveRecord else if options[:dependent] == :destroy # No point in executing the counter update since we're going to destroy the parent anyway - load_target.each(&:mark_for_destruction) + load_target.each { |t| t.destroyed_by_association = reflection } destroy_all else delete_all diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index 55542262b0..0df3e57947 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -212,6 +212,7 @@ module ActiveRecord # Reloads the attributes of the object as usual and clears marked_for_destruction flag. def reload(options = nil) @marked_for_destruction = false + @destroyed_by_association = nil super end @@ -231,6 +232,19 @@ module ActiveRecord @marked_for_destruction end + # Records the association that is being destroyed and destroying this + # record in the process. + def destroyed_by_association=(reflection) + @destroyed_by_association = reflection + end + + # Returns the association for the parent being destroyed. + # + # Used to avoid updating the counter cache unnecessarily. + def destroyed_by_association + @destroyed_by_association + end + # Returns whether or not this record has been changed in any way (including whether # any of its nested autosave associations are likewise changed) def changed_for_autosave? diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb index aa56219755..968dad5844 100644 --- a/activerecord/lib/active_record/core.rb +++ b/activerecord/lib/active_record/core.rb @@ -427,6 +427,7 @@ module ActiveRecord @readonly = false @destroyed = false @marked_for_destruction = false + @destroyed_by_association = nil @new_record = true @txn = nil @_start_transaction_state = {} diff --git a/activerecord/test/cases/counter_cache_test.rb b/activerecord/test/cases/counter_cache_test.rb index 7d06fb5093..5379a70034 100644 --- a/activerecord/test/cases/counter_cache_test.rb +++ b/activerecord/test/cases/counter_cache_test.rb @@ -115,6 +115,14 @@ class CounterCacheTest < ActiveRecord::TestCase end end + test "update other counters on parent destroy" do + david, joanna = dog_lovers(:david, :joanna) + + assert_difference 'joanna.reload.dogs_count', -1 do + david.destroy + end + end + test "reset the right counter if two have the same foreign key" do michael = people(:michael) assert_nothing_raised(ActiveRecord::StatementInvalid) do diff --git a/activerecord/test/fixtures/dog_lovers.yml b/activerecord/test/fixtures/dog_lovers.yml index d3e5e4a1aa..3f4c6c9e4c 100644 --- a/activerecord/test/fixtures/dog_lovers.yml +++ b/activerecord/test/fixtures/dog_lovers.yml @@ -2,3 +2,6 @@ david: id: 1 bred_dogs_count: 0 trained_dogs_count: 1 +joanna: + id: 2 + dogs_count: 1 diff --git a/activerecord/test/fixtures/dogs.yml b/activerecord/test/fixtures/dogs.yml index 16d19be2c5..b5eb2c7b74 100644 --- a/activerecord/test/fixtures/dogs.yml +++ b/activerecord/test/fixtures/dogs.yml @@ -1,3 +1,4 @@ sophie: id: 1 trainer_id: 1 + dog_lover_id: 2 diff --git a/activerecord/test/models/dog.rb b/activerecord/test/models/dog.rb index 72b7d33a86..b02b8447b8 100644 --- a/activerecord/test/models/dog.rb +++ b/activerecord/test/models/dog.rb @@ -1,4 +1,5 @@ class Dog < ActiveRecord::Base - belongs_to :breeder, :class_name => "DogLover", :counter_cache => :bred_dogs_count - belongs_to :trainer, :class_name => "DogLover", :counter_cache => :trained_dogs_count + belongs_to :breeder, class_name: "DogLover", counter_cache: :bred_dogs_count + belongs_to :trainer, class_name: "DogLover", counter_cache: :trained_dogs_count + belongs_to :doglover, foreign_key: :dog_lover_id, class_name: "DogLover", counter_cache: true end diff --git a/activerecord/test/models/dog_lover.rb b/activerecord/test/models/dog_lover.rb index a33dc575c5..2c5be94aea 100644 --- a/activerecord/test/models/dog_lover.rb +++ b/activerecord/test/models/dog_lover.rb @@ -1,4 +1,5 @@ class DogLover < ActiveRecord::Base - has_many :trained_dogs, :class_name => "Dog", :foreign_key => :trainer_id - has_many :bred_dogs, :class_name => "Dog", :foreign_key => :breeder_id + has_many :trained_dogs, class_name: "Dog", foreign_key: :trainer_id, dependent: :destroy + has_many :bred_dogs, class_name: "Dog", foreign_key: :breeder_id + has_many :dogs end diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index a952738e84..8fd4898ad6 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -230,14 +230,16 @@ ActiveRecord::Schema.define do t.integer :access_level, :default => 1 end - create_table :dog_lovers, :force => true do |t| - t.integer :trained_dogs_count, :default => 0 - t.integer :bred_dogs_count, :default => 0 + create_table :dog_lovers, force: true do |t| + t.integer :trained_dogs_count, default: 0 + t.integer :bred_dogs_count, default: 0 + t.integer :dogs_count, default: 0 end create_table :dogs, :force => true do |t| t.integer :trainer_id t.integer :breeder_id + t.integer :dog_lover_id end create_table :edges, :force => true, :id => false do |t| -- cgit v1.2.3