-
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
fix: unsafe methods not causing cache purge #3739
Conversation
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>
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.
lgtm
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.
have to review
I think there is a bigger issue here. |
According to my assessment the whole logic is flawed. |
provided an alternative PR #3739 |
cache Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>
What is the point of "methods" if you ignore it basically? |
It isn't being ignored, the check at the beginning of each request is all it's needed for |
I thought about it and I think the methods option makes not much sense. lets say you have array safeMethods with values [A,B] and methods with values [B] So method is A => [A,B].includes(A) && !([B].includes(A)) => true && true => true But what if I want to add in the future e.g. PROPFIND, which should be in theory a safe method (also ignore the fact that we would throw now a TypeError) I add methods to PROPFIND but it wont work, because it is not in the safeMethods Array. I would purge the cache even if I added PROPFIND to the methods array. |
I think you partially convinced me. |
We could safe the two includes by generating ONE array const safeMethodsToNotCache = util.safeHTTPMethods.filter(method => methods.includes(method) === false) safeMethods would be [A,B], methods would be [B], so the resulting safeMethodToNotCache would be [A] then the table would look like: A => [A].includes(A) => true |
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.
not sure why the cache-interceptor/interceptor.js
was deleted. I'd prefer slightly more test coverage, but wont block on that. lgtm otherwise
@Uzlopak @Ethan-Arrowood the deleted test file was a duplicate - it still exists under |
please adapt the code regarding the one array includes check. |
Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>
I do think something like the I do think it'd be a good idea to make an issue or pr to talk about supporting methods that were defined in extension specs though |
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.
LGTM
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.
cc @mcollina @Uzlopak