From 729e0b086daac92bb9c6eb16f01cd2e13d258fdd Mon Sep 17 00:00:00 2001 From: Simon Charette Date: Sat, 9 Jan 2016 03:12:46 -0500 Subject: [PATCH] Fixed #24109 -- Allowed RunSQL and RunPython operations to be elided. Thanks to Markus Holtermann and Tim Graham for their review. --- django/db/migrations/operations/base.py | 7 +++++++ django/db/migrations/operations/fields.py | 12 ++++++++++-- django/db/migrations/operations/models.py | 12 ++++++++++-- django/db/migrations/operations/special.py | 6 ++++-- docs/ref/migration-operations.txt | 18 ++++++++++++++++-- docs/releases/1.10.txt | 5 +++++ docs/spelling_wordlist | 1 + tests/migrations/test_operations.py | 8 ++++++++ tests/migrations/test_optimizer.py | 20 ++++++++++++++++++++ 9 files changed, 81 insertions(+), 8 deletions(-) diff --git a/django/db/migrations/operations/base.py b/django/db/migrations/operations/base.py index b8e4e3d445..95829f716a 100644 --- a/django/db/migrations/operations/base.py +++ b/django/db/migrations/operations/base.py @@ -30,6 +30,9 @@ class Operation(object): # DDL transaction support (i.e., does it have no DDL, like RunPython) atomic = False + # Should this operation be considered safe to elide and optimize across? + elidable = False + serialization_expand_args = [] def __new__(cls, *args, **kwargs): @@ -117,6 +120,10 @@ class Operation(object): replaced with or a boolean that indicates whether or not the specified operation can be optimized across. """ + if self.elidable: + return [operation] + elif operation.elidable: + return [self] return False def __repr__(self): diff --git a/django/db/migrations/operations/fields.py b/django/db/migrations/operations/fields.py index 87d56e4747..4701ad1433 100644 --- a/django/db/migrations/operations/fields.py +++ b/django/db/migrations/operations/fields.py @@ -27,7 +27,10 @@ class FieldOperation(Operation): return self.is_same_model_operation(operation) and self.name_lower == operation.name_lower def reduce(self, operation, in_between, app_label=None): - return not operation.references_field(self.model_name, self.name, app_label) + return ( + super(FieldOperation, self).reduce(operation, in_between, app_label=app_label) or + not operation.references_field(self.model_name, self.name, app_label) + ) class AddField(FieldOperation): @@ -333,4 +336,9 @@ class RenameField(FieldOperation): operation.new_name, ), ] - return not operation.references_field(self.model_name, self.new_name, app_label) + # Skip `FieldOperation.reduce` as we want to run `references_field` + # against self.new_name. + return ( + super(FieldOperation, self).reduce(operation, in_between, app_label=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 ac1e80e81b..31c90f9f87 100644 --- a/django/db/migrations/operations/models.py +++ b/django/db/migrations/operations/models.py @@ -21,7 +21,10 @@ class ModelOperation(Operation): return self.name.lower() def reduce(self, operation, in_between, app_label=None): - return not operation.references_model(self.name, app_label) + return ( + super(ModelOperation, self).reduce(operation, in_between, app_label=app_label) or + not operation.references_model(self.name, app_label) + ) class CreateModel(ModelOperation): @@ -365,7 +368,12 @@ class RenameModel(ModelOperation): operation.new_name, ), ] - return not operation.references_model(self.new_name, app_label) + # Skip `ModelOperation.reduce` as we want to run `references_model` + # against self.new_name. + return ( + super(ModelOperation, self).reduce(operation, in_between, app_label=app_label) or + not operation.references_model(self.new_name, app_label) + ) class AlterModelTable(ModelOperation): diff --git a/django/db/migrations/operations/special.py b/django/db/migrations/operations/special.py index 95da11f5d2..a7da3cc81c 100644 --- a/django/db/migrations/operations/special.py +++ b/django/db/migrations/operations/special.py @@ -71,11 +71,12 @@ class RunSQL(Operation): """ noop = '' - def __init__(self, sql, reverse_sql=None, state_operations=None, hints=None): + def __init__(self, sql, reverse_sql=None, state_operations=None, hints=None, elidable=False): self.sql = sql self.reverse_sql = reverse_sql self.state_operations = state_operations or [] self.hints = hints or {} + self.elidable = elidable def deconstruct(self): kwargs = { @@ -138,7 +139,7 @@ class RunPython(Operation): reduces_to_sql = False - def __init__(self, code, reverse_code=None, atomic=True, hints=None): + def __init__(self, code, reverse_code=None, atomic=True, hints=None, elidable=False): self.atomic = atomic # Forwards code if not callable(code): @@ -152,6 +153,7 @@ class RunPython(Operation): raise ValueError("RunPython must be supplied with callable arguments") self.reverse_code = reverse_code self.hints = hints or {} + self.elidable = elidable def deconstruct(self): kwargs = { diff --git a/docs/ref/migration-operations.txt b/docs/ref/migration-operations.txt index 5b8a6c14a5..6255176f80 100644 --- a/docs/ref/migration-operations.txt +++ b/docs/ref/migration-operations.txt @@ -196,7 +196,7 @@ Special Operations RunSQL ------ -.. class:: RunSQL(sql, reverse_sql=None, state_operations=None, hints=None) +.. class:: RunSQL(sql, reverse_sql=None, state_operations=None, hints=None, elidable=False) Allows running of arbitrary SQL on the database - useful for more advanced features of database backends that Django doesn't support directly, like @@ -249,6 +249,9 @@ The optional ``hints`` argument will be passed as ``**hints`` to the routing decisions. See :ref:`topics-db-multi-db-hints` for more details on database hints. +The optional ``elidable`` argument determines whether or not the operation will +be removed (elided) when :ref:`squashing migrations `. + .. attribute:: RunSQL.noop Pass the ``RunSQL.noop`` attribute to ``sql`` or ``reverse_sql`` when you @@ -257,10 +260,14 @@ database hints. .. _sqlparse: https://pypi.python.org/pypi/sqlparse +.. versionadded:: 1.10 + + The ``elidable`` argument was added. + RunPython --------- -.. class:: RunPython(code, reverse_code=None, atomic=True, hints=None) +.. class:: RunPython(code, reverse_code=None, atomic=True, hints=None, elidable=False) Runs custom Python code in a historical context. ``code`` (and ``reverse_code`` if supplied) should be callable objects that accept two arguments; the first is @@ -278,6 +285,9 @@ The optional ``hints`` argument will be passed as ``**hints`` to the routing decision. See :ref:`topics-db-multi-db-hints` for more details on database hints. +The optional ``elidable`` argument determines whether or not the operation will +be removed (elided) when :ref:`squashing migrations `. + You are advised to write the code as a separate function above the ``Migration`` class in the migration file, and just pass it to ``RunPython``. Here's an example of using ``RunPython`` to create some initial objects on a ``Country`` @@ -366,6 +376,10 @@ attribute. you want the operation not to do anything in the given direction. This is especially useful in making the operation reversible. +.. versionadded:: 1.10 + + The ``elidable`` argument was added. + SeparateDatabaseAndState ------------------------ diff --git a/docs/releases/1.10.txt b/docs/releases/1.10.txt index 30e0660a51..bab0f900bf 100644 --- a/docs/releases/1.10.txt +++ b/docs/releases/1.10.txt @@ -239,6 +239,11 @@ Migrations * Added support for serialization of ``enum.Enum`` objects. +* Added the ``elidable`` argument to the + :class:`~django.db.migrations.operations.RunSQL` and + :class:`~django.db.migrations.operations.RunPython` operations to allow them + to be removed when squashing migrations. + Models ~~~~~~ diff --git a/docs/spelling_wordlist b/docs/spelling_wordlist index 85b115d99b..f9995b5d64 100644 --- a/docs/spelling_wordlist +++ b/docs/spelling_wordlist @@ -230,6 +230,7 @@ dumpdata Dunck dwithin editability +elidable encodings Endian endswith diff --git a/tests/migrations/test_operations.py b/tests/migrations/test_operations.py index de32353aa4..22d2df2754 100644 --- a/tests/migrations/test_operations.py +++ b/tests/migrations/test_operations.py @@ -1532,6 +1532,10 @@ class OperationTests(OperationTestBase): self.assertEqual(definition[0], "RunSQL") self.assertEqual(definition[1], []) self.assertEqual(sorted(definition[2]), ["reverse_sql", "sql", "state_operations"]) + # And elidable reduction + self.assertIs(False, operation.reduce(operation, [])) + elidable_operation = migrations.RunSQL('SELECT 1 FROM void;', elidable=True) + self.assertEqual(elidable_operation.reduce(operation, []), [operation]) def test_run_sql_params(self): """ @@ -1705,6 +1709,10 @@ class OperationTests(OperationTestBase): operation.database_forwards("test_runpython", editor, project_state, new_state) self.assertEqual(project_state.apps.get_model("test_runpython", "Pony").objects.count(), 6) self.assertEqual(project_state.apps.get_model("test_runpython", "ShetlandPony").objects.count(), 2) + # And elidable reduction + self.assertIs(False, operation.reduce(operation, [])) + elidable_operation = migrations.RunPython(inner_method, elidable=True) + self.assertEqual(elidable_operation.reduce(operation, []), [operation]) def test_run_python_atomic(self): """ diff --git a/tests/migrations/test_optimizer.py b/tests/migrations/test_optimizer.py index 07341a0c57..3a7acc2c89 100644 --- a/tests/migrations/test_optimizer.py +++ b/tests/migrations/test_optimizer.py @@ -1,6 +1,7 @@ # -*- coding: utf-8 -*- from django.db import migrations, models +from django.db.migrations import operations from django.db.migrations.optimizer import MigrationOptimizer from django.test import SimpleTestCase @@ -631,3 +632,22 @@ class OptimizerTests(SimpleTestCase): migrations.CreateModel("Bar", [("width", models.IntegerField())]), ], ) + + def test_optimize_elidable_operation(self): + elidable_operation = operations.base.Operation() + elidable_operation.elidable = True + self.assertOptimizesTo( + [ + elidable_operation, + migrations.CreateModel("Foo", [("name", models.CharField(max_length=255))]), + elidable_operation, + migrations.CreateModel("Bar", [("size", models.IntegerField())]), + elidable_operation, + migrations.RenameModel("Foo", "Phou"), + migrations.DeleteModel("Bar"), + elidable_operation, + ], + [ + migrations.CreateModel("Phou", [("name", models.CharField(max_length=255))]), + ], + )