Fixed #24529 -- Allowed double squashing of migrations.

Co-authored-by: Raphael Gaschignard <raphael@rtpg.co>

django/core/management/commands/squashmigrations.py

@@ -5,12 +5,11 @@ from django.apps import apps
 from django.conf import settings
 from django.core.management.base import BaseCommand, CommandError
 from django.core.management.utils import run_formatters
-from django.db import DEFAULT_DB_ALIAS, connections, migrations
+from django.db import migrations
 from django.db.migrations.loader import AmbiguityError, MigrationLoader
 from django.db.migrations.migration import SwappableTuple
 from django.db.migrations.optimizer import MigrationOptimizer
 from django.db.migrations.writer import MigrationWriter
-from django.utils.version import get_docs_version
 
 
 class Command(BaseCommand):
@@ -75,7 +74,7 @@ class Command(BaseCommand):
             raise CommandError(str(err))
         # Load the current graph state, check the app and migration they asked
         # for exists.
-        loader = MigrationLoader(connections[DEFAULT_DB_ALIAS])
+        loader = MigrationLoader(None)
         if app_label not in loader.migrated_apps:
             raise CommandError(
                 "App '%s' does not have migrations (so squashmigrations on "
@@ -141,12 +140,6 @@ class Command(BaseCommand):
         # as it may be 0002 depending on 0001
         first_migration = True
         for smigration in migrations_to_squash:
-            if smigration.replaces:
-                raise CommandError(
-                    "You cannot squash squashed migrations! Please transition it to a "
-                    "normal migration first: https://docs.djangoproject.com/en/%s/"
-                    "topics/migrations/#squashing-migrations" % get_docs_version()
-                )
             operations.extend(smigration.operations)
             for dependency in smigration.dependencies:
                 if isinstance(dependency, SwappableTuple):
@@ -180,14 +173,7 @@ class Command(BaseCommand):
                         % (len(operations), len(new_operations))
                     )
 
