Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#24225 closed Bug (fixed)

KeyError when migrating in 1.8a1/master@728b6fd (does not occur in 1.7.3)

Reported by: Henrik Heimbuerger Owned by: Markus Holtermann
Component: Migrations Version: 1.8alpha1
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Henrik Heimbuerger)

When running the migrate management command (even if there is nothing to migrate) on 1.8a1 or the current GitHub master, I'm getting the following uncaught KeyError:

Running migrations:
  Rendering model states... DONE
Traceback (most recent call last):
  File "manage.py", line 10, in <module>
    execute_from_command_line(sys.argv)
  File "d:\develop\myproj\django-trunk\django\core\management\__init__.py", line 330, in execute_from_command_line
    utility.execute()
  File "d:\develop\myproj\django-trunk\django\core\management\__init__.py", line 322, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "d:\develop\myproj\django-trunk\django\core\management\base.py", line 350, in run_from_argv
    self.execute(*args, **cmd_options)
  File "d:\develop\myproj\django-trunk\django\core\management\base.py", line 401, in execute
    output = self.handle(*args, **options)
  File "d:\develop\myproj\django-trunk\django\core\management\commands\migrate.py", line 187, in handle
    executor.migrate(targets, plan, fake=options.get("fake", False))
  File "d:\develop\myproj\django-trunk\django\db\migrations\executor.py", line 89, in migrate
    state = migration.mutate_state(state)  # state is cloned inside
  File "d:\develop\myproj\django-trunk\django\db\migrations\migration.py", line 78, in mutate_state
    operation.state_forwards(self.app_label, new_state)
  File "d:\develop\myproj\django-trunk\django\db\migrations\operations\fields.py", line 50, in state_forwards
    state.reload_model(app_label, self.model_name_lower)
  File "d:\develop\myproj\django-trunk\django\db\migrations\state.py", line 61, in reload_model
    self._reload_one_model(rel_model._meta.app_label, rel_model._meta.model_name)
  File "d:\develop\myproj\django-trunk\django\db\migrations\state.py", line 68, in _reload_one_model
    self.models[app_label, model_name].render(self.apps)
KeyError: ('myapp', 'somemodel')

The reported model is created and then only appears twice in my migrations:

In myapp/migrations/0001_initial.py:

        migrations.AddField(
            model_name='somemodel',
            name='somefieldname',
            field=models.ForeignKey(to='myapp.AValidModel'),
            preserve_default=True,
        ),

and in myapp/migrations/0002_a.py:

        migrations.RemoveField(
            model_name='somemodel',
            name='somefieldname',
        ),

Other than that, the model hasn't been changed in the migrations.

The application is closed source and I can't share real code with you unfortunately.

I do have a debugger connected to the issue, but I have no idea where to even start debugging this. By checking the stack frame for mutation.py:78 (mutate_state()), I can see that the "current migration" when this occurs is a 0002_b.py, i.e. another migration dependent on 0001_initial.py which therefore shouldn't be aware of the field removal yet.

These migrations do work on 1.7.3. (But I can't really work on 1.7.3 because unit test startup takes minutes without --keepdb…)

Whatever the cause of this is, I highly recommend catching KeyError in _reload_one_model() and rethrowing a proper exception with a descriptive error message, including the stage and specific migration at which this occurs. Because a KeyError being raised somewhere in the depths of the migration system tells me nothing about what or where the issue is.

Attachments (1)

myproj.zip (4.6 KB) - added by Henrik Heimbuerger 4 years ago.
reproduction case #1

Download all attachments as: .zip

Change History (18)

comment:1 Changed 4 years ago by Henrik Heimbuerger

Description: modified (diff)

I didn't want to unset those fields. I clicked on the "revert" button before submitting and they disappeared, but they were unset nevertheless.

Last edited 4 years ago by Henrik Heimbuerger (previous) (diff)

comment:2 Changed 4 years ago by Claude Paroz

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted
Version: master1.8alpha1

Would you be able to reduce your set of models to reproduce this issue on a minimal structure you would then be able to share?

comment:3 Changed 4 years ago by Henrik Heimbuerger

Unfortunately, I don't. I extracted the suspected issue I described above into a new project, but that works fine. So there must be one more factor I'm missing to trigger the issue. Really reducing my actual big project down to the cause would take me days, which I unfortunately don't have. Besides, I wouldn't even know where to start.

I still can attach the debugger, so if you can tell me where to start inspecting the state at the point in time the exception was raised, I can do that.

Other than that, I now just removed those two aforementioned AddField and RemoveField instructions from my old migrations (they are really old, the two very first migrations—so while I'm sure editing old migrations by hand is not a best practice, it shouldn't break anything in this case) and that fixed it for my app.

So feel free to close as 'cannot reproduce'.

Changed 4 years ago by Henrik Heimbuerger

Attachment: myproj.zip added

reproduction case #1

comment:4 Changed 4 years ago by Henrik Heimbuerger

Claude inspired me to try again, and I think I managed to create a fairly minimal reproduction case, which is now attached.

Run python manage.py migrate.

Result on Django 1.7.4:

Operations to perform:
  Apply all migrations: admin, contenttypes, myapp, auth, sessions
Running migrations:
  Applying contenttypes.0001_initial... OK
  Applying auth.0001_initial... OK
  Applying admin.0001_initial... OK
  Applying myapp.0001_initial... OK
  Applying myapp.0002_a... OK
  Applying myapp.0003_a... OK
  Applying myapp.0004_a... OK
  Applying myapp.0002_b... OK
  Applying myapp.0005_merge_a_and_b... OK
  Applying sessions.0001_initial... OK

Result on Django master (from Jan 27):

Operations to perform:
  Apply all migrations: admin, contenttypes, myapp, auth, sessions
Running migrations:
  Rendering model states... DONE
Traceback (most recent call last):
  File "manage.py", line 10, in <module>
    execute_from_command_line(sys.argv)
  File "d:\develop\django-trunk\django\core\management\__init__.py", line 330, in execute_from_command_line
    utility.execute()
  File "d:\develop\django-trunk\django\core\management\__init__.py", line 322, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "d:\develop\django-trunk\django\core\management\base.py", line 350, in run_from_argv
    self.execute(*args, **cmd_options)
  File "d:\develop\django-trunk\django\core\management\base.py", line 401, in execute
    output = self.handle(*args, **options)
  File "d:\develop\django-trunk\django\core\management\commands\migrate.py", line 187, in handle
    executor.migrate(targets, plan, fake=options.get("fake", False))
  File "d:\develop\django-trunk\django\db\migrations\executor.py", line 89, in migrate
    state = migration.mutate_state(state)  # state is cloned inside
  File "d:\develop\django-trunk\django\db\migrations\migration.py", line 78, in mutate_state
    operation.state_forwards(self.app_label, new_state)
  File "d:\develop\django-trunk\django\db\migrations\operations\fields.py", line 50, in state_forwards
    state.reload_model(app_label, self.model_name_lower)
  File "d:\develop\django-trunk\django\db\migrations\state.py", line 61, in reload_model
    self._reload_one_model(rel_model._meta.app_label, rel_model._meta.model_name)
  File "d:\develop\django-trunk\django\db\migrations\state.py", line 68, in _reload_one_model
    self.models[app_label, model_name].render(self.apps)
KeyError: ('myapp', 'somemodel')

comment:5 Changed 4 years ago by Henrik Heimbuerger

My biggest mysteries:

a) Why does removing the last operation from 0002_b.py 'unbreak' the issue?

