[Django] #31286: Database specific checks always goes to the 'default' backend
Django
-------------------------------------+-------------------------------------
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
-------------------------------------+-------------------------------------
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
-------------------------------------+-------------------------------------
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
-------------------------------------+-------------------------------------
| Ma
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* 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
-------------------------------------+-------------------------------------
Reporter: Hongtao Ma | Owner: Hongtao
| Ma
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 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
-------------------------------------+-------------------------------------
Reporter: Hongtao Ma | Owner: Hongtao
| Ma
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
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
-------------------------------------+-------------------------------------
Reporter: Hongtao Ma | Owner: Hongtao
| Ma
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
Comment:
Thanks for the patch @Taoup!
--
Ticket URL: <https://code.djangoproject.com/ticket/31286#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Hongtao Ma | Owner: Hongtao
| Ma
Component: Database layer | Version: master
(models, ORM) |
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
-------------------------------------+-------------------------------------
* 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
-------------------------------------+-------------------------------------
Reporter: Hongtao Ma | Owner: Hongtao
| Ma
Type: Bug | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 1 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
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
-------------------------------------+-------------------------------------
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
Needs tests: 1 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* cc: Jacob Walls (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/31286#comment:8>
Django
-------------------------------------+-------------------------------------
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
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
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
normal checks.
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
-------------------------------------+-------------------------------------
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
-------------------------------------+-------------------------------------
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>