`manage.py check` unexpectedly accesses database when JSONField is used

[Storm Heg]

I suspect this is a regression in ​https://github.com/django/django/commit/3aba1fced8254435b947467739721ec6b4fb865c, the commit message does not suggest to me that this was intentional.

Issue: if you run manage.py check in a project that uses django.db.models.JSONField, the support_json_field SQLite feature detection is now being run (​https://github.com/django/django/blob/d506e4a52847ed4fb80754175d76b6b83ff90929/django/db/backends/sqlite3/features.py#L160) which unexpectedly interacts with the database.

This surfaced in the test suite of a package django-otp-webauthn, where it validates its own system checks behave as expected. The test cases in question are not marked as requiring database access, which is why the test against django's main branch started failing last night: ​https://github.com/Stormbase/django-otp-webauthn/actions/runs/19415998875/job/55581034014

Raising this here as a potential bug, feel free to close if not a bug (though a mention in 6.1 upgrade considerations would be welcome in that case)

[Storm Heg]

I've prepared a small django project to reproduce the issue: ​https://github.com/Stormheg/django-36739-manage-py-check-unexpected-database-access

[Simon Charette]

Hello Storm, thank you for your report!

This is effectively a change in behavior. I would say expected though as the commit intentionally made check perform all non-database tagged checks against all databases and JSONField.check chain of registration is not tagged Tags.database.

The question now becomes whether we want all checks that potentially access the database to be tagged with Tag.database. From my understanding the Tag.database tag was meant to convey check that the database is setup properly and never might potentially access the database so in this sense I don't think it would be appropriate.

Another way to see it is that staticfiles checks could perform access control gated I/O to determine if they are properly setup (e.g. has access to local or remote destination) some model checks and we don't treat them differently. The fact this was surfaced as a potential problem here is that SimpleTestCase was used to avoid database queries and previously the suite of checks performed by default didn't access the database. The thing is even if Django passes databases=None or doesn't call tests tagged with Tag.database it cannot guarantee that the check it calls actually don't access the database, it just happened to be the case within Django core for a long time.

For these reasons I think we should keep things as they are but in all cases I would likely augment the release notes to describe better that this is expected and running check without --tag database might require database access from now on.

Thoughts?

[Jacob Walls]

I agree with Simon's analysis.

(though a mention in 6.1 upgrade considerations would be welcome in that case)

I would welcome a PR adding a mention of this. We can take it as Refs #36663 -- .... Storm, would you like to make a PR?

[Storm Heg]

Thanks for the quick responses.

I suspected this might have been intentional, but wanted to make sure. Happy to close 👍

@Jacob what kind of wording would you like to see in the release notes?

I was looking in the documentation for system checks for a good place to document this behavior / expectation so this does not stay undocumented, but I'm not sure of a good place this would fit. Currently, the databases section (​https://github.com/django/django/blob/0c487aa3a7b2417481bf48c1e5355c855873e210/docs/ref/checks.txt#L81) reads Database checks are not run by default because they do more than static code analysis as regular checks do which implies regular checks only do static analysis (which turns out to not necessarily be true). Seems like low hanging fruit for a small docs improvement too.

[Jacob Walls]

Good point, we should weasel-ify that existing wording: "because they *typically*" or "because they *can be expected to*" etc

@Jacob what kind of wording would you like to see in the release notes?

Under "Backwards incompatible changes in 6.1" > "System checks" a sentence about how system checks now supply all databases if not specified, so calling code should be prepared for databases to potentially be accessed. Maybe a link to the docs for the databases kwarg:
​https://docs.djangoproject.com/en/stable/topics/checks/#writing-your-own-checks

Thanks.