Bug: 2 overlapping routes on the same level are executed

[jeromew]

Hello,

I tried to upgrade from 3.1.0 to 5.3.0 and ran into a problem which I think is a backward incompatibility issue. I am not sure if it is now a feature or a bug.

When you have 2 conflicting routes, koa-router used to select one (the first defined) and go with that route. Now, koa-router follow both routes.

After some tests, the problem seem to have appeared between 5.1.3 and 5.2.0. Here is a quick explanation of the problem :

var co = require('co');
var Router = require('koa-router');

var r = Router();

r.get('/my/:path/1', function*(next){ console.log('route #1'); yield next})
r.get('/my/:path/:id', function*(next){ console.log('route #2'); yield next})

var ctx = { path: '/my/path/1', method: 'GET' }
var mw = co.wrap(r.middleware())
mw.call(ctx,function*(){}()).then(function(res) { console.log('done')}, function(err) {console.log(err)})

Launched with 5.1.2, it gives

route #1
done

Launched with 5.2.0 it gives

route #1
route #2
done

Launched with 5.3.0 it gives

route #1
route #2
done

I currently rely on the fact that only one route get executed in such a scenario.

Is this a bug or should I rewrite my routes differently ?

multiple route matching seems incompatible with #41 (comment) so I wonder if this is a major evolution of the design related the nesting feature or just a bug.

The release note on 5.2 - 8f30c9c does not seem to mention this.

[jeromew]

The same observation is done for koa-router@next

var r = require('koa-router')();


r.get('/my/:path/1', function(ctx, next){ console.log('route #1'); next() })
r.get('/my/:path/:id', function(ctx, next){ console.log('route #2'); next()})

var ctx = { path: '/my/path/1', method: 'GET' }
var mw = r.middleware()
mw(ctx,function(){}).then(function(res) { console.log('done')}, function(err) {console.log(err)})

leads to

route #1
route #2
done

[jeromew]

I modified an existing test to show a reproduction of the multiple matching route bug

[alexmingoia]

Yes, this is a regression. The intended behavior is: the more specific route matches, then the route that was registered first. Ideally it would throw an error when registering overlapping routes, but that would incur more backwards-incompatibility.

[jeromew]

The code has changed a lot since I looked at it in 3.1.0, with the layers and inheritance. I could take a look at it to see how we could modify things, but does koa-router already have "order matching tests" where all these ordering rules are tested ?

I just looked rapidly at the tests but did not find something clearly identified as such + I am not sure I understand how orderding should work among a "native" route like "/a/b/c" and a route composed with multiple routers like "/a" ("/b/c")

[msmesnik]

Any ETA on a fix for this issue? Just like @jeromew I too am relying on only one route being executed, and I'd rather not stick with my hacky workaround for any length of time.
Or will this behaviour stick?

[Bekt]

Has anybody found a workaround for this? Any thoughts on this @alexmingoia?

I've looked at other non-Koa routers, and they all seem to act they way you described above.

[jeromew]

@jbielick I saw your comment on #292 regarding this. I am trying to see how it could be fixed.

As I understand it, the current code is trying to mix 2 features :
(1) - match only one route, "the more specific route matches, then the route that was registered first" as @alexmingoia said above
(2) - allow for "shared" middlewares on mount points. This is what router.use('/mount', mw) seem to be used for

(1) was working and when (2) was introduced it created backward incompatibility because the algorithm forgot to take (1) into account.

Now this was more than a year ago and people seem to be trying to make usage of this bug in unexpected ways (leading to more bugs) - cf issue #311.

Looking at the code, here is a fix that may help fix (1) while keeping (2) : among all matching routes keep "the more specific route matches, then the route that was registered first" + keep all the matching routes which have the end: false param value. Indeed, end: false is used only in the (2) case.

After that, what will be missing I think is a cristal clear priority rule mechanism in koa-router that could be tested and to which people could be directed to avoid issues.

Here are some docs I extracted from hapi router - https://hapijs.com/api - for comparison

Path matching order

The router iterates through the routing table on each incoming request and executes the first (and only the first) matching route. Route matching is done based on the combination of the request path and the HTTP verb (e.g. 'GET, 'POST'). The query is excluded from the routing logic. Requests are matched in a deterministic order where the order in which routes are added does not matter.

Routes are matched based on the specificity of the route which is evaluated at each segment of the incoming request path. Each request path is split into its segment (the parts separated by '/'). The segments are compared to the routing table one at a time and are matched against the most specific path until a match is found. If no match is found, the next match is tried.

When matching routes, string literals (no path parameter) have the highest priority, followed by mixed parameters ('/a{p}b'), parameters ('/{p}'), and then wildcard (/{p*}).

Note that mixed parameters are slower to compare as they cannot be hashed and require an array iteration over all the regular expressions representing the various mixed parameter at each routing table node.

Tell me what you think

[mikew]

Is this fixed in the upcoming 8.0 branch? It looks like it is, but I'm not sure. There's so many issues related to this one thing: https://github.com/alexmingoia/koa-router/blob/c0e9e088cd679f3c4030b73d8c0fc7b69e1ffecb/test/matched.test.js#L64-L73

[jbielick]

@mikew it is addressed in the 8.x branch

[landau]

Any ETA on this? This feels like a bug that should patched separately from 8.x. Why delay this bug fix? It has been dangling for 2 1/2 years. Thank you.

[jbielick]

@landau Would you like to pull request a fix? There is no one working on this library full-time.

[drdozer]

I've just been bitten by this :( I have /goi/create and /goi/:goiId and both are being executed which leads to properly weird behaviour. Is this fixed, or is there a work-around?

"koa-router": "^7.4.0"

[OFRBG]

Note: You need to make sure that more specific routes are matched first! For convenience, all my endpoints are resources with a prefix:

/logs
 - GET /
 - GET /all
 - GET /:id

By compiling middleware in order, /all is matched before /:id, and the debouncer below avoids resetting ctx.body and ctx.status.

---

The debouncing middleware:

/**
 * Debounce router calls for multiple matches
 */
export const debounceRouter: Koa.Middleware = async (ctx, next) => {
  ctx.state.firstMatch = _.isNil(ctx.state.firstMatch);

  if (!ctx.state.firstMatch) {
    return; // terminate middleware chain
  }

  await next();
};

Make sure the router middleware runs functions as debounceRouter -> [your middleware].

[crazyyi]

I've seen weird behaviors too. Maybe I misunderstood the usage.
But when I have something like

router.del("/books/:id", async (cxt, next) => {});

and

router.post("/books", async (ctx, next) => {});

at the same time, I can't even complete a DELETE request using the curl -X DELETE command. It will give me a 405 Method Not Allowed error. The error will only go away if I change one of the above paths to a different path string.