CacheCow
CacheCow copied to clipboard
aliostad
Observe the `DefaultVaryHeaders` when creating the cache key
If the server replies with a Vary header then the DefaultVaryHeaders are not used when computing the cache key.
The new IncludeRangeHeaderInCacheKey test demonstrates this issue and ensure it's fixed.
This is a followup to #268
Hi,
I am not going to close this one ye. But if the server itself tells us what Vary headers are, then what good are default vary headers? The default vary headers are there as a starting point.
The point is the server is the definitive authority in declaring the parameters of its resources. Now if you are saying this is a means of dealing with Bad Servers is another thing but let me understand what we are trying to fix/improve.
If I understood the code correctly, the existing code
varyHeaders = serverResponse.Headers.Vary.Select(x => x).ToArray() are always replacing either there is varyHeaders or not.
DefaultVaryHeaders are not being used as it is being replaced.
Therefore, in my understanding at least
if (serverResponse.Headers.Vary != null)
should be
if (serverResponse.Headers.Vary != null && serverResponse.Headers.Vary.Count() > 0)
Let me review, thanks for raising again. I might have overlooked it.
Any update on this @aliostad
The issue (before this pull request) is that if you want to explicitly always include a header in the cache key (e.g. the Range header) and the server returns a Vary header, then what was explicitly configured is overridden by the content of the Vary header.
This pull request fixes this. Have a look at the IncludeRangeHeaderInCacheKey: if you run this test without the fix applied in CachineHandler.cs then the test doesn't pass because the Range header is not part of the cache key.
Also, the condition testing whether a Vary header exists in the response was incorrect: serverResponse.Headers.Vary != null is always true. The correct way to test if a Vary header is present in the response is if (serverResponse.Headers.Vary?.Any() ?? false).
OK apologies for not reviewing it earlier.
The behaviour is the one that is intended by deign. The server is indeed the ultimate authority to define its vary headers. There is a reason what you pass in is called DefaultVaryHeaders, it is merely the default which can be used in absence of server headers. As such your pull request cannot be accepted.
Now if you are saying that the server sends wrong headers and you need a means to deal with it then that is a different thing.
Can you please confirm if this was the case?
By the way as for the if (serverResponse.Headers.Vary != null) you are right that is a bug - but not one that hurts much. So let's clarify the situation and decide what to do.
OK apologies for not reviewing it earlier.
No need to apologise, you get to choose your own pace!
Now if you are saying that the server sends wrong headers and you need a means to deal with it then that is a different thing. Can you please confirm if this was the case?
I'm not familiar enough with RFC 7234 to assess whether it's wrong but yes, that's the case: I'd like to be able to choose a request header to be part of the cache key no matter what the server replies. Granted, re-using the DefaultVaryHeaders property and hijacking its intended purpose was probably not the best idea.
I personally like Cashew design with its ICacheKeyStrategy interface. Would you mind a similar approach? Or would you prefer to introduce a new property for the purpose of always including a header in the cache key?
No I think you have acted quite naturally.
Looking at the decision taken by Cashew, I must say it might be OK for them, but for me is too broad. There are issues such as someone might try to use the response stream and messing up with it (especially now that the ASP.NET have imposed restrictions on when something can be accessed and all). I would probably go for passing a copy of the headers. But that is perhaps just taste.
Anyway, it is perhaps not ideal but you can create your own IVaryHeaderStore which when returns the list of header names, adds the headers you are interested in on top based on the URL (obviously after checking if they are not already there). Unfortunately TryGet is not virtual but you can achieve the same with minimal code with composition (based on InMemoryVaryHeaderStore) instead of inheritance.
Meanwhile I will have a think about this where it can best fit - but cannot guarantee when can be done although it does not necessarily mean never!
Thanks for your interest. Keep it open for conversations but once you are OK please close it.
At least we can fix the bug that we know, hence created the PR, because it gives confidence to move ahead in use of this library further.