-
Notifications
You must be signed in to change notification settings - Fork 84
Add Vary: Accept-Encoding if response was compressed #199
Conversation
@@ -214,6 +214,11 @@ private void OnWrite() | |||
var compressionProvider = ResolveCompressionProvider(); | |||
if (compressionProvider != null) | |||
{ | |||
if (!HeaderUtilities.ContainsCacheDirective(HeaderNames.Vary, HeaderNames.AcceptEncoding)) | |||
{ | |||
_context.Response.Headers.Add(HeaderNames.Vary, HeaderNames.AcceptEncoding); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Append or AppendCommaSeperatedValue
@@ -214,6 +214,11 @@ private void OnWrite() | |||
var compressionProvider = ResolveCompressionProvider(); | |||
if (compressionProvider != null) | |||
{ | |||
if (!HeaderUtilities.ContainsCacheDirective(HeaderNames.Vary, HeaderNames.AcceptEncoding)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This utility is named poorly but does what is needed which is to scan the values of the headers and see if the specific value, in this case Accept-Encoding, exist. Maybe we need to rename it in HeaderUtilities or at least add a comment here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File a bug for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually what I said previously isn't accurate, the logic in HeaderUtilities is specific for cache directives. This is because it only handles the cases where the header values are in the format valueName=<someValue>, ...
which doesn't generalize to other headers such as Accept where the headers can look like valueName;q=<qValue>, ...
. For the use case here, the layout of the header value is even simpler since it is a comma separated list of tokens. I'll add a separate helper function.
var containsVaryAcceptEncoding = false; | ||
foreach (var value in response.Headers.GetValues(HeaderNames.Vary)) | ||
{ | ||
if (value.Contains(HeaderNames.AcceptEncoding)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why a Contains vs. an exact match?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or are you trying to avoid parsing the header into multiple segments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case either would work since the tests only set one value to the Vary header but in general when we are dealing with headers, we can't just assume each string in StringValues contains one value. We ran into this problem when we received Header["Cache-Control"] = new[] {"no-cache,no-store"} from MVC for example. So it's best to check using contains rather than exact match.
In the same regard, we also shouldn't be assuming the format of the headers. For example, the test could break with a bad Vary header like Headers["Vary"] = new [] {"badValue=Accept-Encoding"} but I ignored this corner case in the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you actually tested this together with the caching middleware? Middleware order would matter, you need to make sure this works as expected regardless of which middleware came first. E.g. the vary should only affect caching if the content gets compressed before caching.
@@ -9,12 +9,32 @@ | |||
using Microsoft.AspNetCore.Http; | |||
using Moq; | |||
using Xunit; | |||
using Microsoft.Net.Http.Headers; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sort
a8fc894
to
fbd38d1
Compare
Tested with the response caching middleware and filed a bug aspnet/ResponseCaching#92. Currently if the response is compressed before being cached, the expected behaviour where the response is cached per accept encoding is observed. If the compression comes after caching, however, the response is still cached per accept encoding. This issue will be fixed in the response caching middleware. |
fbd38d1
to
6855f9b
Compare
Fixes #187