THREESCALE-12077: Upgrade to Rails 7.2 #575
Conversation
Result of running `bundle update rails`
These are part of ruby but won't be after ruby 3.4.
| gem "mutex_m", "~> 0.3.0" | ||
| gem "csv", "~> 3.3" |
There was a problem hiding this comment.
These gems were part of Ruby in previous versions but they won't be for ruby 3.4+. I'm adding them to the gemfile to remove a couple of warnings:
/home/jlledom/.asdf/installs/ruby/3.3.1/lib/ruby/gems/3.3.0/gems/bootsnap-1.16.0/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:32: warning: /home/jlledom/.asdf/installs/ruby/3.3.1/lib/ruby/3.3.0/mutex_m.rb was loaded from the standard library, but will no longer be part of the default gems since Ruby 3.4.0. Add mutex_m to your Gemfile or gemspec.
/home/jlledom/.asdf/installs/ruby/3.3.1/lib/ruby/gems/3.3.0/gems/bootsnap-1.16.0/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:32: warning: /home/jlledom/.asdf/installs/ruby/3.3.1/lib/ruby/3.3.0/csv.rb was loaded from the standard library, but will no longer be part of the default gems since Ruby 3.4.0. Add csv to your Gemfile or gemspec. Also contact author of license_finder-7.0.1 to add csv into its gemspec.
| concurrent-ruby (1.3.5) | ||
| connection_pool (2.5.4) | ||
| concurrent-ruby (1.3.6) | ||
| connection_pool (3.0.2) |
There was a problem hiding this comment.
Some gems I updated switched to new major versions. From the top of my head they were:
- minitest
- connection_pool
- rdoc
- erb
For all of them, I looked for breaking changes and I found most of them released new majors because they raised their minimal ruby version to 3.2. I didn't see anything really breaking for us.
The only one I couldn't update was minitest, because the 6.x branch doesn't support rails < 8.1. This is the issue I hit: minitest/minitest#1059
So we are trapped in minitest 5.x until we upgrade Rails to 8.1
| has_many :integration_states, dependent: :destroy | ||
|
|
||
| enum state: %i[active disabled].index_with{ |status| status.to_s } | ||
| enum :state, %i[active disabled].index_with{ |status| status.to_s } |
bin/rubocop
Outdated
| # explicit rubocop config increases performance slightly while avoiding config confusion. | ||
| ARGV.unshift("--config", File.expand_path("../.rubocop.yml", __dir__)) | ||
|
|
||
| load Gem.bin_path("rubocop", "rubocop") |
There was a problem hiding this comment.
This, and a few other files in the PR were created by the update task, see the commit: cd07041
There was a problem hiding this comment.
I don't lkike it but as you decide
| # Raise error when a before_action's only/except options reference missing actions. | ||
| config.action_controller.raise_on_missing_callback_actions = true | ||
|
|
||
| config.active_job.queue_adapter = :test |
There was a problem hiding this comment.
This is required from now on:
| # backends that use the same database as Active Record as a queue, hence they | ||
| # don't need this feature. | ||
| #++ | ||
| Rails.application.config.active_job.enqueue_after_transaction_commit = :always |
There was a problem hiding this comment.
This setting changed several times along different rails verions. I explain this in my comment at #560 (comment)
For Rails 7.2 in particular, the value must be :always. Otherwise it breaks Que: que-rb/que#430
There was a problem hiding this comment.
maybe leave a note in a comment about the setting. Unless you plan to upgrade us all the way to 8.1 in which case I'm fine :)
| # Any libraries that use a connection pool or another resource pool should | ||
| # be configured to provide at least as many connections as the number of | ||
| # threads. This includes Active Record's `pool` parameter in `database.yml`. | ||
| max_threads_count = ENV.fetch("RAILS_MAX_THREADS") { 3 } |
There was a problem hiding this comment.
I guess we want the default here. It changed to 3 in Rails 7.2: https://guides.rubyonrails.org/v7.2/7_2_release_notes.html#set-a-new-default-for-the-puma-thread-count
|
|
||
| execute do |connection| | ||
| connection.select_all(relation.to_sql) | ||
| connection.select_all(relation) |
There was a problem hiding this comment.
For some reason, to_sql doesn't bind values in Rails 7.2, which generates SQL conditions like error_count = $1. This fixes it.
There was a problem hiding this comment.
I thought it was since rails 8 :(
| def with_que_adapter(&block) | ||
| ApplicationJob.stub(:queue_adapter, ActiveJob::QueueAdapters::QueAdapter.new, &block) | ||
| end |
There was a problem hiding this comment.
All tests run now using the default :test queue adapter I set in the environments/test.rb file. However, this particular test suite actually requires using the Que adapter, Claude suggested this.
| class ModelTest < ActiveSupport::TestCase | ||
| self.use_transactional_tests = false | ||
|
|
||
| def test_weak_lock |
There was a problem hiding this comment.
The original idea for this test was simulating a race condition and ensure the corresponding exception is raised. But this test had a few problems:
- Instead of simulating different threads belonging to different requests, it used Fibers within a single thread.
- It was using a stub that didn't work because the stub was set only when the fiber was created, but remove just before the fiber starts to run.
- The test tries to simulate an unfinished transaction, but the whole test itself ran inside a parent transaction.
- It relied on stubbing some rails internals, but those internals changed in Deprecate
ConnectionPool#connectionrails/rails#51230
I rewrote it in a way it more closely reproduces reality.
jlledom
changed the title
|
I think, to make the PR complete, we need to delete the |
Yes, I left the file to discuss about the particular settings. But my intention is to remove it before merging. |
| # The events in the two threads must happen in a particular order: | ||
| # 1. The new thread must lock the record and wait. This way the lock | ||
| # and the transaction are kept open. | ||
| # 2. The main thread must wait until the lock is taken, only then it can |
There was a problem hiding this comment.
a nitpick if I understand your intention correctly
| # 2. The main thread must wait until the lock is taken, only then it can | |
| # 2. The main thread must wait until the lock is released, only then it can |
akostadinov
left a comment
akostadinov
left a comment
There was a problem hiding this comment.
Awesome! A few nitpicks. Also agree with Daria that we want there new_framework_defaults files gone :)
|
@akostadinov I applied your suggestions I also set |
We already have another PR opened by DepFu (#560) which upgrades Rails to 8.0. I started working on that and I think it mostly done.
However, there are safety concerns about jumping from 7.1 to 8.0 bypassing 7.2. So I created a new branch which upgrades to Rails 7.2. We can merge this PR first and then #560. IMO #560 is still valid, but we can leave it aside for now.
Most of the changes in this PR are the same as in 8.0 upgrade, the only relevant difference here is I ran
bundle udpate railswhich updates more gems than the strictly rails ones. I think it's always good to update deps.I'll add comments to the code directly, to explain all changes. Most of them are the same as in #560, since this PR is kinda a subset of that one.
Jira Issue: https://issues.redhat.com/browse/THREESCALE-12077