Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ActiveRecord updates counters when it shouldn't #41250

Closed
milani opened this issue Jan 27, 2021 · 11 comments · Fixed by #48665
Closed

ActiveRecord updates counters when it shouldn't #41250

milani opened this issue Jan 27, 2021 · 11 comments · Fixed by #48665

Comments

@milani
Copy link

milani commented Jan 27, 2021

Steps to reproduce

Here is a test case:

# frozen_string_literal: true

require 'bundler/inline'

gemfile(true) do
  source 'https://rubygems.org'

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem 'rails', github: 'rails/rails', branch: 'main'
  gem 'sqlite3'
end

require 'active_record'
require 'minitest/autorun'
require 'logger'

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
    t.integer :comments_count, default: 0
  end

  create_table :users, force: true do |t|
    t.integer :comments_count, default: 0
  end

  create_table :comments, force: true do |t|
    t.integer :user_id
  end

  create_table :comment_maps, force: true do |t|
    t.integer :comment_id
    t.integer :commentable_id
    t.string :commentable_type
  end
end

class User < ActiveRecord::Base
end

class CommentMap < ActiveRecord::Base
  belongs_to :comment
  belongs_to :commentable, polymorphic: true
end

class Comment < ActiveRecord::Base
  belongs_to :user, counter_cache: true
  has_many :comment_maps, dependent: :destroy
  has_many :commented_posts, through: :comment_maps, source: :commentable, source_type: 'Post'
end

class Post < ActiveRecord::Base
  has_many :comment_maps, as: :commentable
  has_many :comments, through: :comment_maps
end

class BugTest < Minitest::Test
  def test_counter
    post = Post.create!
    post.comments << Comment.create!

    assert_equal 0, post.comments_count
  end
end

Expected Behaviour

The test should pass. Since there is no counter_cache defined for the comments-posts relation, I don't expect its counters get the updates.

Actual Behaviour

The test fails:

Failure:
BugTest#test_counter [test.rb:66]:
Expected: 0
  Actual: 1

The Cause

It is happening because a belongs_to relation is defined for the User model. The root of the problem is here:

def inverse_which_updates_counter_cache
return @inverse_which_updates_counter_cache if defined?(@inverse_which_updates_counter_cache)
@inverse_which_updates_counter_cache = klass.reflect_on_all_associations(:belongs_to).find do |inverse|
inverse.counter_cache_column == counter_cache_column
end
end

It needs a more strict condition to determine the inverse relation.

@intrip
Copy link
Contributor

intrip commented Jan 27, 2021

@milani are you panning to open a PR to fix this? Otherwise I'd give it a try.

@milani
Copy link
Author

milani commented Jan 27, 2021

@intrip not really. I am not sure about the best conditions to add there.

@intrip
Copy link
Contributor

intrip commented Jan 27, 2021

Allright, I'll take a look 🙂

@intrip
Copy link
Contributor

intrip commented Feb 9, 2021

@milani #41321 should fix this. @kamipo mind taking a look 🙏 ?

@rails-bot
Copy link

rails-bot bot commented Aug 2, 2021

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 6-1-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot added the stale label Aug 2, 2021
@intrip
Copy link
Contributor

intrip commented Aug 2, 2021

Attached PR. Still relevant

@rails-bot rails-bot bot removed the stale label Aug 2, 2021
intrip added a commit to intrip/rails that referenced this issue Aug 9, 2021
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 improving the inverse relation detection in 2 ways:
- by comparing `inverse.klass` with the current
  `active_record` (doesn't work for polymorphic)
- by relying on `inverse_of`

The above adjustments allows to correctly map the
`inverse_which_updates_counter_cache` for both non-polymorphic/polymorphic
relations.

Fixes rails#41250
@rails-bot
Copy link

rails-bot bot commented Oct 31, 2021

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 6-1-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot added the stale label Oct 31, 2021
@intrip
Copy link
Contributor

intrip commented Oct 31, 2021

Attached PR. Still relevant

@rails-bot rails-bot bot removed the stale label Oct 31, 2021
@milani
Copy link
Author

milani commented Oct 31, 2021

Can someone take a look at the PR and review it?

@rails-bot
Copy link

rails-bot bot commented Jan 29, 2022

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 7-0-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot added the stale label Jan 29, 2022
@rails-bot rails-bot bot closed this as completed Feb 5, 2022
@intrip
Copy link
Contributor

intrip commented Feb 7, 2022

not stale, will update the PR in the next days.

@eugeneius eugeneius reopened this Feb 7, 2022
@rails-bot rails-bot bot removed the stale label Feb 7, 2022
casperisfine pushed a commit to Shopify/rails that referenced this issue Jul 13, 2023
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants