Opened 4 years ago

Last modified 4 months ago

#32263 assigned Bug

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

Reported by: InvalidInterrupt Owned by: Bhuvnesh
Component: Migrations Version: 3.1
Severity: Normal Keywords:
Cc: InvalidInterrupt, Bhuvnesh, David Wobrock 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 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)

Attachments (2)

ticket_32263_1.tar.gz (3.0 KB ) - added by Mariusz Felisiak 4 years ago.
Sample project 1.
ticket_32263_2.tar.gz (3.1 KB ) - added by Mariusz Felisiak 4 years ago.
Sample project 2.

Download all attachments as: .zip

Change History (21)

comment:1 by InvalidInterrupt, 4 years ago

Description: modified (diff)

by Mariusz Felisiak, 4 years ago

Attachment: ticket_32263_1.tar.gz added

Sample project 1.

by Mariusz Felisiak, 4 years ago

Attachment: ticket_32263_2.tar.gz added

Sample project 2.

comment:2 by Mariusz Felisiak, 4 years ago

Triage Stage: UnreviewedAccepted

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

comment:3 by Mariusz Felisiak, 4 years ago

Summary: squashmigrations produces incorrect result when a RenameModel operation is performed on a ForeignKey targetsquashmigrations produces incorrect result with a RenameModel on a ForeignKey target.

comment:4 by Aditya parashar, 4 years ago

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

comment:5 by gasparb16, 4 years ago

Owner: changed from nobody to gasparb16
Status: newassigned

comment:6 by gasparb16, 4 years ago

Owner: gasparb16 removed

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

comment:8 by Anvesh Mishra, 2 years ago

Owner: set to Anvesh Mishra

in reply to:  7 comment:9 by Anvesh Mishra, 2 years ago

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.

comment:10 by Bhuvnesh, 2 years ago

Cc: Bhuvnesh added

comment:11 by Anvesh Mishra, 2 years ago

Tried running around the files, the first problem that was the creation of model with ForeignKey before the target of ForeignKey seems to be a case for Sample Project 2 provided by Mariusz Felisiak. 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.

Last edited 2 years ago by Anvesh Mishra (previous) (diff)

comment:12 by Anvesh Mishra, 2 years ago

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.

comment:13 by Anvesh Mishra, 2 years ago

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.

comment:14 by Durval Carvalho, 20 months ago

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?

Last edited 20 months ago by Durval Carvalho (previous) (diff)

comment:15 by Bhuvnesh, 14 months ago

Has patch: set
Owner: changed from Anvesh Mishra to Bhuvnesh

PR
Thanks to Durval Carvalho for a basic idea.

comment:16 by David Wobrock, 13 months ago

Cc: David Wobrock added

comment:17 by bcail, 9 months ago

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?

in reply to:  17 comment:18 by Mariusz Felisiak, 9 months ago

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.

comment:19 by Sarah Boyce, 4 months ago

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