yunojuno / django-charid-field

Provides a prefixable, string-based ID field for your Django models. Supports cuid, ksuid, ulid & more.
MIT License
33 stars 4 forks source link

Skip the automatic unique index setting #11

Open georgeyk opened 10 months ago

georgeyk commented 10 months ago

The proposed change is related to https://github.com/jazzband/django-simple-history When creating an historical table, the fields are copied and adjusted, so it can reflect the fields properly. For the id field, that mostly means it's no longer unique, since the historical table is tracking changes of a single record. I'm guessing that when generating migrations (using deconstruct), it will see "unique=True" that wasn't really there. So, in short, I believe that is just easier to do the same as the regular CharField, instead of fixing deconstruct, but your call.

poc looks like this:

 >>> from charidfield import CharIDField
 >>> f = CharIDField(primary_key=True)
 >>> f.deconstruct()
(None, 'charidfield.fields.CharIDField', [], {'primary_key': True, 'unique': True, 'prefix': '', 'default': <class 'django.db.models.fields.NOT_PROVIDED'>})
 >>> f.primary_key = False
 >>> f.deconstruct()
(None, 'charidfield.fields.CharIDField', [], {'unique': True, 'prefix': '', 'default': <class  
'django.db.models.fields.NOT_PROVIDED'>})
 >>> from django.db.models import CharField
 >>> g = CharField(primary_key=True)
 >>> g.deconstruct()
(None, 'django.db.models.CharField', [], {'primary_key': True})
 >>> g.primary_key = False
 >>> g.deconstruct()
(None, 'django.db.models.CharField', [], {})
 >>>
djm commented 10 months ago

Thanks for the PR but it'd unlikely we'd be able to accept it. Django convention for ID related fields is to create a unique index by default, and this library follows that pattern.

If you wish to disable the unique index - the idea is you explicitly disable it with unique=False; I'm not familiar with the package you linked, is there a reason that does not work?

georgeyk commented 10 months ago

@djm well, that's unfortunate, but i get it. tks It does not work because of the unique=True is injected in the field definition, and after setting primary=False, and calling deconstruct, the unique=True is there, even though I didn't add that (because with primary_key=True, by definition is unique).

georgeyk commented 10 months ago

also, idk what are you using as a convention to force this, but UUIDField for instance doesn't do this, nor Big/Small AutoFields.

djm commented 10 months ago

UUIDField for instance doesn't do this, nor Big/Small AutoFields.

This is true. We chose to ship the default as True largely because this library is designed to be used as a replacement primary key but that definitely isn't the only use case.

It does not work because of the unique=True is injected in the field definition, and after setting primary=False, and calling deconstruct, the unique=True is there, even though I didn't add that (because with primary_key=True, by definition is unique).

This I need to understand more. It looks like django-simple-history deliberately takes action to remove the unique constraint from the new tables it creates but what I don't understand yet is why this is not working for you.

I'm guessing that when generating migrations (using deconstruct), it will see "unique=True" that wasn't really there.

Have you got any code to show what it does? Or a small replication project to show the problem better?

We're open to changing the default but it would naturally be a breaking change for this project so we must do it for the right reasons.

Thanks!

georgeyk commented 10 months ago

@djm here you go: https://github.com/georgeyk/foo/blob/main/demo/foo/migrations/0001_initial.py#L47 You can remove the migration, and regenerate it if you want. If you want to see the integrity error, just create a record and update it. Let me know when i can drop the repo. tks

djm commented 10 months ago

Thanks @georgeyk, we'll look into this and I'll give an update soon.

ezarowny commented 5 months ago

I'm not seeing a way to disable the unique setting either at the partial level or at the model level. Neither seem to disable the unique flag which is really annoying for making a series of migration that adds a new unique field. Instead of following the directions in Django's documentation, you'll need to remove the default from the first migration. It's not the end of the world but that's going to confuse a lot of people.