Skip to content
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.

Sign up for GitHub

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

Jump to bottom

PR for middleware support #293

Closed
roobre opened this issue Sep 8, 2017 · 45 comments
Closed

PR for middleware support #293

roobre opened this issue Sep 8, 2017 · 45 comments
Assignees
@elithrar
Labels
enhancement

Comments

@roobre
Copy link
Contributor

roobre commented Sep 8, 2017

Hello there,

I've been using gorilla/mux for a while on an internal project at my organization, and due to some requirements, I ended up forking the project and adding some extra features, such as internal support for middleware.

Yes, I am aware that negroni is a thing, and that it also provides this functionality. However, there are a few key things you can't do just chaining mux after your middleware:

  • Acting only if a route match is found: Waiting to see if a route actually matches before acting allows for more flexibility and coherent design. For example, an authentication middleware can happily return 403 if the required credentials are not supplied, and at the same time a normal 404 will be returned by mux if the route does not exist. This option does not exist if you need to process authentication headers before the request is matched.

  • Adding middleware for subrouters is simpler if it is embed inside mux.

  • It is more efficient. If your site receives heavy traffic and your middleware performs heavy tasks, you'll appreciate this kind of saving.

After pondering a bit, I decided that this simple addon was worth implementing, so I did.

As a middleware needs to be able to stop the handlers chain, I implemented it using a slightly modified http.Handler interface:

type Middleware interface {
	ServeHTTP(http.ResponseWriter, *http.Request) (http.ResponseWriter, *http.Request)
}

type MiddlewareFunc func(http.ResponseWriter, *http.Request) (http.ResponseWriter, *http.Request)

If the middleware implementations return nil as either http.ResponseWriter or *http.Request the chain is stopped. Also, http.ResponseWriter and *http.Request can be hijacked if the middleware wants to. This is an approach slightly different to negroni's, where a third parameter of *http.HandlerFunc is added. I thought this way were better as it simplifies greatly building and iterating the middleware chain. Also, I don't like recursion. Of course, wrapper functions which receive http.Handler and http.HandlerFunc exists, so standard handlers can be chained too.

I'm not sure if you think this feature is kind of out-of scope. But if you don't, I'll happily open a PR and discuss any modifications or style changes you'd want to be done.
@abdullah2993
Copy link

abdullah2993 commented Sep 11, 2017 •
edited
Loading

I second this proposal. it will actually be very useful...I'm currently working on an authentication middleware so I wanted to add the ability to allow certain routes without authentication but can't because mux.CurrentRoute returns nil if not called from the the handler it self
@roobre IMO you are making it more complex then it has to be. something like this is fine IMO

@abdullah2993
Copy link

abdullah2993 commented Sep 11, 2017 •
edited
Loading

Can I send a pull request with something like this
Diff: master...Abdullah2993:master
commit: https://github.com/Abdullah2993/mux/commit/5297e1495880923aed73b18911ad4ce6f4e27472

@elithrar
Copy link
Contributor

elithrar commented Sep 11, 2017 via email

@roobre
Copy link
Contributor Author

roobre commented Sep 11, 2017

My fork actually looks like this:

master...roobre:middleware

As I said, I feel that returning the responseWriter and request seems better (to me at least) than adding a third parameter like Negroni does. Other options are, of course, open to discussion. However, please take into account that middleware writers would value the ability to hijack these two objects, so a modification to the http.Handler interface is required.

@roobre roobre mentioned this issue Sep 11, 2017
Merged
@elithrar
Copy link
Contributor

elithrar commented Sep 11, 2017 via email

@roobre
Copy link
Contributor Author

roobre commented Sep 11, 2017 •
edited
Loading

Also, http.ResponseWriter and *http.Request can be hijacked if the middleware wants to.

Basically your middleware might want to create a new Request or ResponseWriter, and pass them down to the rest of the chain. Think about returning Request.WithContext(), for example. Or maybe creating a custom ResponseWriter implementation.

@abdullah2993
Copy link

Basically your middleware might want to create a new Request or ResponseWriter, and pass them down to the rest of the chain. Think about returning Request.WithContext(), for example. Or maybe creating a custom ResponseWriter implementation.

@roobre this can be done with the classical func(http.Handler)http.Handler

@roobre
Copy link
Contributor Author

roobre commented Sep 12, 2017

Now it's me who is probably missing something. How can you create a middleware chain using that interface?

Which http.Handler is passed and what meaning have the returned http.Handler? The purpose of middleware is to operate on the ResponseWriter and Request objects, not on handlers themselves.

