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

The counter_cache_column? method is depending of initialisation order. #50768

Open
BuonOmo opened this issue Jan 16, 2024 · 1 comment
Open

Comments

@BuonOmo
Copy link
Contributor

BuonOmo commented Jan 16, 2024

Steps to reproduce

# frozen_string_literal: true

require "bundler/inline"

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

  gem "rails", github: "rails/rails", branch: "main"

  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

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

  create_table :cars, force: true do |t|
    t.belongs_to :person
  end
end

class Car < ActiveRecord::Base
  belongs_to :person, counter_cache: true
end

# If the `Person` class was defined before, the test below would work
class Person < ActiveRecord::Base
  has_many :cars
end

class BugTest < Minitest::Test
  def test_counter_cache_column?
    assert Person.counter_cache_column?("cars_count")
    assert_not Car.counter_cache_column?("cars_count")
  end
end

Expected behavior

the Person class should identify clearly cars_count as a counter cache column

Actual behavior

Since Person is initialized afterwards, when running belong_to, the cars_count column is not added to its counter_cache

System configuration

Rails version: found in 7.1.2, present in edge as well

Ruby version: 3.2.1

Notes

In the actual ActiveRecord test suite, this test could fall if in the future a file requiring first models/car is loaded before the counter_cache_test file.

Also I'd be so glad to work on the solution. It seems to me that the issue is that this expression can return nil. I'm just not sure how it should be solved (by raising, autoloading, warning, ...)

@skipkayhil
Copy link
Member

The inconsistency was introduced in 7b6720d, although I don't think the impact is very high at the moment (and is no worse than before the mentioned commit).

In that commit, counter_cache_column? was basically extracted from attr_readonly so that the counter_cache columns could continue to be updated by Rails but not by a user. So before the mentioned commit, I believe you would see the same behavior, but the inconsistency would be on whether the column was marked attr_readonly.

I started looking into a fix, which I believe needs to come from the other side of the association. I've pushed up a wip branch here: fa34f9a however it appears the tests are failing due to my use of inverse_of raising. I haven't dug into why yet but wanted to dump my thoughts here in the meantime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants