timeflake icon indicating copy to clipboard operation
timeflake copied to clipboard

Subclass TimeflakePrimaryKeyBinary from UUIDfield

Open sebst opened this issue 3 years ago • 6 comments

UUIDField is build into Django and should therefore be preferred as it will likely increase compat with other third party libraries such as DRF.

See previous issue #8 .

However, doing a subclassing now would likely result in migration incompatible changes (as the db type may change and foreign key relations could point to this). This would justify a Major Version increase according to SemVer even if only the django extension would be affected.

So, imho, there are two options:

  1. Do the subclass now and increase to 1.0.0
  2. Remove the django extension and make a new Python package like django-timeflake.

What do you think, @anthonynsimon ?

sebst avatar Apr 19 '21 13:04 sebst

I think subclassing and doing a major version bump would be best. My main worry would be if many people already rely on this, it might lead to a painful migration (UUIDField might not map 1:1 with the current types used in DBs).

For example, while in Postgres Timeflake uses the native uuid type, in MySQL and in SQLite it uses binary types. We'd need to ensure that the UUIDField built into Django translates properly in those databases too.

Ref in Timeflake:

https://github.com/anthonynsimon/timeflake/blob/master/timeflake/extensions/django/init.py#L42

In Django's codebase:

https://github.com/django/django/blob/ca9872905559026af82000e46cde6f7dedc897b6/django/db/models/fields/init.py#L2420

anthonynsimon avatar Apr 22 '21 16:04 anthonynsimon

You're right, the concern about painful migrations is exactly what would justify the major version increase.

A migration path for the timeflake package might be:

  1. Mark the current (pre-PR) TimeflakeBinary deprecated, support the 0.x.x branch for some time.
  2. Rename TimeflakeBinary in the PR to TimeflakeUUID and keep the TimeflakeBinary, but raise a deprecation warning on usage.
  3. 2.x.x should then remove the django extension entirely and provide a separate timeflake-django package.

Another option would could be (because we already subclassed Timeflake from UUID) to just keep things as they are and just update the documentation, such that users are advised to use a primary key like this

from timeflake import random

class MyModel(models.Model):
    id = models.UUIDField(primary_key=True, editable=False, default=random)

sebst avatar Apr 23 '21 11:04 sebst

Hey thanks for the suggestions!

Maybe it makes sense to instead provide different classes for the different encodings? That way we keep full backwards compatibility (no changes required for existing users), while better supporting the ecosystem (DRF, admin extensions, and so on).

For example, we could introduce two new fields: TimeflakeBase62Field, and TimeflakeUUIDField, while keeping TimeflakeBinaryField.

That way users could pick the encoding that best fits their needs, given the pros and cons of each (TimeflakeUUIDField would work out of the box with stuff that already handles standard UUID fields, while TimeflakeBinaryField not).

As a plus, it would also be consistent with the peewee integration here: https://github.com/anthonynsimon/timeflake/blob/master/timeflake/extensions/peewee/init.py

What do you think?

anthonynsimon avatar Apr 30 '21 16:04 anthonynsimon

I will also alias the existing TimeflakeBinary with TimeflakeBinaryField to make this consistent with Django :)

anthonynsimon avatar Apr 30 '21 16:04 anthonynsimon

Yes, that sounds great

sebst avatar May 15 '21 20:05 sebst

Hey do you need any assistance pushing this through? Looks like there's a PR out with the implementation already.

RobRoseKnows avatar Jun 15 '21 19:06 RobRoseKnows