-        # Work out the value of replaces (any squashed ones we're re-squashing)
+        replaces = [(m.app_label, m.name) for m in migrations_to_squash]
-        # need to feed their replaces into ours
-        replaces = []
-        for migration in migrations_to_squash:
-            if migration.replaces:
-                replaces.extend(migration.replaces)
-            else:
-                replaces.append((migration.app_label, migration.name))
 
         # Make a new migration with those operations
         subclass = type(

django/db/migrations/loader.py

@@ -4,6 +4,7 @@ from importlib import import_module, reload
 
 from django.apps import apps
 from django.conf import settings
+from django.core.management import CommandError
 from django.db.migrations.graph import MigrationGraph
 from django.db.migrations.recorder import MigrationRecorder
 
@@ -219,11 +220,37 @@ class MigrationLoader:
             if child is not None:
                 self.graph.add_dependency(migration, child, key, skip_validation=True)
 
+    def _resolve_replaced_migration_keys(self, migration):
+        resolved_keys = set()
+        for migration_key in set(migration.replaces):
+            migration_entry = self.disk_migrations.get(migration_key)
+            if migration_entry and migration_entry.replaces:
+                replace_keys = self._resolve_replaced_migration_keys(migration_entry)
+                resolved_keys.update(replace_keys)
+            else:
+                resolved_keys.add(migration_key)
+        return resolved_keys
+
     def replace_migration(self, migration_key):
+        if completed_replacement := self.replacements_progress.get(migration_key, None):
+            return
+        elif completed_replacement is False:
+            # Called before but not finished the replacement, this means there
+            # is a circular dependency.
+            raise CommandError(
+                f"Cyclical squash replacement found, starting at {migration_key}"
+            )
+        self.replacements_progress[migration_key] = False
         migration = self.replacements[migration_key]
+        # Process potential squashed migrations that the migration replaces.
+        for replace_migration_key in migration.replaces:
+            if replace_migration_key in self.replacements:
+                self.replace_migration(replace_migration_key)
+
+        replaced_keys = self._resolve_replaced_migration_keys(migration)
         # Get applied status of each found replacement target.
         applied_statuses = [
-            (target in self.applied_migrations) for target in migration.replaces
+            (target in self.applied_migrations) for target in replaced_keys
         ]
         # The replacing migration is only marked as applied if all of its
         # replacement targets are applied.
@@ -241,6 +268,8 @@ class MigrationLoader:
             # dependencies to it (#25945).
             self.graph.remove_replacement_node(migration_key, migration.replaces)
 
+        self.replacements_progress[migration_key] = True
+
     def build_graph(self):
         """
         Build a migration dependency graph using both the disk and database.
@@ -272,6 +301,7 @@ class MigrationLoader:
             self.add_external_dependencies(key, migration)
         # Carry out replacements where possible and if enabled.
         if self.replace_migrations:
+            self.replacements_progress = {}
             for migration_key in self.replacements.keys():
                 self.replace_migration(migration_key)
         # Ensure the graph is consistent.

docs/releases/6.0.txt

@@ -169,7 +169,8 @@ Management Commands
 Migrations
 ~~~~~~~~~~
 
-* ...
+* Squashed migrations can now themselves be squashed before being transitioned
+  to normal migrations.
 
 Models
 ~~~~~~

docs/topics/migrations.txt

@@ -733,7 +733,7 @@ migrations it replaces and distribute this change to all running instances
 of your application, making sure that they run ``migrate`` to store the change
 in their database.
 
-You must then transition the squashed migration to a normal migration by:
+You can then transition the squashed migration to a normal migration by:
 
 - Deleting all the migration files it replaces.
 - Updating all migrations that depend on the deleted migrations to depend on
@@ -742,8 +742,11 @@ You must then transition the squashed migration to a normal migration by:
   squashed migration (this is how Django tells that it is a squashed migration).
 
 .. note::
-    Once you've squashed a migration, you should not then re-squash that squashed
+    You can squash squashed migrations themselves without transitioning to
-    migration until you have fully transitioned it to a normal migration.
+    normal migrations, which might be useful for situations where every
+    environment has not yet run the original squashed migration set. But in
+    general it is better to transition squashed migrations to normal migrations
+    to be able to clean up older migration files.
 
 .. admonition:: Pruning references to deleted migrations
 
@@ -751,6 +754,10 @@ You must then transition the squashed migration to a normal migration by:
     future, you should remove references to it from Django’s migrations table
     with the :option:`migrate --prune` option.
 
+.. versionchanged:: 6.0
+
+    Support for squashing squashed migrations was added.
+
 .. _migration-serializing:
 
 Serializing values

tests/migrations/test_commands.py

@@ -2,6 +2,7 @@ import datetime
 import importlib
 import io
 import os
+import re
 import shutil
 import sys
 from pathlib import Path
@@ -28,7 +29,9 @@ from django.db.backends.base.schema import BaseDatabaseSchemaEditor
 from django.db.backends.utils import truncate_name
 from django.db.migrations.autodetector import MigrationAutodetector
 from django.db.migrations.exceptions import InconsistentMigrationHistory
+from django.db.migrations.loader import MigrationLoader
 from django.db.migrations.recorder import MigrationRecorder
+from django.db.migrations.writer import MigrationWriter
 from django.test import TestCase, override_settings, skipUnlessDBFeature
 from django.test.utils import captured_stdout, extend_sys_path, isolate_apps
 from django.utils import timezone
@@ -2939,6 +2942,137 @@ class SquashMigrationsTests(MigrationTestBase):
             "  you can delete them.\n" % squashed_migration_file,
         )
 
+    def test_squashmigrations_replacement_cycle(self):
+        out = io.StringIO()
+        with self.temporary_migration_module(
+            module="migrations.test_migrations_squashed_loop"
+        ):
+            # Hits a squash replacement cycle check error, but the actual failure is
+            # dependent on the order in which the files are read on disk.
+            with self.assertRaisesRegex(
+                CommandError,
+                r"Cyclical squash replacement found, starting at"
+                r" \('migrations', '2_(squashed|auto)'\)",
+            ):
+                call_command(
+                    "migrate", "migrations", "--plan", interactive=False, stdout=out
+                )
+
+    def test_squashmigrations_squashes_already_squashed(self):
+        out = io.StringIO()
+
+        with self.temporary_migration_module(
+            module="migrations.test_migrations_squashed_complex"
+        ):
+            call_command(
+                "squashmigrations",
+                "migrations",
+                "3_squashed_5",
+                "--squashed-name",
+                "double_squash",
+                stdout=out,
+                interactive=False,
+            )
+
+            loader = MigrationLoader(connection)
+            migration = loader.disk_migrations[("migrations", "0001_double_squash")]
+            # Confirm the replaces mechanism holds the squashed migration
+            # (and not what it squashes, as the squash operations are what
+            # end up being used).
+            self.assertEqual(
+                migration.replaces,
+                [
+                    ("migrations", "1_auto"),
+                    ("migrations", "2_auto"),
+                    ("migrations", "3_squashed_5"),
+                ],
+            )
+
+            out = io.StringIO()
+            call_command(
+                "migrate", "migrations", "--plan", interactive=False, stdout=out
+            )
+
+            migration_plan = re.findall("migrations.(.+)\n", out.getvalue())
+            self.assertEqual(migration_plan, ["0001_double_squash", "6_auto", "7_auto"])
+
+    def test_squash_partially_applied(self):
+        """
+        Replacement migrations are partially applied. Then we squash again and
+        verify that only unapplied migrations will be applied by "migrate".
+        """
+        out = io.StringIO()
+
+        with self.temporary_migration_module(
+            module="migrations.test_migrations_squashed_partially_applied"
+        ):
+            # Apply first 2 migrations.
+            call_command("migrate", "migrations", "0002", interactive=False, stdout=out)
+
+            # Squash the 2 migrations, that we just applied + 1 more.
+            call_command(
+                "squashmigrations",
+                "migrations",
+                "0001",
+                "0003",
+                "--squashed-name",
+                "squashed_0001_0003",
+                stdout=out,
+                interactive=False,
+            )
+
+            # Update the 4th migration to depend on the squash(replacement) migration.
+            loader = MigrationLoader(connection)
+            migration = loader.disk_migrations[
+                ("migrations", "0004_remove_mymodel1_field_1_mymodel1_field_3_and_more")
+            ]
+            migration.dependencies = [("migrations", "0001_squashed_0001_0003")]
+            writer = MigrationWriter(migration)
+            with open(writer.path, "w", encoding="utf-8") as fh:
+                fh.write(writer.as_string())
+
+            # Squash the squash(replacement) migration with the 4th migration.
+            call_command(
+                "squashmigrations",
+                "migrations",
+                "0001_squashed_0001_0003",
+                "0004",
+                "--squashed-name",
+                "squashed_0001_0004",
+                stdout=out,
+                interactive=False,
+            )
+
+            loader = MigrationLoader(connection)
+            migration = loader.disk_migrations[
+                ("migrations", "0001_squashed_0001_0004")
+            ]
+            self.assertEqual(
+                migration.replaces,
+                [
+                    ("migrations", "0001_squashed_0001_0003"),
+                    (
+                        "migrations",
+                        "0004_remove_mymodel1_field_1_mymodel1_field_3_and_more",
+                    ),
+                ],
+            )
+
+            # Verify that only unapplied migrations will be applied.
+            out = io.StringIO()
+            call_command(
+                "migrate", "migrations", "--plan", interactive=False, stdout=out
+            )
+
+            migration_plan = re.findall("migrations.(.+)\n", out.getvalue())
+            self.assertEqual(
+                migration_plan,
+                [
+                    "0003_alter_mymodel2_unique_together",
+                    "0004_remove_mymodel1_field_1_mymodel1_field_3_and_more",
+                ],
+            )
+
     def test_squashmigrations_initial_attribute(self):
         with self.temporary_migration_module(
             module="migrations.test_migrations"

tests/migrations/test_migrations_squashed_loop/1_auto.py

@@ -0,0 +1,5 @@
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+    operations = [migrations.RunPython(migrations.RunPython.noop)]

tests/migrations/test_migrations_squashed_loop/2_auto.py

@@ -0,0 +1,7 @@
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+    replaces = [("migrations", "2_squashed")]
+    dependencies = [("migrations", "1_auto")]
+    operations = [migrations.RunPython(migrations.RunPython.noop)]

tests/migrations/test_migrations_squashed_loop/2_squashed.py

@@ -0,0 +1,7 @@
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+    replaces = [("migrations", "2_auto")]
+    dependencies = [("migrations", "1_auto")]
+    operations = [migrations.RunPython(migrations.RunPython.noop)]

tests/migrations/test_migrations_squashed_partially_applied/0001_initial.py

@@ -0,0 +1,35 @@
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+    operations = [
+        migrations.CreateModel(
+            name="MyModel1",
+            fields=[
+                (
+                    "id",
+                    models.BigAutoField(
+                        auto_created=True,
+                        primary_key=True,
+                        serialize=False,
+                        verbose_name="ID",
+                    ),
+                ),
+            ],
+        ),
+        migrations.CreateModel(
+            name="MyModel2",
+            fields=[
+                (
+                    "id",
+                    models.BigAutoField(
+                        auto_created=True,
+                        primary_key=True,
+                        serialize=False,
+                        verbose_name="ID",
+                    ),
+                ),
+                ("field_1", models.IntegerField()),
+            ],
+        ),
+    ]

tests/migrations/test_migrations_squashed_partially_applied/0002_mymodel1_field_1_mymodel2_field_2_and_more.py

@@ -0,0 +1,23 @@
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+    dependencies = [("migrations", "0001_initial")]
+
+    operations = [
+        migrations.AddField(
+            model_name="mymodel1",
+            name="field_1",
+            field=models.IntegerField(null=True),
+        ),
+        migrations.AddField(
+            model_name="mymodel2",
+            name="field_2",
+            field=models.IntegerField(null=True),
+        ),
+        migrations.AlterField(
+            model_name="mymodel2",
+            name="field_1",
+            field=models.IntegerField(null=True),
+        ),
+    ]

tests/migrations/test_migrations_squashed_partially_applied/0003_alter_mymodel2_unique_together.py

@@ -0,0 +1,12 @@
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+    dependencies = [("migrations", "0002_mymodel1_field_1_mymodel2_field_2_and_more")]
+
+    operations = [
+        migrations.AlterUniqueTogether(
+            name="mymodel2",
+            unique_together={("field_1", "field_2")},
+        ),
+    ]

tests/migrations/test_migrations_squashed_partially_applied/0004_remove_mymodel1_field_1_mymodel1_field_3_and_more.py

@@ -0,0 +1,22 @@
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+    dependencies = [("migrations", "0003_alter_mymodel2_unique_together")]
+
+    operations = [
+        migrations.RemoveField(
+            model_name="mymodel1",
+            name="field_1",
+        ),
+        migrations.AddField(
+            model_name="mymodel1",
+            name="field_3",
+            field=models.IntegerField(null=True),
+        ),
+        migrations.AddField(
+            model_name="mymodel1",
+            name="field_4",
+            field=models.IntegerField(null=True),
+        ),
+    ]