@elithrar
Copy link
Contributor

elithrar commented Sep 12, 2017 via email

@roobre
Copy link
Contributor Author

roobre commented Sep 12, 2017

Thanks for the info :D

It certainly looks good, I'll take a look at it and change the implementation if no functionality is lost.

@elithrar
Copy link
Contributor

elithrar commented Sep 12, 2017 via email

@roobre
Copy link
Contributor Author

roobre commented Sep 12, 2017

Then that is not the same approach I'm proposing here. There are plenty of tools which can be used to chain middleware before (and after, I guess) mux, but they get executed wether there is a route match or not. The point of adding middleware support inside mux was to circunvent this limitation, allowing middlewares to perform expensive tasks only if the endpoint is correct, or modifying the http status code only if a route is found (particularly useful for authentication middlewares).

@elithrar
Copy link
Contributor

Can you share a design proposal for this? Take a look at how other routers are doing this, as well as the previous PRs/Issues raised here for background.

@elithrar elithrar self-assigned this Sep 19, 2017
@roobre
Copy link
Contributor Author

roobre commented Sep 19, 2017

I've looked at Negroni and Martini, each having his own approach. I'll look at a couple more and add a few drawings to explain my point better. I'm not sure when I'll be able to do this, this is being a tough week at work. Stay tuned :D

@roobre
Copy link
Contributor Author

roobre commented Sep 28, 2017

Key differences bewteen external and embedded middleware

Hello again. I finally have some time to explain this with more detail. I've (hand-)drawn a diagram to better illustrate the difference between the external middleware approach, which is what you are mentioning, and mine:

2017-09-28-125450_1920x1030_scrot

As i've said, middleware embedded in mux is evaluated only if a route matches. This saves processing time for non-existing routes, and allows writing of coherent authentication middleware: It will return 404 if no route is found, and 403 if a route matches but the middleware doesn't like it).

Regarding the Middleware and MiddlewareFunc interfaces

If we look at how urfave/negroni implements middlewares, we see:

HandlerFunc func(rw http.ResponseWriter, r *http.Request, next http.HandlerFunc)

This final next parameter allows middlewares to do two important things:

  • Stop the Handler chain, and abort the request if they don't call next(rw, r) (pressumably writing an error to rw)

  • Call the next Handler with new, modified, or tweaked ResponseWriter and Request.

I've chosen to keep both functionalities, so a modified http.Handler which returns its own ResponseWriter and Request. This approach allows the same a flexibility as the third next parameter, but I personally found it more elegant.

I hope this will help to clarify my point, and why I find this useful.

PS: I know there are more ways to do this. In fact, our project does have an authentication middleware implemented with the default mux, which works by creating a new middleware object per route, and wrapping the route handler inside a closure. This is ugly, performs really bad, and has multiple race conditions. So this is why I'm trying to found an alternative.

@elithrar
Copy link
Contributor

@roobre Thanks for describing the design in detail. I'll make time to review today/tomorrow and we can decide about moving ahead.

@roobre
Copy link
Contributor Author

roobre commented Sep 28, 2017

Sure!

Also please note that the particular signatures and implementation are open to discussion. The key feature I'm trying to get with this is putting middleware after a match is found, and before the handler is called.

@elithrar
Copy link
Contributor

elithrar commented Sep 28, 2017 via email

@abdullah2993
Copy link

@elithrar thats the whole point, the ability to get current route on the Request object is what is desired in the middleware especially with middlewares that deals with auth. With conventional middlewares a call to mux.CurrentRoute returns nil because the context is set after matching and the middlewares are executed before matching the route.
But IMO its much better to use the classic func(http.Handler) http.Handler for this as demonstrated in master...Abdullah2993:master

@roobre
Copy link
Contributor Author

roobre commented Sep 28, 2017

I'll do some experiments tomorrow and think about how could this be implemented using func(http.Handler) http.Handler. I'm concerned about how we could define an interface with this signature, as often middlewares are complex enough to be full objects and not just functions.

@abdullah2993
Copy link

abdullah2993 commented Sep 28, 2017 •
edited
Loading

@roobre it is actually much simpler then you think...take a look at this all the middlewares use this pattern and it is actually quite simple and elegant

here is a sample of it working

package main

import (
	"fmt"
	"log"
	"net/http"

	"github.com/abdullah2993/mux"
)

type routeLogger struct {
	logger *log.Logger
	next   http.Handler
}

