Fixed #23614 -- Changed the way the migration autodetector orders unique/index_together

Thanks to Naddiseo for the report and Tim Graham for the review

django/db/migrations/autodetector.py

@@ -198,7 +198,9 @@ class MigrationAutodetector(object):
                         if dep[0] == app_label:
                             # Alright, there's a dependency on the same app.
                             for j, op2 in enumerate(ops):
-                                if self.check_dependency(op2, dep) and j > i:
+                                if j > i and self.check_dependency(op2, dep):
+                                    # shift the operation from position i after
+                                    # the operation at position j
                                     ops = ops[:i] + ops[i + 1:j + 1] + [op] + ops[j + 1:]
                                     found = True
                                     break
@@ -320,7 +322,8 @@ class MigrationAutodetector(object):
 
     def check_dependency(self, operation, dependency):
         """
-        Checks if an operation dependency matches an operation.
+        Returns ``True`` if the given operation depends on the given dependency,
+        ``False`` otherwise.
         """
         # Created model
         if dependency[2] is None and dependency[3] is True:
@@ -369,6 +372,19 @@ class MigrationAutodetector(object):
                 operation.name.lower() == dependency[1].lower() and
                 (operation.order_with_respect_to or "").lower() != dependency[2].lower()
             )
+        # Field is removed and part of an index/unique_together
+        elif dependency[2] is not None and dependency[3] == "foo_together_change":
+            if operation.name.lower() == dependency[1].lower():
+                return (
+                    (
+                        isinstance(operation, operations.AlterUniqueTogether) and
+                        any(dependency[2] not in t for t in operation.unique_together)
+                    ) or
+                    (
+                        isinstance(operation, operations.AlterIndexTogether) and
+                        any(dependency[2] not in t for t in operation.index_together)
+                    )
+                )
         # Unknown dependency. Raise an error.
         else:
             raise ValueError("Can't handle dependency %r" % (dependency, ))
@@ -828,9 +844,13 @@ class MigrationAutodetector(object):
                     model_name=model_name,
                     name=field_name,
                 ),
-                # We might need to depend on the removal of an order_with_respect_to;
+                # We might need to depend on the removal of an
+                # order_with_respect_to or index/unique_together operation;
                 # this is safely ignored if there isn't one
-                dependencies=[(app_label, model_name, field_name, "order_wrt_unset")],
+                dependencies=[
+                    (app_label, model_name, field_name, "order_wrt_unset"),
+                    (app_label, model_name, field_name, "foo_together_change"),
+                ],
             )
 
     def generate_altered_fields(self):

docs/releases/1.7.2.txt

@@ -29,3 +29,6 @@ Bugfixes
 
 * Fixed MySQL 5.6+ crash with ``GeometryField``\s in migrations
   (:ticket:`23719`).
+
+* Fixed a migration crash when removing a field that is referenced in
+  ``AlterIndexTogether`` or ``AlterUniqueTogether`` (:ticket:`23614`).

tests/migrations/test_autodetector.py

@@ -86,6 +86,12 @@ class AutodetectorTests(TestCase):
     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")}})
+    book_unique_4 = ModelState("otherapp", "Book", [
+        ("id", models.AutoField(primary_key=True)),
+        ("newfield2", models.IntegerField()),
+        ("author", models.ForeignKey("testapp.Author")),
+        ("title", models.CharField(max_length=200)),
+    ], {"unique_together": {("title", "newfield2")}})
     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))], bases=(AbstractBaseUser, ))
@@ -749,16 +755,9 @@ class AutodetectorTests(TestCase):
         after = self.make_project_state([self.author_empty, self.book_unique_2])
         autodetector = MigrationAutodetector(before, after)
         changes = autodetector._detect_changes()
-        # Right number of migrations?
+        self.assertNumberMigrations(changes, "otherapp", 1)
-        self.assertEqual(len(changes['otherapp']), 1)
+        self.assertOperationTypes(changes, "otherapp", 0, ["AlterUniqueTogether"])
-        # Right number of actions?
+        self.assertOperationAttributes(changes, "otherapp", 0, 0, name="book", unique_together={("title", "author")})
-        migration = changes['otherapp'][0]
-        self.assertEqual(len(migration.operations), 1)
-        # Right action?
-        action = migration.operations[0]
-        self.assertEqual(action.__class__.__name__, "AlterUniqueTogether")
-        self.assertEqual(action.name, "book")
-        self.assertEqual(action.unique_together, {("title", "author")})
 
     def test_add_field_and_unique_together(self):
         "Tests that added fields will be created before using them in unique together"
