Repository: charettes/django-syzygy (language: python)

Issue:
Implement makemigrations stage splitting.
`makemigrations` should split an automatically created migration containing both pre-deploy and post-deploy stage changes into two migrations if possible.


Discussion / hints:

Fix patch:
diff --git a/syzygy/autodetector.py b/syzygy/autodetector.py
new file mode 100644
index 0000000..8250ed3
--- /dev/null
+++ b/syzygy/autodetector.py
@@ -0,0 +1,106 @@
+import itertools
+
+import django
+from django.db.migrations import operations
+from django.db.migrations.autodetector import (
+    MigrationAutodetector as _MigrationAutodetector,
+)
+from django.db.migrations.operations.base import Operation
+
+
+class Stage(Operation):
+    """
+    Fake operation that servers as a placeholder to break operations into
+    multiple migrations.
+    """
+
+    def __repr__(self):
+        return f"<Stage {id(self)}>"
+
+
+class MigrationAutodetector(_MigrationAutodetector):
+    """
+    Migration auto-detector that splits migrations containing sequence of
+    operations incompatible with staged deployments.
+
+    It works by inserting fake `Stage` operations into a fake __stage__
+    application since `_build_migration_list` will only split operations of a
+    single application into multiple migrations if it has external
+    dependencies.
+
+    By creating a chain of external application dependencies between operations::
+
+        app.FirstOperation -> __stage__.Stage -> app.SecondOperation
+
+    The auto-detector will generate a sequence of migrations of the form::
+
+        app.Migration1(operations=[FirstOperation])
+        __stage__.Migration1(operations=[Stage])
+        app.Migration2(operations=[FirstOperation])
+
+    And automatically remove the __stage__ migrations since it's a not
+    an existing application.
+    """
+
+    STAGE_SPLIT = "__stage__"
+
+    def _generate_removed_field(self, app_label, model_name, field_name):
+        if django.VERSION >= (3, 1):
+            removed_field = self.from_state.models[app_label, model_name].fields[
+                field_name
+            ]
+        else:
+            for fname, field in self.from_state.models[app_label, model_name].fields:
+                if fname == field_name:
+                    removed_field = field
+                    break
+        if removed_field.null:
+            super()._generate_removed_field(app_label, model_name, field_name)
+            return
+        # Ensure removed fields are NULL'able before removal to allow INSERT
+        # during the deployment stage.
+        nullable_field = removed_field.clone()
+        nullable_field.null = True
+        self.add_operation(
+            app_label,
+            operations.AlterField(
+                model_name=model_name, name=field_name, field=nullable_field
+            ),
+        )
+        stage = Stage()
+        self.add_operation(
+            self.STAGE_SPLIT,
+            stage,
+            dependencies=[
+                (app_label, self.STAGE_SPLIT, self.generated_operations[app_label][-1])
+            ],
+        )
+        self.add_operation(
+            app_label,
+            operations.RemoveField(model_name=model_name, name=field_name),
+            dependencies=[
+                (app_label, model_name, field_name, "order_wrt_unset"),
+                (app_label, model_name, field_name, "foo_together_change"),
+                (self.STAGE_SPLIT, stage),
+            ],
+        )
+
+    def check_dependency(self, operation, dependency):
+        # Stage dependency on a previous operation.
+        if dependency[1] == self.STAGE_SPLIT:
+            return dependency[2] is operation
+        # Dependency on a stage.
+        if dependency[0] == self.STAGE_SPLIT:
+            return dependency[1] is operation
+        return super().check_dependency(operation, dependency)
+
+    def _build_migration_list(self, *args, **kwargs):
+        super()._build_migration_list(*args, **kwargs)
+        # Remove all dangling references to stage migrations.
+        if self.migrations.pop(self.STAGE_SPLIT, None):
+            for migration in itertools.chain.from_iterable(self.migrations.values()):
+                migration.dependencies = [
+                    dependency
+                    for dependency in migration.dependencies
+                    if dependency[0] != self.STAGE_SPLIT
+                ]
diff --git a/syzygy/management/commands/makemigrations.py b/syzygy/management/commands/makemigrations.py
new file mode 100644
index 0000000..5d9df28
--- /dev/null
+++ b/syzygy/management/commands/makemigrations.py
@@ -0,0 +1,15 @@
+from django.core.management.commands import makemigrations  # type: ignore
+
+from syzygy.autodetector import MigrationAutodetector
+
+
+class Command(makemigrations.Command):
+    def handle(self, *args, **options):
+        # Monkey-patch makemigrations.MigrationAutodetector since the command
+        # doesn't allow it to be overridden in any other way.
+        MigrationAutodetector_ = makemigrations.MigrationAutodetector
+        makemigrations.MigrationAutodetector = MigrationAutodetector
+        try:
+            super().handle(*args, **options)
+        finally:
+            makemigrations.MigrationAutodetector = MigrationAutodetector_
diff --git a/tox.ini b/tox.ini
index f3280db..7422795 100644
--- a/tox.ini
+++ b/tox.ini
@@ -4,9 +4,9 @@ args_are_paths = false
 envlist =
     flake8,
     isort,
