This repository has been archived by the owner on Nov 22, 2018. It is now read-only.
Add support for vary by query string params #21
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JunTaoLuo
force-pushed
the
johluo/vary-params
branch
from
837b982
to
efdcb34
Compare
| if (varyBy.Params.Count == 1 && string.Equals(varyBy.Params[0], "*", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| // Vary by all available query params | ||
| foreach (var query in _httpContext.Request.Query.OrderBy(q => q.Key)) |
There was a problem hiding this comment.
A few issues here.
- order by case sensitivity? The QueryCollection operates on case-insensitive keys so maybe we need to order by
IgnoreCaseand convert all query keys to upper/lower case. Sigh perf... - Linq vs Array.Sort() or List.Sort(). Was using Linq to guarantee a stable sort but I think the keys are unique so it doesn't matter. Need to check the impact on performance.
- Could consider using the raw
QueryStringbut that approach will forgo order-insensitivity of the params. I don't think we should do this unless perf is a big problem here.
JunTaoLuo
force-pushed
the
johluo/vary-params
branch
from
ea0c741
to
c00a684
Compare
|
|
JunTaoLuo
force-pushed
the
johluo/vary-params
branch
from
c00a684
to
1d6c5af
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
cc @Tratcher @davidfowl