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

Convert "blocks" table to utf8mb4 to accept 4 byte utf8 characters #23493

Merged
merged 3 commits into from Jul 9, 2018

Conversation

joshlory
Copy link
Contributor

@joshlory joshlory commented Jul 3, 2018

Also changed the "category" column from t.text -> t.string. Category should be a short string.

After migration:

mysql> show full columns from blocks;
+-------------+--------------+--------------------+------+-----+---------+----------------+---------------------------------+---------+
| Field       | Type         | Collation          | Null | Key | Default | Extra          | Privileges                      | Comment |
+-------------+--------------+--------------------+------+-----+---------+----------------+---------------------------------+---------+
| id          | int(11)      | NULL               | NO   | PRI | NULL    | auto_increment | select,insert,update,references |         |
| name        | varchar(255) | utf8mb4_unicode_ci | YES  |     | NULL    |                | select,insert,update,references |         |
| level_type  | varchar(255) | utf8mb4_unicode_ci | YES  |     | NULL    |                | select,insert,update,references |         |
| category    | varchar(255) | utf8mb4_unicode_ci | YES  |     | NULL    |                | select,insert,update,references |         |
| config      | text         | utf8mb4_unicode_ci | YES  |     | NULL    |                | select,insert,update,references |         |
| helper_code | text         | utf8mb4_unicode_ci | YES  |     | NULL    |                | select,insert,update,references |         |
| created_at  | datetime     | NULL               | NO   |     | NULL    |                | select,insert,update,references |         |
| updated_at  | datetime     | NULL               | NO   |     | NULL    |                | select,insert,update,references |         |
+-------------+--------------+--------------------+------+-----+---------+----------------+---------------------------------+---------+
8 rows in set (0.00 sec)

Also changed the "category" column from `t.text` -> `t.string`. Category should be a short string.
execute 'alter table blocks modify name varchar(255) charset utf8 collate utf8_unicode_ci'
execute 'alter table blocks modify category varchar(255) charset utf8 collate utf8_unicode_ci'
execute 'alter table blocks modify config text(21845) charset utf8 collate utf8_unicode_ci'
execute 'alter table blocks modify helper_code text(21845) charset utf8 collate utf8_unicode_ci'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rails annotates the model as text(65355) but MySQL wants the value to be the actual number of characters stored. 65355 / 3 = 21845.

@joshlory
Copy link
Contributor Author

joshlory commented Jul 3, 2018

@@ -5,7 +5,7 @@
# id :integer not null, primary key
# name :string(255)
# level_type :string(255)
# category :text(65535)
# category :string(255)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Intentional change. Category should be a short string.

Copy link
Contributor

@balderdash balderdash left a comment

Choose a reason for hiding this comment

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

🐼!

@wjordan
Copy link
Contributor

wjordan commented Jul 3, 2018

Migration looks good. I thought handling utf8mb4 queries would also require changes to our mysql2 adapter's client-connection charset/encoding settings as well. This migration should be safe either way, but might not be effective without further changes to the client. In any case, it would be good to test/verify that this migration actually fixes emoji input in block definitions as expected.

@joshlory
Copy link
Contributor Author

joshlory commented Jul 3, 2018

@wjordan good call, I updated the default adaptor to also support utf8mb4 in commit 2ba71c4. Are there any downsides to this change?

When testing locally, tables that have utf8 encoding instead of utf8mb4 still get the Mysql2::Error: Incorrect string value error when trying to save 4-byte emoji, but saving these the "blocks" table now works.

image

Copy link
Contributor

@wjordan wjordan left a comment

Choose a reason for hiding this comment

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

LGTM. I don't know any other details on whether the client-adapter encoding change poses other potential problems, but as long as the test suite passes (which it looks like it did), I wouldn't expect there to be any issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants