Fixed #22397 -- Issues removing M2M field with explicit through model

Changed the migration autodetector to remove models last so that FK
and M2M fields will not be left as dangling references. Added a check
in the migration state renderer to error out in the presence of
dangling references instead of leaving them as strings. Fixed a bug
in the sqlite backend to handle the deletion of M2M fields with
"through" models properly (i.e., do nothing successfully).

Thanks to melinath for report, loic for tests and andrewgodwin and
charettes for assistance with architecture.

django/db/backends/sqlite3/schema.py

@@ -121,11 +121,15 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor):
         Removes a field from a model. Usually involves deleting a column,
         but for M2Ms may involve deleting a table.
         """
-        # Special-case implicit M2M tables
-        if isinstance(field, ManyToManyField) and field.rel.through._meta.auto_created:
-            return self.delete_model(field.rel.through)
+        # M2M fields are a special case
+        if isinstance(field, ManyToManyField):
+            # For implicit M2M tables, delete the auto-created table
+            if field.rel.through._meta.auto_created:
+                self.delete_model(field.rel.through)
+            # For explicit "through" M2M fields, do nothing
         # For everything else, remake.
-        self._remake_table(model, delete_fields=[field])
+        else:
+            self._remake_table(model, delete_fields=[field])
 
     def alter_field(self, model, old_field, new_field, strict=False):
         """

django/db/migrations/autodetector.py

@@ -223,16 +223,6 @@ class MigrationAutodetector(object):
                     unique_together=unique_together
                 )
             )
-        # Removing models
-        removed_models = set(old_model_keys) - set(new_model_keys)
-        for app_label, model_name in removed_models:
-            model_state = self.from_state.models[app_label, model_name]
-            self.add_to_migration(
-                app_label,
-                operations.DeleteModel(
-                    model_state.name,
-                )
-            )
         # Changes within models
         kept_models = set(old_model_keys).intersection(new_model_keys)
         old_fields = set()
@@ -348,6 +338,16 @@ class MigrationAutodetector(object):
                 )
         for app_label, operation in unique_together_operations:
             self.add_to_migration(app_label, operation)
+        # Removing models
+        removed_models = set(old_model_keys) - set(new_model_keys)
+        for app_label, model_name in removed_models:
+            model_state = self.from_state.models[app_label, model_name]
+            self.add_to_migration(
+                app_label,
+                operations.DeleteModel(
+                    model_state.name,
+                )
+            )
         # Alright, now add internal dependencies
         for app_label, migrations in self.migrations.items():
             for m1, m2 in zip(migrations, migrations[1:]):

django/db/migrations/migration.py

@@ -127,7 +127,7 @@ class Migration(object):
         to_run.reverse()
         for operation, to_state, from_state in to_run:
             operation.database_backwards(self.app_label, schema_editor, from_state, to_state)
-
+        return project_state
 
 def swappable_dependency(value):
     """

django/db/migrations/state.py

@@ -51,6 +51,16 @@ class ProjectState(object):
                 if len(new_unrendered_models) == len(unrendered_models):
                     raise InvalidBasesError("Cannot resolve bases for %r" % new_unrendered_models)
                 unrendered_models = new_unrendered_models
+            # make sure apps has no dangling references
+            if self.apps._pending_lookups:
+                # Raise an error with a best-effort helpful message
+                # (only for the first issue). Error message should look like:
+                # "ValueError: Lookup failed for model referenced by
+                # field migrations.Book.author: migrations.Author"
+                dangling_lookup = list(self.apps._pending_lookups.items())[0]
+                raise ValueError("Lookup failed for model referenced by field {field}: {model[0]}.{model[1]}".format(
+                    field=dangling_lookup[1][0][1],
+                    model=dangling_lookup[0]))
         return self.apps
 
     @classmethod

tests/migrations/test_autodetector.py

@@ -33,11 +33,15 @@ class AutodetectorTests(TestCase):
     other_stable = ModelState("otherapp", "Stable", [("id", models.AutoField(primary_key=True))])
     third_thing = ModelState("thirdapp", "Thing", [("id", models.AutoField(primary_key=True))])
     book = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("author", models.ForeignKey("testapp.Author")), ("title", models.CharField(max_length=200))])
+    book_with_no_author = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("title", models.CharField(max_length=200))])
     book_with_author_renamed = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("author", models.ForeignKey("testapp.Writer")), ("title", models.CharField(max_length=200))])
     book_with_field_and_author_renamed = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("writer", models.ForeignKey("testapp.Writer")), ("title", models.CharField(max_length=200))])
+    book_with_multiple_authors = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("authors", models.ManyToManyField("testapp.Author")), ("title", models.CharField(max_length=200))])
+    book_with_multiple_authors_through_attribution = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("authors", models.ManyToManyField("testapp.Author", through="otherapp.Attribution")), ("title", models.CharField(max_length=200))])
     book_unique = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("author", models.ForeignKey("testapp.Author")), ("title", models.CharField(max_length=200))], {"unique_together": [("author", "title")]})
     book_unique_2 = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("author", models.ForeignKey("testapp.Author")), ("title", models.CharField(max_length=200))], {"unique_together": [("title", "author")]})
     book_unique_3 = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("newfield", models.IntegerField()), ("author", models.ForeignKey("testapp.Author")), ("title", models.CharField(max_length=200))], {"unique_together": [("title", "newfield")]})
+    attribution = ModelState("otherapp", "Attribution", [("id", models.AutoField(primary_key=True)), ("author", models.ForeignKey("testapp.Author")), ("book", models.ForeignKey("otherapp.Book"))])
     edition = ModelState("thirdapp", "Edition", [("id", models.AutoField(primary_key=True)), ("book", models.ForeignKey("otherapp.Book"))])
     custom_user = ModelState("thirdapp", "CustomUser", [("id", models.AutoField(primary_key=True)), ("username", models.CharField(max_length=255))])
     knight = ModelState("eggs", "Knight", [("id", models.AutoField(primary_key=True))])
@@ -552,18 +556,65 @@ class AutodetectorTests(TestCase):
         """
         # Make state
         before = self.make_project_state([self.author_with_publisher_string])
