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 , 11 years ago
comment:2 by , 10 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 10 years ago
Verified this also happens if you simultaneously delete two models with ForeignKey fields pointing to one another.
comment:4 by , 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 , 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.
comment:6 by , 10 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Version: | master → 1.7-beta-2 |
comment:7 by , 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 , 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 , 10 years ago
Owner: | changed from | to
---|
comment:10 by , 10 years ago
andrewsg: Thanks for your work on this. Here's hoping we can get it fixed soon...
comment:11 by , 10 years ago
The aforementioned rewrite is now in progress here: https://github.com/andrewgodwin/django/compare/autodetector_rewrite
comment:12 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
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.