Deprecate automatic counter caches on has_many :through

This functionality has been broken for a long time. We can only set it
one way. It does not update when the intermediate records are destroyed.
It is entirely duplicating the behavior of a counter_cache on the through
record. It acts inconsistently with manual counter caches.

It was proposed that we should deprecate this behavior a while back
(rails#3903 (comment)). I
think the time has come to do so.

activerecord/lib/active_record/associations/has_many_association.rb

@@ -100,6 +100,10 @@ def update_counter(difference, reflection = reflection())
         # Hence this method.
         def inverse_updates_counter_cache?(reflection = reflection())
           counter_name = cached_counter_attribute_name(reflection)
+          inverse_updates_counter_named?(counter_name, reflection)
+        end
+
+        def inverse_updates_counter_named?(counter_name, reflection = reflection())
           reflection.klass._reflections.values.any? { |inverse_reflection|
             :belongs_to == inverse_reflection.macro &&
             inverse_reflection.counter_cache_column == counter_name

activerecord/lib/active_record/associations/has_many_through_association.rb

@@ -63,7 +63,15 @@ def insert_record(record, validate = true, raise = false)
         end
 
         save_through_record(record)
-        update_counter(1)
+        if has_cached_counter? && !through_reflection_updates_counter_cache?
+          ActiveSupport::Deprecation.warn(<<-MESSAGE.strip_heredoc)
+            Automatic updating of counter caches on through associations has been
+            deprecated, and will be removed in Rails 5.0. Instead, please set the
+            appropriate counter_cache options on the has_many and belongs_to for
+            your associations to #{through_reflection.name}.
+          MESSAGE
+          update_counter(1)
+        end
         record
       end
 
@@ -216,6 +224,11 @@ def find_target
         def invertible_for?(record)
           false
         end
+
+        def through_reflection_updates_counter_cache?
+          counter_name = cached_counter_attribute_name
+          inverse_updates_counter_named?(counter_name, through_reflection)
+        end
     end
   end
 end

activerecord/test/cases/associations/belongs_to_associations_test.rb

@@ -787,8 +787,8 @@ def test_polymorphic_counter_cache
     post    = posts(:welcome)
     comment = comments(:greetings)
 
-    assert_difference lambda { post.reload.taggings_count }, -1 do
-      assert_difference 'comment.reload.taggings_count', +1 do
+    assert_difference lambda { post.reload.tags_count }, -1 do
+      assert_difference 'comment.reload.tags_count', +1 do
         tagging.taggable = comment
       end
     end

activerecord/test/cases/associations/cascaded_eager_loading_test.rb

@@ -35,9 +35,9 @@ def test_eager_association_loading_with_cascaded_two_levels_and_one_level
 
   def test_eager_association_loading_with_hmt_does_not_table_name_collide_when_joining_associations
     assert_nothing_raised do
-      Author.joins(:posts).eager_load(:comments).where(:posts => {:taggings_count => 1}).to_a
+      Author.joins(:posts).eager_load(:comments).where(:posts => {:tags_count => 1}).to_a
     end
-    authors = Author.joins(:posts).eager_load(:comments).where(:posts => {:taggings_count => 1}).to_a
+    authors = Author.joins(:posts).eager_load(:comments).where(:posts => {:tags_count => 1}).to_a
     assert_equal 1, assert_no_queries { authors.size }
     assert_equal 10, assert_no_queries { authors[0].comments.size }
   end

activerecord/test/cases/associations/has_many_associations_test.rb

@@ -36,7 +36,7 @@ def test_should_generate_valid_sql
     author = authors(:david)
     # this can fail on adapters which require ORDER BY expressions to be included in the SELECT expression
     # if the reorder clauses are not correctly handled
-    assert author.posts_with_comments_sorted_by_comment_id.where('comments.id > 0').reorder('posts.comments_count DESC', 'posts.taggings_count DESC').last
+    assert author.posts_with_comments_sorted_by_comment_id.where('comments.id > 0').reorder('posts.comments_count DESC', 'posts.tags_count DESC').last
   end
 end
 
@@ -784,14 +784,14 @@ def test_pushing_association_updates_counter_cache
   def test_deleting_updates_counter_cache_without_dependent_option
     post = posts(:welcome)
 
-    assert_difference "post.reload.taggings_count", -1 do
+    assert_difference "post.reload.tags_count", -1 do
       post.taggings.delete(post.taggings.first)
     end
   end
 
   def test_deleting_updates_counter_cache_with_dependent_delete_all
     post = posts(:welcome)
-    post.update_columns(taggings_with_delete_all_count: post.taggings_count)
+    post.update_columns(taggings_with_delete_all_count: post.tags_count)
 
     assert_difference "post.reload.taggings_with_delete_all_count", -1 do
       post.taggings_with_delete_all.delete(post.taggings_with_delete_all.first)
@@ -800,7 +800,7 @@ def test_deleting_updates_counter_cache_with_dependent_delete_all
 
   def test_deleting_updates_counter_cache_with_dependent_destroy
     post = posts(:welcome)
-    post.update_columns(taggings_with_destroy_count: post.taggings_count)
+    post.update_columns(taggings_with_destroy_count: post.tags_count)
 
     assert_difference "post.reload.taggings_with_destroy_count", -1 do
       post.taggings_with_destroy.delete(post.taggings_with_destroy.first)

activerecord/test/cases/associations/has_many_through_associations_test.rb

@@ -489,7 +489,7 @@ 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
+    assert_difference ['post.reload.tags_count'], -1 do
       posts(:welcome).tags.delete(tag)
     end
   end
@@ -499,7 +499,7 @@ def test_update_counter_caches_on_delete_with_dependent_destroy
     tag  = post.tags.create!(:name => 'doomed')
     post.update_columns(tags_with_destroy_count: post.tags.count)
 
-    assert_difference ['post.reload.taggings_count', 'post.reload.tags_with_destroy_count'], -1 do
+    assert_difference ['post.reload.tags_with_destroy_count'], -1 do
       posts(:welcome).tags_with_destroy.delete(tag)
     end
   end
@@ -509,7 +509,7 @@ def test_update_counter_caches_on_delete_with_dependent_nullify
     tag  = post.tags.create!(:name => 'doomed')
     post.update_columns(tags_with_nullify_count: post.tags.count)
 
-    assert_no_difference 'post.reload.taggings_count' do
+    assert_no_difference 'post.reload.tags_count' do
       assert_difference 'post.reload.tags_with_nullify_count', -1 do
         posts(:welcome).tags_with_nullify.delete(tag)
       end
@@ -524,14 +524,14 @@ def test_update_counter_caches_on_replace_association
     tag.tagged_posts = []
     post.reload
 
-    assert_equal(post.taggings.count, post.taggings_count)
+    assert_equal(post.taggings.count, post.tags_count)
   end
 
   def test_update_counter_caches_on_destroy
     post = posts(:welcome)
     tag  = post.tags.create!(name: 'doomed')
 
-    assert_difference 'post.reload.taggings_count', -1 do
+    assert_difference 'post.reload.tags_count', -1 do
       tag.tagged_posts.destroy(post)
     end
   end

activerecord/test/cases/associations/join_model_test.rb

@@ -326,11 +326,11 @@ def test_has_many_through_with_custom_primary_key_on_has_many_source
   end
 
   def test_belongs_to_polymorphic_with_counter_cache
-    assert_equal 1, posts(:welcome)[:taggings_count]
+    assert_equal 1, posts(:welcome)[:tags_count]
     tagging = posts(:welcome).taggings.create(:tag => tags(:general))
-    assert_equal 2, posts(:welcome, :reload)[:taggings_count]
+    assert_equal 2, posts(:welcome, :reload)[:tags_count]
     tagging.destroy
-    assert_equal 1, posts(:welcome, :reload)[:taggings_count]
+    assert_equal 1, posts(:welcome, :reload)[:tags_count]
   end
 
   def test_unavailable_through_reflection
@@ -489,23 +489,23 @@ def test_create_associate_when_adding_to_has_many_through
                 message = "Expected a Tag in tags collection, got #{wrong.class}.")
     assert_nil( wrong = post_thinking.taggings.detect { |t| t.class != Tagging },
                 message = "Expected a Tagging in taggings collection, got #{wrong.class}.")
-    assert_equal(count + 1, post_thinking.tags.size)
+    assert_equal(count + 1, post_thinking.reload.tags.size)
     assert_equal(count + 1, post_thinking.tags(true).size)
 
     assert_kind_of Tag, post_thinking.tags.create!(:name => 'foo')
     assert_nil( wrong = post_thinking.tags.detect { |t| t.class != Tag },
                 message = "Expected a Tag in tags collection, got #{wrong.class}.")
     assert_nil( wrong = post_thinking.taggings.detect { |t| t.class != Tagging },
                 message = "Expected a Tagging in taggings collection, got #{wrong.class}.")
-    assert_equal(count + 2, post_thinking.tags.size)
+    assert_equal(count + 2, post_thinking.reload.tags.size)
     assert_equal(count + 2, post_thinking.tags(true).size)
 
     assert_nothing_raised { post_thinking.tags.concat(Tag.create!(:name => 'abc'), Tag.create!(:name => 'def')) }
     assert_nil( wrong = post_thinking.tags.detect { |t| t.class != Tag },
                 message = "Expected a Tag in tags collection, got #{wrong.class}.")
     assert_nil( wrong = post_thinking.taggings.detect { |t| t.class != Tagging },
                 message = "Expected a Tagging in taggings collection, got #{wrong.class}.")
-    assert_equal(count + 4, post_thinking.tags.size)
+    assert_equal(count + 4, post_thinking.reload.tags.size)
     assert_equal(count + 4, post_thinking.tags(true).size)
 
     # Raises if the wrong reflection name is used to set the Edge belongs_to
@@ -554,34 +554,35 @@ def test_delete_associate_when_deleting_from_has_many_through_with_nonstandard_i
 
   def test_delete_associate_when_deleting_from_has_many_through
     count = posts(:thinking).tags.count
-    tags_before = posts(:thinking).tags
+    tags_before = posts(:thinking).tags.sort
     tag = Tag.create!(:name => 'doomed')
     post_thinking = posts(:thinking)
     post_thinking.tags << tag
     assert_equal(count + 1, post_thinking.taggings(true).size)
-    assert_equal(count + 1, post_thinking.tags(true).size)
+    assert_equal(count + 1, post_thinking.reload.tags(true).size)
+    assert_not_equal(tags_before, post_thinking.tags.sort)
 
     assert_nothing_raised { post_thinking.tags.delete(tag) }
     assert_equal(count, post_thinking.tags.size)
     assert_equal(count, post_thinking.tags(true).size)
     assert_equal(count, post_thinking.taggings(true).size)
-    assert_equal(tags_before.sort, post_thinking.tags.sort)
+    assert_equal(tags_before, post_thinking.tags.sort)
   end
 
   def test_delete_associate_when_deleting_from_has_many_through_with_multiple_tags
     count = posts(:thinking).tags.count
-    tags_before = posts(:thinking).tags
+    tags_before = posts(:thinking).tags.sort
     doomed = Tag.create!(:name => 'doomed')
     doomed2 = Tag.create!(:name => 'doomed2')
     quaked = Tag.create!(:name => 'quaked')
     post_thinking = posts(:thinking)
     post_thinking.tags << doomed << doomed2
-    assert_equal(count + 2, post_thinking.tags(true).size)
+    assert_equal(count + 2, post_thinking.reload.tags(true).size)
 
     assert_nothing_raised { post_thinking.tags.delete(doomed, doomed2, quaked) }
     assert_equal(count, post_thinking.tags.size)
     assert_equal(count, post_thinking.tags(true).size)
-    assert_equal(tags_before.sort, post_thinking.tags.sort)
+    assert_equal(tags_before, post_thinking.tags.sort)
   end
 
   def test_deleting_junk_from_has_many_through_should_raise_type_mismatch

activerecord/test/cases/connection_adapters/schema_cache_test.rb

@@ -45,8 +45,8 @@ def test_dump_and_load
 
         @cache = Marshal.load(Marshal.dump(@cache))
 
-        assert_equal 12, @cache.columns('posts').size
-        assert_equal 12, @cache.columns_hash('posts').size
+        assert_equal 11, @cache.columns('posts').size
+        assert_equal 11, @cache.columns_hash('posts').size
         assert @cache.tables('posts')
         assert_equal 'id', @cache.primary_keys('posts')
       end

activerecord/test/cases/finder_test.rb

@@ -144,8 +144,8 @@ def test_exists_with_distinct_association_includes_and_limit
 
   def test_exists_with_distinct_association_includes_limit_and_order
     author = Author.first
-    assert_equal false, author.unique_categorized_posts.includes(:special_comments).order('comments.taggings_count DESC').limit(0).exists?
-    assert_equal true, author.unique_categorized_posts.includes(:special_comments).order('comments.taggings_count DESC').limit(1).exists?
+    assert_equal false, author.unique_categorized_posts.includes(:special_comments).order('comments.tags_count DESC').limit(0).exists?
+    assert_equal true, author.unique_categorized_posts.includes(:special_comments).order('comments.tags_count DESC').limit(1).exists?
   end
 
   def test_exists_with_empty_table_and_no_args_given

activerecord/test/fixtures/posts.yml

@@ -4,7 +4,6 @@ welcome:
   title: Welcome to the weblog
   body: Such a lovely day
   comments_count: 2
-  taggings_count: 1
   tags_count: 1
   type: Post
 
@@ -14,7 +13,6 @@ thinking:
   title: So I was thinking
   body: Like I hopefully always am
   comments_count: 1
-  taggings_count: 1
   tags_count: 1
   type: SpecialPost
 

activerecord/test/models/post.rb

@@ -88,7 +88,7 @@ def greeting
   has_and_belongs_to_many :categories
   has_and_belongs_to_many :special_categories, :join_table => "categories_posts", :association_foreign_key => 'category_id'
 
-  has_many :taggings, :as => :taggable
+  has_many :taggings, :as => :taggable, :counter_cache => :tags_count
   has_many :tags, :through => :taggings do
     def add_joins_and_select
       select('tags.*, authors.id as author_id')

activerecord/test/models/tagging.rb

@@ -8,6 +8,6 @@ class Tagging < ActiveRecord::Base
   belongs_to :invalid_tag, :class_name => 'Tag', :foreign_key => 'tag_id'
   belongs_to :blue_tag, -> { where :tags => { :name => 'Blue' } }, :class_name => 'Tag', :foreign_key => :tag_id
   belongs_to :tag_with_primary_key, :class_name => 'Tag', :foreign_key => :tag_id, :primary_key => :custom_primary_key
-  belongs_to :taggable, :polymorphic => true, :counter_cache => true
+  belongs_to :taggable, :polymorphic => true, :counter_cache => :tags_count
   has_many :things, :through => :taggable
 end

activerecord/test/schema/schema.rb

@@ -185,7 +185,7 @@ def except(adapter_names_to_exclude)
       t.text    :body, null: false
     end
     t.string  :type
-    t.integer :taggings_count, default: 0
+    t.integer :tags_count, default: 0
     t.integer :children_count, default: 0
     t.integer :parent_id
     t.references :author, polymorphic: true
@@ -564,7 +564,6 @@ def except(adapter_names_to_exclude)
     end
     t.string  :type
     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