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] Match the right controller when multiple routes matches without calling next() #168

Open
3 tasks done
kravorkid opened this issue Jul 26, 2023 · 2 comments
Open
3 tasks done
Labels
bug Something isn't working

Comments

@kravorkid
Copy link

kravorkid commented Jul 26, 2023

Describe the bug

Node.js version:
Node v16.15.1

OS version:
MacOs Ventura Version 13.3.1 (a)

Description:
When multiple routes match, the router doesn't seems to execute the right controller without calling next().

Actual behavior

When not calling next() it execute the first controller and don't pass through the second, when next() is called both controllers are executed

Expected behavior

Without calling next() it should match the right route controller and execute it

Edit i've looked at the source code to see how it works under the hood i've seen the exclusive options that i have tested before on my use case but did not fit well with other of my controllers (that's an another story) but i have one question why execute the mostSpecificLayer should be an option ?
As stated in the documentation here if we want to execute multiple middleware we just have to chain them in the route definition

Code to reproduce

Here a test i've written (for now i've found a workaround by calling my route users/count before the route users/:id)

  it('Matches right controller when multiple routes match', function (done) {
    const app = new Koa();
    const router = new Router({ prefix: '/users' });
    
    router
      .get('/:id', function (ctx, next) {
        console.log('executed')
        ctx.body = { id: true };
        // next();
      })
      .get('/count', function (ctx, next) {
        ctx.body = { count: true };
      })


      request(http.createServer(app.use(router.routes(), router.allowedMethods()).callback()))
      .get('/users/count')
      .expect(200)
      .end(function (err, res) {
        if (err) return done(err);
        expect(res.body).to.have.property('count', true);
        done();
      });
  })

Checklist

  • I have searched through GitHub issues for similar issues.
  • I have completely read through the README and documentation.
  • I have tested my code with the latest version of Node.js and this package and confirmed it is still not working.
@kravorkid kravorkid added the bug Something isn't working label Jul 26, 2023
@sombriks
Copy link

hi @kravorkid did you found the solution or just a workaround to avoid the bug? do you mind if i open a pull request for this issue in the future if needed?

@kravorkid
Copy link
Author

@sombriks the workaround i've found its to call the named routes before the route with the params, you can open a pull request for this.
I've looked deeper in the source code for what i remember it looked like both controllers we're matched and the ctx.body was filled with both properties id and count

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants