THREESCALE-12434: Migrate from protected attributes to strong parameters - Part 3

[mayorova]

What this PR does / why we need it:

This is part 3 (final) of the migration from protected attributes to strong parameters. See also:

• part 1: THREESCALE-12434: Migrate from protected attributes to strong parameters - Part 1 #4248
• part 2: THREESCALE-12434: Migrate from protected attributes to strong parameters - Part 2 #4249

Protected attributes is an old Rails feature which was deprecated a long time ago. We were using protected_attributes_continued gem to keep it working, but now it's also discontinued and does not support Rails 7+, so it's a blocker for upgrading to Rails 7.2 for us.

This part clears up all the remaining references to protected attributes, and removes the protected_attributes_continued gem.

Which issue(s) this PR fixes

https://redhat.atlassian.net/browse/THREESCALE-12434

Verification steps

All tests should pass, and all features should work as before.

Special notes for your reviewer:

I recommend to review commit-by-commit. First three a kind of trivial, the others are tricky and try to solve side-effects caused by removal of the gem.

[qltysh]

❌ 9 blocking issues (9 total)

Tool	Category	Rule	Count
rubocop	Lint	Class has too many lines. [206/200]	5	❌
rubocop	Style	Use anonymous block arguments forwarding (&).	1	❌
rubocop	Lint	Use anonymous block forwarding.	1	❌
rubocop	Lint	Block has too many lines. [118/25]	1	❌
rubocop	Lint	Assignment Branch Condition size for create\_a\_complete\_provider is too high. [<16, 156, 1> 156.8/20]	1	❌

[mayorova]

I am not sure whether this is the right thing to do...

This is basically keeping the old behavior.

The test that is failing without it is:

  def test_fetch_with_retry
    expected = nil
    rule = MailDispatchRule.fetch_with_retry!(account_id: 1, system_operation_id: 1) do |rule|
      expected = rule.dup
      expected.save!
    end

    assert_equal expected , rule
  end

It verifies (how I understand it) that if a race condition occurs when creating MailDispatchRule twiche with the same attributes, .fetch_with_retry! will still successfully create just a single object.

With no code change, the test fails with the error:

