diff --git a/CHANGELOG.md b/CHANGELOG.md index 7332967..14da09e 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/DECISIONS.md b/DECISIONS.md index 649104c..034482a 100644 --- a/DECISIONS.md +++ b/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 -fully linted, so `RunSQL`, `RunPython`, and operations on existing tables retain their -checks. +NOT EXISTS`, SQL inserts, and data operations, including nested +`SeparateDatabaseAndState` operations, do not prove an empty table. The migration remains fully +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. diff --git a/src/django_migration_linter/migration_linter.py b/src/django_migration_linter/migration_linter.py index 50aae47..94aed31 100644 --- a/src/django_migration_linter/migration_linter.py +++ b/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.migrations import Migration, RunPython, RunSQL +from django.db import DEFAULT_DB_ALIAS, ProgrammingError, connections, migrations +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 diff --git a/src/django_migration_linter/sql_analyser/analyser.py b/src/django_migration_linter/sql_analyser/analyser.py index 9ea3d54..9adf0b8 100644 --- a/src/django_migration_linter/sql_analyser/analyser.py +++ b/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 diff --git a/src/django_migration_linter/sql_analyser/base.py b/src/django_migration_linter/sql_analyser/base.py index 3f14476..10fb967 100644 --- a/src/django_migration_linter/sql_analyser/base.py +++ b/src/django_migration_linter/sql_analyser/base.py @@ -39,9 +39,16 @@ _ADD_COLUMN = re.compile( rf"^ALTER TABLE (?:ONLY )?(?P{_TABLE_NAME}) ADD (?:COLUMN )?", re.IGNORECASE, ) +_INSERT_INTO = re.compile(rf"^INSERT INTO (?P
{_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("(? 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 diff --git a/tests/unit/test_linter.py b/tests/unit/test_linter.py index ae97b8b..4463cd5 100644 --- a/tests/unit/test_linter.py +++ b/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 migrations +from django.db import ProgrammingError, migrations, models 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.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 + linter = MigrationLinter() 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) diff --git a/tests/unit/test_sql_analyser.py b/tests/unit/test_sql_analyser.py index ccd83ee..63d88aa 100644 --- a/tests/unit/test_sql_analyser.py +++ b/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)