Opened 4 years ago

Closed 3 years ago

#32256 closed Bug (fixed)

squashmigrations optimizer crashes when fields' names are swapped using a temporary name

Reported by: InvalidInterrupt Owned by: Hasan Ramezani
Component: Migrations Version: 3.1
Severity: Normal Keywords:
Cc: InvalidInterrupt Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If you rename fields using a pattern like a->c; b->a; c->b (such as if previously DateTimeFields using auto_now and auto_now_add had been mixed-up) and then attempt to squashmigrations with an optimization barrier between the CreateModel and RenameFields, 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.

Attachments (2)

0001_initial.py (1.2 KB ) - added by InvalidInterrupt 4 years ago.
example migration
ticket_32256.tar.gz (3.0 KB ) - added by Mariusz Felisiak 4 years ago.
Sample project.

Download all attachments as: .zip

Change History (13)

by InvalidInterrupt, 4 years ago

Attachment: 0001_initial.py added

example migration

comment:1 by InvalidInterrupt, 4 years ago

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.

comment:2 by InvalidInterrupt, 4 years ago

Cc: InvalidInterrupt added

comment:3 by Mariusz Felisiak, 4 years ago

Summary: squashmigrations may traceback when fields' names are swapped using a temporary namesquashmigrations optimizer crashes when fields' names are swapped using a temporary name
Triage Stage: UnreviewedAccepted

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.

by Mariusz Felisiak, 4 years ago

Attachment: ticket_32256.tar.gz added

Sample project.

comment:4 by Mariusz Felisiak, 4 years ago

I attached a sample project, manage.py squashmigrations test_one 0001 0004 reproduces this issue.

comment:5 by Hasan Ramezani, 4 years ago

Has patch: set
Owner: changed from nobody to Hasan Ramezani
Status: newassigned

comment:6 by InvalidInterrupt, 4 years ago

Patch needs improvement: set

comment:7 by Hasan Ramezani, 4 years ago

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

comment:8 by Hasan Ramezani, 3 years ago

Patch needs improvement: unset

comment:9 by Mariusz Felisiak, 3 years ago

Patch needs improvement: set

I am not sure about the solution proposed by @InvalidInterrupt.

Sounds reasonable.

comment:10 by Simon Charette, 3 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:11 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In 7c18b22e:

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 <InvalidInterrupt@users.noreply.github.com>

Note: See TracTickets for help on using tickets.
Back to Top