[Django] #26223: Squashing migrations with preserve_default=False keeps the default
Django
--------------------------------+--------------------
Reporter: bartekwojcicki | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.8
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+--------------------
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.
{{{
#!div style="font-size: 80%"
{{{#!python
class FooBar(models.Model):
pass
}}}
}}}
2. Add field without default, make migrations, provide a one-off default.
{{{
#!div style="font-size: 80%"
{{{#!python
class FooBar(models.Model):
foo = models.TextField()
}}}
}}}
3. Squash migrations.
Squashed migrations keeps the default in CreateModel operation.
{{{
#!div style="font-size: 80%"
{{{#!python
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
{{{
#!div style="font-size: 80%"
{{{#!python
operations = [
migrations.AlterField(
model_name='foobar',
name='foo',
field=models.TextField(),
),
]
}}}
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26223>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: bartekwojcicki | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.8
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by bartekwojcicki):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Old description:
New description:
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.
{{{
#!div style="font-size: 80%"
{{{#!python
class FooBar(models.Model):
pass
}}}
}}}
2. Add field without default, make migrations, provide a one-off default.
{{{
#!div style="font-size: 80%"
{{{#!python
class FooBar(models.Model):
foo = models.TextField()
}}}
}}}
3. Squash migrations.
Squashed migration keeps the default in CreateModel operation.
{{{
#!div style="font-size: 80%"
{{{#!python
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.
{{{
#!div style="font-size: 80%"
{{{#!python
operations = [
migrations.AlterField(
model_name='foobar',
name='foo',
field=models.TextField(),
),
]
}}}
}}}
--
--
Ticket URL: <https://code.djangoproject.com/ticket/26223#comment:1>
Django
Reporter: bartekwojcicki | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/26223#comment:2>
Django
Reporter: bartekwojcicki | Owner: vytisb
Type: Bug | Status: assigned
Component: Migrations | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by vytisb):
* owner: nobody => vytisb
* status: new => assigned
Comment:
I have analyzed the migration optimizer code and identified 5 cases where
`preserve_default` is not handled correctly.
{{{
#!div style="font-size: 80%"
{{{#!python
>>> 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)),
],
),
]
}}}
}}}
{{{
#!div style="font-size: 80%"
{{{#!python
>>> 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)),
],
),
]
}}}
}}}
{{{
#!div style="font-size: 80%"
{{{#!python
>>> 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),
),
]
}}}
}}}
{{{
#!div style="font-size: 80%"
{{{#!python
>>> 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),
),
]
}}}
}}}
{{{
#!div style="font-size: 80%"
{{{#!python
>>> 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26223#comment:3>
Django
Reporter: bartekwojcicki | Owner: vytisb
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* version: 1.8 => master
Comment:
I'm also not sure about about the `real_field`'s naming but that sounds
like the right approach to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/26223#comment:4>
Django
Reporter: bartekwojcicki | Owner: vytisb
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by vytisb):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/6772 PR]
Went with `actual_field` instead of `real_field`.
--
Ticket URL: <https://code.djangoproject.com/ticket/26223#comment:5>
Django
Reporter: bartekwojcicki | Owner: vytisb
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by vytisb):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26223#comment:6>
Django
Reporter: bartekwojcicki | Owner: vytisb
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by vytisb):
I found another faulty optimization for `AddField`+`AlterField`:
{{{
#!div style="font-size: 80%"
{{{#!python
>>> 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26223#comment:7>
Django
Reporter: bartekwojcicki | Owner: vytisb
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* cc: MarkusH (added)
Comment:
Markus, any thoughts about the approach in the current PR vs. the
alternative suggested in comment:6?
--
Ticket URL: <https://code.djangoproject.com/ticket/26223#comment:8>
Django
Reporter: bartekwojcicki | Owner: vytisb
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by vytisb):
I have a WIP patch for the approach from comment:6, if anyone's
interested: https://github.com/vytisb/django/pull/1
--
Ticket URL: <https://code.djangoproject.com/ticket/26223#comment:9>
Django
Reporter: Bartek Wójcicki | Owner: Vytis
| Banaitis
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/26223#comment:10>
Django
Reporter: Bartek Wójcicki | Owner: Vytis
| Banaitis
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by InvalidInterrupt):
* cc: InvalidInterrupt (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/26223#comment:11>
Django
Reporter: Bartek Wójcicki | Owner: (none)
Type: Bug | Status: new
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Mariusz Felisiak):
* owner: Vytis Banaitis => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/26223#comment:12>
Django
Reporter: Bartek Wójcicki | Owner: (none)
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Ülgen Sarıkavak):
* cc: Ülgen Sarıkavak (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/26223#comment:13>
Django
Reporter: Bartek Wójcicki | Owner: (none)
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Roman Donchenko):
* cc: Roman Donchenko (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/26223#comment:14>