THREESCALE-12076: Upgrade Rails to 7.2 by mayorova · Pull Request #4232 · 3scale/porta

mayorova · 2026-02-24T13:11:23Z

What this PR does / why we need it:

Upgrade Rails version to v7.2.

Which issue(s) this PR fixes

https://issues.redhat.com/browse/THREESCALE-12076

Verification steps

Special notes for your reviewer:

jlledom

😺

mayorova · 2026-02-24T14:01:37Z

config/environments/production.rb

  config.active_record.dump_schema_after_migration = false

+  # Only use :id for inspections in production.
+  config.active_record.attributes_for_inspect = [ :id ]


This is a new default config.

I am not sure about this one, we need to analyze the code to see if it may cause some issues.

For console usage it's not a problem, because there is #full_inspect method that ignores attributes_for_inspect.
However, if we rely on #inspect somewhere, we might need to comment this out.

We get some error messages which show the #inspect value. Ideally we would go over the objects to define what makes sense per object but I assume we can check the database with the id if needed so it's ok to be like that.

mayorova · 2026-02-24T14:02:18Z

config/puma.rb

+
+# Specify the PID file. Defaults to tmp/pids/server.pid in development.
+# In other environments, only set the PID file if requested.
+pidfile ENV["PIDFILE"] if ENV["PIDFILE"]


This is new, but I think it's harmless, and can actually be useful.

mayorova · 2026-02-24T16:25:47Z

config/puma.rb

+# 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`.
+threads_count = ENV.fetch("RAILS_MAX_THREADS", 5)


The new default is 3, see https://guides.rubyonrails.org/7_2_release_notes.html#set-a-new-default-for-the-puma-thread-count

Maybe we should change our default too? 🤔

I did it on Zync

+1 for the new default. We don't use Puma in production anyway.

mayorova · 2026-02-25T16:44:41Z

test/unit/account_test.rb


      ::Sidekiq::Testing.inline! do
-        provider.destroy!
+        # Prevent exception when deserializing ProviderDomainsChangedEvent with a non-existing (destroyed) provider


I believe this is a global issue, in fact, that happens in production. I opened a Jira for investigating and fixing this: https://issues.redhat.com/browse/THREESCALE-12414

I am not sure why it was not manifesting previously, but I guess it could be because of the following change in Rails 7.2:
https://guides.rubyonrails.org/7_2_release_notes.html#prevent-jobs-from-being-scheduled-within-transactions

Probably, because of the fact that now the jobs are deferred until after the transactions is committed, when the job is being processed, the provider object is no longer in the DB, while previously, I guess the job was executed immediately, while the object was still present.

Your explanation for why it fails makes sense. I wonder what happens when this job doesn't run properly. What is it about? Zync something?

Yes, the Jira description mentions it, but I am not sure.
In theory, if a new account is created, or domains change in the existing accounts, Zync needs to be notified so it creates appropriate routes. However, I don't know if something is supposed to happen, when accounts are deleted.

perhaps routes need to be deleted?

That would be logical

qltysh · 2026-02-26T10:59:43Z

❌ 1 blocking issue (1 total)

Tool	Category	Rule	Count
rubocop	Lint	Class has too many lines. [418/200]	1	❌

mayorova · 2026-02-26T11:34:28Z

app/models/invoice.rb

  CHARGE_PRECISION   = 2

-  enum creation_type: {manual: 'manual', background: 'background'}
+  enum :creation_type, { manual: 'manual', background: 'background' }


DEPRECATION WARNING: Defining enums with keyword arguments is deprecated and will be removed in Rails 8.0. Positional arguments should be used instead: enum :creation_type, {:manual=>"manual", :background=>"background"} (called from <class:Invoice> at /home/dmayorov/Projects/3scale/porta/app/models/invoice.rb:14)

mayorova · 2026-02-26T11:34:46Z

app/models/authentication_provider.rb

  include SystemName

-  enum account_type: {developer: 'developer', provider: 'provider'}
+  enum :account_type, { developer: 'developer', provider: 'provider' }


DEPRECATION WARNING: Defining enums with keyword arguments is deprecated and will be removed in Rails 8.0. Positional arguments should be used instead: enum :account_type, {:developer=>"developer", :provider=>"provider"} (called from <class:AuthenticationProvider> at /home/dmayorov/Projects/3scale/porta/app/models/authentication_provider.rb:13)

mayorova · 2026-02-26T11:36:09Z

test/unit/account_test.rb

    buyer = FactoryBot.create(:simple_buyer)
-    buyer.update(nil)
+    assert_nothing_raised do
+      buyer.update(nil)


Test is missing assertions: `test_update_with_nil_as_param_should_not_raise_error` /home/dmayorov/Projects/3scale/porta/test/unit/account_test.rb:107

