Adds the Allow header on 405 response #776
Conversation
|
@pkieltyka Any news about this ? |
|
Why is this not merged? Is it still pending on a PR review or is it something else? |
|
Any updates regarding this matter? |
There was a problem hiding this comment.
Hi,
I think the chi router itself should be able to set the Allow header along with the HTTP 405 responses. If we do this only on HTTP 405s, we wouldn't introduce any performance impact in the "happy path" unlike this middleware.
However, I think you've done a great job on this middleware. Could you share it as a standalone package with the community?
Best,
Vojtech
Makes chi compliant with rfc9110 (cf https://httpwg.org/specs/rfc9110.html#field.allow) Currently this behaviour is not supported, and it adds a small but non-negligable performance offset. That's why it is proposed as a middleware
|
I really believe that it is possible to add this important feature without performance impact, and I am actively working on it. I have a working proposal that has no (really 0ms/0byte) performance impact on the happy path for the default I am refactoring it to try to keep the "chi" spirit (simple, light, idiomatic and maintainable) so that it is easy to review and rework later. Thank you for your review and comment. All routes are unaffected, except the route trigerring a 405 (the
|
EwenQuim
changed the title
| func (mx *Mux) MethodNotAllowedHandler() http.HandlerFunc { | ||
| func (mx *Mux) MethodNotAllowedHandler(methodsAllowed ...methodTyp) http.HandlerFunc { | ||
| if mx.methodNotAllowedHandler != nil { | ||
| return mx.methodNotAllowedHandler | ||
| } | ||
| return methodNotAllowedHandler | ||
| return methodNotAllowedHandler(methodsAllowed...) |
There was a problem hiding this comment.
Unfortunately I cannot pass the Allowed Methods to the (Mux).methodNotAllowedHandler given by the user directly in the Mux with the MethodNotAllowed() method, as it would break the current API.
It only set the Allow header when using default handler. But I think it is enough anyway, at least for this PR.
We might want to give the possibility for users to implement more easily this feature in their own handler by making the (Context).methodsAllowed field public. Is it worth it? It's up to you.
|
Adding RFC 9110 compliance sounds like a good idea. 👍 |
There was a problem hiding this comment.
This looks so much better compared to the previous middleware solution. Good job!
@pkieltyka This PR sets Allow header on HTTP 405 responses (which makes chi compliant with RFC 9110) and doesn't affect the performance of the "happy path". Looks pretty good to me. Mind having a look?
|
Seems really useful as this is required by the RFC 😄 |
|
Intent looks good, code looks good, perf look good. 😁 I hope it will be merged soon! |
|
Hey, @VojtechVitek! I was wondering if you had any updates on @pkieltyka? In case he's tied up or not available, I was thinking we could explore alternative ways to merge this feature. Seems like it could be good for a lot of people! |
|
Do we have docs on how to use this? |
|
thanks for the contribution @EwenQuim and to the reviewers (@VojtechVitek + others) merged + released https://github.com/go-chi/chi/releases/tag/v5.0.10 |
@thexpand it works "out of the box", it automatically sets the Allow header in case of a 405, so no docs are required :) |
By adding the
Allowheader with the lists of available methods on405 Method Not Founderror, this PR makes chi compliant with rfc9110, because this behaviour is not currently supported in Chi.Note that it also make chi look better on this article (that will need to be updated). This article is the first to appear when searching on google "golang choose router".
Closes #446.
Don't hesitate to ask for changes. Thanks for this amazing package :)
EDIT : Now this PR does not inflicts any performance issues on the happy path (all routes except in case of 405). It simply does not touch any code that is not a 405. See this comment to get performance details.