Skip to content

Commit

Permalink
(koajs#105) Fix Router.match pathAndMethod to only include layers tha…
Browse files Browse the repository at this point in the history 
…t have methods
  • Loading branch information
@Gary-Osteen-Q2
Gary-Osteen-Q2 committed Sep 23, 2020
1 parent 56735f0 commit a3c2e61
Show file tree
Hide file tree
Showing 2 changed files with 129 additions and 46 deletions.
50 changes: 26 additions & 24 deletions lib/router.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ function Router(opts) {

this.params = {};
this.stack = [];
};
}

/**
* Create `router.verb()` methods, where *verb* is one of the HTTP verbs such
Expand Down Expand Up @@ -343,23 +343,25 @@ Router.prototype.routes = Router.prototype.middleware = function () {
let layerChain;

if (ctx.matched) {
ctx.matched.push.apply(ctx.matched, matched.path);
ctx.matched.push.apply(ctx.matched, matched.allMatchedPaths);
} else {
ctx.matched = matched.path;
ctx.matched = matched.allMatchedPaths;
}

ctx.router = router;

if (!matched.route) return next();

const matchedLayers = matched.pathAndMethod
const mostSpecificLayer = matchedLayers[matchedLayers.length - 1]
ctx._matchedRoute = mostSpecificLayer.path;
if (mostSpecificLayer.name) {
ctx._matchedRouteName = mostSpecificLayer.name;
const mostSpecificLayer = matched.pathAndMethod[matched.pathAndMethod.length - 1]
if (mostSpecificLayer) {
ctx._matchedRoute = mostSpecificLayer.path;
if (mostSpecificLayer.name) {
ctx._matchedRouteName = mostSpecificLayer.name;
}
}

layerChain = matchedLayers.reduce(function(memo, layer) {
const pathChain = [...matched.pathOnly, ...matched.pathAndMethod]
layerChain = pathChain.reduce(function(memo, layer) {
memo.push(function(ctx, next) {
ctx.captures = layer.captures(path, ctx.captures);
ctx.params = ctx.request.params = layer.params(path, ctx.captures, ctx.params);
Expand Down Expand Up @@ -446,11 +448,9 @@ Router.prototype.allowedMethods = function (options) {

if (!~implemented.indexOf(ctx.method)) {
if (options.throw) {
let notImplementedThrowable = (typeof options.notImplemented === 'function')
throw (typeof options.notImplemented === 'function')
? options.notImplemented() // set whatever the user returns from their function
: new HttpError.NotImplemented();

throw notImplementedThrowable;
} else {
ctx.status = 501;
ctx.set('Allow', allowedArr.join(', '));
Expand All @@ -462,11 +462,9 @@ Router.prototype.allowedMethods = function (options) {
ctx.set('Allow', allowedArr.join(', '));
} else if (!allowed[ctx.method]) {
if (options.throw) {
let notAllowedThrowable = (typeof options.methodNotAllowed === 'function')
throw (typeof options.methodNotAllowed === 'function')
? options.methodNotAllowed() // set whatever the user returns from their function
: new HttpError.MethodNotAllowed();

throw notAllowedThrowable;
} else {
ctx.status = 405;
ctx.set('Allow', allowedArr.join(', '));
Expand Down Expand Up @@ -661,17 +659,18 @@ Router.prototype.url = function (name, params) {
*
* @param {String} path
* @param {String} method
* @returns {Object.<path, pathAndMethod>} returns layers that matched path and
* path and method.
* @returns {Object.<pathOnly, pathAndMethod, pathNotMethod, allMatchedPaths, route>} returns layers broken down by how the path and methods were matched.
* @private
*/

