fix: unsafe methods not causing cache purge

[flakey5]

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

[mcollina]

lgtm

[Uzlopak]

have to review

[Uzlopak]

I think there is a bigger issue here.

[Uzlopak]

According to my assessment the whole logic is flawed.

[Uzlopak]

provided an alternative PR #3739

[Uzlopak]

What is the point of "methods" if you ignore it basically?

[flakey5]

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

[Uzlopak]

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
B => [A,B].includes(B) && !([B].includes(B)) => true && false => false
C => [A,B].includes(C) && !([B].includes(C)) => false && true => false

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.

[Uzlopak]

I think you partially convinced me.

[Uzlopak]

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
B => [A].includes(B) => false
C => [A].includes(C) => false

[Ethan-Arrowood]

not sure why the cache-interceptor/interceptor.js was deleted. I'd prefer slightly more test coverage, but wont block on that. lgtm otherwise

[flakey5]

@Uzlopak @Ethan-Arrowood the deleted test file was a duplicate - it still exists under test/interceptors/cache.js

[Uzlopak]

please adapt the code regarding the one array includes check.

[flakey5]

But what if I want to add in the future e.g. PROPFIND

I do think something like the safeMethods option you were talking about would be valuable for this, but I'm not sure how well that behavior falls into the scope of this pr. This pr (and the original caching pr) was mainly targeting the methods specified as safe in the http semantics spec (9.2.1)

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

[Uzlopak]

LGTM