Trim optional white space in CombinedHttpHeaders values #6502
Conversation
| } | ||
|
|
||
| StringBuilder result = new StringBuilder(length + CSV_NUMBER_ESCAPE_CHARACTERS); | ||
| boolean quoted = isDoubleQuote(value.charAt(start)) && isDoubleQuote(value.charAt(last)) && length != 1; |
There was a problem hiding this comment.
better make length != 1 as first condition bcz it has lower cost.
UPD: Although may be not. One-char params is not very likely..
sorry for noise.
There was a problem hiding this comment.
If quoted==true, can we increment start and decrement last to exclude check i == start || i == last from the loop?
There was a problem hiding this comment.
I honestly haven't found a way to make the code better while doing the suggested increment/decrement. There's more involved than just excluding the i == start || i == last from the loop, and I have tried to not alter too much the existing code in this PR.
@fenik17 please send a follow-up PR to implement the change you envisioned.
There was a problem hiding this comment.
Ok. I'll do it soon. Thanks for an effort!
| return start == 0 && end == length - 1 ? value : value.subSequence(start, end + 1); | ||
| } | ||
|
|
||
| private static int indexOfFirstNonOwsChar(CharSequence value, int length) { |
There was a problem hiding this comment.
IMHO, make sense add comment that if the value don't contain non-OWS chars, it returns length, but not -1.
| @@ -34,6 +34,7 @@ | |||
| public static final char LINE_FEED = '\n'; | |||
| public static final char CARRIAGE_RETURN = '\r'; | |||
| public static final char TAB = '\t'; | |||
| public static final char SPACE = ' '; | |||
There was a problem hiding this comment.
I think ' ' is easier to read...
There was a problem hiding this comment.
I just wanted to emphasize specific unicode value.
Not everything can be distinguished in appearance )
Spaces may be different: https://www.cs.tut.fi/~jkorpela/chars/spaces.html
|
@normanmaurer @Scottmitch @fenik17 Sorry for the delay in addressing the comments. I've done a few basic changes and invited @fenik17 to submit a follow-up with the changes he has in mind. |
| * according to <a href="https://tools.ietf.org/html/rfc7230#section-7">RFC-7230</a> | ||
| * @return {@link CharSequence} the escaped value if necessary, or the value unchanged | ||
| */ | ||
| public static CharSequence escapeCsv(CharSequence value, boolean trimWhiteSpace) { |
There was a problem hiding this comment.
The Cyclomatic Complexity of this method "escapeCsv" is 28 which is greater than 25 authorized. 
ddossot
force-pushed
the
6452-trim-ows-in-header-values
branch
from
e79442c
to
5b62a2c
Compare
|
Just for closure here, after discussing with @normanmaurer in meatspace, we will not port to |
|
👍
… Am 22.03.2017 um 21:05 schrieb David Dossot ***@***.***>:
Just for closure here, after discussing with @normanmaurer in meatspace, we will not port to 4.0 as the CombinedHttpHeaders feature is lacking altogether there.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Motivation:
The updated HTTP/1.x RFC allows for header values to be
CSVand separated byOWS[1].CombinedHttpHeadersshould remove thisOWSon insertion.[1] https://tools.ietf.org/html/rfc7230#section-7
Modification:
CombinedHttpHeadersdoesn't account for theOWSand returns it back to the user as part of the value.Result:
Fixes #6452