squashmigrations produces incorrect result with a RenameModel on a ForeignKey target.

[InvalidInterrupt]

Given a list of operations:

    operations = [
        migrations.CreateModel(
            name="Author",
            fields=[
                ("id",
                 models.AutoField(auto_created=True, primary_key=True, serialize=False,
                                  verbose_name="ID")),
            ]
        ),
        migrations.CreateModel(
            name="Book",
            fields=[
                ("id",
                 models.AutoField(auto_created=True, primary_key=True, serialize=False,
                                  verbose_name="ID")),
                ("author", models.ForeignKey(on_delete=models.deletion.PROTECT,
                                             to="testapp.Author")),
            ]
        ),
        migrations.RenameModel("Author", "Person"),
        migrations.AddField(model_name="person",
                            name="birthdate",
                            field=models.DateField()),
    ]

squashmigrations produces the result:

    operations = [
        migrations.CreateModel(
            name='Book',
            fields=[
                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
                ('author', models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, to='testapp.author')),
            ],
        ),
        migrations.CreateModel(
            name='Person',
            fields=[
                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
                ('birthdate', models.DateField()),
            ],
        ),
    ]

There are two apparent problems with the result:

• The model with the ForeignKey field is created before the target of the ForeignKey
• The to parameter of the ForeignKey is not updated with the new model name (the final AddField operation is not necessary to cause this problem)

