Skip to content

Skip the automatic unique index setting #11

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

georgeyk
Copy link

@georgeyk georgeyk commented Sep 3, 2023

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
Copy link
Contributor

djm commented Sep 3, 2023

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
Copy link
Author

georgeyk commented Sep 4, 2023

@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
Copy link
Author

georgeyk commented Sep 4, 2023

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
Copy link
Contributor

djm commented Sep 4, 2023

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
Copy link
Author

georgeyk commented Sep 5, 2023

@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
Copy link
Contributor

djm commented Sep 5, 2023

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

@ezarowny
Copy link

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.

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.

3 participants