so long for rubocop complaining about assert_nothing_raised

According to @akostadinov assert_nothing_raised is useless because if you don't call it, test will pass anyway in the case nothing raises, and tests will fail anyway if something raises.

mayorova · 2026-02-26T11:36:27Z

test/unit/account_test.rb

+    new_org_name = 'account is not readonly'
+    @named_scoped_provider.update(org_name: new_org_name)
+
+    assert_equal new_org_name, @named_scoped_provider.reload.org_name


Test is missing assertions: `test_providers_named_scope_return_non-readonly_objects` /home/dmayorov/Projects/3scale/porta/test/unit/account_test.rb:153

mayorova · 2026-03-26T21:47:00Z

config/environments/test.rb


  config.active_support.test_order = :random

+  config.active_job.queue_adapter = :test


So, this is an important change, see ActiveJob changelog (first bullet point).

We are setting :sidekiq as the queue adapter globally for all environments in config/application.rb.

However, as the issue linked in the Changelog explains, this was not applied to all tests, so there was a mix of some tests running with ActiveJob::QueueAdapters::SidekiqAdapter adapter, and others with ActiveJob::QueueAdapters::TestAdapter, and it's not always clear which tests run with which adapter.

Without setting :test adapter in tests explicitly, many tests that were previously using the TestAdapter started failing, with errors similar to:

assert_enqueued_jobs requires the Active Job test adapter, you're using ActiveJob::QueueAdapters::SidekiqAdapter.

These tests typically use include ActiveJob::TestHelper and some of this helper's methods, like assert_enqueued_jobs, assert_performed_jobs, perform_enqueued_jobs.

So there were several ways to potentially solve this:

Migrate everything to :sidekiq adapter.

Use :test adapter everywhere.

Use a mix of the two.

I opted for 3, even though at first I didn't like the idea of mixing adapters. However, later I thought that it was the lesser evil for the following reasons:

Migrating everything to Sidekiq seemed to be a very big task, given how extended the usage of the ActiveJob::TestHelper is. Also, many of our jobs inherit directly from ApplicationJob (agnostic of the adapter), so it didn't feel to right to make the tests too bound to Sidekiq. Also, the TestHelper's methods seem simpler than the Sidekiq alternatives.
Also, most of the tests don't even care about running jobs, so using the default adapter (which doesn't actually run anything - just puts the enqueued jobs in some internal data structures) seemed reasonable.
It's worth noting though that Sidekiq's :fake (Sidekiq::Testing.fake! ) mode does pretty much the same - just saves the jobs without performing them, so in theory it would also be a suitable solution.

I discarded this option mainly because there are some tests that are Sidekiq-specific (for example, StaleThrottledDeleteTest). So, I thought that using only :test would not allow us to tests some specific features that only Sidekiq has.

