Impossible to delete two models with a M2M field

[Andrew Godwin]

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

[Andrew Godwin]

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.

[andrewsg]

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

[andrewsg]

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

[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_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.

[Andrew Godwin]

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!

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

[Andrew Godwin]

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

[Andrew Godwin]

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

[Andrew Godwin <andrew@…>]

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