[Django] #35141: CACHE_MIDDLEWARE_SECONDS can be set to a float but has to be an int

[Django]

#35141: CACHE_MIDDLEWARE_SECONDS can be set to a float but has to be an int
-----------------------------------------------+------------------------
Reporter: Alexander Lazarević | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | 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 |
-----------------------------------------------+------------------------
CACHE_MIDDLEWARE_SECONDS can be a float like 2.0 instead of 2 and will
also be set in the response header `Cache-Control` to `max-age: 2.0`

This showed up in a template testcase, where it is set to a float

{{{
@override_settings(
CACHE_MIDDLEWARE_SECONDS=2.0,
ROOT_URLCONF="template_tests.alternate_urls"
)
class CacheMiddlewareTest(SimpleTestCase):
}}}

It would be sufficient to change the `override_settings` to `2` to make
the test correct, but I propose to cast the
`settings.CACHE_MIDDLEWARE_SECONDS` value to int at the places it is used,
for the same reasons as in https://code.djangoproject.com/ticket/31982
--
Ticket URL: <https://code.djangoproject.com/ticket/35141>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35141: CACHE_MIDDLEWARE_SECONDS can be set to a float but has to be an int

-------------------------------------+-------------------------------------

Reporter: Alexander Lazarević | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: dev

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 Alexander Lazarević):

This is related to https://code.djangoproject.com/ticket/27225
--
Ticket URL: <https://code.djangoproject.com/ticket/35141#comment:1>

[Django]

#35141: CACHE_MIDDLEWARE_SECONDS can be set to a float but has to be an int
-------------------------------------+-------------------------------------
Reporter: Alexander Lazarević | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: dev
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 Alexander Lazarević):

The testcase in question produces a `TemplateResponse` like this:

{{{
<TemplateResponse status_code=200, "text/html; charset=utf-8">
{'Content-Type': 'text/html; charset=utf-8', 'Expires': 'Thu, 25 Jan 2024
03:19:03 GMT', 'Cache-Control': 'max-age=2.0'}
}}}

And from https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-
Control

"The max-age=N response directive indicates that the response remains
fresh until N seconds after the response is generated."

Where N is an int
--
Ticket URL: <https://code.djangoproject.com/ticket/35141#comment:2>

[Django]

#35141: CACHE_MIDDLEWARE_SECONDS can be set to a float but has to be an int
-------------------------------------+-------------------------------------
Reporter: Alexander Lazarević | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: dev
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 Alexander Lazarević):

Created a PR https://github.com/django/django/pull/17776
--
Ticket URL: <https://code.djangoproject.com/ticket/35141#comment:3>

[Django]

#35141: CACHE_MIDDLEWARE_SECONDS can be set to a float but has to be an int
-------------------------------------+-------------------------------------

Reporter: Alexander Lazarević | Owner: Alexander
| Lazarević
Type: Bug | Status: assigned

Component: Core (Cache system) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Alexander Lazarević):

* owner: nobody => Alexander Lazarević
* status: new => assigned
* has_patch: 0 => 1

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

[Django]

#35141: CACHE_MIDDLEWARE_SECONDS can be set to a float but has to be an int
-------------------------------------+-------------------------------------
Reporter: Alexander Lazarević | Owner: Alexander

Type: | Lazarević
Cleanup/optimization | Status: assigned

Component: Core (Cache system) | Version: dev
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* type: Bug => Cleanup/optimization
* stage: Unreviewed => Accepted

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

[Django]

#35141: CACHE_MIDDLEWARE_SECONDS can be set to a float but has to be an int
-------------------------------------+-------------------------------------
Reporter: Alexander Lazarević | Owner: Alexander
Type: | Lazarević
Cleanup/optimization | Status: assigned
Component: Core (Cache system) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

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

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):

* needs_docs: 0 => 1
* needs_tests: 0 => 1

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

[Django]

#35141: CACHE_MIDDLEWARE_SECONDS can be set to a float but has to be an int
-------------------------------------+-------------------------------------
Reporter: Alexander Lazarević | Owner: Alexander
Type: | Lazarević
Cleanup/optimization | Status: assigned
Component: Core (Cache system) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Tim Graham):

I'm not sure we want to go down the path of casting every integer setting?
(e.g. #35041 for `DATA_UPLOAD_MAX_MEMORY_SIZE` was recently a wontfix.)
--
Ticket URL: <https://code.djangoproject.com/ticket/35141#comment:7>

[Django]

#35141: CACHE_MIDDLEWARE_SECONDS can be set to a float but has to be an int
-------------------------------------+-------------------------------------
Reporter: Alexander Lazarević | Owner: Alexander
Type: | Lazarević
Cleanup/optimization | Status: assigned
Component: Core (Cache system) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Alexander Lazarević):

But a float for `max-age` is definitely wrong.

