Opened 11 years ago

Last modified 3 weeks ago

#26223 assigned Bug

Squashing migrations with preserve_default=False keeps the default

Reported by: Bartek Wójcicki Owned by: Shadow_Lu
Component: Migrations Version: dev
Severity: Normal Keywords:
Cc: Markus Holtermann, InvalidInterrupt, Ülgen Sarıkavak, Roman Donchenko Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
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 (15)

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

Description: modified (diff)

comment:2 by Tim Graham, 11 years ago

Triage Stage: Unreviewed → Accepted

comment:3 by Vytis Banaitis, 10 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.

Last edited 10 years ago by Vytis Banaitis (previous) (diff)

comment:4 by Simon Charette, 10 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, 10 years ago

Has patch: set

​PR

Went with actual_field instead of real_field.

comment:6 by Vytis Banaitis, 10 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, 10 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, 10 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, 10 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, 9 years ago

Patch needs improvement: set

comment:11 by InvalidInterrupt, 6 years ago

Cc: InvalidInterrupt added

comment:12 by Mariusz Felisiak, 3 years ago

Owner: Vytis Banaitis removed
Status: assigned → new

comment:13 by Ülgen Sarıkavak, 2 years ago

Cc: Ülgen Sarıkavak added

comment:14 by Roman Donchenko, 10 months ago

Cc: Roman Donchenko added

comment:15 by Shadow_Lu, 3 weeks ago

Owner: set to Shadow_Lu
Patch needs improvement: unset
Status: new → assigned

Taking this on. PR ​https://github.com/django/django/pull/21682 covers the case in
the description: CreateModel.reduce() folded AddField/AlterField in using the
operation's field, ignoring preserve_default, so a one-off default became part of
the model state. Squashing kept the default and makemigrations then generated an
AlterField to remove it again. AddField.reduce() also kept its own
preserve_default instead of the one from the AlterField it absorbed.

Not covered: the cases in comments 3 and 7, where AddField/AlterField with
preserve_default=False merges into a later AlterField that has no default. The
resulting state is correct there, but the one-off default that populated existing
rows is dropped. Carrying an old one-off default onto a differently shaped field
seemed like a separate decision, so I left it out.

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