Skip to content

Commit

Permalink
Fix counter_cache create/concat with overlapping counter_cache_column
Browse files Browse the repository at this point in the history
Fix when multiple `belongs_to` maps to the same counter_cache column.
In such situation `inverse_which_updates_counter_cache` may find the
wrong relation which leads into an invalid increment of the
counter_cache.

This is done by releying on the `inverse_of` property of the relation
as well as comparing the models the association points two.

Note however that this second check doesn't work for polymorphic
associations.

Fixes rails#41250

Co-Authored-By: Jean Boussier <jean.boussier@gmail.com>
  • Loading branch information
intrip and byroot committed Jul 13, 2023
1 parent f043f74 commit 186474f
Show file tree
Hide file tree
Showing 5 changed files with 60 additions and 3 deletions.
7 changes: 7 additions & 0 deletions 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:
Expand Down
7 changes: 5 additions & 2 deletions activerecord/lib/active_record/reflection.rb
Expand Up @@ -275,8 +275,11 @@ def check_validity_of_inverse!
# 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
Expand Down
18 changes: 18 additions & 0 deletions activerecord/test/cases/associations/has_many_associations_test.rb
Expand Up @@ -43,6 +43,7 @@
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
Expand Down Expand Up @@ -1403,6 +1404,23 @@ def test_counter_cache_updates_in_memory_after_update_with_inverse_of_disabled
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")

Expand Down
15 changes: 15 additions & 0 deletions 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
16 changes: 15 additions & 1 deletion activerecord/test/schema/schema.rb
Expand Up @@ -376,8 +376,14 @@
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
Expand Down Expand Up @@ -981,6 +987,10 @@
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
Expand Down Expand Up @@ -1419,6 +1429,10 @@
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
Expand Down

0 comments on commit 186474f

Please sign in to comment.