fix: (#3387) use collation to determine if string type is compatible #3400
Conversation
There was a problem hiding this comment.
@abonander @alu I'd love your feedback on this, whenever you get a chance 😄
sqlx-mysql/src/collation.rs
Outdated
There was a problem hiding this comment.
these changes are all a result of checking against SELECT ID, COLLATION_NAME FROM INFORMATION_SCHEMA.COLLATIONS ORDER BY ID; locally - let me know if any of them should be thrown out.
- There was an additional ~100 collations on my machine, which I added and then commented out, rather than implementing all the other pieces necessary. let me know if you'd like me to finish that out, I'd be happy to, if that's something we want. otherwise, we can remove the comments of those extra collations.
uft8_*was displayed asutf8mb3_*on my machine. let me know if those changes should be reverted- also added
impl TryFrom<u16> for Collationfor use here
| // https://dev.mysql.com/doc/internals/en/com-query-response.html#column-type | ||
| // dead link ^ | ||
| // https://dev.mysql.com/doc/dev/mysql-server/9.0.0/page_protocol_com_query_response_text_resultset_column_definition.html | ||
| // the "type of the column as defined in enum_field_types" | ||
| // https://dev.mysql.com/doc/dev/mysql-server/9.0.0/field__types_8h.html |
There was a problem hiding this comment.
feel free to suggest which of these links we should keep - the original link here was dead when i tried it.
DrewMcArthur
changed the title
DrewMcArthur
changed the title
| @@ -12,6 +13,7 @@ impl Type<MySql> for str { | |||
| MySqlTypeInfo { | |||
| r#type: ColumnType::VarString, // VARCHAR | |||
| flags: ColumnFlags::empty(), | |||
| collation: Collation::utf8mb3_bin, | |||
There was a problem hiding this comment.
I think using utf8mb3_bin regardless of session state is misleading.
In my opinion, the only difference needed in this context is whether it is binary or string.
How about using an enum like this one?
CollationType {
Binary,
String
}There was a problem hiding this comment.
This was one of the pieces I wasn't sure about, what the default should be for some of these. I think you're right, _bin probably isn't right & I'll update this to utf8mb3_general_ci if you think that'd be correct.
As for changing the type of MySqlTypeInfo's collation field to an enum, I think the source of the issue on #3387 is that there are three cases being compressed into the ColumnFlag::binary flag:
- a
VARCHARhas a collation likeutf8_general_ci, and theColumnFlag::binaryis false, so this is compatible with a RustString - a
BLOBhas a collation ofbinary,ColumnFlag::binaryis true, so it's not compatible with a RustString, which is also correct - a
VARCHARhas a collation of*_bin, soColumnFlag::binaryis true, so it's marked incompatible when we should be able to decode this into aString. This is the case that needs to be fixed.
This could be handled with that enum, if the value is String for both VARCHAR cases, but I feel like that could also be confusing to have a CollationType::String (as opposed to binary) while simultaneously having ColumnFlag::binary = true.
I think my preference is to keep track of each unique collation here, since it's only a byte, but I'll defer to you and @abonander to decide what makes more sense.
There was a problem hiding this comment.
| collation: Collation::utf8mb3_bin, | |
| collation: Collation::utf8mb3_general_ci, |
There was a problem hiding this comment.
^ @alu (sorry, I'm terrible at remembering to directly @ people in replies here)
|
@alu This fixes only for MySQL, not for SQLite one. I understand that original question is for MySQL, but SQLite and presumably PostgreSQL are also affected. Please, tell me if i need to create a separate issue for SQLite. |
|
can you folks merge this? |
|
@eirnym oh good catch - my bandwidth is pretty low right now, so if you wanted to open a PR into my branch with those changes, or even just the tests for them that'd be awesome! @alvico I'm not sure their release schedule, but you can target this branch in your |
|
any news on merging this? |
Does your PR solve an issue?
fixes #3387
fixes #3390
impl TryFrom<u16> for Collationcollationfield toTypeInfostrcompatibility check to compare withCollation::binaryinstead ofColumnFlags::BinaryVARCHARwith*_bincollation incorrectly interpreted asVARBINARYColumn #3387, failed onmain, and passes now. (let me know if we'd like to make this test more robust)todo
<uuid::fmt::Hyphenated as sqlx::Type<MySql>::compatible()is wrong? #3409