From 9fe1e9b22b8e753c44dec77d1dee3d0061b2e991 Mon Sep 17 00:00:00 2001 From: Justin Wind Date: Wed, 27 Apr 2022 17:58:45 -0700 Subject: [PATCH] minor refactoring in router, clarifying names and complicating parameter objects --- lib/{router.js => router/index.js} | 102 +++++++++++++---------------- lib/router/path-parameter.js | 49 ++++++++++++++ test/lib/router.js | 93 +++++++++++++------------- test/lib/router/path-parameter.js | 51 +++++++++++++++ 4 files changed, 194 insertions(+), 101 deletions(-) rename lib/{router.js => router/index.js} (67%) create mode 100644 lib/router/path-parameter.js create mode 100644 test/lib/router/path-parameter.js diff --git a/lib/router.js b/lib/router/index.js similarity index 67% rename from lib/router.js rename to lib/router/index.js index 2bad3e2..a13ac47 100644 --- a/lib/router.js +++ b/lib/router/index.js @@ -6,12 +6,12 @@ */ const { METHODS: httpMethods } = require('http'); -const common = require('./common'); -const { DingusError } = require('./errors'); +const common = require('../common'); +const { DingusError } = require('../errors'); +const PathParameter = require('./path-parameter'); // Internal identifiers for route entries. -const METHODS = Symbol('METHODS'); -const PARAM = Symbol('PARAM'); +const kPathMethods = Symbol('kSqueepDingusRouterPathMethods'); const defaultOptions = { ignoreTrailingSlash: false, @@ -30,8 +30,6 @@ const defaultOptions = { * mapping of methods to handler functions. * * @property {Object} pathsByLength index to registered paths by number of parts - * @property {Symbol} METHODS key to method:handler map on search paths - * @property {Symbol} PARAM key to parameter name in search path parts */ class Router { /** @@ -46,57 +44,50 @@ class Router { this.pathsByLength = { 1: [], }; - - this.METHODS = METHODS; - this.PARAM = PARAM; } + /** - * @typedef {Object} Router~ParamPart - * @property {String} PARAM (symbol key) - */ - /** - * @typedef {Array} Router~SearchPath - * @property {Object} METHODS (symbol key) + * @typedef {Array} Router~RoutePath + * @property {Object} kPathMethods (symbol key) */ /** * Prepare a path for insertion into search list. - * A searchable path is a list of path parts, with a property of an object of method handlers. - * @param {String} pathDefinition - * @returns {Router~SearchPath} + * A route path is an Array of path parts, with a symbolic property of an object mapping method handlers. + * @param {String} rawPath + * @returns {Router~RoutePath} * @private */ - _pathDefinitionToPathMatch(pathDefinition) { - const pathMatch = pathDefinition + _pathToRoutePath(rawPath) { + const routePath = rawPath .split('/') - .map((p) => p.startsWith(this.paramPrefix) ? { [PARAM]: p.slice(this.paramPrefix.length) } : p); + .map((p) => p.startsWith(this.paramPrefix) ? new PathParameter(p.slice(this.paramPrefix.length)) : p); if (this.ignoreTrailingSlash - && pathMatch[pathMatch.length - 1] === '') { - pathMatch.pop(); + && routePath[routePath.length - 1] === '') { + routePath.pop(); } - pathMatch[METHODS] = {}; - pathMatch.forEach((p) => Object.freeze(p)); - Object.freeze(pathMatch); - return pathMatch; + routePath[kPathMethods] = {}; + Object.freeze(routePath); + return routePath; } /** * Compare checkPath to fixedPath, no param substitution, params must match. - * @param {Router~SearchPath} fixedPath - * @param {Router~SearchPath} checkPath + * @param {Router~RoutePath} routePath + * @param {Router~RoutePath} checkPath * @returns {Boolean} * @private */ - static _pathCompareExact(fixedPath, checkPath) { - if (fixedPath.length !== checkPath.length) { + static _pathCompareExact(routePath, checkPath) { + if (routePath.length !== checkPath.length) { return false; } - for (let i = 0; i < fixedPath.length; i++) { - const fixedPart = fixedPath[i]; + for (let i = 0; i < routePath.length; i++) { + const fixedPart = routePath[i]; const checkPart = checkPath[i]; - if (typeof fixedPart === 'object' && typeof checkPart === 'object') { - if (fixedPart[PARAM] !== checkPart[PARAM]) { + if (fixedPart instanceof PathParameter && checkPart instanceof PathParameter) { + if (fixedPart.parameter !== checkPart.parameter) { return false; } } else if (fixedPart !== checkPart) { @@ -108,24 +99,24 @@ class Router { /** - * Compare checkPath to fixedPath, populating params. - * @param {Router~SearchPath} fixedPath + * Compare routePath to fixedPath, populating params. + * @param {Router~RoutePath} routePath * @param {Array} checkPath * @param {Object} returnParams * @returns {Boolean} * @private */ - static _pathCompareParam(fixedPath, checkPath, returnParams = {}) { + static _pathCompareParam(routePath, checkPath, returnParams = {}) { const params = {}; - if (fixedPath.length !== checkPath.length) { + if (routePath.length !== checkPath.length) { return false; } - for (let i = 0; i < fixedPath.length; i++) { - const fixedPart = fixedPath[i]; + for (let i = 0; i < routePath.length; i++) { + const fixedPart = routePath[i]; const checkPart = checkPath[i]; - if (typeof fixedPart === 'object') { - params[fixedPart[PARAM]] = checkPart; + if (fixedPart instanceof PathParameter) { + params[fixedPart.parameter] = checkPart; } else if (fixedPart !== checkPart) { return false; } @@ -138,7 +129,7 @@ class Router { /** * @typedef Router~MatchedPath * @property {Object} pathParams populated param fields - * @property {Router~SearchPath} matchedPath + * @property {Router~RoutePath} matchedPath */ /** * Search for an existing path, return matched path and path parameters. @@ -166,15 +157,15 @@ class Router { /** * Return a matching path, no param substitution, params must match - * @param {Router~SearchPath} matchParts - * @returns {Router~SearchPath=} + * @param {Router~RoutePath} routePath + * @returns {Router~RoutePath=} * @private */ - _pathFindExact(matchParts) { - const pathsByLength = this.pathsByLength[matchParts.length]; + _pathFindExact(routePath) { + const pathsByLength = this.pathsByLength[routePath.length]; if (pathsByLength) { for (const p of pathsByLength) { - if (Router._pathCompareExact(p, matchParts)) { + if (Router._pathCompareExact(p, routePath)) { return p; } } @@ -199,7 +190,7 @@ class Router { * @param {any[]} handlerArgs */ on(methods, urlPath, handler, handlerArgs = []) { - const matchParts = this._pathDefinitionToPathMatch(urlPath); + const matchParts = this._pathToRoutePath(urlPath); let existingPath = this._pathFindExact(matchParts); if (!existingPath) { existingPath = matchParts; @@ -218,7 +209,7 @@ class Router { if (!httpMethods.includes(method) && method !== '*') { throw new DingusError(`invalid method '${method}'`); } - existingPath[METHODS][method] = { handler, handlerArgs }; + existingPath[kPathMethods][method] = { handler, handlerArgs }; }); } @@ -243,11 +234,11 @@ class Router { ctx.params = pathParams; if (matchedPath) { ctx.matchedPath = matchedPath; - if (method in matchedPath[METHODS]) { - return matchedPath[METHODS][method]; + if (method in matchedPath[kPathMethods]) { + return matchedPath[kPathMethods][method]; } - if ('*' in matchedPath[METHODS]) { - return matchedPath[METHODS]['*']; + if ('*' in matchedPath[kPathMethods]) { + return matchedPath[kPathMethods]['*']; } throw new DingusError('NoMethod'); } @@ -257,5 +248,6 @@ class Router { } +Router.kPathMethods = kPathMethods; module.exports = Router; diff --git a/lib/router/path-parameter.js b/lib/router/path-parameter.js new file mode 100644 index 0000000..585aaec --- /dev/null +++ b/lib/router/path-parameter.js @@ -0,0 +1,49 @@ +'use strict'; + +const kPathParameter = Symbol('kSqueepDingusRouterPathParameter'); +const parameters = new Map(); +/** + * De-duplicating factory of minimal-objects to track the named-parameter parts of path definitions. + * + * @property {String} parameter + */ +class PathParameter extends null { + constructor(parameter) { + if (!parameter || typeof(parameter) !== 'string') { + throw new RangeError('parameter must be string'); + } + if (parameters.has(parameter)) { + return parameters.get(parameter); + } + const pathParameter = Object.create(PathParameter.prototype); + pathParameter[kPathParameter] = parameter; // eslint-disable-line security/detect-object-injection + parameters.set(parameter, pathParameter); + Object.freeze(pathParameter); + return pathParameter; + } + + /** + * Return the parameter name. + */ + get parameter() { + return this[kPathParameter];// eslint-disable-line security/detect-object-injection + } + + /** + * @returns {String} + */ + toString() { + return `{${this.constructor.name} ${this.parameter}}`; // eslint-disable-line security/detect-object-injection + } + + /** + * Clear the de-duplication table, for tests. + */ + static _flush() { + this.parameters.clear(); + } +} +PathParameter.kPathParameter = kPathParameter; +PathParameter.parameters = parameters; + +module.exports = PathParameter; \ No newline at end of file diff --git a/test/lib/router.js b/test/lib/router.js index 7cc92a0..d639170 100644 --- a/test/lib/router.js +++ b/test/lib/router.js @@ -5,6 +5,7 @@ const assert = require('assert'); const sinon = require('sinon'); // eslint-disable-line node/no-unpublished-require const Router = require('../../lib/router'); +const PathParameter = require('../../lib/router/path-parameter') const { DingusError } = require('../../lib/errors'); const noExpectedException = 'did not get expected exception'; @@ -22,61 +23,61 @@ describe('Router', function () { router.ignoreTrailingSlash = _its; }); - describe('_pathDefinitionToPathMatch', function () { + describe('_pathToRoutePath', function () { it('defines a simple path', function () { const p = '/a/b/c'; const expected = ['', 'a', 'b', 'c']; - expected[router.METHODS] = {}; - const r = router._pathDefinitionToPathMatch(p); + expected[Router.kPathMethods] = {}; + const r = router._pathToRoutePath(p); assert.deepStrictEqual(r, expected); }); it('defines a path with parameter', function () { const p = '/a/b/:c'; - const expected = ['', 'a', 'b', { [router.PARAM]: 'c' }]; - expected[router.METHODS] = {}; - const r = router._pathDefinitionToPathMatch(p); + const expected = ['', 'a', 'b', new PathParameter('c')]; + expected[Router.kPathMethods] = {}; + const r = router._pathToRoutePath(p); assert.deepStrictEqual(r, expected); }); it('defines a path with trailing slash', function () { router.ignoreTrailingSlash = true; const p = '/a/b/:c/'; - const expected = ['', 'a', 'b', { [router.PARAM]: 'c' }]; - expected[router.METHODS] = {}; - const r = router._pathDefinitionToPathMatch(p); + const expected = ['', 'a', 'b', new PathParameter('c')]; + expected[Router.kPathMethods] = {}; + const r = router._pathToRoutePath(p); assert.deepStrictEqual(r, expected); }); - }); // _pathDefinitionToPathMatch + }); // _pathToRoutePath describe('_pathCompareExact', function () { let fixedPath, checkPath; it('compares static paths which match', function () { - fixedPath = router._pathDefinitionToPathMatch('/a/b/c'); - checkPath = router._pathDefinitionToPathMatch('/a/b/c'); + fixedPath = router._pathToRoutePath('/a/b/c'); + checkPath = router._pathToRoutePath('/a/b/c'); const r = Router._pathCompareExact(fixedPath, checkPath); assert.strictEqual(r, true); }); it('compares static paths which do not match', function () { - fixedPath = router._pathDefinitionToPathMatch('/a/b/c'); - checkPath = router._pathDefinitionToPathMatch('/a/b/d'); + fixedPath = router._pathToRoutePath('/a/b/c'); + checkPath = router._pathToRoutePath('/a/b/d'); const r = Router._pathCompareExact(fixedPath, checkPath); assert.strictEqual(r, false); }); it('compares unequal static paths', function () { - fixedPath = router._pathDefinitionToPathMatch('/a/b/c'); - checkPath = router._pathDefinitionToPathMatch('/a/b'); + fixedPath = router._pathToRoutePath('/a/b/c'); + checkPath = router._pathToRoutePath('/a/b'); const r = Router._pathCompareExact(fixedPath, checkPath); assert.strictEqual(r, false); }); it('compares param paths which match', function () { - fixedPath = router._pathDefinitionToPathMatch('/a/:b/c'); - checkPath = router._pathDefinitionToPathMatch('/a/:b/c'); + fixedPath = router._pathToRoutePath('/a/:b/c'); + checkPath = router._pathToRoutePath('/a/:b/c'); const r = Router._pathCompareExact(fixedPath, checkPath); assert.strictEqual(r, true); }); it('compares param paths which do not match', function () { - fixedPath = router._pathDefinitionToPathMatch('/a/:b/c'); - checkPath = router._pathDefinitionToPathMatch('/a/:g/c'); + fixedPath = router._pathToRoutePath('/a/:b/c'); + checkPath = router._pathToRoutePath('/a/:g/c'); const r = Router._pathCompareExact(fixedPath, checkPath); assert.strictEqual(r, false); }); @@ -88,8 +89,8 @@ describe('Router', function () { it('compares static paths which match', function () { const params = {}; const expectedParams = {}; - fixedPath = router._pathDefinitionToPathMatch('/a/b/c'); - checkPath = router._pathDefinitionToPathMatch('/a/b/c'); + fixedPath = router._pathToRoutePath('/a/b/c'); + checkPath = router._pathToRoutePath('/a/b/c'); const r = Router._pathCompareParam(fixedPath, checkPath); assert.strictEqual(r, true); assert.deepStrictEqual(params, expectedParams); @@ -97,8 +98,8 @@ describe('Router', function () { it('compares static paths which do not match', function () { const params = {}; const expectedParams = {}; - fixedPath = router._pathDefinitionToPathMatch('/a/b/c'); - checkPath = router._pathDefinitionToPathMatch('/a/b/d'); + fixedPath = router._pathToRoutePath('/a/b/c'); + checkPath = router._pathToRoutePath('/a/b/d'); const r = Router._pathCompareParam(fixedPath, checkPath, params); assert.strictEqual(r, false); assert.deepStrictEqual(params, expectedParams); @@ -106,8 +107,8 @@ describe('Router', function () { it('compares unequal static paths', function () { const params = {}; const expectedParams = {}; - fixedPath = router._pathDefinitionToPathMatch('/a/b/c'); - checkPath = router._pathDefinitionToPathMatch('/a/b'); + fixedPath = router._pathToRoutePath('/a/b/c'); + checkPath = router._pathToRoutePath('/a/b'); const r = Router._pathCompareParam(fixedPath, checkPath, params); assert.strictEqual(r, false); assert.deepStrictEqual(params, expectedParams); @@ -117,8 +118,8 @@ describe('Router', function () { const expectedParams = { b: 'bar', }; - fixedPath = router._pathDefinitionToPathMatch('/a/:b/c'); - checkPath = router._pathDefinitionToPathMatch('/a/bar/c'); + fixedPath = router._pathToRoutePath('/a/:b/c'); + checkPath = router._pathToRoutePath('/a/bar/c'); const r = Router._pathCompareParam(fixedPath, checkPath, params); assert.strictEqual(r, true); assert.deepStrictEqual(params, expectedParams); @@ -129,8 +130,8 @@ describe('Router', function () { b: 'gaz', c: '123', }; - fixedPath = router._pathDefinitionToPathMatch('/a/:b/:c'); - checkPath = router._pathDefinitionToPathMatch('/a/gaz/123'); + fixedPath = router._pathToRoutePath('/a/:b/:c'); + checkPath = router._pathToRoutePath('/a/gaz/123'); const r = Router._pathCompareParam(fixedPath, checkPath, params); assert.strictEqual(r, true); assert.deepStrictEqual(params, expectedParams); @@ -143,8 +144,8 @@ describe('Router', function () { beforeEach(function () { pathsByLengthOrig = router.pathsByLength; router.pathsByLength = { - 2: [ router._pathDefinitionToPathMatch('/:id') ], - 3: [ router._pathDefinitionToPathMatch('/a/b') ], + 2: [ router._pathToRoutePath('/:id') ], + 3: [ router._pathToRoutePath('/a/b') ], }; }); afterEach(function () { @@ -167,7 +168,7 @@ describe('Router', function () { describe('_pathFindExact', function () { it('finds a path', function () { - const pathParts = ['', { [router.PARAM]: 'id' }]; + const pathParts = ['', new PathParameter('id')]; const r = router._pathFindExact(pathParts); assert.strictEqual(r, router.pathsByLength[2][0]); }); @@ -191,7 +192,7 @@ describe('Router', function () { beforeEach(function () { pathsByLengthOrig = router.pathsByLength; router.pathsByLength = { - 2: [ router._pathDefinitionToPathMatch('/:id') ], + 2: [ router._pathToRoutePath('/:id') ], }; }); afterEach(function () { @@ -200,17 +201,17 @@ describe('Router', function () { it('adds new path', function () { const urlPath = '/a/:id'; - const expected = router._pathDefinitionToPathMatch(urlPath); - expected[router.METHODS]['GET'] = stubEntry; + const expected = router._pathToRoutePath(urlPath); + expected[Router.kPathMethods]['GET'] = stubEntry; router.on('GET', urlPath, stubHandler); assert.deepStrictEqual(router.pathsByLength[3][0], expected); }); it('adds new method to path', function () { const urlPath = '/a/:id'; - const expected = router._pathDefinitionToPathMatch(urlPath); - expected[router.METHODS]['GET'] = stubEntry; - expected[router.METHODS]['POST'] = stubEntry; + const expected = router._pathToRoutePath(urlPath); + expected[Router.kPathMethods]['GET'] = stubEntry; + expected[Router.kPathMethods]['POST'] = stubEntry; router.on('GET', urlPath, stubHandler); router.on('POST', urlPath, stubHandler); @@ -219,8 +220,8 @@ describe('Router', function () { it('add some more paths', function () { let urlPath = '/a/b/c/d'; - const expected = router._pathDefinitionToPathMatch(urlPath); - expected[router.METHODS]['GET'] = stubEntry; + const expected = router._pathToRoutePath(urlPath); + expected[Router.kPathMethods]['GET'] = stubEntry; router.on('GET', urlPath, stubHandler); urlPath = '/a/b/x/y'; router.on('GET', urlPath, stubHandler); @@ -230,9 +231,9 @@ describe('Router', function () { it('adds multiple methods', function () { const urlPath = '/:id'; - const expected = router._pathDefinitionToPathMatch(urlPath); - expected[router.METHODS]['GET'] = stubEntry; - expected[router.METHODS]['HEAD'] = stubEntry; + const expected = router._pathToRoutePath(urlPath); + expected[Router.kPathMethods]['GET'] = stubEntry; + expected[Router.kPathMethods]['HEAD'] = stubEntry; router.on(['GET', 'HEAD'], urlPath, stubHandler); assert.deepStrictEqual(router.pathsByLength[2][0], expected); @@ -240,8 +241,8 @@ describe('Router', function () { it('adds new wildcard path', function () { const urlPath = '/a/:id'; - const expected = router._pathDefinitionToPathMatch(urlPath); - expected[router.METHODS]['*'] = stubEntry; + const expected = router._pathToRoutePath(urlPath); + expected[Router.kPathMethods]['*'] = stubEntry; router.on('*', urlPath, stubHandler); assert.deepStrictEqual(router.pathsByLength[3][0], expected); }); diff --git a/test/lib/router/path-parameter.js b/test/lib/router/path-parameter.js new file mode 100644 index 0000000..9b2fb1e --- /dev/null +++ b/test/lib/router/path-parameter.js @@ -0,0 +1,51 @@ +/* eslint-env mocha */ +'use strict'; + +const assert = require('assert'); +const PathParameter = require('../../../lib/router/path-parameter'); + +const noExpectedException = 'did not receive expected exception'; +describe('PathParameter', function () { + beforeEach(function () { + PathParameter._flush(); + }); + it('requires a parameter', function () { + try { + new PathParameter(); + assert.fail(noExpectedException); + } catch (e) { + assert(e instanceof RangeError, noExpectedException); + } + }); + it('requires parameter be a string', function () { + try { + new PathParameter({}); + assert.fail(noExpectedException); + } catch (e) { + assert(e instanceof RangeError, noExpectedException); + } + }); + it('creates a parameter object', function () { + const p = new PathParameter('foo'); + assert(p instanceof PathParameter); + assert.strictEqual(p.parameter, 'foo'); + }); + it('duplicate parameters are the same object', function () { + const p1 = new PathParameter('foo'); + const p2 = new PathParameter('foo'); + assert.strictEqual(p1, p2); + }); + it('shows itself', function () { + const p = new PathParameter('foo'); + assert(p.toString().includes('foo')); + }); + it('parameters are immutable', function () { + const p = new PathParameter('foo'); + try { + p[PathParameter.kPathParameter] = 'bar'; + assert.fail(noExpectedException); + } catch (e) { + assert(e instanceof TypeError, noExpectedException); + } + }); +}); // PathParameter \ No newline at end of file -- 2.43.2