Allow the rails method to be called and change the result only when called from the migration #776
Conversation
jrafanie
force-pushed
the
fix-rails-7-1-plus-test-failure
branch
from
746c2ba to
cb6d873
Compare
| original_method.call(*args, &block).dup.tap { |i| i.delete(:password) } | ||
| allow(ActiveRecord::Base.connection_db_config).to receive(:configuration_hash).and_wrap_original do |original_method, *args, &block| | ||
| # set a value for any calls originating from the migration file, not from rails itself | ||
| original_method.call(*args, &block).dup.tap { |i| i.delete(:password) if caller_locations.any? {|loc| loc.path.include?("db/migrate/20241017013023_reencrypt_password_scramsha.rb")} } |
There was a problem hiding this comment.
Note, this is still super brittle but that's because the test is modifying the database configuration in the context of the test and code outside our migration can be impacted by that change. This is why we had 10 calls originally in rails 7.0 and only 1 in 7.1.
In rails 7.0, it's called 10 times plus 1 more on the next line. In rails 7.1+, it's called only once. From a test perspective, we don't care if it's even called. We only care that the SQL query is executed with the scramsha password or not. To change the username/password only in calls from the migration itself and not from rails, we hack around the caller_locations to make the change only for the migration callers. With this, I believe we can expect test success on rails 7.1 and 7.2. It was added in 742841c but I think this change emphasizes what we really care about testing and removes an assertion on internals that we don't really have interest in testing. Fixes ManageIQ#768
jrafanie
force-pushed
the
fix-rails-7-1-plus-test-failure
branch
from
cb6d873 to
7af2420
Compare
jrafanie
changed the title
| @@ -9,9 +9,9 @@ | |||
|
|
|||
| username = ActiveRecord::Base.connection_db_config.configuration_hash[:username] | |||
|
|
|||
| expect(ActiveRecord::Base.connection_db_config).to receive(:configuration_hash).exactly(10).times.and_call_original | |||
There was a problem hiding this comment.
Let me explain this... the reason it's called 10 times here is because rails itself is calling it in rails 7.0. Previously, we called the original rails method as is 10 times, then, we change the result on the 11th call to inject a password if it wasn't configured, or in the case of the second test remove the password.
This changed in rails 7.1 as it's only called 1 time and it was in our migration probably due to internal rails changes. To support both versions, I'm checking if the caller locations shows it coming from the migration file and only then change the result for the test scenario.
I guess we could also do a rails version check but felt that it's brittle to expect or not expect a rails method to be called or worse, to modify the result from it if it's originating from rails itself. In fact, my first try at this PR had this failing in CI because on CI the password wasn't configured as I guess it's allowing local socket connections.
In rails 7.0, it's called 10 times plus 1 more on the next line. In rails 7.1+, it's called only once. From a test perspective, we don't care if it's even called. We only care that the SQL query is executed with the scramsha password or not.
To change the username/password only in calls from the migration itself and not from
rails, we hack around the caller_locations to make the change only for the migration callers.
With this, I believe we can expect test success on rails 7.1 and 7.2.
It was added in 742841c but I think this change emphasizes what we really care about testing and removes an assertion on internals that we don't really have interest in testing.
Fixes #768