koajs / router

Router middleware for Koa. Maintained by @forwardemail and @ladjs.
MIT License
871 stars 176 forks source link

Router.match is returning pathAndMethod matches for layers without methods such as nested routers or middleware #105

Open Gary-Osteen-Q2 opened 4 years ago

Gary-Osteen-Q2 commented 4 years ago

Description When using nested routers and other middleware, the Router.match can include layers without methods in the matched.pathAndMethod array. This can cause issues downstream when setting the ctx._matchedRouteName, causing named routes to not be set properly. The assumption being that the last item in the pathAndMethod array being the most specific layer.

node.js version: v12.18.3 npm/yarn and version: 6.14.6 @koa/router version: 9.4.0 koa version: 2.13.0

Code sample:

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

const app = new Koa()

const router = new Router()
router.get('main#info', '/info', function() {})

const nestedRouter = new Router()
nestedRouter.get('nested#name', '/updates', function() {})

router.use('/v1/api', nestedRouter.routes(), nestedRouter.allowedMethods())

app.use(router.routes()).use(router.allowedMethods())

app.listen(8084)

Expected Behavior:

Only layers with path and methods should be included in matched.pathAndMethods array.

Actual Behavior:

Any layer which matches the given path will appear in matched.pathAndMethods array. Even if the layer has zero methods associated with them. Screen Shot 2020-09-23 at 2 57 15 PM

Gary-Osteen-Q2 commented 4 years ago

I have a branch with fixes. I just need to figure out what I need to do to get permission to push to here.

dominicegginton commented 4 years ago

Hi @Gary-Osteen-Q2, I havent had time to look into the issue you are having but it sounds like you have found a solution 😄. In order to push your fixes to this repo you will need to submit a Pull Request.

Hope this helps

Gary-Osteen-Q2 commented 4 years ago

I figured it out. First time contributing to open source. Learned a lot.

Gary-Osteen-Q2 commented 4 years ago

@dominicegginton Have you got a chance to look over my PR yet? I'm curious if it meets the standards and want to include a new version in the project I am using this in. I have a workaround for the time being. Thanks in advance for any insights and feedback.

jmealo commented 4 years ago

@dominicbarnes @niftylettuce: This is @Gary-Osteen-Q2's first open source contribution. I believe these changes are required/related to the following upstream issue for Koa's Open Telemetry instrumentation plugin: https://github.com/open-telemetry/opentelemetry-js-contrib/issues/222. Do either of you have some time to review this? I'd like to build upon it.

dominicegginton commented 4 years ago

@jmealo I have already approved #106, just waiting on a core contributor to take a look and possibly merge.