Skip to content

Commit 8b2bbf2

Browse files
stephenplusplusstephenplusplus
committed
datastore: properly parse HTTP responses from the API
1 parent 9fd2420 commit 8b2bbf2

4 files changed

Lines changed: 264 additions & 145 deletions

File tree

‎lib/common/util.js‎

Lines changed: 47 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -139,58 +139,77 @@ util.ApiError = function(errorBody) {
139139
function handleResp(err, resp, body, callback) {
140140
callback = callback || noop;
141141

142-
var parsedResp = util.parseApiResp(err, resp, body);
142+
var parsedResp = extend(
143+
true,
144+
{ err: err || null },
145+
util.parseHttpRespMessage(resp),
146+
util.parseHttpRespBody(body)
147+
);
143148

144149
callback(parsedResp.err, parsedResp.body, parsedResp.resp);
145150
}
146151

147152
util.handleResp = handleResp;
148153

149154
/**
150-
* From an HTTP response, generate an error if one occurred.
155+
* Sniff an incoming HTTP response message for errors.
151156
*
152-
* @param {*} err - Error value.
153-
* @param {*} resp - Response value.
154-
* @param {*=} body - Body value.
155-
* @return {object} parsedResp - The parsed response.
156-
* @param {?error} parsedResp.err - An error detected.
157-
* @param {object} parsedResp.resp - The original response object.
158-
* @param {*} parsedResp.body - The original body value provided will try to be
159-
* JSON.parse'd. If it's successful, the parsed value will be returned here,
160-
* otherwise the original value.
157+
* @param {object} httpRespMessage - An incoming HTTP response message from
158+
* `request`.
159+
* @return {object} parsedHttpRespMessage - The parsed response.
160+
* @param {?error} parsedHttpRespMessage.err - An error detected.
161+
* @param {object} parsedHttpRespMessage.resp - The original response object.
161162
*/
162-
function parseApiResp(err, resp, body) {
163-
var parsedResp = {
164-
err: err || null,
165-
body: body,
166-
resp: resp
163+
function parseHttpRespMessage(httpRespMessage) {
164+
var parsedHttpRespMessage = {
165+
resp: httpRespMessage
167166
};
168167

169-
if (resp && (resp.statusCode < 200 || resp.statusCode > 299)) {
168+
if (httpRespMessage.statusCode < 200 || httpRespMessage.statusCode > 299) {
170169
// Unknown error. Format according to ApiError standard.
171-
parsedResp.err = new util.ApiError({
170+
parsedHttpRespMessage.err = new util.ApiError({
172171
errors: [],
173-
code: resp.statusCode,
174-
message: resp.statusMessage,
175-
response: resp
172+
code: httpRespMessage.statusCode,
173+
message: httpRespMessage.statusMessage,
174+
response: httpRespMessage
176175
});
177176
}
178177

178+
return parsedHttpRespMessage;
179+
}
180+
181+
util.parseHttpRespMessage = parseHttpRespMessage;
182+
183+
/**
184+
* Parse the response body from an HTTP request.
185+
*
186+
* @param {object} body - The response body.
187+
* @return {object} parsedHttpRespMessage - The parsed response.
188+
* @param {?error} parsedHttpRespMessage.err - An error detected.
189+
* @param {object} parsedHttpRespMessage.body - The original body value provided
190+
* will try to be JSON.parse'd. If it's successful, the parsed value will be
191+
* returned here, otherwise the original value.
192+
*/
193+
function parseHttpRespBody(body) {
194+
var parsedHttpRespBody = {
195+
body: body
196+
};
197+
179198
if (is.string(body)) {
180199
try {
181-
parsedResp.body = JSON.parse(body);
200+
parsedHttpRespBody.body = JSON.parse(body);
182201
} catch(err) {}
183202
}
184203

185-
if (parsedResp.body && parsedResp.body.error) {
204+
if (parsedHttpRespBody.body && parsedHttpRespBody.body.error) {
186205
// Error from JSON API.
187-
parsedResp.err = new util.ApiError(parsedResp.body.error);
206+
parsedHttpRespBody.err = new util.ApiError(parsedHttpRespBody.body.error);
188207
}
189208

190-
return parsedResp;
209+
return parsedHttpRespBody;
191210
}
192211

193-
util.parseApiResp = parseApiResp;
212+
util.parseHttpRespBody = parseHttpRespBody;
194213

195214
/**
196215
* Take a Duplexify stream, fetch an authenticated connection header, and create
@@ -406,8 +425,8 @@ function makeRequest(reqOpts, config, callback) {
406425

407426
retries: config.autoRetry !== false ? config.maxRetries || 3 : 0,
408427

409-
shouldRetryFn: function(resp) {
410-
var err = util.parseApiResp(null, resp).err;
428+
shouldRetryFn: function(httpRespMessage) {
429+
var err = util.parseHttpRespMessage(httpRespMessage).err;
411430
return err && util.shouldRetryRequest(err);
412431
}
413432
};

‎lib/datastore/request.js‎

Lines changed: 24 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -771,6 +771,8 @@ DatastoreRequest.prototype.makeReq_ = function(method, body, callback) {
771771
projectId: this.projectId,
772772
method: method
773773
}),
774+
body: is.empty(body) ? '' : pbRequest,
775+
encoding: null,
774776
headers: {
775777
'Content-Type': 'application/x-protobuf'
776778
}
@@ -779,34 +781,32 @@ DatastoreRequest.prototype.makeReq_ = function(method, body, callback) {
779781
this.makeAuthenticatedRequest_(reqOpts, {
780782
onAuthenticated: function(err, authenticatedReqOpts) {
781783
if (err) {
782-
callback(err, null); // TODO(ryanseys): What goes as third parameter?
784+
callback(err, null);
783785
return;
784786
}
785787

786-
authenticatedReqOpts.headers = authenticatedReqOpts.headers || {};
787-
authenticatedReqOpts.headers['Content-Length'] = pbRequest.length;
788-
789-
var apiRequest = request(authenticatedReqOpts);
790-
791-
apiRequest.on('error', callback);
792-
793-
apiRequest.on('response', function(resp) {
794-
var buffer = new Buffer('');
795-
resp.on('data', function(chunk) {
796-
buffer = Buffer.concat([buffer, chunk]);
797-
});
798-
resp.on('end', function() {
799-
util.handleResp(null, resp, buffer.toString(), function(err, result) {
800-
if (err) {
801-
callback(err, null, result);
802-
return;
803-
}
804-
callback(null, pbResponse.decode(buffer), result);
805-
});
806-
});
807-
});
788+
request(authenticatedReqOpts, function(err, resp, body) {
789+
if (err) {
790+
callback(err, null);
791+
return;
792+
}
793+
794+
var parsedResp = util.parseHttpRespMessage(resp);
795+
796+
if (parsedResp.err) {
797+
callback(parsedResp.err, null, parsedResp.resp);
798+
return;
799+
}
808800

809-
apiRequest.end(pbRequest);
801+
var parsedBody = util.parseHttpRespBody(pbResponse.decode(body));
802+
803+
if (parsedBody.err) {
804+
callback(parsedBody.err, null, parsedResp.resp);
805+
return;
806+
}
807+
808+
callback(null, parsedBody.body, resp);
809+
});
810810
}
811811
});
812812
};

‎test/common/util.js‎

Lines changed: 57 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -181,84 +181,106 @@ describe('common/util', function() {
181181
var returnedBody = { a: 'b', c: 'd' };
182182
var returnedResp = { a: 'b', c: 'd' };
183183

184-
utilOverrides.parseApiResp = function(err_, resp_, body_) {
185-
assert.strictEqual(err_, err);
184+
utilOverrides.parseHttpRespMessage = function(resp_) {
186185
assert.strictEqual(resp_, resp);
187-
assert.strictEqual(body_, body);
188186

189187
return {
190-
err: returnedErr,
191-
body: returnedBody,
192188
resp: returnedResp
193189
};
194190
};
195191

192+
utilOverrides.parseHttpRespBody = function(body_) {
193+
assert.strictEqual(body_, body);
194+
195+
return {
196+
body: returnedBody
197+
};
198+
};
199+
196200
util.handleResp(err, resp, body, function(err, body, resp) {
197-
assert.strictEqual(err, returnedErr);
198-
assert.strictEqual(body, returnedBody);
199-
assert.strictEqual(resp, returnedResp);
201+
assert.deepEqual(err, returnedErr);
202+
assert.deepEqual(body, returnedBody);
203+
assert.deepEqual(resp, returnedResp);
200204
done();
201205
});
202206
});
203207

204208
it('should parse response for error', function(done) {
205209
var error = new Error('Error.');
206210

207-
utilOverrides.parseApiResp = function() {
211+
utilOverrides.parseHttpRespMessage = function() {
208212
return { err: error };
209213
};
210214

211215
util.handleResp(null, {}, {}, function(err) {
212-
assert.strictEqual(err, error);
216+
assert.deepEqual(err, error);
217+
done();
218+
});
219+
});
220+
221+
it('should parse body for error', function(done) {
222+
var error = new Error('Error.');
223+
224+
utilOverrides.parseHttpRespBody = function() {
225+
return { err: error };
226+
};
227+
228+
util.handleResp(null, {}, {}, function(err) {
229+
assert.deepEqual(err, error);
213230
done();
214231
});
215232
});
216233
});
217234

218-
describe('parseApiResp', function() {
219-
describe('non-200s response status', function() {
220-
it('should build ApiError with status and message', function(done) {
221-
var error = { statusCode: 400, statusMessage: 'Not Good' };
235+
describe('parseHttpRespMessage', function() {
236+
it('should build ApiError with non-200 status and message', function(done) {
237+
var httpRespMessage = { statusCode: 400, statusMessage: 'Not Good' };
222238

223-
utilOverrides.ApiError = function(error_) {
224-
assert.strictEqual(error_.code, error.statusCode);
225-
assert.strictEqual(error_.message, error.statusMessage);
226-
assert.strictEqual(error_.response, error);
239+
utilOverrides.ApiError = function(error_) {
240+
assert.strictEqual(error_.code, httpRespMessage.statusCode);
241+
assert.strictEqual(error_.message, httpRespMessage.statusMessage);
242+
assert.strictEqual(error_.response, httpRespMessage);
227243

228-
done();
229-
};
244+
done();
245+
};
230246

231-
util.parseApiResp(null, error);
232-
});
247+
util.parseHttpRespMessage(httpRespMessage);
233248
});
234249

235-
it('should not throw when there is just an error', function() {
236-
assert.doesNotThrow(function() {
237-
var error = {};
238-
util.parseApiResp(error);
239-
});
250+
it('should return the original response message', function() {
251+
var httpRespMessage = {};
252+
var parsedHttpRespMessage = util.parseHttpRespMessage(httpRespMessage);
253+
assert.strictEqual(parsedHttpRespMessage.resp, httpRespMessage);
240254
});
255+
});
241256

257+
describe('parseHttpRespBody', function() {
242258
it('should detect body errors', function() {
243259
var apiErr = {
244260
errors: [{ foo: 'bar' }],
245261
code: 400,
246262
message: 'an error occurred'
247263
};
248264

249-
var parsedApiResp = util.parseApiResp(null, {}, { error: apiErr });
265+
var parsedHttpRespBody = util.parseHttpRespBody({ error: apiErr });
250266

251-
assert.deepEqual(parsedApiResp.err.errors, apiErr.errors);
252-
assert.strictEqual(parsedApiResp.err.code, apiErr.code);
253-
assert.deepEqual(parsedApiResp.err.message, apiErr.message);
267+
assert.deepEqual(parsedHttpRespBody.err.errors, apiErr.errors);
268+
assert.strictEqual(parsedHttpRespBody.err.code, apiErr.code);
269+
assert.deepEqual(parsedHttpRespBody.err.message, apiErr.message);
254270
});
255271

256272
it('should try to parse JSON if body is string', function() {
257-
var body = '{ "foo": "bar" }';
273+
var httpRespBody = '{ "foo": "bar" }';
274+
var parsedHttpRespBody = util.parseHttpRespBody(httpRespBody);
275+
276+
assert.strictEqual(parsedHttpRespBody.body.foo, 'bar');
277+
});
258278

259-
var parsedApiResp = util.parseApiResp(null, {}, body);
279+
it('should return the original body', function() {
280+
var httpRespBody = {};
281+
var parsedHttpRespBody = util.parseHttpRespBody(httpRespBody);
260282

261-
assert.strictEqual(parsedApiResp.body.foo, 'bar');
283+
assert.strictEqual(parsedHttpRespBody.body, httpRespBody);
262284
});
263285
});
264286

@@ -692,7 +714,7 @@ describe('common/util', function() {
692714
assert.strictEqual(config.request, fakeRequest);
693715

694716
var error = new Error('Error.');
695-
utilOverrides.parseApiResp = function() {
717+
utilOverrides.parseHttpRespMessage = function() {
696718
return { err: error };
697719
};
698720
utilOverrides.shouldRetryRequest = function(err) {

0 commit comments

Comments
 (0)