[appveyor] Error handling in BaseService

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

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

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;
+        });
+      }
     });
   };
 }

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) {

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

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)$/,

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') {

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')

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,15 +167,15 @@ 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,
     handleRequest(async (queryParams, match, sendBadge, request) => {
       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);