Opened 10 months ago

Closed 9 months ago

Last modified 9 months ago

#35141 closed Cleanup/optimization (fixed)

Clarify that CACHE_MIDDLEWARE_SECONDS should be an integer.

Reported by: Alexander Lazarević Owned by: Alexander Lazarević
Component: Core (Cache system) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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

Change History (19)

comment:1 by Alexander Lazarević, 10 months ago

comment:2 by Alexander Lazarević, 10 months ago

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

comment:4 by Alexander Lazarević, 10 months ago

Has patch: set
Owner: changed from nobody to Alexander Lazarević
Status: newassigned

comment:5 by Mariusz Felisiak, 10 months ago

Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

comment:6 by Mariusz Felisiak, 10 months ago

Needs documentation: set
Needs tests: set

comment:7 by Tim Graham, 10 months ago

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

comment:8 by Alexander Lazarević, 10 months ago

But a float for max-age is definitely wrong.

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

comment:9 by Tim Graham, 10 months ago

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

I wonder if the Python standard library has a policy abou handling floats when integers are accepted.

Last edited 10 months ago by Tim Graham (previous) (diff)

comment:10 by Alexander Lazarević, 9 months ago

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.

comment:11 by Alexander Lazarević, 9 months ago

Needs documentation: unset
Needs tests: unset

I changed the PR accordingly

comment:12 by Alexander Lazarević, 9 months ago

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

in reply to:  12 ; comment:13 by Mariusz Felisiak, 9 months ago

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.

comment:14 by Mariusz Felisiak, 9 months ago

Summary: CACHE_MIDDLEWARE_SECONDS can be set to a float but has to be an intClarify that CACHE_MIDDLEWARE_SECONDS should be an integer.

comment:15 by Mariusz Felisiak, 9 months ago

Triage Stage: AcceptedReady for checkin

in reply to:  13 comment:16 by Alexander Lazarević, 9 months ago

Replying to Mariusz Felisiak:

Let's focus in clarifying this in docs.

ok

comment:17 by Mariusz Felisiak <felisiak.mariusz@…>, 9 months ago

In 22785f0:

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

comment:18 by Mariusz Felisiak <felisiak.mariusz@…>, 9 months ago

Resolution: fixed
Status: assignedclosed

In a5365339:

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

comment:19 by Mariusz Felisiak <felisiak.mariusz@…>, 9 months ago

In 28d6db2:

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

Backport of a5365339eaee043895a79dbbdd7462f1399136e5 from main

Note: See TracTickets for help on using tickets.
Back to Top