Fix failing railties framework tests #42991
Conversation
hahmed
force-pushed
the
ha/fix-railties-framwork-tests
branch
3 times, most recently
from
1316d71 to
9df9511
Compare
| @@ -125,7 +125,7 @@ def validate_column_size(attribute_name) | |||
| if table_exists? && limit = columns_hash[attribute_name.to_s]&.limit | |||
| validates_length_of attribute_name, maximum: limit | |||
| end | |||
| rescue ActiveRecord::ConnectionNotEstablished => e | |||
| rescue ActiveRecord::ConnectionNotEstablished, ActiveRecord::DatabaseConnectionError => e | |||
| Rails.logger.warn "Skipping adding length validation for #{self.name}\##{attribute_name}. Can't check column limit due to: #{e.inspect}" | |||
| end | |||
There was a problem hiding this comment.
Honestly I think this whole check should be reconsidered, having to access the DB at class definition time is kind of an anti-pattern. Can we check who introduced this and bring them in the discussion to see if this could either be eliminated or moved later in the boot process?
I'm thinking it could be part of define_attribute_methods as that's the time when we expect to have access to the database schema.
There was a problem hiding this comment.
Agreed... cc/ @jorgemanrubia would love to hear your thoughts?
A bit of background, it looks as though EncryptableRecord is being called too early in the boot process, here is a stack trace that I found whilst working on these failing tests:
Error:
ApplicationTests::FrameworksTest#test_expire_schema_cache_dump_if_the_version_can't_be_checked_because_the_database_is_unhealthy:
ActiveRecord::DatabaseConnectionError: There is an issue connecting with your hostname: 127.0.0.1.
Please check your database configuration and ensure there is a valid connection to your database.
/Users/haroon/projects/rails/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb:51:in `rescue in new_client'
/Users/haroon/projects/rails/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb:43:in `new_client'
/Users/haroon/projects/rails/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb:23:in `mysql2_connection'
/Users/haroon/projects/rails/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:643:in `public_send'
/Users/haroon/projects/rails/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:643:in `new_connection'
/Users/haroon/projects/rails/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:687:in `checkout_new_connection'
/Users/haroon/projects/rails/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:666:in `try_to_checkout_new_connection'
/Users/haroon/projects/rails/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:627:in `acquire_connection'
/Users/haroon/projects/rails/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:328:in `checkout'
/Users/haroon/projects/rails/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:168:in `connection'
/Users/haroon/projects/rails/activerecord/lib/active_record/connection_adapters/abstract/connection_handler.rb:211:in `retrieve_connection'
/Users/haroon/projects/rails/activerecord/lib/active_record/connection_handling.rb:309:in `retrieve_connection'
/Users/haroon/projects/rails/activerecord/lib/active_record/connection_handling.rb:265:in `connection'
/Users/haroon/projects/rails/activerecord/lib/active_record/model_schema.rb:395:in `table_exists?'
/Users/haroon/projects/rails/activerecord/lib/active_record/encryption/encryptable_record.rb:125:in `validate_column_size'
/Users/haroon/projects/rails/activerecord/lib/active_record/encryption/encryptable_record.rb:92:in `encrypt_attribute'
/Users/haroon/projects/rails/activerecord/lib/active_record/encryption/encryptable_record.rb:53:in `block in encrypts'
/Users/haroon/projects/rails/activerecord/lib/active_record/encryption/encryptable_record.rb:50:in `each'
/Users/haroon/projects/rails/activerecord/lib/active_record/encryption/encryptable_record.rb:50:in `encrypts'
/Users/haroon/projects/rails/actiontext/app/models/action_text/encrypted_rich_text.rb:7:in `<class:EncryptedRichText>'
/Users/haroon/projects/rails/actiontext/app/models/action_text/encrypted_rich_text.rb:4:in `<module:ActionText>'
/Users/haroon/projects/rails/actiontext/app/models/action_text/encrypted_rich_text.rb:3:in `<top (required)>'
/Users/haroon/.gem/ruby/3.0.2/gems/zeitwerk-2.5.0.beta/lib/zeitwerk/kernel.rb:27:in `require'
/Users/haroon/.gem/ruby/3.0.2/gems/zeitwerk-2.5.0.beta/lib/zeitwerk/kernel.rb:27:in `require'
/Users/haroon/.gem/ruby/3.0.2/gems/zeitwerk-2.5.0.beta/lib/zeitwerk/loader/helpers.rb:93:in `const_get'
/Users/haroon/.gem/ruby/3.0.2/gems/zeitwerk-2.5.0.beta/lib/zeitwerk/loader/helpers.rb:93:in `cget'
/Users/haroon/.gem/ruby/3.0.2/gems/zeitwerk-2.5.0.beta/lib/zeitwerk/loader.rb:230:in `block (2 levels) in eager_load'
/Users/haroon/.gem/ruby/3.0.2/gems/zeitwerk-2.5.0.beta/lib/zeitwerk/loader/helpers.rb:24:in `block in ls'
/Users/haroon/.gem/ruby/3.0.2/gems/zeitwerk-2.5.0.beta/lib/zeitwerk/loader/helpers.rb:16:in `each_child'
/Users/haroon/.gem/ruby/3.0.2/gems/zeitwerk-2.5.0.beta/lib/zeitwerk/loader/helpers.rb:16:in `ls'
/Users/haroon/.gem/ruby/3.0.2/gems/zeitwerk-2.5.0.beta/lib/zeitwerk/loader.rb:225:in `block in eager_load'
/Users/haroon/.gem/ruby/3.0.2/gems/zeitwerk-2.5.0.beta/lib/zeitwerk/loader.rb:212:in `synchronize'
/Users/haroon/.gem/ruby/3.0.2/gems/zeitwerk-2.5.0.beta/lib/zeitwerk/loader.rb:212:in `eager_load'
/Users/haroon/.gem/ruby/3.0.2/gems/zeitwerk-2.5.0.beta/lib/zeitwerk/loader.rb:301:in `each'
/Users/haroon/.gem/ruby/3.0.2/gems/zeitwerk-2.5.0.beta/lib/zeitwerk/loader.rb:301:in `eager_load_all'
/Users/haroon/projects/rails/railties/lib/rails/application/finisher.rb:111:in `block in <module:Finisher>'
/Users/haroon/projects/rails/railties/lib/rails/initializable.rb:32:in `instance_exec'
/Users/haroon/projects/rails/railties/lib/rails/initializable.rb:32:in `run'
/Users/haroon/projects/rails/railties/lib/rails/initializable.rb:61:in `block in run_initializers'
/Users/haroon/.rubies/ruby-3.0.2/lib/ruby/3.0.0/tsort.rb:228:in `block in tsort_each'
/Users/haroon/.rubies/ruby-3.0.2/lib/ruby/3.0.0/tsort.rb:350:in `block (2 levels) in each_strongly_connected_component'
/Users/haroon/.rubies/ruby-3.0.2/lib/ruby/3.0.0/tsort.rb:431:in `each_strongly_connected_component_from'
/Users/haroon/.rubies/ruby-3.0.2/lib/ruby/3.0.0/tsort.rb:349:in `block in each_strongly_connected_component'
/Users/haroon/.rubies/ruby-3.0.2/lib/ruby/3.0.0/tsort.rb:347:in `each'
/Users/haroon/.rubies/ruby-3.0.2/lib/ruby/3.0.0/tsort.rb:347:in `call'
/Users/haroon/.rubies/ruby-3.0.2/lib/ruby/3.0.0/tsort.rb:347:in `each_strongly_connected_component'
/Users/haroon/.rubies/ruby-3.0.2/lib/ruby/3.0.0/tsort.rb:226:in `tsort_each'
/Users/haroon/.rubies/ruby-3.0.2/lib/ruby/3.0.0/tsort.rb:205:in `tsort_each'
/Users/haroon/projects/rails/railties/lib/rails/initializable.rb:60:in `run_initializers'
/Users/haroon/projects/rails/railties/lib/rails/application.rb:369:in `initialize!'
/Users/haroon/projects/rails/tmp/d20210811-10419-oivctw/app/config/environment.rb:5:in `<top (required)>'
/Users/haroon/.gem/ruby/3.0.2/gems/zeitwerk-2.5.0.beta/lib/zeitwerk/kernel.rb:35:in `require'
/Users/haroon/.gem/ruby/3.0.2/gems/zeitwerk-2.5.0.beta/lib/zeitwerk/kernel.rb:35:in `require'
/Users/haroon/projects/rails/activesupport/lib/active_support/dependencies.rb:291:in `block in require'
/Users/haroon/projects/rails/activesupport/lib/active_support/dependencies.rb:261:in `load_dependency'
/Users/haroon/projects/rails/activesupport/lib/active_support/dependencies.rb:291:in `require'
/Users/haroon/projects/rails/railties/test/application/initializers/frameworks_test.rb:264:in `block (2 levels) in <class:FrameworksTest>'
/Users/haroon/projects/rails/railties/test/env_helpers.rb:27:in `switch_env'
/Users/haroon/projects/rails/railties/test/application/initializers/frameworks_test.rb:263:in `block in <class:FrameworksTest>'
The relevant parts start with models/action_text/encrypted_rich_text.rb, looking at that stack trace not sure why action_text has caused encryptable record to appear so early in the boot process. Also we may we need to move the defining of the attribute encrypted_attribute to define_attribute_methods :encrypted_attribute?
I'm not too familiar with the EncryptableRecord so I'm going to dig into this a little more.
There was a problem hiding this comment.
So the problem is because when you declare an attribute as encrypted with:
class Post < ActiveRecord::Base
encrypts :title
endInternally, it adds a validation that validates the length of title based on the column size (see validate_column_size option in the guide). To read the column size, it checks the database schema. All this happens at the "class declaration" level.
We can definitely run that logic ahead in the process, when a connection with the database happens. I'll investigate a good approach to do that, but if you have suggestions please shoot. Some kind of callback would make sense to me.
There was a problem hiding this comment.
load_schema! could make sense:
Or as mentioned previously, define_attribute_methods as that's when Active Record generate all the methods based on the DB schema.
There was a problem hiding this comment.
Taking a stab here #43009. I think I prefer to go load_schema!, but maybe it's because I am not understanding how would you use define_attribute_methods here. We can move the discussion about the solution there. Thanks!
|
Weird, I swear those tests are passing for me locally on both Ruby 2.7 and 3.0, but it's now failing on the CI here https://buildkite.com/rails/rails/builds/80175#129ad9c4-307c-4dfa-951e-fe62fd4ba0da/1302-1306 |
Doing it before meant it required a database connection at class loading time, which was a source a trouble. See rails#42991 (comment)
hahmed
force-pushed
the
ha/fix-railties-framwork-tests
branch
from
9df9511 to
69b1aff
Compare
errors are not being rescued correctly
hahmed
force-pushed
the
ha/fix-railties-framwork-tests
branch
from
69b1aff to
46d6637
Compare
1) Failure: AppGeneratorTest#test_assets [test/generators/app_generator_test.rb:121] related rails#42991
|
was this fixed by #43009 ? |
|
Nope, fixed in cabe311 however. Going to close this. |
Summary
A couple of FrameworksTest gets "undefined class/module Mysql2:: (ArgumentError)". https://buildkite.com/rails/rails/builds/79982#a24742cc-7394-4ff0-8713-aa64164fd925 introduced by the PR #39723
In the 2 failing tests we are trying to establish a connection to the database using incorrect config so that we can continue with the boot process rather than crash. In the PR I created #39723 we now catch incorrect host which is what's being used to simulate unhealthy db for mysql here https://github.com/rails/rails/pull/39723/files#diff-538a9bf3450c3aed556a0d2eacc26839a60becd744cda8c0a247dcb06aa0f02fR50 which is what's causing the test failures.
In this PR we are rescuing the
ActiveRecord::DatabaseConnectionErrorin the EncryptableRecord which fixes those failing framework tests. I also rescued theDatabaseConnectionErrorinside the railties but if I remove those the tests still pass, I'm wondering if I should remove that, wdyt @byroot ?Other Information
Issue reported here #42968