[Django] #27768: makemigrations uses unnecessary AddField for ForeignKey depending on model name
Django
name
--------------------------------------+------------------------
Reporter: Ed Morley | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
--------------------------------------+------------------------
Using Django master, for this model `makemigrations` generates an
inefficient migrations file, which uses a combination of `CreateModel` and
`AddField`, rather than just using a single `CreateModel` operation.
{{{#!python
class Aaa(models.Model):
pass
class Foo(models.Model):
fk = models.ForeignKey(Aaa,
on_delete=django.db.models.deletion.CASCADE)
}}}
Resultant migration operations generated by `./manage.py makemigrations`:
{{{#!python
operations = [
migrations.CreateModel(
name='Foo',
fields=[
('id', models.AutoField(auto_created=True,
primary_key=True, serialize=False, verbose_name='ID')),
],
),
migrations.CreateModel(
name='Zzz',
fields=[
('id', models.AutoField(auto_created=True,
primary_key=True, serialize=False, verbose_name='ID')),
],
),
migrations.AddField(
model_name='foo',
name='fk',
field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE,
to='testapp.Zzz'),
),
]
}}}
However if the `Zzz` model was instead called `Aaa`, then the migration
file is correctly optimised (ie: the `ForeignKey` is defined within the
`CreateModel` operation):
{{{#!python
operations = [
migrations.CreateModel(
name='Aaa',
fields=[
('id', models.AutoField(auto_created=True,
primary_key=True, serialize=False, verbose_name='ID')),
],
),
migrations.CreateModel(
name='Foo',
fields=[
('id', models.AutoField(auto_created=True,
primary_key=True, serialize=False, verbose_name='ID')),
('fk',
models.ForeignKey(on_delete=django.db.models.deletion.CASCADE,
to='testapp.Aaa')),
],
),
]
}}}
Ideally the optimizer would either just use the order as defined in
models.py (which would at least allow for users to order them sensibly),
or else intelligently sort the models such that those with no (or fewer)
foreign keys are listed in `operations` first, thereby reducing the number
of cases where the FK has to be added in a separate `AddField` operation.
--
Ticket URL: <https://code.djangoproject.com/ticket/27768>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
name
Reporter: Ed Morley | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Ed Morley):
I guess there are two parts to this:
1) When performing a `makemigrations` to generate the initial migrations
file, if the order that the models were declared in models.py was
preserved, then the resultant operations would (a) likely require less
optimization in the first place (since unless people use string references
to other models, they will be declared in the correct order in the file),
and (b) users could at least control the order.
2) Ideally the migrations optimizer would reorder `CreateModel`s (if no
other dependencies between them) rather than refuse to optimize across
them.
ie for (2),
[https://github.com/django/django/blob/e5c2e43cc832028a974399af07a1c3ba6afa2467/tests/migrations/test_optimizer.py#L313-L329
this existing test]:
{{{#!python
def test_create_model_add_field_not_through_fk(self):
"""
AddField should NOT optimize into CreateModel if it's an FK to a
model
that's between them.
"""
self.assertOptimizesTo(
[
migrations.CreateModel("Foo", [("name",
models.CharField(max_length=255))]),
migrations.CreateModel("Link", [("url",
models.TextField())]),
migrations.AddField("Foo", "link",
models.ForeignKey("migrations.Link", models.CASCADE)),
],
[
migrations.CreateModel("Foo", [("name",
models.CharField(max_length=255))]),
migrations.CreateModel("Link", [("url",
models.TextField())]),
migrations.AddField("Foo", "link",
models.ForeignKey("migrations.Link", models.CASCADE)),
],
)
}}}
Should actually be changed to:
{{{#!python
self.assertOptimizesTo(
[
migrations.CreateModel("Foo", [("name",
models.CharField(max_length=255))]),
migrations.CreateModel("Link", [("url",
models.TextField())]),
migrations.AddField("Foo", "link",
models.ForeignKey("migrations.Link", models.CASCADE)),
],
[
migrations.CreateModel("Link", [("url",
models.TextField())]),
migrations.CreateModel("Foo", [("name",
models.CharField(max_length=255)), ("link",
models.ForeignKey("migrations.Link", models.CASCADE)]),
],
)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27768#comment:1>
Django
name
Reporter: Ed Morley | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Ed Morley:
Old description:
New description:
Using Django master, for this model `makemigrations` generates an
inefficient migrations file, which uses a combination of `CreateModel` and
`AddField`, rather than just using a single `CreateModel` operation.
{{{#!python
class Zzz(models.Model):
pass
class Foo(models.Model):
fk = models.ForeignKey(Zzz,
on_delete=django.db.models.deletion.CASCADE)
}}}
--
--
Ticket URL: <https://code.djangoproject.com/ticket/27768#comment:2>
Django
name
Reporter: Ed Morley | Owner: nobody
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* type: Bug => Cleanup/optimization
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/27768#comment:3>
Django
name
Reporter: Ed Morley | Owner: Ed Morley
Type: | Status: assigned
Cleanup/optimization |
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 Ed Morley):
* status: new => assigned
* owner: nobody => Ed Morley
* has_patch: 0 => 1
Comment:
PR opened.
I've added additional testcases for the edge-cases where reordering will
cause issues (eg inherited models, circular FKs etc) but may be missing
some, so open to suggestions :-)
--
Ticket URL: <https://code.djangoproject.com/ticket/27768#comment:4>
Django
name
Reporter: Ed Morley | Owner: Ed Morley
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* cc: Simon Charette (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/27768#comment:5>
Django
name
Reporter: Ed Morley | Owner: Ed Morley
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
[https://github.com/django/django/pull/7999 Adjusted PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/27768#comment:6>
Django
name
Reporter: Ed Morley | Owner: Ed Morley
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27768#comment:7>
Django
name
Reporter: Ed Morley | Owner: Ed Morley
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"d3a935f01f6cb36d21b09d81c5f022a3a5116ab0" d3a935f0]:
{{{
#!CommitTicketReference repository=""
revision="d3a935f01f6cb36d21b09d81c5f022a3a5116ab0"
Refs #27768 -- Reversed order of optimized and in-between operations.
Operations can only be optimized through if they don't reference any of
the
state the operation they are compared against defines or alters, so it's
safe to reverse the order.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27768#comment:8>
Django
name
Reporter: Ed Morley | Owner: Ed Morley
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"a97845a823c86b1d6e65af70fbce64e6e747e081" a97845a8]:
{{{
#!CommitTicketReference repository=""
revision="a97845a823c86b1d6e65af70fbce64e6e747e081"
Fixed #27768 -- Allowed migration optimization of CreateModel order.
Thanks Ed Morley from Mozilla for the tests.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27768#comment:9>