Couldn't find MailDispatchRule with [WHERE `mail_dispatch_rules`.`account_id` = ? AND `mail_dispatch_rules`.`system_operation_id` = ? AND `mail_dispatch_rules`.`account_id` = ? AND `mail_dispatch_rules`.`system_operation_id` = ?]
/opt/ci/workdir/vendor/bundle/ruby/3.3.0/gems/activerecord-7.1.5.2/lib/active_record/relation/finder_methods.rb:413:in `raise_record_not_found_exception!'
/opt/ci/workdir/vendor/bundle/ruby/3.3.0/gems/activerecord-7.1.5.2/lib/active_record/relation/finder_methods.rb:127:in `take!'
/opt/ci/workdir/vendor/bundle/ruby/3.3.0/gems/activerecord-7.1.5.2/lib/active_record/relation/finder_methods.rb:110:in `find_by!'
/opt/ci/workdir/vendor/bundle/ruby/3.3.0/gems/activerecord-7.1.5.2/lib/active_record/relation.rb:232:in `rescue in create_or_find_by!'
/opt/ci/workdir/vendor/bundle/ruby/3.3.0/gems/activerecord-7.1.5.2/lib/active_record/relation.rb:228:in `create_or_find_by!'
/opt/ci/workdir/vendor/bundle/ruby/3.3.0/gems/activerecord-7.1.5.2/lib/active_record/relation.rb:183:in `find_or_create_by!'
/opt/ci/workdir/vendor/bundle/ruby/3.3.0/gems/activerecord-7.1.5.2/lib/active_record/querying.rb:23:in `find_or_create_by!'
/opt/ci/workdir/app/models/mail_dispatch_rule.rb:13:in `fetch_with_retry!'
/opt/ci/workdir/test/unit/mail_dispatch_rule_test.rb:7:in `test_fetch_with_retry'

So, with protected_attributes_continued gem, there was this code:

def find_or_create_by!(attributes, options = {}, &block)
  find_by(attributes.respond_to?(:to_unsafe_h) ? attributes.to_unsafe_h : attributes) || create!(attributes, options, &block)
end

With the gem removed, the ActiveRecord method is being called directly:

def find_or_create_by!(attributes, &block)
  find_by(attributes) || create_or_find_by!(attributes, &block)
end

create_or_find_by! wraps the createion in a transaction, so if anything fails within it, all created objects get rolled back.
So, apparently, ActiveRecord::RecordNotUnique is raised, and then find_by! fails with ActiveRecord::RecordNotFound, which is what the test detects.

[mayorova]

So, honestly, my feeling is that it's that the test doesn't represent what would be happening in the production code. If the object is created in two different places, then Rails' standard find_or_create_by! should just handle it.

So, we probably don't need this loop, and the race condition simulation should be done in a different way.

However, I don't want to risk it, because I don't quite understand the possible use case, so I decided to just keep the previous behavior.

[jlledom]

I think MailDispatchRule is dead code after I migrated to the new notification system. I checked and fetch_with_retry! is not called from anywhere in our codebase.

I don't remember all details now, but I think any changes there will not have any effect.

[jlledom]

I found this: https://redhat.atlassian.net/browse/THREESCALE-11825

[mayorova]

Good, thank you for the info!

[mayorova]

This issue with #backend_api just drives me nuts.

This commit contains a solution that Claude provided, and I'm putting his explanation below.

I am not convinced or happy with this though, and I find the whole lazy building messy... I think we need to review the behavior again, and try to get rid of any lazy building, because it just makes things complicated and error-prone.
WDYT @akostadinov @jlledom ?

[mayorova]

BackendApiProxy Cache Issue After Removing protected_attributes_continued

Problem

After removing the protected_attributes_continued gem (strong-params-part3 branch), several tests
started failing with errors like:

• NoMethodError: undefined method 'children' for nil (when service.backend_api returns a stale
  unpersisted BackendApi with no metrics)
• StateMachines::InvalidTransition: Cannot transition state via :approve from :created (Reason(s): Backend apis invalid) (when account validation encounters the unpersisted BackendApi)

Root Cause

The behavioral difference is caused by inverse_of working correctly without
protected_attributes_continued.

How BackendApiProxy caches a backend_api

When a Service is created, after_create :create_default_proxy builds and saves a Proxy. During
Proxy validation, validate_backend_api? (in BackendApiLogic::ProxyExtension) calls
backend_api.private_endpoint_changed?. This delegates through:

Proxy#backend_api -> Service#backend_api -> Service#backend_api_proxy -> BackendApiProxy#backend_api

BackendApiProxy#backend_api does:

@backend_api ||= backend_api_configs.first&.backend_api ||
                 account.backend_apis.build(system_name: ..., name: ..., description: ...)

Since no backend_api_configs exist yet during initial service creation, it falls through to
account.backend_apis.build(...), which creates an unpersisted BackendApi (no private_endpoint,
no metrics) and caches it in @backend_api.

What changed: inverse_of behavior

The Proxy model has belongs_to :service, touch: true, inverse_of: :service and the Service model
has has_one :proxy, dependent: :destroy, inverse_of: :service, autosave: true.

On master (with protected_attributes_continued)

The gem subtly breaks inverse_of for has_one associations. When a Proxy is created via
service.create_proxy!, the Proxy's service method resolves to a different Service instance
(loaded from DB), not the original one.

This means the stale @backend_api_proxy (with its cached unpersisted BackendApi) is set on a
throwaway Service instance that gets garbage collected. The FactoryBot-returned Service instance
has a clean @backend_api_proxy — calling service.backend_api creates a fresh BackendApiProxy
that correctly finds the persisted BackendApi from backend_api_configs.

On the branch (without the gem)

inverse_of works correctly. The Proxy's service method returns the same Service instance
that created it. The stale @backend_api_proxy (with its cached unpersisted BackendApi) is set on
the FactoryBot-returned Service instance.

Later calls to service.backend_api return the stale unpersisted BackendApi instead of the real
persisted one created by the :with_default_backend_api factory trait.

Evidence

Tracing BackendApiProxy#backend_api calls during FactoryBot.create(:simple_service):

Master:

BackendApiProxy SET on Service object_id=32920 (different from FactoryBot's 32940)
After factory create: @backend_api_proxy cached? false

Branch:

BackendApiProxy SET on Service object_id=32600 (same as FactoryBot's 32600)
After factory create: @backend_api_proxy cached? true, @backend_api.persisted? false

Impact on Production Code

The main production code path that uses service.backend_api is ServiceCreator:

def save!(params)
  @service.save!                     # triggers create_default_proxy -> caches stale @backend_api_proxy
  ...
  save_default_backend_api(params)   # calls backend_api_proxy.update!(params)
end

ServiceCreator is not affected because:

1. It intentionally works with the unpersisted BackendApi built during proxy creation
2. backend_api_proxy.update! sets private_endpoint on it and calls save!, persisting it
3. The same cached BackendApiProxy instance is used throughout, so it operates on the same
   BackendApi object

Other production code paths access backend_api through BackendApiConfig associations (not through
the BackendApiProxy cache), so they are also unaffected.

Fix

Code fix: BackendApiProxy#backend_api

Changed to handle stale caches by re-checking backend_api_configs when the cached value is
unpersisted:

def backend_api
  return @backend_api if @backend_api&.persisted?

  @backend_api = backend_api_configs.first&.backend_api ||
                 @backend_api
  @backend_api ||= account.backend_apis.build(...)
end

Logic:

1. Persisted cached BA - return immediately (fast path, same behavior as before)
2. Unpersisted/nil cached BA - check backend_api_configs first (picks up configs created
   after initial cache, e.g. by :with_default_backend_api factory trait)
3. Reuse existing unpersisted BA if no config found (avoids building duplicate unpersisted
   instances via account.backend_apis.build, which would pollute the association)
4. Build new only if nothing exists yet

This is safe for ServiceCreator because step 3 reuses the same unpersisted BA across calls
within update!.

Factory fix: service factories

Both :simple_service and :service factories clean up the account's backend_apis association
in after(:create):

after(:create) do |record|
  record.service_tokens.first_or_create!(value: 'token')
  # Proxy validation during service creation calls account.backend_apis.build(...),
  # which adds an unpersisted BackendApi to the association. Remove it to prevent
  # validation failures in later operations on the account.
  record.account.backend_apis.target.delete_if { |ba| !ba.persisted? }
end

This is needed because account.backend_apis.build(...) (called during proxy validation) adds an
unpersisted BackendApi to the account's loaded association. If the account is later validated (e.g.,
during approve! in the provider signup flow), this unpersisted BA fails validation because it has
no private_endpoint. The code fix in BackendApiProxy#backend_api handles the separate issue of
the stale cache but does not remove the unpersisted BA from the association.

[jlledom]

I don't see it so problematic, maybe a comment might help, but if you tried and it works, I think its fine.

# Lookup priority
# 1. Current backend if persisted
# 2. DB backend if any
# 3. Current backend if not persisted
# 4. New backend
def backend_api
  return @backend_api if @backend_api&.persisted?

  @backend_api = backend_api_configs.first&.backend_api ||
                 @backend_api
  @backend_api ||= account.backend_apis.build(system_name: service_system_name, name: "#{service_name} Backend", description: "Backend of #{service_name}")
end

Maybe a clearer refactor (maybe not):

def backend_api
  return @backend_api if @backend_api&.persisted?
  return backend_api_configs.first&.backend_api if backend_api_configs.first&.backend_api&.present?
  return @backend_api if @backend_api&.present?

  account.backend_apis.build(system_name: service_system_name, name: "#{service_name} Backend", description: "Backend of #{service_name}")
end

[mayorova]

I don't remember how exactly I came up with this replacement, but I think it was just debugging and checking what fields were actually available there.

[jlledom]

According to Claude, I broke this method in this commit: 33b70d3

However, after my changes it was effectively doing the same you do in this commit, just returning [:updated_at, :updated_by, state.to_s].

So I guess:

1. Either versioning was broke and nobody noticed
2. This worked by coincidence

There are no tests for Template#revert_to or Version#revert_attributes. So if I broke something there were no tests to catch the regression. There's a cucumber about template versioning which passed, though.

[mayorova]

Hm... I don't see changes to that file in the commit you mentioned 🤔

[jlledom]

In that commit, I removed the protected attributes for the Template model:

- attr_accessible :provider, :draft, :liquid_enabled, :handler

After that change, template.class.accessible_attributes returned nothing, so the end result was equivalent to [:updated_at, :updated_by, state.to_s]

[mayorova]

I, I see, right, thank you for the explanation!

Yeah, it rings a bell now, indeed, that accessible_attributes was returning an empty array.

[jlledom]

Do you think the versioning feature is working fine? didn't I break it?

[mayorova]

When I tried, it seemed to be doing the right thing.

[jlledom]

A good engineer knows how to make mistakes!

[jlledom]

If tests pass and the template versioning is in fact not broken, then it looks good to me. 👍