diff --git a/lib/error-helper.js b/lib/error-helper.js index ec53da94a2146..a12ea3130981a 100644 --- a/lib/error-helper.js +++ b/lib/error-helper.js @@ -1,5 +1,10 @@ 'use strict'; +const { + NotFound, + InvalidResponse, +} = require('../services/errors'); + const checkErrorResponse = function(badgeData, err, res, notFoundMessage = 'not found') { if (err != null) { badgeData.text[1] = 'inaccessible'; @@ -18,6 +23,27 @@ const checkErrorResponse = function(badgeData, err, res, notFoundMessage = 'not } }; +checkErrorResponse.asPromise = function ({ notFoundMessage } = {}) { + return async function ({ buffer, res }) { + if (res.statusCode === 404) { + throw new NotFound(notFoundMessage); + } else if (res.statusCode !== 200) { + const underlying = Error(`Got status code ${res.statusCode} (expected 200)`); + throw new InvalidResponse(undefined, underlying); + } + return { buffer, res }; + }; +}; + +async function asJson({ buffer, res }) { + try { + return JSON.parse(buffer); + } catch (err) { + throw new InvalidResponse(); + } +}; + module.exports = { checkErrorResponse, + asJson, }; diff --git a/lib/error-helper.spec.js b/lib/error-helper.spec.js index e3ab65103efa2..b3c4c3e4642ed 100644 --- a/lib/error-helper.spec.js +++ b/lib/error-helper.spec.js @@ -1,7 +1,11 @@ 'use strict'; -const { assert } = require('chai'); +const chai = require('chai'); +const { assert, expect } = chai; const { checkErrorResponse } = require('./error-helper'); +const { NotFound, InvalidResponse } = require('../services/errors'); + +chai.use(require('chai-as-promised')); describe('Standard Error Handler', function() { it('makes inaccessible badge', function() { @@ -36,3 +40,56 @@ describe('Standard Error Handler', function() { assert.equal(false, checkErrorResponse({'text': []}, null, {statusCode: 200})); }); }); + +describe('async error handler', function() { + context('when status is 200', function() { + it('passes through the inputs', async function() { + const args = { buffer: 'buffer', res: { statusCode: 200 } }; + expect(await checkErrorResponse.asPromise()(args)) + .to.deep.equal(args); + }); + }); + + context('when status is 404', function() { + const res = { statusCode: 404 }; + + it('throws NotFound', async function() { + try { + await checkErrorResponse.asPromise()({ res }); + expect.fail('Expected to throw'); + } catch (e) { + expect(e).to.be.an.instanceof(NotFound); + expect(e.message).to.equal('Not Found'); + expect(e.prettyMessage).to.equal('not found'); + } + }); + + it('displays the custom not found message', async function() { + const notFoundMessage = 'no goblins found'; + const res = { statusCode: 404 }; + try { + await checkErrorResponse.asPromise({ notFoundMessage })({ res }); + expect.fail('Expected to throw'); + } catch (e) { + expect(e).to.be.an.instanceof(NotFound); + expect(e.message).to.equal('Not Found: no goblins found'); + expect(e.prettyMessage).to.equal('no goblins found'); + } + }); + }); + + context('when status is 500', function() { + const res = { statusCode: 500 }; + + it('throws InvalidResponse', async function() { + try { + await checkErrorResponse.asPromise()({ res }); + expect.fail('Expected to throw'); + } catch (e) { + expect(e).to.be.an.instanceof(InvalidResponse); + expect(e.message).to.equal('Invalid Response: Got status code 500 (expected 200)'); + expect(e.prettyMessage).to.equal('invalid'); + } + }); + }); +}); diff --git a/lib/request-handler.js b/lib/request-handler.js index 8d89449e58ad9..7977fea0d6536 100644 --- a/lib/request-handler.js +++ b/lib/request-handler.js @@ -9,6 +9,7 @@ const LruCache = require('./lru-cache'); const analytics = require('./analytics'); const { makeSend } = require('./result-sender'); const queryString = require('query-string'); +const { Inaccessible } = require('../services/errors'); // We avoid calling the vendor's server for computation of the information in a // number of badges. @@ -167,20 +168,22 @@ function handleRequest (makeBadge, handlerOptions) { }); } - // Wrapper around `cachingRequest` that returns a promise rather than - // needing to pass a callback. - cachingRequest.asPromise = (uri, options) => new Promise((resolve, reject) => { - cachingRequest(uri, options, (err, res, buffer) => { - if (err) { - reject(err); - } else { - resolve({res, buffer}); - } + // Wrapper around `cachingRequest` that returns a promise rather than + // needing to pass a callback. + cachingRequest.asPromise = (uri, options) => new Promise((resolve, reject) => { + cachingRequest(uri, options, (err, res, buffer) => { + if (err) { + // Wrap the error in an Inaccessible so it can be identified + // by the BaseService handler. + reject(new Inaccessible(err)); + } else { + resolve({ res, buffer }); + } }); }); vendorDomain.run(() => { - handlerOptions.handler(filteredQueryParams, match, function sendBadge(format, badgeData) { + const result = handlerOptions.handler(filteredQueryParams, match, function sendBadge(format, badgeData) { if (serverUnresponsive) { return; } clearTimeout(serverResponsive); // Check for a change in the data. @@ -206,6 +209,11 @@ function handleRequest (makeBadge, handlerOptions) { makeSend(format, ask.res, end)(svg); } }, cachingRequest); + if (result && result.catch) { + result.catch(err => { + throw err; + }); + } }); }; } diff --git a/lib/server-config.js b/lib/server-config.js index 82a10405ca9bc..d4d6af3db66d7 100644 --- a/lib/server-config.js +++ b/lib/server-config.js @@ -65,6 +65,7 @@ const config = { makeBadge: envFlag(process.env.PROFILE_MAKE_BADGE), }, rateLimit: envFlag(process.env.RATE_LIMIT, true), + handleInternalErrors: envFlag(process.env.HANDLE_INTERNAL_ERRORS, true), }; if (config.font.fallbackPath) { diff --git a/package-lock.json b/package-lock.json index f79ba9229fc2e..d2ed798d96a99 100644 --- a/package-lock.json +++ b/package-lock.json @@ -2381,6 +2381,15 @@ "type-detect": "4.0.3" } }, + "chai-as-promised": { + "version": "7.1.1", + "resolved": "https://registry.npmjs.org/chai-as-promised/-/chai-as-promised-7.1.1.tgz", + "integrity": "sha512-azL6xMoi+uxu6z4rhWQ1jbdUhOMhis2PvscD/xjLqNMkv3BPPp2JyyuTHOrf9BOosGpNQ11v6BKv/g57RXbiaA==", + "dev": true, + "requires": { + "check-error": "1.0.2" + } + }, "chai-string": { "version": "1.4.0", "resolved": "https://registry.npmjs.org/chai-string/-/chai-string-1.4.0.tgz", diff --git a/package.json b/package.json index 9d3c5ac126ac0..3d849e240f536 100644 --- a/package.json +++ b/package.json @@ -58,10 +58,10 @@ "lint": "eslint \"**/*.js\"", "danger": "danger", "test:js:frontend": "NODE_ENV=mocha mocha --require babel-polyfill --require babel-register \"frontend/**/*.spec.js\"", - "test:js:server": "mocha \"*.spec.js\" \"lib/**/*.spec.js\" \"services/**/*.spec.js\"", - "test:services": "mocha --delay lib/service-test-runner/cli.js", + "test:js:server": "HANDLE_INTERNAL_ERRORS=false mocha \"*.spec.js\" \"lib/**/*.spec.js\" \"services/**/*.spec.js\"", + "test:services": "HANDLE_INTERNAL_ERRORS=false mocha --delay lib/service-test-runner/cli.js", "test:services:pr:prepare": "node lib/service-test-runner/pull-request-services-cli.js > pull-request-services.log", - "test:services:pr:run": "mocha --delay lib/service-test-runner/cli.js --stdin < pull-request-services.log", + "test:services:pr:run": "HANDLE_INTERNAL_ERRORS=false mocha --delay lib/service-test-runner/cli.js --stdin < pull-request-services.log", "test:services:pr": "npm run test:services:pr:prepare && npm run test:services:pr:run", "test": "npm run lint && npm run test:js:frontend && npm run test:js:server", "circle-images:build": "docker build -t shieldsio/shields-ci-node-8:${IMAGE_TAG} -f .circleci/images/node-8/Dockerfile . && docker build -t shieldsio/shields-ci-node-9:${IMAGE_TAG} -f .circleci/images/node-9/Dockerfile .", @@ -73,7 +73,7 @@ "build": "npm run examples && next build && next export -o build/", "heroku-postbuild": "npm run build", "analyze": "ANALYZE=true LONG_CACHE=false BASE_URL=https://img.shields.io npm run build", - "start:server": "RATE_LIMIT=false node server 8080 ::", + "start:server": "HANDLE_INTERNAL_ERRORS=false RATE_LIMIT=false node server 8080 ::", "now-start": "node server", "prestart": "npm run depcheck && npm run examples", "start": "concurrently --names server,frontend \"ALLOWED_ORIGIN=http://localhost:3000 npm run start:server\" \"BASE_URL=http://[::]:8080 next dev\"" @@ -99,6 +99,7 @@ "babel-polyfill": "^6.26.0", "babel-preset-env": "^1.6.1", "chai": "^4.1.2", + "chai-as-promised": "^7.1.1", "chai-string": "^1.4.0", "chainsmoker": "^0.1.0", "check-node-version": "^3.1.0", diff --git a/server.js b/server.js index bc8a4ee78a4d5..d343470a8ae75 100644 --- a/server.js +++ b/server.js @@ -201,7 +201,10 @@ camp.notfound(/.*/, function(query, match, end, request) { // Vendors. loadServiceClasses().forEach( - serviceClass => serviceClass.register(camp, cache)); + serviceClass => serviceClass.register( + camp, + cache, + { handleInternalErrors: config.handleInternalErrors })); // JIRA issue integration camp.route(/^\/jira\/issue\/(http(?:s)?)\/(.+)\/([^/]+)\.(svg|png|gif|jpg|json)$/, diff --git a/services/appveyor/appveyor.js b/services/appveyor/appveyor.js index e414e81a29bcd..50a6f0ac8c056 100644 --- a/services/appveyor/appveyor.js +++ b/services/appveyor/appveyor.js @@ -1,26 +1,23 @@ 'use strict'; const BaseService = require('../base'); +const { + checkErrorResponse, + asJson, +} = require('../../lib/error-helper'); -/** - * AppVeyor CI integration. - */ module.exports = class AppVeyor extends BaseService { async handle({repo, branch}) { let apiUrl = 'https://ci.appveyor.com/api/projects/' + repo; if (branch != null) { apiUrl += '/branch/' + branch; } - const {buffer, res} = await this._sendAndCacheRequest(apiUrl, { + const json = await this._sendAndCacheRequest(apiUrl, { headers: { 'Accept': 'application/json' } - }); + }).then(checkErrorResponse.asPromise({ notFoundMessage: 'project not found or access denied' })) + .then(asJson); - if (res.statusCode === 404) { - return {message: 'project not found or access denied'}; - } - - const data = JSON.parse(buffer); - const status = data.build.status; + const { build: { status } } = json; if (status === 'success') { return {message: 'passing', color: 'brightgreen'}; } else if (status !== 'running' && status !== 'queued') { diff --git a/services/appveyor/appveyor.tester.js b/services/appveyor/appveyor.tester.js index f67ce6afb7c8c..d3812d165909b 100644 --- a/services/appveyor/appveyor.tester.js +++ b/services/appveyor/appveyor.tester.js @@ -25,6 +25,11 @@ t.create('CI 404') .get('/ci/somerandomproject/thatdoesntexits.json') .expectJSON({ name: 'build', value: 'project not found or access denied' }); +t.create('CI (connection error)') + .get('/ci/this-one/is-not-real-either.json') + .networkOff() + .expectJSON({ name: 'build', value: 'inaccessible' }); + // Test AppVeyor tests status badge t.create('tests status') .get('/tests/NZSmartie/coap-net-iu0to.json') diff --git a/services/base.js b/services/base.js index c7113919562e2..23f527dbacd0b 100644 --- a/services/base.js +++ b/services/base.js @@ -1,5 +1,10 @@ 'use strict'; +const { + NotFound, + InvalidResponse, + Inaccessible, +} = require('./errors'); const { makeLogo, toArray, @@ -8,8 +13,9 @@ const { } = require('../lib/badge-data'); module.exports = class BaseService { - constructor({sendAndCacheRequest}) { + constructor({ sendAndCacheRequest }, { handleInternalErrors }) { this._sendAndCacheRequest = sendAndCacheRequest; + this._handleInternalErrors = handleInternalErrors; } /** @@ -32,6 +38,7 @@ module.exports = class BaseService { static get category() { return 'unknown'; } + /** * Returns an object: * - base: (Optional) The base path of the URLs for this service. This is @@ -95,8 +102,27 @@ module.exports = class BaseService { try { return await this.handle(namedParams); } catch (error) { - console.log(error); - return { message: 'error' }; + if (error instanceof NotFound) { + return { + message: error.prettyMessage, + color: 'red', + }; + } else if (error instanceof InvalidResponse || + error instanceof Inaccessible) { + return { + message: error.prettyMessage, + color: 'lightgray', + }; + } else if (this._handleInternalErrors) { + console.log(error); + return { + label: 'shields', + message: 'internal error', + color: 'lightgray', + }; + } else { + throw error; + } } } @@ -118,15 +144,15 @@ module.exports = class BaseService { link: serviceLink, } = serviceData; - const defaultLabel = this.category; const { color: defaultColor, logo: defaultLogo, + label: defaultLabel, } = this.defaultBadgeData; const badgeData = { text: [ - overrideLabel || serviceLabel || defaultLabel, + overrideLabel || serviceLabel || defaultLabel || this.category, serviceMessage || 'n/a', ], template: style, @@ -141,7 +167,7 @@ module.exports = class BaseService { return badgeData; } - static register(camp, handleRequest) { + static register(camp, handleRequest, { handleInternalErrors }) { const ServiceClass = this; // In a static context, "this" is the class. camp.route(this._regex, @@ -149,7 +175,7 @@ module.exports = class BaseService { const namedParams = this._namedParamsForMatch(match); const serviceInstance = new ServiceClass({ sendAndCacheRequest: request.asPromise, - }); + }, { handleInternalErrors }); const serviceData = await serviceInstance.invokeHandler(namedParams); const badgeData = this._makeBadgeData(queryParams, serviceData); diff --git a/services/base.spec.js b/services/base.spec.js index a09075597e352..5ea764cbb4314 100644 --- a/services/base.spec.js +++ b/services/base.spec.js @@ -26,6 +26,8 @@ class DummyService extends BaseService { } describe('BaseService', () => { + const defaultConfig = { handleInternalErrors: false }; + describe('URL pattern matching', function () { const regexExec = str => DummyService._regex.exec(str); const getSomeArg = str => { @@ -66,17 +68,21 @@ describe('BaseService', () => { }); it('Invokes the handler as expected', async function () { - const serviceInstance = new DummyService({}); + const serviceInstance = new DummyService({}, defaultConfig); const serviceData = await serviceInstance.invokeHandler({ someArg: 'bar.bar.bar' }); expect(serviceData).to.deep.equal({ message: 'Hello bar.bar.bar' }); }); describe('Error handling', function () { it('Handles internal errors', async function () { - const serviceInstance = new DummyService({}); + const serviceInstance = new DummyService({}, { handleInternalErrors: true }); serviceInstance.handle = () => { throw Error("I've made a huge mistake"); }; const serviceData = await serviceInstance.invokeHandler({ someArg: 'bar.bar.bar' }); - expect(serviceData).to.deep.equal({ message: 'error' }); + expect(serviceData).to.deep.equal({ + color: 'lightgray', + label: 'shields', + message: 'internal error', + }); }); }); @@ -129,7 +135,7 @@ describe('BaseService', () => { route: sinon.spy(), }; mockHandleRequest = sinon.spy(); - DummyService.register(mockCamp, mockHandleRequest); + DummyService.register(mockCamp, mockHandleRequest, defaultConfig); }); it('registers the service', () => { diff --git a/services/errors.js b/services/errors.js new file mode 100644 index 0000000000000..8357509b830cf --- /dev/null +++ b/services/errors.js @@ -0,0 +1,39 @@ +'use strict'; + +class NotFound extends Error { + constructor(prettyMessage = 'not found') { + const message = prettyMessage === 'not found' + ? 'Not Found' + : `Not Found: ${prettyMessage}`; + super(message); + this.prettyMessage = prettyMessage; + this.name = 'NotFound'; + } +} + +class InvalidResponse extends Error { + constructor(prettyMessage = 'invalid', underlyingError) { + const message = underlyingError + ? `Invalid Response: ${underlyingError.message}` + : 'Invalid Response'; + super(message); + this.stack = underlyingError.stack; + this.prettyMessage = prettyMessage; + this.name = 'InvalidResponse'; + } +} + +class Inaccessible extends Error { + constructor(underlyingError, prettyMessage = 'inaccessible') { + super(`Inaccessible: ${underlyingError.message}`); + this.stack = underlyingError.stack; + this.prettyMessage = prettyMessage; + this.name = 'Inaccessible'; + } +} + +module.exports = { + NotFound, + InvalidResponse, + Inaccessible, +};