From a3c2e61141f19dd106183106ebc1b8f6ee9d8319 Mon Sep 17 00:00:00 2001 From: Gary Osteen Date: Tue, 22 Sep 2020 11:18:59 -0500 Subject: [PATCH] (#105) Fix Router.match pathAndMethod to only include layers that have methods --- lib/router.js | 50 +++++++++--------- test/lib/router.js | 125 +++++++++++++++++++++++++++++++++++++-------- 2 files changed, 129 insertions(+), 46 deletions(-) diff --git a/lib/router.js b/lib/router.js index 4c27578..7b3f2e9 100644 --- a/lib/router.js +++ b/lib/router.js @@ -63,7 +63,7 @@ function Router(opts) { this.params = {}; this.stack = []; -}; +} /** * Create `router.verb()` methods, where *verb* is one of the HTTP verbs such @@ -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); @@ -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(', ')); @@ -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(', ')); @@ -661,8 +659,7 @@ Router.prototype.url = function (name, params) { * * @param {String} path * @param {String} method - * @returns {Object.} returns layers that matched path and - * path and method. + * @returns {Object.} returns layers broken down by how the path and methods were matched. * @private */ @@ -670,8 +667,10 @@ Router.prototype.match = function (path, method) { const layers = this.stack; let layer; const matched = { - path: [], + pathOnly: [], pathAndMethod: [], + pathNotMethod: [], + allMatchedPaths: [], route: false }; @@ -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; }; @@ -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) { diff --git a/test/lib/router.js b/test/lib/router.js index 71c601a..6568d04 100644 --- a/test/lib/router.js +++ b/test/lib/router.js @@ -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(); @@ -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) { @@ -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'); @@ -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' }); @@ -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' }); @@ -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(); }); @@ -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(); @@ -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); @@ -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' }; }); @@ -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() }) @@ -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; @@ -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' } ); @@ -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; }); @@ -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(); @@ -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(); })