Clarify that CACHE_MIDDLEWARE_SECONDS should be an integer.

[Alexander Lazarević]

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

[Alexander Lazarević]

This is related to https://code.djangoproject.com/ticket/27225

[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

[Alexander Lazarević]

Created a PR ​https://github.com/django/django/pull/17776

[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.)

[Alexander Lazarević]

But a float for max-age is definitely wrong.

'Cache-Control': 'max-age=3.1415927'

[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)?

[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.

[Alexander Lazarević]

I changed the PR accordingly

[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 []

[Mariusz Felisiak]

Replying to 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.

[Alexander Lazarević]

Replying to Mariusz Felisiak:

Let's focus in clarifying this in docs.

ok

[Mariusz Felisiak <felisiak.mariusz@…>]

In 22785f0:

Refs #35141 -- Corrected value of CACHE_MIDDLEWARE_SECONDS in CacheMiddlewareTest tests.

[Mariusz Felisiak <felisiak.mariusz@…>]

In a5365339:

Fixed #35141 -- Clarified the expected type of CACHE_MIDDLEWARE_SECONDS setting.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 28d6db2:

[5.0.x] Fixed #35141 -- Clarified the expected type of CACHE_MIDDLEWARE_SECONDS setting.

Backport of a5365339eaee043895a79dbbdd7462f1399136e5 from main