From ad82900ad94ed4bbad050b9993373dafbe66b610 Mon Sep 17 00:00:00 2001 From: Simon Charette Date: Thu, 2 Feb 2017 22:09:12 -0500 Subject: [PATCH] Fixed #26720 -- Prevented invalid CreateModel optimizations of related fields. --- django/db/migrations/operations/fields.py | 61 ++++++++++++++++------- django/db/migrations/operations/models.py | 5 ++ tests/migrations/test_optimizer.py | 29 +++++++++++ 3 files changed, 77 insertions(+), 18 deletions(-) diff --git a/django/db/migrations/operations/fields.py b/django/db/migrations/operations/fields.py index 05e57da000..66231292e8 100644 --- a/django/db/migrations/operations/fields.py +++ b/django/db/migrations/operations/fields.py @@ -7,9 +7,10 @@ from .utils import is_referenced_by_foreign_key class FieldOperation(Operation): - def __init__(self, model_name, name): + def __init__(self, model_name, name, field=None): self.model_name = model_name self.name = name + self.field = field @cached_property def model_name_lower(self): @@ -29,22 +30,42 @@ class FieldOperation(Operation): name_lower = name.lower() if name_lower == self.model_name_lower: return True - field = getattr(self, 'field', None) - if field and field.remote_field: - remote_app_label, remote_model_name = self.model_to_key(field.remote_field.model) - if (remote_model_name == name_lower and app_label is None or - not remote_app_label or remote_app_label == app_label): - return True - through = getattr(field.remote_field, 'through', None) - if through and self.model_to_key(through) == (app_label, name_lower): - through_app_label, through_model_name = self.model_to_key(through) - if (through_model_name == name_lower and app_label is None or - not through_app_label or through_app_label == app_label): + if self.field: + if self.field.remote_field: + remote_app_label, remote_model_name = self.model_to_key(self.field.remote_field.model) + if (remote_model_name == name_lower and app_label is None or + not remote_app_label or remote_app_label == app_label): return True - return False + through = getattr(self.field.remote_field, 'through', None) + if through and self.model_to_key(through) == (app_label, name_lower): + through_app_label, through_model_name = self.model_to_key(through) + if (through_model_name == name_lower and app_label is None or + not through_app_label or through_app_label == app_label): + return True + return False + return True def references_field(self, model_name, name, app_label=None): - return self.references_model(model_name, app_label) and name.lower() == self.name_lower + if self.field: + model_name_lower = model_name.lower() + remote_field = self.field.remote_field + if remote_field: + remote_app_label, remote_model_name = self.model_to_key(remote_field.model) + if (remote_model_name == model_name_lower and + (app_label is None or not remote_app_label or remote_app_label == app_label)): + # TODO: Consider to_fields/from_fields. + return True + through = getattr(remote_field, 'through', None) + if through and self.model_to_key(through) == (app_label, model_name_lower): + through_app_label, through_model_name = self.model_to_key(through) + if (through_model_name == model_name_lower and + (app_label is None or not through_app_label or through_app_label == app_label) and + (remote_field.through_fields is None or name in remote_field.through_fields)): + return True + elif model_name_lower == self.model_name_lower and name == self.name: + return True + return False + return True def reduce(self, operation, in_between, app_label=None): return ( @@ -57,9 +78,8 @@ class AddField(FieldOperation): """Add a field to a model.""" def __init__(self, model_name, name, field, preserve_default=True): - self.field = field self.preserve_default = preserve_default - super().__init__(model_name, name) + super().__init__(model_name, name, field) def deconstruct(self): kwargs = { @@ -173,6 +193,12 @@ class RemoveField(FieldOperation): def describe(self): return "Remove field %s from %s" % (self.name, self.model_name) + def reduce(self, operation, in_between, app_label=None): + from .models import DeleteModel + if isinstance(operation, DeleteModel) and operation.name_lower == self.model_name_lower: + return [operation] + return super().reduce(operation, in_between, app_label=app_label) + class AlterField(FieldOperation): """ @@ -181,9 +207,8 @@ class AlterField(FieldOperation): """ def __init__(self, model_name, name, field, preserve_default=True): - self.field = field self.preserve_default = preserve_default - super().__init__(model_name, name) + super().__init__(model_name, name, field) def deconstruct(self): kwargs = { diff --git a/django/db/migrations/operations/models.py b/django/db/migrations/operations/models.py index 24147751a4..88f3507c22 100644 --- a/django/db/migrations/operations/models.py +++ b/django/db/migrations/operations/models.py @@ -225,6 +225,11 @@ class DeleteModel(ModelOperation): if self.allow_migrate_model(schema_editor.connection.alias, model): schema_editor.create_model(model) + def references_model(self, name, app_label=None): + # The deleted model could be referencing the specified model through + # related fields. + return True + def describe(self): return "Delete model %s" % self.name diff --git a/tests/migrations/test_optimizer.py b/tests/migrations/test_optimizer.py index 6c1c5f5c6b..973be67c80 100644 --- a/tests/migrations/test_optimizer.py +++ b/tests/migrations/test_optimizer.py @@ -270,6 +270,35 @@ class OptimizerTests(SimpleTestCase): app_label="testapp", ) + # This could be optimized a bit more but it generates a valid set of + # operations. + self.assertOptimizesTo( + [ + migrations.CreateModel('Book', [('name', models.CharField(max_length=255))]), + migrations.CreateModel('Person', [('name', models.CharField(max_length=255))]), + migrations.AddField('book', 'author', models.ForeignKey('test_app.Person', models.CASCADE)), + migrations.CreateModel('Review', [('book', models.ForeignKey('test_app.Book', models.CASCADE))]), + migrations.CreateModel('Reviewer', [('name', models.CharField(max_length=255))]), + migrations.AddField('review', 'reviewer', models.ForeignKey('test_app.Reviewer', models.CASCADE)), + migrations.RemoveField('book', 'author'), + migrations.DeleteModel('Person'), + ], + [ + migrations.CreateModel('Person', [('name', models.CharField(max_length=255))]), + migrations.CreateModel('Book', [ + ('name', models.CharField(max_length=255)), + ('author', models.ForeignKey('test_app.Person', models.CASCADE)), + ]), + migrations.CreateModel('Reviewer', [('name', models.CharField(max_length=255))]), + migrations.CreateModel('Review', [ + ('book', models.ForeignKey('test_app.Book', models.CASCADE)), + ('reviewer', models.ForeignKey('test_app.Reviewer', models.CASCADE)), + ]), + migrations.RemoveField('book', 'author'), + migrations.DeleteModel('Person'), + ], + ) + def test_create_model_add_field(self): """ AddField should optimize into CreateModel.