Update select_all for Rails 7.1
#50
Conversation
|
|
||
| if Gem::Version.new(ActiveRecord.version) < Gem::Version.new('5.1') |
There was a problem hiding this comment.
replica_pools/replica_pools.gemspec
Line 25 in bf9a238
We don't support versions of activerecord below 5.1, so no need for this conditional anymore.
| def select_all(*args, **kwargs) | ||
| relation, name, raw_binds = args | ||
|
|
||
| if !query_cache_enabled || locked?(relation) | ||
| return route_to(current, :select_all, *args, **kwargs) | ||
| def select_all(arel, name = nil, binds = [], *args, preparable: nil, **kwargs) | ||
| if !query_cache_enabled || locked?(arel) |
There was a problem hiding this comment.
I moved relation, name, and raw_binds into the method signature as arel, name, and binds because I thought it made the method much easier to understand / read, and also easier to compare with the method we are imitating from activerecord.
a-lavis
requested review from
corincerami,
talaris,
amancevice,
warrenwegs,
iambchan and
tedPen
| end | ||
|
|
||
| # duplicate binds_from_relation behavior introduced in 4.2. | ||
| if raw_binds.blank? && relation.is_a?(ActiveRecord::Relation) | ||
| arel, binds = relation.arel, relation.bind_values |
There was a problem hiding this comment.
This was the line that was erroring. #bind_values hasn't existed since Rails 5.2 (see rails/rails@213796f), which means this line of code hasn't been ran in a long time. With the upgrade to Rails 7.1, we are running into this line of code again.
The reason this line of code is now running in Rails 7.1 is because the #ids method was redefined: rails/rails@8fd3369#diff-d736385b70592e7f46357f56e80d3fcf8baddf3f12b6725eb133554f66d4691cL325-R346.
An easy way to test this is by running the test test/serializers/comment_serializer_test.rb, which calls this line of code which uses #ids: https://github.com/kickstarter/kickstarter/blob/12bfc7a58a524f7ec200e6cb75f1a59ad96d22d9/app/models/comment.rb#L203
|
|
||
| if !query_cache_enabled || locked?(relation) | ||
| return route_to(current, :select_all, *args, **kwargs) | ||
| def select_all(arel, name = nil, binds = [], *args, preparable: nil, **kwargs) |
There was a problem hiding this comment.
I added preparable: because it is passed into to_sql_and_binds.
| describe '.ids regression test' do | ||
| it 'should work with query caching' do | ||
| TestModel.connection.enable_query_cache! | ||
| expect(TestModel.ids.count).to eql TestModel.all.count | ||
| end | ||
|
|
||
| it 'should work if query cache is not enabled' do | ||
| TestModel.connection.disable_query_cache! | ||
| expect(TestModel.ids.count).to eql TestModel.all.count | ||
| end | ||
| end |
There was a problem hiding this comment.
Here's these tests failing without the corresponding functional changes:
https://github.com/kickstarter/replica_pools/actions/runs/7465077872/job/20313460149
In order to update to Rails 7.1, we need to accomodate the changes in rails/rails@213796f, which was released as part of Rails 5.2.
Many of the methods that were being used here either no longer exist or have different signatures. I think its just coincidence that we haven't seen errors with this yet.
Here's the method we're imitating in 7.0.8:
https://github.com/rails/rails/blob/fc734f28e65ef8829a1a939ee6702c1f349a1d5a/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb#L96-L112
See
https://adhoc2.dev.kickstarter.com/adhoc2 is now being used for something else, but if you'd like to try it out yourself feel free to deploy https://github.com/kickstarter/kickstarter/pull/25976 to an adhoc env!With these changes, Kickstarter CI passes:
7.0.8 (https://github.com/kickstarter/kickstarter/pull/25976): https://app.circleci.com/pipelines/github/kickstarter/kickstarter/58295/workflows/875aa7ce-9968-4735-ab4f-ad78b9c66084