[Django] #31286: Database specific checks always goes to the 'default' backend

[Django]

#31286: Database specific checks always goes to the 'default' backend
-------------------------------------+-------------------------------------
Reporter: Hongtao | Owner: nobody
Ma |
Type: Bug | Status: new
Component: Database | Version: master
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
In an attempt to trigger the check Error mentioned in Tickets: #31144, I
found that the check for database backend doesn't check against the
backend specified, e.g. --database mysql, rather, it always checks against
the 'default' backend.

After diving into the source code, I found the following method defined in
django/db/models/fields/__init__.py
{{{
def _check_backend_specific_checks(self, **kwargs):
app_label = self.model._meta.app_label
for db in connections:
if router.allow_migrate(db, app_label,
model_name=self.model._meta.model_name):
return connections[db].validation.check_field(self,
**kwargs)
return []
}}}
It checks the first db defined in connections rather those provided by
users.

A proposed change would be:
{{{
def _check_backend_specific_checks(self, **kwargs):
app_label = self.model._meta.app_label
errors = []
for db in kwargs['databases'] or ['default']:
if router.allow_migrate(db, app_label,
model_name=self.model._meta.model_name):
errors.extend(connections[db].validation.check_field(self,
**kwargs))
return errors
}}}

It worked as intended on my local machine.
I would happily provide a patch for this one.

--
Ticket URL: <https://code.djangoproject.com/ticket/31286>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#31286: Database specific fields checks should be databases aware.
-------------------------------------+-------------------------------------
Reporter: Hongtao Ma | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by felixxm):

* cc: Simon Charette (added)
* easy: 0 => 1
* stage: Unreviewed => Accepted

Comment:

Thanks, this was missed in 0b83c8cc4db95812f1e15ca19d78614e94cf38dd. We
should use here the same approach and pass `databases` to these checks:
{{{
diff --git a/django/db/models/fields/__init__.py
b/django/db/models/fields/__init__.py
index 6bd95f542c..c54b8f6e31 100644
--- a/django/db/models/fields/__init__.py
+++ b/django/db/models/fields/__init__.py
@@ -335,11 +335,13 @@ class Field(RegisterLookupMixin):
else:
return []

- def _check_backend_specific_checks(self, **kwargs):
+ def _check_backend_specific_checks(self, databases=None, **kwargs):
app_label = self.model._meta.app_label
- for db in connections:
- if router.allow_migrate(db, app_label,
model_name=self.model._meta.model_name):
- return connections[db].validation.check_field(self,
**kwargs)
+ if databases is None:
+ return []
+ for alias in databases:
+ if router.allow_migrate(alias, app_label,
model_name=self.model._meta.model_name):
+ return connections[alias].validation.check_field(self,
**kwargs)
return []

def _check_validators(self):
}}}

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

[Django]

#31286: Database specific fields checks should be databases aware.
-------------------------------------+-------------------------------------

Reporter: Hongtao Ma | Owner: Hongtao
| Ma
Type: Bug | Status: assigned

Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

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

Changes (by Hongtao Ma):

* owner: nobody => Hongtao Ma
* status: new => assigned

Comment:

Applied the patch, two testcases failed, however:

* FAIL: test_checks_called_on_the_default_database
(check_framework.test_multi_db.TestMultiDBChecks)
* FAIL: test_checks_called_on_the_other_database
(check_framework.test_multi_db.TestMultiDBChecks)

{{{
for alias in databases:
if router.allow_migrate(alias, app_label,
model_name=self.model._meta.model_name):

return connections[alias].validation.check_field(self,
**kwargs)
return []
}}}