{{{
'Cache-Control': 'max-age=3.1415927'
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35141#comment:8>

[Django]

#35141: CACHE_MIDDLEWARE_SECONDS can be set to a float but has to be an int
-------------------------------------+-------------------------------------
Reporter: Alexander Lazarević | Owner: Alexander
Type: | Lazarević
Cleanup/optimization | Status: assigned
Component: Core (Cache system) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Tim Graham):

Certainly, but I wonder if it's Django's responsibility (and any third-
party code that uses the setting) to silently correct such a mistake. I
see some precedent in #31982 although there isn't much elaboration on the
rationale.

If we are going down this route, should reopen #35041 and do the same (as
well as do a more extensive audit throughout Django and handle this
pattern proactively)?
--
Ticket URL: <https://code.djangoproject.com/ticket/35141#comment:9>

[Django]

#35141: CACHE_MIDDLEWARE_SECONDS can be set to a float but has to be an int
-------------------------------------+-------------------------------------
Reporter: Alexander Lazarević | Owner: Alexander
Type: | Lazarević
Cleanup/optimization | Status: assigned
Component: Core (Cache system) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Alexander Lazarević):

Ok in #35041 it's been said that

> ... however we cannot add type checks for all settings. It's documented
as integer and Django crashes when you use it incorrectly, so it's hard to
miss.

Well currently a float for CACHE_MIDDLEWARE_SECONDS will just work, but
produce the wrong value for max_age, which is easy to miss.

But looking forward I have a draft PR for #27225 that will make Django
crash, when CACHE_MIDDLEWARE_SECONDS is a float. So this would be a harsh
indicator that the setting for CACHE_MIDDLEWARE_SECONDS is wrong.

'''With that I'm suggesting I change the PR for this ticket to "just" fix
the comment and the setting in the testcase? What do you think?'''

Regardless of that, maybe two more thoughts:

> Certainly, but I wonder if it's Django's responsibility (and any third-
party code that uses the setting) to silently correct such a mistake.

In this case I would rather want Django sillently do the right thing than
silently do the wrong thing.

> I'm not sure we want to go down the path of casting every integer
setting?

I think we shouldn't make the perfect (type checking every setting) the
enemy of the good (fixing one particular problem with one setting). In
general.
--
Ticket URL: <https://code.djangoproject.com/ticket/35141#comment:10>

[Django]

#35141: CACHE_MIDDLEWARE_SECONDS can be set to a float but has to be an int
-------------------------------------+-------------------------------------
Reporter: Alexander Lazarević | Owner: Alexander
Type: | Lazarević
Cleanup/optimization | Status: assigned
Component: Core (Cache system) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

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

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

Changes (by Alexander Lazarević):

* needs_docs: 1 => 0
* needs_tests: 1 => 0

Comment:

I changed the PR accordingly
--
Ticket URL: <https://code.djangoproject.com/ticket/35141#comment:11>

[Django]

#35141: CACHE_MIDDLEWARE_SECONDS can be set to a float but has to be an int
-------------------------------------+-------------------------------------
Reporter: Alexander Lazarević | Owner: Alexander
Type: | Lazarević
Cleanup/optimization | Status: assigned
Component: Core (Cache system) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Alexander Lazarević):

> I'm not sure we want to go down the path of casting every integer
setting?

Maybe this could be part of the system checks? Here as an example only for
one setting, but could be extended to more settings as well.

{{{
@register(Tags.files)
def check_settings_types(app_configs, **kwargs):
setting = getattr(settings, "CACHE_MIDDLEWARE_SECONDS", None)
if setting and not isinstance(setting, int):
return [
Error(
"The CACHE_MIDDLEWARE_SECONDS setting should be an
integer.",
id="files.E002",
)
]
return []
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35141#comment:12>

[Django]

#35141: CACHE_MIDDLEWARE_SECONDS can be set to a float but has to be an int
-------------------------------------+-------------------------------------
Reporter: Alexander Lazarević | Owner: Alexander
Type: | Lazarević
Cleanup/optimization | Status: assigned
Component: Core (Cache system) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak):

Replying to [comment:12 Alexander Lazarević]:

> > I'm not sure we want to go down the path of casting every integer
setting?
>
> Maybe this could be part of the system checks? Here as an example only
for one setting, but could be extended to more settings as well.
>
> {{{
> @register(Tags.files)
> def check_settings_types(app_configs, **kwargs):
> setting = getattr(settings, "CACHE_MIDDLEWARE_SECONDS", None)
> if setting and not isinstance(setting, int):
> return [
> Error(
> "The CACHE_MIDDLEWARE_SECONDS setting should be an
integer.",
> id="files.E002",
> )
> ]
> return []
> }}}

All such checks have brought more bad than good effects in the past. We
almost always run into issues when folks make some tricky things, where
something behave like something else but does not pass `isinstance()`
check.

Let's focus in clarifying this in docs.
--
Ticket URL: <https://code.djangoproject.com/ticket/35141#comment:13>