Opened 2 years ago

Last modified 21 months ago

#23408 new New feature

Add makemigrations warning for unique fields with callable defaults

Reported by: Harper04 Owned by: nobody
Component: Migrations Version: master
Severity: Normal Keywords:
Cc: Markus Holtermann, mail@… Triage Stage: Accepted
Has patch: no 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 (11)

comment:1 Changed 2 years ago by Baptiste Mispelon

Triage Stage: UnreviewedAccepted

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.

comment:2 Changed 2 years ago by Harper04

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 Changed 2 years ago by Markus Holtermann

Cc: Markus Holtermann 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 Changed 2 years ago by Andrew Godwin

Resolution: invalid
Status: newclosed

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 Changed 2 years ago by Stavros Korokithakis

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 Changed 2 years ago by michaeljohnbarr

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,
        )
    ]

Last edited 2 years ago by michaeljohnbarr (previous) (diff)

comment:7 Changed 2 years ago by Danilo Bargen

Cc: mail@… 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 Changed 2 years ago by Tim Graham

Summary: Automaticly created migration calls callable for default only onceMigrations call callable defaults only once

comment:9 Changed 2 years ago by Rowan Seymour

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 Changed 21 months ago by Luke Plant

Resolution: invalid
Status: closednew

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 Changed 21 months ago by Tim Graham

Summary: Migrations call callable defaults only onceAdd makemigrations warning for unique fields with callable defaults
Type: BugNew feature
Version: 1.7master
Note: See TracTickets for help on using tickets.
Back to Top