The early return will cause only a single backend be checked, in case of
multiple backends provided:
{{{
(djangodev) ➜ my_test_proj git:(iss_check) ✗ python manage.py check
--database sqlite3 --database mysql
System check identified no issues (0 silenced).
(djangodev) ➜ my_test_proj git:(iss_check) ✗ python manage.py check
--database mysql
SystemCheckError: System check identified some issues:

ERRORS:
issue.AnotherIndex.name: (mysql.E001) MySQL does not allow unique
CharFields to have a max_length > 255.
issue.Networkservergroup.groupname: (mysql.E001) MySQL does not allow
unique CharFields to have a max_length > 255.

System check identified 2 issues (0 silenced).
(djangodev) ➜ my_test_proj git:(iss_check) ✗
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/31286#comment:2>

[Django]

#31286: Database specific fields checks should be databases aware.
-------------------------------------+-------------------------------------
Reporter: Hongtao Ma | Owner: Hongtao
| Ma
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

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

Comment (by felixxm):

You have to pass `databases={'default', 'other'}` to `model.check()` calls
in `TestMultiDBChecks`.

--
Ticket URL: <https://code.djangoproject.com/ticket/31286#comment:3>

[Django]

#31286: Database specific fields checks should be databases aware.
-------------------------------------+-------------------------------------
Reporter: Hongtao Ma | Owner: Hongtao
| Ma
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0

Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Hongtao Ma):

* has_patch: 0 => 1
* needs_tests: 0 => 1

Comment:

[https://github.com/django/django/pull/12472 PR]

--
Ticket URL: <https://code.djangoproject.com/ticket/31286#comment:4>

[Django]

#31286: Database specific fields checks should be databases aware.
-------------------------------------+-------------------------------------
Reporter: Hongtao Ma | Owner: Hongtao
| Ma
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:

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
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* stage: Accepted => Ready for checkin

Comment:

Thanks for the patch @Taoup!

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

[Django]

#31286: Database specific fields checks should be databases aware.
-------------------------------------+-------------------------------------
Reporter: Hongtao Ma | Owner: Hongtao
| Ma

Type: Bug | Status: closed

Component: Database layer | Version: master
(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
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak <felisiak.mariusz@…>):

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

Comment:

In [changeset:"271fdab8b78af558238df51c64b4d1c8dd0792bb" 271fdab]:
{{{
#!CommitTicketReference repository=""
revision="271fdab8b78af558238df51c64b4d1c8dd0792bb"
Fixed #31286 -- Made database specific fields checks databases aware.

Follow up to 0b83c8cc4db95812f1e15ca19d78614e94cf38dd.
}}}

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

[Django]

#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'm not following the analysis in the ticket description. My understanding
is that before this change, the database-specific field checks always ran
for all databases (`for db in connections`). Now that it uses `databases`
as specified by the `check` command, these checks are skipped unless
`check --database=...` is specified. While `migrate`'s check does
[https://github.com/django/django/blob/a0fb35eb726f1a04eaa1b47b8de191fafe55a0ab/django/core/management/commands/migrate.py#L94
specify a database], this change postpones these errors/warnings from the
`makemigrations` stage to the `migrate` stage, at which point the user
needs to take an extra corrective step of recreating the migration file.
(In my case, I wrote [https://github.com/mongodb/django-mongodb-
backend/pull/185/files a check to prohibit AutoField] for the MongoDB
backend.)

Although it's documented that "Database checks are not run by default
because they do more than static code analysis as regular checks do," the
[https://github.com/django/django/blob/a0fb35eb726f1a04eaa1b47b8de191fafe55a0ab/django/db/backends/mysql/validation.py#L38-L77
database-specific field checks] are only static analysis (as opposed to
something like
[https://github.com/django/django/blob/a0fb35eb726f1a04eaa1b47b8de191fafe55a0ab/django/db/backends/mysql/validation.py#L12-L36
the MySQL check for strict mode] which does query the database), so I see
no reason they shouldn't be included as part of the normal checks. I think
this patch should be reverted, but let me know if I've missed something.
--
Ticket URL: <https://code.djangoproject.com/ticket/31286#comment:7>

[Django]

#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
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* cc: Jacob Walls (added)

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

[Django]

#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 Simon Charette):

Thanks for sharing your thoughts Tim, having you work on that MongoDB
backend has been a great source of feedback for the rough edges of build
such a third-party app.

The intent of #31055 (0b83c8cc4db95812f1e15ca19d78614e94cf38dd) was two
fold

1. Ensure that the check framework is provided the exact set of databases
it should perform checks against (as not all of them might be available
all the time, e.g. migrations)
2. Have the test framework take advantage of 1. as since #28478 test
database creation is skipped if unneeded.

Now in retrospect I think that making the `check` command default
`databases=None` to `[]` instead of `list(connections)` might have been a
mistake but it's also not clear whether `[DEFAULT_DB_ALIAS]` might not
have been also a good candidate (that's what other database related
commands do)

In summary I wouldn't be opposed to reverting the part of this patch that
made `check` default to checking all databases (or only the `default` one)
but we must maintain the part that made check databases aware as it's
something the migration and testing framework rely on.

> Although it's documented that "Database checks are not run by default
because they do more than static code analysis as regular checks do," the

​database-specific field checks are only static analysis (as opposed to

something like ​the MySQL check for strict mode which does query the

database), so I see no reason they shouldn't be included as part of the
normal checks.

I don't disagree in principle but that seems like a totally different can
of worm isn't it? Database checks
[https://github.com/django/django/commit/0b83c8cc4db95812f1e15ca19d78614e94cf38dd
#diff-
f10ce154e1f5bcdba92b54ebf8cb57357b404a3f03ab51ca85a031358f7372ecL66-L69
were never ran by default even prior to this patch] so it has little to do
this with this particular ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/31286#comment:9>

[Django]

#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]

#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 Simon Charette):

Thanks for the analysis Tim

> This check queries the database server, but as far as I can tell, it
doesn't require the presence of the test database.

The mode is effectively not a database specific setting but it is
nonetheless a value that must be retrieved using a query (`SELECT
@@sql_mode` to be precise) which means that a connection must exists
against the server and if no test database is setup this leaves the
testing machinery no choice but to connect directly to the non-test
database which is frowned upon in a testing scenario. Now that we don't
systematically create / setup for reuse test databases when the suite
subset doesn't need it (#28478) we need a way to ensure we don't issue
database queries against un-prepared databases.

> These checks do static analysis (checking field attributes), and while
some of these checks may rely on a connection to the database server to
populate connection.features, for example, I don't believe these checks
require the presence of the test database.

Similar scenario in this case, they may or may not require to issue
database queries to perform these checks and we likely don't want them to
be performed against non-test databases.

> They are really static checks that don't require the presence of a
database.

That's the crux of the issue I believe. They may or may not as they are
opaque to the caller and its this opacity that forces setups where only a
subset of the databases to be safely available (e.g. test runs) to limit
their scope.

As I've shared in comment:9 I'm in agreement with you here though, I do
believe that we should default to having unspecified `database` subset
mean ''all databases'' instead of ''no databases'' and not only for
`makemigrations`. What I mean is that we should be doing something like
the following

{{{!diff
diff --git a/django/core/checks/database.py
b/django/core/checks/database.py
index d125f1eaa6..91265ac966 100644
--- a/django/core/checks/database.py
+++ b/django/core/checks/database.py
@@ -4,9 +4,7 @@


@register(Tags.database)
-def check_database_backends(databases=None, **kwargs):
- if databases is None:
- return []
+def check_database_backends(databases, **kwargs):
issues = []
for alias in databases:
conn = connections[alias]
diff --git a/django/core/checks/registry.py
b/django/core/checks/registry.py
index 3139fc3ef4..a285abbc3f 100644
--- a/django/core/checks/registry.py
+++ b/django/core/checks/registry.py
@@ -1,6 +1,7 @@
from collections.abc import Iterable
from itertools import chain

+from django.db import connections
from django.utils.inspect import func_accepts_kwargs


@@ -85,6 +86,9 @@ def run_checks(
if tags is not None:
checks = [check for check in checks if not
set(check.tags).isdisjoint(tags)]

+ if databases is None:

+ databases = list(connections)
+
for check in checks:
new_errors = check(app_configs=app_configs,
databases=databases)
if not isinstance(new_errors, Iterable):
diff --git a/django/core/management/commands/check.py
b/django/core/management/commands/check.py
index e61cff79f3..531e059c37 100644
--- a/django/core/management/commands/check.py
+++ b/django/core/management/commands/check.py
@@ -46,7 +46,7 @@ def add_arguments(self, parser):
action="append",
choices=tuple(connections),
dest="databases",
- help="Run database related checks against these aliases.",
+ help="Run database related checks only against these
aliases.",
)

def handle(self, *app_labels, **options):
}}}

I've given the full suite [https://github.com/django/django/pull/19912 a
go here]. Note that this preserves the behaviour of commands that directly
operate against a database and default to `[DB_DEFAULT_ALIAS]` such as
`migrate` as it's likely that not all databases are available in the
context they are run.
--
Ticket URL: <https://code.djangoproject.com/ticket/31286#comment:11>