Skip to content

datastore: properly parse HTTP responses from the API #913

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

Merged
callmehiphop merged 1 commit into from
Oct 14, 2015
Merged

datastore: properly parse HTTP responses from the API #913

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
75 changes: 47 additions & 28 deletions lib/common/util.js
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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