storage: Streaming operations should return response body as an object

lib/common/util.js

@@ -49,14 +49,38 @@ module.exports.format = function(templ, args) {
   return templ;
 };
 
-module.exports.noop = function(){};
+var noop = function() {};
 
-module.exports.ApiError = function (errorBody) {
+module.exports.noop = noop;
+
+function ApiError (errorBody) {
   Error.call(this);
   Error.captureStackTrace(this, arguments.callee);
   this.errors = errorBody.errors;
   this.code = errorBody.code;
   this.message = errorBody.message;
-}
+};
 
-util.inherits(module.exports.ApiError, Error);
+util.inherits(ApiError, Error);
+
+module.exports.handleResp = function(err, resp, body, opt_callback) {
+  var callback = opt_callback || noop;
+  if (err) {
+    callback(err);
+    return;
+  }
+  if (typeof body === 'string') {
+    try {
+      body = JSON.parse(body);
+    } catch(err) {}
+  }
+  if (body && body.error) {
+    callback(new ApiError(body.error));
+    return;
+  }
+  if (resp && (resp.statusCode < 200 || resp.statusCode > 299)) {
+    callback(new Error('error during request, statusCode: ' + resp.statusCode));
+    return;
+  }
+  callback(null, body, resp);
+};

lib/datastore/index.js

@@ -301,11 +301,7 @@ Transaction.prototype.makeReq = function(method, req, callback) {
     uri: DATASTORE_BASE_URL + '/' + this.datasetId + '/' + method,
     json: req
   }, function(err, res, body) {
-    if (body && body.error) {
-      var error = new util.ApiError(body.error);
-      return callback(error, null);
-    }
-    callback(err, body);
+    util.handleResp(err, res, body, callback);
   });
 };
 

lib/pubsub/index.js

@@ -398,7 +398,6 @@ Connection.prototype.fullProjectName_ = function() {
   });
 };
 
-// TOOD(jbd): Don't duplicate, unify this with bucket.makeReq.
 Connection.prototype.makeReq = function(method, path, q, body, callback) {
   var reqOpts = {
     method: method,
@@ -412,13 +411,7 @@ Connection.prototype.makeReq = function(method, path, q, body, callback) {
     reqOpts.json = body;
   }
   this.conn.req(reqOpts, function(err, res, body) {
-    if (body && body.error) {
-      callback(new util.ApiError(body.error)); return;
-    }
-    if (res && (res.statusCode < 200 || res.statusCode > 299)) {
-      callback(new Error('error during request, statusCode: ' + res.statusCode)); return;
-    }
-    callback(null, body);
+    util.handleResp(err, res, body, callback);
   });
 };
 

lib/storage/index.js

@@ -33,16 +33,12 @@ var STORAGE_BASE_URL = 'https://www.googleapis.com/storage/v1/b';
     STORAGE_UPLOAD_BASE_URL = 'https://www.googleapis.com/upload/storage/v1/b';
 
 var reqStreamToCallback = function(st, callback) {
+  st.callback = util.noop;
   st.on('error', function(err) {
     callback(err);
   });
   st.on('complete', function(resp) {
-    // TODO(jbd): Buffer the response to pass the resp body
-    // to the callback.
-    if (resp.statusCode < 200 || resp.statusCode > 299) {
-      return callback(new Error('error during request, statusCode: ' + resp.statusCode), resp);
-    }
-    callback(null, resp);
+    util.handleResp(null, resp, resp.body, callback);
   });
 };
 
@@ -330,13 +326,7 @@ Bucket.prototype.makeReq = function(method, path, q, body, callback) {
     reqOpts.json = body;
   }
   this.conn.req(reqOpts, function(err, res, body) {
-    if (body && body.error) {
-      return callback(body.error);
-    }
-    if (res && (res.statusCode < 200 || res.statusCode > 299)) {
-      return callback(new Error('error during request, statusCode: ' + res.statusCode));
-    }
-    callback(err, body);
+    util.handleResp(err, res, body, callback);
   });
 };
 

test/common.util.js

@@ -32,4 +32,45 @@ describe('arrayize', function() {
     assert.deepEqual(o, ['text']);
     done();
   });
-});
+});
+
+describe('handleResp', function() {
+
+  it('should handle errors', function(done) {
+    var defaultErr = new Error('new error');
+    util.handleResp(defaultErr, null, null, function(err) {
+      assert.equal(err, defaultErr);
+      done();
+    });
+  });
+
+  it('should handle body errors', function(done) {
+    var apiErr = {
+      errors: [{ foo: 'bar' }],
+      code:  400,
+      message: 'an error occurred'
+    }
+    util.handleResp(null, {}, { error: apiErr }, function(err) {
+      assert.deepEqual(err.errors, apiErr.errors);
+      assert.strictEqual(err.code, apiErr.code);
+      assert.deepEqual(err.message, apiErr.message);
+      done();
+    });
+  });
+
+  it('should try to parse JSON if body is string', function(done) {
+    var body = '{ "foo": "bar" }';
+    util.handleResp(null, {}, body, function(err, body) {
+      assert.strictEqual(body.foo, 'bar');
+      done();
+    });
+  });
+
+  it('should return status code as an error if there are not other errors', function(done) {
+    util.handleResp(null, { statusCode: 400 }, null, function(err) {
+      assert.strictEqual(err.message, 'error during request, statusCode: 400');
+      done();
+    });
+  });
+
+});