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.

3 files changed, 129 insertions, 12 deletions

diff --git a/tests/migrations/test_autodetector.py b/tests/migrations/test_autodetector.py
index a56e31568a..2b8e940c4e 100644
--- a/tests/migrations/test_autodetector.py
+++ b/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")
diff --git a/tests/migrations/test_operations.py b/tests/migrations/test_operations.py
index 6472fea42c..ec0d046df8 100644
--- a/tests/migrations/test_operations.py
+++ b/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.
diff --git a/tests/migrations/test_state.py b/tests/migrations/test_state.py
index 10f5e7d9ab..045ac6be10 100644
--- a/tests/migrations/test_state.py
+++ b/tests/migrations/test_state.py
@@ -291,3 +291,33 @@ class StateTests(TestCase):
         ))
         self.assertNotEqual(project_state, other_state)
         self.assertEqual(project_state == other_state, False)
+
+    def test_dangling_references_throw_error(self):
+        class Author(models.Model):
+            name = models.TextField()
+
+        class Book(models.Model):
+            author = models.ForeignKey(Author)
+
+        class Magazine(models.Model):
+            authors = models.ManyToManyField(Author)
+
+        # 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()