From 5c9c1e029d139bd3d5213804af2ed9f317cd0b86 Mon Sep 17 00:00:00 2001 From: Markus Holtermann Date: Tue, 28 Oct 2014 00:28:37 +0100 Subject: [PATCH] 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 | 28 ++++++++-- docs/releases/1.7.2.txt | 3 ++ tests/migrations/test_autodetector.py | 77 +++++++++++++-------------- 3 files changed, 65 insertions(+), 43 deletions(-) diff --git a/django/db/migrations/autodetector.py b/django/db/migrations/autodetector.py index 574f99f4c6e..7369cfad5c3 100644 --- a/django/db/migrations/autodetector.py +++ b/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): diff --git a/docs/releases/1.7.2.txt b/docs/releases/1.7.2.txt index db65ec1885e..245512650fe 100644 --- a/docs/releases/1.7.2.txt +++ b/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`). diff --git a/tests/migrations/test_autodetector.py b/tests/migrations/test_autodetector.py index 89f5eee430b..33236c320f2 100644 --- a/tests/migrations/test_autodetector.py +++ b/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.assertEqual(len(changes['otherapp']), 1) - # Right number of actions? - 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")}) + self.assertNumberMigrations(changes, "otherapp", 1) + self.assertOperationTypes(changes, "otherapp", 0, ["AlterUniqueTogether"]) + self.assertOperationAttributes(changes, "otherapp", 0, 0, name="book", 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.assertEqual(len(changes['otherapp']), 1) - # Right number of actions? - migration = changes['otherapp'][0] - self.assertEqual(len(migration.operations), 2) - # Right actions order? - action1 = migration.operations[0] - action2 = migration.operations[1] - self.assertEqual(action1.__class__.__name__, "AddField") - self.assertEqual(action2.__class__.__name__, "AlterUniqueTogether") - self.assertEqual(action2.unique_together, {("title", "newfield")}) + self.assertNumberMigrations(changes, "otherapp", 1) + self.assertOperationTypes(changes, "otherapp", 0, ["AddField", "AlterUniqueTogether"]) + self.assertOperationAttributes(changes, "otherapp", 0, 1, name="book", unique_together={("title", "newfield")}) + + def test_remove_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]) + autodetector = MigrationAutodetector(before, after) + changes = autodetector._detect_changes() + 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.assertEqual(len(changes['testapp']), 1) - migration = changes['testapp'][0] - # 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()) + self.assertNumberMigrations(changes, "testapp", 1) + self.assertOperationTypes(changes, "testapp", 0, ["AlterIndexTogether"]) + self.assertOperationAttributes(changes, "testapp", 0, 0, name="author", 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.assertEqual(len(changes['testapp']), 1) - migration = changes['testapp'][0] - # 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()) + self.assertNumberMigrations(changes, "testapp", 1) + self.assertOperationTypes(changes, "testapp", 0, ["AlterUniqueTogether"]) + self.assertOperationAttributes(changes, "testapp", 0, 0, name="author", unique_together=set()) def test_proxy(self): "Tests that the autodetector correctly deals with proxy models"