Extract MySQL into adapter so we can run against other databases #4
Conversation
shioyama
force-pushed
the
shioyama/database_adapters
branch
from
844c4d0 to
ec33d77
Compare
There was a problem hiding this comment.
It looks like in all places we have access to the model class which makes me wonder if that's what we need to use to choose the adapter. Model's connection is what defines the adapter and all columns should belong to the same adapter for a given model. So I'd like to suggest we change the DatabaseValidations::Adapters.for implementation to accept an AR model class/connection class or database name to base the selection on with potential memoization to avoid executing the same code for every column
| @@ -77,6 +77,10 @@ def validate_each(record, attribute, _value) | |||
| validator.validate(record) | |||
| end | |||
| end | |||
|
|
|||
| def adapter_for(column) | |||
There was a problem hiding this comment.
I wonder if we should design it in a way to allow choosing the adapter per connection or database configuration. For majority of the applications adapter will not change regardless of the model used. Some application in rare cases may use different databases for different models and we need to account for that. But it's unfortunate to keep executing the same code for every column even though we know columns of the same model will belong to the same adapter
| @registry = {} | ||
|
|
||
| def self.for(column) | ||
| name = column.class.name.delete_prefix("ActiveRecord::ConnectionAdapters::") |
There was a problem hiding this comment.
I think this is what makes me the most concerned. It feels like we are relying too much on the internal structure of Rails which may change. I'd much rather rely on adapter names like mysql2, sqlite3 and postgresql which are unlikely to be changed
| @@ -7,8 +7,9 @@ def truncate_value_to_field_limit(field, value) | |||
| return if value.nil? | |||
|
|
|||
| column = self.class.columns_hash[field.to_s] | |||
| maximum, type, encoding = ActiveRecord::DatabaseValidations::MySQL.column_size_limit(column) | |||
| value = ActiveRecord::DatabaseValidations::MySQL.value_for_column(value, encoding) | |||
| adapter = DatabaseValidations::Adapters.for(column) | |||
There was a problem hiding this comment.
I looked around the docs and my suggestion is build the registry around adapter names and not column instances
| adapter = DatabaseValidations::Adapters.for(column) | |
| adapter = DatabaseValidations::Adapters.for(self.class.connection_db_config.adapter) |
shioyama
force-pushed
the
shioyama/cleanup
branch
from
f115529 to
5b9b481
Compare
Get tests to pass locally
shioyama
force-pushed
the
shioyama/database_adapters
branch
from
ec33d77 to
de65d1a
Compare
|
I'm really not confident this is the right approach (even with the suggestions here, thanks!). It feels like we shouldn't need this gem at all, if ActiveRecord provided enough of the column type configuration. |
I would really love to see Rails implementing the features we need instead! |
|
Replaced by #6 |
What the title says: extract the stuff in
ActiveRecord::DatabaseValidations::MySQLintoActiveRecord::DatabaseValidations::Adapters::MySQLso we can add other databases. I implemented a minimal registry with proc values that require/return the adapter class.