From be9510165ed2c498bb945738f0015b55208cbd1d Mon Sep 17 00:00:00 2001 From: Tim Schaub Date: Wed, 5 Aug 2015 14:57:35 -0600 Subject: [PATCH 1/3] Allow numbers in urls.join() --- api/urls.js | 6 +++++- test/api/urls.test.js | 25 +++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/api/urls.js b/api/urls.js index b0c057b..5872b33 100644 --- a/api/urls.js +++ b/api/urls.js @@ -13,7 +13,11 @@ var API = require('./config').API_URL; */ function join() { return Array.prototype.map.call(arguments, function(part) { - return part.replace(/^\/?(.*?)\/?$/, '$1'); + if (!(typeof part === 'string' || typeof part === 'number')) { + throw new Error( + 'join must be called with strings or numbers, got: ' + part); + } + return String(part).replace(/^\/?(.*?)\/?$/, '$1'); }).join('/'); } diff --git a/test/api/urls.test.js b/test/api/urls.test.js index 4fdb8ad..fb5899f 100644 --- a/test/api/urls.test.js +++ b/test/api/urls.test.js @@ -47,6 +47,31 @@ describe('api/urls', function() { } }); + it('works with numbers', function() { + var cases = [{ + actual: urls.join('http://example.com', 42), + expected: 'http://example.com/42' + }, { + actual: urls.join('http://example.com', 0, 'foo'), + expected: 'http://example.com/0/foo' + }, { + actual: urls.join('http://example.com', 10, 'bar', 20), + expected: 'http://example.com/10/bar/20' + }]; + + for (var i = 0, ii = cases.length; i < ii; ++i) { + var c = cases[i]; + assert.deepEqual(c.actual, c.expected, 'case ' + i); + } + }); + + it('throws for invalid input', function() { + function call() { + urls.join('http://example.com', new Date()); + } + assert.throws(call, Error, 'join must be called with strings or numbers'); + }); + }); }); From e6af83d48fd29b21670d119b4fac5f9c7edaadf6 Mon Sep 17 00:00:00 2001 From: Tim Schaub Date: Wed, 5 Aug 2015 15:11:41 -0600 Subject: [PATCH 2/3] Additional request methods --- api/request.js | 26 +++++++++++++++++++++++++ test/api/request.test.js | 42 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+) diff --git a/api/request.js b/api/request.js index 7de8311..4976677 100644 --- a/api/request.js +++ b/api/request.js @@ -254,7 +254,33 @@ function post(config) { return request(assign({method: 'POST'}, config)); } +/** + * Issue a PUT request. + * @param {Object} config The request config. + * @return {Promise} A promise that resolves on a successful + * response. The object includes response and body properties, where the + * body is a JSON decoded object representing the response body. Any + * non-200 status will result in a rejection. + */ +function put(config) { + return request(assign({method: 'PUT'}, config)); +} + +/** + * Issue a DELETE request. + * @param {Object} config The request config. + * @return {Promise} A promise that resolves on a successful + * response. The object includes response and body properties, where the + * body is a JSON decoded object representing the response body. Any + * non-200 status will result in a rejection. + */ +function del(config) { + return request(assign({method: 'DELETE'}, config)); +} + exports.get = get; exports.post = post; +exports.put = put; +exports.del = del; exports.parseConfig = parseConfig; exports.request = request; diff --git a/test/api/request.test.js b/test/api/request.test.js index 7738473..78df711 100644 --- a/test/api/request.test.js +++ b/test/api/request.test.js @@ -352,6 +352,48 @@ describe('api/request', function() { }); + describe('post()', function() { + + it('calls request() with method set to POST', function() { + req.post({url: 'http://example.com'}); + assert.equal(http.request.callCount, 1); + var call = http.request.getCall(0); + assert.lengthOf(call.args, 2); + var config = call.args[0]; + assert.equal(config.method, 'POST'); + assert.equal(config.hostname, 'example.com'); + }); + + }); + + describe('put()', function() { + + it('calls request() with method set to PUT', function() { + req.put({url: 'http://example.com'}); + assert.equal(http.request.callCount, 1); + var call = http.request.getCall(0); + assert.lengthOf(call.args, 2); + var config = call.args[0]; + assert.equal(config.method, 'PUT'); + assert.equal(config.hostname, 'example.com'); + }); + + }); + + describe('del()', function() { + + it('calls request() with method set to DELETE', function() { + req.del({url: 'http://example.com'}); + assert.equal(http.request.callCount, 1); + var call = http.request.getCall(0); + assert.lengthOf(call.args, 2); + var config = call.args[0]; + assert.equal(config.method, 'DELETE'); + assert.equal(config.hostname, 'example.com'); + }); + + }); + describe('parseConfig()', function() { // {api_key: 'my-api-key'} var token = 'eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJhcGlfa2V5Ijoib' + From bbe20426bb1920068fb5c6da807b786b9799961e Mon Sep 17 00:00:00 2001 From: Tim Schaub Date: Wed, 5 Aug 2015 16:21:28 -0600 Subject: [PATCH 3/3] Tighter error handling --- api/errors.js | 38 +++++++++++++++++++++++-- api/request.js | 50 +++++++++++++++++++++++---------- test/api/errors.test.js | 30 ++++++++++++++++++-- test/api/request.test.js | 60 ++++++++++++++++++++++++++++++++++++---- 4 files changed, 152 insertions(+), 26 deletions(-) diff --git a/api/errors.js b/api/errors.js index 5e0ca7d..ce3d50e 100644 --- a/api/errors.js +++ b/api/errors.js @@ -21,10 +21,25 @@ ResponseError.prototype = new Error(); ResponseError.prototype.name = 'ResponseError'; /** - * An error that occurs when the client is unathorized to make the request. + * The request was bad (400). * @param {string} message Error message. * @param {XMLHttpRequest} response The response. - * @param {string} body Any parsed response body. + * @param {Object} body Any parsed response body (as JSON). + * @extends {ResponseError} + * @constructor + */ +function BadRequest(message, response, body) { + ResponseError.apply(this, arguments); +} + +BadRequest.prototype = new ResponseError(); +BadRequest.prototype.name = 'BadRequest'; + +/** + * The request requires user authentication (401). + * @param {string} message Error message. + * @param {XMLHttpRequest} response The response. + * @param {Object} body Any parsed response body (as JSON). * @extends {ResponseError} * @constructor */ @@ -36,7 +51,22 @@ Unauthorized.prototype = new ResponseError(); Unauthorized.prototype.name = 'Unauthorized'; /** - * An error that occurs the API returns an unexpected response. + * The client is forbidden from making the request (403). + * @param {string} message Error message. + * @param {XMLHttpRequest} response The response. + * @param {Object} body Any parsed response body (as JSON). + * @extends {ResponseError} + * @constructor + */ +function Forbidden(message, response, body) { + ResponseError.apply(this, arguments); +} + +Forbidden.prototype = new ResponseError(); +Forbidden.prototype.name = 'Forbidden'; + +/** + * The API returns an unexpected response. * @param {string} message Error message. * @param {XMLHttpRequest} response The response. * @param {string} body Any parsed response body. @@ -63,6 +93,8 @@ AbortedRequest.prototype = new Error(); AbortedRequest.prototype.name = 'AbortedRequest'; exports.ResponseError = ResponseError; +exports.BadRequest = BadRequest; exports.Unauthorized = Unauthorized; +exports.Forbidden = Forbidden; exports.UnexpectedResponse = UnexpectedResponse; exports.AbortedRequest = AbortedRequest; diff --git a/api/request.js b/api/request.js index 4976677..bb114ef 100644 --- a/api/request.js +++ b/api/request.js @@ -80,6 +80,28 @@ function parseConfig(config) { return options; } +/** + * Check if the response represents an error. + * @param {IncomingMessage} response The response. + * @param {Object} body Any parsed body (as JSON). + * @return {errors.ResponseError} A response error (or null if none). + */ +function errorCheck(response, body) { + var err = null; + var status = response.statusCode; + if (status === 400) { + err = new errors.BadRequest('Bad request', response, body); + } else if (status === 401) { + err = new errors.Unauthorized('Unauthorized', response, body); + } else if (status === 403) { + err = new errors.Forbidden('Forbidden', response, body); + } else if (!(status >= 200 && status < 300)) { + err = new errors.UnexpectedResponse('Unexpected response status: ' + + status, response); + } + return err; +} + /** * Create a handler for JSON API responses. * @param {function(Object)} resolve Called on success with response and body @@ -101,15 +123,17 @@ function createResponseHandler(resolve, reject, info) { createResponseHandler(resolve, reject, info)); return; } + if (info.stream) { - if (!(status >= 200 && status < 300)) { - reject(new errors.UnexpectedResponse('Unexpected response status: ' + - status, response)); + var streamErr = errorCheck(response, null); + if (streamErr) { + reject(streamErr); } else { resolve({response: response, body: null}); } return; } + var data = ''; response.on('data', function(chunk) { data += String(chunk); @@ -128,12 +152,7 @@ function createResponseHandler(resolve, reject, info) { } var body = null; var err = null; - if (status === 401) { - err = new errors.Unauthorized('Unauthorized', response, data); - } else if (!(status >= 200 && status < 300)) { - err = new errors.UnexpectedResponse('Unexpected response status: ' + - status, response, data); - } else if (data) { + if (data) { try { body = JSON.parse(data); } catch (parseErr) { @@ -142,14 +161,17 @@ function createResponseHandler(resolve, reject, info) { parseErr.stack + '\n', response, data); } } + + err = errorCheck(response, body) || err; + if (err) { reject(err); - return; + } else { + resolve({ + response: response, + body: body + }); } - resolve({ - response: response, - body: body - }); }); }; } diff --git a/test/api/errors.test.js b/test/api/errors.test.js index fa16e0d..de4b6d3 100644 --- a/test/api/errors.test.js +++ b/test/api/errors.test.js @@ -6,7 +6,7 @@ var errors = require('../../api/errors'); describe('api/errors', function() { describe('ResponseError', function() { - it('is a generic response error', function() { + it('is a base class for response errors', function() { var message = 'foo'; var response = {}; var err = new errors.ResponseError(message, response); @@ -16,8 +16,20 @@ describe('api/errors', function() { }); }); + describe('BadRequest', function() { + it('represents a 400 response', function() { + var message = 'foo'; + var response = {}; + var err = new errors.BadRequest(message, response); + assert.equal(err.message, message); + assert.equal(err.response, response); + assert.instanceOf(err, errors.ResponseError); + assert.instanceOf(err, Error); + }); + }); + describe('Unauthorized', function() { - it('represents an unauthorized request', function() { + it('represents a 401 response', function() { var message = 'foo'; var response = {}; var err = new errors.Unauthorized(message, response); @@ -28,8 +40,20 @@ describe('api/errors', function() { }); }); + describe('Forbidden', function() { + it('represents a 403 response', function() { + var message = 'foo'; + var response = {}; + var err = new errors.Forbidden(message, response); + assert.equal(err.message, message); + assert.equal(err.response, response); + assert.instanceOf(err, errors.ResponseError); + assert.instanceOf(err, Error); + }); + }); + describe('UnexpectedResponse', function() { - it('represents authentication with bad credentials', function() { + it('represents a response that we do not expect', function() { var message = 'foo'; var response = {}; var err = new errors.UnexpectedResponse(message, response); diff --git a/test/api/request.test.js b/test/api/request.test.js index 78df711..0a61f53 100644 --- a/test/api/request.test.js +++ b/test/api/request.test.js @@ -207,10 +207,10 @@ describe('api/request', function() { response.emit('end'); }); - it('rejects for non 2xx response', function(done) { + it('rejects with UnexpectedResponse for 500 response', function(done) { var response = new stream.Readable(); response.statusCode = 500; - var body = 'server error'; + var body = 'server error (maybe a secret in the stack trace)'; var promise = request({url: 'http://example.com'}); promise.then(function(obj) { @@ -218,7 +218,7 @@ describe('api/request', function() { }, function(err) { assert.instanceOf(err, errors.UnexpectedResponse); assert.include(err.message, 'Unexpected response status: 500'); - assert.equal(err.body, body); + assert.equal(err.body, null); // don't leak unexpected responses done(); }).catch(done); @@ -231,10 +231,34 @@ describe('api/request', function() { response.emit('end'); }); + it('rejects with BadRequest for 400', function(done) { + var response = new stream.Readable(); + response.statusCode = 400; + var body = {message: 'Invalid email or password', errors: []}; + + var promise = request({url: 'http://example.com'}); + promise.then(function(obj) { + done(new Error('Expected promise to be rejected')); + }, function(err) { + assert.instanceOf(err, errors.BadRequest); + assert.include(err.message, 'Bad request'); + assert.deepEqual(err.body, body); + done(); + }).catch(done); + + assert.equal(http.request.callCount, 1); + var args = http.request.getCall(0).args; + assert.lengthOf(args, 2); + var callback = args[1]; + callback(response); + response.emit('data', JSON.stringify(body)); + response.emit('end'); + }); + it('rejects with Unauthorized for 401', function(done) { var response = new stream.Readable(); response.statusCode = 401; - var body = 'unauthorized'; + var body = {message: 'Invalid email or password', errors: []}; var promise = request({url: 'http://example.com'}); promise.then(function(obj) { @@ -242,7 +266,7 @@ describe('api/request', function() { }, function(err) { assert.instanceOf(err, errors.Unauthorized); assert.include(err.message, 'Unauthorized'); - assert.equal(err.body, body); + assert.deepEqual(err.body, body); done(); }).catch(done); @@ -251,7 +275,31 @@ describe('api/request', function() { assert.lengthOf(args, 2); var callback = args[1]; callback(response); - response.emit('data', body); + response.emit('data', JSON.stringify(body)); + response.emit('end'); + }); + + it('rejects with Forbidden for 403', function(done) { + var response = new stream.Readable(); + response.statusCode = 403; + var body = {message: 'some user info here'}; + + var promise = request({url: 'http://example.com'}); + promise.then(function(obj) { + done(new Error('Expected promise to be rejected')); + }, function(err) { + assert.instanceOf(err, errors.Forbidden); + assert.include(err.message, 'Forbidden'); + assert.deepEqual(err.body, body); + done(); + }).catch(done); + + assert.equal(http.request.callCount, 1); + var args = http.request.getCall(0).args; + assert.lengthOf(args, 2); + var callback = args[1]; + callback(response); + response.emit('data', JSON.stringify(body)); response.emit('end'); });