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.
I don't think this parsing method belongs here.
There was a problem hiding this comment.
Yeah, I wasn't sure whether this should go here or in Request. I'll move it to the latter.
straight-shoota
force-pushed
the
jm/feature/etag-multi
branch
from
8fb887e
to
29bd1bb
Compare
straight-shoota
force-pushed
the
jm/feature/etag-multi
branch
from
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.
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-Matchheader according to RFC 2732.The header field parser is reusable outside
StaticFileHandlerasHeaders#if_none_matchandHeaders#if_match.