diff --git a/lib/common/util.js b/lib/common/util.js index f3e3e1694da..0f69dd61bd9 100644 --- a/lib/common/util.js +++ b/lib/common/util.js @@ -139,7 +139,12 @@ util.ApiError = function(errorBody) { function handleResp(err, resp, body, callback) { callback = callback || noop; - var parsedResp = util.parseApiResp(err, resp, body); + var parsedResp = extend( + true, + { err: err || null }, + util.parseHttpRespMessage(resp), + util.parseHttpRespBody(body) + ); callback(parsedResp.err, parsedResp.body, parsedResp.resp); } @@ -147,50 +152,64 @@ function handleResp(err, resp, body, callback) { util.handleResp = handleResp; /** - * From an HTTP response, generate an error if one occurred. + * Sniff an incoming HTTP response message for errors. * - * @param {*} err - Error value. - * @param {*} resp - Response value. - * @param {*=} body - Body value. - * @return {object} parsedResp - The parsed response. - * @param {?error} parsedResp.err - An error detected. - * @param {object} parsedResp.resp - The original response object. - * @param {*} parsedResp.body - The original body value provided will try to be - * JSON.parse'd. If it's successful, the parsed value will be returned here, - * otherwise the original value. + * @param {object} httpRespMessage - An incoming HTTP response message from + * `request`. + * @return {object} parsedHttpRespMessage - The parsed response. + * @param {?error} parsedHttpRespMessage.err - An error detected. + * @param {object} parsedHttpRespMessage.resp - The original response object. */ -function parseApiResp(err, resp, body) { - var parsedResp = { - err: err || null, - body: body, - resp: resp +function parseHttpRespMessage(httpRespMessage) { + var parsedHttpRespMessage = { + resp: httpRespMessage }; - if (resp && (resp.statusCode < 200 || resp.statusCode > 299)) { + if (httpRespMessage.statusCode < 200 || httpRespMessage.statusCode > 299) { // Unknown error. Format according to ApiError standard. - parsedResp.err = new util.ApiError({ + parsedHttpRespMessage.err = new util.ApiError({ errors: [], - code: resp.statusCode, - message: resp.statusMessage, - response: resp + code: httpRespMessage.statusCode, + message: httpRespMessage.statusMessage, + response: httpRespMessage }); } + return parsedHttpRespMessage; +} + +util.parseHttpRespMessage = parseHttpRespMessage; + +/** + * Parse the response body from an HTTP request. + * + * @param {object} body - The response body. + * @return {object} parsedHttpRespMessage - The parsed response. + * @param {?error} parsedHttpRespMessage.err - An error detected. + * @param {object} parsedHttpRespMessage.body - The original body value provided + * will try to be JSON.parse'd. If it's successful, the parsed value will be + * returned here, otherwise the original value. + */ +function parseHttpRespBody(body) { + var parsedHttpRespBody = { + body: body + }; + if (is.string(body)) { try { - parsedResp.body = JSON.parse(body); + parsedHttpRespBody.body = JSON.parse(body); } catch(err) {} } - if (parsedResp.body && parsedResp.body.error) { + if (parsedHttpRespBody.body && parsedHttpRespBody.body.error) { // Error from JSON API. - parsedResp.err = new util.ApiError(parsedResp.body.error); + parsedHttpRespBody.err = new util.ApiError(parsedHttpRespBody.body.error); } - return parsedResp; + return parsedHttpRespBody; } -util.parseApiResp = parseApiResp; +util.parseHttpRespBody = parseHttpRespBody; /** * Take a Duplexify stream, fetch an authenticated connection header, and create @@ -406,8 +425,8 @@ function makeRequest(reqOpts, config, callback) { retries: config.autoRetry !== false ? config.maxRetries || 3 : 0, - shouldRetryFn: function(resp) { - var err = util.parseApiResp(null, resp).err; + shouldRetryFn: function(httpRespMessage) { + var err = util.parseHttpRespMessage(httpRespMessage).err; return err && util.shouldRetryRequest(err); } }; diff --git a/lib/datastore/request.js b/lib/datastore/request.js index eb73093e0f8..af72ed2fb74 100644 --- a/lib/datastore/request.js +++ b/lib/datastore/request.js @@ -771,6 +771,8 @@ DatastoreRequest.prototype.makeReq_ = function(method, body, callback) { projectId: this.projectId, method: method }), + body: is.empty(body) ? '' : pbRequest, + encoding: null, headers: { 'Content-Type': 'application/x-protobuf' } @@ -779,34 +781,32 @@ DatastoreRequest.prototype.makeReq_ = function(method, body, callback) { this.makeAuthenticatedRequest_(reqOpts, { onAuthenticated: function(err, authenticatedReqOpts) { if (err) { - callback(err, null); // TODO(ryanseys): What goes as third parameter? + callback(err, null); return; } - authenticatedReqOpts.headers = authenticatedReqOpts.headers || {}; - authenticatedReqOpts.headers['Content-Length'] = pbRequest.length; - - var apiRequest = request(authenticatedReqOpts); - - apiRequest.on('error', callback); - - apiRequest.on('response', function(resp) { - var buffer = new Buffer(''); - resp.on('data', function(chunk) { - buffer = Buffer.concat([buffer, chunk]); - }); - resp.on('end', function() { - util.handleResp(null, resp, buffer.toString(), function(err, result) { - if (err) { - callback(err, null, result); - return; - } - callback(null, pbResponse.decode(buffer), result); - }); - }); - }); + request(authenticatedReqOpts, function(err, resp, body) { + if (err) { + callback(err, null); + return; + } + + var parsedResp = util.parseHttpRespMessage(resp); + + if (parsedResp.err) { + callback(parsedResp.err, null, parsedResp.resp); + return; + } - apiRequest.end(pbRequest); + var parsedBody = util.parseHttpRespBody(pbResponse.decode(body)); + + if (parsedBody.err) { + callback(parsedBody.err, null, parsedResp.resp); + return; + } + + callback(null, parsedBody.body, resp); + }); } }); }; diff --git a/test/common/util.js b/test/common/util.js index d97758f3ab3..a99a3645b8a 100644 --- a/test/common/util.js +++ b/test/common/util.js @@ -181,22 +181,26 @@ describe('common/util', function() { var returnedBody = { a: 'b', c: 'd' }; var returnedResp = { a: 'b', c: 'd' }; - utilOverrides.parseApiResp = function(err_, resp_, body_) { - assert.strictEqual(err_, err); + utilOverrides.parseHttpRespMessage = function(resp_) { assert.strictEqual(resp_, resp); - assert.strictEqual(body_, body); return { - err: returnedErr, - body: returnedBody, resp: returnedResp }; }; + utilOverrides.parseHttpRespBody = function(body_) { + assert.strictEqual(body_, body); + + return { + body: returnedBody + }; + }; + util.handleResp(err, resp, body, function(err, body, resp) { - assert.strictEqual(err, returnedErr); - assert.strictEqual(body, returnedBody); - assert.strictEqual(resp, returnedResp); + assert.deepEqual(err, returnedErr); + assert.deepEqual(body, returnedBody); + assert.deepEqual(resp, returnedResp); done(); }); }); @@ -204,41 +208,53 @@ describe('common/util', function() { it('should parse response for error', function(done) { var error = new Error('Error.'); - utilOverrides.parseApiResp = function() { + utilOverrides.parseHttpRespMessage = function() { return { err: error }; }; util.handleResp(null, {}, {}, function(err) { - assert.strictEqual(err, error); + assert.deepEqual(err, error); + done(); + }); + }); + + it('should parse body for error', function(done) { + var error = new Error('Error.'); + + utilOverrides.parseHttpRespBody = function() { + return { err: error }; + }; + + util.handleResp(null, {}, {}, function(err) { + assert.deepEqual(err, error); done(); }); }); }); - describe('parseApiResp', function() { - describe('non-200s response status', function() { - it('should build ApiError with status and message', function(done) { - var error = { statusCode: 400, statusMessage: 'Not Good' }; + describe('parseHttpRespMessage', function() { + it('should build ApiError with non-200 status and message', function(done) { + var httpRespMessage = { statusCode: 400, statusMessage: 'Not Good' }; - utilOverrides.ApiError = function(error_) { - assert.strictEqual(error_.code, error.statusCode); - assert.strictEqual(error_.message, error.statusMessage); - assert.strictEqual(error_.response, error); + utilOverrides.ApiError = function(error_) { + assert.strictEqual(error_.code, httpRespMessage.statusCode); + assert.strictEqual(error_.message, httpRespMessage.statusMessage); + assert.strictEqual(error_.response, httpRespMessage); - done(); - }; + done(); + }; - util.parseApiResp(null, error); - }); + util.parseHttpRespMessage(httpRespMessage); }); - it('should not throw when there is just an error', function() { - assert.doesNotThrow(function() { - var error = {}; - util.parseApiResp(error); - }); + it('should return the original response message', function() { + var httpRespMessage = {}; + var parsedHttpRespMessage = util.parseHttpRespMessage(httpRespMessage); + assert.strictEqual(parsedHttpRespMessage.resp, httpRespMessage); }); + }); + describe('parseHttpRespBody', function() { it('should detect body errors', function() { var apiErr = { errors: [{ foo: 'bar' }], @@ -246,19 +262,25 @@ describe('common/util', function() { message: 'an error occurred' }; - var parsedApiResp = util.parseApiResp(null, {}, { error: apiErr }); + var parsedHttpRespBody = util.parseHttpRespBody({ error: apiErr }); - assert.deepEqual(parsedApiResp.err.errors, apiErr.errors); - assert.strictEqual(parsedApiResp.err.code, apiErr.code); - assert.deepEqual(parsedApiResp.err.message, apiErr.message); + assert.deepEqual(parsedHttpRespBody.err.errors, apiErr.errors); + assert.strictEqual(parsedHttpRespBody.err.code, apiErr.code); + assert.deepEqual(parsedHttpRespBody.err.message, apiErr.message); }); it('should try to parse JSON if body is string', function() { - var body = '{ "foo": "bar" }'; + var httpRespBody = '{ "foo": "bar" }'; + var parsedHttpRespBody = util.parseHttpRespBody(httpRespBody); + + assert.strictEqual(parsedHttpRespBody.body.foo, 'bar'); + }); - var parsedApiResp = util.parseApiResp(null, {}, body); + it('should return the original body', function() { + var httpRespBody = {}; + var parsedHttpRespBody = util.parseHttpRespBody(httpRespBody); - assert.strictEqual(parsedApiResp.body.foo, 'bar'); + assert.strictEqual(parsedHttpRespBody.body, httpRespBody); }); }); @@ -692,7 +714,7 @@ describe('common/util', function() { assert.strictEqual(config.request, fakeRequest); var error = new Error('Error.'); - utilOverrides.parseApiResp = function() { + utilOverrides.parseHttpRespMessage = function() { return { err: error }; }; utilOverrides.shouldRetryRequest = function(err) { diff --git a/test/datastore/request.js b/test/datastore/request.js index 88786e9d5a8..1adf425f40c 100644 --- a/test/datastore/request.js +++ b/test/datastore/request.js @@ -68,6 +68,16 @@ fakeEntity = Object.keys(entity).reduce(function(fakeEntity, methodName) { return fakeEntity; }, {}); +var utilOverrides = {}; +var fakeUtil; +fakeUtil = Object.keys(util).reduce(function(fakeUtil, methodName) { + fakeUtil[methodName] = function() { + var method = utilOverrides[methodName] || util[methodName]; + return method.apply(this, arguments); + }; + return fakeUtil; +}, {}); + var extended = false; var fakeStreamRouter = { extend: function(Class, methods) { @@ -90,6 +100,7 @@ describe('Request', function() { before(function() { mockery.registerMock('./entity.js', fakeEntity); + mockery.registerMock('../common/util.js', fakeUtil); mockery.registerMock('./pb.js', pb); mockery.registerMock('../common/stream-router.js', fakeStreamRouter); mockery.registerMock('request', fakeRequest); @@ -111,6 +122,7 @@ describe('Request', function() { path: ['Company', 123] }); entityOverrides = {}; + utilOverrides = {}; requestOverride = null; request = new Request(); request.apiEndpoint = CUSTOM_ENDPOINT; @@ -870,7 +882,6 @@ describe('Request', function() { it('should make API request', function(done) { var mockRequest = { mock: 'request' }; requestOverride = function(req) { - assert.equal(req.headers['Content-Length'], 2); assert.deepEqual(req, mockRequest); done(); return new stream.Writable(); @@ -897,41 +908,129 @@ describe('Request', function() { it('should send protobuf request', function(done) { var requestOptions = { mode: 'NON_TRANSACTIONAL' }; var decoded = new pb.CommitRequest(requestOptions).toBuffer(); - requestOverride = function() { - var stream = { on: util.noop }; - stream.end = function(data) { - assert.equal(String(data), String(decoded)); - done(); - }; - return stream; + requestOverride = function(req) { + assert.equal(String(req.body), String(decoded)); + done(); }; request.makeReq_('commit', requestOptions, util.noop); }); - it('should decode protobuf response', function(done) { - pbFakeMethodResponseDecode = function() { + it('should respect API host and port configuration', function(done) { + request.apiEndpoint = CUSTOM_ENDPOINT; + + requestOverride = function(req) { + assert.equal(req.uri.indexOf(CUSTOM_ENDPOINT), 0); done(); }; - requestOverride = function() { - var ws = new stream.Writable(); - setImmediate(function() { - ws.emit('response', ws); - ws.emit('end'); - }); - return ws; + + request.makeReq_('fakeMethod', util.noop); + }); + + it('should execute callback with error from request', function(done) { + var error = new Error('Error.'); + + requestOverride = function(req, callback) { + callback(error); }; + + request.makeReq_('fakeMethod', function(err) { + assert.strictEqual(err, error); + done(); + }); + }); + + it('should parse response', function(done) { + var resp = {}; + + requestOverride = function(req, callback) { + callback(null, resp); + }; + + utilOverrides.parseHttpRespMessage = function(resp_) { + assert.strictEqual(resp_, resp); + setImmediate(done); + return resp; + }; + request.makeReq_('fakeMethod', util.noop); }); - it('should respect API host and port configuration', function(done) { - request.apiEndpoint = CUSTOM_ENDPOINT; + it('should return error from parsed response', function(done) { + var error = new Error('Error.'); + var resp = {}; - requestOverride = function(req) { - assert.equal(req.uri.indexOf(CUSTOM_ENDPOINT), 0); + requestOverride = function(req, callback) { + callback(null, resp); + }; + + utilOverrides.parseHttpRespMessage = function() { + return { + err: error, + resp: resp + }; + }; + + request.makeReq_('fakeMethod', function(err, results, apiResponse) { + assert.strictEqual(err, error); + assert.strictEqual(results, null); + assert.strictEqual(apiResponse, resp); done(); - return new stream.Writable(); + }); + }); + + it('should parse body', function(done) { + var resp = {}; + var body = {}; + + requestOverride = function(req, callback) { + callback(null, resp, body); + }; + + utilOverrides.parseHttpRespBody = function() { + return { + body: body + }; }; + request.makeReq_('fakeMethod', function(err, results, apiResponse) { + assert.strictEqual(err, null); + assert.strictEqual(results, body); + assert.strictEqual(apiResponse, resp); + done(); + }); + }); + + it('should return error from parsed body', function(done) { + var error = new Error('Error.'); + var resp = {}; + var body = {}; + + requestOverride = function(req, callback) { + callback(null, resp, body); + }; + + utilOverrides.parseHttpRespBody = function() { + return { + err: error, + body: body + }; + }; + + request.makeReq_('fakeMethod', function(err, results, apiResponse) { + assert.strictEqual(err, error); + assert.strictEqual(results, null); + assert.strictEqual(apiResponse, resp); + done(); + }); + }); + + it('should decode the protobuf response', function(done) { + pbFakeMethodResponseDecode = function() { + done(); + }; + requestOverride = function(req, callback) { + callback(null, {}, new Buffer('')); + }; request.makeReq_('fakeMethod', util.noop); }); @@ -949,13 +1048,9 @@ describe('Request', function() { var expected = new pb.RollbackRequest({ transaction: request.id }).toBuffer(); - requestOverride = function() { - var stream = { on: util.noop, end: util.noop }; - stream.end = function(data) { - assert.deepEqual(data, expected); - done(); - }; - return stream; + requestOverride = function(req) { + assert.deepEqual(req.body, expected); + done(); }; request.makeReq_('rollback', util.noop); }); @@ -969,13 +1064,9 @@ describe('Request', function() { mode: 'TRANSACTIONAL', transaction: request.id }).toBuffer(); - requestOverride = function() { - var stream = { on: util.noop, end: util.noop }; - stream.end = function(data) { - assert.deepEqual(data, expected); - done(); - }; - return stream; + requestOverride = function(req) { + assert.deepEqual(req.body, expected); + done(); }; request.makeReq_('commit', util.noop); }); @@ -984,13 +1075,9 @@ describe('Request', function() { var expected = new pb.CommitRequest({ mode: 'NON_TRANSACTIONAL' }).toBuffer(); - requestOverride = function() { - var stream = { on: util.noop, end: util.noop }; - stream.end = function(data) { - assert.deepEqual(data, expected); - done(); - }; - return stream; + requestOverride = function(req) { + assert.deepEqual(req.body, expected); + done(); }; request.makeReq_('commit', util.noop); }); @@ -1005,26 +1092,17 @@ describe('Request', function() { transaction: request.id } }).toBuffer(); - requestOverride = function() { - var stream = { on: util.noop, end: util.noop }; - stream.end = function(data) { - assert.deepEqual(data, expected); - done(); - }; - return stream; + requestOverride = function(req) { + assert.deepEqual(req.body, expected); + done(); }; request.makeReq_('lookup', util.noop); }); it('should not attach transactional properties', function(done) { - var expected = new pb.LookupRequest().toBuffer(); - requestOverride = function() { - var ws = new stream.Writable(); - ws.end = function(data) { - assert.deepEqual(data, expected); - done(); - }; - return ws; + requestOverride = function(req) { + assert.strictEqual(req.body, ''); + done(); }; request.makeReq_('lookup', util.noop); });