[Django] #22605: Impossible to delete two models with a M2M field
Django
---------------------------------+--------------------
Reporter: andrewgodwin | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Severity: Release blocker | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+--------------------
In playing around with some test code, I removed the following models and
made a migration to delete them:
{{{
class Table1(models.Model):
pass
class Table2(models.Model):
m2m = models.ManyToManyField(Table1, through='M2M', blank=True)
class M2M(models.Model):
t1 = models.ForeignKey(Table1, related_name='+')
t2 = models.ForeignKey(Table2, related_name='+')
}}}
The resulting deletion is impossible, as Table2 and M2M have a circular
dependency and the "lookup failed" code means you can never get past the
migration with errors such as:
{{{
Applying polls.0005_auto_20140509_0258...Traceback (most recent call
last):
File "./manage.py", line 10, in <module>
execute_from_command_line(sys.argv)
File "/home/andrew/Programs/Django/django/core/management/__init__.py",
line 427, in execute_from_command_line
utility.execute()
File "/home/andrew/Programs/Django/django/core/management/__init__.py",
line 419, in execute
self.fetch_command(subcommand).run_from_argv(self.argv)
File "/home/andrew/Programs/Django/django/core/management/base.py", line
288, in run_from_argv
self.execute(*args, **options.__dict__)
File "/home/andrew/Programs/Django/django/core/management/base.py", line
337, in execute
output = self.handle(*args, **options)
File
"/home/andrew/Programs/Django/django/core/management/commands/migrate.py",
line 146, in handle
executor.migrate(targets, plan, fake=options.get("fake", False))
File "/home/andrew/Programs/Django/django/db/migrations/executor.py",
line 62, in migrate
self.apply_migration(migration, fake=fake)
File "/home/andrew/Programs/Django/django/db/migrations/executor.py",
line 96, in apply_migration
migration.apply(project_state, schema_editor)
File "/home/andrew/Programs/Django/django/db/migrations/migration.py",
line 107, in apply
operation.database_forwards(self.app_label, schema_editor,
project_state, new_state)
File
"/home/andrew/Programs/Django/django/db/migrations/operations/models.py",
line 80, in database_forwards
apps = from_state.render()
File "/home/andrew/Programs/Django/django/db/migrations/state.py", line
94, in render
extra_message=extra_message,
ValueError: Lookup failed for model referenced by field polls.Table2.m2m:
polls.M2M
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22605>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: andrewgodwin | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
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 andrewgodwin):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
I'm not yet sure how to fix this, but noting it here for posterity while I
fix something else. I suspect we may need the DeleteModel's state_forward
to remove ForeignKey references to the model as well, or at least neuter
them somehow.
The reason this doesn't happen for the creation case is that
makemigrations recognised the problem and split the creation of M2M into
two operations. A similar method could also be used as an alternative
approach.
--
Ticket URL: <https://code.djangoproject.com/ticket/22605#comment:1>
Django
Reporter: andrewgodwin | Owner: andrewsg
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by andrewsg):
* status: new => assigned
* owner: nobody => andrewsg
--
Ticket URL: <https://code.djangoproject.com/ticket/22605#comment:2>
Django
Reporter: andrewgodwin | Owner: andrewsg
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by andrewsg):
Verified this also happens if you simultaneously delete two models with
ForeignKey fields pointing to one another.
--
Ticket URL: <https://code.djangoproject.com/ticket/22605#comment:3>
Django
Reporter: andrewgodwin | Owner: andrewsg
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by andrewsg):
Verified this may also happen if you simultaneously delete two models with
even a single ForeignKey field pointing from one to the other.
--
Ticket URL: <https://code.djangoproject.com/ticket/22605#comment:4>
Django
Reporter: andrewgodwin | Owner: andrewsg
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by andrewsg):
I have a prototype autodetector-based solution, but I'm not happy with it
-- it adds a substantial bit of code to the monolithic _detect_chagnes
function (similar to how the autodetector has a 3.1-phase process for
adding models, it becomes a two or three phase process for removing
models). I'll want to consult with a core dev before going further down
this path, because I don't know how complex the alternative state_forward-
based solution would be.
--
Ticket URL: <https://code.djangoproject.com/ticket/22605#comment:5>
Django
Reporter: andrewgodwin | Owner: andrewsg
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* version: master => 1.7-beta-2
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/22605#comment:6>
Django
Reporter: andrewgodwin | Owner: andrewsg
Type: Bug | Status: assigned
Component: Migrations | Version: 1.7-beta-2
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by andrewgodwin):
andrewsg: I have been mulling re-architecting the autodetector to fix this
and a slew of other dependency bugs, so I think the solution may lie down
that road (in particular, I want to change the autodetector to be two-
phase; that is, it works out the changes to make, and then re-orders and
sorts them so the dependencies line up).
Given that, I'm not sure it would be a good idea to do any further work
here, unless you want to; would you mind if I took over the ticket? I'd
appreciate any code you have so far though, as I can probably use it!
--
Ticket URL: <https://code.djangoproject.com/ticket/22605#comment:7>
Django
Reporter: andrewgodwin | Owner: andrewsg
Type: Bug | Status: assigned
Component: Migrations | Version: 1.7-beta-2
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by andrewsg):
andrewgodwin: this commit:
https://github.com/andrewsg/django/commit/ccb3843d482d0e9019f5dc9ffab06130fd1eb8fb
is the current state of my branch, which is an unhappy place between a
working and inefficient prototype (delete all models with no
entanglements; delete ALL fields with references from those models; delete
all remaining models) and my goal, which was for the autodetector to work
out the strategy with the fewest migrations and operations.
I stopped work when I realized that I really needed the delete models step
to be two-phase like you mentioned, and figure out how to do the actual
work intelligently. The strategy my code followed initially, mimicking
the create model code, was too "greedy". Generalizing this so the
autodetector does all of the work like this, so the add model and remove
model special cases can be unified, sounds like a great idea. Please feel
free to take over the ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/22605#comment:8>
Django
Reporter: andrewgodwin | Owner: andrewgodwin
Type: Bug | Status: assigned
Component: Migrations | Version: 1.7-beta-2
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by andrewgodwin):
* owner: andrewsg => andrewgodwin
--
Ticket URL: <https://code.djangoproject.com/ticket/22605#comment:9>
Django
Reporter: andrewgodwin | Owner: andrewgodwin
Type: Bug | Status: assigned
Component: Migrations | Version: 1.7-beta-2
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by andrewgodwin):
andrewsg: Thanks for your work on this. Here's hoping we can get it fixed
soon...
--
Ticket URL: <https://code.djangoproject.com/ticket/22605#comment:10>
Django
Reporter: andrewgodwin | Owner: andrewgodwin
Type: Bug | Status: assigned
Component: Migrations | Version: 1.7-beta-2
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by andrewgodwin):
The aforementioned rewrite is now in progress here:
https://github.com/andrewgodwin/django/compare/autodetector_rewrite
--
Ticket URL: <https://code.djangoproject.com/ticket/22605#comment:11>
Django
Reporter: andrewgodwin | Owner: andrewgodwin
Component: Migrations | Version: 1.7-beta-2
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Andrew Godwin <andrew@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"31fc34e447137631e6ea58fc33f3642e65479472"]:
{{{
#!CommitTicketReference repository=""
revision="31fc34e447137631e6ea58fc33f3642e65479472"
[1.7.x] Rewrote migration autodetector to involve actual computer science.
Fixes #22605, #22735; also lays the ground for some other fixes.
Conflicts:
django/db/migrations/autodetector.py
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22605#comment:12>