Merge pull request #18 from geckoboard/bugfix-non-json-response

Handle non-JSON response for API requests
geckoboard · Nov 8, 2017 · b429076 · b429076
2 parents bcd4430 + 1d64e97
commit b429076
Show file tree

Hide file tree

Showing 2 changed files with 76 additions and 11 deletions.
diff --git a/lib/api-request-test.js b/lib/api-request-test.js
@@ -3,14 +3,16 @@ const nock = require('nock');
 const request = require('request');
 
 describe('api', () => {
-  const HOST = 'https://example.com',
-    API_KEY = '123',
-    USER_AGENT = 'lolbot 0.1';
+  const HOST = 'https://example.com';
+  const API_KEY = '123';
+  const USER_AGENT = 'lolbot 0.1';
+  const METHOD = 'get';
+  const ENDPOINT = '/whoopie-do';
+  const OPTIONS = { endpoint: ENDPOINT };
 
   it('makes requests with configured methods', () => {
     const apiRequest = api.apiRequest(HOST, API_KEY, USER_AGENT);
-    const OPTIONS = { endpoint: '/whoopie-do' },
-      EXPECTED_OPTIONS = { url: `${HOST}/whoopie-do`, headers: { 'User-Agent': USER_AGENT } };
+    const EXPECTED_OPTIONS = { url: `${HOST}${ENDPOINT}`, headers: { 'User-Agent': USER_AGENT } };
     ['get', 'put', 'post', 'delete'].forEach(method => {
       const authSpy = jasmine.createSpy('auth');
       const CALLBACK = () => {};
@@ -25,10 +27,7 @@ describe('api', () => {
 
   it('calls the given callback with response', (done) => {
     const apiRequest = api.apiRequest(HOST, API_KEY, USER_AGENT);
-    const METHOD = 'get',
-      ENDPOINT = '/whoopie-do',
-      OPTIONS = { endpoint: ENDPOINT },
-      RESPONSE_BODY = { ha: 'ha' };
+    const RESPONSE_BODY = { ha: 'ha' };
 
     nock(HOST)[METHOD](ENDPOINT)
     .matchHeader('User-Agent', USER_AGENT)
@@ -46,4 +45,56 @@ describe('api', () => {
       done();
     });
   });
+
+  describe('when the response status is not 200', () => {
+    describe('and the response JSON is not formatted correctly', () => {
+      it('returns an appropriate error', (done) => {
+        const apiRequest = api.apiRequest(HOST, API_KEY, USER_AGENT);
+        const BAD_JSON_RESPONSE_BODY = { i: 'sorry' };
+
+        nock(HOST)[METHOD](ENDPOINT)
+        .reply(400, BAD_JSON_RESPONSE_BODY);
+
+        apiRequest(METHOD, OPTIONS, (err, body) => {
+          expect(body).toBeUndefined()
+          expect(err).toEqual(new Error('Unknown error'))
+          done();
+        });
+      });
+    });
+  });
+
+  describe('when the response is not JSON formatted', () => {
+    describe('and the response status is 200', () => {
+      it('does not blow up', (done) => {
+        const apiRequest = api.apiRequest(HOST, API_KEY, USER_AGENT);
+        const BAD_RESPONSE_BODY = 'sorry';
+
+        nock(HOST)[METHOD](ENDPOINT)
+        .reply(200, BAD_RESPONSE_BODY);
+
+        apiRequest(METHOD, OPTIONS, (err, body) => {
+          expect(body).toBeUndefined()
+          expect(err).toEqual(new Error('The response from the server was not valid JSON'))
+          done();
+        });
+      });
+    });
+
+    describe('and the response status is not 200', () => {
+      it('does not blow up', (done) => {
+        const apiRequest = api.apiRequest(HOST, API_KEY, USER_AGENT);
+        const BAD_RESPONSE_BODY = 'sorry';
+
+        nock(HOST)[METHOD](ENDPOINT)
+        .reply(400, BAD_RESPONSE_BODY);
+
+        apiRequest(METHOD, OPTIONS, (err, body) => {
+          expect(body).toBeUndefined()
+          expect(err).toEqual(new Error('The response from the server was not valid JSON'))
+          done();
+        });
+      });
+    });
+  });
 });
diff --git a/lib/api-request.js b/lib/api-request.js
@@ -10,9 +10,23 @@ module.exports = {
         return;
       }
       if (response.statusCode >= 200 && response.statusCode < 300) {
-        cb(null, JSON.parse(body));
+        try {
+          const parsedBody = JSON.parse(body);
+          cb(null, parsedBody);
+        } catch (e) {
+          cb(new Error('The response from the server was not valid JSON'));
+        }
       } else {
-        cb(new Error(JSON.parse(body).error.message));
+        try {
+          const parsedBody = JSON.parse(body);
+          if (!parsedBody || !parsedBody.error || !!parsedBody.error.message) {
+            cb(new Error('Unknown error'));
+          } else {
+            cb(new Error(parsedBody.error.message));
+          }
+        } catch (e) {
+          cb(new Error('The response from the server was not valid JSON'));
+        }
       }
     }
     const requestOptions = {