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.

Sign up for GitHub

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

Jump to bottom

Fix Router.match pathAndMethod to only include layers that have methods #106

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix Router.match pathAndMethod to only include layers that have methods #106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 26 additions & 24 deletions lib/router.js
View file
Open in desktop
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
Open in desktop
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