[Django] #36739: `manage.py check` unexpectedly accesses database when JSONField is used

[Django]

#36739: `manage.py check` unexpectedly accesses database when JSONField is used
-------------------------------------+-------------------------------------
Reporter: Storm Heg | Type: Bug
Status: new | Component: Core
| (System checks)
Version: dev | Severity: Normal
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
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)
--
Ticket URL: <https://code.djangoproject.com/ticket/36739>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36739: `manage.py check` unexpectedly accesses database when JSONField is used
-------------------------------------+-------------------------------------

Reporter: Storm Heg | Owner: (none)
Type: Bug | Status: new
Component: Core (System | Version: dev
checks) |
Severity: Normal | Resolution:

Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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
--
Ticket URL: <https://code.djangoproject.com/ticket/36739#comment:1>

[Django]

#36739: `manage.py check` unexpectedly accesses database when JSONField is used
-------------------------------------+-------------------------------------
Reporter: Storm Heg | Owner: (none)
Type: Bug | Status: new
Component: Core (System | Version: dev
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* cc: charetttes, Tim Graham (added)

Comment:

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?
--
Ticket URL: <https://code.djangoproject.com/ticket/36739#comment:2>

[Django]

#36739: `manage.py check` unexpectedly accesses database when JSONField is used
-------------------------------------+-------------------------------------
Reporter: Storm Heg | Owner: (none)
Type: Bug | Status: new
Component: Core (System | Version: dev
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Simon Charette):

* cc: charetttes (removed)
* cc: Simon Charette (added)

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

[Django]

#36739: `manage.py check` unexpectedly accesses database when JSONField is used
-------------------------------------+-------------------------------------
Reporter: Storm Heg | Owner: (none)

Type: Bug | Status: closed

Component: Core (System | Version: dev
checks) |

Severity: Normal | Resolution: invalid

Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* resolution: => invalid
* status: new => closed

Comment:

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?
--
Ticket URL: <https://code.djangoproject.com/ticket/36739#comment:4>

[Django]

#36739: `manage.py check` unexpectedly accesses database when JSONField is used
-------------------------------------+-------------------------------------
Reporter: Storm Heg | Owner: (none)
Type: Bug | Status: closed
Component: Core (System | Version: dev
checks) |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36739#comment:5>

[Django]

#36739: `manage.py check` unexpectedly accesses database when JSONField is used
-------------------------------------+-------------------------------------
Reporter: Storm Heg | Owner: (none)
Type: Bug | Status: closed
Component: Core (System | Version: dev
checks) |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36739#comment:6>

[Django]

#36739: `manage.py check` unexpectedly accesses database when JSONField is used
-------------------------------------+-------------------------------------
Reporter: Storm Heg | Owner: (none)
Type: Bug | Status: closed
Component: Core (System | Version: dev
checks) |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Storm Heg):

I've prepared https://github.com/django/django/pull/20276 for the 6.1
release notes.

Hope I met all the standards and expectations around language for release
notes and commit messages.
--
Ticket URL: <https://code.djangoproject.com/ticket/36739#comment:7>

[Django]

#36739: `manage.py check` unexpectedly accesses database when JSONField is used
-------------------------------------+-------------------------------------
Reporter: Storm Heg | Owner: (none)
Type: Bug | Status: closed
Component: Core (System | Version: dev
checks) |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jacob Walls <jacobtylerwalls@…>):

In [changeset:"14fb36e0b083ea963220602d01386cc0fb2c40e4" 14fb36e0]:
{{{#!CommitTicketReference repository=""
revision="14fb36e0b083ea963220602d01386cc0fb2c40e4"
Refs #36663, #36739 -- Added release note for running checks against all
databases.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36739#comment:8>