-
Notifications
You must be signed in to change notification settings - Fork 564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[http-cache] follow up #3733
[http-cache] follow up #3733
Conversation
08bfb82
to
c65844d
Compare
@@ -75,14 +75,14 @@ class CacheHandler extends DecoratorHandler { | |||
) | |||
|
|||
if ( | |||
UNSAFE_METHODS.includes(this.#req.method) && | |||
!this.#methods.includes(this.#requestOptions.method) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WHy did we check for unsafe methods? this whole if condition seems actually kind of dubious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The invalidation logic is kind of strange. Also what is the point of passing methods, when they are ignored anyway? So I guess actually methods would have been an array of safe methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A cache MUST invalidate the target URI (Section 7.1 of [HTTP]) when it receives a non-error status code in response to an unsafe request method (including methods whose safety is unknown).
So I guess my proposed change should be right, as the passed methods will be considered safe methods. Especially in regard of the last sentence in the spec, that methods whose safety is unknown is also covered.
// Safe methods the user wants and unsafe methods | ||
const methods = [...globalOpts.methods, ...UNSAFE_METHODS] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did we "mixed" with unsafe methods?
this would result in caching unsafe requests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this allows requests with unsafe methods to get to the cache handler, where they'll cause a cache purge if the response is a success
@flakey5 PTAL |
Fixes bug introduced in nodejs#3733 where unsafe methods no longer make it to the cache handler and cause a cache purge for an origin. Also removes a duplicate test file. Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>
* fix: unsafe methods not causing cache purge Fixes bug introduced in #3733 where unsafe methods no longer make it to the cache handler and cause a cache purge for an origin. Also removes a duplicate test file. Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> * Update cache.js * extend test to check that safe methods we're not caching don't purge the cache Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> * code review Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --------- Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>
follow up of http-cache pr