From 25bf15c0dac3371be66db0173d26715b9f167529 Mon Sep 17 00:00:00 2001 From: Simon Charette Date: Fri, 3 Apr 2020 22:57:42 -0400 Subject: [PATCH] Refs #22608 -- Made app_label required when optimizing migrations. This paved the way for the removal of lot of logic when app_label was not specified. --- django/db/migrations/autodetector.py | 2 +- django/db/migrations/operations/base.py | 2 +- django/db/migrations/operations/fields.py | 20 ++++++------- django/db/migrations/operations/models.py | 16 +++++------ django/db/migrations/optimizer.py | 9 +++--- tests/migrations/test_optimizer.py | 35 ++++++++++++++++------- 6 files changed, 48 insertions(+), 36 deletions(-) diff --git a/django/db/migrations/autodetector.py b/django/db/migrations/autodetector.py index 73df3a38c1..f01c9feffd 100644 --- a/django/db/migrations/autodetector.py +++ b/django/db/migrations/autodetector.py @@ -369,7 +369,7 @@ class MigrationAutodetector: # Optimize migrations for app_label, migrations in self.migrations.items(): for migration in migrations: - migration.operations = MigrationOptimizer().optimize(migration.operations, app_label=app_label) + migration.operations = MigrationOptimizer().optimize(migration.operations, app_label) def check_dependency(self, operation, dependency): """ diff --git a/django/db/migrations/operations/base.py b/django/db/migrations/operations/base.py index b2f4ddd7d4..f02eae20b4 100644 --- a/django/db/migrations/operations/base.py +++ b/django/db/migrations/operations/base.py @@ -113,7 +113,7 @@ class Operation: return router.allow_migrate_model(connection_alias, model) - def reduce(self, operation, app_label=None): + def reduce(self, operation, app_label): """ Return either a list of operations the actual operation should be replaced with or a boolean that indicates whether or not the specified diff --git a/django/db/migrations/operations/fields.py b/django/db/migrations/operations/fields.py index 86e44e822f..08dfdf430f 100644 --- a/django/db/migrations/operations/fields.py +++ b/django/db/migrations/operations/fields.py @@ -60,9 +60,9 @@ class FieldOperation(Operation): return True return False - def reduce(self, operation, app_label=None): + def reduce(self, operation, app_label): return ( - super().reduce(operation, app_label=app_label) or + super().reduce(operation, app_label) or not operation.references_field(self.model_name, self.name, app_label) ) @@ -122,7 +122,7 @@ class AddField(FieldOperation): def describe(self): return "Add field %s to %s" % (self.name, self.model_name) - def reduce(self, operation, app_label=None): + def reduce(self, operation, app_label): if isinstance(operation, FieldOperation) and self.is_same_field_operation(operation): if isinstance(operation, AlterField): return [ @@ -142,7 +142,7 @@ class AddField(FieldOperation): field=self.field, ), ] - return super().reduce(operation, app_label=app_label) + return super().reduce(operation, app_label) class RemoveField(FieldOperation): @@ -186,11 +186,11 @@ class RemoveField(FieldOperation): def describe(self): return "Remove field %s from %s" % (self.name, self.model_name) - def reduce(self, operation, app_label=None): + def reduce(self, operation, app_label): from .models import DeleteModel if isinstance(operation, DeleteModel) and operation.name_lower == self.model_name_lower: return [operation] - return super().reduce(operation, app_label=app_label) + return super().reduce(operation, app_label) class AlterField(FieldOperation): @@ -256,7 +256,7 @@ class AlterField(FieldOperation): def describe(self): return "Alter field %s on %s" % (self.name, self.model_name) - def reduce(self, operation, app_label=None): + def reduce(self, operation, app_label): if isinstance(operation, RemoveField) and self.is_same_field_operation(operation): return [operation] elif isinstance(operation, RenameField) and self.is_same_field_operation(operation): @@ -268,7 +268,7 @@ class AlterField(FieldOperation): field=self.field, ), ] - return super().reduce(operation, app_label=app_label) + return super().reduce(operation, app_label) class RenameField(FieldOperation): @@ -382,7 +382,7 @@ class RenameField(FieldOperation): name.lower() == self.new_name_lower ) - def reduce(self, operation, app_label=None): + def reduce(self, operation, app_label): if (isinstance(operation, RenameField) and self.is_same_model_operation(operation) and self.new_name_lower == operation.old_name_lower): @@ -396,6 +396,6 @@ class RenameField(FieldOperation): # Skip `FieldOperation.reduce` as we want to run `references_field` # against self.new_name. return ( - super(FieldOperation, self).reduce(operation, app_label=app_label) or + super(FieldOperation, self).reduce(operation, app_label) or not operation.references_field(self.model_name, self.new_name, app_label) ) diff --git a/django/db/migrations/operations/models.py b/django/db/migrations/operations/models.py index 66ecd606ee..1947bb1edf 100644 --- a/django/db/migrations/operations/models.py +++ b/django/db/migrations/operations/models.py @@ -31,9 +31,9 @@ class ModelOperation(Operation): def references_model(self, name, app_label=None): return name.lower() == self.name_lower - def reduce(self, operation, app_label=None): + def reduce(self, operation, app_label): return ( - super().reduce(operation, app_label=app_label) or + super().reduce(operation, app_label) or not operation.references_model(self.name, app_label) ) @@ -117,7 +117,7 @@ class CreateModel(ModelOperation): return True return False - def reduce(self, operation, app_label=None): + def reduce(self, operation, app_label): if (isinstance(operation, DeleteModel) and self.name_lower == operation.name_lower and not self.options.get("proxy", False)): @@ -236,7 +236,7 @@ class CreateModel(ModelOperation): managers=self.managers, ), ] - return super().reduce(operation, app_label=app_label) + return super().reduce(operation, app_label) class DeleteModel(ModelOperation): @@ -411,7 +411,7 @@ class RenameModel(ModelOperation): def describe(self): return "Rename model %s to %s" % (self.old_name, self.new_name) - def reduce(self, operation, app_label=None): + def reduce(self, operation, app_label): if (isinstance(operation, RenameModel) and self.new_name_lower == operation.old_name_lower): return [ @@ -423,16 +423,16 @@ class RenameModel(ModelOperation): # Skip `ModelOperation.reduce` as we want to run `references_model` # against self.new_name. return ( - super(ModelOperation, self).reduce(operation, app_label=app_label) or + super(ModelOperation, self).reduce(operation, app_label) or not operation.references_model(self.new_name, app_label) ) class ModelOptionOperation(ModelOperation): - def reduce(self, operation, app_label=None): + def reduce(self, operation, app_label): if isinstance(operation, (self.__class__, DeleteModel)) and self.name_lower == operation.name_lower: return [operation] - return super().reduce(operation, app_label=app_label) + return super().reduce(operation, app_label) class AlterModelTable(ModelOptionOperation): diff --git a/django/db/migrations/optimizer.py b/django/db/migrations/optimizer.py index f66a318d8a..ee20f62af2 100644 --- a/django/db/migrations/optimizer.py +++ b/django/db/migrations/optimizer.py @@ -9,7 +9,7 @@ class MigrationOptimizer: nothing. """ - def optimize(self, operations, app_label=None): + def optimize(self, operations, app_label): """ Main optimization entry point. Pass in a list of Operation instances, get out a new list of Operation instances. @@ -25,11 +25,10 @@ class MigrationOptimizer: The inner loop is run until the starting list is the same as the result list, and then the result is returned. This means that operation optimization must be stable and always return an equal or shorter list. - - The app_label argument is optional, but if you pass it you'll get more - efficient optimization. """ # Internal tracking variable for test assertions about # of loops + if app_label is None: + raise TypeError('app_label must be a str.') self._iterations = 0 while True: result = self.optimize_inner(operations, app_label) @@ -38,7 +37,7 @@ class MigrationOptimizer: return result operations = result - def optimize_inner(self, operations, app_label=None): + def optimize_inner(self, operations, app_label): """Inner optimization loop.""" new_operations = [] for i, operation in enumerate(operations): diff --git a/tests/migrations/test_optimizer.py b/tests/migrations/test_optimizer.py index 439ab39f2b..2950635514 100644 --- a/tests/migrations/test_optimizer.py +++ b/tests/migrations/test_optimizer.py @@ -23,7 +23,7 @@ class OptimizerTests(SimpleTestCase): return serializer_factory(value).serialize()[0] def assertOptimizesTo(self, operations, expected, exact=None, less_than=None, app_label=None): - result, iterations = self.optimize(operations, app_label) + result, iterations = self.optimize(operations, app_label or 'migrations') result = [self.serialize(f) for f in result] expected = [self.serialize(f) for f in expected] self.assertEqual(expected, result) @@ -39,6 +39,11 @@ class OptimizerTests(SimpleTestCase): def assertDoesNotOptimize(self, operations, **kwargs): self.assertOptimizesTo(operations, operations, **kwargs) + def test_none_app_label(self): + optimizer = MigrationOptimizer() + with self.assertRaisesMessage(TypeError, 'app_label must be a str'): + optimizer.optimize([], None) + def test_single(self): """ The optimizer does nothing on a single operation, @@ -212,16 +217,8 @@ class OptimizerTests(SimpleTestCase): ], [], ) - # This should not work - FK should block it - self.assertDoesNotOptimize( - [ - migrations.CreateModel("Foo", [("name", models.CharField(max_length=255))]), - migrations.CreateModel("Bar", [("other", models.ForeignKey("testapp.Foo", models.CASCADE))]), - migrations.DeleteModel("Foo"), - ], - ) - # The same operations should be optimized if app_label is specified and - # a FK references a model from the other app. + # Operations should be optimized if the FK references a model from the + # other app. self.assertOptimizesTo( [ migrations.CreateModel("Foo", [("name", models.CharField(max_length=255))]), @@ -235,6 +232,13 @@ class OptimizerTests(SimpleTestCase): ) # But it shouldn't work if a FK references a model with the same # app_label. + self.assertDoesNotOptimize( + [ + migrations.CreateModel('Foo', [('name', models.CharField(max_length=255))]), + migrations.CreateModel('Bar', [('other', models.ForeignKey('Foo', models.CASCADE))]), + migrations.DeleteModel('Foo'), + ], + ) self.assertDoesNotOptimize( [ migrations.CreateModel("Foo", [("name", models.CharField(max_length=255))]), @@ -244,12 +248,20 @@ class OptimizerTests(SimpleTestCase): app_label="testapp", ) # This should not work - bases should block it + self.assertDoesNotOptimize( + [ + migrations.CreateModel('Foo', [('name', models.CharField(max_length=255))]), + migrations.CreateModel('Bar', [('size', models.IntegerField())], bases=('Foo',)), + migrations.DeleteModel('Foo'), + ], + ) self.assertDoesNotOptimize( [ migrations.CreateModel("Foo", [("name", models.CharField(max_length=255))]), migrations.CreateModel("Bar", [("size", models.IntegerField())], bases=("testapp.Foo",)), migrations.DeleteModel("Foo"), ], + app_label='testapp', ) # The same operations should be optimized if app_label and none of # bases belong to that app. @@ -293,6 +305,7 @@ class OptimizerTests(SimpleTestCase): ('reviewer', models.ForeignKey('test_app.Reviewer', models.CASCADE)), ]), ], + app_label='test_app', ) def test_create_model_add_field(self):