func (rl *routeLogger) ServeHTTP(w http.ResponseWriter, r *http.Request) {
	mr := mux.CurrentRoute(r)
	if mr != nil {
		if rl.logger != nil {
			rl.logger.Printf("[%s] %s", mr.GetName(), r.URL)
		} else {
			log.Printf("[%s] %s", mr.GetName(), r.URL)
		}
	}
	rl.next.ServeHTTP(w, r)
}

//RouteLogger creates a new route logging middleware which logs to the logger if the
//logger is nil then logs to the stdLogger
func RouteLogger(l *log.Logger) func(http.Handler) http.Handler {
	return func(h http.Handler) http.Handler {
		return &routeLogger{logger: l, next: h}
	}
}

func main() {
	r := mux.NewRouter()
	r.HandleFunc("/", index).Name("Index")

	mw := RouteLogger(nil)

	r.AddMiddleware(mw)

	http.ListenAndServe(":8081", r)
}

func index(w http.ResponseWriter, r *http.Request) {
	fmt.Fprintf(w, "Hello World")
}

@roobre
Copy link
Contributor Author

roobre commented Sep 29, 2017

Correct me if I'm wrong, but doesn't this method imply the creation of a new routeLogger for each HTTP request? I find that a bit suboptimal.

@roobre
Copy link
Contributor Author

roobre commented Sep 29, 2017

Ok, I have migrated implementation to the proposed func(http.Handler) http.Handler function. I've also added an interface (Middleware) which can be used to write complex middlewares without a malloc hell. MiddlewareFunc implements Middleware so it's pretty straightforward.

Here's an example implementation

package main

import (
	"log"
	"net/http"
	"roob.re/gorilla-mux"
)

func main() {
	myMux := mux.NewRouter()
	myMux.AddMiddlewareFunc(simpleMw)

	mw := &myMw{}
	myMux.AddMiddleware(mw)

	myMux.HandleFunc("/", func(writer http.ResponseWriter, request *http.Request) {
		writer.Write([]byte("Handled"))
	})

	http.ListenAndServe("0.0.0.0:8080", myMux)
}

// Simple middleware function
func simpleMw(h http.Handler) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		log.Println("Yee-haw")
		h.ServeHTTP(w, r)
	})
}

// Complex middleware object
type myMw struct {
	logger *log.Logger
}

func (mw *myMw) theRealThing(uri string) {
	if mw.logger != nil {
		mw.logger.Println(uri)
	} else {
		log.Println(uri)
	}
}

func (mw *myMw) Middleware(handler http.Handler) http.Handler {
	return http.HandlerFunc(func (rw http.ResponseWriter, r *http.Request) {
		mw.theRealThing(r.RequestURI)
		handler.ServeHTTP(rw, r)
	})
}

However, due to how the middleware chain is parsed, middlewares are executed in reverse order. Using for len instead of for range would solve this, but I'd like to hear your opinion on that.

@abdullah2993
Copy link

abdullah2993 commented Sep 29, 2017 •
edited
Loading

@roobre you are right about the routeLogger being created for each request which is a problem.
as for your implementation can the for range that handles the middlewares moved on to the Router ( Here )?

@elithrar
Copy link
Contributor

elithrar commented Sep 29, 2017 via email

@roobre
Copy link
Contributor Author

roobre commented Sep 29, 2017

@elithrar Yes, I find it more appropiate and intuitive. I'll change it the next time I'll put my hands on the code.

@abdullah2993 That would require duplicating the code, for both Router.Handle() and Router.HandleFunc(). What are the benefits of moving it there, if I may ask? Moving the load of iterating over the list away from the HTTP request? I guess there could be a performance gain, but I'm not sure if it would be worth the cringe of having the same code written on two places.

@abdullah2993
Copy link

@roobre besides performance gain...it will allow us to apply incremental middlewares i.e the middlewares will only be applied to the routes that are registered after regirstering that middleware...for example in case of some auth middleware you can register allowed path before registering the middleware hence some routes will be executed with middleware and some won't be. In the current situation all the registerd middlewares are applied to all the routes...

@roobre
Copy link
Contributor Author

roobre commented Sep 30, 2017

@abdullah2993 I was planning to circunvent that using subrouters, which I find prettier. Otherwise, if you wanted to add the same auth middleware for 10 routes, you'll end up with 10 different instances of it.

@abdullah2993
Copy link

abdullah2993 commented Sep 30, 2017 •
edited
Loading

