Opened 10 months ago

Last modified 6 months ago

#27768 assigned Cleanup/optimization

makemigrations uses unnecessary AddField for ForeignKey depending on model name

Reported by: Ed Morley Owned by: Ed Morley
Component: Migrations Version: master
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 (7)

comment:1 Changed 10 months ago 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 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 Changed 10 months ago by Ed Morley

Description: modified (diff)

comment:3 Changed 10 months ago by Tim Graham

Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

comment:4 Changed 10 months ago by Ed Morley

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 Changed 10 months ago by Simon Charette

Cc: Simon Charette added

comment:6 Changed 10 months ago by Simon Charette

comment:7 Changed 6 months ago by Tim Graham

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