Configure collation/charset for DB connection #100
Conversation
|
If we use a different collation, should the strings sent as arguments be encoded differently? We would need a couple of specs for this change, I fear it could break the support of other collations if there are none. |
|
@Dakad Would it be possible to add some specs for this? Also note the question in #100 (comment) |
|
@straight-shoota @bcardiff My bad, I completely forgot about this 😞 I will add some specs. About the question, I'm not sure to fully understand it. I expect the |
| } | ||
|
|
||
| def self.default_collation | ||
| "utf8_general_ci" |
There was a problem hiding this comment.
I reverted the default collation to utf8_general_ci as before.
spec/driver_spec.cr
Outdated
| @@ -30,13 +30,28 @@ describe Driver do | |||
| DB.open "mysql://crystal_test:secret@#{database_host}/crystal_mysql_test" do |db| | |||
| db.scalar("SELECT DATABASE()").should eq("crystal_mysql_test") | |||
| db.scalar("SELECT CURRENT_USER()").should match(/^crystal_test@/) | |||
| db.scalar("SELECT @@collation_connection").should eq("utf8_general_ci") | |||
There was a problem hiding this comment.
I added these 2 assertions to check when the encoding is not provided in the connection string, the DB connection's collation should be utf8_general_ci.
Not sure if it's a good practice to have that. I assume the DB server could have a different collation set in the config.ini
There was a problem hiding this comment.
I'd say this is probably fine.
It might be better to break them out into a separate example and add some description of the expectation. That would make it easier to understand the reasons if this happens to fail.
There was a problem hiding this comment.
I added another section to the README.md
There was a problem hiding this comment.
I don't think we need a default_charset function. Seems it's not used.
The specs do not verify if a string column is able to store and retrieve a string with special chars. If this is already working we can add those specs later and add the feature. But it does not seems we are preventing to break this in the future as is.
|
I checked with other native driver implementation and it seems they don't do anything to the payload if the collation/charset is different. So extra 👍 on that regard. |
|
All right, then let's merge it 👍 |
|
Yup. I would drop the unneeded defs I pointed above and merge it. I can take care of it in the following days. |
|
Thanks @Dakad for the contribution and my apologies for the long delay in this PR |
Resolves #46
Context
Allow the configuration of the collation to use during the DB.open connection.
Just include into the connection string URI
&encoding=utf8mb4_unicode_520_ciThe charset is deduced from the encoding.
Only the collation with their ID below than 1 byte can be used and some collation are not supported by MySQL.
See: https://dev.mysql.com/doc/refman/5.7/en/charset-connection.html#charset-connection-impermissible-client-charset