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.

16 files changed, 93 insertions, 36 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 639dbe7df1..c3ded8d4a9 100644
--- a/activerecord/CHANGELOG.md
+++ b/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.
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index 477888228d..453615ba87 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/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
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 af38f2f6dd..007e3bc555 100644
--- a/activerecord/lib/active_record/associations/has_many_through_association.rb
+++ b/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
diff --git a/activerecord/test/cases/associations/belongs_to_associations_test.rb b/activerecord/test/cases/associations/belongs_to_associations_test.rb
index 9c92dc1141..d8d8bbf75e 100644
--- a/activerecord/test/cases/associations/belongs_to_associations_test.rb
+++ b/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
diff --git a/activerecord/test/cases/associations/cascaded_eager_loading_test.rb b/activerecord/test/cases/associations/cascaded_eager_loading_test.rb
index 71c0609df5..51d8e0523e 100644
--- a/activerecord/test/cases/associations/cascaded_eager_loading_test.rb
+++ b/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
diff --git a/activerecord/test/cases/associations/deprecated_counter_cache_on_has_many_through_test.rb b/activerecord/test/cases/associations/deprecated_counter_cache_on_has_many_through_test.rb
new file mode 100644
index 0000000000..48f7ddbe83
--- /dev/null
+++ b/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
diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb
index 3c0b735607..3e5b7e655b 100644
--- a/activerecord/test/cases/associations/has_many_associations_test.rb
+++ b/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)
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 0fa34e829e..a85e020f0c 100644
--- a/activerecord/test/cases/associations/has_many_through_associations_test.rb
+++ b/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
diff --git a/activerecord/test/cases/associations/join_model_test.rb b/activerecord/test/cases/associations/join_model_test.rb
index aabeea025f..cace7ba142 100644
--- a/activerecord/test/cases/associations/join_model_test.rb
+++ b/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
diff --git a/activerecord/test/cases/connection_adapters/schema_cache_test.rb b/activerecord/test/cases/connection_adapters/schema_cache_test.rb
index ecad7c942f..c7531f5418 100644
--- a/activerecord/test/cases/connection_adapters/schema_cache_test.rb
+++ b/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
diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb
index bd77c412f6..40e51a0cdc 100644
--- a/activerecord/test/cases/finder_test.rb
+++ b/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
diff --git a/activerecord/test/fixtures/posts.yml b/activerecord/test/fixtures/posts.yml
index 7298096025..86d46f753a 100644
--- a/activerecord/test/fixtures/posts.yml
+++ b/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
 
diff --git a/activerecord/test/models/post.rb b/activerecord/test/models/post.rb
index 5f01ab0a82..a29858213b 100644
--- a/activerecord/test/models/post.rb
+++ b/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')
diff --git a/activerecord/test/models/tagging.rb b/activerecord/test/models/tagging.rb
index f91f2ad2e9..a6c05da26a 100644
--- a/activerecord/test/models/tagging.rb
+++ b/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
diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb
index fd85050dd4..6ce82c71a8 100644
--- a/activerecord/test/schema/schema.rb
+++ b/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
diff --git a/guides/source/4_2_release_notes.md b/guides/source/4_2_release_notes.md
index 6a847c3087..dd484ccee2 100644
--- a/guides/source/4_2_release_notes.md
+++ b/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.