[Django] #32263: squashmigrations produces incorrect result when a RenameModel operation is performed on a ForeignKey target
Django
performed on a ForeignKey target
--------------------------------------------+------------------------
Reporter: InvalidInterrupt | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 3.1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
--------------------------------------------+------------------------
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
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
[https://code.djangoproject.com/ticket/24849#comment:1 here] but as far as
I can tell that's not the problem originally reported in that ticket)
--
Ticket URL: <https://code.djangoproject.com/ticket/32263>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
performed on a ForeignKey target
Reporter: InvalidInterrupt | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 3.1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by InvalidInterrupt:
Old description:
New description:
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
[https://code.djangoproject.com/ticket/24849#comment:1 here] but as far as
I can tell that's not the problem originally reported in that ticket)
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32263#comment:1>
Django
performed on a ForeignKey target
Reporter: InvalidInterrupt | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 3.1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* Attachment "ticket_32263_1.tar.gz" added.
Sample project 1.
Django
performed on a ForeignKey target
Reporter: InvalidInterrupt | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 3.1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* Attachment "ticket_32263_2.tar.gz" added.
Sample project 2.
Django
performed on a ForeignKey target
Reporter: InvalidInterrupt | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 3.1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Unreviewed => Accepted
Comment:
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`
[https://code.djangoproject.com/raw-
attachment/ticket/32263/ticket_32263_2.tar.gz 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)
[https://code.djangoproject.com/raw-
attachment/ticket/32263/ticket_32263_1.tar.gz Sample project 1]
--
Ticket URL: <https://code.djangoproject.com/ticket/32263#comment:2>
Django
ForeignKey target.
----------------------------------+------------------------------------
Reporter: InvalidInterrupt | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 3.1
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/32263#comment:3>
Django
ForeignKey target.
----------------------------------+------------------------------------
Type: Bug | Status: new
Component: Migrations | Version: 3.1
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by ADI33352):
You'll have to change the order of your save and assignment operations, or
do the extra assignment. it might help
--
Ticket URL: <https://code.djangoproject.com/ticket/32263#comment:4>
Django
ForeignKey target.
Reporter: InvalidInterrupt | Owner: gasparb16
Type: Bug | Status: assigned
Component: Migrations | Version: 3.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by gasparb16):
* owner: nobody => gasparb16
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/32263#comment:5>
Django
ForeignKey target.
----------------------------------+------------------------------------
Type: Bug | Status: assigned
Component: Migrations | Version: 3.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by gasparb16):
* owner: gasparb16 => (none)
Comment:
I have played with this for a while but I am not sure if this can be
solved without a larger scale code change.
--
Ticket URL: <https://code.djangoproject.com/ticket/32263#comment:6>
Django
ForeignKey target.
----------------------------------+------------------------------------
Reporter: InvalidInterrupt | Owner: (none)
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
Feels like it's the same class of issue as #32256 but for `RenameModel`
instead of `RenameField` (7c18b22e2fa70aa8dcfadb33beb17933abdf7ee4)
--
Ticket URL: <https://code.djangoproject.com/ticket/32263#comment:7>
Django
ForeignKey target.
Reporter: InvalidInterrupt | Owner: Anvesh
| Mishra
Type: Bug | Status: assigned
Component: Migrations | Version: 3.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Anvesh Mishra):
* owner: (none) => Anvesh Mishra
--
Ticket URL: <https://code.djangoproject.com/ticket/32263#comment:8>
Django
ForeignKey target.
-------------------------------------+-------------------------------------
Reporter: InvalidInterrupt | Owner: Anvesh
| Mishra
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Anvesh Mishra):
Replying to [comment:7 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32263#comment:9>
Django
ForeignKey target.
-------------------------------------+-------------------------------------
Reporter: InvalidInterrupt | Owner: Anvesh
| Mishra
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Bhuvnesh):
* cc: Bhuvnesh (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/32263#comment:10>
Django
ForeignKey target.
-------------------------------------+-------------------------------------
Reporter: InvalidInterrupt | Owner: Anvesh
| Mishra
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32263#comment:11>
Django
ForeignKey target.
-------------------------------------+-------------------------------------
Reporter: InvalidInterrupt | Owner: Anvesh
| Mishra
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Anvesh Mishra):
After the squashed migration is created if we run `makemigrations` once
again it creates a new migrations file as such:
{{{#!python
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32263#comment:12>
Django
ForeignKey target.
-------------------------------------+-------------------------------------
Reporter: InvalidInterrupt | Owner: Anvesh
| Mishra
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Anvesh Mishra):
A regression test for this ticket would look something like this:
{{{#!python
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()`:
{{{#!python
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:
{{{#!python
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32263#comment:13>
Django
ForeignKey target.
-------------------------------------+-------------------------------------
Reporter: InvalidInterrupt | Owner: Anvesh
| Mishra
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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)
elif all(op.reduce(other, app_label) is True for op in
in_between): # <<<<<<< This condition fails (I didn't understand what is
being checked here)
# 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/32263#comment:14>
Django
ForeignKey target.
Reporter: InvalidInterrupt | Owner: Bhuvnesh
Type: Bug | Status: assigned
Component: Migrations | Version: 3.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Bhuvnesh):
* owner: Anvesh Mishra => Bhuvnesh
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/17257 PR]
Thanks to Durval Carvalho for a basic idea.
--
Ticket URL: <https://code.djangoproject.com/ticket/32263#comment:15>
Django
ForeignKey target.
----------------------------------+------------------------------------
Reporter: InvalidInterrupt | Owner: Bhuvnesh
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by David Wobrock):
* cc: David Wobrock (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/32263#comment:16>
Django
ForeignKey target.
----------------------------------+------------------------------------
Reporter: InvalidInterrupt | Owner: Bhuvnesh
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/32263#comment:17>
Django
ForeignKey target.
----------------------------------+------------------------------------
Reporter: InvalidInterrupt | Owner: Bhuvnesh
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak):
Replying to [comment:17 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
[https://docs.djangoproject.com/en/5.0/internals/contributing/writing-code
/submitting-patches/#patch-review-checklist review it] if you want to
help.
--
Ticket URL: <https://code.djangoproject.com/ticket/32263#comment:18>
Django
ForeignKey target.
----------------------------------+------------------------------------
Reporter: InvalidInterrupt | Owner: Bhuvnesh
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Changes (by Sarah Boyce):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32263#comment:19>
Django
ForeignKey target.
----------------------------------+------------------------------------
Reporter: InvalidInterrupt | Owner: Bhuvnesh
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Changes (by Bhuvnesh):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/32263#comment:20>
Django
ForeignKey target.
----------------------------------+------------------------------------
Reporter: InvalidInterrupt | Owner: Bhuvnesh
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Changes (by InvalidInterrupt):
* needs_better_patch: 0 => 1
Ticket URL: <https://code.djangoproject.com/ticket/32263#comment:21>