Merge pull request #7706 from iangreenleaf/multiple_counter_caches

Update other counter caches on destroy

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 <tt>marked_for_destruction</tt> 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|