From a070da3502156f651566efd47acc5cf31f358713 Mon Sep 17 00:00:00 2001 From: Paul Melnikow Date: Tue, 5 Mar 2019 01:22:28 -0500 Subject: [PATCH] Add query param validation to remaining new-style services [azuredevops appveyor npm] Remove now-obsolete code. Close #2675 --- core/base-service/base.js | 4 +- core/base-service/route.js | 6 +- core/base-service/route.spec.js | 3 +- services/appveyor/appveyor-tests.service.js | 15 +-- .../azure-devops-tests.service.js | 12 +-- services/npm/npm-base.js | 2 + .../npm/npm-dependency-version.service.js | 4 +- services/test-results.js | 91 +++++++++++++++++++ services/test-results.spec.js | 51 +++++++++++ services/text-formatters.js | 77 ---------------- services/text-formatters.spec.js | 44 --------- 11 files changed, 160 insertions(+), 149 deletions(-) create mode 100644 services/test-results.js create mode 100644 services/test-results.spec.js diff --git a/core/base-service/base.js b/core/base-service/base.js index 2b7352bec143f..30bcc09c8c995 100644 --- a/core/base-service/base.js +++ b/core/base-service/base.js @@ -106,9 +106,7 @@ module.exports = class BaseService { * - capture: Array of names for the capture groups in the regular * expression. The handler will be passed an object containing * the matches. - * - queryParams: Array of names for query parameters which will the service - * uses. For cache safety, only the whitelisted query - * parameters will be passed to the handler. + * - queryParamSchema: Joi schema for valid query params. */ static get route() { throw new Error(`Route not defined for ${this.name}`) diff --git a/core/base-service/route.js b/core/base-service/route.js index 688b7ef430c7d..ecae022e686e0 100644 --- a/core/base-service/route.js +++ b/core/base-service/route.js @@ -18,10 +18,8 @@ const isValidRoute = Joi.object({ then: Joi.array().items(Joi.string().required()), }), queryParamSchema: Joi.object().schema(), - queryParams: Joi.array().items(Joi.string().required()), }) .xor('pattern', 'format') - .oxor('queryParamSchema', 'queryParams') .required() function assertValidRoute(route, message = undefined) { @@ -71,12 +69,12 @@ function namedParamsForMatch(captureNames = [], match, ServiceClass) { return result } -function getQueryParamNames({ queryParams = [], queryParamSchema }) { +function getQueryParamNames({ queryParamSchema }) { if (queryParamSchema) { const { children, renames = [] } = Joi.describe(queryParamSchema) return Object.keys(children).concat(renames.map(({ from }) => from)) } else { - return queryParams + return [] } } diff --git a/core/base-service/route.spec.js b/core/base-service/route.spec.js index 86607976cf715..b3f9c2e23c6b0 100644 --- a/core/base-service/route.spec.js +++ b/core/base-service/route.spec.js @@ -14,7 +14,7 @@ describe('Route helpers', function() { const { regex, captureNames } = prepareRoute({ base: 'foo', pattern: ':namedParamA', - queryParams: ['queryParamA'], + queryParamSchema: Joi.object({ queryParamA: Joi.string() }).required(), }) const regexExec = str => regex.exec(str) @@ -108,7 +108,6 @@ describe('Route helpers', function() { }) it('getQueryParamNames', function() { - expect(getQueryParamNames({ queryParams: ['foo'] })).to.deep.equal(['foo']) expect( getQueryParamNames({ queryParamSchema: Joi.object({ foo: Joi.string() }).required(), diff --git a/services/appveyor/appveyor-tests.service.js b/services/appveyor/appveyor-tests.service.js index 0f7a1fdcbff34..df5a95318cc78 100644 --- a/services/appveyor/appveyor-tests.service.js +++ b/services/appveyor/appveyor-tests.service.js @@ -1,7 +1,9 @@ 'use strict' -const Joi = require('joi') -const { renderTestResultBadge } = require('../text-formatters') +const { + testResultQueryParamSchema, + renderTestResultBadge, +} = require('../test-results') const AppVeyorBase = require('./appveyor-base') const documentation = ` @@ -20,18 +22,11 @@ const documentation = `

