[Django] #32256: squashmigrations may traceback when fields' names are swapped using a temporary name
Django
temporary name
--------------------------------------------+------------------------
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 |
--------------------------------------------+------------------------
If you rename fields using a pattern like `a->c; b->a; c->b` (such as if
previously `DateTimeField`s using `auto_now` and `auto_now_add` had been
mixed-up) and then attempt to `squashmigrations` with an optimization
barrier between the `CreateModel` and `RenameField`s, the migration
optimizer will attempt to create a `CreateModel` operation object with two
fields using the same name and fail. I'll attach a migration file that
triggers the failure.
I believe the root cause of this issue is that
`django.db.migrations.operations.fields.RenameField` allows itself to
optimize through (i.e be moved to the right of, I may have gotten this
terminology wrong) other `RenameField` operations that reference
`old_name`.
--
Ticket URL: <https://code.djangoproject.com/ticket/32256>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
temporary name
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 InvalidInterrupt):
* Attachment "0001_initial.py" added.
example migration
Django
temporary name
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
Comment (by InvalidInterrupt):
I don't have time to write a patch and tests myself at the moment. I'll
assign myself once that changes, unless someone else picks this up first.
--
Ticket URL: <https://code.djangoproject.com/ticket/32256#comment:1>
Django
temporary name
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 InvalidInterrupt):
* cc: InvalidInterrupt (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/32256#comment:2>
Django
temporary name
----------------------------------+------------------------------------
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.
It's squashed to a single `CreateModel()` operation when the initial
migration doesn't contain `migrations.RunPython()`, but crashes if it
does:
{{{
Traceback (most recent call last):
File "manage.py", line 22, in <module>
main()
File "manage.py", line 18, in main
execute_from_command_line(sys.argv)
File "/django/django/core/management/__init__.py", line 419, in
execute_from_command_line
utility.execute()
File "/django/django/core/management/__init__.py", line 413, in execute
self.fetch_command(subcommand).run_from_argv(self.argv)
File "/django/django/core/management/base.py", line 354, in
run_from_argv
self.execute(*args, **cmd_options)
File "/django/django/core/management/base.py", line 398, in execute
output = self.handle(*args, **options)
File "/django/django/core/management/commands/squashmigrations.py", line
145, in handle
new_operations = optimizer.optimize(operations, migration.app_label)
File "/django/django/db/migrations/optimizer.py", line 34, in optimize
result = self.optimize_inner(operations, app_label)
File "/django/django/db/migrations/optimizer.py", line 47, in
optimize_inner
result = operation.reduce(other, app_label)
File "/django/django/db/migrations/operations/models.py", line 233, in
reduce
CreateModel(
File "/django/django/db/migrations/operations/models.py", line 55, in
__init__
_check_for_duplicates('fields', (name for name, _ in self.fields))
File "/django/django/db/migrations/operations/models.py", line 17, in
_check_for_duplicates
raise ValueError(
ValueError: Found duplicate value field_a in CreateModel fields argument.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32256#comment:3>
Django
temporary name
----------------------------------+------------------------------------
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
Changes (by Mariusz Felisiak):
* Attachment "ticket_32256.tar.gz" added.
Sample project.
Django
temporary name
----------------------------------+------------------------------------
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
Comment (by Mariusz Felisiak):
I attached [https://code.djangoproject.com/raw-
attachment/ticket/32256/ticket_32256.tar.gz a sample project], `manage.py
squashmigrations test_one 0001 0004` reproduces this issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/32256#comment:4>
Django
temporary name
-------------------------------------+-------------------------------------
Reporter: InvalidInterrupt | Owner: Hasan
| Ramezani
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 Hasan Ramezani):
* owner: nobody => Hasan Ramezani
* status: new => assigned
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/13773 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/32256#comment:5>
Django
temporary name
-------------------------------------+-------------------------------------
Reporter: InvalidInterrupt | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by InvalidInterrupt):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32256#comment:6>
Django
temporary name
-------------------------------------+-------------------------------------
Reporter: InvalidInterrupt | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Hasan Ramezani):
Comment from @InvalidInterrupt on PR:
{{{
Thanks to having your test as reference I put together my idea for fixing
this in RenameField.reduce() here:
https://github.com/InvalidInterrupt/django/tree/ticket_32256.
I think it's better to fix this there rather than try to prevent the
optimizer from considering such optimizations in the first case; other
proposed features I've seen in trac (i.e.
https://code.djangoproject.com/ticket/27844) would be likely to cause the
issue to arise again. My approach does imply that this sequence of
migrations is not optimizable; I would be happy to be proven wrong if
someone identified a way to optimize this in the general case.
I'm (probably overly) concerned about stepping on toes here, so I'm not
making another PR. I may find myself busy again over the next couple days;
please feel free to either ask me to make a PR or just take my commit if
that's easier for you all.
}}}
I am not sure about the solution proposed by @InvalidInterrupt.
@Mariusz Could you please clarify it? If you agree with the proposed
solution I can update the PR with the proposed solution.
Thanks
--
Ticket URL: <https://code.djangoproject.com/ticket/32256#comment:7>
Django
temporary name
-------------------------------------+-------------------------------------
Reporter: InvalidInterrupt | Owner: Hasan
| Ramezani
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 Hasan Ramezani):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/32256#comment:8>
Django
temporary name
-------------------------------------+-------------------------------------
Reporter: InvalidInterrupt | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
Comment:
> I am not sure about the solution proposed by @InvalidInterrupt.
Sounds reasonable.
--
Ticket URL: <https://code.djangoproject.com/ticket/32256#comment:9>
Django
temporary name
-------------------------------------+-------------------------------------
Reporter: InvalidInterrupt | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/32256#comment:10>
Django
temporary name
-------------------------------------+-------------------------------------
Reporter: InvalidInterrupt | Owner: Hasan
| Ramezani
Component: Migrations | Version: 3.1
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"7c18b22e2fa70aa8dcfadb33beb17933abdf7ee4" 7c18b22e]:
{{{
#!CommitTicketReference repository=""
revision="7c18b22e2fa70aa8dcfadb33beb17933abdf7ee4"
Fixed #32256 -- Fixed migration optimization crash when swapping field
names.
This disables optimization of RenameField operation when an old field
name is referenced in subsequent operations.
Co-authored-by: InvalidInterrupt
<InvalidI...@users.noreply.github.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32256#comment:11>