cache temporary redirects with explicit caching directives#405
cache temporary redirects with explicit caching directives#405cburroughs wants to merge 1 commit into
Conversation
RFC 9111 §3 - Storing Responses in Caches, describes the conditions under which a cache may store a response. Notably all final (aka not 1xx) status codes are *potentially* cacheable, subject to the other listed conditions. In brief a response must have either an explicit header (such as a `max-age` directive) OR have a heuristically cacheable status code. Which status codes are heuristically cacheable is helpfully summarized in RFC 9110 §15.1 - Overview of Status Codes: 200, 203, 204, 206, 300, 301, 308, 404, 405, 410, 414, and 501) CacheController has historically maintained a more conservative list of cacheable_status_codes that are a subset of the heuristically cacheable ones. This diff makes related changes: * Temporary redirects are now cached by default. Besides being correct in the general case, temporary redirects are used by several Python Registries as discussed in psf#384. * Because temporary redirects are not heuristically cacheable, add a check to only cache them when explicit headers are present. This means that regardless of which status_codes are passed to CacheController, the HTTP specification is still followed. * As defensive future proofing, make sure the response is final before much else is done. fixes psf#384
|
Hello! With pip 26.0 out this seemed like a good time to gently bump, as with the release already out it can't be part of trying to get something out at the last minute. Please let me know if there is any additional information I can provide for this PR. |
|
Sorry for the delay @cburroughs, I'll try and set aside some time to review this in the coming days. |
|
I'll try the opposite argument this time and say I'd love to get this landed before the upcoming Pip 26.2 pypa/pip#14067 ;-) (Although I know that with Pip's vendor-syncing cadence that might mean Pip doesn't pick this up until 26.3) |
woodruffw
left a comment
woodruffw
left a comment
There was a problem hiding this comment.
Thanks @cburroughs, sorry for my tardiness here. This looks great to me overall, just one question and one nitpick 🙂
| # other conditions. CacheController conservatively only caches common | ||
| # ones codes. For example, even with a max-age set, a 500 Internal |
There was a problem hiding this comment.
Q: what does "common ones" mean here?
|
|
||
| URI = re.compile(r"^(([^:/?#]+):)?(//([^/?#]*))?([^?#]*)(\?([^#]*))?(#(.*))?") | ||
|
|
||
| NEVER_CACHE_STATUSES = ( |
There was a problem hiding this comment.
Nit: let's make this and the other constants introduced by this PR private, since we're not committing to them as new public APIs 🙂
| NEVER_CACHE_STATUSES = ( | |
| _NEVER_CACHE_STATUSES = ( |
There was a problem hiding this comment.
(We do already have some public constants, which is unfortunate. So I'm not 100% opposed to making these public if you have an argument for it, but by default I'd prefer not to make the public API surface any bigger.)
2 participants
RFC 9111 §3 - Storing Responses in Caches, describes the conditions under which a cache may store a response. Notably all final (aka not 1xx) status codes are potentially cacheable, subject to the other listed conditions.
In brief a response must have either an explicit header (such as a
max-agedirective) OR have a heuristically cacheable status code. Which status codes are heuristically cacheable is helpfully summarized in RFC 9110 §15.1 - Overview of Status Codes: 200, 203, 204, 206, 300, 301, 308, 404, 405, 410, 414, and 501)CacheController has historically maintained a more conservative list of cacheable_status_codes that are a subset of the heuristically cacheable ones.
This diff makes related changes:
fixes #384