THREESCALE-12076: Upgrade Rails to 7.2

[mayorova]

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]

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.

[akostadinov]

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]

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

[mayorova]

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? 🤔

[jlledom]

I did it on Zync

[akostadinov]

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

[mayorova]

Done!

[mayorova]

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.

[akostadinov]

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?

[mayorova]

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.

[akostadinov]

perhaps routes need to be deleted?

[mayorova]

That would be logical

[jlledom]

Not sure about this, if I understood this correctly, then whatever this job does, e.g. tell zync to delete the routes, will stop working from now on, right? Before this PR, this happened when the job wasn't being executed fast enough, which is also wrong, but the incidence was lower.

Maybe for this PR we should rescue from DeserializationError like we do in other jobs.

porta/app/workers/create_service_token_worker.rb

Line 17 in 7efaabd

rescue ActiveRecord::RecordNotFound, ActiveJob::DeserializationError => exception

And report to logs and bugsnag about the error once, instead of leaving the job retry many times to fail every time until it dies. Also this would avoid having to add this supress call to the tests, which shadows a bug in production and makes tests less relevant.

IMO we should work in THREESCALE-12414 as a direct followup for this PR as soon as possible.

[mayorova]

Yeah, I think your suggestion to rescue and report this error is good.

I am not sure though if Rails 7.2 makes it worse 🤔 I can look into this.

[jlledom]

Well, it looks worse, because it only failed when the job was not fast enough, and now it fails always. Maybe the job was never fast enough, that looks plausible to me as well, in that case we are not better than before.

[akostadinov]

Should we file a JIRA to prevent the exception in the Job?

[qltysh]

❌ 5 blocking issues (5 total)

Tool	Category	Rule	Count
rubocop	Lint	Gems should be sorted in an alphabetical order within their section of the Gemfile. Gem mutex\_m should appear before pg.	2	❌
brakeman	Vulnerability	Support for Rails 7.2.3 ends on 2026-08-09.	1	❌
rubocop	Lint	Class has too many lines. [419/200]	1	❌
rubocop	Lint	Assignment Branch Condition size for create\_plan is too high. [<10, 37, 12> 40.16/20]	1	❌

[mayorova]

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]

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]

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

[akostadinov]

so long for rubocop complaining about assert_nothing_raised

[jlledom]

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]

I agree with this, but I think having assert_nothing_raised makes the intention of the test clearer.

[akostadinov]

do not forget - messes up your exception

[akostadinov]

Some other rationale.

http://blog.zenspider.com/blog/2012/01/assert_nothing_tested.html
https://stackoverflow.com/questions/12499259/why-does-minitestspec-have-no-wont-raise-assertion

[mayorova]

do not forget - messes up your exception

@akostadinov can you please clarify?

[mayorova]

I made this change: 9481db1

[akostadinov]

https://thagomizer.com/blog/2013/06/11/assert-nothing-raised.html

[mayorova]

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

[mayorova]

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:

1. Migrate everything to :sidekiq adapter.
2. Use :test adapter everywhere.
3. 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:

1. 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.

2. 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.

3. 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).

[jlledom]

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

[akostadinov]

Same - 3, actually a no brainier to me.

[mayorova]

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.

[jlledom]

There's a race condition probably:

1. the first one arriving will set ActiveJob::Base.queue_adapter
2. the second one will store the altered ActiveJob::Base.queue_adapter in an instance variable
3. 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]

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]

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

[jlledom]

There's a race condition probably:

1. the first one arriving will set ActiveJob::Base.queue_adapter
2. the second one will store the altered ActiveJob::Base.queue_adapter in an instance variable
3. 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]

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]

No breaking changes here?

[mayorova]

It drops support for some Ruby and Rails versions, but it doesn not affect us here:
https://github.com/rzane/baby_squeel/blob/master/CHANGELOG.md#400---2024-11-29

