Opened 7 years ago

Closed 6 years ago

#27768 closed Cleanup/optimization (fixed)

makemigrations uses unnecessary AddField for ForeignKey depending on model name

Reported by: Ed Morley Owned by: Ed Morley
Component: Migrations Version: dev
Severity: Normal Keywords:
Cc: Simon Charette 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 Ed Morley)

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.

class Zzz(models.Model):
    pass

class Foo(models.Model):
    fk = models.ForeignKey(Zzz, on_delete=django.db.models.deletion.CASCADE)

Resultant migration operations generated by ./manage.py makemigrations:

    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):

    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.

Change History (9)

comment:1 by Ed Morley, 7 years ago

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 CreateModels (if no other dependencies between them) rather than refuse to optimize across them.

ie for (2), this existing test:

    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:

        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)]),
            ],
        )

comment:2 by Ed Morley, 7 years ago

Description: modified (diff)

comment:3 by Tim Graham, 7 years ago

Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

comment:4 by Ed Morley, 7 years ago

Has patch: set
Owner: changed from nobody to Ed Morley
Status: newassigned

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 :-)

comment:5 by Simon Charette, 7 years ago

Cc: Simon Charette added

comment:6 by Simon Charette, 7 years ago

comment:7 by Tim Graham, 7 years ago

Patch needs improvement: set

comment:8 by Tim Graham <timograham@…>, 6 years ago

In d3a935f0:

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.

comment:9 by Tim Graham <timograham@…>, 6 years ago

Resolution: fixed
Status: assignedclosed

In a97845a8:

Fixed #27768 -- Allowed migration optimization of CreateModel order.

Thanks Ed Morley from Mozilla for the tests.

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