-        after = self.make_project_state([self.author_with_publisher])
+        after = self.make_project_state([self.author_with_publisher, self.publisher])
         autodetector = MigrationAutodetector(before, after)
         changes = autodetector._detect_changes()
         # Right number of migrations?
         self.assertEqual(len(changes['testapp']), 1)
         # Right number of actions?
         migration = changes['testapp'][0]
-        self.assertEqual(len(migration.operations), 2)
+        self.assertEqual(len(migration.operations), 3)
         # Right actions?
         action = migration.operations[0]
+        self.assertEqual(action.__class__.__name__, "CreateModel")
+        self.assertEqual(action.name, "Publisher")
+        action = migration.operations[1]
         self.assertEqual(action.__class__.__name__, "AddField")
         self.assertEqual(action.name, "publisher")
-        action = migration.operations[1]
+        action = migration.operations[2]
         self.assertEqual(action.__class__.__name__, "RemoveField")
         self.assertEqual(action.name, "publisher_name")
+
+    def test_foreign_key_removed_before_target_model(self):
+        """
+        Removing an FK and the model it targets in the same change must remove
+        the FK field before the model to maintain consistency.
+        """
+        before = self.make_project_state([self.author_with_publisher, self.publisher])
+        after = self.make_project_state([self.author_name]) # removes both the model and FK
+        autodetector = MigrationAutodetector(before, after)
+        changes = autodetector._detect_changes()
+        # Right number of migrations?
+        self.assertEqual(len(changes['testapp']), 1)
+        # Right number of actions?
+        migration = changes['testapp'][0]
+        self.assertEqual(len(migration.operations), 2)
+        # Right actions in right order?
+        action = migration.operations[0]
+        self.assertEqual(action.__class__.__name__, "RemoveField")
+        self.assertEqual(action.name, "publisher")
+        action = migration.operations[1]
+        self.assertEqual(action.__class__.__name__, "DeleteModel")
+        self.assertEqual(action.name, "Publisher")
+
+    def test_many_to_many_removed_before_through_model(self):
+        """
+        Removing a ManyToManyField and the "through" model in the same change must remove
+        the field before the model to maintain consistency.
+        """
+        before = self.make_project_state([self.book_with_multiple_authors_through_attribution, self.author_name, self.attribution])
+        after = self.make_project_state([self.book_with_no_author, self.author_name]) # removes both the through model and ManyToMany
+        autodetector = MigrationAutodetector(before, after)
+        changes = autodetector._detect_changes()
+        # Right number of migrations?
+        self.assertEqual(len(changes['otherapp']), 1)
+        # Right number of actions?
+        migration = changes['otherapp'][0]
+        self.assertEqual(len(migration.operations), 2)
+        # Right actions in right order?
+        action = migration.operations[0]
+        self.assertEqual(action.__class__.__name__, "RemoveField")
+        self.assertEqual(action.name, "authors")
+        action = migration.operations[1]
+        self.assertEqual(action.__class__.__name__, "DeleteModel")
+        self.assertEqual(action.name, "Attribution")

tests/migrations/test_operations.py

@@ -1,9 +1,12 @@
 import unittest
