HeadRequestHandler: run GET handler and don't return the body #655
Conversation
|
Thanks a lot for the PR @compumike 🙏 I think this is a good approach 👍 |
| @@ -34,6 +32,11 @@ module Kemal | |||
|
|
|||
| route = @routes.find(lookup_path) | |||
|
|
|||
| if verb == "HEAD" && !route.found? | |||
| # On HEAD requests, implicitly fallback to running the GET handler. | |||
| route = @routes.find(radix_path("GET", path)) | |||
There was a problem hiding this comment.
This looks fine, but I'm wondering if there could be a more sophisticated approach...
Would it be feasible to make this dependent on whether there's a HeadRequestHandler in the handler chain?
Or, potentially only insert that handler in this case to avoid having to check for GET vs HEAD twice?
There was a problem hiding this comment.
Is the concern about increasing the requests-per-second performance of HEAD requests by avoiding multiple @routes.find calls? If so, note that the current approach already gets cached in Kemal's @cached_routes.
My goal with this approach was:
- make it always "do the right thing" by default, and
- allow the user to specify a custom HEAD handler if they wanted to override this behavior. (example:
Kemal::RouteHandler::INSTANCE.add_route("HEAD", "/") { "override HEAD behavior" })
The relevant part of RFC 7231 is:
The HEAD method is identical to GET except that the server MUST NOT send a message body in the response (i.e., the response terminates at the end of the header section). The server SHOULD send the same header fields in response to a HEAD request as it would have sent if the request had been a GET, except that the payload header fields MAY be omitted. This method can be used for obtaining metadata about the selected representation without transferring the representation data and is often used for testing hypertext links for validity, accessibility, and recent modification.
Based on that, I assume basically everyone would want to have a HeadRequestHandler and this change to RouteHandler, and the only reason to override would be to provide a more-performant but identical-headers-result implementation if that was really desired. My use case and the other use case described in the issue #614 are about external systems doing infrequent automated health checks of a Kemal-powered server process, and the external system makes the assumption that the HEAD will execute the same underlying code as the GET would have.
It might be possible to make the RouteHandler fallback be dependent on checking for a HeadRequestHandler in the handler chain, possibly checking some new property Kemal.config.head_request_handler_enabled. But even if we do that, I think it makes sense to have this behavior enabled by default, so I'm unsure if the complexity of configuring and checking is worthwhile.
There was a problem hiding this comment.
Checking other frameworks, reading the RFC and the excepted behavior, I agree with @compumike that this behavior should be the default. We could have a opt-in config for later.
See discussion at #614