#31286: Database specific fields checks should be databases aware.
-------------------------------------+-------------------------------------
Reporter: Hongtao Ma | Owner: Hongtao
| Ma
Type: Bug | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
I think there's a misunderstanding with your statement: "Database checks
​were never ran by default even prior to this patch." The only database
check (designated by `@register(Tags.database)`) invokes
[https://github.com/django/django/blob/74a9c2711cd81708ae754c4d92432511efa96149/django/core/checks/database.py#L6-L14
DatabaseValidation.check()]. This invokes, for example, MySQL's
[https://github.com/django/django/blob/74a9c2711cd81708ae754c4d92432511efa96149/django/db/backends/mysql/validation.py#L7-L36check
on strict mode]. This check introspects the database and fails if it
doesn't exist, so I understand that the test runner may need to skip these
checks.
Separately, the `DatabaseValidation` class also has a
[https://github.com/django/django/blob/74a9c2711cd81708ae754c4d92432511efa96149/django/db/backends/base/validation.py#L13-L32
check_field() hook] which calls `DatabaseValidation.check_field_type()`
(if implemented by the backend, e.g.
[https://github.com/django/django/blob/74a9c2711cd81708ae754c4d92432511efa96149/django/db/backends/mysql/validation.py#L38-L77
MySQL's]). This method does static analysis (checking field attributes)
and doesn't require the presence of a database. The
[https://github.com/django/django/blob/74a9c2711cd81708ae754c4d92432511efa96149/django/db/models/fields/__init__.py#L266-L274
calling chain] is `Field.check()` ->
`Field._check_backend_specific_checks()`. These checks were run by
`makemigrations` (and even by `runserver` which I'll get in to below)
until 271fdab8b78af558238df51c64b4d1c8dd0792bb, which stopped iterating
through all the databases (`django.db.connections`) and instead uses the
list of `databases` aliases passed from (typically) the management
command's `--database` argument (e.g. for
[https://github.com/django/django/blob/74a9c2711cd81708ae754c4d92432511efa96149/django/core/management/commands/migrate.py#L94
migrate]).
I believe it's useful to have field-specific checks run for all databases
at the `makemigrations` stage rather than deferring them until `migrate`,
at which point a potentially invalid migration has already been created.
Besides `Field._check_backend_specific_checks()`, the other checks that
rely on `databases` are therefore are currently deferred are:
-
[https://github.com/django/django/blob/74a9c2711cd81708ae754c4d92432511efa96149/django/db/models/fields/__init__.py#L390-L424
Field._check_db_default()]
-
[https://github.com/django/django/blob/74a9c2711cd81708ae754c4d92432511efa96149/django/db/models/fields/__init__.py#L438-L458
Field._check_db_comment()]
-
[https://github.com/django/django/blob/74a9c2711cd81708ae754c4d92432511efa96149/django/db/models/base.py#L2345C9-L2422
Model._check_long_column_names()]
-
[https://github.com/django/django/blob/74a9c2711cd81708ae754c4d92432511efa96149/django/db/models/base.py#L2159-L2168
Model._check_indexes()]
-
[https://github.com/django/django/blob/74a9c2711cd81708ae754c4d92432511efa96149/django/db/models/base.py#L2440-L2449
Model._check_constraints()]
-
[https://github.com/django/django/blob/74a9c2711cd81708ae754c4d92432511efa96149/django/db/models/base.py#L1837-L1857
Model._check_db_table_comment()]
My proposal is to make `makemigrations` run these checks for all
connections:
{{{#!diff
diff --git a/django/core/management/commands/makemigrations.py
b/django/core/management/commands/makemigrations.py
index 7f711ed7ae..5860c462c2 100644
--- a/django/core/management/commands/makemigrations.py
+++ b/django/core/management/commands/makemigrations.py
@@ -102,6 +102,10 @@ class Command(BaseCommand):
def log(self, msg):
self.log_output.write(msg)
+ def get_check_kwargs(self, options):
+ kwargs = super().get_check_kwargs(options)
+ return {**kwargs, "databases": connections}
+
@no_translations
def handle(self, *app_labels, **options):
self.written_files = []
}}}
While thinking through this, I noticed another surprising, perhaps
inadvertent regression / behavior change. Currently, in a multi-db setup
(I'll use the aliases 'default' and 'other' as an example), unless I
missed a place that passes all db aliases to `check()`, the only way to
get the pre-Django 3.1 behavior for `Model._check_long_column_names()`
(which consults all database aliases to find the maximum allowed column
name) would be to invoke: `check --database=default --database=other`.
Essentially, #31055 started treating the bulleted list of checks above as
"database checks" which was overly aggressive. They are really static
checks that don't require the presence of a database. These checks used to
be a lot more visible since they were even invoked by `runserver`
(right?). So arguably `runserver` (and even `check`) could get the same
`get_check_kwargs()` treatment proposed above, and then we may want to go
back to
[https://github.com/django/django/commit/0b83c8cc4db95812f1e15ca19d78614e94cf38dd
#diff-
f10ce154e1f5bcdba92b54ebf8cb57357b404a3f03ab51ca85a031358f7372ecL66-L69
excluding database tagged checks by default].
--
Ticket URL: <https://code.djangoproject.com/ticket/31286#comment:10>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion visit
https://groups.google.com/d/msgid/django-updates/010701995f953cf2-64d8d0d4-2083-42c2-9cda-020910b22958-000000%40eu-central-1.amazonses.com.
