[WIP] New router

[rgrinberg]

TODO

• trie based implementation
• add **
• port old router
• documentation
• optimization
• proper error messages
• tail recursive addition of routes. let's skip this for now, I doubt this will be useful for realistic routes

Discussion

• Do we need optional parameters? E.g. :foo?

• I still haven't created an api for site composition. Still thinking of what is
  that going to look like

[rgrinberg]

Okay, I think this is ready for some preliminary review.

[joseferben]

Regarding API for site composition, I implemented a very simple router API like this for Sihl:

let all =
  [
    router ~scope:"" ~middlewares:Middleware.site_middlewares public;
    router ~scope:"/applicants"
      ~middlewares:Middleware.logged_in_site_middlewares applicant;
    router ~scope:"/applications"
      ~middlewares:Middleware.applications_site_middlewares application;
    router ~scope:"/admin" ~middlewares:Middleware.admin_middlewares admin;
    router ~scope:"/api" ~middlewares:[] api;
  ]

But it became apparent that we need an API that allows nesting routers. F#'s Giraffe has these combinators.

let notLoggedIn =
    RequestErrors.UNAUTHORIZED
        "Basic"
        "Some Realm"
        "You must be logged in."

let mustBeLoggedIn = requiresAuthentication notLoggedIn

let webApp =
    choose [
        route "/"     >=> text "Hello World"
        route "/user" >=>
            mustBeLoggedIn >=>
                choose [
                    GET  >=> readUserHandler
                    POST >=> submitUserHandler
                ]
    ]

[rgrinberg]

That looks interesting. What I had in mind with composition was the ability to "mount" 3rd party web apps under a URL. Using something like:

val mount : Router.t -> string -> Rock.App.t -> Router.t

For example:

let router = mount router "/metrics" metrics in

The idea is that all the routers inside metrics will now be relative to /metrics prefix now.

What your snippet shows is more of an API that allows for hierarchical construction of the router. What I mean by that is that parameters are transparent to the subroutes. E.g.:

let webApp =
    choose [
        route "/user/:id" >=>
            mustBeLoggedIn >=>
                choose [
                    GET  >=> readUserHandler
                    POST >=> submitUserHandler
                ]
    ]

:id should be visible to both routes, correct?

[rizo]

Just curious, module type of Router wouldn't be the same thing? Or is this to allow adding other elements to the signature?

[rgrinberg]

I recall that the simplest form omitted some type equalities for types inside Router.t. So if one used Router from outside this alias, the type Router.t wouldn't be equal. But I think this bug was fixed not too long ago. I'll see if the simpler form works as well.

[rizo]

Do you have any guidelines on naming conventions for similar functions in Opium? I assume m is "middleware", but would be good to know if this should be used more generally.

[rgrinberg]

Yeah, that's my original naming convention for a module that provides a middleware. It seemed a little long to name things like Middleware_foo.middleware. Not so sure if this convention is still beneficial.

[rizo]

Haven't looked in detail into this, but one thing that I believe is important is to expose the ability to iterate and inspect routes after they have been added. The use case that I have in mind is generating OpenAPI/Swagger specifications for the API.

[rgrinberg]

Haven't looked in detail into this, but one thing that I believe is important is to expose the ability to iterate and inspect routes after they have been added. The use case that I have in mind is generating OpenAPI/Swagger specifications for the API.

This will be covered. Though I think we'll need a handler type that allows for more introspection.

[rgrinberg]

I'm now wondering how to treat trailing slashes and parameter capture. Should * capture empty parameters? For example, should /foo/:bar capture /foo//? Additionally, should /** distinguish /foo/ from /foo?

[tmattio]

Should * capture empty parameters? For example, should /foo/:bar capture /foo//

I would say no, IMO /foo/ should be the same as /foo//, and I would expect that /foo/:bar does not capture /foo/.

Additionally, should /** distinguish /foo/ from /foo

Again, I would say no, but it's a bit more opinionated. I remember some websites where the former redirected to the latter for instance. FWIW, I just tried on Github and it does not make a distinction. That's sensible behavior IMO, and the end-users are always free to implement their own Router, so it's ok to be opinionated I think.

[reynir]

I think it makes sense to remove empty segments (save for / I guess). It's illegal to specify a route matching an empty segment:

opium/opium/src/router.ml

Line 48 in 6a8aff7

then raise (E "Double '/' not allowed")

I don't know what the behavior is with this implementation, but with the current implementation /foo/:bar/ matches /foo/bar/ but not /foo/bar. Meanwhile, /foo/** treats /foo/bar/ and /foo/bar the same. I think that's a bit surprising.

I am using full splat for serving files from an archive file: /:archive/file/**, and I think it's interesting that both /foo/file/README.txt and /foo/file/README.txt/ work.

[rgrinberg]

I would say no, IMO /foo/ should be the same as /foo//, and I would expect that /foo/:bar does not capture /foo/.

That makes it impossible to have something like /foo/:bar/:baz where a route only fills :baz but not :bar. Probably not too big of a deal, but not very elegant. Are you familiar with how other frameworks handle this?

That's sensible behavior IMO, and the end-users are always free to implement their own Router, so it's ok to be opinionated I think.

That reminds me that ** is how I expect users to implement their own router while staying compatible with the rest of the ecosystem. Currently, we just merge * and **, but that needs to change. @reynir are you ok with that?

EDIT:

In particular, if we treat ** opaquely and just pass remaining the user, then you'll be able to handle

I am using full splat for serving files from an archive file: /:archive/file/**, and I think it's interesting that both /foo/file/README.txt and /foo/file/README.txt/ work.

As you prefer.

[reynir]

I would say no, IMO /foo/ should be the same as /foo//, and I would expect that /foo/:bar does not capture /foo/.

That makes it impossible to have something like /foo/:bar/:baz where a route only fills :baz but not :bar. Probably not too big of a deal, but not very elegant. Are you familiar with how other frameworks handle this?

In my (limited) experience it most often doesn't matter if you add empty segments, e.g.: https://github.com/rgrinberg//opium///pull/215

That's sensible behavior IMO, and the end-users are always free to implement their own Router, so it's ok to be opinionated I think.

That reminds me that ** is how I expect users to implement their own router while staying compatible with the rest of the ecosystem. Currently, we just merge * and **, but that needs to change. @reynir are you ok with that?

EDIT:

In particular, if we treat ** opaquely and just pass remaining the user, then you'll be able to handle

I am using full splat for serving files from an archive file: /:archive/file/**, and I think it's interesting that both /foo/file/README.txt and /foo/file/README.txt/ work.

As you prefer.

Yes, I like this.

[tmattio]

That makes it impossible to have something like /foo/:bar/:baz where a route only fills :baz but not :bar. Probably not too big of a deal, but not very elegant. Are you familiar with how other frameworks handle this?

One expectation I have in the handler is that the parameter is present, so for this example, I would not expect the route to be matched if one of the parameters is absent.

I'm looking at Rails documentation, for which the router DSL is similar to Opium's and they have two kinds of parameters:

• :foo that is required and must be non-empty
• (:bar) that is optional and can be empty

We could do something similar?

Now considering the limitation you mention: if the parameter is optional, I would expect something/(:bar)/:foo to be invalid. But I'm not sure how Rails or other frameworks deal with it, I could make some research.

I am using full splat for serving files from an archive file: /:archive/file/**, and I think it's interesting that both /foo/file/README.txt and /foo/file/README.txt/ work.

That seems fine to me. ** should match anything. If the intended behaviour is to only match one segment, /:archive/file/* would make more sense no?