Clean sub-router mounts

[pkieltyka]

This change adds support for wildcard patterns that connect the slashes of a sub-router.

For example.. the current behaviour:

m := web.New()
m.Get("/", indexHandler)

users := web.New()
users.Use(middleware.SubRouter)
m.Handle("/users/*", users)
users.Get("/", listUsersHandler)
users.Post("/", createUserHandler)
users.Get("/:userId", getUserHandler)

getting a list of users, or creating a new user, the client would have to specifically request:
GET /users/
POST /users/
A GET to /users or POST to /users will result in a 404.... which is certainly not the conventional or expected behaviour of an API.

in order to support both /users/ and /users, you have to individually add those routes as so:

m.Get("/users", listUsersHandler) // these should not be necessary.... 
m.Post("/users", createUserHandler) // these should not be necessary.... 

.. and as a I add more and more groups of handlers (sub-routers), its polluting my code.

This PR, allows both cases to work, depending if the wildcard route provided is /* or *. As so:

m := web.New()
m.Get("/", indexHandler)

users := web.New()
users.Use(middleware.SubRouter)
m.Handle("/users*", users)
users.Get("/", listUsersHandler)
users.Post("/", createUserHandler)
users.Get("/:userId", getUserHandler)

Also, this change is fully backwards compatible with the previous functionality. For a full example, see: https://gist.github.com/pkieltyka/3afcfd34bd45f29fe372

[zenazn]

Subroutes used to look a lot more like this, actually. I ended up changing the functionality to what you see today because I think of /foo and /foo/ as different routes [1]. It's likely that in many real-world apps one should redirect to the other (which has been proposed before: see #9 for instance), but I feel slightly uncomfortable providing functionality in Goji that makes it impossible to distinguish between the two.

Also, /foo* looks kind of ugly :P

[1]: a bunch of other frameworks make this same distinction. I know I tested Sinatra, and I think I also tested Flask. While I hate to use this as justification for Goji's behavior here, at least it means I'm not completely off my rocker :)

[pkieltyka]

Well, I'm not married to the syntax or implementation, but I'm just speaking of what would be the expected functionality when you create a namespace that represents a sub-resource. For example, think of a filesystem -- you can access a directory by cd folder or cd folder/. This is how I feel about a Sub-Router.

Also, think about when you set baserouter.Get("/", handler) this will match http://host.com as well as http://host.com/. So, why wouldn't it be intuitive to treat mounted sub-routers in the same way?

Btw /foo* works just fine in Sinatra :) .. but, it's confusing because it would match /foobar.

hrmm.. perhaps a better solution to this PR then is to have..

base := web.New()
base.Get("/", index)
base.Get("/hello", hello)

admin := web.New()
admin.Get("/", adminIndex)
admin.Post("/", adminCreate)
admin.Get("/:adminId", adminGet)

base.Handle("/admin", Mount(admin))
// or...
base.Mount("/admin", admin)

I don't think base.Handle() could be supported like I showed above because the router wouldn't match anything besides literally, /admin.. which is why I have this fix to mount /admin*. But.....! .Mount() could certainly work, and it could set the SubRouter middleware as well on admin. Thoughts? can we add a .Mount() or something better?

[zenazn]

Neither of those would work, because routing is a function of the pattern, not the handler or the mux.

But back to the philosophical question at hand, I don't think /foo and /foo/ are the same, and I don't think Goji should make it easy to blur the two together. In most websites it's common to redirect one route to the other (i.e., by returning an HTTP 301), but in my opinion it's sloppy to route them, uncaring, to the same application logic.

The root of a domain is actually a special case: something like GET / is actually transmitted on the wire even if your browser UI doesn't display the trailing / to you. For example:

~$ curl -v goji.io
* Rebuilt URL to: goji.io/
* Hostname was NOT found in DNS cache
*   Trying 192.155.86.120...
* Connected to goji.io (192.155.86.120) port 80 (#0)
> GET / HTTP/1.1
> User-Agent: curl/7.37.1
> Host: goji.io
> Accept: */*
>

[pkieltyka]

I think this is getting a bit extreme to nit-pick such a detail that could really go 50/50, so why not support both cases?

Question, if you were designing + writing a REST API with Goji, and you had a resource, /users, would you design the routes as GET /users to get a list of the users and POST /users to create a new user? or would have GET /users/ and POST /users/ ? .. because the way Goji is written today, if favors /users/ for both of those http methods. In fact, you couldn't even make the first case work without adding specifically adding routes for GET /users and POST /users, and then.. writing a handler (maybe middleware if you want to get fancy), to redirect a 301 to a route that is defined on the parent... do this for a few sub-routers and lol... shoot me in the face :/

I think its fair that you say they are different, and stricter design would lean on the side to do a 301 from /users/ to /users .. but actually that wouldn't even work with how Goji is written today.. and so.. respectfully, this strictness concerns me for the evolution of the framework as a whole

[moorereason]

I agree with Carl: /users & /users/ are distinct routes that should be treated as such by the default router. I never include the trailing slash in my APIs. If you GET /users/ on my systems, you'll get a 404 -- as you should.

What's wrong with expecting your clients to use the proper URL??

[zenazn]

