unique registration_id as textfield broken on mysql about fcm-django HOT 11 CLOSED

If you could add some tests, that would be 10/10.

Also, you don't really need to do any checking, this package will update the DB entry upon duplicate registration id (see

The unique index is more of an optimization, so it's fine if for mysql it isn't there at all.

from fcm-django.

I've had a look at this. From a database agnostic view it would be better to use a CharField because we can add unique to it and not care about which database it runs on. For FCM registration_ids a better fit is TextField because of the quote below from Google engineer Mr Johns.

Trevor Johns is likely someone to follow the advice from:

The documentation doesn't specify any pattern, therefore any valid string is allowed. The format may change in the future; please do not validate this input against any pattern, as this may cause your app to break if this happens.

As with the "registration_id" field, the upper bound on size is the max size for a cookie, which is 4K (4096 bytes).
https://stackoverflow.com/a/12502351

Now, if we opt for using CharField, this would require an explicit rollback of the migrations to pre-unique, then remove old migration path and apply an alter migration. This is tedious as hell for end users of a library.

@xtrinch I'm open for any ideas and can write the change once your happy with the path to take.

from fcm-django.

@henriklindgren We need solve this in a backward compatible way. We can modify the existing migration to do nothing on mysql and just leave it as non-unique (since 255 max length for CharField is not enough). Not sure if this will try to generate new migrations for mysql users, and not sure if we can somehow skip that (Maybe managed=False on the model?)..

What do you think?
I found some insight here https://stackoverflow.com/questions/32384547/django-migration-runsql-conditional-on-database-type

from fcm-django.

A migration that’s part of a released version should not be changed, or it will cause trouble and require manual steps for people who are already on that version.

Let’s call the existing migration that doesn’t work on mysql M1.
What you could do is change the model to generate a migration M2 that undoes the changes, change the model to change the field in the way that achieves all goals (size and uniqueness) and get a migration M3 for that, then create a squashed migration M4 that’s equivalent to M1-M2-M3. People already on M1 will be able to run the intermediate migrations, and people who have the previous version will migrate directly to M4.

from fcm-django.

A migration that’s part of a released version should not be changed, or it will cause trouble and require manual steps for people who are already on that version.

Let’s call the existing migration that doesn’t work on mysql M1.
What you could do is change the model to generate a migration M2 that undoes the changes, change the model to change the field in the way that achieves all goals (size and uniqueness) and get a migration M3 for that, then create a squashed migration M4 that’s equivalent to M1-M2-M3. People already on M1 will be able to run the intermediate migrations, and people who have the previous version will migrate directly to M4.

It cant cause problems if all we do is skip the existing migrations content for mysql, as mysql users cannot run it.

There also does not exist an alternative that covers the size and uniqueness constraints for mysql. Char field is too small and text field cannot be made unique.

So the best option is to not penalize other users for mysql's + django lack of support for this and enable the uniqueness of the text field to work for everybody else.

from fcm-django.

Some more limits to add to the discussion,

unique key length can't be longer than 3072 bytes in MySQL 5, not sure about 8
https://dev.mysql.com/doc/refman/5.7/en/innodb-limits.html

So it would not be possible to fit and verify uniqueness of a 4096 bytes key with the database in any case for MySQL, regardless of Django. I wonder if the package could be evolved to get installed with extras for special case databases (i.e. fcm_django[mysql]). I agree with the last sentiment, the restrictions of MySQL are not fcm_django's responsibility in its core form. As fcm_django has to be somewhat performant, it feels like a downgrade to do "manual" unique checking as a workaround IMO.

I'm thinking of having a look on adding tests, feels like the split of database behaviour needs to be locked down with some tests.

Good discussion, thanks for putting in the effort peeps 🙇

from fcm-django.

Having trouble finding the time to test it, maybe later this evening, this is the WIP.

master...henriklindgren:fcm-django:bugfix/issue236

The idea was to skip database migration state, but keep app state moving forward. From what I've understood the unique flag should not cause issues even if we skip it, but I need to verify it all. Soon come :)

from fcm-django.

Awesome!
@henriklindgren It might generate migrations when mysql users run makemigrations, this needs to be somehow avoided if yes

from fcm-django.

Tested migration behaviour, makemigrations is not triggered, wrote specifics on what's tested at the moment in the PR #238

from fcm-django.

Fixed in 1.0.15

from fcm-django.

Hey there! I see this was fixed last month but I encountered this issue when I derived my model from AbstractFCMDevice and ran migrations.

I'm using MySQL. Any ideas on how I can work around this? Thanks!

from fcm-django.