#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 , 8 months ago
comment:2 by , 8 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 , 8 months ago
Has patch: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
comment:5 by , 8 months ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Type: | Bug → Cleanup/optimization |
comment:6 by , 8 months ago
Needs documentation: | set |
---|---|
Needs tests: | set |
comment:7 by , 8 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 , 8 months ago
But a float for max-age
is definitely wrong.
'Cache-Control': 'max-age=3.1415927'
comment:9 by , 8 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)?
comment:10 by , 8 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 , 8 months ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
I changed the PR accordingly
follow-up: 13 comment:12 by , 8 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 []
follow-up: 16 comment:13 by , 7 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 , 7 months ago
Summary: | CACHE_MIDDLEWARE_SECONDS can be set to a float but has to be an int → Clarify that CACHE_MIDDLEWARE_SECONDS should be an integer. |
---|
comment:15 by , 7 months ago
Triage Stage: | Accepted → Ready for checkin |
---|
This is related to https://code.djangoproject.com/ticket/27225