Merge pull request #48665 from Shopify/fix-update-counter-cache-same-column-name

Fix counter_cache create/concat with overlapping counter_cache_column

activerecord/CHANGELOG.md

@@ -1,3 +1,10 @@
+*   Fix incrementation of in memory counter caches when associations overlap
+
+    When two associations had a similarly named counter cache column, Active Record
+    could sometime increment the wrong one.
+
+    *Jacopo Beschi*, *Jean Boussier*
+
 *   Don't show secrets for Active Record's `Cipher::Aes256Gcm#inspect`.
 
     Before:

activerecord/lib/active_record/reflection.rb

@@ -275,8 +275,11 @@ module ActiveRecord
       # Hence this method.
       def inverse_which_updates_counter_cache
         unless @inverse_which_updates_counter_cache_defined
-          @inverse_which_updates_counter_cache = klass.reflect_on_all_associations(:belongs_to).find do |inverse|
-            inverse.counter_cache_column == counter_cache_column
+          if counter_cache_column
+            inverse_candidates = inverse_of ? [inverse_of] : klass.reflect_on_all_associations(:belongs_to)
+            @inverse_which_updates_counter_cache = inverse_candidates.find do |inverse|
+              inverse.counter_cache_column == counter_cache_column && (inverse.polymorphic? || inverse.klass == active_record)
+            end
           end
           @inverse_which_updates_counter_cache_defined = true
         end

activerecord/test/cases/associations/has_many_associations_test.rb

@@ -43,6 +43,7 @@ require "models/interest"
 require "models/human"
 require "models/sharded"
 require "models/cpk"
+require "models/comment_overlapping_counter_cache"
 
 class HasManyAssociationsTestForReorderWithJoinDependency < ActiveRecord::TestCase
   fixtures :authors, :author_addresses, :posts, :comments
@@ -1403,6 +1404,23 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
     assert_equal 2, topic.reload.replies_count
   end
 
+  def test_counter_cache_updates_in_memory_after_create_with_overlapping_counter_cache_columns
+    user = UserCommentsCount.create!
+    post = PostCommentsCount.create!
+
+    assert_difference "user.comments_count", +1 do
+      assert_no_difference "post.comments_count" do
+        post.comments << CommentOverlappingCounterCache.create!(user_comments_count: user)
+      end
+    end
+
+    assert_difference "user.comments_count", +1 do
+      assert_no_difference "post.comments_count" do
+        user.comments << CommentOverlappingCounterCache.create!(post_comments_count: post)
+      end
+    end
+  end
+
   def test_counter_cache_updates_in_memory_after_update_with_inverse_of_enabled
     category = Category.create!(name: "Counter Cache")
 

activerecord/test/models/comment_overlapping_counter_cache.rb

@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+class CommentOverlappingCounterCache < ActiveRecord::Base
+  belongs_to :user_comments_count, counter_cache: :comments_count
+  belongs_to :post_comments_count, class_name: "PostCommentsCount"
+  belongs_to :commentable, polymorphic: true, counter_cache: :comments_count
+end
+
+class UserCommentsCount < ActiveRecord::Base
+  has_many :comments, as: :commentable, class_name: "CommentOverlappingCounterCache"
+end
+
+class PostCommentsCount < ActiveRecord::Base
+  has_many :comments, class_name: "CommentOverlappingCounterCache"
+end

activerecord/test/schema/schema.rb

@@ -376,8 +376,14 @@ ActiveRecord::Schema.define do
     t.integer :company
   end
 
+  create_table :comment_overlapping_counter_caches, force: true do |t|
+    t.integer :user_comments_count_id
+    t.integer :post_comments_count_id
+    t.references :commentable, polymorphic: true, index: false
+  end
+
   create_table :companies, force: true do |t|
-    t.string  :type
+    t.string :type
     t.references :firm, index: false
     t.string  :firm_name
     t.string  :name
@@ -981,6 +987,10 @@ ActiveRecord::Schema.define do
     t.string :author_id
   end
 
+  create_table :post_comments_counts, force: true do |t|
+    t.integer :comments_count, default: 0
+  end
+
   create_table :serialized_posts, force: true do |t|
     t.integer :author_id
     t.string :title, null: false
@@ -1419,6 +1429,10 @@ ActiveRecord::Schema.define do
     t.timestamps null: true
   end
 
+  create_table :user_comments_counts, force: true do |t|
+    t.integer :comments_count, default: 0
+  end
+
   create_table :test_with_keyword_column_name, force: true do |t|
     t.string :desc
   end