-    py36-{2.2,3.0,master},
-    py37-{2.2,3.0,master},
-    py38-{2.2,master},
+    py36-{2.2,3.0,3.1,master},
+    py37-{2.2,3.0,3.1,master},
+    py38-{2.2,3.0,3.1,master},
 
 [tox:travis]
 3.6 = py36


Test patch:
diff --git a/tests/models.py b/tests/models.py
new file mode 100644
index 0000000..7f89123
--- /dev/null
+++ b/tests/models.py
@@ -0,0 +1,5 @@
+from django.db import models
+
+
+class Foo(models.Model):
+    pass
diff --git a/tests/test_autodetector.py b/tests/test_autodetector.py
new file mode 100644
index 0000000..da6ac7d
--- /dev/null
+++ b/tests/test_autodetector.py
@@ -0,0 +1,36 @@
+from typing import List
+
+from django.db import models
+from django.db.migrations.state import ModelState, ProjectState
+from django.test import TestCase
+
+from syzygy.autodetector import MigrationAutodetector
+from syzygy.constants import Stage
+from syzygy.plan import get_migration_stage
+
+
+class AutodetectorTests(TestCase):
+    @staticmethod
+    def make_project_state(model_states: List[ModelState]) -> ProjectState:
+        project_state = ProjectState()
+        for model_state in model_states:
+            project_state.add_model(model_state.clone())
+        return project_state
+
+    def get_changes(
+        self, before_states: List[ModelState], after_states: List[ModelState]
+    ):
+        return MigrationAutodetector(
+            self.make_project_state(before_states),
+            self.make_project_state(after_states),
+        )._detect_changes()
+
+    def test_non_nullable_field_removal(self):
+        from_model = ModelState("tests", "Model", [("field", models.IntegerField())])
+        to_model = ModelState("tests", "Model", [])
+        changes = self.get_changes([from_model], [to_model])["tests"]
+        self.assertEqual(len(changes), 2)
+        self.assertEqual(get_migration_stage(changes[0]), Stage.PRE_DEPLOY)
+        self.assertEqual(changes[0].dependencies, [])
+        self.assertEqual(get_migration_stage(changes[1]), Stage.POST_DEPLOY)
+        self.assertEqual(changes[1].dependencies, [("tests", "auto_1")])
diff --git a/tests/test_commands.py b/tests/test_commands.py
index 8ced7bb..3b32b99 100644
--- a/tests/test_commands.py
+++ b/tests/test_commands.py
@@ -1,9 +1,10 @@
 from io import StringIO
 
+import django
 from django.core.management import CommandError, call_command
 from django.db import connection
 from django.db.migrations.recorder import MigrationRecorder
-from django.test import TransactionTestCase, override_settings
+from django.test import TestCase, TransactionTestCase, override_settings
 
 
 class MigrateTests(TransactionTestCase):
