From e470f311d654267ec86f9a6325ec500345b9dff2 Mon Sep 17 00:00:00 2001 From: Markus Holtermann Date: Sat, 13 Jun 2015 00:21:07 +0200 Subject: [PATCH] Fixed #24828 -- Allowed migration optimization across AlterFooTogether The idea behind this change is, that AlterUniqueTogether, AlterIndexTogether and AlterOrderWithRespectTo can always be moved after an Add/Alter/Rename/RemoveField operation if they don't refer to the respective field and are not empty sets / None. Combined with the optimizations of duplicate AlterUniqueTogether, AlterIndexTogether, and AlterOrderWithRespectTo operations from 128caa1e16ec2627737748f75c8e55600a3df97f, these operations are optimized in a later round of the optimizer. Thanks Tim Graham for the review. --- django/db/migrations/operations/models.py | 27 ++++ django/db/migrations/optimizer.py | 24 ++++ tests/migrations/test_autodetector.py | 7 +- tests/migrations/test_commands.py | 2 +- tests/migrations/test_optimizer.py | 152 ++++++++++++++++++++++ 5 files changed, 208 insertions(+), 4 deletions(-) diff --git a/django/db/migrations/operations/models.py b/django/db/migrations/operations/models.py index 69998331fb..2855107e0e 100644 --- a/django/db/migrations/operations/models.py +++ b/django/db/migrations/operations/models.py @@ -365,6 +365,15 @@ class AlterUniqueTogether(Operation): def references_model(self, name, app_label=None): return name.lower() == self.name_lower + def references_field(self, model_name, name, app_label=None): + return ( + self.references_model(model_name, app_label) and + ( + not self.unique_together or + any((name in together) for together in self.unique_together) + ) + ) + def describe(self): return "Alter %s for %s (%s constraint(s))" % (self.option_name, self.name, len(self.unique_together or '')) @@ -417,6 +426,15 @@ class AlterIndexTogether(Operation): def references_model(self, name, app_label=None): return name.lower() == self.name_lower + def references_field(self, model_name, name, app_label=None): + return ( + self.references_model(model_name, app_label) and + ( + not self.index_together or + any((name in together) for together in self.index_together) + ) + ) + def describe(self): return "Alter %s for %s (%s constraint(s))" % (self.option_name, self.name, len(self.index_together or '')) @@ -474,6 +492,15 @@ class AlterOrderWithRespectTo(Operation): def references_model(self, name, app_label=None): return name.lower() == self.name_lower + def references_field(self, model_name, name, app_label=None): + return ( + self.references_model(model_name, app_label) and + ( + self.order_with_respect_to is None or + name == self.order_with_respect_to + ) + ) + def describe(self): return "Set order_with_respect_to on %s to %s" % (self.name, self.order_with_respect_to) diff --git a/django/db/migrations/optimizer.py b/django/db/migrations/optimizer.py index 556436b96d..96909e748e 100644 --- a/django/db/migrations/optimizer.py +++ b/django/db/migrations/optimizer.py @@ -56,6 +56,20 @@ class MigrationOptimizer(object): (AlterField, RenameField): self.reduce_alter_field_rename_field, (CreateModel, RenameField): self.reduce_create_model_rename_field, (RenameField, RenameField): self.reduce_rename_field_self, + + (AlterIndexTogether, AddField): self.reduce_alter_model_addalterremove_field, + (AlterIndexTogether, AlterField): self.reduce_alter_model_addalterremove_field, + (AlterIndexTogether, RemoveField): self.reduce_alter_model_addalterremove_field, + (AlterOrderWithRespectTo, AddField): self.reduce_alter_model_addalterremove_field, + (AlterOrderWithRespectTo, AlterField): self.reduce_alter_model_addalterremove_field, + (AlterOrderWithRespectTo, RemoveField): self.reduce_alter_model_addalterremove_field, + (AlterUniqueTogether, AddField): self.reduce_alter_model_addalterremove_field, + (AlterUniqueTogether, AlterField): self.reduce_alter_model_addalterremove_field, + (AlterUniqueTogether, RemoveField): self.reduce_alter_model_addalterremove_field, + + (AlterIndexTogether, RenameField): self.reduce_alter_model_rename_field, + (AlterOrderWithRespectTo, RenameField): self.reduce_alter_model_rename_field, + (AlterUniqueTogether, RenameField): self.reduce_alter_model_rename_field, } def optimize(self, operations, app_label=None): @@ -310,6 +324,16 @@ class MigrationOptimizer(object): ), ] + def reduce_alter_model_addalterremove_field(self, operation, other, in_between): + if (operation.name_lower == other.model_name_lower and + not operation.references_field(other.model_name, other.name)): + return [other, operation] + + def reduce_alter_model_rename_field(self, operation, other, in_between): + if (operation.name_lower == other.model_name_lower and + not operation.references_field(other.model_name, other.old_name)): + return [other, operation] + # THROUGH CHECKS def can_optimize_through(self, operation, other, app_label=None): diff --git a/tests/migrations/test_autodetector.py b/tests/migrations/test_autodetector.py index d5ddd30894..e6497314dd 100644 --- a/tests/migrations/test_autodetector.py +++ b/tests/migrations/test_autodetector.py @@ -1149,9 +1149,10 @@ class AutodetectorTests(TestCase): changes = autodetector._detect_changes() # Right number/type of migrations? self.assertNumberMigrations(changes, "otherapp", 1) - self.assertOperationTypes(changes, "otherapp", 0, ["AlterUniqueTogether", "AlterIndexTogether", "RemoveField"]) - self.assertOperationAttributes(changes, "otherapp", 0, 0, name="book", unique_together={("author", "title")}) - self.assertOperationAttributes(changes, "otherapp", 0, 1, name="book", index_together={("author", "title")}) + self.assertOperationTypes(changes, "otherapp", 0, ["RemoveField", "AlterUniqueTogether", "AlterIndexTogether"]) + self.assertOperationAttributes(changes, "otherapp", 0, 0, model_name="book", name="newfield") + self.assertOperationAttributes(changes, "otherapp", 0, 1, name="book", unique_together={("author", "title")}) + self.assertOperationAttributes(changes, "otherapp", 0, 2, name="book", index_together={("author", "title")}) def test_rename_field_and_foo_together(self): """ diff --git a/tests/migrations/test_commands.py b/tests/migrations/test_commands.py index ac4f55fa03..3077dd9e61 100644 --- a/tests/migrations/test_commands.py +++ b/tests/migrations/test_commands.py @@ -889,7 +889,7 @@ class SquashMigrationsTests(MigrationTestBase): out = six.StringIO() with self.temporary_migration_module(module="migrations.test_migrations"): call_command("squashmigrations", "migrations", "0002", interactive=False, verbosity=1, stdout=out) - self.assertIn("Optimized from 7 operations to 5 operations.", force_text(out.getvalue())) + self.assertIn("Optimized from 7 operations to 3 operations.", force_text(out.getvalue())) def test_ticket_23799_squashmigrations_no_optimize(self): """ diff --git a/tests/migrations/test_optimizer.py b/tests/migrations/test_optimizer.py index faf297b5e7..dccb783ac7 100644 --- a/tests/migrations/test_optimizer.py +++ b/tests/migrations/test_optimizer.py @@ -29,6 +29,9 @@ class OptimizerTests(SimpleTestCase): if less_than is not None and iterations >= less_than: raise self.failureException("Optimization did not take less than %s iterations (it took %s)" % (less_than, iterations)) + def assertDoesNotOptimize(self, operations): + self.assertOptimizesTo(operations, operations) + def test_single(self): """ Tests that the optimizer does nothing on a single operation, @@ -447,6 +450,155 @@ class OptimizerTests(SimpleTestCase): ], ) + def _test_create_alter_foo_field(self, alter): + """ + CreateModel, AlterFooTogether/AlterOrderWithRespectTo followed by an + add/alter/rename field should optimize to CreateModel and the Alter* + """ + # AddField + self.assertOptimizesTo( + [ + migrations.CreateModel("Foo", [ + ("a", models.IntegerField()), + ("b", models.IntegerField()), + ]), + alter, + migrations.AddField("Foo", "c", models.IntegerField()), + ], + [ + migrations.CreateModel("Foo", [ + ("a", models.IntegerField()), + ("b", models.IntegerField()), + ("c", models.IntegerField()), + ]), + alter, + ], + ) + + # AlterField + self.assertDoesNotOptimize( + [ + migrations.CreateModel("Foo", [ + ("a", models.IntegerField()), + ("b", models.IntegerField()), + ]), + alter, + migrations.AlterField("Foo", "b", models.CharField(max_length=255)), + ], + ) + + self.assertOptimizesTo( + [ + migrations.CreateModel("Foo", [ + ("a", models.IntegerField()), + ("b", models.IntegerField()), + ("c", models.IntegerField()), + ]), + alter, + migrations.AlterField("Foo", "c", models.CharField(max_length=255)), + ], + [ + migrations.CreateModel("Foo", [ + ("a", models.IntegerField()), + ("b", models.IntegerField()), + ("c", models.CharField(max_length=255)), + ]), + alter, + ], + ) + + # RenameField + self.assertDoesNotOptimize( + [ + migrations.CreateModel("Foo", [ + ("a", models.IntegerField()), + ("b", models.IntegerField()), + ]), + alter, + migrations.RenameField("Foo", "b", "c"), + ], + ) + + self.assertOptimizesTo( + [ + migrations.CreateModel("Foo", [ + ("a", models.IntegerField()), + ("b", models.IntegerField()), + ]), + alter, + migrations.RenameField("Foo", "b", "x"), + migrations.RenameField("Foo", "x", "c"), + ], + [ + migrations.CreateModel("Foo", [ + ("a", models.IntegerField()), + ("b", models.IntegerField()), + ]), + alter, + migrations.RenameField("Foo", "b", "c"), + ], + ) + + self.assertOptimizesTo( + [ + migrations.CreateModel("Foo", [ + ("a", models.IntegerField()), + ("b", models.IntegerField()), + ("c", models.IntegerField()), + ]), + alter, + migrations.RenameField("Foo", "c", "d"), + ], + [ + migrations.CreateModel("Foo", [ + ("a", models.IntegerField()), + ("b", models.IntegerField()), + ("d", models.IntegerField()), + ]), + alter, + ], + ) + + # RemoveField + self.assertDoesNotOptimize( + [ + migrations.CreateModel("Foo", [ + ("a", models.IntegerField()), + ("b", models.IntegerField()), + ]), + alter, + migrations.RemoveField("Foo", "b"), + ], + ) + + self.assertOptimizesTo( + [ + migrations.CreateModel("Foo", [ + ("a", models.IntegerField()), + ("b", models.IntegerField()), + ("c", models.IntegerField()), + ]), + alter, + migrations.RemoveField("Foo", "c"), + ], + [ + migrations.CreateModel("Foo", [ + ("a", models.IntegerField()), + ("b", models.IntegerField()), + ]), + alter, + ], + ) + + def test_create_alter_unique_field(self): + self._test_create_alter_foo_field(migrations.AlterUniqueTogether("Foo", [["a", "b"]])) + + def test_create_alter_index_field(self): + self._test_create_alter_foo_field(migrations.AlterIndexTogether("Foo", [["a", "b"]])) + + def test_create_alter_owrt_field(self): + self._test_create_alter_foo_field(migrations.AlterOrderWithRespectTo("Foo", "b")) + def test_optimize_through_fields(self): """ Checks that field-level through checking is working.