Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: other routers except the first one fails to match in some scenes #123

Closed
wants to merge 1 commit into from

Conversation

TooBug
Copy link

@TooBug TooBug commented Jul 1, 2021

1 Problem

In some cases, when defining multi routers, only the first router get matched, the others are invalid.

Code to confirm problem:

const Koa = require('koa');
const Router = require('@koa/router');

const app = new Koa();
const router1 = new Router();
const router2 = new Router();

router1.all(/.*/, async (ctx, next) => {
    console.log(ctx.path);
    console.log('match anything');
    await next();
});

router2.get('/test', async (ctx, next) => {
    console.log('match /test');
    ctx.body = 'match /test';
});

app.use(router1.routes());
app.use(router2.routes());

app.listen(3000);

Visit localhost:3000/test, will get a 404 response. And the logs:

/test
match anything

2 Course

After some work of debug, I found the problem:

In dispatch() method (middleware), It will set layer.path to ctx.routerPath, for the example above, it's /.*/ (RegExp), Then, the second router get the value, path becomes /.*/, but it should be /test which the request carries with.

Router.prototype.routes = Router.prototype.middleware = function () {
  const router = this;

  let dispatch = function dispatch(ctx, next) {
    debug('%s %s', ctx.method, ctx.path);



    // THIS LINE!!
    // the `path` may get value from `ctx.routerPath`
    const path = router.opts.routerPath || ctx.routerPath || ctx.path;





    const matched = router.match(path, ctx.method);
    let layerChain;

    if (ctx.matched) {
      ctx.matched.push.apply(ctx.matched, matched.path);
    } else {
      ctx.matched = matched.path;
    }

    ctx.router = router;

    if (!matched.route) return next();

    const matchedLayers = matched.pathAndMethod
    const mostSpecificLayer = matchedLayers[matchedLayers.length - 1]
    ctx._matchedRoute = mostSpecificLayer.path;
    if (mostSpecificLayer.name) {
      ctx._matchedRouteName = mostSpecificLayer.name;
    }

    layerChain = matchedLayers.reduce(function(memo, layer) {
      memo.push(function(ctx, next) {
        ctx.captures = layer.captures(path, ctx.captures);
        ctx.params = ctx.request.params = layer.params(path, ctx.captures, ctx.params);




        // THIS LINE!!
        // put the value `layer.path` to `ctx.routerPath`
        ctx.routerPath = layer.path;




        ctx.routerName = layer.name;
        ctx._matchedRoute = layer.path;
        if (layer.name) {
          ctx._matchedRouteName = layer.name;
        }
        return next();
      });
      return memo.concat(layer.stack);
    }, []);

    return compose(layerChain)(ctx, next);
  };

  dispatch.router = this;

  return dispatch;
};

3 More

This piece of code comes from #93 , which tries to fix #34 . I invested the issue, I think maybe the behavior before the fix has no problem, it should be the intended action. Maybe this needs more discussion.

The last thing, this PR breaks a test case, which tests ctx.routerPath should exists.

@3imed-jaberi
Copy link
Member

Fixed through this PR (#160)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The ctx. _matchedRoute is not the match route.
2 participants