diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 3e368407f26d0..771f1333f05ea 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 fbcb21118d670..9ac561b997b19 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 @@ def belongs_to_counter_cache_after_create_for_#{name} 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 b7b4d7e3ae159..29fae809da173 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 @@ def handle_dependency 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 55542262b0f09..0df3e57947b06 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -212,6 +212,7 @@ def add_autosave_association_callbacks(reflection) # 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 @@ def marked_for_destruction? @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 aa56219755224..968dad5844cd6 100644 --- a/activerecord/lib/active_record/core.rb +++ b/activerecord/lib/active_record/core.rb @@ -427,6 +427,7 @@ def init_internals @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 7d06fb50936c6..5379a70034f27 100644 --- a/activerecord/test/cases/counter_cache_test.rb +++ b/activerecord/test/cases/counter_cache_test.rb @@ -115,6 +115,14 @@ class ::SpecialReply < ::Reply 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 d3e5e4a1aa991..3f4c6c9e4c506 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 16d19be2c506b..b5eb2c7b74c16 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 72b7d33a864b0..b02b8447b83d8 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 a33dc575c59ad..2c5be94aea44b 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 a952738e84418..8fd4898ad6032 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -230,14 +230,16 @@ def create_table(*args, &block) 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|