Addons-server: Server doesn't support emoji

[muffinresearch]

STR:

• Anywhere on addon-server e.g. the dev hub save a field and add emoji e.g. "this is before emoji 🔥 this is after emoji"
• Save that field

What happens:

Everything after the emoji is stripped out.

What should happen

I should see the emoji in the output.

Screenshots

Before:

After:

See https://django-mysql.readthedocs.io/en/latest/checks.html#django-mysql-w003-utf8mb4 for a possible solution. If this is the problem then it's at the DB level.

Also this post is useful for a lot more detail into the kind of checks required if making this switch: https://mathiasbynens.be/notes/mysql-utf8mb4

See also:

• https://github.com/mozilla/addons-frontend/issues/5649
• https://github.com/mozilla/addons-frontend/issues/5644

[muffinresearch]

Note: I saw the same behaviour on prod so this isn't a django upgrade issue.

[muffinresearch]

Marking as p2 because this is causing nasty problems with the API for collections. If fixing the DB needs more time we might want to consider doing something else to remedy the scenario where data from the server can be completely blank if saved with a leading emoji. See mozilla/addons-frontend#5649

[diox]

Yeah, it's because we're not using utf8mb4. I filed a bug about this a while ago for AMO and Marketplace back in the day... The migration to switch is really annoying, it would take the database down for quite a while and synchronizing the client changes required with the migration (which needs to be applied to primary and replica dbs) is also non trivial.

[muffinresearch]

I wonder if there is a way to workaround the data losses that we're currently seen if a user includes emoji in data that's written to the db?

Does the emoji and everything after it get stripped on the way in or the way out?

Is there something we can do about that ahead of considering the bigger fix by switching to utf8mb4?

[diox]

Data gets stripped on the way in by MySQL, it only saves the truncated version. Funny enough it's because we're using MySQL in a somewhat relaxed mode - otherwise it would loudly fail and completely prevent saving the content entirely.

I can only see 2 workarounds, both not great IMHO:

• Strip offending characters ourselves, leaving the rest of the data intact. This alters the content that was posted, which is terrible, but maybe less terrible ?
• Transform those characters into HTML entities before saving them in the db. This is not the right layer for this and is going to cause more issues than it solves (need to be careful about not double-escaping when rendering the content, handle returning plain text where needed, etc).

BTW, some bugs about other Mozilla properties that are affected by this where we can find more discussions on the subject:

• MDN (still opened) https://bugzilla.mozilla.org/show_bug.cgi?id=857438
• Bugzilla (fixed, took years) https://bugzilla.mozilla.org/show_bug.cgi?id=1253535
• Marketplace (the one I filed back in the day, never fixed) https://bugzilla.mozilla.org/show_bug.cgi?id=1193265

[muffinresearch]

Stripping emoji is probably better than losing all the data after you enter an emoji. But yeah it's still pretty horrible.

Along the lines of your escaping ideas, maybe we could have some kind of compatibility layer that replaces emoji with something storable on the way in and converts it back on the way out? That is pretty filthy though.

Longer term we should fix the DB tables, we should also consider if Postgres is in our future because if it is, it probably wouldn't be worth doing these changes on MySQL.

Also reading the Marketplace issue - maybe at the point we want to switch to django migrations would be a good point to do this.

[EnTeQuAk]

Longer term we should fix the DB tables, we should also consider if Postgres is in our future because if it is, it probably wouldn't be worth doing these changes on MySQL.

As much as I personally would love us to use PostgreSQL I don't see this happening anytime soon but rather after we switch to Python 3 and Django 2.x in… "next year?"

Also reading the Marketplace issue - maybe at the point we want to switch to django migrations would be a good point to do this.

I think you're referring to…

This would require downtime to do the migration over to utf8mb4. Ideally we would setup a new database server with the new mysql settings and the new schema and then import the data. This would be similar to the maintenance we performed to move from migrations schema to django generated schema.

Ideally we won't do anything like that for switching to django migrations since that'd be crazy hard to do and require a downtime too. But, I personally don't see a reason not to experiment with switching to utf8mb4 and see what steps are required by ops. cc @bqbn and @autrilla for some input on this.

If that can be done with only a bit of downtime - or even readonly time - we should do it and be future proof.

Arguing that this has been around since forever… is this more of a p3 instead of a long-living p2?

