From e1179605eaced9ca3b867b04524da5d9de9b8b8f Mon Sep 17 00:00:00 2001 From: Jean <110341611+jean-michelet@users.noreply.github.com> Date: Tue, 5 Mar 2024 18:52:31 +0100 Subject: [PATCH] Achieve 100% test coverage (#349) * test: Router module integration test: findMyWay.findRoute should also trows an error when wildcard is not the last character test: does not find the route if maxParamLength is exceeded refactor: make integration test more compliant with the existing code base test: cover uncovered constrainer.js test cases test: safeDecodeURIComponent should replace %3x to null for every x that is not a valid lowchar test: SemVerStore version should be a string test: httpMethodStrategy storage handles set and get operations correctly test: case insensitive static routes of level 1 for FindMyWay.findRoute test: findRoute normalizes wildcard patterns to require leading slash refactor: make tests more consistants with the existing test: should return null if no wildchar child Date: Tue Feb 20 14:10:18 2024 +0100 Achieves 100% test coverage * fix: add proxyquire to dev dependencies * test: cover uncovered branches in index.js * fix: revert 'if (nextCharCode === 58)' stmt to one line * fix: remove useless parametric node kind check * fix: if it is a wildchar node, it has a child * fix: a wildcard route created via the router api necessarily has a pattern with a leading slash * fix: non-custom strategy error only happen when user dont use the appropriate api * fix: there is no way found route params is not an array, expect if user break internals * test: Constrainer.noteUsage * Cannot derive constraints without active strategies. * test: should derive multiple async constraints * test: Major version must be a numeric value * test: SemVerStore.maxMajor should increase automatically * test: SemVerStore.maxPatches should increase automatically * refactor: httpMethods module should be a source of truth * refactor: make consistent use of fmw.on * fix: istanbul should ignore the complete function * test: remove 90 min limit for code coverage * fix: when no args is passed, undefined is default value * fix: add new line at the end of .taprc * fix: readd removed comment indicating static route --- .taprc | 4 - index.js | 60 +++---- lib/constrainer.js | 4 +- lib/node.js | 5 +- lib/strategies/accept-version.js | 8 +- lib/strategies/http-method.js | 5 +- package.json | 1 + test/constraint.custom.async.test.js | 10 +- test/issue-330.test.js | 232 +++++++++++++++++++++++++++ 9 files changed, 274 insertions(+), 55 deletions(-) create mode 100644 test/issue-330.test.js diff --git a/.taprc b/.taprc index 7f5f78fd..3cde38bf 100644 --- a/.taprc +++ b/.taprc @@ -1,7 +1,3 @@ ts: false jsx: false flow: false -branches: 90 -functions: 90 -lines: 90 -statements: 90 diff --git a/index.js b/index.js index dc34fc4a..21297bc5 100644 --- a/index.js +++ b/index.js @@ -344,7 +344,6 @@ Router.prototype.findRoute = function findNode (method, path, constraints = {}) const isRegexParam = charCode === 40 const isStaticPart = charCode === 45 || charCode === 46 const isEndOfNode = charCode === 47 || j === pattern.length - if (isRegexParam || isStaticPart || isEndOfNode) { const paramName = pattern.slice(lastParamStartIndex, j) params.push(paramName) @@ -407,9 +406,7 @@ Router.prototype.findRoute = function findNode (method, path, constraints = {}) // add the wildcard parameter params.push('*') currentNode = currentNode.getWildcardChild() - if (currentNode === null) { - return null - } + parentNodePathIndex = i + 1 if (i !== pattern.length - 1) { @@ -422,10 +419,6 @@ Router.prototype.findRoute = function findNode (method, path, constraints = {}) pattern = pattern.toLowerCase() } - if (pattern === '*') { - pattern = '/*' - } - for (const existRoute of this.routes) { const routeConstraints = existRoute.opts.constraints || {} if ( @@ -436,7 +429,7 @@ Router.prototype.findRoute = function findNode (method, path, constraints = {}) return { handler: existRoute.handler, store: existRoute.store, - params: existRoute.params || [] + params: existRoute.params } } } @@ -642,37 +635,36 @@ Router.prototype.find = function find (method, path, derivedConstraints) { continue } - if (currentNode.kind === NODE_TYPES.PARAMETRIC) { - let paramEndIndex = originPath.indexOf('/', pathIndex) - if (paramEndIndex === -1) { - paramEndIndex = pathLen - } + // parametric node + let paramEndIndex = originPath.indexOf('/', pathIndex) + if (paramEndIndex === -1) { + paramEndIndex = pathLen + } - let param = originPath.slice(pathIndex, paramEndIndex) - if (shouldDecodeParam) { - param = safeDecodeURIComponent(param) - } + let param = originPath.slice(pathIndex, paramEndIndex) + if (shouldDecodeParam) { + param = safeDecodeURIComponent(param) + } - if (currentNode.isRegex) { - const matchedParameters = currentNode.regex.exec(param) - if (matchedParameters === null) continue + if (currentNode.isRegex) { + const matchedParameters = currentNode.regex.exec(param) + if (matchedParameters === null) continue - for (let i = 1; i < matchedParameters.length; i++) { - const matchedParam = matchedParameters[i] - if (matchedParam.length > maxParamLength) { - return null - } - params.push(matchedParam) - } - } else { - if (param.length > maxParamLength) { + for (let i = 1; i < matchedParameters.length; i++) { + const matchedParam = matchedParameters[i] + if (matchedParam.length > maxParamLength) { return null } - params.push(param) + params.push(matchedParam) } - - pathIndex = paramEndIndex + } else { + if (param.length > maxParamLength) { + return null + } + params.push(param) } + + pathIndex = paramEndIndex } } @@ -742,8 +734,6 @@ for (const i in httpMethods) { const m = httpMethods[i] const methodName = m.toLowerCase() - if (Router.prototype[methodName]) throw new Error('Method already exists: ' + methodName) - Router.prototype[methodName] = function (path, handler, store) { return this.on(m, path, handler, store) } diff --git a/lib/constrainer.js b/lib/constrainer.js index 0cdffc69..6cd9df65 100644 --- a/lib/constrainer.js +++ b/lib/constrainer.js @@ -153,10 +153,8 @@ class Constrainer { if (!strategy.isCustom) { if (key === 'version') { lines.push(' version: req.headers[\'accept-version\'],') - } else if (key === 'host') { - lines.push(' host: req.headers.host || req.headers[\':authority\'],') } else { - throw new Error('unknown non-custom strategy for compiling constraint derivation function') + lines.push(' host: req.headers.host || req.headers[\':authority\'],') } } else { lines.push(` ${strategy.name}: this.strategies.${key}.deriveConstraint(req, ctx),`) diff --git a/lib/node.js b/lib/node.js index 0008d13c..f1daea7e 100644 --- a/lib/node.js +++ b/lib/node.js @@ -129,10 +129,7 @@ class StaticNode extends ParentNode { } getWildcardChild () { - if (this.wildcardChild) { - return this.wildcardChild - } - return null + return this.wildcardChild } createWildcardChild () { diff --git a/lib/strategies/accept-version.js b/lib/strategies/accept-version.js index 9ee39bc7..3a5f785d 100644 --- a/lib/strategies/accept-version.js +++ b/lib/strategies/accept-version.js @@ -20,7 +20,11 @@ SemVerStore.prototype.set = function (version, store) { } let [major, minor, patch] = version.split('.') - major = Number(major) || 0 + if (isNaN(major)) { + throw new TypeError('Major version must be a numeric value') + } + + major = Number(major) minor = Number(minor) || 0 patch = Number(patch) || 0 @@ -38,7 +42,7 @@ SemVerStore.prototype.set = function (version, store) { this.store[`${major}.x.x`] = store } - if (patch >= (this.store[`${major}.${minor}`] || 0)) { + if (patch >= (this.maxPatches[`${major}.${minor}`] || 0)) { this.maxPatches[`${major}.${minor}`] = patch this.store[`${major}.${minor}.x`] = store } diff --git a/lib/strategies/http-method.js b/lib/strategies/http-method.js index 3a6bdf2c..00e6a6b8 100644 --- a/lib/strategies/http-method.js +++ b/lib/strategies/http-method.js @@ -9,9 +9,6 @@ module.exports = { set: (type, store) => { handlers[type] = store } } }, - deriveConstraint: (req) => { - /* istanbul ignore next */ - return req.method - }, + deriveConstraint: /* istanbul ignore next */ (req) => req.method, mustMatchWhenDerived: true } diff --git a/package.json b/package.json index 99c35d22..b9161d37 100644 --- a/package.json +++ b/package.json @@ -40,6 +40,7 @@ "chalk": "^4.1.2", "inquirer": "^8.2.4", "pre-commit": "^1.2.2", + "proxyquire": "^2.1.3", "rfdc": "^1.3.0", "simple-git": "^3.7.1", "standard": "^14.3.4", diff --git a/test/constraint.custom.async.test.js b/test/constraint.custom.async.test.js index 3aef949a..c25c42ce 100644 --- a/test/constraint.custom.async.test.js +++ b/test/constraint.custom.async.test.js @@ -3,6 +3,7 @@ const t = require('tap') const test = t.test const FindMyWay = require('..') +const rfdc = require('rfdc')({ proto: true }) const customHeaderConstraint = { name: 'requestedBy', @@ -23,11 +24,14 @@ const customHeaderConstraint = { } } -test('should derive async constraint', t => { +test('should derive multiple async constraints', t => { t.plan(2) - const router = FindMyWay({ constraints: { requestedBy: customHeaderConstraint } }) - router.on('GET', '/', { constraints: { requestedBy: 'node' } }, () => 'asyncHandler') + const customHeaderConstraint2 = rfdc(customHeaderConstraint) + customHeaderConstraint2.name = 'requestedBy2' + + const router = FindMyWay({ constraints: { requestedBy: customHeaderConstraint, requestedBy2: customHeaderConstraint2 } }) + router.on('GET', '/', { constraints: { requestedBy: 'node', requestedBy2: 'node' } }, () => 'asyncHandler') router.lookup( { diff --git a/test/issue-330.test.js b/test/issue-330.test.js new file mode 100644 index 00000000..452511b8 --- /dev/null +++ b/test/issue-330.test.js @@ -0,0 +1,232 @@ +const t = require('tap') +const test = t.test +const FindMyWay = require('..') +const proxyquire = require('proxyquire') +const HandlerStorage = require('../lib/handler-storage') +const Constrainer = require('../lib/constrainer') +const { safeDecodeURIComponent } = require('../lib/url-sanitizer') +const acceptVersionStrategy = require('../lib/strategies/accept-version') +const httpMethodStrategy = require('../lib/strategies/http-method') + +test('FULL_PATH_REGEXP and OPTIONAL_PARAM_REGEXP should be considered safe', (t) => { + t.plan(1) + + t.doesNotThrow(() => require('..')) +}) + +test('should throw an error for unsafe FULL_PATH_REGEXP', (t) => { + t.plan(1) + + t.throws(() => proxyquire('..', { + 'safe-regex2': () => false + }), new Error('the FULL_PATH_REGEXP is not safe, update this module')) +}) + +test('Should throw an error for unsafe OPTIONAL_PARAM_REGEXP', (t) => { + t.plan(1) + + let callCount = 0 + t.throws(() => proxyquire('..', { + 'safe-regex2': () => { + return ++callCount < 2 + } + }), new Error('the OPTIONAL_PARAM_REGEXP is not safe, update this module')) +}) + +test('double colon does not define parametric node', (t) => { + t.plan(2) + + const findMyWay = FindMyWay() + + findMyWay.on('GET', '/::id', () => {}) + const route1 = findMyWay.findRoute('GET', '/::id') + t.strictSame(route1.params, []) + + findMyWay.on('GET', '/:foo(\\d+)::bar', () => {}) + const route2 = findMyWay.findRoute('GET', '/:foo(\\d+)::bar') + t.strictSame(route2.params, ['foo']) +}) + +test('case insensitive static routes', (t) => { + t.plan(3) + + const findMyWay = FindMyWay({ + caseSensitive: false + }) + + findMyWay.on('GET', '/foo', () => {}) + findMyWay.on('GET', '/foo/bar', () => {}) + findMyWay.on('GET', '/foo/bar/baz', () => {}) + + t.ok(findMyWay.findRoute('GET', '/FoO')) + t.ok(findMyWay.findRoute('GET', '/FOo/Bar')) + t.ok(findMyWay.findRoute('GET', '/fOo/Bar/bAZ')) +}) + +test('wildcard must be the last character in the route', (t) => { + t.plan(3) + + const expectedError = new Error('Wildcard must be the last character in the route') + + const findMyWay = FindMyWay() + + findMyWay.on('GET', '*', () => {}) + t.throws(() => findMyWay.findRoute('GET', '*1'), expectedError) + t.throws(() => findMyWay.findRoute('GET', '*/'), expectedError) + t.throws(() => findMyWay.findRoute('GET', '*?'), expectedError) +}) + +test('does not find the route if maxParamLength is exceeded', t => { + t.plan(2) + const findMyWay = FindMyWay({ + maxParamLength: 2 + }) + + findMyWay.on('GET', '/:id(\\d+)', () => {}) + + t.equal(findMyWay.find('GET', '/123'), null) + t.ok(findMyWay.find('GET', '/12')) +}) + +test('Should check if a regex is safe to use', (t) => { + t.plan(1) + + const findMyWay = FindMyWay() + + // we must pass a safe regex to register the route + // findRoute will still throws the expected assertion error if we try to access it with unsafe reggex + findMyWay.on('GET', '/test/:id(\\d+)', () => {}) + + const unSafeRegex = /(x+x+)+y/ + t.throws(() => findMyWay.findRoute('GET', `/test/:id(${unSafeRegex.toString()})`), { + message: "The regex '(/(x+x+)+y/)' is not safe!" + }) +}) + +test('Disable safe regex check', (t) => { + t.plan(1) + + const findMyWay = FindMyWay({ allowUnsafeRegex: true }) + + const unSafeRegex = /(x+x+)+y/ + findMyWay.on('GET', `/test2/:id(${unSafeRegex.toString()})`, () => {}) + t.doesNotThrow(() => findMyWay.findRoute('GET', `/test2/:id(${unSafeRegex.toString()})`)) +}) + +test('throws error if no strategy registered for constraint key', (t) => { + t.plan(2) + + const constrainer = new Constrainer() + const error = new Error('No strategy registered for constraint key invalid-constraint') + t.throws(() => constrainer.newStoreForConstraint('invalid-constraint'), error) + t.throws(() => constrainer.validateConstraints({ 'invalid-constraint': 'foo' }), error) +}) + +test('throws error if pass an undefined constraint value', (t) => { + t.plan(1) + + const constrainer = new Constrainer() + const error = new Error('Can\'t pass an undefined constraint value, must pass null or no key at all') + t.throws(() => constrainer.validateConstraints({ key: undefined }), error) +}) + +test('Constrainer.noteUsage', (t) => { + t.plan(3) + + const constrainer = new Constrainer() + t.equal(constrainer.strategiesInUse.size, 0) + + constrainer.noteUsage() + t.equal(constrainer.strategiesInUse.size, 0) + + constrainer.noteUsage({ host: 'fastify.io' }) + t.equal(constrainer.strategiesInUse.size, 1) +}) + +test('Cannot derive constraints without active strategies.', (t) => { + t.plan(1) + + const constrainer = new Constrainer() + const before = constrainer.deriveSyncConstraints + constrainer._buildDeriveConstraints() + t.sameStrict(constrainer.deriveSyncConstraints, before) +}) + +test('getMatchingHandler should return null if not compiled', (t) => { + t.plan(1) + + const handlerStorage = new HandlerStorage() + t.equal(handlerStorage.getMatchingHandler({ foo: 'bar' }), null) +}) + +test('safeDecodeURIComponent should replace %3x to null for every x that is not a valid lowchar', (t) => { + t.plan(1) + + t.equal(safeDecodeURIComponent('Hello%3xWorld'), 'HellonullWorld') +}) + +test('SemVerStore version should be a string', (t) => { + t.plan(1) + + const Storage = acceptVersionStrategy.storage + + t.throws(() => new Storage().set(1), new TypeError('Version should be a string')) +}) + +test('SemVerStore.maxMajor should increase automatically', (t) => { + t.plan(3) + + const Storage = acceptVersionStrategy.storage + const storage = new Storage() + + t.equal(storage.maxMajor, 0) + + storage.set('2') + t.equal(storage.maxMajor, 2) + + storage.set('1') + t.equal(storage.maxMajor, 2) +}) + +test('SemVerStore.maxPatches should increase automatically', (t) => { + t.plan(3) + + const Storage = acceptVersionStrategy.storage + const storage = new Storage() + + storage.set('2.0.0') + t.sameStrict(storage.maxPatches, { '2.0': 0 }) + + storage.set('2.0.2') + t.sameStrict(storage.maxPatches, { '2.0': 2 }) + + storage.set('2.0.1') + t.sameStrict(storage.maxPatches, { '2.0': 2 }) +}) + +test('Major version must be a numeric value', t => { + t.plan(1) + + const findMyWay = FindMyWay() + + t.throws(() => findMyWay.on('GET', '/test', { constraints: { version: 'x' } }, () => {}), + new TypeError('Major version must be a numeric value')) +}) + +test('httpMethodStrategy storage handles set and get operations correctly', (t) => { + t.plan(2) + + const storage = httpMethodStrategy.storage() + + t.equal(storage.get('foo'), null) + + storage.set('foo', { bar: 'baz' }) + t.strictSame(storage.get('foo'), { bar: 'baz' }) +}) + +test('if buildPrettyMeta argument is undefined, will return an object', (t) => { + t.plan(1) + + const findMyWay = FindMyWay() + t.sameStrict(findMyWay.buildPrettyMeta(), {}) +})