minor refactoring in router, clarifying names and complicating parameter objects

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 2bad3e2b509373d22279d6a866f1dbd1e57a11c8..a13ac47d62f083e588d047f7d556f7502f4b91e4 100644 (file)
--- 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<String|Router~ParamPart>} Router~SearchPath
-   * @property {Object} METHODS (symbol key)
+   * @typedef {Array<String|PathParameter>} 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<String>} 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 (file)
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 7cc92a04d984fb9cdac009b9ce672e55acb24691..d63917087d1490021d5441564669008c60f1676c 100644 (file)
--- 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 (file)
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