[EnTeQuAk]

There is some work on going to at least generate the relevant SQL related to this in https://github.com/adamchainz/django-mysql/pull/356/files - it's not complete yet though (e.g rebuilding composite foreign keys doesn't seem to be supported yet, I haven't checked how many we actually have though, if any)

[muffinresearch]

Arguing that this has been around since forever… is this more of a p3 instead of a long-living p2?

@EnTeQuAk The p2 part of this is that we're losing user's data in a weird way with no feedback and that is breaking things on the frontend because data is being truncated but still looks to have been saved which is then causing the API to return nulls unexpectedly.

In 2018 emoji is extremely common on the web so this should be high priority - _it's really bad to lose data in this way_. The fact that this bug has existed for a long time isn't a good reason to make it lower priority IMHO.

I'd like to see if there's a short-term fix to the data loss issue even if the proper fix is further out. If we think we can go the whole fix sooner then that should also be an option.

I'd definitely welcome more input from @mozilla/addons-ops on what it would take to fix the charsets. If we think we can do that sooner rather than later then that may indeed be preferable over a short-term hack.

[EnTeQuAk]

First run of what would be required to migrate our current database: https://gist.github.com/EnTeQuAk/9e5d151ebb7b1e24ecb18459a9871895 - untested for now though and there's a few indexes on VARCHAR columns that need fixing.

[muffinresearch]

This lib [1] would provide a way to covert to and from :colon: format if we want to seriously consider a transform in and out of MySQL.

[1] https://github.com/carpedm20/emoji

[diox]

Note that the main problem is not writing the migration itself, but figuring out a way to apply it so that:

