Opened 9 years ago

Last modified 8 months ago

#26223 new Bug

Squashing migrations with preserve_default=False keeps the default

Reported by: Bartek Wójcicki Owned by:
Component: Migrations Version: dev
Severity: Normal Keywords:
Cc: Markus Holtermann, InvalidInterrupt, Ülgen Sarıkavak Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Bartek Wójcicki)

I have a migration with AlterField with default value provided and preserve_default=False. When I squash it, I got a CreateModel operation with default value for this field set.

Steps to reproduce:

  1. Create a model, make migrations. 
    class FooBar(models.Model):
    pass
  1. Add field without default, make migrations, provide a one-off default. 
    class FooBar(models.Model):
    foo = models.TextField()
  1. Squash migrations.

Squashed migration keeps the default in CreateModel operation.

  operations = [
      migrations.CreateModel(
          name='FooBar',
          fields=[
              ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
              ('foo', models.TextField(default='bar')),
          ],
      ),
  ]

Running makemigrations again creates a migration with AlterField, removing the default.

  operations = [
      migrations.AlterField(
          model_name='foobar',
          name='foo',
          field=models.TextField(),
      ),
  ]

Change History (13)

comment:1 by Bartek Wójcicki, 9 years ago

Description: modified (diff)

comment:2 by Tim Graham, 9 years ago

Triage Stage: Unreviewed → Accepted

comment:3 by Vytis Banaitis, 8 years ago

Owner: changed from nobody to Vytis Banaitis
Status: new → assigned

I have analyzed the migration optimizer code and identified 5 cases where preserve_default is not handled correctly.

>>> optimize([
...     migrations.CreateModel("Foo", [("name", models.CharField(max_length=255))]),
...     migrations.AddField("Foo", "value", models.IntegerField(default=42), preserve_default=False),
... ])
[
    migrations.CreateModel(
        name='Foo',
        fields=[
            ('name', models.CharField(max_length=255)),
            ('value', models.IntegerField(default=42)),
        ],
    ),
]

>>> optimize([
...     migrations.CreateModel("Foo", [("value", models.IntegerField(null=True))]),
...     migrations.AlterField("Foo", "value", models.IntegerField(default=42), preserve_default=False),
... ])
[
    migrations.CreateModel(
        name='Foo',
        fields=[
            ('value', models.IntegerField(default=42)),
        ],
    ),
]

>>> optimize([
...     migrations.AddField("Foo", "value", models.IntegerField(null=True)),
...     migrations.AlterField("Foo", "value", models.IntegerField(default=42), preserve_default=False),
... ])
[
    migrations.AddField(
        model_name='Foo',
        name='value',
        field=models.IntegerField(default=42),
    ),
]

>>> optimize([
...     migrations.AddField("Foo", "value", models.IntegerField(default=42), preserve_default=False),
...     migrations.RenameField("Foo", "value", "price"),
... ])
[
    migrations.AddField(
        model_name='Foo',
        name='price',
        field=models.IntegerField(default=42),
    ),
]

>>> optimize([
...     migrations.AlterField("Foo", "value", models.IntegerField(default=42), preserve_default=False),
...     migrations.RenameField("Foo", "value", "price"),
,,, ])
[
    migrations.RenameField(
        model_name='Foo',
        old_name='value',
        new_name='price',
    ),
    migrations.AlterField(
        model_name='Foo',
        name='price',
        field=models.IntegerField(default=42),
    ),
]

For the cases involving CreateModel, I intend to add a new property real_field (suggestions for a better name are welcome) to AddField/AlterField which would return a possibly modified field after taking preserve_default into account. It would be the same field that these operations put into state. Then CreateModel could use the new property in its optimization.

For the other cases, proper passing of preserve_default should be sufficient.

Version 0, edited 8 years ago by Vytis Banaitis (next)

comment:4 by Simon Charette, 8 years ago

Version: 1.8 → master

I'm also not sure about about the real_field's naming but that sounds like the right approach to me.

comment:5 by Vytis Banaitis, 8 years ago

Has patch: set

​PR

Went with actual_field instead of real_field.

comment:6 by Vytis Banaitis, 8 years ago

Here's an alternative approach.

AddField.field could be changed to mean the actual field that goes into model state. The transient database default value would be a separate attribute on the operation.
This would make code a bit cleaner because default handling would only occur in database_forwards (where it already occurs).
For backwards compatibility, constructor would still accept preserve_default and modify the field if needed.
makemigrations and squashmigrations would emit the operation with the new signature.
If preserve_default is deprecated, we can suggest users to modify existing migrations by hand or simply squash them.

Same goes for AlterField.

If this approach is worth considering, I could make another PR to see how it looks.

comment:7 by Vytis Banaitis, 8 years ago

I found another faulty optimization for AddField+AlterField:

>>> optimize([
...     migrations.AddField("Foo", "value", models.IntegerField(default=42), preserve_default=False),
...     migrations.AlterField("Foo", "value", models.IntegerField("Value")),
... ])
[
    migrations.AddField(
        model_name='Foo',
        name='value',
        field=models.IntegerField(verbose_name='Value'),
    ),
]

PR updated to deal with it.

comment:8 by Tim Graham, 8 years ago

Cc: Markus Holtermann added

Markus, any thoughts about the approach in the current PR vs. the alternative suggested in comment:6?

comment:9 by Vytis Banaitis, 8 years ago

I have a WIP patch for the approach from comment:6, if anyone's interested: ​https://github.com/vytisb/django/pull/1

comment:10 by Tim Graham, 7 years ago

Patch needs improvement: set

comment:11 by InvalidInterrupt, 4 years ago

Cc: InvalidInterrupt added

comment:12 by Mariusz Felisiak, 20 months ago

Owner: Vytis Banaitis removed
Status: assigned → new

comment:13 by Ülgen Sarıkavak, 8 months ago

Cc: Ülgen Sarıkavak added
Note: See TracTickets for help on using tickets.
Back to Top