Opened 10 months ago

Last modified 4 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: Markush2010, 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 10 months ago by bmispelon

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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 10 months 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 10 months ago by Markush2010

  • Cc Markush2010 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 10 months ago by andrewgodwin

  • Resolution set to invalid
  • Status changed from new to 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 Changed 10 months ago by stavros

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 7 months 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 7 months ago by michaeljohnbarr (previous) (diff)

comment:7 Changed 7 months ago by dbrgn

  • 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 7 months ago by timgraham

  • Summary changed from Automaticly created migration calls callable for default only once to Migrations call callable defaults only once

comment:9 Changed 7 months ago by rowanseymour

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 4 months ago by spookylukey

  • Resolution invalid deleted
  • Status changed from closed to 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 Changed 4 months ago by timgraham

  • Summary changed from Migrations call callable defaults only once to Add makemigrations warning for unique fields with callable defaults
  • Type changed from Bug to New feature
  • Version changed from 1.7 to master
Note: See TracTickets for help on using tickets.
Back to Top