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 Baptiste Mispelon, 10 years ago

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 by Harper04, 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 Markus Holtermann, 10 years ago

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 by Andrew Godwin, 10 years ago

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 by Stavros Korokithakis, 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 Michael Barr, 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. I would be much easier for 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,
        )
    ]

Version 6, edited 10 years ago by Michael Barr (previous) (next) (diff)

comment:7 by Danilo Bargen, 10 years ago

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 by Tim Graham, 10 years ago

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

comment:9 by Rowan Seymour, 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 Luke Plant, 10 years ago

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 by Tim Graham, 10 years ago

Summary: Migrations call callable defaults only onceAdd makemigrations warning for unique fields with callable defaults
Type: BugNew feature
Version: 1.7master

comment:12 by max13fr, 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

Last edited 5 years ago by max13fr (previous) (diff)

comment:13 by Jacob Walls, 3 years ago

Owner: changed from nobody to Jacob Walls
Status: newassigned

comment:14 by Jacob Walls, 3 years ago

Has patch: set

comment:15 by Jacob Walls, 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 Jacob Walls, 3 years ago

Patch needs improvement: unset

comment:17 by Mariusz Felisiak, 3 years ago

Triage Stage: AcceptedReady for checkin

comment:18 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In 47f791f:

Fixed #23408 -- Added migrations questioner prompt for adding unique fields with a callable default.

Note: See TracTickets for help on using tickets.
Back to Top