Cache is "skipped" when E-Tag matches.

[clawrenceks]

Summary of issue

I am currently using the HttpCacheHeaders library in one of my projects and I believe I have found a scenario where the cache is being "skipped" (by skipped, I mean a 200 response is generated and a full round trip is performed, when I expected a 304 response to be returned) when a valid E-Tag is provided. The issue that I see is below. At the end of the issue I have outlined the behaviour I was expecting.

I am happy to assist with a pull request to address this issue if that would be helpful, but wanted to discuss my findings before diving into code changes.

Example of the problem

The problem that I am experiencing happens in the following scenario (which I expect is a very common scenario).

1. The CacheHeaders middleware generates an E-Tag header for the response.
2. The CacheHeaders middleware also generates a last-modified header for the response.

In this particular scenario I am not using an ILastModifiedInjector to customise the last-modified date. This means that the current date and time is used for the last-modified header on the http response.

My client application in this scenario is an SPA and the results I have documented below are based on using the Chrome developer tools.

As the Http Response from the API contains both an E-Tag and Last-Modified header, when a request to the same resource is repeated, the values from these headers are used to set the if-none-match and if-modified-since headers on the http request. The HTTP Method in my particular case is a GET request.

Because the browser has included the correct E-Tag in the if-none-match header my expectation is that a 304 response would be returned by the cache headers middleware. What I actually see is that a 200 response is returned and the request makes it all the way through to the backend data store of my solution.

The following logs generated from the middleware confirm the behaviour described above.

info: Marvin.Cache.Headers.HttpCacheHeadersMiddleware[0] Checking for conditional GET/HEAD. info: Marvin.Cache.Headers.HttpCacheHeadersMiddleware[0] Checking If-None-Match. info: Marvin.Cache.Headers.HttpCacheHeadersMiddleware[0] Checking If-None-Match: "1FE5601CF89161A9CC4DBC3CA29389EF". info: Marvin.Cache.Headers.HttpCacheHeadersMiddleware[0] ETag valid: "1FE5601CF89161A9CC4DBC3CA29389EF". info: Marvin.Cache.Headers.HttpCacheHeadersMiddleware[0] Checking If-Modified-Since: Fri, 10 Jan 2020 20:19:03 GMT info: Marvin.Cache.Headers.HttpCacheHeadersMiddleware[0] Don't generate 304 - Not Modified. Continue.

As can be seen in the above logs, the Etag included in the Http Request was a valid E-Tag.

Expected Behaviour

When I was testing this scenario in my application, I was expecting that a 304 result should be returned and not a 200 result. I was also expecting that the request would never hit the backend storage for my solution. The reason I expected this is because of the fact that the E-Tag check passed.

Having looked at the MDN documentation for If-None-Match and If-Modified-Since the following statements exist.

If-None-Match

When used in combination with If-Modified-Since, If-None-Match has precedence (if the server supports it).

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/If-None-Match

If-Modified-Since

When used in combination with If-None-Match, it is ignored, unless the server doesn't support If-None-Match.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/If-Modified-Since

The scenarios described above are currently not the case with this middleware. If the request includes both the If-None-Match headers and the If-Modified-Since headers, both values are evaluated when determining how to respond. This happens in lines 245-248 of the file HttpCacheHeadersMiddleware.cs as shown below.

// check the ETags // return the combined result of all validators. return CheckIfNoneMatchIsValid(httpContext, validatorValue) && await CheckIfModifiedSinceIsValid(httpContext, validatorValue);

From the MDN documentation and from a quick glance of the Http Spec, it looks like this code should actually check If-None-Match first and if that is valid, respond accordingly without checking If-Modified-Since.

If the middleware did work as described above, I think that would significantly increase the cache hit rate when a response does not use a custom ILastModifiedInjector - which I imagine could be a large body of the responses processed by this middleware.

I should add - I have tested using a customer ILastModifiedInjector to generate a data based on information from the resource and this issue does not occur in that scenario. A 304 result is returned as expected, but both header values are still checked by the middleware, which appears to be incorrect behaviour.

[KevinDockx]

Hello Chris,

I investigated this and it seems you're right - this is definitely a bug. If-Modified-Since should be ignored if there's a valid If-None-Match header, so your proposed solution is the way the go.

Would you be willing to create a PR for this (if you don't have time, no worries - I'll fix the bug)?

[clawrenceks]

Hi Kevin,

Thanks for investigating the issue. Sure, I would be happy to create a PR for this one. I have got a couple of things to finish up and then I will look at raising a PR during the week.

[clawrenceks]

Hi Kevin,

I have created a PR which I believe should resolve this issue.

Should this pull request be approved, when do you anticipate a new package would be available on NuGet containing these changes?

[KevinDockx]

Thanks for this - package is available now :) => https://www.nuget.org/packages/Marvin.Cache.Headers/5.0.0-beta