From 2ce4545de1791d6ed2405cb0657401e179bc5357 Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Sat, 23 Nov 2024 11:41:14 -0500 Subject: [PATCH] Fixed #35920 -- Observed requires_system_checks in migrate and runserver. Before, the full suite of system checks was run by these commands regardless if requires_system_checks had been overridden. Co-authored-by: Simon Charette --- django/core/management/base.py | 11 +++-- django/core/management/commands/migrate.py | 13 ++--- django/core/management/commands/runserver.py | 15 +++--- docs/howto/custom-management-commands.txt | 15 ++++++ docs/releases/5.2.txt | 4 ++ tests/admin_scripts/tests.py | 52 +++++++++++++++++++- tests/migrations/test_commands.py | 28 ++++++++++- 7 files changed, 114 insertions(+), 24 deletions(-) diff --git a/django/core/management/base.py b/django/core/management/base.py index 6232b42bd4..ba38ae1748 100644 --- a/django/core/management/base.py +++ b/django/core/management/base.py @@ -450,10 +450,8 @@ class BaseCommand: self.stderr = OutputWrapper(options["stderr"]) if self.requires_system_checks and not options["skip_checks"]: - if self.requires_system_checks == ALL_CHECKS: - self.check() - else: - self.check(tags=self.requires_system_checks) + check_kwargs = self.get_check_kwargs(options) + self.check(**check_kwargs) if self.requires_migrations_checks: self.check_migrations() output = self.handle(*args, **options) @@ -468,6 +466,11 @@ class BaseCommand: self.stdout.write(output) return output + def get_check_kwargs(self, options): + if self.requires_system_checks == ALL_CHECKS: + return {} + return {"tags": self.requires_system_checks} + def check( self, app_configs=None, diff --git a/django/core/management/commands/migrate.py b/django/core/management/commands/migrate.py index fa420ee6e3..4ef6e1a87c 100644 --- a/django/core/management/commands/migrate.py +++ b/django/core/management/commands/migrate.py @@ -19,14 +19,8 @@ class Command(BaseCommand): help = ( "Updates database schema. Manages both apps with migrations and those without." ) - requires_system_checks = [] def add_arguments(self, parser): - parser.add_argument( - "--skip-checks", - action="store_true", - help="Skip system checks.", - ) parser.add_argument( "app_label", nargs="?", @@ -95,12 +89,13 @@ class Command(BaseCommand): help="Delete nonexistent migrations from the django_migrations table.", ) + def get_check_kwargs(self, options): + kwargs = super().get_check_kwargs(options) + return {**kwargs, "databases": [options["database"]]} + @no_translations def handle(self, *args, **options): database = options["database"] - if not options["skip_checks"]: - self.check(databases=[database]) - self.verbosity = options["verbosity"] self.interactive = options["interactive"] diff --git a/django/core/management/commands/runserver.py b/django/core/management/commands/runserver.py index 2c8e445de8..fc1b264472 100644 --- a/django/core/management/commands/runserver.py +++ b/django/core/management/commands/runserver.py @@ -27,8 +27,6 @@ naiveip_re = _lazy_re_compile( class Command(BaseCommand): help = "Starts a lightweight web server for development." - # Validation is called explicitly each time the server is reloaded. - requires_system_checks = [] stealth_options = ("shutdown_message",) suppressed_base_arguments = {"--verbosity", "--traceback"} @@ -61,11 +59,6 @@ class Command(BaseCommand): dest="use_reloader", help="Tells Django to NOT use the auto-reloader.", ) - parser.add_argument( - "--skip-checks", - action="store_true", - help="Skip system checks.", - ) def execute(self, *args, **options): if options["no_color"]: @@ -79,6 +72,10 @@ class Command(BaseCommand): """Return the default WSGI handler for the runner.""" return get_internal_wsgi_application() + def get_check_kwargs(self, options): + """Validation is called explicitly each time the server reloads.""" + return {"tags": set()} + def handle(self, *args, **options): if not settings.DEBUG and not settings.ALLOWED_HOSTS: raise CommandError("You must set settings.ALLOWED_HOSTS if DEBUG is False.") @@ -132,7 +129,9 @@ class Command(BaseCommand): if not options["skip_checks"]: self.stdout.write("Performing system checks...\n\n") - self.check(display_num_errors=True) + check_kwargs = super().get_check_kwargs(options) + check_kwargs["display_num_errors"] = True + self.check(**check_kwargs) # Need to check migrations here, so can't use the # requires_migrations_check attribute. self.check_migrations() diff --git a/docs/howto/custom-management-commands.txt b/docs/howto/custom-management-commands.txt index d3775905d3..de6c38c1e0 100644 --- a/docs/howto/custom-management-commands.txt +++ b/docs/howto/custom-management-commands.txt @@ -319,6 +319,21 @@ the :meth:`~BaseCommand.handle` method must be implemented. checks, and list of database aliases in the ``databases`` to run database related checks against them. +.. method:: BaseCommand.get_check_kwargs(options) + + .. versionadded:: 5.2 + + Supplies kwargs for the call to :meth:`check`, including transforming the + value of :attr:`requires_system_checks` to the ``tag`` kwarg. + + Override this method to change the values supplied to :meth:`check`. For + example, to opt into database related checks you can override + ``get_check_kwargs()`` as follows:: + + def get_check_kwargs(self, options): + kwargs = super().get_check_kwargs(options) + return {**kwargs, "databases": [options["database"]]} + .. _ref-basecommand-subclasses: ``BaseCommand`` subclasses diff --git a/docs/releases/5.2.txt b/docs/releases/5.2.txt index 907159e36d..5e692c0345 100644 --- a/docs/releases/5.2.txt +++ b/docs/releases/5.2.txt @@ -279,6 +279,10 @@ Management Commands ``Command.autodetector`` attribute for subclasses to override in order to use a custom autodetector class. +* The new :meth:`.BaseCommand.get_check_kwargs` method can be overridden in + custom commands to control the running of system checks, e.g. to opt into + database-dependent checks. + Migrations ~~~~~~~~~~ diff --git a/tests/admin_scripts/tests.py b/tests/admin_scripts/tests.py index 3145723920..5d8a5ec97e 100644 --- a/tests/admin_scripts/tests.py +++ b/tests/admin_scripts/tests.py @@ -20,6 +20,8 @@ from user_commands.utils import AssertFormatterFailureCaughtContext from django import conf, get_version from django.conf import settings +from django.core.checks import Error, Tags, register +from django.core.checks.registry import registry from django.core.management import ( BaseCommand, CommandError, @@ -27,7 +29,7 @@ from django.core.management import ( color, execute_from_command_line, ) -from django.core.management.base import LabelCommand +from django.core.management.base import LabelCommand, SystemCheckError from django.core.management.commands.loaddata import Command as LoaddataCommand from django.core.management.commands.runserver import Command as RunserverCommand from django.core.management.commands.testserver import Command as TestserverCommand @@ -1733,7 +1735,53 @@ class ManageRunserver(SimpleTestCase): stdout=self.output, ) self.assertIn("Performing system checks...", self.output.getvalue()) - mocked_check.assert_called() + mocked_check.assert_has_calls( + [mock.call(tags=set()), mock.call(display_num_errors=True)] + ) + + def test_custom_system_checks(self): + original_checks = registry.registered_checks.copy() + + @register(Tags.signals) + def my_check(app_configs, **kwargs): + return [Error("my error")] + + class CustomException(Exception): + pass + + self.addCleanup(setattr, registry, "registered_checks", original_checks) + + class CustomRunserverCommand(RunserverCommand): + """Rather than mock run(), raise immediately after system checks run.""" + + def check_migrations(self, *args, **kwargs): + raise CustomException + + class CustomRunserverCommandWithSignalsChecks(CustomRunserverCommand): + requires_system_checks = [Tags.signals] + + command = CustomRunserverCommandWithSignalsChecks() + with self.assertRaises(SystemCheckError): + call_command( + command, + use_reloader=False, + skip_checks=False, + stdout=StringIO(), + stderr=StringIO(), + ) + + class CustomMigrateCommandWithSecurityChecks(CustomRunserverCommand): + requires_system_checks = [Tags.security] + + command = CustomMigrateCommandWithSecurityChecks() + with self.assertRaises(CustomException): + call_command( + command, + use_reloader=False, + skip_checks=False, + stdout=StringIO(), + stderr=StringIO(), + ) class ManageRunserverMigrationWarning(TestCase): diff --git a/tests/migrations/test_commands.py b/tests/migrations/test_commands.py index 18f7e1e157..5ff5cd4b26 100644 --- a/tests/migrations/test_commands.py +++ b/tests/migrations/test_commands.py @@ -8,6 +8,8 @@ from pathlib import Path from unittest import mock from django.apps import apps +from django.core.checks import Error, Tags, register +from django.core.checks.registry import registry from django.core.management import CommandError, call_command from django.core.management.base import SystemCheckError from django.core.management.commands.makemigrations import ( @@ -96,6 +98,7 @@ class MigrateTests(MigrationTestBase): self.assertTableNotExists("migrations_tribble") self.assertTableNotExists("migrations_book") + @mock.patch("django.core.management.base.BaseCommand.check") @override_settings( INSTALLED_APPS=[ "django.contrib.auth", @@ -103,10 +106,33 @@ class MigrateTests(MigrationTestBase): "migrations.migrations_test_apps.migrated_app", ] ) - def test_migrate_with_system_checks(self): + def test_migrate_with_system_checks(self, mocked_check): out = io.StringIO() call_command("migrate", skip_checks=False, no_color=True, stdout=out) self.assertIn("Apply all migrations: migrated_app", out.getvalue()) + mocked_check.assert_called_once() + + def test_migrate_with_custom_system_checks(self): + original_checks = registry.registered_checks.copy() + + @register(Tags.signals) + def my_check(app_configs, **kwargs): + return [Error("my error")] + + self.addCleanup(setattr, registry, "registered_checks", original_checks) + + class CustomMigrateCommandWithSignalsChecks(MigrateCommand): + requires_system_checks = [Tags.signals] + + command = CustomMigrateCommandWithSignalsChecks() + with self.assertRaises(SystemCheckError): + call_command(command, skip_checks=False, stderr=io.StringIO()) + + class CustomMigrateCommandWithSecurityChecks(MigrateCommand): + requires_system_checks = [Tags.security] + + command = CustomMigrateCommandWithSecurityChecks() + call_command(command, skip_checks=False, stdout=io.StringIO()) @override_settings( INSTALLED_APPS=[