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.

5 files changed, 67 insertions, 10 deletions

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|