datastore: properly parse HTTP responses from the API

Author: stephenplusplus

lib/common/util.js

@@ -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
@@ -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);
     }
   };

lib/datastore/request.js

@@ -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'
     }
@@ -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);
+      });
     }
   });
 };

test/common/util.js

@@ -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);
     });
   });
 
@@ -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) {