search

aliostad / CacheCow

An implementation of HTTP Caching in .NET Core and 4.5.2+ for both the client and the server
MIT License
847 stars 172 forks source link

Include the value of the "Range" header in the cache key #268

Closed 0xced closed 2 years ago

0xced commented 2 years ago

This is required for the caching handler to properly work with partial HTTP requests using the "Range" header. Without including the range in the cache key, the partial content stored first for a given URI would always be retrieved from the cache.

A test was added to ensure that two requests to the same URI with different ranges produce two entires in the cache.

aliostad commented 2 years ago

Hi Cédric,

Thanks for your contribution. I am afraid I cannot accept the PR.

There are a whole host of headers that could impact the caching. CacheCow only defines one but leaves it for the clients to extend it based on their needs. Accept-Language, Accept-Encoding and other headers could also impact the CacheKey. The property is public hence please just change it as per your needs. As you can see it defines just the default one.

This is a client scenario. You know what headers you are applying. Hence please just update the property as per need.

aliostad commented 2 years ago

Having said that, if you believe the docs need to be updated to make consumer aware of the gotcha please provide a PR on the docs.

0xced commented 2 years ago

OK I understand. I have removed the Range header from DefaultVaryHeaders and added it manually in the new IncludesRangeHeaderInCacheKey test.

But this new test still fails because if the server replies with a Vary header (which is the case for https://code.jquery.com/jquery-3.3.1.slim.min.js) then the DefaultVaryHeaders becomes discarded. So the modification that adds Concat(varyHeaders).Distinct(StringComparer.CurrentCultureIgnoreCase) to the vary headers is still required.

Could you please reopen this pull request?

0xced commented 2 years ago

Oh no, I have pushed my changes (in commit 9155a02fae66538a424581c70fd410bdcd7bcbba) but we can't see them on a closed pull request. 🙁

0xced commented 2 years ago

Anyway, I have opened #269 in which I hope I better explained the problem at hand.