• Works with no downtime
• Works with our primary/replicas setup (what happens if one replica has one charset and not the others ?)
• Works with data being added during the migration (what charset does it have? will the data work correctly on hosts that have had the migration applied and hosts that haven't ?)
• Synchronizes the client code changes needed so that no data is added with the wrong charset during of ater the migration

[EnTeQuAk]

Note that the main problem is not writing the migration itself

Yeah, I just put together the SQL to get an idea of what actually needs to happen which may help to figure out a migration strategy.

…but figuring out a way to apply it so that:
…

Yeah, I let @bqbn and @autrilla chime in here since they're probably more experienced in that regard. The easiest (well, not easy, but most straightforward) solution would require an actual downtime (not downtime per se, just read-only mode which should be fine for a limited amount of time) because tbh, it's hard to find anything that doesn't require a downtime.

https://mathiasbynens.be/notes/mysql-utf8mb4 has some good reasoning and ideas, there's some documentation from Percona as well. https://dev.mysql.com/doc/refman/5.7/en/charset-unicode-conversion.html is a good read too.

http://blog.manbolo.com/2014/03/31/using-emojis-in-django-model-fields is a good and straightforward list of things that need to happen too.

So, before we can do anything from an ops perspective we actually need to review our schema. We need to reduce the max_length of all CharField columns that have an db_index set or are part of an index_together because with the new encoding we can't index 255 characters but only 191 instead. That'd need dropping and rebuilding these indexes which is intense as well but should be able to do with an online database potentially.

Personally, I think that putting AMO into read-only mode for the time of the migration is a requirement, everything else simply won't work or would be way too complicated to do. With that in mind, we can simply migrate the data, switch configuration stuff and be happy again. I think client-side init connection commands can be extended/forced from the server-side so ops can control it from there, otherwise, we can add something that reacts to environment variables.

[bqbn]

From operations stand point of view, updating the db to use utf8mb4 character set is a large scale change. Some initial reading indicates that the character set change needs to be applied to almost all levels of a db system, i.e. the database server, the db, the tables and the columns, all at the same time. The size of some of the tables we have and the master-slave architecture also make the issue complicated. For example, we don't know how much time it'll take to alter a table, and what happens if master and slaves have different character sets at a point of time, while there are writes to the database, and etc.

I filed a ticket https://bugzilla.mozilla.org/show_bug.cgi?id=1479111 for us to do further research, testing, and gain more facts about this. Then we can come up with a deployment plan to do this.

[autrilla]

I've added the results of the pre-migration checks I performed to https://bugzilla.mozilla.org/show_bug.cgi?id=1479111#c2.

[ioanarusiczki]

While making some tests for https://github.com/mozilla/addons-frontend/issues/6055 and https://github.com/mozilla/addons-frontend/issues/5998 I noticed that on AMO -dev I could not make a review comment using emoji while on AMO stage it is still working for now. So I filed https://github.com/mozilla/addons-server/issues/9263. It could be a regression.

[kumar303]

In https://github.com/mozilla/addons/issues/813 , a user was trying to reply to a review with emoji. The Python exception looks like https://sentry.prod.mozaws.net/operations/olympia-prod/issues/4647262/ so, out of curiosity, I searched Sentry prod for Incorrect string value. It looks like it's only happened in reviews / replies 140 times. Other endpoints got it less frequently but at a glance it seems like not a lot of people are running into this bug (if this exception message is the only one we get).

[bqbn]

FWIW, Sentry keeps 90-day worth of events.

[willdurand]

According to https://github.com/mozilla/addons/issues/813 and https://github.com/mozilla/addons-frontend/issues/6635, mysql does not accept content with an emoji (which was silently removed), it results in a 500 now.

[autrilla]

-dev now supports emoji! (see the review)

[ioanarusiczki]

@autrilla I tested this on AMO dev with FF64 (Win10 & Android 8.0)

Fields in Dev Hub:

• Add-on Description
• Custom License
• End-User License Agreement
• Privacy Policy
• Version Notes

AMO fronend pages:

• collections
• the profile page
• add-on reviews

Except the "display name" from the user profile page which requires at least one printable character the rest of them can be edited using emojis only. All characters are visible.
Also, the scenarios from https://github.com/mozilla/addons-frontend/issues/5649 and https://github.com/mozilla/addons-frontend/issues/5644 are no longer reproducible.

[autrilla]

Awesome! Since this is all working in -dev, I will proceed and deploy this to -stage today.

[ioanarusiczki]

@autrilla I don't see it yet so just let me know when this lands on AMO stage here or on IRC. Thanks!

[autrilla]

@ioanarusiczki this is in -stage now!

[ioanarusiczki]

@autrilla Re-checked AMO stage - FF65 (Win10 & Android 8.0)

Emoji fans will be thrilled. 👍

[EnTeQuAk]

Sentry issue: OLYMPIA-PROD-2B8

[EnTeQuAk]

Sentry issue: OLYMPIA-PROD-2FW

[EnTeQuAk]

Sentry issue: OLYMPIA-PROD-2JW

[EnTeQuAk]

I wonder if https://sentry.prod.mozaws.net/operations/olympia-dev/issues/5169462/ is a regression of that and if we should double-check our max_length validations in various places :thinking:

[diox]

I wonder if https://sentry.prod.mozaws.net/operations/olympia-dev/issues/5169462/ is a regression of that and if we should double-check our max_length validations in various places thinking

We should get QA on it but I wouldn't be surprised...

[AlexandraMoga]

@EnTeQuAk I'm happy to test this further but I need some context. When you say max_length validations do you refer to any fields that has size restrictions, i.e add-on names/summary, display name etc.

[EnTeQuAk]

I wonder if sentry.prod.mozaws.net/operations/olympia-dev/issues/5169462 is a regression of that and if we should double-check our max_length validations in various places thinking

For the record: This isn't a general problem with the utf8mb4 migration but rather only happening because we never strip/validate the referrer and save it directly to the database (here the database is erroring because we have a database-level max length set)

[autrilla]

@ioanarusiczki this has been deployed to production outside of our regular release schedule.

[ioanarusiczki]

@autrilla Yes, thanks, I knew about it and right now I'm doing some sanity testing on AMO prod.

[ioanarusiczki]

@EnTeQuAk @autrilla

• I tested the other days on AMO stage if the limits still apply for text fields with such restrictions. On the front end, Display Name is restricted to 50 chars but requires at least a number or letter, then Location and Occupation to 250 chars. Here emojis are interpreted as one char.
• The fields from Dev Hub interpret the emojis as 2 chars. On AMO dev Name and Summary will require at least one number or letter.
• Addons with emojis in the Name and Summary fields in the manifest can be submitted.
  
  
  
  • I've noticed two edge cases and I filed them here https://github.com/mozilla/addons-server/issues/10794 and here https://github.com/mozilla/addons-server/issues/10795.
  
  
• I tested today on AMO prod. Looks good 👍