[Fix #50604] Restore compatibility of Active Record Encryption configs with eager loading mode

[maximerety]

Motivation / Background

Fixes #50604

This Pull Request fixes a source of incompatibility between the loading order of Active Record Encryption configs and the loading of AR models in eager loading mode.

Detail

This Pull Request removes an occurrence of after_initialize introduced in #48530, which is not fully compatible with the eager loading mode.

Previous concerns were raised in a discussion from the same PR.

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[rafaelfranca]

This broke a test where the config is set in an intializer. That is because the ActiveRecord::Encryption.configure is called outside of the on_load. We should move it there.

[maximerety]

This broke a test where the config is set in an intializer. That is because the ActiveRecord::Encryption.configure is called outside of the on_load. We should move it there.

@rafaelfranca Thanks for the hint.

I had a hard time adding new tests to demonstrate the issue because, to be honest, there are a lot of possible situations that could still not work as expected.

For example, people may reference ActiveRecord::Base in initializers (this is what is done in the new tests here, and others) and if you do this before setting the desired Rails.application.config.active_record.encryption properties, they will be ignored when configuring ActiveRecord::Encryption. That's why I'm regrouping all configs in a single initializer (config/initializers/active_record.rb) in the tests and declare them in the proper order.

This is the exact problem discussed initially: https://github.com/rails/rails/pull/48530/files#r1237624233.

But this is probably the price we have to pay to ensure that configurations are ready early enough in eager loading mode.

Finally, I'd like to propose a backport of this fix to 7-1-stable as I encountered it while migrating a project from 7.0 to 7.1. Would that be acceptable to you?

[jorgemanrubia]

Good fix 👍 , thanks @maximerety.

I think this case cause trouble when referencing models from initializers, but @rafaelfranca made a good point here about that #48530 (comment). Good to merge from my side 👍 .

Also, thanks for the fantastic stream of improvements on Active Record Encryption @maximerety 🙏.

[maximerety]

Thanks @jorgemanrubia for your kind words 🙇

@rafaelfranca Let me know if you need anything else from me for this PR to be merged and backported to 7-1-stable 🙏

[maximerety]

@rafaelfranca I've rebased and squashed the 2 commits (which I forgot to do initially).

[maximerety]

@jorgemanrubia Too bad I did not manage to have this merged in Rails 7.1.3, I think I'll have to be little more patient 😉 Do you know if I can do something else for this patch to land on main / 7-1-stable so that it'll be ready for a next patch version when that times come? Thanks.

[trevorturk]

@maximerety you might consider pinging the Rails Discord to see if you can get a review?

[jorgemanrubia]

@eileencodes could we merge this one please?

[eileencodes]

Can someone do the backport PR? It doesn't cherry-pick cleanly and I don't have the bandwidth to fight conflicts right now. Thanks!

[maximerety]

Thanks @eileencodes, here is the backport PR to 7-1-stable: #51061.

[tenpaiyomi]

@jorgemanrubia @eileencodes I don't suppose either of you have an idea of when this will actually make it into a release, do you? I saw that the backport to 7-1-stable, #51061, got merged in about 4 months ago, and there seems to have been 3 separate 7.1.x releases since then but none of them actually include this fix which as left me stuck having to pin my rails gem version to the 7-1-stable branch instead of updating to the latest gem release.

So far it seems like it is on track to release with 7.2.x (it exists in the v7.2.0.beta2 tag) but my thought with the backport to the 7-1-stable branch PR was that it would be included in 7.1.x releases going forward as well.

[zzak]

@tenpaiyomi The last bugfix release was 7.1.3, before the backport was merged. So whenever 7.1.4 is out, that should include this. Thanks for your patience.