-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Add middleware support as discussed in #293 #294
Conversation
Status update: I've rebased As this commit list is getting huge, I'll squash them after you review individual changes. |
README.md
Outdated
### Middleware | ||
|
||
|
||
Since **vX.Y.Z**, mux supports the addition of middlewares to a router, which are executed if a |
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.
Replace "router" with [Router](https://godoc.org/github.com/gorilla/mux#Router)
so that we link to the docs.
README.md
Outdated
|
||
|
||
Since **vX.Y.Z**, mux supports the addition of middlewares to a router, which are executed if a | ||
match is found (including subrouters). Middlewares are defined using the de facto standard function: |
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.
s/function/type
README.md
Outdated
func(http.Handler) http.Handler | ||
``` | ||
|
||
Structs that implement the `Middleware` interface can also be used: |
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.
s/Structs/Types
README.md
Outdated
} | ||
``` | ||
|
||
Middlewares can be added to a router using `Router.AddMiddlewareFunc()` and |
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.
Examples for both please!
README.md
Outdated
Structs that implement the `Middleware` interface can also be used: | ||
|
||
```go | ||
type Middleware interface { |
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.
I'm not entirely convinced this interface is useful. Can you expand more on how this is a better API vs. only supporting func(http.Handler) http.Handler
- ?
Thanks for the comments. You're right about the docs, its a bit lacking. I'll add extra examples and fix the issues you pointed out over the course of this week, I'll be pretty busy at work until Wed :( |
I've fixed some of the issues you pointed out in the commit above. Please tell me if there are something else which needs improvement. |
Do you have examples? I'm partial to chi, which has a For context: adding two methods is 100% more decision-making that a package user has to make, and adds to our already extensive package API. Thus, I'm pushing hard for utmost simplicity unless there's a clear case where the interface-based support makes sense. I can't see that it makes sense right now. |
There is an example on https://github.com/roobre/gorilla-mux/blob/middleware/README.md#middleware, line 427: // Define our struct
type authenticationMiddleware struct {
tokenUsers map[string]string
}
// Initialize it somewhere
func (amw *authenticationMiddleware) Populate() {
amw.tokenUsers["00000000"] = "user0"
amw.tokenUsers["aaaaaaaa"] = "userA"
amw.tokenUsers["05f717e5"] = "randomUser"
amw.tokenUsers["deadbeef"] = "user0"
}
// Middleware function, which will be called for each request
func (amw *authenticationMiddleware) Middleware(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
token := r.Header.Get("X-Session-Token")
if user, found := amw.tokenUsers[token]; found {
// We found the token in our map
log.Printf("Authenticated user %s\n", user)
next.ServeHTTP(w, r)
} else {
http.Error(w, "Forbidden", 403)
}
})
} r := mux.NewRouter()
r.HandleFunc("/", handler)
amw := authenticationMiddleware{}
amw.Populate()
r.AddMiddleware(&amw) Without implementing this interface, instead of It's just the same as The extra decision making is indeed here, but it is for a reason: You either want a simple middleware which, for example, logs requests, or a complex one which can hold data structures, have extra logic, etc. Just like it happens with handlers. The package main
import (
"net/http"
"github.com/gorilla/mux"
"roob.re/loggermiddleware"
)
// [...]
func main() {
mux := mux.NewRouter()
rrmw := &randomRejectMiddleware{}
lgmw := loggermiddleware.New(log)
mux.AddMiddleware(rrmw)
mux.AddMiddleware(lgmw)
// [...]
} This looks simple enough. Without the middleware interface, we could have called our package main
import (
"net/http"
"github.com/gorilla/mux"
"roob.re/loggermiddleware"
)
// [...]
func main() {
mux := mux.NewRouter()
rrmw := &randomRejectMiddleware{}
lgmw := loggermiddleware.New(log)
mux.AddMiddleware(rrmw.DoMiddleware)
mux.AddMiddleware(lgmw.MiddlewareLogic)
// [...]
} I know the names are quite absurd, but you get the point. The middleware interface forces an standard middleware writes can adhere to, and makes the code to add them look simpler and nicer. Just like |
Any progress on this? Would love to use this for a new project ;) |
Still needs review, and I’m just getting back from vacation. Soon!
…On Sat, Nov 18, 2017 at 2:49 PM Ad van der Veer ***@***.***> wrote:
Any progress on this? Would love to use this for a new project ;)
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#294 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AABIcH6mB6pICNIIx3a5cqCiw0s7sdS1ks5s3zTfgaJpZM4PTaEY>
.
|
I like this feature, too! |
any status update so far? |
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.
Minor changes. Let's start with a smaller API for this first. I'm otherwise happy with the tests + impl.
middleware.go
Outdated
|
||
import "net/http" | ||
|
||
type Middleware interface { |
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.
I don't think we should keep this. Single method interfaces can be restrictive; best to let the package user define a wrapper if needed. We end up adding similarly-named methods to the API.
(if there is demand over time, we can re-assess)
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.
I still think that adding the Middleware
interface will help to keep middlewares more standard, and the parallelism with net/http
Handler
and HandlerFunc
helps users to understand the behaviour. This pattern is also seen in other routers and middleware managers (see negroni, for example).
However, if you think it doesn't fit the project, I'll remove it.
doc.go
Outdated
}) | ||
} | ||
|
||
Note: The handler chain will be stopped if your middleware doesn't call `next.ServeHTTP()` |
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.
We should document/clarify how handlers are executed (in order they added).
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.
Roger, I'll clarify the calling order and stopping condition.
@kisielk - let me know if you echo my thoughts here. |
I'll address the changes listed above when I return from vacation. I'm sorry for the inconvenience :( Btw, I'll probably rebase master and squash some (or all) of the PR commits, as the list seems to be getting too big. |
No inconvenience at all!
…On Mon, Dec 11, 2017 at 4:59 AM Roberto Santalla ***@***.***> wrote:
I'll address the changes listed above when I return from vacation. I'm
sorry for the inconvenience :(
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#294 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AABIcF9qAba43ejVIg0EZcDxPGXHhsZMks5s_ScwgaJpZM4PTaEY>
.
|
@roobre - let me know if you need any help here. Would love to help get this merged in! 🙌🏼 |
Sorry, I'm on vacation until maybe 3-4 Jan, so I've been quite inactive here :_ I'll get this rebased and fixed as soon as I get home. But I'd like to see if you have reached a consensus about the |
Hm, I'm not sure how I feel about the interface. What would the alternative look like? |
Without the interface, we should just have (as the public API):
The interface we rally around is There may be *some desire for a
... but I'd rather tease that out by starting minimally. |
Ah I understand now. Yes I think just using |
@kisielk It's not exactly as you said. The interface just wraps |
Yeah, I understand that, but having the
Then you don't need to create additional type boilerplate. At this point the interface would be created solely for the purpose of calling one method on Mux, which seems like it's unnecessary. |
like |
Rebased to latest commits on Note: I've also squashed the whole history to make rebase easier from now (it started giving conflicts on re-edited files and it is a bit of a pain). |
Middleware now uses de facto implementation Signed-off-by: Roberto Santalla <roobre@roobre.es> Middlewares are now executed in the same order they're added Signed-off-by: Roberto Santalla <roobre@roobre.es> Moved middleware chain build process from ServeHTTP() to Match() This allows middleware to be executed even when it's not Router who calls the handler directly (e.g. subrouters) Signed-off-by: Roberto Santalla <roobre@roobre.es> Added some tests for middleware functionality go < 1.6 doesn't like http.Method* constants Added more tests to check if middleware is executed correctly Added middleware documentation Added check to execute middleware only if no errors were found Fixed middleware test for custom 404 handler Added middleware tests for subrouters, improve and reorder existing ones Improved docs on README mux.Middleware type is now private (mux.middleware) Rewritten README.md docs, added a few more comments to middleware.go Added Middleware section to index
Good to review?
…On Wed, Jan 10, 2018 at 2:38 AM Roberto Santalla ***@***.***> wrote:
Rebased to latest commits on master.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#294 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AABIcB5KHrRtnDw4V6yspm9PjzjlKu1Oks5tJJMugaJpZM4PTaEY>
.
|
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.
LGTM: almost there @roobre! Just some doc changes and a method naming recommendation.
@@ -447,6 +448,82 @@ func main() { | |||
} | |||
``` | |||
|
|||
### Middleware | |||
|
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.
- Add a quick paragraph describing what middleware is / what it can be used for, and link to https://github.com/gorilla/handlers
- Version will be v1.6.1 if we merge this week (I'll cut a new release after this PR is merged in)
e.g.
Middleware describes intermediate handlers that can check, mutate or log the incoming request or outgoing response. mux makes it easy to add middleware to a Router or Subrouter...
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.
Added!
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.
Btw, I forgot to mention I did not add the link because most handler don't actually implement mux.Middleware
, as they require additional parameters. We could circunvent this using closures, but I think it would confuse some users instead of adding clarity. Maybe we could rework some handlers to make them comply with the interface?
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.
Maybe instead of changing them we could add a XXXXMiddleware
function for each one?
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.
Yes, of course. We should not break existing handlers, so maybe wrapping them somehow, or refactoring the core logic into a different, private function and exposing both the actual and the Middleware flavours of it. I can give it a shot if you think it's worth it.
README.md
Outdated
r.AddMiddlewareFunc(amw.Middleware) | ||
``` | ||
|
||
Note: The handler chain will be stopped if your middleware doesn't call `next.ServeHTTP()` with the corresponding parameters. This can be used to abort a request if the middleware writer wants to. |
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.
"This can be used to avoid passing the request on, but make sure to return a response to the client from within the middleware"
Note: a library of useful logging, CORS and proxy-related middleware can be found in the gorilla/handlers package
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.
Added, but reworded "make sure" to "should", as some particular scenarios may apply and it is not a mux requirement.
@@ -238,5 +238,70 @@ as well: | |||
url, err := r.Get("article").URL("subdomain", "news", | |||
"category", "technology", | |||
"id", "42") | |||
|
|||
Since **vX.Y.Z**, mux supports the addition of middlewares to a [Router](https://godoc.org/github.com/gorilla/mux#Router), which are executed if a |
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.
Update to align with the README
(Note to self: use tooling to make this automatic...)
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.
I removed the version thingie. I'm not sure if its an important enough information to be in the readme. Feel free to re-add it if you think it is.
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.
Good move. I think it's fair to assume that the documentation reflects HEAD
.
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.
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.
Yikes, I totally missed that one :/
I can fix it and create a new PR if you want, but maybe you can edit it quicker @elithrar
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.
I’ll take a look today. The website doesn’t do Markdown, which is a bit of a pain.
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.
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.
Cool, thanks guys :)
middleware.go
Outdated
Middleware(handler http.Handler) http.Handler | ||
} | ||
|
||
// MiddlewareFunc also implements the Middleware interface. |
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.
Update this? As we don't export the Middleware
interface (middleware
) this may be confusing.
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.
Do you mean the comment or the implementation?
The implementation needs to be there in order to store MiddlewareFunc
as Middleware
in the middlewares
slice. But the comment needs to be updated indeed, I slipped there :P
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.
Just the comment 👌🏼
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.
Done!
middleware.go
Outdated
} | ||
|
||
// AddMiddlewareFunc appends a MiddlewareFunc to the chain. | ||
func (r *Router) AddMiddlewareFunc(mwf MiddlewareFunc) { |
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.
AddMiddleware
, Add
or UseMiddleware
or just Use
might be less verbose here.
chi
andecho
call their methodsUse
- express also calls it
use
- Downside to
Use
- not entirely self-documenting.
I'd be OK with just calling this function Middleware
as "Add" is obvious, and subtracting it/removing it later is (mostly) a non-option.
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.
I don't like the AddMiddlewareFunc
name either, seems way too long to me. But I think we should keep the "func" part somewhere, as it makes what this function expects very clear, and avoids the need for a breaking change if the middleware
interface is exported in the future.
What about UseFunc
?
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.
Let’s just stick with Use
- as that’s familiar and aligns with the Handle
(takes a http.Handler) and HandleFunc
(takes a HandlerFunc) on Router
. This somewhat sits in the middle, but familiarity wins out in lieu of other objections.
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.
Done. I've renamed the internal addMiddleware
to useInterface
, to avoid collision with Use
.
middleware.go
Outdated
func (mw MiddlewareFunc) Middleware(handler http.Handler) http.Handler { | ||
return mw(handler) | ||
} | ||
|
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.
Middleware
adds a middleware to the chain. Middleware can be used to intercept or otherwise modify requests and/or responses, and are executed in the order that they are applied to the Router.
I'll patch this along the weekend, probably tomorrow. |
to Use{Interface,} respectively
Fixed most of the things, please tell me what you think :D |
OK - I'm going to test this in an application I have running (hopefully tonight) so we can get this merged. Excited to land this. @kisielk - if you can review in the interim that'd be great! |
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.
Looks good other than some documentation nits.
README.md
Outdated
@@ -447,6 +448,86 @@ func main() { | |||
} | |||
``` | |||
|
|||
### Middleware | |||
|
|||
Mux supports the addition of middlewares to a [Router](https://godoc.org/github.com/gorilla/mux#Router), which are executed in the order they are added if a match is found (including subrouters). |
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.
IMO it's bad style to place so many sentence fragments in parentheses. It just adds unnecessary noise to the text and obfuscate the meaning. You can almost always restructure text to not use them. In this sentence you could say "router or subrouter" instead, or add another sentence here or later in the text that says "when a match is found the request is passed to the middleware in the current router and its parents".
README.md
Outdated
### Middleware | ||
|
||
Mux supports the addition of middlewares to a [Router](https://godoc.org/github.com/gorilla/mux#Router), which are executed in the order they are added if a match is found (including subrouters). | ||
Middlewares are (typically) small pieces of code which take one request, do something with it (e.g. log some fields, add/remove some headers), and pass it down to another middleware or the final handler. |
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.
The (typically) is arguably one valid use of parentheses, but the examples could be another sentence to not interrupt the flow.
README.md
Outdated
type MiddlewareFunc func(http.Handler) http.Handler | ||
``` | ||
|
||
Typically, the returned handler is a closure which does something with the http.ResponseWriter and http.Request passed to it, and then calls the handler passed as parameter to the MiddlewareFunc (closures can access variables from the context where they are created). |
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.
Another use of parens that could be avoided.
There you go, @kisielk. I appreciate feedback on these grammar mistakes, as I'm not a native speaker I sometimes slip things here and there :( |
That one is just something that stands out because it used to be a really bad habit of mine. I used to write stuff that had parentheses in nearly every sentence because that's just how my train of thought works. Then based on feedback I realized it doesn't necessarily make for the easiest reading for other people :) |
Same here, I usually think the main points while mentally adding small exceptions and clarifications here and there, so I tend to add parenthesis and clarifications everywhere instead of rewording the whole thingk and making it more linear. Anyway, as I said, I appreciate the feedback :D |
OK: sanity checked locally and all works as expected, esp. around Subrouters. I'm going to put this in prod soon to expose some heap & goroutine profiles behind authentication, so timing is good! |
Thanks for your work on this @roobre—appreciate your responsiveness and patience! |
Tagged in v1.6.1 - https://github.com/gorilla/mux/releases/tag/v1.6.1 |
Trying to use middleware but i have this error message.
|
It should be |
@kisielk Thanks. It works now. |
See #332 for the docs fix. |
Hi, |
Middleware does indeed need to be thread-safe if it keeps state across requests, as go's http server spawns a new goroutine per request. Maybe I thought this was more obvious than it actually is, so writing it down may help some users. May I open a new PR clarificating it or do you guys prefer to handle this yourselves? |
Just go ahead, it's your baby :-) |
See discussion in #293 for more details.