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

The ctx. _matchedRoute is not the match route. #34

Closed
BainDragneel opened this issue Nov 4, 2019 · 11 comments · Fixed by #93
Closed

The ctx. _matchedRoute is not the match route. #34

BainDragneel opened this issue Nov 4, 2019 · 11 comments · Fixed by #93

Comments

@BainDragneel
Copy link

node.js version: v13.0.1

npm/yarn and version: 6.12.0

koa-router version: 7.4.0 and i tried @koa/router v8.0.2, too.

koa version: 2.7.0

Code sample:

Now I have two routes, one is "/api/order/list" and another is "/api/order/:orderId" ,

const Router = require("koa-router");
const router = new Router({
    "prefix": "/api",
});
router.get('/order/list',controller.getOrderList);
router.get('/order/:orderId',controller.getOrder);

I want to log each request.I want the url I am recording to be the url of my own defined api, not the url directly with params.For example, when I request "http://127.0.0.1:3000/api/order/57d52c52403f2ee865738ec7", I want to record that the url is "/api/order/:orderId" instead of "/api/order/57d52c52403f2ee865738ec7",And when I request "http://127.0.0.1:3000/api/order/list", I want to record that the url is "/api/order/list" instead of "/api/order/:orderId".
But ctx. _matchedRoute is not the route I requested.

Expected Behavior:

when I request "http://127.0.0.1:3000/api/order/list", ctx. _matchedRoute is "/api/order/:orderId"

Actual Behavior:

ctx. _matchedRoute is "/api/order/list"

Additional steps, HTTP request details, or to reproduce the behavior or a test case:

i have the four screenshots.

http:127.0.0.1:3000/api/order/list

image
ctx.matched
image

http:127.0.0.1:3000/api/order/:orderId

image
ctx.matched
image

@niftylettuce
Copy link
Contributor

Can you submit a PR that has a failing test case? This is a good find, thank you!

@kroleg
Copy link

kroleg commented Jan 20, 2020

This is not a bug, it's a feature (kind of). Here is @koa/router code (lib/router.js:342):

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

@BainDragneel in your case matchedLayers look like: [/, /order/list, /order/:orderId] and /order/:orderId is just most specific route.
There is a soluition. You may specifiy regexp for id param like /order/:orderId/:id([0-9]+) but in this case _matchedRoute will equal to /order/:orderId/:id([0-9]+) for url /order/57d52c52403f2ee865738ec7

I can think of better solution. Express allows to specifiy regexp restriction for route param like router.param('orderId', /d+/). koa router's .param can also accept regexp as second param like
.param('orderId', /\d+/, handleOrderIdParam) @niftylettuce i can work on a PR implementing this

@falkenhawk
Copy link

falkenhawk commented Jun 30, 2020

@kroleg This is a bug, see https://github.com/ZijianHe/koa-router/labels/first%20matched%20route
The first matched route should be used for executing its and only its middlewares. (not the last, not the most generic, etc. and definitely not multiple routes - I cannot imagine a case when executing multiple routes per one request would be a desirable and expected result)

Allegedly it was fixed on the original repo for "8.x branch": ZijianHe/koa-router#231 (comment)

but then it must have been lost somewhere along the transition to this repo, as the works on 8.x were completely discarded and instead v8.0.0 was released with just a package name change: 94698a3
(And btw the unfinished 8.x branch still exists on this forked repo: https://github.com/koajs/router/commits/8.x)

I am currently on @koa/router v9.0.1 and still multiple sets of middlewares are executed when multiple paths are "matched", instead just stopping at the first path matched.

And there was also that PR on the original repo which stalled and got closed ZijianHe/koa-router#345 (possibly before the v8.x rewrite). Maybe that could be based on to fix this bug.

I am not asking to continue working on the 8.x branch, but only to acknowledge this issue (as reported here and in multiple issues on the original repo) as a real bug and unintended behaviour. That might hopefully encourage someone with enough time at hand to pick it up and file a PR.

@niftylettuce
Copy link
Contributor

If anyone makes a PR I will award them a bug bounty, payable over PayPal.

@falkenhawk
Copy link

falkenhawk commented Jun 30, 2020

@niftylettuce I've found this PR on the original repo which possibly fixes this issue: ZijianHe/koa-router#525

and also something somewhat related: ZijianHe/koa-router#492
(but the latter is addressing an issue from another set of issues - https://github.com/ZijianHe/koa-router/issues?q=is%3Aopen+is%3Aissue+label%3A%22nested+routes+bug%22 - which apparently were also more or less addressed in 8.x)

@niftylettuce
Copy link
Contributor

Can someone review @mdenisov PR #93? cc @falkenhawk

@mdenisov if you're confident about it lmk

@mdenisov
Copy link

@niftylettuce I transferred this PR from the original repo and this fix solves this problem. Works great on my project :)

@niftylettuce
Copy link
Contributor

@mdenisov can you please send me your PayPal or preferred means of receiving a donation for your bug bounty award here? niftylettuce@gmail.com

@mdenisov
Copy link

@niftylettuce It is not necessary, thanks :)

@niftylettuce
Copy link
Contributor

@koa/router v9.1.0 released to npm and mirrored to koa-router as well

https://github.com/koajs/router/releases/tag/v9.1.0

@BainDragneel
Copy link
Author

Thanks very much, I have some things I did not pay attention to this issue in time, sorry!

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