Opened 9 years ago

Closed 3 years ago

#24900 closed Bug (fixed)

KeyError when trying to migrate backward to a replaced migration

Reported by: Carl Meyer Owned by: Jacob Walls
Component: Migrations Version: dev
Severity: Normal Keywords:
Cc: Shai Berger 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

Django exhibits some internal confusion regarding whether replaced migrations exist or not. Consider this simple app with two migrations and squashed migration replacing both:

$ ls testproj/migrations/
0001_initial.py  0001_squashed_0002_thing_age.py  0002_thing_age.py  __init__.py

When it comes to disambiguating input, Django seems to believe that the replaced migrations still need to be considered:

$ ./manage.py migrate testproj 0001
CommandError: More than one migration matches '0001' in app 'testproj'. Please be more specific.

But if you actually try to disambiguate and specify one of the replaced migrations, Django no longer thinks it exists (and isn't very graceful about telling you so):

$ ./manage.py migrate testproj 0001_initial
Traceback (most recent call last):
  File "./manage.py", line 10, in <module>
    execute_from_command_line(sys.argv)
  File "/home/carljm/projects/django/django/django/django/core/management/__init__.py", line 330, in execute_from_command_line
    utility.execute()
  File "/home/carljm/projects/django/django/django/django/core/management/__init__.py", line 322, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/home/carljm/projects/django/django/django/django/core/management/base.py", line 347, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/home/carljm/projects/django/django/django/django/core/management/base.py", line 398, in execute
    output = self.handle(*args, **options)
  File "/home/carljm/projects/django/django/django/django/core/management/commands/migrate.py", line 135, in handle
    plan = executor.migration_plan(targets)
  File "/home/carljm/projects/django/django/django/django/db/migrations/executor.py", line 50, in migration_plan
    self.loader.graph.node_map[target].children
KeyError: ('testproj', '0001_initial')

There could be several different approaches to fixing this, but my feeling is that Django shouldn't prevent you from migrating to a replaced migration. If a migration still exists on disk, even if it's been squashed and you've fully migrated the squashed set, you should be able to migrate back to a state within the squashed set. It seems like there might be production rollback cases where that could be important, and I don't see in principle why it shouldn't be possible.

If that turns out to be impractical, then I think Django oughtn't bother you about resolving ambiguities with migration names it won't let you migrate to anyway. And the "nonexistent" error for this case should be nicer than a raw KeyError. (In Django 1.7 the error was "ValueError: Node ('testproj17', '0001_initial') not a valid node", which is perhaps a bit better, but not much.)

Change History (16)

comment:1 by Markus Holtermann, 9 years ago

Triage Stage: UnreviewedAccepted

I agree, that should be possible and I think it is possible.

comment:2 by Shai Berger, 9 years ago

Cc: Shai Berger added

Just ran into this. FWIW, workaround, as long as we do not do what comment:5:ticket:24902 recommends (which is to keep the state of squashed migrations in the db), is to simply move the squashed migration file away, migrate backwards to your heart's content, then bring it back.

comment:3 by Jacob Walls, 3 years ago

Owner: changed from nobody to Jacob Walls
Status: newassigned

comment:4 by Jacob Walls, 3 years ago

Has patch: set

comment:5 by Shai Berger, 3 years ago

Triage Stage: AcceptedReady for checkin

comment:6 by Jacob Walls, 3 years ago

Triage Stage: Ready for checkinAccepted

I added two test scenarios, so bumping this back from RFC. Thanks for the review, Shai!

Last edited 3 years ago by Jacob Walls (previous) (diff)

comment:7 by Asif Saifuddin Auvi, 3 years ago

Version: 1.8dev

comment:8 by Shai Berger, 3 years ago

Triage Stage: AcceptedReady for checkin

comment:9 by Mariusz Felisiak, 3 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

I'd like to discuss alternatives before moving forward, see comment.

comment:10 by Jacob Walls, 3 years ago

Patch needs improvement: unset

Updated implementation to reduce complexity.

comment:11 by Jacob Walls, 3 years ago

Summary: KeyError when trying to migrate to a replaced migrationKeyError when trying to migrate backward to a replaced migration

comment:12 by Mariusz Felisiak, 3 years ago

Needs tests: set
Patch needs improvement: set

comment:13 by Jacob Walls, 3 years ago

Needs tests: unset
Patch needs improvement: unset

comment:14 by Mariusz Felisiak, 3 years ago

Triage Stage: AcceptedReady for checkin

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

In 9e17cc0:

Refs #24900 -- Added MigrationLoader test for applying squashed migrations.

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

Resolution: fixed
Status: assignedclosed

In 3219dd33:

Fixed #24900 -- Allowed migrating backward to squashed migrations.

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