I believe this is caused by CreateModel.reduce() allowing optimization though operations that reference models which the model being created has a relationship to. Similar effects are likely to be caused by other migration patterns as well (I think Tim Graham may have found one here but as far as I can tell that's not the problem originally reported in that ticket)

[Mariusz Felisiak]

Sample project 1.

[Mariusz Felisiak]

Sample project 2.

[Mariusz Felisiak]

Thanks for the report. Agreed, we have two separate issues that can be fixed separately. I attached sample projects:

There are two apparent problems with the result:

• The model with the ForeignKey field is created before the target of the ForeignKey

Sample project 2

• The to parameter of the ForeignKey is not updated with the new model name (the final AddField operation is not necessary to cause this problem)

Sample project 1

[Aditya parashar]

You'll have to change the order of your save and assignment operations, or do the extra assignment. it might help

[gasparb16]

I have played with this for a while but I am not sure if this can be solved without a larger scale code change.

[Simon Charette]

Feels like it's the same class of issue as #32256 but for RenameModel instead of RenameField (7c18b22e2fa70aa8dcfadb33beb17933abdf7ee4)

​https://github.com/django/django/blob/64a0d1ef6e7a6739148996e9584bbb61fe3dcc60/django/db/migrations/operations/models.py#L425-L430

[Anvesh Mishra]

Replying to Simon Charette:

Feels like it's the same class of issue as #32256 but for RenameModel instead of RenameField (7c18b22e2fa70aa8dcfadb33beb17933abdf7ee4)

​https://github.com/django/django/blob/64a0d1ef6e7a6739148996e9584bbb61fe3dcc60/django/db/migrations/operations/models.py#L425-L430

Tried doing the same with RenameModel.reduce() dosen't seem to be working might have to look out for something else.

[Anvesh Mishra]

Tried running around the files, the first problem that was the creation of model with ForeignKey before the target of ForeignKey seems to be resolved due to some previous patches I guess. For the second problem it seems that the RenameModel misses the functionality where it needs to change the to parameter of the ForeignKey model. the RenameModel.reduce() doesn't seems to be getting called here.

[Anvesh Mishra]

After the squashed migration is created if we run makemigrations once again it creates a new migrations file as such:

class Migration(migrations.Migration):

    dependencies = [
        ("test_one", "0001_squashed_0003_person_birthdate"),
    ]

    operations = [
        migrations.AlterField(
            model_name="book",
            name="author",
            field=models.ForeignKey(
                on_delete=django.db.models.deletion.PROTECT, to="test_one.person"
            ),
        ),
    ]

so the to field automatically gets referenced to the right table in the new migrations file but still it won't work cause of the previous sqaushed migration.

[Anvesh Mishra]

A regression test for this ticket would look something like this:

    def test_squashmigrations_rename_foreign_key(self):
        
        out = io.StringIO()
        with self.temporary_migration_module(
            module="migrations.test_migrations_rename"
        ) as migration_dir:
            call_command(
                "squashmigrations",
                "migrations",
                "0003",
                interactive=False,
                stdout=out,
                no_color=True,
            )
            squashed_migration_file = os.path.join(
                migration_dir, "0001_squashed_0003_third.py"
            )
            with open(squashed_migration_file, encoding="utf-8") as fp:
                content = fp.read()
                self.assertIn('to="migrations.person"',content)
            self.assertTrue(os.path.exists(squashed_migration_file))
        self.assertEqual(
            out.getvalue(),
            f"Will squash the following migrations:\n"
            f" - 0001_initial\n"
            f" - 0002_second\n"
            f" - 0003_third\n"
            f"Optimizing...\n"
            f"  Optimized from 4 operations to 2 operations.\n"
            f"Created new squashed migration {squashed_migration_file}\n"
            f"  You should commit this migration but leave the old ones in place;\n"
            f"  the new migration will be used for new installs. Once you are sure\n"
            f"  all instances of the codebase have applied the migrations you "
            f"squashed,\n"
            f"  you can delete them.\n",
        )

By running this test I found out that the base migration objects created are of CreateModel so we need to somehow change the functionality of this section of CreateModel.reduce():

def reduce(self,operation,app_label):
        ...
        elif (
            isinstance(operation, RenameModel)
            and self.name_lower == operation.old_name_lower
        ):
            return [
                CreateModel(
                    operation.new_name,
                    fields=self.fields,
                    options=self.options,
                    bases=self.bases,
                    managers=self.managers,
                ),
            ]

to something like:

def reduce(self,operation,app_label):
        ...
        elif (
            isinstance(operation, RenameModel)
            and self.name_lower == operation.old_name_lower
        ):
            return operation.reduce(operation,app_label)

and changing the functionality of RenameModel.reduce(). I hope I'm clear.

[Durval Carvalho]

I tried to deal with this problem as follows. In the CreateModel.reduce function, when executed with a RenameModel operation, I check if the CreateModel operation has any fields that may have been impacted by the RenameModel operation, because in this scenario, these fields need to be updated for this renamed model.

def reduce(self, operation, app_label):
    ...
    elif impacted_fields := [
        (_, field)
        for _, field in self.fields
        if field.remote_field
        and field.remote_field.model == f'{app_label}.{operation.old_name_lower}'
    ]:
        not_impacted_fields = [
            (_, field)
            for (_, field) in self.fields
            if (_, field) not in impacted_fields
        ]

        fixed_fields = []

        for (_, impacted_field) in impacted_fields:
            name, path, args, kwargs = impacted_field.deconstruct()
            kwargs['to'] = f'{app_label}.{operation.new_name_lower}'
            impacted_field = impacted_field.__class__(*args, **kwargs)
            fixed_fields.append((_, impacted_field))

        return [
            CreateModel(
                self.name,
                fields=not_impacted_fields + fixed_fields,
                bases=self.bases,
                managers=self.managers,
            ),
        ]
    ...

However, this approach did not work for the following reason. When the CreateModel operation is executed with its respective RenameModel, both are reduced to the new CreateModel operation. Then, some checks are made to decide whether this new resulting operation will be added to the list of new operations. But with this new reduction that I created, the other CreateModel operations, the ones that reference the renamed model, are also reduced, and this breaks the verification described below.

...
        for i, operation in enumerate(operations):
            right = True  # Should we reduce on the right or on the left.
            # Compare it to each operation after it
            for j, other in enumerate(operations[i + 1 :]):
                result = operation.reduce(other, app_label)
                if isinstance(result, list):
                    in_between = operations[i + 1 : i + j + 1]
                    if right:
                        new_operations.extend(in_between)
                        new_operations.extend(result)

                   # ↓↓↓ This condition fails (I didn't understand what is being checked here)
                    elif all(op.reduce(other, app_label) is True for op in in_between): 
                        # Perform a left reduction if all of the in-between
                        # operations can optimize through other.
                        new_operations.extend(result)
                        new_operations.extend(in_between)


                    else:  # <<<<<<<< New operation `result` is not added to new operations list
                        # Otherwise keep trying.
                        new_operations.append(operation)
                        break
...

Now I am stuck, trying to think of a way to work around this problem.
Do any of you have any suggestions?

[Bhuvnesh]

​PR
Thanks to Durval Carvalho for a basic idea.

[bcail]

Is there anything I can do to help move this forward? The PR seems to handle this case, and create the correct optimization, and the author responded to the last comment on the PR. Is it being held up on the question about multiple apps?

[Mariusz Felisiak]

Replying to bcail:

Is there anything I can do to help move this forward? The PR seems to handle this case, and create the correct optimization, and the author responded to the last comment on the PR. Is it being held up on the question about multiple apps?

It's waiting for a review, you can ​review it if you want to help.