Enhance header name validation exception message #2924
Conversation
|
Lmk what approach you like more and I will adjust tests as needed. |
idelpivnitskiy
changed the title
|
Both approaches would be fine with me but if I had to pick this is my preference since it gives the index of the offending char. |
Motivation: Provide more details about a illegal header name. Modifications: - Use `TokenValidatingProcessor` that captures more context for generating an exception message, if necessary; Result: Users can see what header name has an illegal character as well as the position (index) of an illegal character.
idelpivnitskiy
force-pushed
the
headerName2
branch
from
20c5347
to
fd39ba7
Compare
idelpivnitskiy
force-pushed
the
headerName2
branch
from
fd39ba7
to
cb005bf
Compare
| * DefaultHttpHeadersNameValidationBenchmark.addHeader 8 false thrpt 5 5024017.665 ± 32830.519 ops/s | ||
| * DefaultHttpHeadersNameValidationBenchmark.addHeader 8 true thrpt 5 135011.887 ± 1832.187 ops/s | ||
| * DefaultHttpHeadersNameValidationBenchmark.addHeader 16 false thrpt 5 2523554.519 ± 8780.660 ops/s | ||
| * DefaultHttpHeadersNameValidationBenchmark.addHeader 16 true thrpt 5 68474.840 ± 1154.751 ops/s |
There was a problem hiding this comment.
I added a benchmark and it seems like performance of the "happy-path" was not affected after switching from a method reference to an inner class with extra state. Likely there will be a bit more GC pressure, but overall seems acceptable.
The invalid header validation is expected to become slower because after changes it generates more descriptive exception message.
| private static int stringSize(final int value) { | ||
| assert value >= 0; | ||
| int size = 0; | ||
| int tmp = 1; | ||
| while (tmp <= value && tmp <= 1_000_000_000) { | ||
| ++size; | ||
| tmp = (tmp << 3) + (tmp << 1); | ||
| } | ||
| return size; | ||
| } |
There was a problem hiding this comment.
At a glance it's not obvious what this is doing. What if you just gave it 6 bytes and let it resize in the exceedingly uncommon situation that we are wrong?
Alternatively, if you really want to count digits, would something like this be more readable?
int size = 1;
int test = 10;
while (test <= value && test <= 1_000_000_000) {
size++;
test *= 10;
}
There was a problem hiding this comment.
Updated mehod name, added javadoc, renamed local variables, used multiplication
idelpivnitskiy
changed the title
Motivation:
Provide more details about a illegal header name.
Modifications:
TokenValidatingProcessorthat captures more context for generating an exception message, if necessary;Result:
Users can see what header name has an illegal character as well as the position (index) of an illegal character.
This approach captures more context and doesn't use another try-catch with wrapping, but allocates a new lambda for every token validation.
When users add/set illegal header name:
before
after
When an illegal header name is received over network:
before
after