#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 django-updates+unsubscr...@googlegroups.com.
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.
