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.

7 files changed, 130 insertions, 30 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
diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb
index e36124a055..23b777ac6d 100644
--- a/activerecord/test/cases/associations/has_many_associations_test.rb
+++ b/activerecord/test/cases/associations/has_many_associations_test.rb
@@ -630,7 +630,7 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
     assert_equal topic.replies.to_a.size, topic.replies_count
   end
 
-  def test_deleting_updates_counter_cache_without_dependent_destroy
+  def test_deleting_updates_counter_cache_without_dependent_option
     post = posts(:welcome)
 
     assert_difference "post.reload.taggings_count", -1 do
@@ -640,16 +640,22 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
 
   def test_deleting_updates_counter_cache_with_dependent_delete_all
     post = posts(:welcome)
-
-    # Manually update the count as the tagging will have been added to the taggings association,
-    # rather than to the taggings_with_delete_all one (which is just a 'shadow' of the former)
-    post.update_attribute(:taggings_with_delete_all_count, post.taggings_with_delete_all.to_a.count)
+    post.update_attribute(:taggings_with_delete_all_count, post.taggings_count)
 
     assert_difference "post.reload.taggings_with_delete_all_count", -1 do
       post.taggings_with_delete_all.delete(post.taggings_with_delete_all.first)
     end
   end
 
+  def test_deleting_updates_counter_cache_with_dependent_destroy
+    post = posts(:welcome)
+    post.update_attribute(:taggings_with_destroy_count, post.taggings_count)
+
+    assert_difference "post.reload.taggings_with_destroy_count", -1 do
+      post.taggings_with_destroy.delete(post.taggings_with_destroy.first)
+    end
+  end
+
   def test_deleting_a_collection
     force_signal37_to_load_all_clients_of_firm
     companies(:first_firm).clients_of_firm.create("name" => "Another Client")
@@ -701,9 +707,9 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
   def test_clearing_updates_counter_cache
     topic = Topic.first
 
-    topic.replies.clear
-    topic.reload
-    assert_equal 0, topic.replies_count
+    assert_difference 'topic.reload.replies_count', -1 do
+      topic.replies.clear
+    end
   end
 
   def test_clearing_a_dependent_association_collection
diff --git a/activerecord/test/cases/associations/has_many_through_associations_test.rb b/activerecord/test/cases/associations/has_many_through_associations_test.rb
index 2aaf5750ba..c58068ef75 100644
--- a/activerecord/test/cases/associations/has_many_through_associations_test.rb
+++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb
@@ -25,8 +25,8 @@ require 'models/membership'
 require 'models/club'
 
 class HasManyThroughAssociationsTest < ActiveRecord::TestCase
-  fixtures :posts, :readers, :people, :comments, :authors, :categories,
-           :owners, :pets, :toys, :jobs, :references, :companies, :members,
+  fixtures :posts, :readers, :people, :comments, :authors, :categories, :taggings, :tags,
+           :owners, :pets, :toys, :jobs, :references, :companies, :members, :author_addresses,
            :subscribers, :books, :subscriptions, :developers, :categorizations
 
   # Dummies to force column loads so query counts are clean.
@@ -255,6 +255,37 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
     end
   end
 
+  def test_update_counter_caches_on_delete
+    post = posts(:welcome)
+    tag  = post.tags.create!(:name => 'doomed')
+
+    assert_difference ['post.reload.taggings_count', 'post.reload.tags_count'], -1 do
+      posts(:welcome).tags.delete(tag)
+    end
+  end
+
+  def test_update_counter_caches_on_delete_with_dependent_destroy
+    post = posts(:welcome)
+    tag  = post.tags.create!(:name => 'doomed')
+    post.update_attribute(:tags_with_destroy_count, post.tags.count)
+
+    assert_difference ['post.reload.taggings_count', 'post.reload.tags_with_destroy_count'], -1 do
+      posts(:welcome).tags_with_destroy.delete(tag)
+    end
+  end
+
+  def test_update_counter_caches_on_delete_with_dependent_nullify
+    post = posts(:welcome)
+    tag  = post.tags.create!(:name => 'doomed')
+    post.update_attribute(:tags_with_nullify_count, post.tags.count)
+
+    assert_no_difference 'post.reload.taggings_count' do
+      assert_difference 'post.reload.tags_with_nullify_count', -1 do
+        posts(:welcome).tags_with_nullify.delete(tag)
+      end
+    end
+  end
+
   def test_replace_association
     assert_queries(4){posts(:welcome);people(:david);people(:michael); posts(:welcome).people(true)}
 
@@ -671,4 +702,13 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
 
     assert_equal true, club.reload.membership.favourite
   end
+
+  def test_deleting_from_has_many_through_a_belongs_to_should_not_try_to_update_counter
+    post    = posts(:welcome)
+    address = author_addresses(:david_address)
+
+    assert post.author_addresses.include?(address)
+    post.author_addresses.delete(address)
+    assert post[:author_count].nil?
+  end
 end
diff --git a/activerecord/test/fixtures/posts.yml b/activerecord/test/fixtures/posts.yml
index f817493190..07069a064f 100644
--- a/activerecord/test/fixtures/posts.yml
+++ b/activerecord/test/fixtures/posts.yml
@@ -5,6 +5,7 @@ welcome:
   body: Such a lovely day
   comments_count: 2
   taggings_count: 1
+  tags_count: 1
   type: Post
 
 thinking:
@@ -14,6 +15,7 @@ thinking:
   body: Like I hopefully always am
   comments_count: 1
   taggings_count: 1
+  tags_count: 1
   type: SpecialPost
 
 authorless:
diff --git a/activerecord/test/models/post.rb b/activerecord/test/models/post.rb
index 1c95d30d6b..fd8cd2244a 100644
--- a/activerecord/test/models/post.rb
+++ b/activerecord/test/models/post.rb
@@ -39,6 +39,7 @@ class Post < ActiveRecord::Base
 
   has_many :author_favorites, :through => :author
   has_many :author_categorizations, :through => :author, :source => :categorizations
+  has_many :author_addresses, :through => :author
 
   has_one  :very_special_comment
   has_one  :very_special_comment_with_post, :class_name => "VerySpecialComment", :include => :post
@@ -57,6 +58,10 @@ class Post < ActiveRecord::Base
   end
 
   has_many :taggings_with_delete_all, :class_name => 'Tagging', :as => :taggable, :dependent => :delete_all
+  has_many :taggings_with_destroy, :class_name => 'Tagging', :as => :taggable, :dependent => :destroy
+
+  has_many :tags_with_destroy, :through => :taggings, :source => :tag, :dependent => :destroy
+  has_many :tags_with_nullify, :through => :taggings, :source => :tag, :dependent => :nullify
 
   has_many :misc_tags, :through => :taggings, :source => :tag, :conditions => "tags.name = 'Misc'"
   has_many :funky_tags, :through => :taggings, :source => :tag
diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb
index 09c7b7ba63..665a4fe914 100644
--- a/activerecord/test/schema/schema.rb
+++ b/activerecord/test/schema/schema.rb
@@ -455,6 +455,10 @@ ActiveRecord::Schema.define do
     t.integer :comments_count, :default => 0
     t.integer :taggings_count, :default => 0
     t.integer :taggings_with_delete_all_count, :default => 0
+    t.integer :taggings_with_destroy_count, :default => 0
+    t.integer :tags_count, :default => 0
+    t.integer :tags_with_destroy_count, :default => 0
+    t.integer :tags_with_nullify_count, :default => 0
   end
 
   create_table :price_estimates, :force => true do |t|