b) Why does taking the (entirely empty!) operations 0003 and 0004 out of the loop 'unbreak' the issue?
(I.e. removing 0003 and 0004 and making 0005 dependent on 0002_a directly.)

comment:6 Changed 4 years ago by Claude Paroz

Many thanks for the sample project which indeed triggers this bug.

The problem is that there are still leftover references of 'somemodel.account' foreign key on the Account model state (in _meta.related_objects) even after it has been deleted. I think the delete/remove operations do not clean enough metadata on related models. Investigation continues...

comment:7 Changed 4 years ago by Claude Paroz

Has patch: set

Henrik, could you try the patch to see if it solves the issue for your full application?
https://github.com/django/django/pull/4062

comment:8 Changed 4 years ago by Henrik Heimbuerger

Thanks! I'm on my way out now, but I'll give it a try tomorrow.
Cheers!

comment:9 Changed 4 years ago by Henrik Heimbuerger

I know you've already pulled back this patch because there are other aspects to consider, but for the record:

This patch would have fixed the specific issue in my (closed source) real world test case which originally triggered this bug report.

comment:10 Changed 4 years ago by Markus Holtermann

Owner: changed from nobody to Markus Holtermann
Status: newassigned

comment:11 Changed 4 years ago by Markus Holtermann

Based on Claude's initial patch I started working on bigger migration issue regarding states. More on the PR https://github.com/django/django/pull/4097

comment:12 Changed 4 years ago by Markus Holtermann

Needs tests: set
Patch needs improvement: set

comment:13 Changed 4 years ago by Markus Holtermann

Needs tests: unset
Patch needs improvement: unset

comment:14 Changed 4 years ago by Markus Holtermann <info@…>

In 58d0dd9260156067263ea7a2da2685c3cd88e18a:

Refs #24225 -- Added failing test case for removing a previously added field in migrations

When a related field is deleted, the related model must be updated. As
unchanged models are shared in migration states, the related model must
be re-rendered so that the change applies to a new copy of the related
model.

Thanks Henrik Heimbuerger for the report.

comment:15 Changed 4 years ago by Markus Holtermann <info@…>

Resolution: fixed
Status: assignedclosed

In b29f3b51204d53c1c8745966476543d068c173a2:

Fixed #24225, #24264, #24282 -- Rewrote model reloading in migration project state

Instead of naively reloading only directly related models (FK, O2O, M2M
relationship) the project state needs to reload their relations as well
as the model changes as well. Furthermore inheriting models (and super
models) need to be reloaded in order to keep inherited fields in sync.

To prevent endless recursive calls an iterative approach is taken.

comment:16 Changed 4 years ago by Markus Holtermann <info@…>

In 10ea9ef012cc11c9ae67ab4ba5d84e0d5d192732:

[1.8.x] Refs #24225 -- Added failing test case for removing a previously added field in migrations

When a related field is deleted, the related model must be updated. As
unchanged models are shared in migration states, the related model must
be re-rendered so that the change applies to a new copy of the related
model.

Thanks Henrik Heimbuerger for the report.

Backport of 58d0dd9260156067263ea7a2da2685c3cd88e18a from master

comment:17 Changed 4 years ago by Markus Holtermann <info@…>

In a1ba4627931591b80afa46e38e261f354151d91a:

[1.8.x] Fixed #24225, #24264, #24282 -- Rewrote model reloading in migration project state

Instead of naively reloading only directly related models (FK, O2O, M2M
relationship) the project state needs to reload their relations as well
as the model changes as well. Furthermore inheriting models (and super
models) need to be reloaded in order to keep inherited fields in sync.

To prevent endless recursive calls an iterative approach is taken.

Backport of b29f3b51204d53c1c8745966476543d068c173a2 from master

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