Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raise unknown type error on the definition time #41166

Merged

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Jan 19, 2021

If unknow type is given for attribute (attribute :foo, :unknown),
unknown type error isn't raised on the definition time but runtime.

It should be raised on the definition time.

If unknow type is given for attribute (`attribute :foo, :unknown`),
unknown type error isn't raised on the definition time but runtime.

It should be raised on the definition time.
@kamipo kamipo force-pushed the raise_unknown_type_error_on_definition_time branch from bdfa96e to 26e1fe4 Compare January 19, 2021 06:52
@@ -7,7 +7,7 @@ class PostgresqlTime < ActiveRecord::Base
# Declare attributes to get rid from deprecation warnings on ActiveRecord 6.1
attribute :time_interval, :string
attribute :scaled_time_interval, :interval
end
end if current_adapter?(:PostgreSQLAdapter)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are for bin/test testing.

bundle exec rake test loads test files for the testing adapter, but bin/test always loads all test files, :interval, :point, and :legacy_point types are registered only for postgresql adapter, now it will raise unknown type error for sqlite3 and mysql2 adapters.

t.test_files = (FileList["test/cases/**/*_test.rb"].reject {
|x| x.include?("/adapters/")
} + FileList["test/cases/adapters/#{adapter_short}/**/*_test.rb"])

@kamipo kamipo merged commit ddacedd into rails:main Jan 20, 2021
@kamipo kamipo deleted the raise_unknown_type_error_on_definition_time branch January 20, 2021 09:24
@rafaelfranca
Copy link
Member

We should not connect to the database when defining classes. This can take applications down, and the connection configuration isn't always setup when the class is being defined.

@kamipo
Copy link
Member Author

kamipo commented Jan 29, 2021

Yeah... I realized that in making #41262, it is due to the implementation of Type.adapter_name_from.
I'll be going to fix Type.adapter_name_from.

kamipo added a commit to kamipo/rails that referenced this pull request Feb 2, 2021
rails#41166 made `attribute` require a connection due to calling
`ActiveRecord::Type.adapter_name_from` immediately.

This fixes `adapter_name_from` to get the adapter name from the
connection db config to not retrieve a connection.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants