add initial type mapping support for sqlite only - #5148
Conversation
|
Thanks for working on this. I did not have the capacity to review the changes this week, hopefully I find some time next week.
If it's meaningful possible to support this as CLI flags as well, I would prefer having the CLI flag. |
No problem!
I'll see what I can do there then. |
weiznich
left a comment
weiznich
left a comment
There was a problem hiding this comment.
The basic approach looks right to me. I would strongly prefer having this for all supported backends. I would be fine if the initial implementation only supports renaming all instances of a type, as we always can extend this later to be more sophisticated.
| let Some(type_mappings) = &config.type_map else { | ||
| return None; | ||
| }; | ||
|
|
||
| let Some(type_map) = type_mappings.iter().find(|map| { | ||
| map.database_typename.to_lowercase() == type_name | ||
| && map | ||
| .table_name | ||
| .as_deref() | ||
| .is_none_or(|t| t == table.sql_name) | ||
| && map | ||
| .schema_name | ||
| .as_deref() | ||
| .is_none_or(|s| Some(s) == table.schema.as_deref()) | ||
| && map | ||
| .column_name | ||
| .as_deref() | ||
| .is_none_or(|c| c == attr.column_name) | ||
| }) else { | ||
| return None; | ||
| }; |
There was a problem hiding this comment.
I would suggest to have this rather as function on SchemaTypeMap instead so that it can be shared between all backends.
Something like:
impl SchemaTypeMap {
fn map_type(&self, db_type: &str, table_name: &str, schema_name: Option<&str>, column_name: &str) -> &str {…}
}
| pub table_name: Option<String>, | ||
| pub schema_name: Option<String>, | ||
| pub column_name: Option<String>, | ||
| } |
There was a problem hiding this comment.
Matching on these is currently not implemented, would be great to have that, although that's not a requirement for the initial PR to land.
If you decide not to implement this, I would rather drop these fields for now. We always can add them later.
|
|
||
| #[test] | ||
| #[cfg(feature = "sqlite")] | ||
| fn print_schema_type_mapping() { |
There was a problem hiding this comment.
We do want to have a generate_migration test as well, as we need to reverse that mapping there
2 participants
This adds basic type mapping support to sqlite. This is an implementation of this proposal #4113 (comment). The issue was referenced in these conversations #4192.
This only provides support for Sqlite as a starting point because, one I needed it, and two Sqlite is a lot more permissive with "types" so this would be a good starting point. Also wanted to gather feedback before making anymore broader changes to the other backends. This supports configuring output schema type for specific table, column, or schema or combination of them.
One major question I had was if it was worth it to add a CLI flag for this feature? Or if it is a requirement that there is a CLI flag for all configuration options. I opted to only have this work if using
diesel.tomlbecause it's easier to write the table of fields that way.