Migrate command runs system checks regardless of the value of requires_system_checks

[Jacob Walls]

Without --skip-checks=False, the migrate command runs system checks regardless of the value of requires_system_checks, which is [] by ​default (itself a little misleading) but ultimately something I would like to be able to override at the project level. The fact that [] is not respected and not overridable seems like a bug, and AFAIK it's an asymmetry with other commands. (EDIT: turns out, runserver behaves in a similar way, with a ​comment from 17 years ago, predating migrations, explaining there is a separate mechanism from the one that became requires_system_checks that runs the system checks. I think we can improve the story here.)

---

• I wrote a system check following the ​documented example inside Model.check().
• I did this as part of a feature adding a new column to my model, and I queried this column in my check. I made the necessary migration.
• During manage.py migrate my check ran before my column was added, raising ProgrammingError.
• I verified --skip-checks works like a charm, but I don't want to impose cryptic failures on my fellow developers who expect plain migrate to work.
• I attempted to override the migrate command in my project, like this:

/app/management/commands/migrate.py

from django.core.checks.registry import registry
from django.core.management.commands.migrate import Command as MigrateCommand


class Command(MigrateCommand):
    # Silence model checks that may depend on new columns.
    requires_system_checks = list(registry.tags_available() - {"models"})

• Result: no change: ProgrammingError

• Then, I added this patch to Django. (--skip-checks still works; here, I have to avoid it being added again):

  django/core/management/commands/migrate.py

  @@ -19 +19 @@
       help = (
           "Updates database schema. Manages both apps with migrations and those without."
       )
  -    requires_system_checks = []
  +    requires_system_checks = "__all__"
   
       def add_arguments(self, parser):
  -        parser.add_argument(
  -            "--skip-checks",
  -            action="store_true",
  -            help="Skip system checks.",
  -        )
           parser.add_argument(
               "app_label",
               nargs="?",
  @@ -99 +94 @@
       def handle(self, *args, **options):
           database = options["database"]
           if not options["skip_checks"]:
  -            self.check(databases=[database])
  +            self.check(tags=self.requires_system_checks, databases=[database])
   
           self.verbosity = options["verbosity"]
           self.interactive = options["interactive"]

• Result: check skipped as expected via my project-level override of the migrate command.

---

I suppose there's another question here about whether Tags.models checks should run as part of the migrate command, or whether the change I made at the project level should be pulled into core, given that this is a reasonably realistic scenario for development? I can take that to the forum later. But at the moment, I'm just hoping to get the override working. Happy to PR this if welcome :-)

[Sarah Boyce]

It looks like previously requires_system_checks was a boolean.
It was updated to be able to specify specific checks: c60524c658f197f645b638f9bcc553103bfe2630 (#31546)
Prior to that commit there was no customizing what type of checks are run and it was all or nothing (unless overwritten by the command)

Previously 0b83c8cc4db95812f1e15ca19d78614e94cf38dd (#31055) implemented database specific checks and so marked requires_system_checks to False to run make sure only the database specific checks were run for migrate

So I think you're right that we should do an update here to make this work well together.

[Jacob Walls]

​PR

---

Update on this side point:

I suppose there's another question here about whether Tags.models checks should run as part of the migrate command, or whether the change I made at the project level should be pulled into core, given that this is a reasonably realistic scenario for development? I can take that to the forum later.

On second thought, that's a bad idea. Initially I missed the ​caveat that you need to guard your system check under if kwargs["database"]: if you're going to make queries. Guarding like that was sufficient to prevent the failures.

[Simon Charette]

I agree that it makes sense to adjust the migrate and runserver commands to allow for requires_system_checks to be adjusted but the I believe the proposed solution fails to account for the fact that BaseCommand.execute (which is .handle's caller) ​already calls check when requires_system_checks is not empty.

What we want here is a single call to self.check with the proper arguments being provided (databases and display_num_errors respectively). In the case of display_num_errors=Tue it's pretty trivial as runserver.Command.check can be overridden to call super().check() but in the case of migrate.Command it's a bit more tricky.

Maybe we could introduce a get_check_kwargs(**options) -> dict: Kwargs method that simplifies all that?

django/core/management/base.py

@@ -450 +450 @@
             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 +466 @@
             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,

[Jacob Walls]

Great idea.

[Sarah Boyce <42296566+sarahboyce@…>]

In 2ce4545d:

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@…>