-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for multiple Etags in If-None-Match header #6219
Add support for multiple Etags in If-None-Match header #6219
Conversation
LGTM 👍 I especially liked how it's not coupled to |
@sdogruyol Yeah, all this logic for handling cached requests should eventually be moved to |
src/http/headers.cr
Outdated
@@ -258,6 +258,73 @@ struct HTTP::Headers | |||
|
|||
forward_missing_to @hash | |||
|
|||
def if_none_match : Array(String)? |
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.
I don't think this parsing method belongs 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.
Yeah, I wasn't sure whether this should go here or in Request
. I'll move it to the latter.
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 wasn't removed.
8fb887e
to
29bd1bb
Compare
29bd1bb
to
ccfde89
Compare
Updated |
if_none_match == context.response.headers["Etag"] | ||
if if_none_match = context.request.if_none_match | ||
match = {"*", context.response.headers["Etag"]} | ||
if_none_match.any? { |etag| match.includes?(etag) } |
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.
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.
That's different
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.
Ouch, my bad (I read it other way around), you're right.
Merge? |
need another review and bcardiff is on holiday for the next 3 weeks. |
There are two approvals from core members... why another one? |
Because the PR has had extra commits since then... As soon as you add extra code, all reviews are invalidated, anything otherwise would be silly. |
Approved :-P (someone probably needs to add me as a core team member again, every two or three months I get tired of the language and remove myself, though I'm trying to manage it better now) |
Oh, yes, that's true. My bad. @sdogruyol care for another look? |
LGTM 👍 Thank you @straight-shoota 🙏 |
This enhances Etag supported (which as added in #6145) to recognize multiple values in the
If-None-Match
header according to RFC 2732.The header field parser is reusable outside
StaticFileHandler
asHeaders#if_none_match
andHeaders#if_match
.