Merge pull request #15754 from sgrif/sg-deprecate-hmt-counter-cache

Deprecate automatic counter caches on has_many :through

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.