fix: invalidate empty-table proof after writes and table renames

CHANGELOG.md

@@ -2,7 +2,11 @@
 
 Bug:
 - Do not report a required column added to a table unconditionally created earlier in the same migration.
-  Raw SQL, data migrations, and required columns added to existing tables remain linted.
+  SQL inserts and data operations, including nested `SeparateDatabaseAndState` operations,
+  invalidate that proof. Raw SQL, data migrations, and required columns added to existing tables
+  remain linted; nested data operations receive the same checks as top-level operations. Custom
+  database operations and table renames invalidate the proof unless they are known declarative
+  schema operations.
 
 ## 6.0.0
 

DECISIONS.md

@@ -4,9 +4,12 @@
 
 **Decision.** The `NOT_NULL` SQL check accepts `ALTER TABLE ... ADD COLUMN ... NOT NULL`
 only when the same migration unconditionally created that exact table. `CREATE TABLE IF
-NOT EXISTS` does not prove an empty table. The migration remains
+NOT EXISTS`, SQL inserts, and data operations, including nested
-fully linted, so `RunSQL`, `RunPython`, and operations on existing tables retain their
+`SeparateDatabaseAndState` operations, do not prove an empty table. The migration remains fully
-checks.
+linted, so top-level and nested `RunSQL` and `RunPython`, and operations on existing tables retain
+their checks. Custom database operations are not trusted as declarative schema operations and
+therefore invalidate the proof. Table renames also invalidate it because a name can be reused by a
+populated table.
 
 **Rejected.** Ignoring all initial migrations or naming them as exemptions. Either option
 skips unsafe operations that the linter must inspect.

src/django_migration_linter/migration_linter.py

@@ -13,8 +13,8 @@ from typing import Callable, Dict, Iterable
 
 from django.conf import settings
 from django.core.management import call_command
-from django.db import DEFAULT_DB_ALIAS, ProgrammingError, connections
+from django.db import DEFAULT_DB_ALIAS, ProgrammingError, connections, migrations
-from django.db.migrations import Migration, RunPython, RunSQL
+from django.db.migrations import Migration, RunPython, RunSQL, SeparateDatabaseAndState
 from django.db.migrations.operations.base import Operation
 
 from .cache import Cache
@@ -31,6 +31,33 @@ from .utils import clean_bytes_to_str, get_migration_abspath, split_migration_pa
 logger = logging.getLogger("django_migration_linter")
 
 
+_DECLARATIVE_OPERATION_NAMES = (
+    "AddConstraint",
+    "AddField",
+    "AddIndex",
+    "AlterConstraint",
+    "AlterField",
+    "AlterIndexTogether",
+    "AlterModelManagers",
+    "AlterModelOptions",
+    "AlterModelTableComment",
+    "AlterOrderWithRespectTo",
+    "AlterUniqueTogether",
+    "CreateModel",
+    "DeleteModel",
+    "RemoveConstraint",
+    "RemoveField",
+    "RemoveIndex",
+    "RenameField",
+    "RenameIndex",
+)
+_DECLARATIVE_OPERATION_TYPES = tuple(
+    getattr(migrations, name)
+    for name in _DECLARATIVE_OPERATION_NAMES
+    if hasattr(migrations, name)
+)
+
+
 @unique
 class MessageType(Enum):
     OK = "ok"
@@ -182,6 +209,7 @@ class MigrationLinter:
             self.sql_analyser_class,
             sql_statements,
             self.exclude_migration_tests,
+            has_unknown_data_operations=self.has_unknown_data_operations(operations),
         )
 
         err, ignored_data, warnings_data = self.analyse_data_migration(migration)
@@ -509,6 +537,30 @@ class MigrationLinter:
             or (self.ignore_initial_migrations and is_initial)
         )
 
+    @staticmethod
+    def iter_database_operations(
+        operations: Iterable[Operation],
+    ) -> Iterable[Operation]:
+        """Yield operations that execute database changes, including wrapped ones."""
+        for operation in operations:
+            nested_operations = getattr(operation, "database_operations", None)
+            if type(operation) is SeparateDatabaseAndState:
+                yield from MigrationLinter.iter_database_operations(nested_operations)
+            else:
+                yield operation
+                if nested_operations is not None:
+                    yield from MigrationLinter.iter_database_operations(
+                        nested_operations
+                    )
+
+    @staticmethod
+    def has_unknown_data_operations(operations: Iterable[Operation]) -> bool:
+        """Return whether an operation can populate a newly created table."""
+        return any(
+            type(operation) not in _DECLARATIVE_OPERATION_TYPES
+            for operation in MigrationLinter.iter_database_operations(operations)
+        )
+
     def analyse_data_migration(
         self, migration: Migration
     ) -> tuple[list[Issue], list[Issue], list[Issue]]:
@@ -516,7 +568,7 @@ class MigrationLinter:
         ignored = []
         warnings = []
 
-        for operation in migration.operations:
+        for operation in self.iter_database_operations(migration.operations):
             if isinstance(operation, RunPython):
                 op_errors, op_ignored, op_warnings = self.lint_runpython(operation)
             elif isinstance(operation, RunSQL):
@@ -727,6 +779,7 @@ class MigrationLinter:
                 self.sql_analyser_class,
                 sql_statements,
                 self.exclude_migration_tests,
+                has_unknown_data_operations=True,
             )
             if sql_errors:
                 error += sql_errors
@@ -757,6 +810,7 @@ class MigrationLinter:
                 self.sql_analyser_class,
                 sql_statements,
                 self.exclude_migration_tests,
+                has_unknown_data_operations=True,
             )
             if sql_errors:
                 error += sql_errors

src/django_migration_linter/sql_analyser/analyser.py

@@ -64,7 +64,11 @@ def analyse_sql_statements(
     sql_analyser_class: Type[BaseAnalyser],
     sql_statements: list[str],
     exclude_migration_tests: Iterable[str] | None = None,
+    has_unknown_data_operations: bool = False,
 ) -> tuple[list[Issue], list[Issue], list[Issue]]:
-    sql_analyser = sql_analyser_class(exclude_migration_tests)
+    sql_analyser = sql_analyser_class(
+        exclude_migration_tests,
+        has_unknown_data_operations=has_unknown_data_operations,
+    )
     sql_analyser.analyse(sql_statements)
     return sql_analyser.errors, sql_analyser.ignored, sql_analyser.warnings

src/django_migration_linter/sql_analyser/base.py

@@ -39,9 +39,16 @@ _ADD_COLUMN = re.compile(
     rf"^ALTER TABLE (?:ONLY )?(?P<table>{_TABLE_NAME}) ADD (?:COLUMN )?",
     re.IGNORECASE,
 )
+_INSERT_INTO = re.compile(rf"^INSERT INTO (?P<table>{_TABLE_NAME})", re.IGNORECASE)
+_TABLE_RENAME = re.compile(
+    rf"^(?:ALTER TABLE (?:ONLY )?{_TABLE_NAME} RENAME TO|RENAME TABLE {_TABLE_NAME} TO)",
+    re.IGNORECASE,
+)
 
 
-def has_not_null_column(sql_statements: list[str], **kwargs) -> bool:
+def has_not_null_column(
+    sql_statements: list[str], has_unknown_data_operations: bool = False, **kwargs
+) -> bool:
     """Detect a required column added to a table that already exists."""
     not_null_column = False
     has_default_value = False
@@ -49,9 +56,15 @@ def has_not_null_column(sql_statements: list[str], **kwargs) -> bool:
 
     for sql in sql_statements:
         created_table = _CREATE_TABLE.match(sql)
-        if created_table:
+        if created_table and not has_unknown_data_operations:
             created_tables.add(created_table.group("table"))
 
+        inserted_table = _INSERT_INTO.match(sql)
+        if inserted_table:
+            created_tables.discard(inserted_table.group("table"))
+        if _TABLE_RENAME.match(sql):
+            created_tables.clear()
+
         adds_column = _ADD_COLUMN.match(sql)
         adds_required_column = re.search("(?<!DROP )(?<!IS )NOT NULL", sql)
         adds_to_new_table = (
@@ -190,8 +203,13 @@ class BaseAnalyser:
 
     migration_checks: list[Check] = []
 
-    def __init__(self, exclude_migration_tests: Iterable[str] | None):
+    def __init__(
+        self,
+        exclude_migration_tests: Iterable[str] | None,
+        has_unknown_data_operations: bool = False,
+    ):
         self.exclude_migration_tests: Iterable[str] = exclude_migration_tests or []
+        self.has_unknown_data_operations = has_unknown_data_operations
         self.errors: list[Issue] = []
         self.warnings: list[Issue] = []
         self.ignored: list[Issue] = []
@@ -216,7 +234,11 @@ class BaseAnalyser:
         return (c for c in self.migration_checks if c.mode == CheckMode.TRANSACTION)
 
     def _check_sql(self, check: Check, sql: list[str] | str) -> None:
-        if check.fn(sql, errors=self.errors):
+        if check.fn(
+            sql,
+            errors=self.errors,
+            has_unknown_data_operations=self.has_unknown_data_operations,
+        ):
             if check.code in self.exclude_migration_tests:
                 action = "IGNORED"
                 list_to_add = self.ignored

tests/unit/test_linter.py

@@ -4,12 +4,15 @@ import tempfile
 import unittest
 from unittest.mock import patch
 
-from django.db import ProgrammingError
+from django.db import ProgrammingError, migrations, models
-from django.db import migrations
 from django.db.migrations import Migration
+from django.db.migrations.operations.base import Operation
 
 from django_migration_linter import MigrationLinter
-from django_migration_linter.sql_analyser import get_sql_analyser_class
+from django_migration_linter.sql_analyser import (
+    analyse_sql_statements,
+    get_sql_analyser_class,
+)
 
 
 class LinterFunctionsTestCase(unittest.TestCase):
@@ -79,18 +82,7 @@ class LinterFunctionsTestCase(unittest.TestCase):
         self.assertFalse(linter.should_ignore_migration("app_correct", "0003_bar"))
 
     def test_gather_all_migrations(self):
-        linter = MigrationLinter.__new__(MigrationLinter)
+        linter = MigrationLinter()
-        linter.exclude_migration_tests = []
-        linter.sql_analyser_class = get_sql_analyser_class("sqlite")
-        linter.include_apps = None
-        linter.exclude_apps = None
-        linter.ignore_name_contains = None
-        linter.include_name_contains = None
-        linter.ignore_name = []
-        linter.include_name = []
-        linter.only_applied_migrations = False
-        linter.only_unapplied_migrations = False
-        linter.ignore_initial_migrations = False
         migrations = linter._gather_all_migrations()
         self.assertGreater(len(list(migrations)), 1)
 
@@ -197,9 +189,33 @@ class LinterFunctionsTestCase(unittest.TestCase):
         )
 
     def test_initial_migration_still_lints_runsql_and_runpython(self):
+        class CteInsertOperation(Operation):
+            reduces_to_sql = True
+            reversible = True
+
+            def state_forwards(self, app_label, state):
+                return None
+
+            def database_forwards(self, app_label, schema_editor, from_state, to_state):
+                schema_editor.execute(
+                    'WITH inserted AS (SELECT 1) INSERT INTO "archive_capture" ("id") SELECT * FROM inserted;'
+                )
+
+            def database_backwards(
+                self, app_label, schema_editor, from_state, to_state
+            ):
+                return None
+
+            def describe(self):
+                return "Insert an archive capture through a CTE"
+
         def forward(apps, schema_editor):
             return None
 
+        def unsafe_forward(apps, schema_editor):
+            Legacy = None
+            Legacy.objects.all()
+
         migration = Migration("0001_initial", "app_initial_operations")
         migration.initial = True
         migration.operations = [
@@ -208,7 +224,7 @@ class LinterFunctionsTestCase(unittest.TestCase):
         ]
         linter = MigrationLinter.__new__(MigrationLinter)
         linter.exclude_migration_tests = []
-        linter.sql_analyser_class = get_sql_analyser_class("sqlite")
+        linter.sql_analyser_class = get_sql_analyser_class("postgresql")
         linter.include_apps = None
         linter.exclude_apps = None
         linter.ignore_name_contains = None
@@ -226,8 +242,135 @@ class LinterFunctionsTestCase(unittest.TestCase):
                 migration.app_label, migration.name, is_initial=migration.initial
             )
         )
+        self.assertTrue(linter.has_unknown_data_operations(migration.operations))
+        declarative_migration = Migration("0001_initial", "app_declarative_schema")
+        declarative_migration.initial = True
+        declarative_migration.operations = [
+            migrations.CreateModel(name="ArchiveCapture", fields=[]),
+            migrations.AddField(
+                model_name="archivecapture",
+                name="source_id",
+                field=models.IntegerField(),
+            ),
+            migrations.AddConstraint(
+                model_name="archivecapture",
+                constraint=models.UniqueConstraint(
+                    fields=("source_id",), name="archive_source"
+                ),
+            ),
+        ]
+        self.assertFalse(
+            linter.has_unknown_data_operations(declarative_migration.operations)
+        )
+        renamed_migration = Migration("0001_initial", "app_renamed_schema")
+        renamed_migration.initial = True
+        renamed_migration.operations = [
+            migrations.CreateModel(name="ArchiveCapture", fields=[]),
+            migrations.RenameModel(
+                old_name="archivecapture", new_name="archivedcapture"
+            ),
+            migrations.AddField(
+                model_name="archivecapture",
+                name="source_id",
+                field=models.IntegerField(),
+            ),
+        ]
+        self.assertTrue(
+            linter.has_unknown_data_operations(renamed_migration.operations)
+        )
         self.assertEqual(["DROP_TABLE"], [issue.code for issue in errors])
         self.assertEqual(
             ["RUNSQL_REVERSIBLE", "RUNPYTHON_REVERSIBLE"],
             [issue.code for issue in warnings],
         )
+
+        nested_migration = Migration("0001_nested_initial", "app_nested_operations")
+        nested_migration.initial = True
+        nested_migration.operations = [
+            migrations.SeparateDatabaseAndState(
+                database_operations=[
+                    migrations.RunSQL("DROP TABLE legacy_data;"),
+                    migrations.RunPython(unsafe_forward),
+                ]
+            )
+        ]
+
+        errors, _, warnings = linter.analyse_data_migration(nested_migration)
+
+        self.assertEqual(
+            ["DROP_TABLE", "RUNPYTHON_MODEL_IMPORT"],
+            [issue.code for issue in errors],
+        )
+        self.assertEqual(
+            [
+                "RUNSQL_REVERSIBLE",
+                "RUNPYTHON_REVERSIBLE",
+                "RUNPYTHON_MODEL_VARIABLE_NAME",
+            ],
+            [issue.code for issue in warnings],
+        )
+
+        migration = Migration("0001_initial", "app_initial_schema_and_data")
+        migration.initial = True
+        migration.operations = [
+            migrations.CreateModel(name="ArchiveCapture", fields=[]),
+            migrations.SeparateDatabaseAndState(
+                database_operations=[migrations.RunPython(forward)]
+            ),
+            migrations.AddField(
+                model_name="archivecapture",
+                name="source_id",
+                field=models.IntegerField(),
+            ),
+        ]
+        self.assertTrue(linter.has_unknown_data_operations(migration.operations))
+        linter.django_path = None
+        linter.no_cache = True
+        linter.all_warnings_as_errors = False
+        linter.warnings_as_errors_tests = None
+        linter.no_output = True
+        linter.quiet = []
+        linter.reset_counters()
+        linter.get_migration_hash = lambda *args: "initial-schema-and-data"
+        linter.get_sql = lambda *args: [
+            'CREATE TABLE "archive_capture" ("id" uuid NOT NULL PRIMARY KEY);',
+            'ALTER TABLE "archive_capture" ADD COLUMN "source_id" uuid NOT NULL REFERENCES "archive_source" ("id");',
+        ]
+        linter.analyse_data_migration = lambda *args: ([], [], [])
+
+        linter.lint_migration(migration)
+
+        self.assertTrue(linter.has_errors)
+
+        migration.operations[1] = migrations.SeparateDatabaseAndState(
+            database_operations=[CteInsertOperation()]
+        )
+        linter.get_sql = lambda *args: [
+            'CREATE TABLE "archive_capture" ("id" uuid NOT NULL PRIMARY KEY);',
+            'WITH inserted AS (SELECT 1) INSERT INTO "archive_capture" ("id") SELECT * FROM inserted;',
+            'ALTER TABLE "archive_capture" ADD COLUMN "source_id" uuid NOT NULL REFERENCES "archive_source" ("id");',
+        ]
+        linter.reset_counters()
+
+        self.assertTrue(linter.has_unknown_data_operations(migration.operations))
+        errors, _, _ = analyse_sql_statements(
+            linter.sql_analyser_class,
+            linter.get_sql(),
+            has_unknown_data_operations=True,
+        )
+        self.assertEqual(["NOT_NULL"], [issue.code for issue in errors])
+
+        linter.lint_migration(migration)
+
+        self.assertTrue(linter.has_errors)
+
+        migration.operations[1] = migrations.SeparateDatabaseAndState(
+            database_operations=[
+                migrations.RunSQL("INSERT INTO archive_capture VALUES (1);")
+            ]
+        )
+        linter.reset_counters()
+
+        linter.lint_migration(migration)
+
+        self.assertTrue(linter.has_errors)

tests/unit/test_sql_analyser.py

@@ -11,12 +11,13 @@ from django_migration_linter.sql_analyser import (
 class SqlAnalyserTestCase(unittest.TestCase):
     database_vendor = "default"
 
-    def analyse_sql(self, sql):
+    def analyse_sql(self, sql, **kwargs):
         if isinstance(sql, str):
             sql = sql.splitlines()
         return analyse_sql_statements(
             get_sql_analyser_class(self.database_vendor),
             sql_statements=sql,
+            **kwargs,
         )
 
     def assertValidSql(self, sql, allow_warnings=False):
@@ -249,6 +250,31 @@ class PostgresqlAnalyserTestCase(SqlAnalyserTestCase):
         ]
         self.assertValidSql(sql)
 
+    def test_add_required_foreign_key_after_insert_into_new_table(self):
+        sql = [
+            'CREATE TABLE "archive_capture" ("id" uuid NOT NULL PRIMARY KEY);',
+            'INSERT INTO "archive_capture" ("id") VALUES (gen_random_uuid());',
+            'ALTER TABLE "archive_capture" ADD COLUMN "source_id" uuid NOT NULL REFERENCES "archive_source" ("id") DEFERRABLE INITIALLY DEFERRED;',
+        ]
+        self.assertBackwardIncompatibleSql(sql, "NOT_NULL")
+
+    def test_add_required_foreign_key_after_unknown_data_operation(self):
+        sql = [
+            'CREATE TABLE "archive_capture" ("id" uuid NOT NULL PRIMARY KEY);',
+            'ALTER TABLE "archive_capture" ADD COLUMN "source_id" uuid NOT NULL REFERENCES "archive_source" ("id") DEFERRABLE INITIALLY DEFERRED;',
+        ]
+        errors, _, _ = self.analyse_sql(sql, has_unknown_data_operations=True)
+        self.assertTrue(any(error.code == "NOT_NULL" for error in errors))
+
+    def test_add_required_foreign_key_after_table_name_reuse(self):
+        sql = [
+            'CREATE TABLE "foo" ("id" uuid NOT NULL PRIMARY KEY);',
+            'ALTER TABLE "foo" RENAME TO "baz";',
+            'ALTER TABLE "bar" RENAME TO "foo";',
+            'ALTER TABLE "foo" ADD COLUMN "source_id" uuid NOT NULL REFERENCES "archive_source" ("id");',
+        ]
+        self.assertBackwardIncompatibleSql(sql, "NOT_NULL")
+
     def test_alter_column(self):
         sql = 'ALTER TABLE "app_alter_column_a" ALTER COLUMN "field" TYPE varchar(10) USING "field"::varchar(10);'
         self.assertBackwardIncompatibleSql(sql)