Skip to content
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

Add User.api_token migration #235

Closed
wants to merge 1 commit into from

Conversation

haplo
Copy link
Contributor

@haplo haplo commented Oct 10, 2022

I was working on #215 and noticed that manage.py makemigrations was picking up a change for User.api_token. It looks like it was added in commit 787ce17.

@milesmcc
Copy link
Owner

Just want to confirm — will this work correctly on databases with multiple users? Dynamic defaults combined with unique constraints always make me nervous. :)

@haplo
Copy link
Contributor Author

haplo commented Oct 11, 2022

Just want to confirm — will this work correctly on databases with multiple users? Dynamic defaults combined with unique constraints always make me nervous. :)

Ah, I don't know, I haven't tested it on a live multi-user deploy.

I was checking the referenced commit and noticed that there is already a migration adding the User.api_token field, so why would Django be trying to alter it again? There might be something fishy going on, let me research it a bit.

787ce17#diff-2ae8edd0c5dcc60a4f98b32e7a2ede51f213275567ae72c33356e8e6f0d18acaR14-R18

@haplo haplo closed this Oct 11, 2022
@haaavk
Copy link
Contributor

haaavk commented Oct 11, 2022

Migration was edited manually in c34388e.
Default value was removed from migration but it is present in model.
I think that's why Django generated new migration.
Default value for api token is 32 long random string and it shouldn't cause problems with multiple users.
On the other hand, most users won't use api token so it may be a good idea to keep it disabled by default.
In that case default value should be removed from model.

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