Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Pass apiResponse along with errors #741

Merged
merged 2 commits into from
Jul 24, 2015
Merged

Pass apiResponse along with errors #741

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3ef99fb
core: decorate requests before authorizedReqOpts earlier
stephenplusplus Jul 24, 2015
0587618
core: fix error handling in util.handleResp
stephenplusplus Jul 24, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 22 additions & 31 deletions lib/common/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,45 +182,36 @@ nodeutil.inherits(ApiError, Error);
function handleResp(err, resp, body, callback) {
callback = callback || noop;

if (err) {
callback(err);
return;
}

var parsedApiResponse = util.parseApiResp(resp, body);

if (parsedApiResponse.err) {
callback(parsedApiResponse.err);
return;
}
var parsedResp = util.parseApiResp(err, resp, body);

callback(null, parsedApiResponse.body, parsedApiResponse.resp);
callback(parsedResp.err, parsedResp.body, parsedResp.resp);
}

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.',
Expand All @@ -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;
Expand Down Expand Up @@ -643,6 +634,8 @@ function makeAuthorizedRequestFactory(config) {
return;
}

authorizedReqOpts = util.decorateRequest(authorizedReqOpts);

if (options && options.onAuthorized) {
options.onAuthorized(null, authorizedReqOpts);
} else {
Expand Down Expand Up @@ -711,15 +704,13 @@ function makeRequest(reqOpts, config, callback) {

config = config || {};

reqOpts = util.decorateRequest(reqOpts);

var options = {
request: request,

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);
}
};
Expand Down
68 changes: 36 additions & 32 deletions test/common/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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.');
Expand All @@ -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);
Expand All @@ -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');
});
Expand Down Expand Up @@ -610,6 +613,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' };

Expand Down Expand Up @@ -723,10 +737,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();
}
});
Expand All @@ -735,8 +754,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();
};
Expand Down Expand Up @@ -937,18 +961,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);

Expand Down Expand Up @@ -981,19 +998,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);
Expand Down