You're right: I'd probably design the routes as GET /users, for instance, and redirect GET /users/ to the version without the slash. The sticking point for me, however, is the fact that under the sub-router semantics you propose, the two are indistinguishable for the sub-router. I want Goji's router to always be able to tell the difference, since I believe there is a difference. In practice, this means I'm forced to do a little bit more work. I'm okay with that.

Perhaps the misunderstanding here is about the vision for how much abstraction power Goji provides (or ought to provide). It's always been my intention to give users all the primitives they require to build websites and APIs, but to provide very few of the higher-level abstractions. I fully expect users to write higher-level utilities on top of Goji (e.g., you could make an "API resource" helper function that correctly dealt with the sub-router and redirects), but I'm not confident enough in any one design to include them in the base project. In this sense, "micro-framework" is perhaps a bit of a misnomer: Goji is in some strong sense a routing and middleware library.

Does that make sense? And please let me know if I'm misunderstanding or misrepresenting you :)

[pkieltyka]

Yea I also agree that GET /users/ is a useless route for the case we're discussing, and should be a 404. But, again.. Goji is not designed to support stacking sub-routers in a clean way for the expected functionality as the pattern is /subapp/*, which always requires the / .. and this is my point and the point of this ticket, albeit maybe not the perfect solution, it's the then a function of a larger issue at hand in the router design.

I am saying that I should be able to through a single statement to a router, mount a sub application to that router something like this:

base.Get("/", someroute)

// Create and mount admin sub-router
adminRouter := web.New()
adminRouter.Use(middleware.SubRouter)
adminRouter.Get("/", adminIndexHandler)
adminRouter.Post("/", createAdminHandler)
base.Handle("/admin*", adminRouter)

// Create and mount sites sub-router
sitesRouter := web.New()
sitesRouter.Use(middleware.SubRouter)
sitesRouter.Get("/", sitesIndexHandler)
sitesRouter.Post("/", createSitesHandler)
base.Handle("/sites*",sitesRouter)

... NOT as currently is .........:

base.Get("/", someroute)

// Create and mount admin sub-router
adminRouter := web.New() // likely defined in the admin package itself
adminRouter.Use(middleware.SubRouter)
adminRouter.Get("/", adminIndexHandler)
adminRouter.Post("/", createAdminHandler)
base.Handle("/admin/*", adminRouter)

base.Get("/admin", adminIndexHandler) // ...pls no..
base.Post("/admin", createAdminHandler) // ...pls no..
base.Get("/admin/", http.RedirectHandler("/admin", 301)) // ...pls no..
base.Post("/admin/", http.NotFoundHandler()) // ...pls no..

// Mount sites sub-router
sitesRouter := web.New() // likely defined in the sites package itself
sitesRouter.Use(middleware.SubRouter)
sitesRouter.Get("/", sitesIndexHandler)
sitesRouter.Post("/", createSitesHandler)
base.Handle("/sites*", sitesRouter)

base.Get("/sites", sitesIndexHandler) // ...pls no..
base.Post("/sites", createSitesHandler) // ...pls no..
base.Get("/sites/", http.RedirectHandler("/sites", 301)) // ...pls no..
base.Post("/sites/", http.NotFoundHandler()) // ...pls no..

.. do you see...? The web app that I wrote with Goji has 10 sub-routers... you can imagine why I am so frustrated about this code cruft. If you see a better way and can fix my example above, then I am happy to admit being wrong and apologize for wasting your time, but I'm not seeing it.

... I hear you that this PR and example in this comment still uses /admin* which incorrectly allows /admin/ to work for GET and POST.

Another proposal, as I mentioned, is to support router.Mount() .. I understand the router is a function of the pattern, and so Mount() would take a vanilla pattern, and set some internal state that would inform the router there are expected to be other routes mounted on this path. That is surely possible, and then you could get opinionated about how the sub-routers would stack with all the /'s .. and the code could still function the same way. One way would be to use characters in the pattern to inform the string pattern router/parser, or make an interface of some kind, or add some internal state field on a pattern type etc....... many ways.

Perhaps other ideas could be proposed to this issue that I've put forth, or let me know it will never be fixed and I will accept that and move on to another http router.

[zenazn]

Would a helper function be appropriate for your use case?

// Everything is completely untested
func fakeRoot(h web.Handler) web.Handler {
    fn := func(c web.C, w http.ResponseWriter, r *http.Request) {
        c.URLParams["*"] = "/"
        h.ServeHTTPC(c, w, r)
    }
    return web.HandlerFunc(fn)
}
func SubRoute(prefix string, h web.Handler) {
    goji.Get(prefix, fakeRoot(h))
    goji.Handle(prefix, fakeRoot(h))
    goji.Get(prefix + "/", func(w http.ResponseWriter, r *http.Request) {
        http.Redirect(w, r, prefix, 301)
    })
    goji.Handle(prefix + "/", http.NotFound)
    goji.Handle(prefix + "/*", h)
}

Now you can do something like this:

SubRoute("/admin", adminRouter)
SubRoute("/sites", siteRouter)

[pkieltyka]

thanks Carl! I'll give that a shot

[pkieltyka]

@zenazn thanks for that, I have a variation of that solving my problem now. I still think it would be nice to have a Mount() method on the mux to do this and get rid of the SubRouter middleware altogether. cheers.