Router.prototype.match = function (path, method) {
const layers = this.stack;
let layer;
const matched = {
path: [],
pathOnly: [],
pathAndMethod: [],
pathNotMethod: [],
allMatchedPaths: [],
route: false
};

Expand All @@ -681,15 +680,19 @@ Router.prototype.match = function (path, method) {
debug('test %s %s', layer.path, layer.regexp);

if (layer.match(path)) {
matched.path.push(layer);

if (layer.methods.length === 0 || ~layer.methods.indexOf(method)) {
matched.pathAndMethod.push(layer);
if (layer.methods.length) matched.route = true;
if (layer.methods.length !== 0 && ~layer.methods.indexOf(method)) {
matched.pathAndMethod.push(layer)
matched.route = true
} else if(layer.methods.length === 0 || method === 'OPTIONS') {
matched.pathOnly.push(layer)
} else {
matched.pathNotMethod.push(layer)
}
}
}

matched.allMatchedPaths = [...matched.pathOnly, ...matched.pathAndMethod, ...matched.pathNotMethod]

return matched;
};

Expand Down Expand Up @@ -744,7 +747,6 @@ Router.prototype.param = function(param, middleware) {
* ```
*
* @param {String} path url pattern
* @param {Object} params url parameters
* @returns {String}
*/
Router.url = function (path) {
Expand Down
125 changes: 103 additions & 22 deletions test/lib/router.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ const assert = require('assert');

describe('Router', function () {
it('creates new router with koa app', function (done) {
const app = new Koa();
const router = new Router();
router.should.be.instanceOf(Router);
done();
Expand Down Expand Up @@ -79,7 +78,7 @@ describe('Router', function () {
});
});

it('router can be accecced with ctx', function (done) {
it('router can be accessed with ctx', function (done) {
const app = new Koa();
const router = new Router();
router.get('home', '/', function (ctx) {
Expand Down Expand Up @@ -158,7 +157,6 @@ describe('Router', function () {
});

it('exposes middleware factory', function (done) {
const app = new Koa();
const router = new Router();
router.should.have.property('routes');
router.routes.should.be.type('function');
Expand Down Expand Up @@ -265,7 +263,6 @@ describe('Router', function () {

it('nests routers with prefixes at root', function (done) {
const app = new Koa();
const api = new Router();
const forums = new Router({
prefix: '/forums'
});
Expand Down Expand Up @@ -316,7 +313,6 @@ describe('Router', function () {

it('nests routers with prefixes at path', function (done) {
const app = new Koa();
const api = new Router();
const forums = new Router({
prefix: '/api'
});
Expand Down Expand Up @@ -809,7 +805,7 @@ describe('Router', function () {
.end(function (err, res) {
if (err) return done(err);
res.header.should.have.property('allow', 'HEAD, GET');
let allowHeaders = res.res.rawHeaders.filter((item) => item == 'Allow');
let allowHeaders = res.res.rawHeaders.filter((item) => item === 'Allow');
expect(allowHeaders.length).to.eql(1);
done();
});
Expand Down Expand Up @@ -990,7 +986,7 @@ describe('Router', function () {

});

describe('Router#use()', function (done) {
describe('Router#use()', function () {
it('uses router middleware without path', function (done) {
const app = new Koa();
const router = new Router();
Expand Down Expand Up @@ -1266,7 +1262,7 @@ describe('Router', function () {
const router = new Router();
router.should.have.property('register');
router.register.should.be.type('function');
const route = router.register('/', ['GET', 'POST'], function () {});
router.register('/', ['GET', 'POST'], function () {});
app.use(router.routes());
router.stack.should.be.an.instanceOf(Array);
router.stack.should.have.property('length', 1);
Expand Down Expand Up @@ -1309,7 +1305,6 @@ describe('Router', function () {

describe('Router#route()', function () {
it('inherits routes from nested router', function () {
const app = new Koa();
const subrouter = Router().get('child', '/hello', function (ctx) {
ctx.body = { hello: 'world' };
});
Expand All @@ -1318,7 +1313,6 @@ describe('Router', function () {
});

it('should return false if no name matches', function () {
const app = new Koa()
const value = Router().route('Picard')
value.should.be.false()
})
Expand Down Expand Up @@ -1400,7 +1394,6 @@ describe('Router', function () {
});

it('generates URL for given route name with params and query params', function(done) {
const app = new Koa();
const router = new Router();
router.get('books', '/books/:category/:id', function (ctx) {
ctx.status = 204;
Expand All @@ -1423,22 +1416,22 @@ describe('Router', function () {
})

it('generates URL for given route name without params and query params', function(done) {
var router = new Router();
const router = new Router();
router.get('books', '/books', function (ctx) {
ctx.status = 204;
});
var url = router.url('books');
let url = router.url('books');
url.should.equal('/books');
var url = router.url('books');
url = router.url('books');
url.should.equal('/books', {});
var url = router.url('books');
url = router.url('books');
url.should.equal('/books', {}, {});
var url = router.url('books',
url = router.url('books',
{},
{ query: { page: 3, limit: 10 } }
);
url.should.equal('/books?page=3&limit=10');
var url = router.url('books',
url = router.url('books',
{},
{ query: 'page=3&limit=10' }
);
Expand All @@ -1448,7 +1441,7 @@ describe('Router', function () {


it('generates URL for given route name without params and query params', function(done) {
var router = new Router();
const router = new Router();
router.get('category', '/category', function (ctx) {
ctx.status = 204;
});
Expand All @@ -1473,6 +1466,97 @@ describe('Router', function () {
});
});

describe('Router#match()', function () {
let matchRouter
before(function () {
matchRouter = new Router()
matchRouter.get('/updates', function (ctx) {})
matchRouter.post('/updates', function (ctx) {})

const nestedRouterOne = new Router()
nestedRouterOne.get('/firstpath', function (ctx) {})
nestedRouterOne.get('/secondpath', function (ctx) {})

const nestedRouterTwo = new Router()
nestedRouterTwo.get('/firstpath', function (ctx) {})
nestedRouterTwo.post('/firstpath', function (ctx) {})
nestedRouterTwo.get('/secondpath', function (ctx) {})

nestedRouterOne.use('/secondnest', nestedRouterTwo.routes(), nestedRouterTwo.allowedMethods())
matchRouter.use('/firstnest', nestedRouterOne.routes(), nestedRouterOne.allowedMethods())
})

it('no matches', function(done) {
const matched = matchRouter.match('/bogus/path', 'GET')

matched.should.deepEqual({
pathOnly: [],
pathAndMethod: [],
pathNotMethod: [],
allMatchedPaths: [],
route: false
})
done()
})

it('match for OPTIONS method', function(done) {
const matched = matchRouter.match('/updates', 'OPTIONS')

matched.pathOnly.length.should.equal(2)
matched.pathOnly[0].path.should.equal('/updates')
matched.pathOnly[0].methods.should.deepEqual(['HEAD', 'GET'])
matched.pathOnly[1].path.should.equal('/updates')
matched.pathOnly[1].methods.should.deepEqual(['POST'])

matched.pathNotMethod.should.deepEqual([])

matched.pathNotMethod.should.deepEqual([])

matched.allMatchedPaths.length.should.equal(2)

matched.route.should.equal(false)
done()
})

it('route matched is false', function(done) {
const matched = matchRouter.match('/firstnest/firstpath', 'POST')

matched.pathOnly.length.should.equal(1)
matched.pathOnly[0].path.should.equal('/firstnest')

matched.pathAndMethod.length.should.equal(0)

matched.pathNotMethod.length.should.equal(1)
matched.pathNotMethod[0].path.should.equal('/firstnest/firstpath')

matched.allMatchedPaths.length.should.equal(2)

matched.route.should.equal(false)
done()
})

it('route matched is true', function(done) {
const matched = matchRouter.match('/firstnest/secondnest/firstpath', 'GET')

matched.pathOnly.length.should.equal(2)
matched.pathOnly[0].path.should.equal('/firstnest/secondnest')
matched.pathOnly[1].path.should.equal('/firstnest')

matched.pathAndMethod.length.should.equal(1)
matched.pathAndMethod[0].path.should.equal('/firstnest/secondnest/firstpath')
matched.pathAndMethod[0].methods.should.deepEqual(['HEAD', 'GET'])

matched.pathNotMethod.length.should.equal(1)
matched.pathNotMethod[0].path.should.equal('/firstnest/secondnest/firstpath')
matched.pathNotMethod[0].methods.should.deepEqual(['POST'])

matched.allMatchedPaths.length.should.equal(4)

matched.route.should.equal(true)
done()
})
})

describe('Router#param()', function () {
it('runs parameter middleware', function (done) {
const app = new Koa();
Expand Down Expand Up @@ -1511,10 +1595,7 @@ describe('Router', function () {
return next();
})
.param('first', function (id, ctx, next) {
ctx.ranFirst = true;
if (ctx.user) {
ctx.ranFirst = false;
}
ctx.ranFirst = !ctx.user;
if (!id) return ctx.status = 404;
return next();
})
Expand Down

0 comments on commit a3c2e61

Please sign in to comment.