Opened 10 years ago
Closed 3 years ago
#23408 closed New feature (fixed)
Add makemigrations warning for unique fields with callable defaults
Reported by: | Harper04 | Owned by: | Jacob Walls |
---|---|---|---|
Component: | Migrations | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Markus Holtermann, mail@… | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Callables on properties for ModelFields are used for various reasons. One use case is to autocreate random file names or user passwords if not present.
The migration seems to call them only once because after the migration every "Buchung" has the same wlan_password.
My Model:
def random_wlan_key(): return ''.join(random.SystemRandom().choice("1234567890abcdefghkmnpqrstuvwxyz") for i in range(9)) class Buchung(models.Model): [...] wlan_passwort = models.CharField(max_length=10, default=random_wlan_key)
The generated migration:
# -*- coding: utf-8 -*- from __future__ import unicode_literals from django.db import models, migrations import main.models class Migration(migrations.Migration): dependencies = [ ('main', '0001_initial'), ] operations = [ migrations.AddField( model_name='buchung', name='wlan_passwort', field=models.CharField(default=main.models.random_wlan_key, max_length=10), preserve_default=True, ), ]
Change History (18)
comment:1 by , 10 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 10 years ago
Hi,
thanks for your time.
I would argue that this behavior is not intended as best practice to create fields like "created_at" fields is to assign a callable.
Calling it only once during a migration is killing dynamic behavior. What if the default value of this field is dependent on another one?
regards
Tom
comment:3 by , 10 years ago
Cc: | added |
---|
I wouldn't consider this a bug as the default value is nothing that is enforced on database level but inside Django. If you want to have random values per row, you need to add a RunPython operation that updates the value per row.
comment:4 by , 10 years ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
This is exactly as designed; when creating columns databases only take a single argument for the DEFAULT clause, so we can only provide a single value, so we only call a callable once. If you want unique values per row, you'll need to populate them in RunPython as Markush2010 suggests.
comment:5 by , 10 years ago
I do indeed want unique values per row, and will populate them myself, but, in that case, I don't want Django setting the default and having me wonder down the line why the rows for which I didn't specify a value have a default of 2014-06-24. The correct behavior there would be to ignore the default completely, since there's no reasonable use case where it's supposed to call the callable once and use it for all the columns.
comment:6 by , 10 years ago
Callables on properties for ModelFields are used for various reasons. One use case is to autocreate UUIDs (https://docs.djangoproject.com/en/dev/ref/models/fields/#django.db.models.UUIDField).
My Models:
import uuid class AbstractUUIDModel(models.Model): [...] uuid = models.UUIDField(default=uuid.uuid4, editable=False, max_length=32, help_text='Universally unique identifier', unique=True, verbose_name='UUID') class Meta: abstract = True class Company(AbstractUUIDModel): pass
The generated migration:
# -*- coding: utf-8 -*- from __future__ import unicode_literals from django.db import models, migrations import main.models import uuid class Migration(migrations.Migration): dependencies = [ ('main', '0001_initial'), ] operations = [ migrations.AddField( model_name='company', name='uuid', field=models.UUIDField(default=uuid.uuid4, editable=False, max_length=32, help_text='Universally unique identifier', unique=True, verbose_name='UUID'), preserve_default=True, ), ]
Results:
django.db.utils.IntegrityError: could not create unique index "company_company_uuid_key" DETAIL: Key (uuid)=(1ba0e330-9786-4928-b89d-b1fbef72b9c1) is duplicated.
Based on the documentation, you should add the default=uuid.uuid4 on the models.UUIDField. However, you cannot do this with the current way migrations work. Django currently tries to generate the same UUID for each instance that is created. In this instance (Django 1.8 isn't released yet), you will run into issues:
import uuid from django.db import models class MyUUIDModel(models.Model): id = models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False) # other fields
If default is set to a callable, I believe that it is implied that the result should be dynamically generated in Python/Django. If the functionality is to stay as stated in the result of this ticket, I would have to hand-code this migration for each app that previously existed to first create a nullable UUIDField with no default, then create a data migration to add distinct UUIDs to the Company, then write a migrations.AlterField migration to set null=False. The alternative might be to consider creating a special case for the UUIDField to allow for non-duplicated UUIDs to be created during migrations.
From what I can take away, it has been stated that you cannot add/alter a field with a default=callable, null=False, unique=True without writing a custom migration. It would be much easier if migrations followed the same standards as the django.db.models.fields.Field.get_default() method. Currently, we have to hand-code the following migration to avoid the failure for every app which utilizes the AbstractUUIDModel, which I feel violates the DRY principle:
# -*- coding: utf-8 -*- from __future__ import unicode_literals from django.db import models, migrations import uuid def generate_uuid(apps, schema_editor): Company = apps.get_model('company', 'Company') for company in Company.objects.all().iterator(): company.uuid = uuid.uuid4() company.save() class Migration(migrations.Migration): dependencies = [ ('main', '0001_initial'), ] operations = [ migrations.AddField( model_name='company', name='uuid', field=models.UUIDField(editable=False, max_length=32, help_text='Universally unique identifier', null=True, verbose_name='UUID'), preserve_default=False, ), migrations.RunPython( generate_uuid, ), migrations.AlterField( model_name='company', name='uuid', field=models.UUIDField(editable=False, max_length=32, help_text='Universally unique identifier', unique=True, verbose_name='UUID'), preserve_default=True, ) ]
comment:7 by , 10 years ago
Cc: | added |
---|
I just ran into the same issue (trying to set default UUIDs on an UUID field). Using per-row callables as default value in migrations would be very useful, although it should probably be specified in an explicit way.
comment:8 by , 10 years ago
Summary: | Automaticly created migration calls callable for default only once → Migrations call callable defaults only once |
---|
comment:9 by , 10 years ago
I also ran into this problem with a UUID field. I would have thought that this is a sufficiently common use case to be catered for in core. But core devs feel that the onus is on people to write custom 3-step migrations each time, then makemigrations should at least show a warning. I'd imagine that in most cases, a single value for all rows is not what the user is expecting. If makemigrations sees unique=True on the field then it knows that the migration will fail.
comment:10 by , 10 years ago
Resolution: | invalid |
---|---|
Status: | closed → new |
It seems reasonable to me that makemigrations
should, at the very least, issue a warning when it finds a field with a default
which is callable and unique==True
, and probably for all callable defaults when the field is being added. I'm re-opening on that basis, hope that's OK with other devs.
I think an ideal solution is that the warning should reference some shortcut/info for implementing the needed migration.
comment:11 by , 10 years ago
Summary: | Migrations call callable defaults only once → Add makemigrations warning for unique fields with callable defaults |
---|---|
Type: | Bug → New feature |
Version: | 1.7 → master |
The warning could point to this documentation: https://docs.djangoproject.com/en/dev/howto/writing-migrations/#migrations-that-add-unique-fields
comment:12 by , 5 years ago
Hello,
I'm still running this issue on Django five years later, is there any hope to see this issue fixed ?
I don't understand why default id with primary key (so is unique too) doesn't have the same issue.
Default values are normally used only when we are not providing a value (or providing None value), so I don't understand why when I specify a value, Django just uses it instead of replace it with default value.
What I'm missing ?
If needed here my example:
api/models.py :
class Model Test(models.Model): id = models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False) name = models.CharField(max_length=100)
api/fixtures/initial_data.json :
{ "model": "api.Test", "pk": "724ea599-10a2-49a1-be2f-c1fcb04d003e", "fields": { "name": "Test 1" } }, { "model": "api.Test", "pk": "6bcb72f0-099b-416d-8b2c-fcd0bef76cbf", "fields": { "name": "Test 2" } }
python manage.py loaddata initial_data
django.db.utils.IntegrityError: Problem installing fixture 'api/fixtures/initial_data.json': Could not load api.Test(pk=724ea599-10a2-49a1-be2f-c1fcb04d003e): UNIQUE constraint failed: api_test.id
Thanks in advance
Max
EDIT: oh my bad it's not the same issue (here it's about makemigrations / migrate instead of loaddata fixture in my case).
Create a dedicated issue here : https://code.djangoproject.com/ticket/31531#ticket
comment:13 by , 3 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:15 by , 3 years ago
Patch needs improvement: | set |
---|
Marking PNI until I can move this over the to the interactive questioner per Simon's review.
comment:16 by , 3 years ago
Patch needs improvement: | unset |
---|
comment:17 by , 3 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Hi,
I can reproduce this indeed. At this point, I'm not sure if this behavior is by design or not.
If not, we should definitely fix this and if it is, we should document it.
Thanks.