Allow middleware before routers

[aantron]

...or some other solution for: I'd like to partially percent-decode URLs before routing. At the moment, it seems that routing happens before any other middlewares are applied (didn't check the code; that's how I interpret Sihl allowing per-route-group middlewares, among other evidence).

In case of internationalized routes, this implies percent-encoding the routes on the app side so that Sihl's routing setup will select them. This is doable, but annoying, and ultimately pointless, since the app is percent-decoding variable URL components later anyway. It seems better to apply the decoding right away.

I just added a hacky percent-decoder to the Opium App created internally by Sihl, and it seems much better to use.

[joseferben]

Hey @aantron

Thanks for creating the issue, I had a chat with @aronerben about this. You are right about the order of routing.

In Sihl the API change could be something like

...
  ; Sihl.Web.register ~global:[pct_encode_middleware; Sihl.Web.Static.middleware] Web.Route.all

where we have the concept of a "global" middleware stack that is applied before the routing happens. What do you think?

[joseferben]

How did you add your own middleware to the Opium App if I may ask?

[aantron]

The API looks good to me. The optional arg could even be called middlewares, the same as for route groups. At least, that seems a bit more clear and consistent to me.

I added a middleware like this, near Sihl.Web.Http.start_server implementation:

let url_decode =
  Rock.Middleware.create
    ~filter:(fun service request ->
      service
        Rock.Request.{request with target = Uri.pct_decode request.target})
    ~name:"url_decode"
  |> Opium.App.middleware

let start_server () =
  (* ... *)
  let app = Opium.App.(empty |> url_decode |> port port_nr |> cmd_name "Sihl App") in
  let builders = routers_to_opium_builders !registered_routers in

Don't look too much at my actual middleware :) The decoder is a sloppy one for a quick prototype I am working on.

You probably already planned the same thing as I'm about to suggest — just create some builders from the global middlewares, and insert them before the router builders (as I'm already doing). I wasn't confident enough in Sihl to pass the argument in from register to here, yet, so I didn't open a PR.

[aantron]

This also seems necessary for the Sihl.Web.Static middleware. At the moment, the static middleware can only be triggered if the user also adds a dummy route for the static URI prefix. It needs to be processed before routing to avoid the need for that.

[aronerben]

@aantron Thanks for your report and your code snippet! The fix will be in the next release.

[aantron]

Thanks!