-from django.db import connection, models, migrations, router
+
+from django.db import connection, migrations, models, router
+from django.db.migrations.migration import Migration
+from django.db.migrations.state import ProjectState
 from django.db.models.fields import NOT_PROVIDED
 from django.db.transaction import atomic
 from django.db.utils import IntegrityError
-from django.db.migrations.state import ProjectState
+
 from .test_base import MigrationTestBase
 
 
@@ -15,15 +18,16 @@ class OperationTests(MigrationTestBase):
     """
 
     def apply_operations(self, app_label, project_state, operations):
-        new_state = project_state.clone()
-        for operation in operations:
-            operation.state_forwards(app_label, new_state)
+        migration = Migration('name', app_label)
+        migration.operations = operations
+        with connection.schema_editor() as editor:
+            return migration.apply(project_state, editor)
 
-        # Set up the database
+    def unapply_operations(self, app_label, project_state, operations):
+        migration = Migration('name', app_label)
+        migration.operations = operations
         with connection.schema_editor() as editor:
-            for operation in operations:
-                operation.database_forwards(app_label, editor, project_state, new_state)
-        return new_state
+            return migration.unapply(project_state, editor)
 
     def set_up_test_model(self, app_label, second_model=False, related_model=False, mti_model=False):
         """
@@ -396,6 +400,38 @@ class OperationTests(MigrationTestBase):
         Pony = new_apps.get_model("test_alflmm", "Pony")
         self.assertTrue(Pony._meta.get_field('stables').blank)
 
+    def test_remove_field_m2m(self):
+        project_state = self.set_up_test_model("test_rmflmm", second_model=True)
+
+        project_state = self.apply_operations("test_rmflmm", project_state, operations=[
+            migrations.AddField("Pony", "stables", models.ManyToManyField("Stable", related_name="ponies"))
+        ])
+        self.assertTableExists("test_rmflmm_pony_stables")
+
+        operations = [migrations.RemoveField("Pony", "stables")]
+        self.apply_operations("test_rmflmm", project_state, operations=operations)
+        self.assertTableNotExists("test_rmflmm_pony_stables")
+
+        # And test reversal
+        self.unapply_operations("test_rmflmm", project_state, operations=operations)
+        self.assertTableExists("test_rmflmm_pony_stables")
+
+    def test_remove_field_m2m_with_through(self):
+        project_state = self.set_up_test_model("test_rmflmmwt", second_model=True)
+
+        self.assertTableNotExists("test_rmflmmwt_ponystables")
+        project_state = self.apply_operations("test_rmflmmwt", project_state, operations=[
+            migrations.CreateModel("PonyStables", fields=[
+                ("pony", models.ForeignKey('test_rmflmmwt.Pony')),
+                ("stable", models.ForeignKey('test_rmflmmwt.Stable')),
+            ]),
+            migrations.AddField("Pony", "stables", models.ManyToManyField("Stable", related_name="ponies", through='test_rmflmmwt.PonyStables'))
+        ])
+        self.assertTableExists("test_rmflmmwt_ponystables")
+
+        operations = [migrations.RemoveField("Pony", "stables")]
+        self.apply_operations("test_rmflmmwt", project_state, operations=operations)
+
     def test_remove_field(self):
         """
         Tests the RemoveField operation.

tests/migrations/test_state.py

@@ -291,3 +291,44 @@ class StateTests(TestCase):
         ))
         self.assertNotEqual(project_state, other_state)
         self.assertEqual(project_state == other_state, False)
+
+    def test_dangling_references_throw_error(self):
+        new_apps = Apps()
+
+        class Author(models.Model):
+            name = models.TextField()
+            class Meta:
+                app_label = "migrations"
+                apps = new_apps
+
+        class Book(models.Model):
+            author = models.ForeignKey(Author)
+            class Meta:
+                app_label = "migrations"
+                apps = new_apps
+
+        class Magazine(models.Model):
+            authors = models.ManyToManyField(Author)
+            class Meta:
+                app_label = "migrations"
+                apps = new_apps
+
+        # Make a valid ProjectState and render it
+        project_state = ProjectState()
+        project_state.add_model_state(ModelState.from_model(Author))
+        project_state.add_model_state(ModelState.from_model(Book))
+        project_state.add_model_state(ModelState.from_model(Magazine))
+        rendered_state = project_state.render()
+        self.assertEqual(len(rendered_state.get_models()), 3)
+
+        # now make an invalid one with a ForeignKey
+        project_state = ProjectState()
+        project_state.add_model_state(ModelState.from_model(Book))
+        with self.assertRaises(ValueError):
+            rendered_state = project_state.render()
+
+        # and another with ManyToManyField
+        project_state = ProjectState()
+        project_state.add_model_state(ModelState.from_model(Magazine))
+        with self.assertRaises(ValueError):
+            rendered_state = project_state.render()