Feedback: mux v2?

[elithrar]

What would a mux v2 look like?

One of mux's strengths, compared to other HTTP routing libraries, is it's extensive API. Regular expression matching, header matching, prefix-based matching, etc. Another strength is that it builds around the standard library's http.Handler and http.HandlerFunc types, as well as the HandleFunc and Handle function signatures. This makes adopting mux, or even moving away from it, easy: we're not wrapping everything a custom mux.Context type. Bring your own custom handler types, or migrate over from plain vanilla net/http when you want more powerful matching tools.

At the same time, the API is roughly double the "size" of its contemporaries. There's a LOT of things to stitch together, and the code has grown a lot, making debugging interoperability problems between features like PathPrefix, StrictSlash, middleware and everything else an ongoing challenge.

So, if you've read this far:

• What features of mux are you using the most?
• What features do you not use, or see yourself using?
• What do you wish we had, and why? (concrete examples useful here)

Note that this isn't a commitment to actually building a "v2": it may be that some of the things that come out of a discussion here can be rolled into the current mux package without breaking the API. But let's aim high, and then adjust as we need to.

[kisielk]

One thing which I think would help reduce the size of the mux API by roughly half is if the Router / Route duality was eliminated. They both have roughly the same methods, with just a few extras on Router. We should evaluate what's strictly necessary to have only once at the Router scope and then just put everything else on Route. The Router could have top-level Route for the root path and then a tree below that.

[elithrar]

Great idea, and definitely agree. Let me think on that and draft up what the API would look like under that approach.

Need to think about inheritance/parent-child relationships between a router / sub-routers, etc.

Thinking out loud: Having a Router return a “implicit” Route would support method chaining, and that implicit route would simplify internals too. On Fri, Jun 8, 2018 at 8:15 AM Kamil Kisiel notifications@github.com wrote:

One thing which I think would help reduce the size of the mux API by roughly half is if the Router / Route duality was eliminated. They both have roughly the same methods, with just a few extras on Router. We should evaluate what's strictly necessary to have only once at the Router scope and then just put everything else on Route. The Router could have top-level Route for the root path and then a tree below that.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/gorilla/mux/issues/387#issuecomment-395792994, or mute the thread https://github.com/notifications/unsubscribe-auth/AABIcMxbwm9uiLyQk8u7gtoWDXSBl9LGks5t6pUGgaJpZM4UgeL_ .

[pzillmann]

What features I use most

• I name my routes so a worker can run a function according to that name in a function register
• I check for Authentication Headers
• I use Regex to serve static files when using no proxy

I'm sometimes confused between the difference of Route vs. Router

[matejkramny]

We use a lot of subrouter and middleware functions. Also sort of intermediate middleware, example route below:

    r.Handle("/my-route", myMw(myHandler))).Methods("GET", "HEAD")

Ideally it would be something like

    r.Get("/my-route", myMw, myHandler)
    // or
    r.HandleFunc("/my-route", myMw, myHandler).Methods("GET", "HEAD")

