Only keep looping matching route middlewares if they yield next

[ilkkao]

No description provided.

[ilkkao]

This will execute routes one after another instead of true cascading like Koa

[alexmingoia]

@ilkkao Route middleware is meant to be run sequentially as it uses koa-compose. I'm not sure I understand why route matching should be controlled by anything other than the URI? I'm struggling to see the use case for multiple route matching in general.

[jeromew]

I have also have a hard time understanding the use case for multiple matching routes.

As a matter of fact, I was going to ask for a feature where koa-router could throw an error if multiple routes are matched. @ikkao what are you using the multiple route matching for ?

currently if someone composes a koa-router middleware with another middleware with

compose(router.middleware(), otherMiddleware)

then otherMiddleware will be potentially called N times which is not correct from my understanding since otherMiddleware does not expect to be called N times for the same request in the koa habits.

I would be interested in understanding the real use case behind this + either one of :

• have a configuration flag so that koa-router throws an exception when multiple routes are matched
• have a configuration flag so that koa-router can become first-matched route only
• maybe remove the mutli-route handling if we can find another solution for @ilkkao's use case

[alexmingoia]

@jeromew I'm planning on refactoring the router for 3.x and removing the multiple route matching, along with moving Resource into a separate module: https://github.com/alexmingoia/koa-router/issues?milestone=2&page=1&state=open

[ilkkao]

The idea was to have something like:

app.get(/^/api/.*/, authenticator);

and then

app.get('/api/endpoint1/:xyz', endpoint1Controller);
app.get('/api/endpoint2/:xyz', endpoint2Controller);
...

instead of

app.get('/api/endpoint1/:xyz', authenticator, endpoint1Controller);
app.get('/api/endpoint2/:xyz', authenticator, endpoint2Controller);
...

But yeah, even currently, authenticator can only set context variable that needs to be checked in endpoint controllers.

If this multi route matching can't be patched to work satisfactory or doesn't make sense to you, then by all means, feel free to take it out.

[jeromew]

Interesting.

beware that I would not rely on the order of koa-router route matching. The algo may change so you could very well end up with

app.get('/api/endpoint1/:xyz', endpoint1Controller);
app.get(/^/api/.*/, authenticator);

instead of the expected

app.get(/^/api/.*/, authenticator);
app.get('/api/endpoint1/:xyz', endpoint1Controller);

as of now I think it would be more prudent to always stack the mws as

app.get('/api/endpoint1/:xyz', authenticator, endpoint1Controller);

If you are afraid to forget 1 authenticator somewhere and since I don't know if koa-router is going to add a wrapper for authentified routes but you could start by monkey patching app.get with

var _get = app.get.bind(app)
var authenticator = function*() {}
app.get = function(path, mw) {
   if (test on /api/) {
   _get(path, authenticator, mw);
  } else {
   _get(path, mw);
  }
}

that is monkey-patching, but it looks a lot safer to me than multi-route matching.

I hope this will help.

[ilkkao]

@jeromew Order of Koa (and Express) middlewares matter. The key idea in Koa is that a middleware can decide whether or not to process the downstream middlewares. This PR and the discussion started from @rkusa's example which would not work if the order was random. See: #32 (comment)

I have now updated the PR and I'm happy with the solution. Router takes all the matching middlewares, wraps them so that this.params is set correctly for every of them. Then it creates one composed middleware and executes that.

This way the matching routes work like Koa middlewares. If all them yield next then the next Koa middleware after router is run if it exists. If there is a router middleware without yield next then the whole Koa middleware execution stops there. This feels natural to me.

In general multi route matching adds more flexibility in my opinion. You can have common routes that do all kinds of preprocessing (yield next case) before handling the request in the controller. Or they can validate/reject request (no yield next case) before it reaches the controller. Something that is well supported in Rails.

[rkusa]

This way the matching routes work like Koa middlewares. If all them yield next then the next Koa middleware after router is run if it exists. If there is a router middleware without yield next then the whole Koa middleware execution stops there. This feels natural to me.

@ilkkao I would agree that this feels natural. At least to a certain point. An example, I am not sure how I would expect its outcome.

var api = new Router()
api.get('/api/...', function*(next) {
   ...
   yield next
})
api.get(/^\/api.*$/i, function*() {
  // catch all, for error page or SPA
  // do not yield
})

app.use(api.middleware())

app.use(function*() {
  // e.g. add API related headers
})

When injecting a router as a middleware itself, I think I would expect yield next to leave this middleware and call the immediately following Koa middleware instead calling the next matching route. What do you guys think?

As a side note, @ilkkao's example could be solved using koa-mount or by doing the following:

var public = new Router()
public.get(...)

app.use(public.middleware())

var api = new Router()
api.get('/api/endpoint1/:xyz', endpoint1Controller);
api.get('/api/endpoint2/:xyz', endpoint2Controller);

app.use(authenticator)
app.use(api.middleware())

However, I have no doubt that there are usecases where multiple matching routes are useful and where no other clean solution exists.

[jeromew]

@ilkkao the new solution of composing all the matched middlewares looks better but I sitll have a hard time to think that this would not bite us a some point.

1/ as far as I understand it, koa-router does not yet have a policy regarding the order of route matching. I am convinced that this is very dangerous to assume. As soon as an optimization will take place (look at what was done with Journey in rails) the order will change and you can get busted.

2/ I think that it is not the 'authentification' use case that must drive the multi-route setup. There are other solutions for this. You can add the mw on each route manually, I proposed monkey-patching, @rkusa gives another option. Probably koa-router will propose an integrated solution in the future when a nice API is found.

3/ you say that rails has multi-route matching. Do you have a documentation link ? I only found devise that approaches this but it looks to be authentication oriented. The only link I found was http://stackoverflow.com/questions/5786725/rails-3-routing-how-to-match-multiple ; but I am not a rails expert and would be interested in knowing more about the solution you are talking about.

4/ finding an API for prepending mw to a set of routes is a +. koa-router could implement nested routes (a kind of koa-router-mount) that would swallow parts of the path in exchange of the systematic application of a mw.
5/ I still have a hard time with multi-route matching.

get('/part1/:part', mw1)
get('/:part/part1', mw2)

would both get matched by '/part1/part1' which would be a problem. In situations where you have a lot of middleware, collisions like this can happen and can be very hard to track down.

There are of course use cases where you want to prepend or append a mw to all paths that match a pattern (authentification, throttling, logging, probably others) so koa-router should probably have an API for this.

In fact you can maybe already do what you want koa-router (in pseudo code, not tested):

prepend = new Router()
prepend.get('/api/', authenticate)
router = new Router()
router.get ....
append = new Router()
append.get('/dangerous/', send_email)

app.use( compose(prepend.middleware(), router.middleware() ,append.middleware()) )

do you think this would work ?

[alexmingoia]

I have released 3.0.0 of koa-router. It does not match multiple routes, and I think that ultimately matching multiple routes is not a good design for a number of reasons:

• Regular expressions are meant to be used for finer control over parameter matching and basic URL parameter validation - not running multiple functions for a given URL, which is the purpose of route middleware.
• URL's are unique, so conceptually it is odd to have separate declarations that match the same URL, and it becomes difficult to reason about which one matches first because you must make assumptions about koa-router's matching internals.
• It overlaps with the purpose of route middleware; there is no unique functionality that requires matching multiple routes, even if you have to structure your application differently.

It's not the API direction I prefer for koa-router.

[ilkkao]

Just in case somebody finds this useful, I'm keeping the multi route matching patch here: ilkkao@ba2669c. It's one commit on top of official koa-router.