From 6a737b7b38cef520b72f563c6690e6a1178bba4a Mon Sep 17 00:00:00 2001 From: Paul Melnikow Date: Thu, 6 Dec 2018 16:45:40 -0500 Subject: [PATCH] Rewrite the DynamicJson badge (#2399) This starts the rewrite of the dynamic badges. I've pulled into BaseService an initial version of the query param validation from #2325. I've extended from BaseJsonService to avoid duplicating the deserialization logic, though it means there is a bit of duplicated code among the three dynamic services. The way to unravel this would be to move the logic from `_requestJson` and friends from the base classes into functions so DynamicJson can inherit from BaseDynamic. Would that be worth it? This introduces a regression of #1446 for this badge. Close #2345 --- lib/validate.js | 15 ++++-- lib/validate.spec.js | 23 ++++++++- server.js | 18 +------ services/base.js | 14 +++++ services/base.spec.js | 12 +++++ services/dynamic/dynamic-helpers.js | 39 ++++++++++++++ services/dynamic/dynamic-json.service.js | 51 +++++++++++++++++++ .../dynamic-json.tester.js} | 42 +++++++-------- .../dynamic-xml.tester.js} | 0 .../dynamic-yaml.tester.js} | 0 .../gitlab/gitlab-pipeline-status.service.js | 25 ++------- services/validators.js | 6 +++ 12 files changed, 180 insertions(+), 65 deletions(-) create mode 100644 services/dynamic/dynamic-helpers.js create mode 100644 services/dynamic/dynamic-json.service.js rename services/{json/json.tester.js => dynamic/dynamic-json.tester.js} (86%) rename services/{xml/xml.tester.js => dynamic/dynamic-xml.tester.js} (100%) rename services/{yaml/yaml.tester.js => dynamic/dynamic-yaml.tester.js} (100%) diff --git a/lib/validate.js b/lib/validate.js index 6b44c89523d0b..4b262cbb6a7bc 100644 --- a/lib/validate.js +++ b/lib/validate.js @@ -8,6 +8,7 @@ function validate( { ErrorClass, prettyErrorMessage = 'data does not match schema', + includeKeys = false, traceErrorMessage = 'Data did not match schema', traceSuccessMessage = 'Data after validation', }, @@ -28,10 +29,16 @@ function validate( traceErrorMessage, error.message ) - throw new ErrorClass({ - prettyMessage: prettyErrorMessage, - underlyingError: error, - }) + + let prettyMessage = prettyErrorMessage + if (includeKeys) { + const keys = error.details.map(({ path }) => path) + if (keys) { + prettyMessage = `${prettyErrorMessage}: ${keys.join(',')}` + } + } + + throw new ErrorClass({ prettyMessage, underlyingError: error }) } else { trace.logTrace('validate', emojic.bathtub, traceSuccessMessage, value, { deep: true, diff --git a/lib/validate.spec.js b/lib/validate.spec.js index 51a1121764cfd..57cf560ee832b 100644 --- a/lib/validate.spec.js +++ b/lib/validate.spec.js @@ -61,7 +61,7 @@ describe('validate', function() { }) context('data does not match schema', function() { - it('logs the data and throws the expected error', async function() { + it('logs the data and throws the expected error', function() { try { validate( options, @@ -83,5 +83,26 @@ describe('validate', function() { 'child "requiredString" fails because ["requiredString" must be a string]' ) }) + + context('with includeKeys: true', function() { + it('includes keys in the error text', function() { + try { + validate( + { ...options, includeKeys: true }, + { requiredString: ['this', "shouldn't", 'work'] }, + schema + ) + expect.fail('Expected to throw') + } catch (e) { + expect(e).to.be.an.instanceof(InvalidParameter) + expect(e.message).to.equal( + 'Invalid Parameter: child "requiredString" fails because ["requiredString" must be a string]' + ) + expect(e.prettyMessage).to.equal( + `${prettyErrorMessage}: requiredString` + ) + } + }) + }) }) }) diff --git a/server.js b/server.js index b554d25215102..4d50af7589d96 100644 --- a/server.js +++ b/server.js @@ -113,7 +113,7 @@ loadServiceClasses().forEach(serviceClass => // User defined sources - JSON response camp.route( - /^\/badge\/dynamic\/(json|xml|yaml)\.(svg|png|gif|jpg|json)$/, + /^\/badge\/dynamic\/(xml|yaml)\.(svg|png|gif|jpg|json)$/, cache(config.cacheHeaders, { queryParams: ['uri', 'url', 'query', 'prefix', 'suffix'], handler: function(query, match, sendBadge, request) { @@ -146,14 +146,6 @@ camp.route( } switch (type) { - case 'json': - requestOptions = { - headers: { - Accept: 'application/json', - }, - json: true, - } - break case 'xml': requestOptions = { headers: { @@ -185,14 +177,6 @@ camp.route( let innerText = [] switch (type) { - case 'json': - data = typeof data === 'object' ? data : JSON.parse(data) - data = jp.query(data, pathExpression) - if (!data.length) { - throw Error('no result') - } - innerText = data - break case 'xml': data = new DOMParser().parseFromString(data) data = xpath.select(pathExpression, data) diff --git a/services/base.js b/services/base.js index a80ab38067fe7..b7b03aea28259 100644 --- a/services/base.js +++ b/services/base.js @@ -478,6 +478,20 @@ class BaseService { ) } + static _validateQueryParams(queryParams, queryParamSchema) { + return validate( + { + ErrorClass: InvalidParameter, + prettyErrorMessage: 'invalid query parameter', + includeKeys: true, + traceErrorMessage: 'Query params did not match schema', + traceSuccessMessage: 'Query params after validation', + }, + queryParams, + queryParamSchema + ) + } + async _request({ url, options = {}, errorMessages = {} }) { const logTrace = (...args) => trace.logTrace('fetch', ...args) logTrace(emojic.bowAndArrow, 'Request', url, '\n', options) diff --git a/services/base.spec.js b/services/base.spec.js index 581bea3e75a51..0e874fa9a23e6 100644 --- a/services/base.spec.js +++ b/services/base.spec.js @@ -605,6 +605,18 @@ describe('BaseService', function() { expect(e).to.be.an.instanceof(InvalidResponse) } }) + + it('throws error for invalid query params', async function() { + try { + DummyService._validateQueryParams( + { requiredString: ['this', "shouldn't", 'work'] }, + dummySchema + ) + expect.fail('Expected to throw') + } catch (e) { + expect(e).to.be.an.instanceof(InvalidParameter) + } + }) }) describe('request', function() { diff --git a/services/dynamic/dynamic-helpers.js b/services/dynamic/dynamic-helpers.js new file mode 100644 index 0000000000000..8fdd1d1cb9df0 --- /dev/null +++ b/services/dynamic/dynamic-helpers.js @@ -0,0 +1,39 @@ +'use strict' + +const Joi = require('joi') +const { optionalUrl } = require('../validators') + +function createRoute(which) { + return { + base: `badge/dynamic/${which}`, + pattern: '', + queryParams: ['uri', 'url', 'query', 'prefix', 'suffix'], + } +} + +const queryParamSchema = Joi.object({ + url: optionalUrl.required(), + query: Joi.string().required(), + prefix: Joi.alternatives().try(Joi.string(), Joi.number()), + suffix: Joi.alternatives().try(Joi.string(), Joi.number()), +}) + .rename('uri', 'url', { ignoreUndefined: true, override: true }) + .required() + +const errorMessages = { + 404: 'resource not found', +} + +function renderDynamicBadge({ values, prefix = '', suffix = '' }) { + return { + message: `${prefix}${values.join(', ')}${suffix}`, + color: 'brightgreen', + } +} + +module.exports = { + createRoute, + queryParamSchema, + errorMessages, + renderDynamicBadge, +} diff --git a/services/dynamic/dynamic-json.service.js b/services/dynamic/dynamic-json.service.js new file mode 100644 index 0000000000000..206dcd9cac7b5 --- /dev/null +++ b/services/dynamic/dynamic-json.service.js @@ -0,0 +1,51 @@ +'use strict' + +const Joi = require('joi') +const jp = require('jsonpath') +const BaseJsonService = require('../base-json') +const { InvalidResponse } = require('../errors') +const { + createRoute, + queryParamSchema, + errorMessages, + renderDynamicBadge, +} = require('./dynamic-helpers') + +module.exports = class DynamicJson extends BaseJsonService { + static get category() { + return 'dynamic' + } + + static get route() { + return createRoute('json') + } + + static get defaultBadgeData() { + return { + label: 'custom badge', + } + } + + async handle(namedParams, queryParams) { + const { + url, + query: pathExpression, + prefix, + suffix, + } = this.constructor._validateQueryParams(queryParams, queryParamSchema) + + const data = await this._requestJson({ + schema: Joi.any(), + url, + errorMessages, + }) + + const values = jp.query(data, pathExpression) + + if (!values.length) { + throw new InvalidResponse({ prettyMessage: 'no result' }) + } + + return renderDynamicBadge({ values, prefix, suffix }) + } +} diff --git a/services/json/json.tester.js b/services/dynamic/dynamic-json.tester.js similarity index 86% rename from services/json/json.tester.js rename to services/dynamic/dynamic-json.tester.js index c0e14de10f847..3c58c38dfef55 100644 --- a/services/json/json.tester.js +++ b/services/dynamic/dynamic-json.tester.js @@ -2,14 +2,9 @@ const Joi = require('joi') const { expect } = require('chai') -const ServiceTester = require('../service-tester') const { colorScheme: colorsB } = require('../test-helpers') -const t = new ServiceTester({ - id: 'dynamic-json', - title: 'User Defined JSON Source Data', - pathPrefix: '/badge/dynamic/json', -}) +const t = require('../create-service-tester')() module.exports = t t.create('Connection error') @@ -20,14 +15,14 @@ t.create('Connection error') .expectJSON({ name: 'Package Name', value: 'inaccessible', - colorB: colorsB.red, + colorB: colorsB.lightgrey, }) t.create('No URL specified') .get('.json?query=$.name&label=Package Name&style=_shields_test') .expectJSON({ name: 'Package Name', - value: 'no url specified', + value: 'invalid query parameter: url', colorB: colorsB.red, }) @@ -37,7 +32,7 @@ t.create('No query specified') ) .expectJSON({ name: 'Package Name', - value: 'no query specified', + value: 'invalid query parameter: query', colorB: colorsB.red, }) @@ -47,8 +42,8 @@ t.create('Malformed url') ) .expectJSON({ name: 'Package Name', - value: 'malformed url', - colorB: colorsB.red, + value: 'invalid', + colorB: colorsB.lightgrey, }) t.create('JSON from url') @@ -57,7 +52,7 @@ t.create('JSON from url') ) .expectJSON({ name: 'custom badge', - value: 'gh-badges', + value: 'shields.io', colorB: colorsB.brightgreen, }) @@ -67,7 +62,7 @@ t.create('JSON from uri (support uri query paramater)') ) .expectJSON({ name: 'custom badge', - value: 'gh-badges', + value: 'shields.io', colorB: colorsB.brightgreen, }) @@ -116,14 +111,14 @@ t.create('JSON from url | invalid url') .expectJSON({ name: 'custom badge', value: 'resource not found', - colorB: colorsB.lightgrey, + colorB: colorsB.red, }) t.create('JSON from url | user color overrides default') .get( '.json?url=https://github.com/badges/shields/raw/master/package.json&query=$.name&colorB=10ADED&style=_shields_test' ) - .expectJSON({ name: 'custom badge', value: 'gh-badges', colorB: '#10ADED' }) + .expectJSON({ name: 'custom badge', value: 'shields.io', colorB: '#10ADED' }) t.create('JSON from url | error color overrides default') .get( @@ -132,17 +127,18 @@ t.create('JSON from url | error color overrides default') .expectJSON({ name: 'custom badge', value: 'resource not found', - colorB: colorsB.lightgrey, - }) - -t.create('JSON from url | error color overrides user specified') - .get('.json?query=$.version&colorB=10ADED&style=_shields_test') - .expectJSON({ - name: 'custom badge', - value: 'no url specified', colorB: colorsB.red, }) +// FIXME This is a regression which should be fixed in BaseService. +// t.create('JSON from url | error color overrides user specified') +// .get('.json?query=$.version&colorB=10ADED&style=_shields_test') +// .expectJSON({ +// name: 'custom badge', +// value: 'invalid query parameter: url', +// colorB: colorsB.red, +// }) + let headers t.create('JSON from url | request should set Accept header') .get('.json?url=https://json-test/api.json&query=$.name') diff --git a/services/xml/xml.tester.js b/services/dynamic/dynamic-xml.tester.js similarity index 100% rename from services/xml/xml.tester.js rename to services/dynamic/dynamic-xml.tester.js diff --git a/services/yaml/yaml.tester.js b/services/dynamic/dynamic-yaml.tester.js similarity index 100% rename from services/yaml/yaml.tester.js rename to services/dynamic/dynamic-yaml.tester.js diff --git a/services/gitlab/gitlab-pipeline-status.service.js b/services/gitlab/gitlab-pipeline-status.service.js index d18e6f6f9f01c..92e888147b55d 100644 --- a/services/gitlab/gitlab-pipeline-status.service.js +++ b/services/gitlab/gitlab-pipeline-status.service.js @@ -1,9 +1,9 @@ 'use strict' const Joi = require('joi') -const validate = require('../../lib/validate') const BaseSvgScrapingService = require('../base-svg-scraping') -const { InvalidParameter, NotFound } = require('../errors') +const { optionalUrl } = require('../validators') +const { NotFound } = require('../errors') const { isPipelineStatus } = require('./gitlab-helpers') const badgeSchema = Joi.object({ @@ -12,10 +12,8 @@ const badgeSchema = Joi.object({ .required(), }).required() -const queryParamsSchema = Joi.object({ - // TODO This accepts URLs with query strings and fragments, which should be - // rejected. - gitlab_url: Joi.string().uri({ scheme: ['https'] }), +const queryParamSchema = Joi.object({ + gitlab_url: optionalUrl, }).required() module.exports = class GitlabPipelineStatus extends BaseSvgScrapingService { @@ -63,19 +61,6 @@ module.exports = class GitlabPipelineStatus extends BaseSvgScrapingService { ] } - static validateQueryParams(queryParams) { - return validate( - { - ErrorClass: InvalidParameter, - prettyErrorMessage: 'invalid query parameter', - traceErrorMessage: 'Query params did not match schema', - traceSuccessMessage: 'Query params after validation', - }, - queryParams, - queryParamsSchema - ) - } - static render({ status }) { const color = { pending: 'yellow', @@ -95,7 +80,7 @@ module.exports = class GitlabPipelineStatus extends BaseSvgScrapingService { async handle({ user, repo, branch = 'master' }, queryParams) { const { gitlab_url: baseUrl = 'https://gitlab.com', - } = this.constructor.validateQueryParams(queryParams) + } = this.constructor._validateQueryParams(queryParams, queryParamSchema) const { message: status } = await this._requestSvg({ schema: badgeSchema, url: `${baseUrl}/${user}/${repo}/badges/${branch}/pipeline.svg`, diff --git a/services/validators.js b/services/validators.js index e736eae779acb..77c3c2d7128b5 100644 --- a/services/validators.js +++ b/services/validators.js @@ -7,10 +7,16 @@ module.exports = { .integer() .min(0) .required(), + anyInteger: Joi.number() .integer() .required(), + semver: Joi.semver() .valid() .required(), + + // TODO This accepts URLs with query strings and fragments, which for some + // purposes should be rejected. + optionalUrl: Joi.string().uri({ scheme: ['http', 'https'] }), }