I should note we use a wrapper for mux (https://github.com/castawaylabs/mulekick) for readability

[kisielk]

Not sure if I'm on board with that idea. There's an infinite number of possible HTTP methods, so you can't cover them all that way. It also doesn't read well to me because it looks like you're trying to get something from the router.

[elithrar]

Agree. Further, one of the friendliest features of mux is that you can swap from net/http's ServeMux to mux.Router easily, since the Handle and HandleFunc arguments are the same.

Per-route middleware is often a mistake, especially for security and logging. Log at the router level, and apply security features at the SubRouter level to prevent "missing" applying to a route. It would also be hard to express the default - HandleFunc("/path", nil, myHandler) would be more common than the per-route middleware use-case.

[jwilner]

Hi -- I'd like to chime in as a many-time user and one-time contributor to mux and maybe revitalize this convo, which I think is important!

I very much think there is a space for a new v2 gorilla mux. I think that the API has gotten to a place where it tries to be all things to all people and that can both increase the complexity of use and implementation. To me, I think it makes sense that mux coordinate on http.Handler and http.HandlerFunc are good ideas, and more generally that it aim to be the easiest router to adopt when you step out of the stdlib for dynamic routing. I think a lot of the current accessories that go beyond path and method -- scheme, host, header matching -- get away from that goal, are comparatively unused, and introduce a whole ton of complexity to the internal logic. I also would argue that the subrouter concept is more powerful simply as an unfinished route builder. I would argue for a course would be to aim for a V2 which provides breaks support for the more ancillary options, while keeping the core ones.

As a general API, I think mux should have a core Router which embeds some sort of zero'd RouteBuilder, the RouteBuilder be treated as an immutable value builder for chaining, and the chain always end with a Handle(METHOD, handler) or a HandleFunc(METHOD, handleFunc), which are the only actions which actually mutate the core router.

Spitballing the interface:

func New() Router {
     ...
}

type Router interface {
       RouteBuilder
      // _maybe_ some shortcuts permitting common use cases
       http.Handler
}

type RouteBuilder interface {
       // maybe some other things, always returning RouteBuilder though
       Path(path string) RouteBuilder
       Middleware(mw Middleware) RouteBuilder

       Handle(method string, handler http.Handler)
       HandleFunc(method string, f func(http.ResponseWriter, *http.Request))
}

E.g.

r := v2.NewRouter()
// bar is a RouteBuilder and the conceptual replacement for a "subrouter"
bar := r.Path("/foo/bar") 
// bar value remains unchanged -- barID is a qualified version of it
barID := bar.Path("/{bar_id}")  
// Handle (or HandleFunc) always terminates a RouteBuilder; handling methods at the end is consistent
// with the RESTful notion of resources IMO -- you build the URI to a resource and then define various 
// operations (methods) on it.
barID.Handle(http.MethodGet, barReadHandler)
barID.HandleFunc(http.MethodPut, barUpdateHandler.ServeHTTP)
bar.Handle(http.MethodPost, barCreateHandler)

Equivalently:

r := v2.NewRouter()
r.Path("/foo/bar").Handle(http.MethodPost, barReadHandler)
r.Path("/foo/bar").Path("/{bar_id}").Handle(http.MethodGet, barReadHandler)
r.Path("/foo/bar").Path("/{bar_id}").HandleFunc(http.MethodPut, barUpdateHandler.ServeHTTP)

Resulting routes:

POST /foo/bar
GET /foo/bar/{bar_id}
PUT /foo/bar/{bar_id}

Each route chain method would have clearly defined compositional behavior. E.g.:

r.Path("/foo").Path("/bar") // r.Path("/foo/bar")
r.Middleware(f).Middleware(g) // r.Middleware(f(g))

Best of all, this would be relatively simple to implement.

func New() Router {
        r := &router{trie: make(trie) /* or whatever */}
        r.RouteBuilder = routeBuilder{router: r}
        return r
}

type router {
        RouteBuilder
        trie
}

type routeBuilder struct {
        router *router
        path string
        // other stuff
}
// note no pointer receivers! this is key
func (rb routeBuilder) Path(p string) RouteBuilder {
        rb.path += p
        return rb
}
func (rb routeBuilder) Handle(string method, handler http.Handler) {
        if rb.middleware {
                 handler = rb.middleware(handler)
        }
        rb.router.trie.insert(method, rb.path, handler)
}

Ended up writing more than I intended, but I hope this is helpful.

PS to the degree that it's important to keep host, header, etc matching, I'd encourage having a general purpose:

func (rb routeBuilder) MatchFunc(mf MatchFunc) RouteBuilder {
         rb.matchFuncs = append(rb.matchFuncs, mf)
         return rb
}

With orthogonal helper methods:

func HeaderMatcher(headers ...string) MatchFunc {
    ...
}

[elithrar]

Thanks @jwilner -

1. I agree that the API has grown, although some of that flexibility is often why users leave the std. lib (i.e. as they outgrow it)
2. I'd argue that fixing each handler to a single method promotes repetition and doesn't make things much clearer - there are other libraries that promote the func(method string, h http.Handler[Func]) signature as first-class. If users prefer that, it probably makes sense for them to use those libraries over mux.
3. I am for making sure that method chaining works easily - e.g. methods should all return a *Route such that you can either chain more specific matching logic, or build your own on top.
4. I don't think breaking everything is the right move (also see no. 2) - a clear migration path is key. Yes, mux' API has grown a little larger than anticipated - such is the life of any widely used software project - but each addition has often been rational at the time. The bar for adding to the API has also increased over time - dramatically (I think we say "no" a lot more now).

Critically:

I think a lot of the current accessories that go beyond path and method -- scheme, host, header matching -- get away from that goal, are comparatively unused, and introduce a whole ton of complexity to the internal logic.

I'd want to be careful about assuming this! In fact, I'm working on a project that can help quantify the methods-in-use by importers, so that we don't have to make assumptions here.

[stale[bot]]

This issue has been automatically marked as stale because it hasn't seen a recent update. It'll be automatically closed in a few days.

[dimkasta]

Hi. I don t know if you are still interested in feedback, but here are some of my thoughts. At its current state, mux is more of an opinionated mini-framework than a router. It makes decisions about how you should wire your application, and much of its functionality cannot be used on its own. For example, lets say that instead of middleware we want to wire our application using an event dispatcher and that we need url matching and var extraction on separate events and listeners. In my opinion all separate functionality should be available standalone. And if you still want to cover the current turn-key use case, a thin wrapper of that functionality would cover everyone that just want to patch something into ListenAndServe

[elithrar]

@dimkasta mux is designed to be a more powerful http.ServeMux - fleshing out its basic matching with support for more request attributes, regex, etc.

It focuses heavily on that, and speaks the http.Handler interface so that package users can bring their own middleware, handlers, etc with minimal effort. Using common interfaces is a design goal.

An “event dispatcher” would be a very different design, and would be less a v2 and more a completely different library.

[timsolov]

It'd be nice to add these features:

• possibility to use beautiful setting of cors middleware:

  // this is terrible:
  http.ListenAndServe(":8000", handlers.RecoveryHandler()(handlers.CORS(rules)(r)))
  // better:
  r.UseOver(handlers.RecoveryHandler())
  r.UseOver(handlers.CORS(rules))

• add routes by method name:

  r.GET("/hello", handler.Hello)
  r.POST("/create", handler.Create)
  r.PUT("/edit", handler.Edit)

  I'm using my github.com/timsolov/gorilla-mux-booster package for all above. But I think it should be out of the box.

[fharding1]

@timsolov

You might find that people are more receptive to your feedback when you explain why you don't like something in polite terms rather than call it terrible without explanation.

How is your .UseOver different than .Use? What makes it better specifically?

I'm not a fan of the method name style API. You need a generic function that can take any string as a method anyways, so what's the point of adding helper functions? It seems like a lot of added cruft to let devs write r.GET("/hello", handler.Hello) instead of r.Handle("/hello", handler.Hello).Methods(http.MethodGet), and it's not much "safer" (regarding typos and stuff) than using HTTP method constants.

Also, if you look at the Go standard library, you will see HTTP methods are taken as arbitrary strings without helper functions. For example, the function signature of http.NewRequest is func NewRequest(method, url string, body io.Reader) (*Request, error). There's not http.NewRequestGet, http.NewRequestPost, etc. It does have http.Get, http.Post, etc, but this is different since those helper functions accept different arguments and behave differently from each other, it's not just matching on a string like mux does.

[timsolov]

@fharding1

About .UseOver:

The Use() method in gorilla/mux setups middleware which executes after route-matching that is why we can't set CORS middleware by that func. We need mechanism to define middleware over the route-matching for CORS, Recovery and some other specific middlewares which should be executed before mux start working.

And if you look at chi for example, it has two methods for setting up these different types of middleware: .Use() and .With(), in my wrapper it's similar to .UseOver() and .Use().

About methods naming:

I don't insist on this change and agree to your opinion. But I think we are going to next generation of library which should resolve programmers pain and probably someone's laziness to write much symbols :) There is only one more approach and community can choose for itself.

