Browse Source

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 <charette.s@gmail.com>
Jacob Walls 4 months ago
parent
commit
2ce4545de1

+ 7 - 4
django/core/management/base.py

@@ -450,10 +450,8 @@ class BaseCommand:
             self.stderr = OutputWrapper(options["stderr"])
             self.stderr = OutputWrapper(options["stderr"])
 
 
         if self.requires_system_checks and not options["skip_checks"]:
         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:
         if self.requires_migrations_checks:
             self.check_migrations()
             self.check_migrations()
         output = self.handle(*args, **options)
         output = self.handle(*args, **options)
@@ -468,6 +466,11 @@ class BaseCommand:
             self.stdout.write(output)
             self.stdout.write(output)
         return 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(
     def check(
         self,
         self,
         app_configs=None,
         app_configs=None,

+ 4 - 9
django/core/management/commands/migrate.py

@@ -19,14 +19,8 @@ class Command(BaseCommand):
     help = (
     help = (
         "Updates database schema. Manages both apps with migrations and those without."
         "Updates database schema. Manages both apps with migrations and those without."
     )
     )
-    requires_system_checks = []
 
 
     def add_arguments(self, parser):
     def add_arguments(self, parser):
-        parser.add_argument(
-            "--skip-checks",
-            action="store_true",
-            help="Skip system checks.",
-        )
         parser.add_argument(
         parser.add_argument(
             "app_label",
             "app_label",
             nargs="?",
             nargs="?",
@@ -95,12 +89,13 @@ class Command(BaseCommand):
             help="Delete nonexistent migrations from the django_migrations table.",
             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
     @no_translations
     def handle(self, *args, **options):
     def handle(self, *args, **options):
         database = options["database"]
         database = options["database"]
-        if not options["skip_checks"]:
-            self.check(databases=[database])
-
         self.verbosity = options["verbosity"]
         self.verbosity = options["verbosity"]
         self.interactive = options["interactive"]
         self.interactive = options["interactive"]
 
 

+ 7 - 8
django/core/management/commands/runserver.py

@@ -27,8 +27,6 @@ naiveip_re = _lazy_re_compile(
 class Command(BaseCommand):
 class Command(BaseCommand):
     help = "Starts a lightweight web server for development."
     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",)
     stealth_options = ("shutdown_message",)
     suppressed_base_arguments = {"--verbosity", "--traceback"}
     suppressed_base_arguments = {"--verbosity", "--traceback"}
 
 
@@ -61,11 +59,6 @@ class Command(BaseCommand):
             dest="use_reloader",
             dest="use_reloader",
             help="Tells Django to NOT use the auto-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):
     def execute(self, *args, **options):
         if options["no_color"]:
         if options["no_color"]:
@@ -79,6 +72,10 @@ class Command(BaseCommand):
         """Return the default WSGI handler for the runner."""
         """Return the default WSGI handler for the runner."""
         return get_internal_wsgi_application()
         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):
     def handle(self, *args, **options):
         if not settings.DEBUG and not settings.ALLOWED_HOSTS:
         if not settings.DEBUG and not settings.ALLOWED_HOSTS:
             raise CommandError("You must set settings.ALLOWED_HOSTS if DEBUG is False.")
             raise CommandError("You must set settings.ALLOWED_HOSTS if DEBUG is False.")
@@ -132,7 +129,9 @@ class Command(BaseCommand):
 
 
         if not options["skip_checks"]:
         if not options["skip_checks"]:
             self.stdout.write("Performing system checks...\n\n")
             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
         # Need to check migrations here, so can't use the
         # requires_migrations_check attribute.
         # requires_migrations_check attribute.
         self.check_migrations()
         self.check_migrations()

+ 15 - 0
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
     checks, and list of database aliases in the ``databases`` to run database
     related checks against them.
     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:
 .. _ref-basecommand-subclasses:
 
 
 ``BaseCommand`` subclasses
 ``BaseCommand`` subclasses

+ 4 - 0
docs/releases/5.2.txt

@@ -279,6 +279,10 @@ Management Commands
   ``Command.autodetector`` attribute for subclasses to override in order to use
   ``Command.autodetector`` attribute for subclasses to override in order to use
   a custom autodetector class.
   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
 Migrations
 ~~~~~~~~~~
 ~~~~~~~~~~
 
 

+ 50 - 2
tests/admin_scripts/tests.py

@@ -20,6 +20,8 @@ from user_commands.utils import AssertFormatterFailureCaughtContext
 
 
 from django import conf, get_version
 from django import conf, get_version
 from django.conf import settings
 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 (
 from django.core.management import (
     BaseCommand,
     BaseCommand,
     CommandError,
     CommandError,
@@ -27,7 +29,7 @@ from django.core.management import (
     color,
     color,
     execute_from_command_line,
     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.loaddata import Command as LoaddataCommand
 from django.core.management.commands.runserver import Command as RunserverCommand
 from django.core.management.commands.runserver import Command as RunserverCommand
 from django.core.management.commands.testserver import Command as TestserverCommand
 from django.core.management.commands.testserver import Command as TestserverCommand
@@ -1733,7 +1735,53 @@ class ManageRunserver(SimpleTestCase):
             stdout=self.output,
             stdout=self.output,
         )
         )
         self.assertIn("Performing system checks...", self.output.getvalue())
         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):
 class ManageRunserverMigrationWarning(TestCase):

+ 27 - 1
tests/migrations/test_commands.py

@@ -8,6 +8,8 @@ from pathlib import Path
 from unittest import mock
 from unittest import mock
 
 
 from django.apps import apps
 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 import CommandError, call_command
 from django.core.management.base import SystemCheckError
 from django.core.management.base import SystemCheckError
 from django.core.management.commands.makemigrations import (
 from django.core.management.commands.makemigrations import (
@@ -96,6 +98,7 @@ class MigrateTests(MigrationTestBase):
         self.assertTableNotExists("migrations_tribble")
         self.assertTableNotExists("migrations_tribble")
         self.assertTableNotExists("migrations_book")
         self.assertTableNotExists("migrations_book")
 
 
+    @mock.patch("django.core.management.base.BaseCommand.check")
     @override_settings(
     @override_settings(
         INSTALLED_APPS=[
         INSTALLED_APPS=[
             "django.contrib.auth",
             "django.contrib.auth",
@@ -103,10 +106,33 @@ class MigrateTests(MigrationTestBase):
             "migrations.migrations_test_apps.migrated_app",
             "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()
         out = io.StringIO()
         call_command("migrate", skip_checks=False, no_color=True, stdout=out)
         call_command("migrate", skip_checks=False, no_color=True, stdout=out)
         self.assertIn("Apply all migrations: migrated_app", out.getvalue())
         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(
     @override_settings(
         INSTALLED_APPS=[
         INSTALLED_APPS=[