makemigrations uses unnecessary AddField for ForeignKey depending on model name

[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.

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

[Ed Morley]

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

[Simon Charette]

​Adjusted PR

[Tim Graham <timograham@…>]

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.

[Tim Graham <timograham@…>]

In a97845a8:

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

Thanks Ed Morley from Mozilla for the tests.