[Django] #35920: Migrate command runs system checks regardless of the value of requires_system_checks

[Django]

#35920: Migrate command runs system checks regardless of the value of
requires_system_checks
-------------------------------------+-------------------------------------
Reporter: Jacob | Owner: Jacob Walls
Walls |
Type: Bug | Status: assigned
Component: Core | Version: 5.1
(Management commands) |
Severity: Normal | Keywords: skip-checks
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Without `--skip-checks=False`, the `migrate` command runs system checks
regardless of the value of `requires_system_checks`, which is `[]` by
[https://github.com/django/django/blob/4c452cc377f6f43acd90c6e54826ebd2e6219b0d/django/core/management/commands/migrate.py#L22
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.

----

- I wrote a system check following the
[https://docs.djangoproject.com/en/5.1/topics/checks/#field-model-manager-
template-engine-and-database-checks 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):
{{{#!diff
diff --git a/django/core/management/commands/migrate.py
b/django/core/management/commands/migrate.py
index fa420ee6e3..4000d76f3b 100644
--- a/django/core/management/commands/migrate.py
+++ b/django/core/management/commands/migrate.py
@@ -19,14 +19,9 @@ class Command(BaseCommand):
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,7 +94,7 @@ class Command(BaseCommand):
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 :-)
--
Ticket URL: <https://code.djangoproject.com/ticket/35920>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35920: Migrate command runs system checks regardless of the value of
requires_system_checks
-------------------------------------+-------------------------------------

Reporter: Jacob Walls | Owner: Jacob

| Walls
Type: Bug | Status: assigned

Component: Core (Management | Version: 5.1
commands) |
Severity: Normal | Resolution:
Keywords: skip-checks | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Description changed by Jacob Walls:

Old description:

New description:

Without `--skip-checks=False`, the `migrate` command runs system checks
regardless of the value of `requires_system_checks`, which is `[]` by
[https://github.com/django/django/blob/4c452cc377f6f43acd90c6e54826ebd2e6219b0d/django/core/management/commands/migrate.py#L22
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
[https://github.com/django/django/blob/c2c7dbb2f88ce8f0ef6d48a61b93866c9926349a/django/core/management/commands/runserver.py#L30
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.)

--
Ticket URL: <https://code.djangoproject.com/ticket/35920#comment:1>

[Django]

#35920: Migrate command runs system checks regardless of the value of
requires_system_checks
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: Core (Management | Version: 5.1
commands) |
Severity: Normal | Resolution:

Keywords: skip-checks | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* cc: Simon Charette, Hasan Ramezani (added)
* stage: Unreviewed => Accepted

Comment:

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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35920#comment:2>

[Django]

#35920: Migrate command runs system checks regardless of the value of
requires_system_checks
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: Core (Management | Version: 5.1
commands) |
Severity: Normal | Resolution:
Keywords: skip-checks | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* has_patch: 0 => 1

Comment:

[https://github.com/django/django/pull/18839 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
[https://docs.djangoproject.com/en/5.1/topics/checks/#writing-your-own-
checks 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35920#comment:3>

[Django]

#35920: Migrate command runs system checks regardless of the value of
requires_system_checks
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: Core (Management | Version: 5.1
commands) |
Severity: Normal | Resolution:
Keywords: skip-checks | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* needs_better_patch: 0 => 1

Comment:

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)
[https://github.com/django/django/blob/857b1048d53ebf5fc5581c110e85c212b81ca83a/django/core/management/base.py#L452-L459
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?

{{{#!patch
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 @@ def execute(self, *args, **options):
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 @@ def execute(self, *args, **options):
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,
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35920#comment:4>

[Django]

#35920: Migrate command runs system checks regardless of the value of
requires_system_checks
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: Core (Management | Version: 5.1
commands) |
Severity: Normal | Resolution:
Keywords: skip-checks | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* needs_better_patch: 1 => 0

Comment:

Great idea.
--
Ticket URL: <https://code.djangoproject.com/ticket/35920#comment:5>

[Django]

#35920: Migrate command runs system checks regardless of the value of
requires_system_checks
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: Core (Management | Version: 5.1
commands) |
Severity: Normal | Resolution:
Keywords: skip-checks | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 1

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* needs_docs: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/35920#comment:6>

[Django]

#35920: Migrate command runs system checks regardless of the value of
requires_system_checks
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: Core (Management | Version: 5.1
commands) |
Severity: Normal | Resolution:
Keywords: skip-checks | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* needs_docs: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/35920#comment:7>

[Django]

#35920: Migrate command runs system checks regardless of the value of
requires_system_checks
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: Core (Management | Version: 5.1
commands) |
Severity: Normal | Resolution:

Keywords: skip-checks | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/35920#comment:8>

[Django]

#35920: Migrate command runs system checks regardless of the value of
requires_system_checks
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls

Type: Bug | Status: closed

Component: Core (Management | Version: 5.1
commands) |

Severity: Normal | Resolution: fixed

Keywords: skip-checks | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce <42296566+sarahboyce@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"2ce4545de1791d6ed2405cb0657401e179bc5357" 2ce4545d]:
{{{#!CommitTicketReference repository=""
revision="2ce4545de1791d6ed2405cb0657401e179bc5357"
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 <chare...@gmail.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35920#comment:9>