Make request and response header names lower-case #96
Conversation
|
@rouzwawi @ccarpita @mattnworb @zalenski opinions? |
| .filter(HeadersValue::hasNonLowerCase) | ||
| .findAny() | ||
| .map(lowerCaseKeys(headers)) | ||
| .orElse(headers); |
There was a problem hiding this comment.
this stream operation seems confusing to me:
- it would seem to filter out any keys that are all lower-case
- and the map operation ignores the input (the
sins ->)
is the goal to avoid modifying the map in the happy case when the keys are already all lower case? since you have to iterate over all the keys anyway, it seems like it there wouldn't be any real work saved
There was a problem hiding this comment.
I mean, I see in the tests that it works, it was just confusing as to how it works until I ran the tests manually to see 😄
|
Overall seems sensible to me, although I don't have much of a dog in this fight 👍 |
pettermahlen
force-pushed
the
case-insensitive-headers
branch
from
820f0c0
to
885eaa0
Compare
|
The failed build appears to be due to a problem with codecov.io. They seem to be having problems at the moment, according to: http://status.codecov.io/, which says there are intermittent failures due to a systems upgrade since a couple of days. |
|
@mattnworb thanks for the feedback, i've tried to clarify the implementation a bit - better? |
|
I did happen to have a dog in this fight, and am pleased with the results :) FWIW the updated lowercase-checking implementation seemed clear to me at first read. |
|
@pettermahlen 👍 on improved clarity! |
This change introduces a Headers type so as to break backwards compatibility and trigger compiler errors if the Request.headers() or Response.headers() methods are called expecting a Map as the return value. Header names are lower-cased, to simplify case comparisons and be compatible with HTTP/2. A part of the reasoning around making this simplistic change is that lower-casing is very cheap for a string that is already lower-case, and thus doing the lower- casing always will only lead to a performance hit the first time a header is processed. Headers that are propagated internally will always be lower case when handled by downstream services.
pettermahlen
force-pushed
the
case-insensitive-headers
branch
from
6d3ca0e
to
86534a7
Compare
Current coverage is 66.56%@@ master #96 diff @@
==========================================
Files 117 119 +2
Lines 2443 2461 +18
Methods 0 0
Messages 0 0
Branches 207 212 +5
==========================================
+ Hits 1620 1638 +18
Misses 780 780
Partials 43 43 |
This change introduces a Headers type so as to break backwards
compatibility and trigger compiler errors if the Request.headers()
or Response.headers() methods are called expecting a Map as the
return value.
Header names are lower-cased, to simplify case comparisons and be
compatible with HTTP/2. A part of the reasoning around making
this simplistic change is that lower-casing is very cheap for
a string that is already lower-case, and thus doing the lower-
casing always will only lead to a performance hit the first time
a header is processed. Headers that are propagated internally
will always be lower case when handled by downstream services.
This is an alternative solution to #93.