-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Allow
header support in Router, MethodNotFoundHandler (405) and CORS middleware
#2039
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2039 +/- ##
==========================================
+ Coverage 91.32% 91.57% +0.24%
==========================================
Files 33 33
Lines 2871 2919 +48
==========================================
+ Hits 2622 2673 +51
+ Misses 159 157 -2
+ Partials 90 89 -1
Continue to review full report at Codecov.
|
@lammel when you have time. please review. |
Working on it... so many lines to review ;-)
Martti T. ***@***.***> schrieb am So., 5. Dez. 2021, 18:35:
… @lammel <https://github.com/lammel> when you have time. please review.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2039 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAKVHUTUACCGXVWWVWZSTTUPOPFPANCNFSM5JL7GEXQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't have time to run or check on the tests yet, but code is mostly reviewed now an LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine piece of coding work. LGTM!
This PR adds support for
Allow
header toAllow
header lists all method types registered for given routed url path.Related RFCs:
Allow
RFC https://datatracker.ietf.org/doc/html/rfc7231#section-7.4.1 all 405 should haveAllow
header listing all methods that router has registered for given path.Allow
is optional but useful header to haveImplementation notes:
next(c)
and hope to meet our router optionshandler we probably not succeed because of different kinds of authentication middlewares (ala JWT or BasicAuth or KeyAuth) which check for stuff like that.Allow
value to contextecho.ContextKeyHeaderAllow
is because default 405 handler needs to be able to access that value and possibly CORS middleware is interested in that value. Asecho.MethodNotAllowedHandler
is part of public API we can not change how that method is created due backwards compatibility.optionsMethodHandler
is kept private as it is router specific implementation detail. If you want your own then you can add them withe.OPTIONS()
method for paths you choose.Using context values in Echo core is so far unprecedented and potentially controversial decision. I did not want to introduce new field into
echo.context
struct as it is quite specific case and I know that Go standard libraryhttp.Server
is usingcontext.Context
to add some info to each request context - so it is not so unheard of but probably should be avoided:See:
Let's have a discussion