koajs / router

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

[fix] Match the right controller when multiple routes matches without calling next() #168

Closed kravorkid closed 3 weeks ago

kravorkid commented 1 year ago

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

sombriks commented 1 year ago

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 commented 1 year ago

@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

3imed-jaberi commented 3 weeks ago

Hello guys, you can archive the asked behavior by marking the exclusive option as true.

    const router = new Router({ exclusive: true });