Add MethodMiddleware #366
Add MethodMiddleware #366
Conversation
|
Thanks @fharding1 - can you add tests to this as well? |
|
@elithrar 👍 done. |
|
bump @elithrar |
|
Thanks for the reminder. On my list for this week! |
|
The usage of this seems a bit confusing to me. The idea is that the router will use itself as middleware? |
|
Yeah, because you might only want to apply the middleware to certain routes. I'm not super familiar with mux beyond using it in personal projects with fairly normal use cases, would there be a better way to do this that you would recommend? @kisielk |
|
Since it doesn't depend on any unexported fields of the router, I don't think it needs to be a method of the router itself. It could be a new middleware function or type that takes the router as an argument. That way we don't need to grow the already large API of the Router type. |
|
With the way it’s been implemented, it doesn’t need to be on the router at all. I had assumed it would be similar to NotFoundHandler, where it is a field on the Router, or a toggle (Boolean). I don’t feel strongly either way. |
|
Alright, I've removed it from |
middleware.go
Outdated
| // on a request, by matching routes based only on paths. It also handles | ||
| // OPTIONS requests, by settings Access-Control-Allow-Methods, and then | ||
| // returning without calling the next http handler. | ||
| func MethodMiddleware(r *Router) MiddlewareFunc { |
There was a problem hiding this comment.
Two parts:
- I think the function name could be more descriptive here:
CORSMethodMiddleware- something to indicate that it applies to a CORS-driven use-case. - Do we want to build the routes on every request? (may not be avoidable to avoid stale data, but thinking out loud here)
There was a problem hiding this comment.
I agree with the first part. As for building the routes on every request, I think it would be fairly difficult to do, but I don't know the mux architecture super well either. I can't think of any clean solution that keeps our data fresh. It would be nice to not have to walk the router on each request though of course, so I'm open to any ideas!
mux_test.go
Outdated
| @@ -2315,6 +2315,14 @@ func stringMapEqual(m1, m2 map[string]string) bool { | |||
| return true | |||
| } | |||
|
|
|||
| // stringHandler returns a handler func that writes a message 's' to the http | |||
| // response writer. | |||
There was a problem hiding this comment.
"to the http.ResponseWriter" or just "to the response".
middleware.go
Outdated
|
|
||
| // CORSMethodMiddleware returns a MiddlewareFunc intended for setting the CORS | ||
| // Method header, Access-Control-Allow-Methods. It sets Access-Control-Allow-Methods | ||
| // on a request, by matching routes based only on paths. It also handles |
There was a problem hiding this comment.
Extra space between "a" and "request"
middleware.go
Outdated
| @@ -28,3 +31,43 @@ func (r *Router) Use(mwf ...MiddlewareFunc) { | |||
| func (r *Router) useInterface(mw middleware) { | |||
| r.middlewares = append(r.middlewares, mw) | |||
| } | |||
|
|
|||
| // CORSMethodMiddleware returns a MiddlewareFunc intended for setting the CORS | |||
| // Method header, Access-Control-Allow-Methods. It sets Access-Control-Allow-Methods | |||
There was a problem hiding this comment.
"Sets the Access-Control-Allow-Methods response header..."
|
Fixed! Thank you for maintaining gorilla/mux and helping me add this feature! |
|
Thanks for the contribution. Apologies for the slow responses. Right in the middle of changing jobs, so have been extremely busy! |
Implements the behavior discussed in #353 with @elithrar. I'm not sure if this is the best way to only match based on path only.