Use method as cache key prefix for non-GET requests

[hx]

The response for a HEAD request shouldn't be served in response to a GET request.

In practise, HEAD responses contain http.noBody values, which return 0, io.EOF if you try to read them. Prior to this change, that's what happened if you hit a server with a HEAD request and then GET the same URI.

Rather than prefix all keys, I've only added prefixing to non-GET requests. As well as saving the cost of a string concatenation, it ensures existing disk caches can upgrade without completely invalidating.

[hx]

@gregjones @shurcooL do you have any time to review this?

[dmitshur]

Thanks a lot for contributing @hx.

At first, this fix seemed surprising to me. I was quite sure that the library already takes methods into account and doesn't serve cache responses ignoring methods.

Now I understand why I thought that. What we do is:

cacheable := (req.Method == "GET" || req.Method == "HEAD") && req.Header.Get("range") == ""

So all non-GET and non-HEAD methods are never cached or returned from cache. However, I see this bug is about mixing up those two methods which we do cache.

Now, it makes complete sense to me. This is definitely a very real bug that we should fix. (I also suspect there's a chance some buggy caching behavior I saw in the past might have been because of this bug.)

I like the idea of only using method prefix for non-GET methods, since they're less common.

Overall, I don't see any ways to improve this, and this change looks correct and logical to me. See some minor comments about the test. Otherwise LGTM.

[dmitshur]

Minor, but there are 196 matches for "resp" in this file. Let's use resp here too, for consistency.

[dmitshur]

I think you should also check for error here. We wouldn't want this test to falsely pass if this Do happens to fail.

_, err = s.client.Do(req)
if err != nil {
	t.Fatal(err)
}

[dmitshur]

I've taken the liberty of applying those minor suggestions for improving the test; please let me know if you disagree with them.

[hx]

I agree with your changes @shurcooL. Consistency is good. I also didn't know GitHub allowed the addition of commits to someone else's PR; TIL.

Regarding consistency, I used the http package's method constants http.MethodGet and http.MethodHead, which ignores the library's precedent of using plain quoted strings. The reasoning is more typo elimination (e.g. http.MethodHaed will fail more emphatically) than optimisation. I'm happy either way, but it's worth noting.

Thanks for your time reviewing the PR. I'll try to keep the test style more consistent in the next one.

[dmitshur]

I also didn't know GitHub allowed the addition of commits to someone else's PR; TIL.

It's a new feature from last year, see https://github.com/blog/2247-improving-collaboration-with-forks. Maintainers can only edit your PR if you have the "Allow edits from maintainers" checkbox checked.

I used the http package's method constants http.MethodGet and http.MethodHead, which ignores the library's precedent of using plain quoted strings. The reasoning is more typo elimination (e.g. http.MethodHaed will fail more emphatically) than optimisation. I'm happy either way, but it's worth noting.

That's completely okay. I prefer using the named consts too, but I'm okay with either.

[hx]

@shurcooL gotcha. Anything stopping us from merging?

[dmitshur]

I'll give @gregjones a day or two to see if he has any other comments. After that, I can merge.

[gregjones]

❤️ Smart fix, I like it! Thanks for identifying and fixing

[dmitshur]

Thank you @hx again for this valuable fix, and for your patience with us getting to this PR.