• Remove ActiveRecord boundary
• Added support for ActiveRecord 8.1
• Added support for Ransack 4.4 ([webpack] Do not precompile webpack assets #134)
• Added support for ActiveRecord 8.0
• Added support for ActiveRecord 7.2
• Droped support for ActiveRecord 6.1 and 7.0
• Added Ruby 3.3 and 3.4 to test matrix
• Droped support for Ruby 3.0 and 3.1

[akostadinov]

Yeah, no announced breaking changes, we can fully trust that!

[mayorova]

ActiveSupport::ProxyObject is deprecated and will be removed in the next version: https://github.com/rails/rails/pull/51638/changes

[mayorova]

This is to get rid of the following warnings:

/home/dmayorov/.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/dmayorov/.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/dmayorov/.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/dmayorov/.asdf/installs/ruby/3.3.1/lib/ruby/3.3.0/observer.rb was loaded from the standard library, but will no longer be part of the default gems since Ruby 3.4.0. Add observer to your Gemfile or gemspec. Also contact author of factory_bot-6.2.0 to add observer into its gemspec.

/home/dmayorov/.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/dmayorov/.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.

[akostadinov]

also here. what broke for this to be needed?

[mayorova]

Unofrtunately, I don't know why exactly this happened after the upgrade, and whether it's due to Rails 7.2 upgrade itself, or bumping the audited version (had to be done to add Rails 7.2 support).

But essentially the difference in the behavior is the following:
previously (in master), app_key.save! was causing a single Audit to be created - for ApplicationKey.

Now, in this branch, it actually triggers creating Audit objects for all associated objects created by FactoryBot: ServicePlan, Service, ApplicationPlan, Cinstance, ApplicationKey.

So, assert_difference(Audited.audit_class.method(:count)) do was failing, instead of finding 1 Audit object it was finding 5.

So, the fix here is to first pre-save all of the associated objects, so that ApplicationKey is saved individually.

[akostadinov]

This is important and has to be investigated. I can take alook later

[mayorova]

Another one that needed a fix:
3b40d5a#diff-165b4bb882e8229dcf33da2f971022ef377561da6f8ce0daa23d70fdef5ad2f9

There I changed from

assert_difference(Audited.audit_class.method(:count), +2) do

to

assert_difference(-> { Audited.audit_class.where(auditable_type: 'ProviderConstraints').count }, +2) do

as, I guess, again there were more audits created than previously.

[akostadinov]

If the issue is that Rails started to save associations where it previously didn't, then that is fine. But if audited started to record touch operations or something, then that would be undesirable. I destroyed my VM though and spawning a new one fails very weirdly so couldn't investigate yet.

[akostadinov]

AI FTW!

Root Cause

It's neither "more stuff started to be audited" nor "Rails persists stuff more
consistently." The issue is in the audited gem's storage backend change.

Audited 5.6.0 (changelog PR #702) replaced RequestStore with
ActiveSupport::CurrentAttributes for its internal Audited.store. This store
holds per-class auditing_enabled flags.

In tests, the project disables auditing for every model at load time
(self.disable_auditing if Rails.env.test? in audited_hacks.rb:84). With
RequestStore (audited 5.4), these flags persisted in Thread.current for the
entire test process. With CurrentAttributes (audited 5.6+), they get wiped by
ActiveSupport::CurrentAttributes.reset_all which Rails calls in before_setup
of every test.

The gem's class_auditing_enabled method defaults to true when the key is
missing from the store. So after the reset, ALL audited models appear to have
auditing enabled. When ApplicationKey.with_synchronous_auditing sets the
global flag to true, every model that gets cascade-saved writes an audit
record — producing 5 audits instead of 1.

Fix

Override class_auditing_enabled in the existing AuditedHacks::ClassMethods to
default to false in test environment instead of true:

config/initializers/audited_hacks.rb — added 8 lines after
with_synchronous_auditing:

  private

  # audited 5.6+ replaced RequestStore with ActiveSupport::CurrentAttributes,
  # which resets between tests, losing the disable_auditing calls from model
  # loading. Default to false in tests so only explicitly enabled models audit.
  def class_auditing_enabled
    Audited.store.fetch("#{table_name}_auditing_enabled", !Rails.env.test?)
  end

This means the test changes in the PR (FactoryBot.build →
FactoryBot.create(:cinstance) in application_keys_test.rb) are not needed —
the original test code works correctly with this fix.

[akostadinov]

🐚 cat 0001-fix-audited-class_auditing_enabled-default-for-tests.patch 
From: Claude Opus 4.6 <noreply@anthropic.com>
Subject: [PATCH] fix: audited class_auditing_enabled default for test env

audited 5.6+ replaced RequestStore with ActiveSupport::CurrentAttributes,
which resets between tests (via CurrentAttributes.reset_all in
before_setup). This wipes the per-class disable_auditing flags set during
model loading, causing class_auditing_enabled to default to true for all
models. When with_synchronous_auditing enables the global flag, every
cascade-saved model writes an audit instead of just the target model.

Override class_auditing_enabled to default to false in test environment,
matching the effective behavior under RequestStore (audited <= 5.4).
---
 config/initializers/audited_hacks.rb | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/config/initializers/audited_hacks.rb b/config/initializers/audited_hacks.rb
index 5b08fe82e..c64c348c1 100644
--- a/config/initializers/audited_hacks.rb
+++ b/config/initializers/audited_hacks.rb
@@ -113,6 +113,15 @@ module AuditedHacks
     def with_synchronous_auditing(&block)
       synchronous_audits { with_auditing(&block) }
     end
+
+    private
+
+    # audited 5.6+ replaced RequestStore with ActiveSupport::CurrentAttributes,
+    # which resets between tests, losing the disable_auditing calls from model
+    # loading. Default to false in tests so only explicitly enabled models audit.
+    def class_auditing_enabled
+      Audited.store.fetch("#{table_name}_auditing_enabled", !Rails.env.test?)
+    end
   end

   module InstanceMethods
--
2.49.0

[akostadinov]

Or maybe better this one:

🐚 cat 0001-fix-audited-class_auditing_enabled-default-for-tests.patch 
From: Claude Opus 4.6 <noreply@anthropic.com>
Subject: [PATCH] fix: re-disable auditing after CurrentAttributes reset

audited 5.6+ replaced RequestStore with ActiveSupport::CurrentAttributes
for its internal store. CurrentAttributes resets between tests (via
reset_all in before_setup), wiping the per-class disable_auditing flags
set during model loading. Since class_auditing_enabled defaults to true
when the key is missing, all models become auditable when
with_synchronous_auditing temporarily enables the global flag.

Add a setup hook that re-disables auditing for all audited models after
each CurrentAttributes reset.
---
 test/test_helper.rb | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/test/test_helper.rb b/test/test_helper.rb
index 49d18b3cf..8b95c72b0 100644
--- a/test/test_helper.rb
+++ b/test/test_helper.rb
@@ -68,6 +68,14 @@ class ActiveSupport::TestCase
     assert_not (str =~ Regexp.compile(regexp)), "Should not match '#{regexp}'"
   end

+  # audited 5.6+ uses ActiveSupport::CurrentAttributes which resets between
+  # tests, wiping per-class auditing flags. Re-disable after each reset.
+  setup do
+    Audited.audit_class.audited_class_names.each do |name|
+      name.safe_constantize&.disable_auditing
+    end
+  end
+
   def assert_can(ability, *args)
     assert ability.can?(*args), "User is not able to #{args.join(' ')}"
   end
--
2.49.0

[mayorova]

Nice find @akostadinov thank you!
I'll apply the last suggestion

[akostadinov]

Why do we need to add this tag? I mean, it might be fine but I'm trying to understand if something broke by the update or this is something you wanted to improve.

[mayorova]

Full explanation is in https://github.com/3scale/porta/pull/4232/changes#r2997838267 - under number 3.

So, after switching to config.active_job.queue_adapter = :test, we need to either explicitly perform the scheduled jobs, or, as in this and similar tests - I decided to just label the scenarios with @email.

You can then see in features/support/email.rb that it just clears the queue and sets queue adapter to :inline, to ensure the jobs are executed immediately.

I guess previously sidekiq adapter was implicitly used for these scenarios... In general it was kind of unpredictable (I think) what adapter would be used in each case.

[akostadinov]

Isn't it better to tag it as @background-jobs or something?

[mayorova]

Isn't it better to tag it as @background-jobs or something?

Well, then it would rather be @inline-jobs to reflect what it does exactly.

But I opted for this more generic @email for several reasons:

1. This label was already used in multiple scenarios (apparently, affecting databaase cleaner):

2. I thought that it was easier to understand - you need the emails to get delivered as part of the scenario? just add `@emails` label, without thinking, what's underneath (what queue adapter is used or what setup/teardown extra steps need to be performed for this to work fine). 3. This is similar to `@search` label, which by the way also uses `:inline` adapter now, but it also does more stuff (starting and stopping search etc.)

[akostadinov]

ok, fine, if we use only for email needing tests, makes sense even if implementation is more generic. It carries semantics that can later be useful.

[akostadinov]

Suggested change

	ActiveJob::Base.queue_adapter = @original_queue_adapter
	ActiveJob::Base.queue_adapter = Rails.configuration.active_job.queue_adapter

[mayorova]

Unline in cucumber scenario labels, where saving the original adapter in a variable was error prone (in case multiple labels are used, or there are some other clashes), here the situation is more controlled, so I think it's fine to use it. But I don't mind applying the suggestion 😉

[akostadinov]

Fine either way, but thank you :)

[akostadinov]

Another chunk of amazing work!

[jlledom]

You reverted the suppress call in other tests but forgot this one.

[mayorova]

Yeah, and also, it's weird that I removed surpressing there, and nothing started failing (without other chcanges) 😬
Let's see this one...

[akostadinov]

Why should provider.destroy! result in errors. We perform the hierarchical destruction and it is not async, not so much at least, so we shouldn't hit and shouldn't raise errors. I'd expect this with the new implementation. Whether that can in fact never happen is beyond me though 👼

[mayorova]

Well, now I am not sure, because I can't reproduce this deserialization error anymore 😅

But in this specific test we're not using standard background deletion, which is what I assume we're referring to?

It runs provider.destroy! directly, and I think the problem was here:

before_destroy :publish_domain_events, if: :provider?

in app/models/account/provider_domains.rb.

This event is created and then triggers ProcessDomainEventsWorker which tries to instantiate the provider using its ID from the event data.

Now, I don't know why at some point of working on the PR the deserialization exception started popping up, and why it later went away on its own 🥲

[mayorova]

Support for Rails 7.2 was added in 5.7.0 https://github.com/collectiveidea/audited/blob/main/CHANGELOG.md#570-2024-08-13

[akostadinov]

Thank you very much!