So, in the end the approach is to use :test whenever possible, and where needed - explicitly set a different adapter.
So, in the abovementioned StaleThrottledDeleteTest :sidekiq adapter is used, and in some Cucumber tests (in the current code base those labeled with @emails and @search annotations) - :inline adapter (ActiveJob::QueueAdapters::InlineAdapter, also ActiveJob's standard one) is used. Unlike :test adapter, that doesn't perform the jobs, :inline one performs them immediately, which is needed for tests with search and emails (execute indexing jobs or email jobs).
This adapter is also similar to Sidekiq's Sidekiq::Testing.inline! mode.

So, with these hacks applied, all the tests are currently passing. However, I am still open to discuss the possibility of another solution (maybe using Sidekiq everywhere, even though it would mean changing a lot of assertions).

I agree in #3, use the default unless a particular test suite requires the sidekiq adapter.

mayorova · 2026-03-27T09:14:05Z

features/support/email.rb

-  Sidekiq::Job.clear_all
+Before '@emails' do
  ActionMailer::Base.deliveries.clear
+  @original_queue_adapter = ActiveJob::Base.queue_adapter


Now thinking about it, I am not sure what would happen if both @emails and @search annotations are used on a scenario, would the hooks conflict? Probably need to think about it a bit more.

There's a race condition probably:

the first one arriving will set ActiveJob::Base.queue_adapter

the second one will store the altered ActiveJob::Base.queue_adapter in an instance variable

the second one will restore the wrong adapter from its instance variable

You can probably get rid of the instance variable here (and in @search) and just always set ActiveJob::Base.queue_adapter to :inline in the before callback, and to :test in the after callback.

mayorova · 2026-03-27T09:34:17Z

features/step_definitions/finance/invoicing_steps.rb


 Given "an invoice of {buyer} for {date} with items(:)" do |buyer, month, items|
-  ActiveSupport::Deprecation.warn '[Cucumber] Deprecated! Use the newer step.'
+  ActiveSupport.deprecator.warn '[Cucumber] Deprecated! Use the newer step.'


Message:private method `warn' called for class ActiveSupport::Deprecation (NoMethodError) /opt/ci/project/features/support/capybara_extensions.rb:22:in `fill_in' /opt/ci/project/features/support/helpers/session_helpers.rb:8:in `try_buyer_login_internal' /opt/ci/project/features/step_definitions/session_steps.rb:102:in `block in <top (required)>'

jlledom

Happy to see this doesn't require big changes to pass the tests. My only concern is whether the deferred execution of jobs enqueued from inside transactions can break something. Applying logic, it shouldn't happen, in fact we should see less errors now. But we won't know until we actually deploy.

jlledom · 2026-03-30T12:37:01Z

config/environments/test.rb


  config.active_support.test_order = :random

+  config.active_job.queue_adapter = :test


I agree in #3, use the default unless a particular test suite requires the sidekiq adapter.

jlledom · 2026-03-30T12:52:05Z

features/support/email.rb

-  Sidekiq::Job.clear_all
+Before '@emails' do
  ActionMailer::Base.deliveries.clear
+  @original_queue_adapter = ActiveJob::Base.queue_adapter


There's a race condition probably:

the first one arriving will set ActiveJob::Base.queue_adapter

the second one will store the altered ActiveJob::Base.queue_adapter in an instance variable

the second one will restore the wrong adapter from its instance variable

You can probably get rid of the instance variable here (and in @search) and just always set ActiveJob::Base.queue_adapter to :inline in the before callback, and to :test in the after callback.

jlledom · 2026-03-30T13:03:29Z

test/unit/account_test.rb

    buyer = FactoryBot.create(:simple_buyer)
-    buyer.update(nil)
+    assert_nothing_raised do
+      buyer.update(nil)


According to @akostadinov assert_nothing_raised is useless because if you don't call it, test will pass anyway in the case nothing raises, and tests will fail anyway if something raises.

jlledom · 2026-03-30T13:11:04Z

Gemfile


 gem 'acts-as-taggable-on', '~> 11.0'
-gem 'baby_squeel', '~> 3.0', '>= 3.0.0'
+gem 'baby_squeel', '~> 4.0.0'


No breaking changes here?

jlledom previously approved these changes Feb 24, 2026

View reviewed changes

mayorova commented Feb 24, 2026

View reviewed changes

mayorova dismissed jlledom’s stale review via 64eb7d6 February 25, 2026 15:31

mayorova force-pushed the rails72 branch from c1ea815 to 64eb7d6 Compare February 25, 2026 15:31

mayorova commented Feb 25, 2026

View reviewed changes

mayorova force-pushed the rails72 branch from 8d3c549 to e0a1a23 Compare February 25, 2026 22:57

mayorova commented Feb 26, 2026

View reviewed changes

mayorova force-pushed the rails72 branch from b67fcf1 to 8a90d3c Compare February 26, 2026 13:08

mayorova marked this pull request as draft March 2, 2026 13:53

mayorova force-pushed the rails72 branch from 8a90d3c to c55bc46 Compare March 24, 2026 16:36

mayorova changed the base branch from master to strong-params-part3 March 24, 2026 16:36

mayorova force-pushed the rails72 branch from c55bc46 to 53f8b79 Compare March 24, 2026 17:10

mayorova force-pushed the strong-params-part3 branch from cfc3944 to 2f62399 Compare March 25, 2026 09:18

Upgrade rails to 7.2 and dependencies

5f44dcf

mayorova force-pushed the rails72 branch from 0267a74 to 767e0a1 Compare March 25, 2026 14:49

mayorova added 8 commits March 26, 2026 11:35

Upgrade other dependencies

db4e875

rails app:update

f327825

Set the default threads count for Puma to 3

f640c86

Fix unit tests

0641b9c

Fix some deprecations and warnings

fd1d2a2

Fix formatting issues

4afa752

Fix: private method 'warn' called for class ActiveSupport::Deprecation

bde57bb

Use ActiveJob::QueueAdapters::TestAdapter for tests

7bdf655

mayorova force-pushed the rails72 branch from ce05e5b to 7bdf655 Compare March 26, 2026 21:01

mayorova commented Mar 26, 2026

View reviewed changes

mayorova commented Mar 27, 2026

View reviewed changes

jlledom reviewed Mar 30, 2026

View reviewed changes


		config.active_support.test_order = :random

		config.active_job.queue_adapter = :test

Conversation

mayorova commented Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jlledom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlledom Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qltysh bot commented Feb 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mayorova Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlledom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mayorova commented Feb 24, 2026 •

edited

Loading

jlledom Feb 24, 2026 •

edited

Loading

qltysh bot commented Feb 26, 2026 •

edited

Loading

mayorova Mar 27, 2026 •

edited

Loading