From 106b019dc9b48aa749947e406e5445feb045e8a2 Mon Sep 17 00:00:00 2001 From: Andrew Godwin Date: Wed, 6 Nov 2013 13:47:58 +0000 Subject: [PATCH] Massive migration optimiser improvements + RenameModel opn --- django/db/migrations/operations/__init__.py | 4 +- django/db/migrations/operations/base.py | 12 ++ django/db/migrations/operations/fields.py | 39 +++- django/db/migrations/operations/models.py | 48 +++++ django/db/migrations/optimizer.py | 217 +++++++++++++++++++- tests/migrations/test_operations.py | 24 +++ tests/migrations/test_optimizer.py | 195 +++++++++++++++++- 7 files changed, 520 insertions(+), 19 deletions(-) diff --git a/django/db/migrations/operations/__init__.py b/django/db/migrations/operations/__init__.py index c26fd847442..440b9305267 100644 --- a/django/db/migrations/operations/__init__.py +++ b/django/db/migrations/operations/__init__.py @@ -1,11 +1,11 @@ from .models import (CreateModel, DeleteModel, AlterModelTable, - AlterUniqueTogether, AlterIndexTogether) + AlterUniqueTogether, AlterIndexTogether, RenameModel) from .fields import AddField, RemoveField, AlterField, RenameField from .special import SeparateDatabaseAndState, RunSQL, RunPython __all__ = [ 'CreateModel', 'DeleteModel', 'AlterModelTable', 'AlterUniqueTogether', - 'AlterIndexTogether', + 'RenameModel', 'AlterIndexTogether', 'AddField', 'RemoveField', 'AlterField', 'RenameField', 'SeparateDatabaseAndState', 'RunSQL', 'RunPython', ] diff --git a/django/db/migrations/operations/base.py b/django/db/migrations/operations/base.py index 69f24b1dc67..8b8602e81a0 100644 --- a/django/db/migrations/operations/base.py +++ b/django/db/migrations/operations/base.py @@ -9,6 +9,9 @@ class Operation(object): Note that some operations won't modify memory state at all (e.g. data copying operations), and some will need their modifications to be optionally specified by the user (e.g. custom Python code snippets) + + Due to the way this class deals with deconstruction, it should be + considered immutable. """ # If this migration can be run in reverse. @@ -76,6 +79,15 @@ class Operation(object): """ return True + def references_field(self, model_name, name, app_label=None): + """ + Returns True if there is a chance this operation references the given + field name, with an optional app label for accuracy. + + Used for optimization. If in doubt, return True. + """ + return self.references_model(model_name, app_label) + def __repr__(self): return "<%s %s%s>" % ( self.__class__.__name__, diff --git a/django/db/migrations/operations/fields.py b/django/db/migrations/operations/fields.py index 6a567d727a3..5b687c66564 100644 --- a/django/db/migrations/operations/fields.py +++ b/django/db/migrations/operations/fields.py @@ -8,7 +8,7 @@ class AddField(Operation): """ def __init__(self, model_name, name, field): - self.model_name = model_name.lower() + self.model_name = model_name self.name = name self.field = field @@ -33,10 +33,16 @@ class AddField(Operation): return ( (self.__class__ == other.__class__) and (self.name == other.name) and - (self.model_name == other.model_name) and + (self.model_name.lower() == other.model_name.lower()) and (self.field.deconstruct()[1:] == other.field.deconstruct()[1:]) ) + def references_model(self, name, app_label=None): + return name.lower() == self.model_name.lower() + + def references_field(self, model_name, name, app_label=None): + return self.references_model(model_name) and name.lower() == self.name.lower() + class RemoveField(Operation): """ @@ -44,7 +50,7 @@ class RemoveField(Operation): """ def __init__(self, model_name, name): - self.model_name = model_name.lower() + self.model_name = model_name self.name = name def state_forwards(self, app_label, state): @@ -68,6 +74,12 @@ class RemoveField(Operation): def describe(self): return "Remove field %s from %s" % (self.name, self.model_name) + def references_model(self, name, app_label=None): + return name.lower() == self.model_name.lower() + + def references_field(self, model_name, name, app_label=None): + return self.references_model(model_name) and name.lower() == self.name.lower() + class AlterField(Operation): """ @@ -75,7 +87,7 @@ class AlterField(Operation): """ def __init__(self, model_name, name, field): - self.model_name = model_name.lower() + self.model_name = model_name self.name = name self.field = field @@ -104,10 +116,16 @@ class AlterField(Operation): return ( (self.__class__ == other.__class__) and (self.name == other.name) and - (self.model_name == other.model_name) and + (self.model_name.lower() == other.model_name.lower()) and (self.field.deconstruct()[1:] == other.field.deconstruct()[1:]) ) + def references_model(self, name, app_label=None): + return name.lower() == self.model_name.lower() + + def references_field(self, model_name, name, app_label=None): + return self.references_model(model_name) and name.lower() == self.name.lower() + class RenameField(Operation): """ @@ -115,7 +133,7 @@ class RenameField(Operation): """ def __init__(self, model_name, old_name, new_name): - self.model_name = model_name.lower() + self.model_name = model_name self.old_name = old_name self.new_name = new_name @@ -146,3 +164,12 @@ class RenameField(Operation): def describe(self): return "Rename field %s on %s to %s" % (self.old_name, self.model_name, self.new_name) + + def references_model(self, name, app_label=None): + return name.lower() == self.model_name.lower() + + def references_field(self, model_name, name, app_label=None): + return self.references_model(model_name) and ( + name.lower() == self.old_name.lower() or + name.lower() == self.new_name.lower() + ) diff --git a/django/db/migrations/operations/models.py b/django/db/migrations/operations/models.py index 5c573aac75f..8c7fe126a80 100644 --- a/django/db/migrations/operations/models.py +++ b/django/db/migrations/operations/models.py @@ -91,6 +91,54 @@ class DeleteModel(Operation): return "Delete model %s" % (self.name, ) +class RenameModel(Operation): + """ + Renames a model. + """ + + def __init__(self, old_name, new_name): + self.old_name = old_name + self.new_name = new_name + + def state_forwards(self, app_label, state): + state.models[app_label, self.new_name.lower()] = state.models[app_label, self.old_name.lower()] + state.models[app_label, self.new_name.lower()].name = self.new_name + del state.models[app_label, self.old_name.lower()] + + def database_forwards(self, app_label, schema_editor, from_state, to_state): + old_app_cache = from_state.render() + new_app_cache = to_state.render() + old_model = old_app_cache.get_model(app_label, self.old_name) + new_model = new_app_cache.get_model(app_label, self.new_name) + if router.allow_migrate(schema_editor.connection.alias, new_model): + schema_editor.alter_db_table( + new_model, + old_model._meta.db_table, + new_model._meta.db_table, + ) + + def database_backwards(self, app_label, schema_editor, from_state, to_state): + old_app_cache = from_state.render() + new_app_cache = to_state.render() + old_model = old_app_cache.get_model(app_label, self.new_name) + new_model = new_app_cache.get_model(app_label, self.old_name) + if router.allow_migrate(schema_editor.connection.alias, new_model): + schema_editor.alter_db_table( + new_model, + old_model._meta.db_table, + new_model._meta.db_table, + ) + + def references_model(self, name, app_label=None): + return ( + name.lower() == self.old_name.lower() or + name.lower() == self.new_name.lower() + ) + + def describe(self): + return "Rename model %s to %s" % (self.old_name, self.new_name) + + class AlterModelTable(Operation): """ Renames a model's table diff --git a/django/db/migrations/optimizer.py b/django/db/migrations/optimizer.py index fe527a5c3ee..b87dc29bdb8 100644 --- a/django/db/migrations/optimizer.py +++ b/django/db/migrations/optimizer.py @@ -71,10 +71,86 @@ class MigrationOptimizer(object): or None, meaning this pair cannot be optimized. """ submethods = [ - (migrations.CreateModel, migrations.DeleteModel, self.reduce_model_create_delete), - (migrations.AlterModelTable, migrations.DeleteModel, self.reduce_model_alter_delete), - (migrations.AlterUniqueTogether, migrations.DeleteModel, self.reduce_model_alter_delete), - (migrations.AlterIndexTogether, migrations.DeleteModel, self.reduce_model_alter_delete), + ( + migrations.CreateModel, + migrations.DeleteModel, + self.reduce_model_create_delete, + ), + ( + migrations.AlterModelTable, + migrations.DeleteModel, + self.reduce_model_alter_delete, + ), + ( + migrations.AlterUniqueTogether, + migrations.DeleteModel, + self.reduce_model_alter_delete, + ), + ( + migrations.AlterIndexTogether, + migrations.DeleteModel, + self.reduce_model_alter_delete, + ), + ( + migrations.CreateModel, + migrations.RenameModel, + self.reduce_model_create_rename, + ), + ( + migrations.RenameModel, + migrations.RenameModel, + self.reduce_model_rename_self, + ), + ( + migrations.CreateModel, + migrations.AddField, + self.reduce_create_model_add_field, + ), + ( + migrations.CreateModel, + migrations.AlterField, + self.reduce_create_model_alter_field, + ), + ( + migrations.CreateModel, + migrations.RemoveField, + self.reduce_create_model_remove_field, + ), + ( + migrations.AddField, + migrations.AlterField, + self.reduce_add_field_alter_field, + ), + ( + migrations.AddField, + migrations.RemoveField, + self.reduce_add_field_delete_field, + ), + ( + migrations.AlterField, + migrations.RemoveField, + self.reduce_alter_field_delete_field, + ), + ( + migrations.AddField, + migrations.RenameField, + self.reduce_add_field_rename_field, + ), + ( + migrations.AlterField, + migrations.RenameField, + self.reduce_alter_field_rename_field, + ), + ( + migrations.CreateModel, + migrations.RenameField, + self.reduce_create_model_rename_field, + ), + ( + migrations.RenameField, + migrations.RenameField, + self.reduce_rename_field_self, + ), ] for ia, ib, om in submethods: if isinstance(operation, ia) and isinstance(other, ib): @@ -85,17 +161,130 @@ class MigrationOptimizer(object): """ Folds a CreateModel and a DeleteModel into nothing. """ - if operation.name == other.name: + if operation.name.lower() == other.name.lower(): return [] - return None def reduce_model_alter_delete(self, operation, other): """ - Folds an AlterModelSomething and a DeleteModel into nothing. + Folds an AlterModelSomething and a DeleteModel into just delete. """ - if operation.name == other.name: + if operation.name.lower() == other.name.lower(): return [other] - return None + + def reduce_model_create_rename(self, operation, other): + """ + Folds a model rename into its create + """ + if operation.name.lower() == other.old_name.lower(): + return [migrations.CreateModel( + other.new_name, + fields = operation.fields, + options = operation.options, + bases = operation.bases, + )] + + def reduce_model_rename_self(self, operation, other): + """ + Folds a model rename into another one + """ + if operation.new_name.lower() == other.old_name.lower(): + return [ + migrations.RenameModel( + operation.old_name, + other.new_name, + ) + ] + + def reduce_create_model_add_field(self, operation, other): + if operation.name.lower() == other.model_name.lower(): + return [migrations.CreateModel( + operation.name, + fields = operation.fields + [(other.name, other.field)], + options = operation.options, + bases = operation.bases, + )] + + def reduce_create_model_alter_field(self, operation, other): + if operation.name.lower() == other.model_name.lower(): + return [migrations.CreateModel( + operation.name, + fields = [ + (n, other.field if n == other.name else v) + for n, v in operation.fields + ], + options = operation.options, + bases = operation.bases, + )] + + def reduce_create_model_rename_field(self, operation, other): + if operation.name.lower() == other.model_name.lower(): + return [migrations.CreateModel( + operation.name, + fields = [ + (other.new_name if n == other.old_name else n, v) + for n, v in operation.fields + ], + options = operation.options, + bases = operation.bases, + )] + + def reduce_create_model_remove_field(self, operation, other): + if operation.name.lower() == other.model_name.lower(): + return [migrations.CreateModel( + operation.name, + fields = [ + (n, v) + for n, v in operation.fields + if n.lower() != other.name.lower() + ], + options = operation.options, + bases = operation.bases, + )] + + def reduce_add_field_alter_field(self, operation, other): + if operation.model_name.lower() == other.model_name.lower() and operation.name.lower() == other.name.lower(): + return [migrations.AddField( + model_name = operation.model_name, + name = operation.name, + field = other.field, + )] + + def reduce_add_field_delete_field(self, operation, other): + if operation.model_name.lower() == other.model_name.lower() and operation.name.lower() == other.name.lower(): + return [] + + def reduce_alter_field_delete_field(self, operation, other): + if operation.model_name.lower() == other.model_name.lower() and operation.name.lower() == other.name.lower(): + return [other] + + def reduce_add_field_rename_field(self, operation, other): + if operation.model_name.lower() == other.model_name.lower() and operation.name.lower() == other.old_name.lower(): + return [migrations.AddField( + model_name = operation.model_name, + name = other.new_name, + field = operation.field, + )] + + def reduce_alter_field_rename_field(self, operation, other): + if operation.model_name.lower() == other.model_name.lower() and operation.name.lower() == other.old_name.lower(): + return [ + other, + migrations.AlterField( + model_name = operation.model_name, + name = other.new_name, + field = operation.field, + ), + ] + + def reduce_rename_field_self(self, operation, other): + if operation.model_name.lower() == other.model_name.lower() and operation.new_name.lower() == other.old_name.lower(): + return [ + migrations.RenameField( + operation.model_name, + operation.old_name, + other.new_name, + ), + ] #### THROUGH CHECKS #### @@ -107,14 +296,22 @@ class MigrationOptimizer(object): """ MODEL_LEVEL_OPERATIONS = ( migrations.CreateModel, - migrations.DeleteModel, migrations.AlterModelTable, migrations.AlterUniqueTogether, migrations.AlterIndexTogether, ) + FIELD_LEVEL_OPERATIONS = ( + migrations.AddField, + migrations.AlterField, + ) # If it's a model level operation, let it through if there's # nothing that looks like a reference to us in 'other'. if isinstance(operation, MODEL_LEVEL_OPERATIONS): if not other.references_model(operation.name, app_label): return True + # If it's field level, only let it through things that don't reference + # the field (which includes not referencing the model) + if isinstance(operation, FIELD_LEVEL_OPERATIONS): + if not other.references_field(operation.model_name, operation.name, app_label): + return True return False diff --git a/tests/migrations/test_operations.py b/tests/migrations/test_operations.py index 9121755000d..fb386050303 100644 --- a/tests/migrations/test_operations.py +++ b/tests/migrations/test_operations.py @@ -100,6 +100,30 @@ class OperationTests(MigrationTestBase): operation.database_backwards("test_dlmo", editor, new_state, project_state) self.assertTableExists("test_dlmo_pony") + def test_rename_model(self): + """ + Tests the RenameModel operation. + """ + project_state = self.set_up_test_model("test_rnmo") + # Test the state alteration + operation = migrations.RenameModel("Pony", "Horse") + new_state = project_state.clone() + operation.state_forwards("test_rnmo", new_state) + self.assertNotIn(("test_rnmo", "pony"), new_state.models) + self.assertIn(("test_rnmo", "horse"), new_state.models) + # Test the database alteration + self.assertTableExists("test_rnmo_pony") + self.assertTableNotExists("test_rnmo_horse") + with connection.schema_editor() as editor: + operation.database_forwards("test_rnmo", editor, project_state, new_state) + self.assertTableNotExists("test_rnmo_pony") + self.assertTableExists("test_rnmo_horse") + # And test reversal + with connection.schema_editor() as editor: + operation.database_backwards("test_rnmo", editor, new_state, project_state) + self.assertTableExists("test_dlmo_pony") + self.assertTableNotExists("test_rnmo_horse") + def test_add_field(self): """ Tests the AddField operation. diff --git a/tests/migrations/test_optimizer.py b/tests/migrations/test_optimizer.py index 7a481c905c8..8ba42db4672 100644 --- a/tests/migrations/test_optimizer.py +++ b/tests/migrations/test_optimizer.py @@ -1,4 +1,5 @@ # encoding: utf8 +import operator from django.test import TestCase from django.db.migrations.optimizer import MigrationOptimizer from django.db import migrations @@ -64,7 +65,7 @@ class OptimizerTests(TestCase): self.assertOptimizesTo( [migrations.DeleteModel("Foo")], [migrations.DeleteModel("Foo")], - exact=1, + exact = 1, ) def test_create_delete_model(self): @@ -79,6 +80,34 @@ class OptimizerTests(TestCase): [], ) + def test_create_rename_model(self): + """ + CreateModel should absorb RenameModels. + """ + self.assertOptimizesTo( + [ + migrations.CreateModel("Foo", [("name", models.CharField(max_length=255))]), + migrations.RenameModel("Foo", "Bar"), + ], + [ + migrations.CreateModel("Bar", [("name", models.CharField(max_length=255))]), + ], + ) + + def test_rename_model_self(self): + """ + RenameModels should absorb themselves. + """ + self.assertOptimizesTo( + [ + migrations.RenameModel("Foo", "Baa"), + migrations.RenameModel("Baa", "Bar"), + ], + [ + migrations.RenameModel("Foo", "Bar"), + ], + ) + def test_create_alter_delete_model(self): """ CreateModel, AlterModelTable, AlterUniqueTogether, and DeleteModel should collapse into nothing. @@ -154,3 +183,167 @@ class OptimizerTests(TestCase): migrations.DeleteModel("Foo"), ], ) + + def test_create_model_add_field(self): + """ + AddField should optimize into CreateModel. + """ + self.assertOptimizesTo( + [ + migrations.CreateModel("Foo", [("name", models.CharField(max_length=255))]), + migrations.AddField("Foo", "age", models.IntegerField()), + ], + [ + migrations.CreateModel("Foo", [ + ("name", models.CharField(max_length=255)), + ("age", models.IntegerField()), + ]), + ], + ) + + def test_create_model_alter_field(self): + """ + AlterField should optimize into CreateModel. + """ + self.assertOptimizesTo( + [ + migrations.CreateModel("Foo", [("name", models.CharField(max_length=255))]), + migrations.AlterField("Foo", "name", models.IntegerField()), + ], + [ + migrations.CreateModel("Foo", [ + ("name", models.IntegerField()), + ]), + ], + ) + + def test_create_model_rename_field(self): + """ + RenameField should optimize into CreateModel. + """ + self.assertOptimizesTo( + [ + migrations.CreateModel("Foo", [("name", models.CharField(max_length=255))]), + migrations.RenameField("Foo", "name", "title"), + ], + [ + migrations.CreateModel("Foo", [ + ("title", models.CharField(max_length=255)), + ]), + ], + ) + + def test_add_field_rename_field(self): + """ + RenameField should optimize into AddField + """ + self.assertOptimizesTo( + [ + migrations.AddField("Foo", "name", models.CharField(max_length=255)), + migrations.RenameField("Foo", "name", "title"), + ], + [ + migrations.AddField("Foo", "title", models.CharField(max_length=255)), + ], + ) + + def test_alter_field_rename_field(self): + """ + RenameField should optimize to the other side of AlterField, + and into itself. + """ + self.assertOptimizesTo( + [ + migrations.AlterField("Foo", "name", models.CharField(max_length=255)), + migrations.RenameField("Foo", "name", "title"), + migrations.RenameField("Foo", "title", "nom"), + ], + [ + migrations.RenameField("Foo", "name", "nom"), + migrations.AlterField("Foo", "nom", models.CharField(max_length=255)), + ], + ) + + def test_create_model_remove_field(self): + """ + RemoveField should optimize into CreateModel. + """ + self.assertOptimizesTo( + [ + migrations.CreateModel("Foo", [ + ("name", models.CharField(max_length=255)), + ("age", models.IntegerField()), + ]), + migrations.RemoveField("Foo", "age"), + ], + [ + migrations.CreateModel("Foo", [ + ("name", models.CharField(max_length=255)), + ]), + ], + ) + + def test_add_field_alter_field(self): + """ + AlterField should optimize into AddField. + """ + self.assertOptimizesTo( + [ + migrations.AddField("Foo", "age", models.IntegerField()), + migrations.AlterField("Foo", "age", models.FloatField(default=2.4)), + ], + [ + migrations.AddField("Foo", "age", models.FloatField(default=2.4)), + ], + ) + + def test_add_field_delete_field(self): + """ + RemoveField should cancel AddField + """ + self.assertOptimizesTo( + [ + migrations.AddField("Foo", "age", models.IntegerField()), + migrations.RemoveField("Foo", "age"), + ], + [], + ) + + def test_alter_field_delete_field(self): + """ + RemoveField should absorb AlterField + """ + self.assertOptimizesTo( + [ + migrations.AlterField("Foo", "age", models.IntegerField()), + migrations.RemoveField("Foo", "age"), + ], + [ + migrations.RemoveField("Foo", "age"), + ], + ) + + def test_optimize_through_fields(self): + """ + Checks that field-level through checking is working. + This should manage to collapse model Foo to nonexistence, + and model Bar to a single IntegerField called "width". + """ + self.assertOptimizesTo( + [ + migrations.CreateModel("Foo", [("name", models.CharField(max_length=255))]), + migrations.CreateModel("Bar", [("size", models.IntegerField())]), + migrations.AddField("Foo", "age", models.IntegerField()), + migrations.AddField("Bar", "width", models.IntegerField()), + migrations.AlterField("Foo", "age", models.IntegerField()), + migrations.RenameField("Bar", "size", "dimensions"), + migrations.RemoveField("Foo", "age"), + migrations.RenameModel("Foo", "Phou"), + migrations.RemoveField("Bar", "dimensions"), + migrations.RenameModel("Phou", "Fou"), + migrations.DeleteModel("Fou"), + ], + [ + migrations.CreateModel("Bar", [("width", models.IntegerField())]), + ], + )