Database specific fields checks should be databases aware.

[Hongtao Ma]

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.

[Mariusz Felisiak]

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):

[Hongtao Ma]

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) ✗ 

[Mariusz Felisiak]

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

[Hongtao Ma]

​PR

[Simon Charette]

Thanks for the patch @Taoup!

[Mariusz Felisiak <felisiak.mariusz@…>]

In 271fdab:

Fixed #31286 -- Made database specific fields checks databases aware.

Follow up to 0b83c8cc4db95812f1e15ca19d78614e94cf38dd.

[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 ​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 ​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 ​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 think this patch should be reverted, but let me know if I've missed something.

[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 part of this patch that have check default to 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 somewhat different can of worm isn't it? Database checks ​were never ran by default even prior to this patch so it has little to do this with this particular ticket beyond the fact that specifying a --database is now the opt-in for enabling those instead of --tag database.