Skip to content

Commit

Permalink
Merge pull request #913 from stephenplusplus/spp--datastore-response-…
Browse files Browse the repository at this point in the history 
…parsing

datastore: properly parse HTTP responses from the API
  • Loading branch information
@callmehiphop
callmehiphop committed Oct 14, 2015
2 parents 9fd2420 + 8b2bbf2 commit e2371f5
Show file tree
Hide file tree
Showing 4 changed files with 264 additions and 145 deletions.
75 changes: 47 additions & 28 deletions lib/common/util.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,58 +139,77 @@ 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);
}

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
Expand Down Expand Up @@ -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);
}
};
Expand Down
48 changes: 24 additions & 24 deletions lib/datastore/request.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'
}
Expand All @@ -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);
});
}
});
};
Expand Down
92 changes: 57 additions & 35 deletions test/common/util.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,84 +181,106 @@ 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();
});
});

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' }],
code: 400,
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);
});
});

Expand Down Expand Up @@ -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) {
Expand Down
Loading

0 comments on commit e2371f5

Please sign in to comment.