I love to group my routes like:

// all can access
public := gmb.NewRouter()

// only authorized users
auth := public.Subrouter()
auth.Use(middlewares.AuthRequired)

public.
    POST("/auth/signin", auth.SignIn(authService)).
    POST("/auth/signup", auth.SignUp(authService)).
    GET("/auth/email/confirm/{code}", auth.EmailConfirm(authService)).

auth.
    GET("/auth/signout", auth.SignOut(authService)).
    GET("/user", user.State(userService))

In real projects here could be very big list with long strings for example:

admin.HandleFunc("/admin/users/{user_id}/roles/{role_id}/json", AdminUsersRoles(store)).Methods(http.MethodGet)

To find MethodGet you should scroll to right of your editor but as for me I'd prefer this:

admin.GET("/admin/users/{user_id}/roles/{role_id}/json", AdminUsersRoles(store))

Here I see http method first then route and handler. All significant information we have at the beginning of the line.

[fharding1]

@timsolov thanks for the reply

UseOver does seem useful and I agree that there are valid use cases for running "middleware" before/over route matching. My main concern with something like this would be explaining the difference between Use and UseOver to new users. With good documentation it might not be a big problem, but I could totally see the similar names and functionality causing confusion. Since this is a pretty easy thing to implement outside of mux, we would have to decide whether the potential confusion is worth it.

Readability is important to me and I've seen what you're describing in "real projects" before. Navigating router setup can be a huge pain. I'm still not sure whether a bunch of redundant function names is the best solution, but this is something we should definitely think about for a mux 2.

[ibraheemdev]

Is performance a concern for gorilla mux users or v2? Chi is 2-4 times as fast consistently in benchmarks. It uses a radix tree model that makes use of common prefixes, similar to httprouter. I noticed gorilla mux simply loops through the registered routes until it finds a match. Is that scalable? It sounds like it would be O(n)?