rspec / rspec-rails

RSpec for Rails 6+
https://rspec.info
MIT License
5.15k stars 1.03k forks source link

Support for encrypted attributes in fixtures cannot be enabled when using Rails 7.2.0 #2779

Closed mbaird closed 2 weeks ago

mbaird commented 1 month ago

What Ruby, Rails and RSpec versions are you using?

Ruby version: 3.3.4 Rails version: 7.2.0 RSpec version: 3.13

Observed behaviour

Support for encrypted attributes in fixtures cannot be enabled when using Rails 7.2.0.

When the active_record_fixture_set hook in the Active Record railtie runs, the value of ActiveRecord::Encryption.config.encrypt_fixtures is false, and the [module is not prepended][1].

This is because the [active_record][2] hook has not yet run, and encryption has not been configured.

(Note, this is only an issue when eager_loading is not enabled, as eager loading the application causes Active Record to be loaded before RSpec loads the Active Record Test Fixtures.)

[2]: https://github.com/rails/rails/blob/fb6c4305939da06efdf2893d99130e7829c53e8b/activerecord/lib/active_record/railtie.rb#L359

[1]: https://github.com/rails/rails/blob/fb6c4305939da06efdf2893d99130e7829c53e8b/activerecord/lib/active_record/railtie.rb#L379

Expected behaviour

Setting config.active_record.encryption.encrypt_fixtures = true should allow fixtures to set encrypted attributes. Instead, a ActiveRecord::Encryption::Errors::Decryption exception is raised whenever an encrypted attribute is attempted to be read.

Can you provide an example reproduction?

I've made a simple reproduction repo here: https://github.com/mbaird/fixture-encryption-bug

The main branch includes the failing example, the output can be seen in the GitHub Action run.

The rails-7-1 branch switches to Rails 7.1, the tests pass on this branch.

I think these Rails issues may be related https://github.com/rails/rails/pull/50606, https://github.com/rails/rails/pull/48577.

pirj commented 1 month ago

Thanks for reporting. The first three links are broken.

Shouldn’t the build fail because it expects an encrypted value for the email?

mbaird commented 1 month ago

Thanks for reporting. The first three links are broken.

Fixed now, thanks for flagging.

Shouldn’t the build fail because it expects an encrypted value for the email?

The test failure is because ActiveRecord is attempting to decrypt the un-encrypted value set by the fixture.

If config.active_record.encryption.encrypt_fixtures is set to true, the EncryptedFixtures module should be prepended, and the encrypt_fixture_data method encrypts each of the attributes before the record is created, allowing the use of plain, unencrypted values in fixture files.

pirj commented 4 weeks ago

the active_record hook has not yet run Active Record to be loaded before RSpec loads the Active Record Test Fixtures

Sounds like we need to change the order. We can work without activerecord. And we seem to trigger loading fixtures here. Should we also trigger loading AR somehow, too?

mbaird commented 4 weeks ago

Sounds like we need to change the order. We can work without activerecord. And we seem to trigger loading fixtures here. Should we also trigger loading AR somehow, too?

The hook runs a little earlier when ActiveSupport::TestFixtures is included here, it runs hooks, so that's when FixtureSupport is required in rails.rb.

The AR hooks are run here, on require of active_record/base.

pirj commented 4 weeks ago

How comes if defined?(ActiveRecord::TestFixtures) is true if the hook activerecord/base wasn’t run? 🤔 Shouldn’t test fixtures depend on activerecord/base to be loaded? Or can test fixtures, in theory, be used without activerecord? Like with bare activemodel?

mbaird commented 4 weeks ago

How comes if defined?(ActiveRecord::TestFixtures) is true if the hook activerecord/base wasn’t run? 🤔

I think this is because of autoloading. The constant is defined, but hasn't yet been loaded so this guard will pass. It'll then be loaded by the include a few lines later.

Shouldn’t test fixtures depend on activerecord/base to be loaded? Or can test fixtures, in theory, be used without activerecord? Like with bare activemodel?

I think they need ActiveRecord, and in fact there is a reference to ActiveRecord::Base inside TestFixtures, so the hook will be run then but this is too late.

maximerety commented 3 weeks ago

Hello,

I'm the implementer of the fix from https://github.com/rails/rails/pull/50606 that you mentioned above in the description of this issue.

I've tried a fix in https://github.com/rails/rails/pull/52669, to support access to properly configured ActiveRecord::Encryption.config properties before the lazy loading of ActiveRecord::Base.

Could you test if this solves your problem?

Thank you.

mbaird commented 3 weeks ago

Thanks for that @maximerety, I can confirm this fixes the issue!

pirj commented 3 weeks ago

Fantastic! Thanks a lot for the fix and keeping an eye open @maximerety ! 🙌 I’ml close this once the Rails PR is merged.

maximerety commented 2 weeks ago

@mbaird / @pirj Thanks for your feedbacks.

I've added missing tests to https://github.com/rails/rails/pull/52669, which is now in good shape. We'll have to wait for a review.

pirj commented 2 weeks ago

Fixed in Rails. Thank you, @maximerety and @mbaird !