From 3ef99fbb3e00ff1a3c1bff1959fd1775a69870c3 Mon Sep 17 00:00:00 2001 From: Stephen Sawchuk Date: Fri, 24 Jul 2015 13:41:33 -0400 Subject: [PATCH 1/2] core: decorate requests before authorizedReqOpts earlier --- lib/common/util.js | 6 +++--- test/common/util.js | 49 +++++++++++++++++++++++---------------------- 2 files changed, 28 insertions(+), 27 deletions(-) diff --git a/lib/common/util.js b/lib/common/util.js index 9b690379842..bdbaeb4765f 100644 --- a/lib/common/util.js +++ b/lib/common/util.js @@ -207,7 +207,7 @@ util.handleResp = handleResp; * @return {object} parsedResponse - The parsed response. * @param {?error} parsedResponse.err - An error detected. * @param {object} parsedResponse.resp - The original response object. - * @param {*} parsedREsponse.body - The original body value provided will try to + * @param {*} parsedResponse.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. */ @@ -643,6 +643,8 @@ function makeAuthorizedRequestFactory(config) { return; } + authorizedReqOpts = util.decorateRequest(authorizedReqOpts); + if (options && options.onAuthorized) { options.onAuthorized(null, authorizedReqOpts); } else { @@ -711,8 +713,6 @@ function makeRequest(reqOpts, config, callback) { config = config || {}; - reqOpts = util.decorateRequest(reqOpts); - var options = { request: request, diff --git a/test/common/util.js b/test/common/util.js index 60f61d1e03d..4f744725505 100644 --- a/test/common/util.js +++ b/test/common/util.js @@ -610,6 +610,17 @@ describe('common/util', function() { }); }); + it('should decorate the request', function(done) { + var reqOpts = { a: 'b', c: 'd' }; + + utilOverrides.decorateRequest = function(reqOpts_) { + assert.strictEqual(reqOpts_, reqOpts); + done(); + }; + + makeAuthorizedRequest(reqOpts, { onAuthorized: assert.ifError }); + }); + it('should pass options back to onAuthorized callback', function(done) { var reqOpts = { a: 'b', c: 'd' }; @@ -723,10 +734,15 @@ describe('common/util', function() { }); it('should return the authorized request to callback', function(done) { + utilOverrides.decorateRequest = function(reqOpts_) { + assert.strictEqual(reqOpts_, reqOpts); + return reqOpts; + }; + var makeAuthorizedRequest = util.makeAuthorizedRequestFactory(); makeAuthorizedRequest(reqOpts, { onAuthorized: function(err, authorizedReqOpts) { - assert.deepEqual(authorizedReqOpts, reqOpts); + assert.strictEqual(authorizedReqOpts, reqOpts); done(); } }); @@ -735,8 +751,13 @@ describe('common/util', function() { it('should make request with correct options', function(done) { var config = { a: 'b', c: 'd' }; + utilOverrides.decorateRequest = function(reqOpts_) { + assert.strictEqual(reqOpts_, reqOpts); + return reqOpts; + }; + utilOverrides.makeRequest = function(authorizedReqOpts, cfg, cb) { - assert.deepEqual(authorizedReqOpts, reqOpts); + assert.strictEqual(authorizedReqOpts, reqOpts); assert.deepEqual(cfg, config); cb(); }; @@ -937,18 +958,11 @@ describe('common/util', function() { }); describe('makeRequest', function() { - var PKG = require('../../package.json'); - var USER_AGENT = 'gcloud-node/' + PKG.version; var reqOpts = { a: 'b', c: 'd' }; - var expectedReqOpts = extend(true, {}, reqOpts, { - headers: { - 'User-Agent': USER_AGENT - } - }); function testDefaultRetryRequestConfig(done) { - return function(reqOpts, config) { - assert.deepEqual(reqOpts, expectedReqOpts); + return function(reqOpts_, config) { + assert.strictEqual(reqOpts_, reqOpts); assert.equal(config.retries, 3); assert.strictEqual(config.request, fakeRequest); @@ -981,19 +995,6 @@ describe('common/util', function() { }; } - it('should decorate the request', function(done) { - var reqOpts = { a: 'b', c: 'd' }; - - retryRequestOverride = util.noop; - - utilOverrides.decorateRequest = function(reqOpts_) { - assert.strictEqual(reqOpts_, reqOpts); - done(); - }; - - util.makeRequest(reqOpts, {}, assert.ifError); - }); - describe('stream mode', function() { it('should pass the default options to retryRequest', function(done) { retryRequestOverride = testDefaultRetryRequestConfig(done); From 0587618aec5dd04b678a7a6157f0ef0777afe0a4 Mon Sep 17 00:00:00 2001 From: Stephen Sawchuk Date: Fri, 24 Jul 2015 14:36:00 -0400 Subject: [PATCH 2/2] core: fix error handling in util.handleResp --- lib/common/util.js | 49 ++++++++++++++++++--------------------------- test/common/util.js | 19 ++++++++++-------- 2 files changed, 31 insertions(+), 37 deletions(-) diff --git a/lib/common/util.js b/lib/common/util.js index bdbaeb4765f..f95c9ce990b 100644 --- a/lib/common/util.js +++ b/lib/common/util.js @@ -182,19 +182,9 @@ nodeutil.inherits(ApiError, Error); function handleResp(err, resp, body, callback) { callback = callback || noop; - if (err) { - callback(err); - return; - } - - var parsedApiResponse = util.parseApiResp(resp, body); + var parsedResp = util.parseApiResp(err, resp, body); - if (parsedApiResponse.err) { - callback(parsedApiResponse.err); - return; - } - - callback(null, parsedApiResponse.body, parsedApiResponse.resp); + callback(parsedResp.err, parsedResp.body, parsedResp.resp); } util.handleResp = handleResp; @@ -202,25 +192,26 @@ util.handleResp = handleResp; /** * From an HTTP response, generate an error if one occurred. * + * @param {*} err - Error value. * @param {*} resp - Response value. * @param {*=} body - Body value. - * @return {object} parsedResponse - The parsed response. - * @param {?error} parsedResponse.err - An error detected. - * @param {object} parsedResponse.resp - The original response object. - * @param {*} parsedResponse.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. + * @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. */ -function parseApiResp(resp, body) { - var parsedResponse = { - err: null, - resp: resp, - body: body +function parseApiResp(err, resp, body) { + var parsedResp = { + err: err || null, + body: body, + resp: resp }; if (resp.statusCode < 200 || resp.statusCode > 299) { // Unknown error. Format according to ApiError standard. - parsedResponse.err = new ApiError({ + parsedResp.err = new ApiError({ errors: [], code: resp.statusCode, message: 'Error during request.', @@ -230,16 +221,16 @@ function parseApiResp(resp, body) { if (util.is(body, 'string')) { try { - parsedResponse.body = JSON.parse(body); + parsedResp.body = JSON.parse(body); } catch(err) {} } - if (parsedResponse.body && parsedResponse.body.error) { + if (parsedResp.body && parsedResp.body.error) { // Error from JSON API. - parsedResponse.err = new ApiError(parsedResponse.body.error); + parsedResp.err = new ApiError(parsedResp.body.error); } - return parsedResponse; + return parsedResp; } util.parseApiResp = parseApiResp; @@ -719,7 +710,7 @@ function makeRequest(reqOpts, config, callback) { retries: config.autoRetry !== false ? config.maxRetries || 3 : 0, shouldRetryFn: function(resp) { - var err = util.parseApiResp(resp).err; + var err = util.parseApiResp(null, resp).err; return err && util.shouldRetryRequest(err); } }; diff --git a/test/common/util.js b/test/common/util.js index 4f744725505..0953f1d3adb 100644 --- a/test/common/util.js +++ b/test/common/util.js @@ -158,32 +158,35 @@ describe('common/util', function() { it('should handle errors', function(done) { var error = new Error('Error.'); - util.handleResp(error, null, null, function(err) { + util.handleResp(error, {}, null, function(err) { assert.strictEqual(err, error); done(); }); }); it('should parse response', function(done) { + var err = { a: 'b', c: 'd' }; var resp = { a: 'b', c: 'd' }; var body = { a: 'b', c: 'd' }; + var returnedErr = { a: 'b', c: 'd' }; var returnedBody = { a: 'b', c: 'd' }; var returnedResp = { a: 'b', c: 'd' }; - utilOverrides.parseApiResp = function(resp_, body_) { + utilOverrides.parseApiResp = function(err_, resp_, body_) { + assert.strictEqual(err_, err); assert.strictEqual(resp_, resp); assert.strictEqual(body_, body); return { - err: null, + err: returnedErr, body: returnedBody, resp: returnedResp }; }; - util.handleResp(null, resp, body, function(err, body, resp) { - assert.ifError(err); + util.handleResp(err, resp, body, function(err, body, resp) { + assert.strictEqual(err, returnedErr); assert.strictEqual(body, returnedBody); assert.strictEqual(resp, returnedResp); done(); @@ -206,7 +209,7 @@ describe('common/util', function() { describe('parseApiResp', function() { it('should return err code if there are not other errors', function() { - var parsedApiResp = util.parseApiResp({ statusCode: 400 }); + var parsedApiResp = util.parseApiResp(null, { statusCode: 400 }); assert.strictEqual(parsedApiResp.err.code, 400); assert.strictEqual(parsedApiResp.err.message, 'Error during request.'); @@ -219,7 +222,7 @@ describe('common/util', function() { message: 'an error occurred' }; - var parsedApiResp = util.parseApiResp({}, { error: apiErr }); + var parsedApiResp = util.parseApiResp(null, {}, { error: apiErr }); assert.deepEqual(parsedApiResp.err.errors, apiErr.errors); assert.strictEqual(parsedApiResp.err.code, apiErr.code); @@ -229,7 +232,7 @@ describe('common/util', function() { it('should try to parse JSON if body is string', function() { var body = '{ "foo": "bar" }'; - var parsedApiResp = util.parseApiResp({}, body); + var parsedApiResp = util.parseApiResp(null, {}, body); assert.strictEqual(parsedApiResp.body.foo, 'bar'); });