From ac5870c77d8afc3677ee041518755d7fed6887af Mon Sep 17 00:00:00 2001 From: Tom Hirst Date: Tue, 7 Nov 2017 18:08:12 +0000 Subject: [PATCH 1/3] do not blow up if response body is not a JSON fixes issues address here https://github.com/geckoboard/geckoboard-node/pull/17 with additional tests --- lib/api-request-test.js | 40 ++++++++++++++++++++++++++++++++++++++++ lib/api-request.js | 14 ++++++++++++-- 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/lib/api-request-test.js b/lib/api-request-test.js index 5c17114..193bc9c 100644 --- a/lib/api-request-test.js +++ b/lib/api-request-test.js @@ -46,4 +46,44 @@ describe('api', () => { 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 METHOD = 'get', + ENDPOINT = '/whoopie-do', + OPTIONS = { endpoint: ENDPOINT }, + BAD_RESPONSE_BODY = 'sorry'; + + nock(HOST)[METHOD](ENDPOINT) + .reply(200, BAD_RESPONSE_BODY); + + apiRequest(METHOD, OPTIONS, (err, body) => { + expect(body).toBeUndefined() + expect(err).not.toBeNull() + 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 METHOD = 'get', + ENDPOINT = '/whoopie-do', + OPTIONS = { endpoint: ENDPOINT }, + BAD_RESPONSE_BODY = 'sorry'; + + nock(HOST)[METHOD](ENDPOINT) + .reply(400, BAD_RESPONSE_BODY); + + apiRequest(METHOD, OPTIONS, (err, body) => { + expect(body).toBeUndefined() + expect(err).not.toBeNull() + done(); + }); + }); + }); + }); }); diff --git a/lib/api-request.js b/lib/api-request.js index a1dbad4..07dc20a 100644 --- a/lib/api-request.js +++ b/lib/api-request.js @@ -10,9 +10,19 @@ 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 a JSON')); + } } else { - cb(new Error(JSON.parse(body).error.message)); + try { + const parsedBody = JSON.parse(body); + cb(new Error(parsedBody.error.message)); + } catch (e) { + cb(new Error('The response from the server was not a JSON')); + } } } const requestOptions = { From d65f265101d6d11000670ff7ed4e856f44ba1b98 Mon Sep 17 00:00:00 2001 From: Tom Hirst Date: Tue, 7 Nov 2017 19:25:39 +0000 Subject: [PATCH 2/3] if error response does not contain message set error to Unknown fixes issues addressed here https://github.com/geckoboard/geckoboard-node/pull/17 with additional tests --- lib/api-request-test.js | 49 +++++++++++++++++++++++++---------------- lib/api-request.js | 6 ++++- 2 files changed, 35 insertions(+), 20 deletions(-) diff --git a/lib/api-request-test.js b/lib/api-request-test.js index 193bc9c..6fc70a2 100644 --- 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) @@ -47,21 +46,36 @@ describe('api', () => { }); }); + 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 METHOD = 'get', - ENDPOINT = '/whoopie-do', - OPTIONS = { endpoint: ENDPOINT }, - BAD_RESPONSE_BODY = 'sorry'; + const BAD_RESPONSE_BODY = 'sorry'; nock(HOST)[METHOD](ENDPOINT) .reply(200, BAD_RESPONSE_BODY); apiRequest(METHOD, OPTIONS, (err, body) => { expect(body).toBeUndefined() - expect(err).not.toBeNull() + expect(err).toEqual(new Error('The response from the server was not a JSON')) done(); }); }); @@ -70,17 +84,14 @@ describe('api', () => { describe('and the response status is not 200', () => { it('does not blow up', (done) => { const apiRequest = api.apiRequest(HOST, API_KEY, USER_AGENT); - const METHOD = 'get', - ENDPOINT = '/whoopie-do', - OPTIONS = { endpoint: ENDPOINT }, - BAD_RESPONSE_BODY = 'sorry'; + const BAD_RESPONSE_BODY = 'sorry'; nock(HOST)[METHOD](ENDPOINT) .reply(400, BAD_RESPONSE_BODY); apiRequest(METHOD, OPTIONS, (err, body) => { expect(body).toBeUndefined() - expect(err).not.toBeNull() + expect(err).toEqual(new Error('The response from the server was not a JSON')) done(); }); }); diff --git a/lib/api-request.js b/lib/api-request.js index 07dc20a..4a40f56 100644 --- a/lib/api-request.js +++ b/lib/api-request.js @@ -19,7 +19,11 @@ module.exports = { } else { try { const parsedBody = JSON.parse(body); - cb(new Error(parsedBody.error.message)); + 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 a JSON')); } From 1d64e97110671ff88370751cccdf48bcce1cfba2 Mon Sep 17 00:00:00 2001 From: Tom Hirst Date: Tue, 7 Nov 2017 21:38:39 +0000 Subject: [PATCH 3/3] use better JSON parse error message --- lib/api-request-test.js | 4 ++-- lib/api-request.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/api-request-test.js b/lib/api-request-test.js index 6fc70a2..debe761 100644 --- a/lib/api-request-test.js +++ b/lib/api-request-test.js @@ -75,7 +75,7 @@ describe('api', () => { apiRequest(METHOD, OPTIONS, (err, body) => { expect(body).toBeUndefined() - expect(err).toEqual(new Error('The response from the server was not a JSON')) + expect(err).toEqual(new Error('The response from the server was not valid JSON')) done(); }); }); @@ -91,7 +91,7 @@ describe('api', () => { apiRequest(METHOD, OPTIONS, (err, body) => { expect(body).toBeUndefined() - expect(err).toEqual(new Error('The response from the server was not a JSON')) + 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 index 4a40f56..5ff6f04 100644 --- a/lib/api-request.js +++ b/lib/api-request.js @@ -14,7 +14,7 @@ module.exports = { const parsedBody = JSON.parse(body); cb(null, parsedBody); } catch (e) { - cb(new Error('The response from the server was not a JSON')); + cb(new Error('The response from the server was not valid JSON')); } } else { try { @@ -25,7 +25,7 @@ module.exports = { cb(new Error(parsedBody.error.message)); } } catch (e) { - cb(new Error('The response from the server was not a JSON')); + cb(new Error('The response from the server was not valid JSON')); } } }