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.

Sign up for GitHub

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

Jump to bottom

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

Open
Gary-Osteen-Q2 opened this issue Sep 23, 2020 · 6 comments · May be fixed by #106
Open

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

Gary-Osteen-Q2 opened this issue Sep 23, 2020 · 6 comments · May be fixed by #106

Comments

@Gary-Osteen-Q2
Copy link

Gary-Osteen-Q2 commented Sep 23, 2020 •
edited

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
Copy link
Author

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
Copy link
Contributor

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.

  • First Fork the koajs/router repo to your GitHub account
  • Commit your changes to your GitHub fork
  • Open a Pull Request so maintainers can review your changes and hopefully merge them

Hope this helps

Gary-Osteen-Q2 added a commit to unbill/router that referenced this issue Sep 23, 2020
@Gary-Osteen-Q2
(koajs#105) Fix Router.match pathAndMethod to only include layers tha…
a3c2e61 
…t have methods
@Gary-Osteen-Q2
Copy link
Author

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

@Gary-Osteen-Q2
Copy link
Author

@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.

@dominicegginton dominicegginton linked a pull request Oct 1, 2020 that will close this issue
Open
@jmealo
Copy link

jmealo commented Oct 29, 2020

@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:
open-telemetry/opentelemetry-js-contrib#222. Do either of you have some time to review this? I'd like to build upon it.

@dominicegginton
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix Router.match pathAndMethod to only include layers that have methods unbill/router
3 participants
@jmealo @dominicegginton @Gary-Osteen-Q2