@roobre subrouters can be used to achieve the same thing but this will allow an elegant alternative and will come in handy, not to mention the negligible performance gain.
PS: You don't need 10 instances for 10 routes...you only need 1 instance provided that the routes are registered after registering the middleware...also i don't see this cluttering the code after all its only 3 lines of code, which if needed can be refactored in to a function

@elithrar
Copy link
Contributor

@abdullah2993 Can you show a simple example of the benefits of your approach from an user API perspective?

Right now, @roobre's approach—scoping to (sub-)routers makes sense to me, and aligns with other libraries like chi.

@abdullah2993
Copy link

@elithrar sure as soon as I get my hands on the computer. This approach will allow not keep you from scoping to sub-routers, if you want to you can but if you don't want to then you can get away with it

@roobre
Copy link
Contributor Author

roobre commented Oct 2, 2017

@abdullah2993 Another problem of moving middleware handling to the route is that becomes difficult to add a middleware to the whole router. Either the user copypastes the whole .AddMiddleware(mw) thing for every route, or they need to use NewRoute().AddMiddleware().Subrouter(), which seems really ugly to me.

We could also allow both Route and Router middlewares, where routes would have their own middlewares []Middleware and AddMiddleware(). However, I find this too complex and maybe confusing (could easily lead to wrong setups where the same middleware is executed twice).

My personal opinion is that middlewares should belong to a router, and not to a route. It just seems more coherent that way. In the end, if you have a middleware that you need to apply to only one or two routes, it makes more sense for that code to be in the route handler instead.

@abdullah2993
Copy link

@elithrar @roobre

package main

import (
	"fmt"
	"net/http"

	m "github.com/abdullah2993/mux"
	// m "github.com/abdullah2993/mux1"
)

func main() {
	r := m.NewRouter()

	r.AddMiddleware(mw("1"))
	r.Handle("/", dummyHandler("Index"))

	r.AddMiddleware(mw("2"))
	r.Handle("/login", dummyHandler("Login"))
	r.Handle("/user", dummyHandler("User"))

	r.AddMiddleware(mw("3"))
	r.Handle("/about", dummyHandler("About"))

	fmt.Printf("------------\n")
	http.ListenAndServe(":8080", r)

}

func mw(name string) func(h http.Handler) http.Handler {
	return func(h http.Handler) http.Handler {
		fmt.Printf("Evaluating Middleware %s\n", name)
		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			fmt.Printf("[Middleware: %s]: %s\n", name, r.URL)
			h.ServeHTTP(w, r)
		})
	}
}

func dummyHandler(name string) http.Handler {
	fmt.Printf("Adding Handler for %s\n", name)
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		fmt.Fprintf(w, name)
	})
}

Output with first approach:

Adding Handler for Index
Adding Handler for Login
Adding Handler for User
Adding Handler for About
------------
Evaluating Middleware 1
Evaluating Middleware 2
Evaluating Middleware 3
[Middleware: 3]: /
[Middleware: 2]: /
[Middleware: 1]: /
Evaluating Middleware 1
Evaluating Middleware 2
Evaluating Middleware 3
[Middleware: 3]: /login
[Middleware: 2]: /login
[Middleware: 1]: /login
Evaluating Middleware 1
Evaluating Middleware 2
Evaluating Middleware 3
[Middleware: 3]: /user
[Middleware: 2]: /user
[Middleware: 1]: /user
Evaluating Middleware 1
Evaluating Middleware 2
Evaluating Middleware 3
[Middleware: 3]: /about
[Middleware: 2]: /about
[Middleware: 1]: /about

Output with 2nd approach:

Adding Handler for Index
Evaluating Middleware 1
Adding Handler for Login
Evaluating Middleware 1
Evaluating Middleware 2
Adding Handler for User
Evaluating Middleware 1
Evaluating Middleware 2
Adding Handler for About
Evaluating Middleware 1
Evaluating Middleware 2
Evaluating Middleware 3
------------
[Middleware: 1]: /
[Middleware: 2]: /login
[Middleware: 1]: /login
[Middleware: 2]: /user
[Middleware: 1]: /user
[Middleware: 3]: /about
[Middleware: 2]: /about
[Middleware: 1]: /about

which can also allow us to have an api like this

package main

import (
	"fmt"
	"net/http"
	// m "github.com/abdullah2993/mux"
	// m "github.com/abdullah2993/mux1"
	m "github.com/abdullah2993/mux2"
)

