ZijianHe / koa-router

Router middleware for koa.
MIT License
4.85k stars 406 forks source link

Params are parsed for all matching routes, not just the first route. #292

Open pepkin88 opened 8 years ago

pepkin88 commented 8 years ago

Considering the following program:

'use strict'
var koa = require('koa')
var Router = require('koa-router')
var app = koa()
var router = new Router()

function* checkIdParam(next) {
    var {id} = this.params
    console.log(this.path, this.params) // here it shows {id: 'new'} for the path '/new'
    if ('id' in this.params && !/^[1-9]\d*$/.test(id)) this.throw(404)
    yield* next
}

router.use(checkIdParam)
router.get('/new', function* () {
    this.body = 'new'
})
router.get('/:id/edit', function* () {
    this.body = 'edit'
})
router.get('/:id', function* () {
    this.redirect(`${this.path}/edit${this.search}`)
})

app.use(router.routes())
app.listen(8080)

when I go to http://localhost:8080/new the app matches the /new route and the /:id route. But it runs param setters far all the matching routes, instead of just the first one. The result is, that although the path is /new (so it should match the /new route, since it is the first one), the checkIdParam middleware gets 'new' as an id param, which it shouldn't.

GavinOsborn commented 7 years ago

👍 I just ran into this myself when trying to implement some contextual middle-ware. I would really like to see this fixed.

jbielick commented 7 years ago

This is actually caused by #231

here's a failing test case for 7.x

  it('only runs param functions for matched route', function (done) {

    var app = new Koa();
    var router = new Router();

    router
      .get('/new', function (ctx, next) {
        ctx.body = {ran: ''};
        ctx.body.ran += '/new';
        return next();
      })
      .get('/:id', function (ctx, next) {
        ctx.body.ran += '/:id';
        return next();
      });

    app.use(router.routes());

    app.use(function(ctx, next) {
      ctx.body.params = ctx.params;
      // console.log(ctx._matchedRoute) // == /:id
      return next();
    });

    request(http.createServer(app.callback()))
      .get('/new')
      .expect(200)
      .end(function (err, res) {
        expect(res.body.ran).to.eql('/new/:id');
        expect(res.body.params).not.to.have.property('id');
        done(err);
      });

  });
jpravetz commented 7 years ago

I am seeing this same problem while working to add koa2 support to express-restify-mongoose. For that project I am testing with both koa-router and koa-better-router. The later of these is fully working at this point.

With REST, we have /customer/count and /customer/:id. koa-router v7.1.0 is using the last matching route, and middleware for both routes. It shouldn't be hard to fix this to take the first matching route and just the middleware for this route, and I'll try to fix in within the next couple of days.

However, I am wondering whether there is background (reasons) for the way it is now, and therefore a reason my PR wouldn't be accepted? I'd include the work along with my existing koa-router PR 334, which makes debugging these routes and spotting poorly assembled middleware chains a lot easier.