@@ -766,17 +765,29 @@ class AutodetectorTests(TestCase):
         after = self.make_project_state([self.author_empty, self.book_unique_3])
         autodetector = MigrationAutodetector(before, after)
         changes = autodetector._detect_changes()
-        # Right number of migrations?
+        self.assertNumberMigrations(changes, "otherapp", 1)
-        self.assertEqual(len(changes['otherapp']), 1)
+        self.assertOperationTypes(changes, "otherapp", 0, ["AddField", "AlterUniqueTogether"])
-        # Right number of actions?
+        self.assertOperationAttributes(changes, "otherapp", 0, 1, name="book", unique_together={("title", "newfield")})
-        migration = changes['otherapp'][0]
+
-        self.assertEqual(len(migration.operations), 2)
+    def test_remove_field_and_unique_together(self):
-        # Right actions order?
+        "Tests that removed fields will be removed after updating unique_together"
-        action1 = migration.operations[0]
+        before = self.make_project_state([self.author_empty, self.book_unique_3])
-        action2 = migration.operations[1]
+        after = self.make_project_state([self.author_empty, self.book_unique])
-        self.assertEqual(action1.__class__.__name__, "AddField")
+        autodetector = MigrationAutodetector(before, after)
-        self.assertEqual(action2.__class__.__name__, "AlterUniqueTogether")
+        changes = autodetector._detect_changes()
-        self.assertEqual(action2.unique_together, {("title", "newfield")})
+        self.assertNumberMigrations(changes, "otherapp", 1)
+        self.assertOperationTypes(changes, "otherapp", 0, ["AlterUniqueTogether", "RemoveField"])
+        self.assertOperationAttributes(changes, "otherapp", 0, 0, name="book", unique_together={("author", "title")})
+
+    def test_rename_field_and_unique_together(self):
+        "Tests that removed fields will be removed after updating unique together"
+        before = self.make_project_state([self.author_empty, self.book_unique_3])
+        after = self.make_project_state([self.author_empty, self.book_unique_4])
+        autodetector = MigrationAutodetector(before, after, MigrationQuestioner({"ask_rename": True}))
+        changes = autodetector._detect_changes()
+        self.assertNumberMigrations(changes, "otherapp", 1)
+        self.assertOperationTypes(changes, "otherapp", 0, ["RenameField", "AlterUniqueTogether"])
+        self.assertOperationAttributes(changes, "otherapp", 0, 1, name="book", unique_together={("title", "newfield2")})
 
     def test_remove_index_together(self):
         author_index_together = ModelState("testapp", "Author", [
@@ -787,15 +798,9 @@ class AutodetectorTests(TestCase):
         after = self.make_project_state([self.author_name])
         autodetector = MigrationAutodetector(before, after)
         changes = autodetector._detect_changes()
-        # Right number of migrations?
+        self.assertNumberMigrations(changes, "testapp", 1)
-        self.assertEqual(len(changes['testapp']), 1)
+        self.assertOperationTypes(changes, "testapp", 0, ["AlterIndexTogether"])
-        migration = changes['testapp'][0]
+        self.assertOperationAttributes(changes, "testapp", 0, 0, name="author", index_together=set())
-        # Right number of actions?
-        self.assertEqual(len(migration.operations), 1)
-        # Right actions?
-        action = migration.operations[0]
-        self.assertEqual(action.__class__.__name__, "AlterIndexTogether")
-        self.assertEqual(action.index_together, set())
 
     def test_remove_unique_together(self):
         author_unique_together = ModelState("testapp", "Author", [
@@ -806,15 +811,9 @@ class AutodetectorTests(TestCase):
         after = self.make_project_state([self.author_name])
         autodetector = MigrationAutodetector(before, after)
         changes = autodetector._detect_changes()
-        # Right number of migrations?
+        self.assertNumberMigrations(changes, "testapp", 1)
-        self.assertEqual(len(changes['testapp']), 1)
+        self.assertOperationTypes(changes, "testapp", 0, ["AlterUniqueTogether"])
-        migration = changes['testapp'][0]
+        self.assertOperationAttributes(changes, "testapp", 0, 0, name="author", unique_together=set())
-        # Right number of actions?
-        self.assertEqual(len(migration.operations), 1)
-        # Right actions?
-        action = migration.operations[0]
-        self.assertEqual(action.__class__.__name__, "AlterUniqueTogether")
-        self.assertEqual(action.unique_together, set())
 
     def test_proxy(self):
         "Tests that the autodetector correctly deals with proxy models"