Add query param validation to remaining new-style services [azuredevo…

…ps appveyor npm]

Remove now-obsolete code.

Close #2675

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}`)

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 []
   }
 }
 

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(),

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 = `
 </p>
 `
 
-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,
     }
   }
 

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,
     }
   }
 

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

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,
     }
   }
 

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,
+}

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',
+    })
+  })
+})

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,
 }