Opened 22 months ago

Last modified 6 months ago

#26223 assigned Bug

Squashing migrations with preserve_default=False keeps the default

Reported by: Bartek Wójcicki Owned by: Vytis Banaitis
Component: Migrations Version: master
Severity: Normal Keywords:
Cc: Markus Holtermann 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 (10)

comment:1 Changed 22 months ago by Bartek Wójcicki

Description: modified (diff)

comment:2 Changed 22 months ago by Tim Graham

Triage Stage: UnreviewedAccepted

comment:3 Changed 18 months ago by Vytis Banaitis

Owner: changed from nobody to Vytis Banaitis
Status: newassigned

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 18 months ago by Vytis Banaitis (previous) (diff)

comment:4 Changed 18 months ago by Simon Charette

Version: 1.8master

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

comment:5 Changed 18 months ago by Vytis Banaitis

Has patch: set

PR

Went with actual_field instead of real_field.

comment:6 Changed 18 months ago by Vytis Banaitis

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 Changed 18 months ago by Vytis Banaitis

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

Cc: Markus Holtermann added

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

comment:9 Changed 17 months ago by Vytis Banaitis

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

comment:10 Changed 6 months ago by Tim Graham

Patch needs improvement: set
Note: See TracTickets for help on using tickets.
Back to Top