func main() {
	r := m.NewRouter()
	mw1 := mw("1")
	mw2 := mw("2")
	mw3 := mw("3")

	r.WithMiddlewares(mw1)
	r.Handle("/", dummyHandler("Index"))

	r.WithMiddlewares(mw1, mw2)
	r.Handle("/login", dummyHandler("Login"))
	r.Handle("/user", dummyHandler("User"))

	r.WithMiddlewares(mw1, mw2, mw3)
	r.Handle("/about", dummyHandler("About"))

	fmt.Printf("------------\n")
	http.ListenAndServe(":8080", r)

}

func mw(name string) func(h http.Handler) http.Handler {
	return func(h http.Handler) http.Handler {
		fmt.Printf("Evaluating Middleware %s\n", name)
		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			fmt.Printf("[Middleware: %s]: %s\n", name, r.URL)
			h.ServeHTTP(w, r)
		})
	}
}

func dummyHandler(name string) http.Handler {
	fmt.Printf("Adding Handler for %s\n", name)
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		fmt.Fprintf(w, name)
	})
}

@roobre
Copy link
Contributor Author

roobre commented Oct 2, 2017

I definitely don't like this. Using order of addition to define which middlewares affect which routes is completely intuitive.

@roobre
Copy link
Contributor Author

roobre commented Oct 2, 2017

Status update: I moved middleware chain building from ServeHTTP() to Match().

This is because ServeHTTP() is called on the parent router, even if the match is a subrouter. See 9315e1c for details.

@elithrar
Copy link
Contributor

elithrar commented Oct 2, 2017 via email

@roobre
Copy link
Contributor Author

roobre commented Oct 2, 2017

No prob. I'll probably add a few test cases tomorrow, as well as code documentation and README manuals.

@roobre
Copy link
Contributor Author

roobre commented Oct 3, 2017

Status update: Test cases added, brief documentation included in README.md and doc.go.

@elithrar
Copy link
Contributor

elithrar commented Oct 13, 2017 via email

@roobre
Copy link
Contributor Author

roobre commented Oct 18, 2017 •
edited
Loading

I've discovered an annoying bug on my implementation.

Middlewares aren't supposed to be run if no match is found, however, given how Router.Match() works right now, if a subrouter's Router.NotFoundHandler is not nil, the middleware will be executed, as the match is not nil.

This bug makes me think about the way mux handles this event: correct me if I'm wrong, but if a method mismatch occurs, Route.Match() returns false and sets matchError accordingly. However, if no route is found, instead of returning false and setting matchError to ErrMatchNotFound (or something like that), Router.Match() just returns true as if it were a normal match. Why does mux behaves this way?

Changing this behaviour as I described would allow an easy check for a not found event, and avoid building the middleware chain if it took place.

@elithrar
Copy link
Contributor

Sorry for the slow reply @roobe - catching up on issues.

I'm not the original author for mux, so I can't explain precisely why it behaves that way.

I'm OK with:

  • Method mismatch (not-found) returning false and setting matchError
  • Route not-found returning false and setting matchError to ErrMatchNotFound

Notably, there may be valid cases where you still want middleware to execute when there is no match (logging all requests, for example!), so we should provide a way to account for that too.

@roobre
Copy link
Contributor Author

roobre commented Oct 29, 2017 •
edited
Loading

No prob, I agree with you in both points. I think it's best if I create a new PR for setting ErrMatchNotFound when falling back to NotFoundHandler, and then continuing on the middleware branch when it is merged. That should avoid unnecessary conflicts. Are you okay with that?

@elithrar
Copy link
Contributor

elithrar commented Oct 29, 2017 via email

@roobre roobre mentioned this issue Nov 3, 2017
Merged
@roobre
Copy link
Contributor Author

roobre commented Nov 3, 2017

#311 created.

elithrar pushed a commit that referenced this issue Jan 16, 2018
@roobre @elithrar
[feat] Add middleware support as discussed in #293 (#294)
53c1911 
* mux.Router now has a `Use` method that allows you to add middleware to request processing.
@elithrar
Copy link
Contributor

Closed in #294.

@roobre roobre closed this as completed Jan 17, 2018
tbutts added a commit to pereztr5/cyboard that referenced this issue Jan 20, 2018
@tbutts
Update all vendored deps (glide cc && glide update)
26f6ceb 
A new dev cycle means it's a good time to update. I had to remember
to clear my glide cache before updating due to a long standing bug
(see: Masterminds/glide#592).

I was definitely motivated, to see if we can benefit from first-class
middleware support in `gorilla/mux` (see:
gorilla/mux#293).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@elithrar elithrar

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@elithrar @roobre @abdullah2993