` -const queryParamSchema = Joi.object({ - compact_message: Joi.equal(''), - passed_label: Joi.string(), - failed_label: Joi.string(), - skipped_label: Joi.string(), -}).required() - module.exports = class AppVeyorTests extends AppVeyorBase { static get route() { return { ...this.buildRoute('appveyor/tests'), - queryParamSchema, + queryParamSchema: testResultQueryParamSchema, } } diff --git a/services/azure-devops/azure-devops-tests.service.js b/services/azure-devops/azure-devops-tests.service.js index 8663da3d12a46..52d43983ebc9e 100644 --- a/services/azure-devops/azure-devops-tests.service.js +++ b/services/azure-devops/azure-devops-tests.service.js @@ -1,7 +1,10 @@ 'use strict' const Joi = require('joi') -const { renderTestResultBadge } = require('../text-formatters') +const { + testResultQueryParamSchema, + renderTestResultBadge, +} = require('../test-results') const AzureDevOpsBase = require('./azure-devops-base') const { getHeaders } = require('./azure-devops-helpers') @@ -177,12 +180,7 @@ module.exports = class AzureDevOpsTests extends AzureDevOpsBase { return { base: 'azure-devops/tests', pattern: ':organization/:project/:definitionId/:branch*', - queryParams: [ - 'compact_message', - 'passed_label', - 'failed_label', - 'skipped_label', - ], + queryParamSchema: testResultQueryParamSchema, } } diff --git a/services/npm/npm-base.js b/services/npm/npm-base.js index 326455f465f65..21b9547b40253 100644 --- a/services/npm/npm-base.js +++ b/services/npm/npm-base.js @@ -131,3 +131,5 @@ module.exports = class NpmBase extends BaseJsonService { return this.constructor._validate(packageData, packageDataSchema) } } + +module.exports.queryParamSchema = queryParamSchema diff --git a/services/npm/npm-dependency-version.service.js b/services/npm/npm-dependency-version.service.js index da4f08bdfc12e..5a13b83004b3e 100644 --- a/services/npm/npm-dependency-version.service.js +++ b/services/npm/npm-dependency-version.service.js @@ -3,6 +3,7 @@ const { getDependencyVersion } = require('../package-json-helpers') const NpmBase = require('./npm-base') +const { queryParamSchema } = NpmBase const keywords = ['node'] module.exports = class NpmDependencyVersion extends NpmBase { @@ -11,11 +12,10 @@ module.exports = class NpmDependencyVersion extends NpmBase { } static get route() { - const { queryParams } = this.buildRoute('') return { base: 'npm/dependency-version', pattern: ':scope(@[^/]+)?/:packageName/:kind(dev|peer)?/:dependency', - queryParams, + queryParamSchema, } } diff --git a/services/test-results.js b/services/test-results.js new file mode 100644 index 0000000000000..ff752c1f17c45 --- /dev/null +++ b/services/test-results.js @@ -0,0 +1,91 @@ +'use strict' + +const Joi = require('joi') + +const testResultQueryParamSchema = Joi.object({ + compact_message: Joi.equal(''), + passed_label: Joi.string(), + failed_label: Joi.string(), + skipped_label: Joi.string(), +}).required() + +function renderTestResultMessage({ + passed, + failed, + skipped, + total, + passedLabel, + failedLabel, + skippedLabel, + isCompact, +}) { + const labels = { passedLabel, failedLabel, skippedLabel } + if (total === 0) { + return 'no tests' + } else if (isCompact) { + ;({ passedLabel = '✔', failedLabel = '✘', skippedLabel = '➟' } = labels) + return [ + `${passedLabel} ${passed}`, + failed > 0 && `${failedLabel} ${failed}`, + skipped > 0 && `${skippedLabel} ${skipped}`, + ] + .filter(Boolean) + .join(' | ') + } else { + ;({ + passedLabel = 'passed', + failedLabel = 'failed', + skippedLabel = 'skipped', + } = labels) + return [ + `${passed} ${passedLabel}`, + failed > 0 && `${failed} ${failedLabel}`, + skipped > 0 && `${skipped} ${skippedLabel}`, + ] + .filter(Boolean) + .join(', ') + } +} + +function renderTestResultBadge({ + passed, + failed, + skipped, + total, + passedLabel, + failedLabel, + skippedLabel, + isCompact, +}) { + const message = renderTestResultMessage({ + passed, + failed, + skipped, + total, + passedLabel, + failedLabel, + skippedLabel, + isCompact, + }) + + let color + if (total === 0) { + color = 'yellow' + } else if (failed > 0) { + color = 'red' + } else if (skipped > 0 && passed > 0) { + color = 'green' + } else if (skipped > 0) { + color = 'yellow' + } else { + color = 'brightgreen' + } + + return { message, color } +} + +module.exports = { + testResultQueryParamSchema, + renderTestResultMessage, + renderTestResultBadge, +} diff --git a/services/test-results.spec.js b/services/test-results.spec.js new file mode 100644 index 0000000000000..feb9a5771e8f3 --- /dev/null +++ b/services/test-results.spec.js @@ -0,0 +1,51 @@ +'use strict' + +const { test, given } = require('sazerac') +const { + renderTestResultMessage, + renderTestResultBadge, +} = require('./test-results') + +describe('Test result helpers', function() { + function renderBothStyles(props) { + const { message: standardMessage, color } = renderTestResultBadge(props) + const compactMessage = renderTestResultMessage({ + ...props, + isCompact: true, + }) + return { standardMessage, compactMessage, color } + } + + test(renderBothStyles, () => { + given({ passed: 12, failed: 3, skipped: 3, total: 18 }).expect({ + standardMessage: '12 passed, 3 failed, 3 skipped', + compactMessage: '✔ 12 | ✘ 3 | ➟ 3', + color: 'red', + }) + given({ passed: 12, failed: 3, skipped: 0, total: 15 }).expect({ + standardMessage: '12 passed, 3 failed', + compactMessage: '✔ 12 | ✘ 3', + color: 'red', + }) + given({ passed: 12, failed: 0, skipped: 3, total: 15 }).expect({ + standardMessage: '12 passed, 3 skipped', + compactMessage: '✔ 12 | ➟ 3', + color: 'green', + }) + given({ passed: 0, failed: 0, skipped: 3, total: 3 }).expect({ + standardMessage: '0 passed, 3 skipped', + compactMessage: '✔ 0 | ➟ 3', + color: 'yellow', + }) + given({ passed: 12, failed: 0, skipped: 0, total: 12 }).expect({ + standardMessage: '12 passed', + compactMessage: '✔ 12', + color: 'brightgreen', + }) + given({ passed: 0, failed: 0, skipped: 0, total: 0 }).expect({ + standardMessage: 'no tests', + compactMessage: 'no tests', + color: 'yellow', + }) + }) +}) diff --git a/services/text-formatters.js b/services/text-formatters.js index 4c37d72628d32..c6768a1c3b8e0 100644 --- a/services/text-formatters.js +++ b/services/text-formatters.js @@ -116,81 +116,6 @@ function formatRelativeDate(timestamp) { .toLowerCase() } -function renderTestResultMessage({ - passed, - failed, - skipped, - total, - passedLabel, - failedLabel, - skippedLabel, - isCompact, -}) { - const labels = { passedLabel, failedLabel, skippedLabel } - if (total === 0) { - return 'no tests' - } else if (isCompact) { - ;({ passedLabel = '✔', failedLabel = '✘', skippedLabel = '➟' } = labels) - return [ - `${passedLabel} ${passed}`, - failed > 0 && `${failedLabel} ${failed}`, - skipped > 0 && `${skippedLabel} ${skipped}`, - ] - .filter(Boolean) - .join(' | ') - } else { - ;({ - passedLabel = 'passed', - failedLabel = 'failed', - skippedLabel = 'skipped', - } = labels) - return [ - `${passed} ${passedLabel}`, - failed > 0 && `${failed} ${failedLabel}`, - skipped > 0 && `${skipped} ${skippedLabel}`, - ] - .filter(Boolean) - .join(', ') - } -} - -function renderTestResultBadge({ - passed, - failed, - skipped, - total, - passedLabel, - failedLabel, - skippedLabel, - isCompact, -}) { - const message = renderTestResultMessage({ - passed, - failed, - skipped, - total, - passedLabel, - failedLabel, - skippedLabel, - isCompact, - }) - - let color - if (total === 0) { - color = 'yellow' - } else if (failed > 0) { - color = 'red' - } else if (skipped > 0 && passed > 0) { - color = 'green' - } else if (skipped > 0) { - color = 'yellow' - } else { - color = 'brightgreen' - } - - return { message, color } -} - module.exports = { starRating, currencyFromCode, @@ -201,6 +126,4 @@ module.exports = { maybePluralize, formatDate, formatRelativeDate, - renderTestResultMessage, - renderTestResultBadge, } diff --git a/services/text-formatters.spec.js b/services/text-formatters.spec.js index 036a95392737f..a9e79f39ffc69 100644 --- a/services/text-formatters.spec.js +++ b/services/text-formatters.spec.js @@ -12,8 +12,6 @@ const { maybePluralize, formatDate, formatRelativeDate, - renderTestResultMessage, - renderTestResultBadge, } = require('./text-formatters') describe('Text formatters', function() { @@ -123,46 +121,4 @@ describe('Text formatters', function() { .expect('a month ago') }) }) - - function renderBothStyles(props) { - const { message: standardMessage, color } = renderTestResultBadge(props) - const compactMessage = renderTestResultMessage({ - ...props, - isCompact: true, - }) - return { standardMessage, compactMessage, color } - } - - test(renderBothStyles, () => { - given({ passed: 12, failed: 3, skipped: 3, total: 18 }).expect({ - standardMessage: '12 passed, 3 failed, 3 skipped', - compactMessage: '✔ 12 | ✘ 3 | ➟ 3', - color: 'red', - }) - given({ passed: 12, failed: 3, skipped: 0, total: 15 }).expect({ - standardMessage: '12 passed, 3 failed', - compactMessage: '✔ 12 | ✘ 3', - color: 'red', - }) - given({ passed: 12, failed: 0, skipped: 3, total: 15 }).expect({ - standardMessage: '12 passed, 3 skipped', - compactMessage: '✔ 12 | ➟ 3', - color: 'green', - }) - given({ passed: 0, failed: 0, skipped: 3, total: 3 }).expect({ - standardMessage: '0 passed, 3 skipped', - compactMessage: '✔ 0 | ➟ 3', - color: 'yellow', - }) - given({ passed: 12, failed: 0, skipped: 0, total: 12 }).expect({ - standardMessage: '12 passed', - compactMessage: '✔ 12', - color: 'brightgreen', - }) - given({ passed: 0, failed: 0, skipped: 0, total: 0 }).expect({ - standardMessage: 'no tests', - compactMessage: 'no tests', - color: 'yellow', - }) - }) })