Opened 11 years ago

Closed 10 years ago

#22605 closed Bug (fixed)

Impossible to delete two models with a M2M field

Reported by: Andrew Godwin Owned by: Andrew Godwin
Component: Migrations Version: 1.7-beta-2
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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

Change History (12)

comment:1 by Andrew Godwin, 11 years ago

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.

comment:2 by andrewsg, 10 years ago

Owner: changed from nobody to andrewsg
Status: newassigned

comment:3 by andrewsg, 10 years ago

Verified this also happens if you simultaneously delete two models with ForeignKey fields pointing to one another.

comment:4 by andrewsg, 10 years ago

Verified this may also happen if you simultaneously delete two models with even a single ForeignKey field pointing from one to the other.

comment:5 by andrewsg, 10 years ago

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_changes 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.

Last edited 10 years ago by andrewsg (previous) (diff)

comment:6 by Tim Graham, 10 years ago

Triage Stage: UnreviewedAccepted
Version: master1.7-beta-2

comment:7 by Andrew Godwin, 10 years ago

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!

comment:8 by andrewsg, 10 years ago

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.

comment:9 by Andrew Godwin, 10 years ago

Owner: changed from andrewsg to Andrew Godwin

comment:10 by Andrew Godwin, 10 years ago

andrewsg: Thanks for your work on this. Here's hoping we can get it fixed soon...

comment:11 by Andrew Godwin, 10 years ago

The aforementioned rewrite is now in progress here: https://github.com/andrewgodwin/django/compare/autodetector_rewrite

comment:12 by Andrew Godwin <andrew@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In 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

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