Verify header field names#191
Conversation
| // | ||
| // The presence of a message-body in a request is signaled by the inclusion of a | ||
| // Content-Length or Transfer-Encoding header field in the request's message-headers. | ||
| return |
There was a problem hiding this comment.
RFC 2616 is dead, let's stop citing it. It's RFC 7230 section 3.3. However, this change is not cautious enough: if there's no body, but the request verb is POST, "Content-Length: 0" is mandatory. Content-Length: 0 is also harmless on all request types that do not define a body. A patch that users this escape hatch on only GET, HEAD, DELETE, CONNECT, and TRACE would be acceptable though, so long as it sends C-L 0 everywhere else.
Lukasa
left a comment
Lukasa
left a comment
There was a problem hiding this comment.
Love it. If we fix up the compile errors this should be good to go.
|
@Lukasa Compile errors are fixed. There are some failing tests though (nothing I touched). |
|
@fabianfett it seems that most tests fail due to change in behaviour, |
|
@artemredkin ow shoot. fixed. |
|
awesome, thanks! there is one failing test, but this one is flaky (#175), I'll try to run tests again |
|
@swift-server-bot test this please |
|
@fabianfett From what I can tell, we're good but would you mind checking if we have a test case which covers this #146 ? If yes, we're all good, if no we might want to add it here? |
|
@swift-server-bot test this please |
2 similar comments
|
@swift-server-bot test this please |
|
@swift-server-bot test this please |
|
thank you @fabianfett ! |
4 participants
This pr builds on top of #189 and will need a rebase once the former is merged.
Motivation
Having spent about four hours today debugging AsyncHTTPClient just to find out I had a whitespace instead of a dash in my http header field name, I decided I want to save somebody else's time – probably future me.
User AgentvsUser-AgentChanges
HTTPHeaders.validateFieldNames() throws.invalidHeaderFieldNames([String])Result
Hours of time saved. Karma for me.