diff --git a/django_migration_linter/migration_linter.py b/django_migration_linter/migration_linter.py index ec5e273..95f97dd 100644 --- a/django_migration_linter/migration_linter.py +++ b/django_migration_linter/migration_linter.py @@ -1,14 +1,16 @@ from __future__ import print_function import hashlib +import inspect import logging import os import re +from collections import defaultdict from subprocess import Popen, PIPE from django.conf import settings from django.core.management import call_command -from django.db import DEFAULT_DB_ALIAS, connections, ProgrammingError +from django.db import DEFAULT_DB_ALIAS, connections, ProgrammingError, migrations from enum import Enum, unique from .cache import Cache @@ -20,12 +22,14 @@ from .sql_analyser import analyse_sql_statements logger = logging.getLogger(__name__) DJANGO_APPS_WITH_MIGRATIONS = ("admin", "auth", "contenttypes", "sessions") +EXPECTED_DATA_MIGRATION_ARGS = ("apps", "schema_editor") @unique class MessageType(Enum): OK = "ok" IGNORE = "ignore" + WARNING = "warning" ERROR = "error" @staticmethod @@ -55,7 +59,7 @@ class MigrationLinter(object): self.ignore_name = ignore_name or tuple() self.include_apps = include_apps self.exclude_apps = exclude_apps - self.exclude_migration_tests = exclude_migration_tests + self.exclude_migration_tests = exclude_migration_tests or [] self.database = database or DEFAULT_DB_ALIAS self.cache_path = cache_path or DEFAULT_CACHE_PATH self.no_cache = no_cache @@ -66,6 +70,7 @@ class MigrationLinter(object): # Initialise counters self.nb_valid = 0 self.nb_ignored = 0 + self.nb_warnings = 0 self.nb_erroneous = 0 self.nb_total = 0 @@ -123,18 +128,32 @@ class MigrationLinter(object): return sql_statements = self.get_sql(app_label, migration_name) - exclude_migration_tests = self.exclude_migration_tests or [] errors, ignored = analyse_sql_statements( sql_statements, settings.DATABASES[self.database]["ENGINE"], - exclude_migration_tests, + self.exclude_migration_tests, ) + err, warnings = self.analyse_data_migration( + app_label, migration_name, self.exclude_migration_tests + ) + if err: + errors += err + if errors: self.print_linting_msg(app_label, migration_name, "ERR", MessageType.ERROR) self.nb_erroneous += 1 self.print_errors(errors) - value_to_cache = {"result": "ERR", "errors": errors} + if warnings: + self.print_warnings(warnings) + value_to_cache = {"result": "ERR", "errors": errors, "warnings": warnings} + elif warnings: + self.print_linting_msg( + app_label, migration_name, "WARNING", MessageType.WARNING + ) + self.nb_warnings += 1 + self.print_warnings(warnings) + value_to_cache = {"result": "WARNING", "warnings": warnings} else: if ignored: self.print_linting_msg( @@ -169,6 +188,12 @@ class MigrationLinter(object): app_label, migration_name, "OK (cached)", MessageType.OK ) self.nb_valid += 1 + elif cached_value["result"] == "WARNING": + self.print_linting_msg( + app_label, migration_name, "WARNING (cached)", MessageType.WARNING + ) + self.nb_warnings += 1 + self.print_warnings(cached_value["warnings"]) else: self.print_linting_msg( app_label, migration_name, "ERR (cached)", MessageType.ERROR @@ -176,6 +201,8 @@ class MigrationLinter(object): self.nb_erroneous += 1 if "errors" in cached_value: self.print_errors(cached_value["errors"]) + if "warnings" in cached_value and cached_value["warnings"]: + self.print_warnings(cached_value["warnings"]) self.new_cache[md5hash] = cached_value @@ -196,14 +223,32 @@ class MigrationLinter(object): error_str += ")" print(error_str) + def print_warnings(self, warnings): + if MessageType.WARNING.value in self.quiet: + return + + for function_name, warning_details in warnings.items(): + for warn_detail in warning_details: + warn_str = "\tWARN '{}': {}".format( + function_name, warn_detail["warn_msg"] + ) + print(warn_str) + def print_summary(self): print("*** Summary:") print( ( "Valid migrations: {1}/{0} - " "erroneous migrations: {2}/{0} - " - "ignored migrations: {3}/{0}" - ).format(self.nb_total, self.nb_valid, self.nb_erroneous, self.nb_ignored) + "migrations with warnings: {3}/{0} - " + "ignored migrations: {4}/{0}" + ).format( + self.nb_total, + self.nb_valid, + self.nb_erroneous, + self.nb_warnings, + self.nb_ignored, + ) ) @property @@ -315,3 +360,46 @@ class MigrationLinter(object): in self.migration_loader.applied_migrations ) ) + + def analyse_data_migration( + self, app_label, migration_name, exclude_migration_tests + ): + migration = self.migration_loader.disk_migrations[(app_label, migration_name)] + errors = [] + warnings = defaultdict(list) + + for operation in migration.operations: + if isinstance(operation, migrations.RunPython): + function_name = operation.code.__name__ + op_errors, op_warnings = self.lint_runpython(operation) + if op_errors: + errors += op_errors + if op_warnings: + warnings[function_name] = op_warnings + + return errors, warnings + + @staticmethod + def lint_runpython(runpython): + error = [] + warning = [] + + if not runpython.reversible: + warning.append( + { + "code": "REVERSIBLE_DATA_MIGRATION", + "warn_msg": "RunPython data migration is not reversible", + } + ) + + args_spec = inspect.getfullargspec(runpython.code) + if tuple(args_spec.args) != EXPECTED_DATA_MIGRATION_ARGS: + warning.append( + { + "code": "NAMING_CONVENTION_RUNPYTHON_ARGS", + "warn_msg": "By convention, RunPython names the two arguments: apps, schema_editor", + } + ) + + # TODO: test if importing model directly or correctly using "get_model()" + return error, warning diff --git a/docs/usage.md b/docs/usage.md index 2c46cad..bc32b7e 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -24,7 +24,7 @@ Detailed command line options: |`--unapplied-migrations` | Only lint migrations that are not yet applied to the selected database. Other migrations are ignored. | |`--project-root-path DJANGO_PROJECT_FOLDER` | An absolute or relative path to the django project. | |`--include-migrations-from FILE_PATH` | If specified, only migrations listed in the given file will be considered. | -|`--quiet or -q {ok,ignore,error}` | Suppress certain output messages, instead of writing them to stdout. | +|`--quiet or -q {ok,ignore,warning,error}` | Suppress certain output messages, instead of writing them to stdout. | ## Ignoring migration tests diff --git a/tests/fixtures.py b/tests/fixtures.py index 585bfb1..d037295 100644 --- a/tests/fixtures.py +++ b/tests/fixtures.py @@ -10,3 +10,4 @@ ALTER_COLUMN = "app_alter_column" ALTER_COLUMN_DROP_NOT_NULL = "app_alter_column_drop_not_null" DROP_UNIQUE_TOGETHER = "app_unique_together" ADD_MANYTOMANY_FIELD = "app_add_manytomany_field" +DATA_MIGRATIONS = "app_data_migrations" diff --git a/tests/functional/test_data_migrations.py b/tests/functional/test_data_migrations.py new file mode 100644 index 0000000..4ff7ae2 --- /dev/null +++ b/tests/functional/test_data_migrations.py @@ -0,0 +1,36 @@ +import os +import unittest + +from django.conf import settings + +from django_migration_linter import MigrationLinter +from tests import fixtures + + +class DataMigrationDetectionTestCase(unittest.TestCase): + def setUp(self, *args, **kwargs): + self.test_project_path = os.path.dirname(settings.BASE_DIR) + self.linter = MigrationLinter( + self.test_project_path, + include_apps=fixtures.DATA_MIGRATIONS, + ) + + def test_reverse_data_migration(self): + self.assertEqual(0, self.linter.nb_warnings) + reverse_migration = self.linter.migration_loader.disk_migrations[ + ("app_data_migrations", "0002_missing_reverse") + ] + self.linter.lint_migration(reverse_migration) + + self.assertEqual(1, self.linter.nb_warnings) + self.assertFalse(self.linter.has_errors) + + def test_reverse_data_migration_ignore(self): + self.assertEqual(0, self.linter.nb_warnings) + reverse_migration = self.linter.migration_loader.disk_migrations[ + ("app_data_migrations", "0003_incorrect_arguments") + ] + self.linter.lint_migration(reverse_migration) + + self.assertEqual(1, self.linter.nb_warnings) + self.assertFalse(self.linter.has_errors) diff --git a/tests/test_project/settings.py b/tests/test_project/settings.py index a221a97..c2a4bc3 100644 --- a/tests/test_project/settings.py +++ b/tests/test_project/settings.py @@ -53,6 +53,7 @@ INSTALLED_APPS = [ "tests.test_project.app_rename_column", "tests.test_project.app_rename_table", "tests.test_project.app_unique_together", + "tests.test_project.app_data_migrations", ] MIDDLEWARE = [