Do not reject UNKNOWN HttpMethod #6172
Conversation
Also makes Tomcat use the METHOD header. Jetty already uses the METHOD header.
AlexProgrammerDE
requested review from
ikhoon,
jrhee17,
minwoox and
trustin
as code owners
🔍 Build Scan® (commit: 59b48a1) |
jrhee17
left a comment
jrhee17
left a comment
There was a problem hiding this comment.
Direction looks good.
I'm not sure if you want to handle here as well, but client-side won't be able to send custom HTTP methods. We can keep this as a known issue and proceed with server-side first though
| // Set the method. | ||
| final HttpMethod method = req.method(); | ||
| coyoteReq.method().setString(method.name()); | ||
| coyoteReq.method().setString(req.headers().get(HttpHeaderNames.METHOD)); |
There was a problem hiding this comment.
Can you also add a integration test which validates the server can successfully process the custom http method?
jrhee17
left a comment
jrhee17
left a comment
There was a problem hiding this comment.
Changes look good to me once the CI is fixed 👍
One more note is the current approach is more of a workaround due to breaking changes - later we may allow registration of custom HttpMethods in a type-safe way when we bump the major version
| * | ||
| * @param additionalAllowedHttpMethods the additional allowed HTTP methods | ||
| */ | ||
| public ServerBuilder additionalAllowedHttpMethods(String... additionalAllowedHttpMethods) { |
There was a problem hiding this comment.
optional) for consistency with other APIs, it may be worth adding a additionalAllowedHttpMethods(Iterable<String>) variant
| // Set the method. | ||
| final HttpMethod method = req.method(); | ||
| coyoteReq.method().setString(method.name()); | ||
| coyoteReq.method().setString(req.headers().get(HttpHeaderNames.METHOD)); |
There was a problem hiding this comment.
note: I think it's fine to support just tomcat for now, and add support for jetty later if needed/reported
|
Gentle ping. We could also finish this implementation if you are busy @AlexProgrammerDE |
|
Hi, university started for me, so I'm a little more busy than usual. I'd appreciate if the team could finish this. |
4 participants
Motivation:
Adds unknown HTTP Methods from other RFCs.
I need this feature because WebDav extends the list of HTTP Method names and
Http1RequestDecoderrejects unknown method names.More information in the Discord: https://canary.discord.com/channels/1087271586832318494/1087272728177942629/1351931184451686420
Modifications:
Result: