grpc: changed error handling to not modify original error

lib/common/grpc-service.js

@@ -28,6 +28,7 @@ var path = require('path');
 var retryRequest = require('retry-request');
 var through = require('through2');
 var dotProp = require('dot-prop');
+var extend = require('extend');
 
 /**
  * @type {module:common/service}
@@ -243,7 +244,7 @@ GrpcService.prototype.request = function(protoOpts, reqOpts, callback) {
 
       service[protoOpts.method](reqOpts, grpcOpts, function(err, resp) {
         if (err) {
-          respError = GrpcService.getError_(err);
+          respError = GrpcService.decorateError_(err);
 
           if (respError) {
             onResponse(null, respError);
@@ -325,7 +326,7 @@ GrpcService.prototype.requestStream = function(protoOpts, reqOpts) {
     request: function() {
       return service[protoOpts.method](reqOpts, grpcOpts)
         .on('status', function(status) {
-          var grcpStatus = GrpcService.getError_(status);
+          var grcpStatus = GrpcService.decorateStatus_(status);
 
           this.emit('response', grcpStatus || status);
         });
@@ -334,7 +335,7 @@ GrpcService.prototype.requestStream = function(protoOpts, reqOpts) {
 
   return retryRequest(null, retryOpts)
     .on('error', function(err) {
-      var grpcError = GrpcService.getError_(err);
+      var grpcError = GrpcService.decorateError_(err);
 
       stream.destroy(grpcError || err);
     })
@@ -446,25 +447,54 @@ GrpcService.createDeadline_ = function(timeout) {
 };
 
 /**
- * Checks for a grpc error code and extends the Error object with additional
+ * Checks for a grpc status code and extends the supplied object with additional
  * information.
  *
  * @private
  *
- * @param {error} err - The original request error.
- * @return {error|null}
+ * @param {object} obj - The object to be extended.
+ * @param {object} response - The grpc response.
+ * @return {object|null}
  */
-GrpcService.getError_ = function(err) {
-  if (err && GRPC_ERROR_CODE_TO_HTTP[err.code]) {
-    var defaultErrorDetails = GRPC_ERROR_CODE_TO_HTTP[err.code];
-    err.code = defaultErrorDetails.code;
-    err.message = err.message || defaultErrorDetails.message;
-    return err;
+GrpcService.decorateGrpcResponse_ = function(obj, response) {
+  if (response && GRPC_ERROR_CODE_TO_HTTP[response.code]) {
+    var defaultResponseDetails = GRPC_ERROR_CODE_TO_HTTP[response.code];
+
+    return extend(true, obj, response, {
+      code: defaultResponseDetails.code,
+      message: response.message || defaultResponseDetails.message
+    });
   }
 
   return null;
 };
 
+/**
+ * Checks for a grpc status code and extends the error object with additional
+ * information.
+ *
+ * @private
+ *
+ * @param {error|object} err - The grpc error.
+ * @return {error|null}
+ */
+GrpcService.decorateError_ = function(err) {
+  var errorObj = is.error(err) ? new Error() : {};
+  return GrpcService.decorateGrpcResponse_(errorObj, err);
+};
+
+/**
+ * Checks for grpc status code and extends the status object with additional
+ * information
+ *
+ * @private
+ * @param {object} status - The grpc status.
+ * @return {object|null}
+ */
+GrpcService.decorateStatus_ = function(status) {
+  return GrpcService.decorateGrpcResponse_({}, status);
+};
+
 /**
  * Function to decide whether or not a request retry could occur.
  *

test/common/grpc-service.js

@@ -25,6 +25,7 @@ var mockery = require('mockery-next');
 var path = require('path');
 var retryRequest = require('retry-request');
 var through = require('through2');
+var sinon = require('sinon').sandbox.create();
 
 var util = require('../../lib/common/util.js');
 
@@ -138,6 +139,7 @@ describe('GrpcService', function() {
   afterEach(function() {
     googleProtoFilesOverride = null;
     grpcLoadOverride = null;
+    sinon.restore();
   });
 
   describe('grpc error to http error map', function() {
@@ -1120,16 +1122,18 @@ describe('GrpcService', function() {
     });
   });
 
-  describe('getError_', function() {
-    it('should retrieve the HTTP error from the gRPC error map', function() {
+  describe('decorateGrpcResponse_', function() {
+    it('should retrieve the HTTP code from the gRPC error map', function() {
       var errorMap = GrpcService.GRPC_ERROR_CODE_TO_HTTP;
       var codes = Object.keys(errorMap);
 
       codes.forEach(function(code) {
-        var error = GrpcService.getError_({ code: code });
+        var error = new Error();
+        var extended = GrpcService.decorateGrpcResponse_(error, { code: code });
 
-        assert.notStrictEqual(error, errorMap[code]);
-        assert.deepEqual(error, errorMap[code]);
+        assert.notStrictEqual(extended, errorMap[code]);
+        assert.deepEqual(extended, errorMap[code]);
+        assert.strictEqual(error, extended);
       });
     });
 
@@ -1141,14 +1145,73 @@ describe('GrpcService', function() {
         message: errorMessage
       };
 
-      var error = GrpcService.getError_(err);
-      assert.strictEqual(error.message, errorMessage);
+      var error = new Error();
+      var extended = GrpcService.decorateGrpcResponse_(error, err);
+
+      assert.strictEqual(extended.message, errorMessage);
+    });
+
+    it('should return null for unknown errors', function() {
+      var error = new Error();
+      var extended = GrpcService.decorateGrpcResponse_(error, { code: 9999 });
+
+      assert.strictEqual(extended, null);
+    });
+  });
+
+  describe('decorateError_', function() {
+    var fakeError = new Error('err.');
+
+    beforeEach(function() {
+      sinon.stub(GrpcService, 'decorateGrpcResponse_', function() {
+        return fakeError;
+      });
+    });
+
+    it('should call decorateGrpcResponse_ with an error object', function() {
+      var grpcError = new Error('err.');
+
+      grpcError.code = 2;
+
+      var error = GrpcService.decorateError_(grpcError);
+      var args = GrpcService.decorateGrpcResponse_.getCall(0).args;
+
+      assert.strictEqual(fakeError, error);
+      assert(args[0] instanceof Error);
+      assert.strictEqual(args[1], grpcError);
     });
 
-    it('should return the original object for unknown errors', function() {
-      var error = GrpcService.getError_({ code: 9999 });
+    it('should call decorateGrpcResponse_ with a plain object', function() {
+      var grpcMessage = { code: 2 };
+
+      var error = GrpcService.decorateError_(grpcMessage);
+      var args = GrpcService.decorateGrpcResponse_.getCall(0).args;
+
+      assert.strictEqual(fakeError, error);
+      assert.deepEqual(args[0], {});
+      assert(!(args[0] instanceof Error));
+      assert.strictEqual(args[1], grpcMessage);
+    });
+  });
+
+  describe('decorateStatus_', function() {
+    var fakeStatus = { status: 'a' };
+
+    beforeEach(function() {
+      sinon.stub(GrpcService, 'decorateGrpcResponse_', function() {
+        return fakeStatus;
+      });
+    });
+
+    it('should call decorateGrpcResponse_ with an object', function() {
+      var grpcStatus = { code: 2 };
+
+      var status = GrpcService.decorateStatus_(grpcStatus);
+      var args = GrpcService.decorateGrpcResponse_.getCall(0).args;
 
-      assert.strictEqual(error, null);
+      assert.strictEqual(status, fakeStatus);
+      assert.deepEqual(args[0], {});
+      assert.strictEqual(args[1], grpcStatus);
     });
   });