@@ -23,9 +24,11 @@ def assert_not_applied(self, name):
     @override_settings(MIGRATION_MODULES={"tests": "tests.test_migrations.functional"})
     def test_pre_deploy_forward(self):
         stdout = StringIO()
-        call_command("migrate", "tests", plan=True, no_color=True, pre_deploy=True, stdout=stdout)
-        self.assertIn('tests.0001_pre_deploy', stdout.getvalue())
-        self.assertNotIn('tests.0002_post_deploy', stdout.getvalue())
+        call_command(
+            "migrate", "tests", plan=True, no_color=True, pre_deploy=True, stdout=stdout
+        )
+        self.assertIn("tests.0001_pre_deploy", stdout.getvalue())
+        self.assertNotIn("tests.0002_post_deploy", stdout.getvalue())
         call_command("migrate", "tests", pre_deploy=True, verbosity=0)
         self.assertEqual(self.get_applied_migrations(), {"0001_pre_deploy"})
 
@@ -36,13 +39,42 @@ def test_pre_deploy_backward(self):
             self.get_applied_migrations(), {"0001_pre_deploy", "0002_post_deploy"}
         )
         stdout = StringIO()
-        call_command("migrate", "tests", "zero", plan=True, no_color=True, pre_deploy=True, stdout=stdout)
-        self.assertIn('tests.0002_post_deploy', stdout.getvalue())
-        self.assertNotIn('tests.0001_pre_deploy', stdout.getvalue())
+        call_command(
+            "migrate",
+            "tests",
+            "zero",
+            plan=True,
+            no_color=True,
+            pre_deploy=True,
+            stdout=stdout,
+        )
+        self.assertIn("tests.0002_post_deploy", stdout.getvalue())
+        self.assertNotIn("tests.0001_pre_deploy", stdout.getvalue())
         call_command("migrate", "tests", "zero", pre_deploy=True, verbosity=0)
         self.assertEqual(self.get_applied_migrations(), {"0001_pre_deploy"})
 
     @override_settings(MIGRATION_MODULES={"tests": "tests.test_migrations.ambiguous"})
     def test_ambiguous(self):
-        with self.assertRaisesMessage(CommandError, 'Cannot automatically determine stage of tests.0001_initial.'):
+        with self.assertRaisesMessage(
+            CommandError, "Cannot automatically determine stage of tests.0001_initial."
+        ):
             call_command("migrate", "tests", pre_deploy=True, verbosity=0)
+
+
+class MakeMigrationsTests(TestCase):
+    @override_settings(
+        MIGRATION_MODULES={"tests": "tests.test_migrations.null_field_removal"}
+    )
+    def test_null_field_removal(self):
+        stdout = StringIO()
+        call_command(
+            "makemigrations", "tests", no_color=True, dry_run=True, stdout=stdout
+        )
+        output = stdout.getvalue()
+        if django.VERSION >= (3, 2):
+            self.assertIn("null_field_removal/0002_alter_foo_bar.py", output)
+        else:
+            self.assertIn("null_field_removal/0002_auto", output)
+        self.assertIn("- Alter field bar on foo", output)
+        self.assertIn("null_field_removal/0003_remove_foo_bar.py", output)
+        self.assertIn("- Remove field bar from foo", output)
diff --git a/tests/test_migrations/null_field_removal/0001_initial.py b/tests/test_migrations/null_field_removal/0001_initial.py
new file mode 100644
index 0000000..5a91212
--- /dev/null
+++ b/tests/test_migrations/null_field_removal/0001_initial.py
@@ -0,0 +1,22 @@
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+    initial = True
+    operations = [
+        migrations.CreateModel(
+            "Foo",
+            fields=[
+                (
+                    "id",
+                    models.AutoField(
+                        auto_created=True,
+                        primary_key=True,
+                        serialize=False,
+                        verbose_name="ID",
+                    ),
+                ),
+                ("bar", models.IntegerField()),
+            ],
+        ),
+    ]
diff --git a/tests/test_migrations/null_field_removal/__init__.py b/tests/test_migrations/null_field_removal/__init__.py
new file mode 100644
index 0000000..e69de29