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

Fix rails7 default scope issue with select #582

Closed
wants to merge 2 commits into from

Conversation

borisrorsvort
Copy link
Contributor

We did not investigate too much but after upgrading to rails 7 we had issues when using default_scope { I18n } coupled with active record select method.

This PR seems to fix it but feel free to change and or add tests for other use cases.

This seems to fix issues described in #513 back in the days

@borisrorsvort
Copy link
Contributor Author

Currently this PR causes some issues in our rspec tests:

expected ActiveRecord::RecordNotFound, got #<NoMethodError: undefined method `[]=' for nil:NilClass> with backtrace:
           # /Users/ghost/Sites/mobility/lib/mobility/backends/active_record/pg_hash.rb:26:in `write'
           # /Users/ghost/Sites/mobility/lib/mobility/plugins/cache.rb:78:in `write'
           # /Users/ghost/Sites/mobility/lib/mobility/plugins/writer.rb:29:in `name='
           # /Users/ghost/Sites/mobility/lib/mobility/plugins/locale_accessors.rb:68:in `public_send'
           # /Users/ghost/Sites/mobility/lib/mobility/plugins/locale_accessors.rb:68:in `name_en='

@shioyama
Copy link
Owner

I'd really like to reproduce the issue here before shipping a fix.

There's already a regression spec for #513 that runs on Rails 7:

describe "regression for #513" do
plugins :active_record, :query
before do
m = ActiveRecord::Migration.new
m.verbose = false
m.create_table :cars
stub_const('Car', Class.new(ActiveRecord::Base) do
has_many :parking_lots
has_many :car_parts
end)
translates Car, backend: :column
m.create_table(:parking_lots) { |t| t.integer :car_id }
stub_const('ParkingLot', Class.new(ActiveRecord::Base) do
belongs_to :car
end)
m.create_table(:car_parts) { |t| t.integer :car_id }
stub_const('CarPart', Class.new(ActiveRecord::Base) do
belongs_to :car
end)
translates CarPart, backend: :column
end
after do
m = ActiveRecord::Migration.new
m.verbose = false
m.drop_table :cars
m.drop_table :parking_lots
m.drop_table :car_parts
end
it "does not raise NameError" do
query = ParkingLot.includes(car: :car_parts).references(:car).merge(Car.i18n)
expect { query.first }.not_to raise_error
expect { query.order(:car_id) }.not_to raise_error
end
end

It is passing so I'm not sure what's happening in your case. I don't doubt you're seeing the error but if the spec is not testing the problem correctly I'd like to fix that too.

@shioyama
Copy link
Owner

Also:

default_scope { I18n }

I assume you mean:

default_scope { i18n }

right? (i18n not I18n)

@shioyama
Copy link
Owner

Ah I got it by adding a test with select:

  1) Mobility::Plugins::ActiveRecord::Query regression for #513 does not raise NameError
     Failure/Error: expect { query.select(:name) }.not_to raise_error
     
       expected no Exception, got #<NameError: undefined method `mobility_attribute?' for class `#<Class:ParkingLot(id: integer, car_id: integer)>'> with backtrace:
         # ./lib/mobility/plugins/active_record/query.rb:169:in `method'
         # ./lib/mobility/plugins/active_record/query.rb:169:in `block (2 levels) in <module:QueryExtension>'
         # ./spec/mobility/plugins/active_record/query_spec.rb:195:in `block (4 levels) in <top (required)>'
         # ./spec/mobility/plugins/active_record/query_spec.rb:195:in `block (3 levels) in <top (required)>'

I'll ship this with your fix.

@shioyama
Copy link
Owner

Currently this PR causes some issues in our rspec tests:

That doesn't look related to anything here.

@shioyama
Copy link
Owner

Currently this PR causes some issues in our rspec tests:

This is on the master branch which is no the same as the latest release (1.2.x).

Your tests are probably failing on #536

Try #584 which I'll ship as the latest 1.2.x

@shioyama
Copy link
Owner

A fix for this was released in 1.2, closing this. Thanks for the issue report and fix!

@shioyama shioyama closed this Jun 24, 2022
@borisrorsvort
Copy link
Contributor Author

@shioyama awesome! ❤️

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

Successfully merging this pull request may close these issues.

2 participants