search

milesmcc / shynet

Modern, privacy-friendly, and detailed web analytics that works without cookies or JS.
Apache License 2.0
2.89k stars 183 forks source link

Add User.api_token migration #235

Closed haplo closed 1 year ago

haplo commented 1 year ago

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 787ce1775f5bd5699.

milesmcc commented 1 year ago

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

haplo commented 1 year ago

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.

https://github.com/milesmcc/shynet/commit/787ce1775f5bd56991a47eb6218e29afcd93d813#diff-2ae8edd0c5dcc60a4f98b32e7a2ede51f213275567ae72c33356e8e6f0d18acaR14-R18

haaavk commented 1 year ago

Migration was edited manually in c34388e6c9d08f7974f36407444716f07c79dc5b. 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.