Deprecate automatic counter caches on has_many :through

Reliant on https://github.com/rails/rails/pull/15747 but pulled to a
separate PR to reduce noise. `has_many :through` associations have the
undocumented behavior of automatically detecting counter caches.
However, the way in which it does so is inconsistent with counter caches
everywhere else, and doesn't actually work consistently.

As with normal `has_many` associations, the user should specify the
counter cache on the `belongs_to`, if they'd like it updated.

activerecord/CHANGELOG.md

@@ -1,3 +1,8 @@
+*   Deprecate automatic counter caches on `has_many :through`. The behavior was
+    broken and inconsistent.
+
+    *Sean Griffin*
+
 *   `preload` preserves readonly flag for associations.
 
     See #15853.

activerecord/lib/active_record/associations/has_many_association.rb

@@ -80,10 +80,14 @@ module ActiveRecord
         end
 
         def update_counter(difference, reflection = reflection())
+          update_counter_in_database(difference, reflection)
+          update_counter_in_memory(difference, reflection)
+        end
+
+        def update_counter_in_database(difference, reflection = reflection())
           if has_cached_counter?(reflection)
             counter = cached_counter_attribute_name(reflection)
             owner.class.update_counters(owner.id, counter => difference)
-            update_counter_in_memory(difference, reflection)
           end
         end
 
@@ -107,6 +111,10 @@ module ActiveRecord
         # 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,6 +63,15 @@ module ActiveRecord
         end
 
         save_through_record(record)
+        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_in_database(1)
+        end
         record
       end
 
@@ -217,6 +226,11 @@ module ActiveRecord
         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 @@ class BelongsToAssociationsTest < ActiveRecord::TestCase
     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 @@ class CascadedEagerLoadingTest < ActiveRecord::TestCase
 
   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/deprecated_counter_cache_on_has_many_through_test.rb

@@ -0,0 +1,26 @@
+require "cases/helper"
+
+class DeprecatedCounterCacheOnHasManyThroughTest < ActiveRecord::TestCase
+  class Post < ActiveRecord::Base
+    has_many :taggings, as: :taggable
+    has_many :tags, through: :taggings
+  end
+
+  class Tagging < ActiveRecord::Base
+    belongs_to :taggable, polymorphic: true
+    belongs_to :tag
+  end
+
+  class Tag < ActiveRecord::Base
+  end
+
+  test "counter caches are updated in the database if the belongs_to association doesn't specify a counter cache" do
+    post = Post.create!(title: 'Hello', body: 'World!')
+    assert_deprecated { post.tags << Tag.create!(name: 'whatever') }
+
+    assert_equal 1, post.tags.size
+    assert_equal 1, post.tags_count
+    assert_equal 1, post.reload.tags.size
+    assert_equal 1, post.reload.tags_count
+  end
+end

activerecord/test/cases/associations/has_many_associations_test.rb

@@ -36,7 +36,7 @@ class HasManyAssociationsTestForReorderWithJoinDependency < ActiveRecord::TestCa
     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
 
@@ -814,14 +814,14 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
   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)
@@ -830,7 +830,7 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
 
   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 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
     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 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
     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 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
     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 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
     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 @@ class AssociationsJoinModelTest < ActiveRecord::TestCase
   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,7 +489,7 @@ class AssociationsJoinModelTest < ActiveRecord::TestCase
                 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')
@@ -497,7 +497,7 @@ class AssociationsJoinModelTest < ActiveRecord::TestCase
                 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')) }
@@ -505,7 +505,7 @@ class AssociationsJoinModelTest < ActiveRecord::TestCase
                 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 @@ class AssociationsJoinModelTest < ActiveRecord::TestCase
 
   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 @@ module ActiveRecord
 
         @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 @@ class FinderTest < ActiveRecord::TestCase
 
   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 @@ class Post < ActiveRecord::Base
   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

@@ -190,7 +190,7 @@ ActiveRecord::Schema.define do
       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
@@ -569,7 +569,6 @@ ActiveRecord::Schema.define do
     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

guides/source/4_2_release_notes.md

@@ -201,6 +201,12 @@ for detailed changes.
 
   ([Commit](https://github.com/rails/rails/commit/91949e48cf41af9f3e4ffba3e5eecf9b0a08bfc3))
 
+* Deprecated broken support for automatic detection of counter caches on
+  `has_many :through` associations. You should instead manually specify the
+  counter cache on the `has_many` and `belongs_to` associations for the through
+  records.
+  ([Pull Request](https://github.com/rails/rails/pull/15754))
+
 ### Notable changes
 
 * Added support for `#pretty_print` in `ActiveRecord::Base` objects.