From 894753a4b5ade5419c0654586333bece1be44b86 Mon Sep 17 00:00:00 2001 From: Dave Gramlich Date: Fri, 15 Jul 2016 14:03:07 -0400 Subject: [PATCH 1/5] grpc: changed error handling to not modify original error --- lib/common/grpc-service.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/common/grpc-service.js b/lib/common/grpc-service.js index f26b845c75c..9235506c4ac 100644 --- a/lib/common/grpc-service.js +++ b/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} @@ -457,11 +458,12 @@ GrpcService.createDeadline_ = function(timeout) { 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; - } + return extend(true, new Error(), err, { + code: defaultErrorDetails.code, + message: err.message || defaultErrorDetails.message + }); + } return null; }; From 46f601655063569c0adf2a342cfd9353c9b58454 Mon Sep 17 00:00:00 2001 From: Dave Gramlich Date: Fri, 15 Jul 2016 14:16:33 -0400 Subject: [PATCH 2/5] conditionally use a new error object --- lib/common/grpc-service.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/common/grpc-service.js b/lib/common/grpc-service.js index 9235506c4ac..73e54aa3e1d 100644 --- a/lib/common/grpc-service.js +++ b/lib/common/grpc-service.js @@ -458,8 +458,9 @@ GrpcService.createDeadline_ = function(timeout) { GrpcService.getError_ = function(err) { if (err && GRPC_ERROR_CODE_TO_HTTP[err.code]) { var defaultErrorDetails = GRPC_ERROR_CODE_TO_HTTP[err.code]; + var errorObj = is.error(err) ? new Error() : {}; - return extend(true, new Error(), err, { + return extend(true, errorObj, err, { code: defaultErrorDetails.code, message: err.message || defaultErrorDetails.message }); From eeb06e76ed6e476cd40c6dbda97477bfcd5b9187 Mon Sep 17 00:00:00 2001 From: Dave Gramlich Date: Fri, 15 Jul 2016 14:36:43 -0400 Subject: [PATCH 3/5] renamed getError_ to getStatus_ --- lib/common/grpc-service.js | 28 ++++++++++++++-------------- test/common/grpc-service.js | 12 ++++++------ 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/lib/common/grpc-service.js b/lib/common/grpc-service.js index 73e54aa3e1d..9c20a88060e 100644 --- a/lib/common/grpc-service.js +++ b/lib/common/grpc-service.js @@ -244,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.getStatus_(err); if (respError) { onResponse(null, respError); @@ -326,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.getStatus_(status); this.emit('response', grcpStatus || status); }); @@ -335,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.getStatus_(err); stream.destroy(grpcError || err); }) @@ -447,22 +447,22 @@ 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 status object with additional * information. * * @private * - * @param {error} err - The original request error. - * @return {error|null} + * @param {error|object} status - The original status object. + * @return {error|object|null} */ -GrpcService.getError_ = function(err) { - if (err && GRPC_ERROR_CODE_TO_HTTP[err.code]) { - var defaultErrorDetails = GRPC_ERROR_CODE_TO_HTTP[err.code]; - var errorObj = is.error(err) ? new Error() : {}; - - return extend(true, errorObj, err, { - code: defaultErrorDetails.code, - message: err.message || defaultErrorDetails.message +GrpcService.getStatus_ = function(status) { + if (status && GRPC_ERROR_CODE_TO_HTTP[status.code]) { + var defaultStatusDetails = GRPC_ERROR_CODE_TO_HTTP[status.code]; + var statusObj = is.error(status) ? new Error() : {}; + + return extend(true, statusObj, status, { + code: defaultStatusDetails.code, + message: status.message || defaultStatusDetails.message }); } return null; diff --git a/test/common/grpc-service.js b/test/common/grpc-service.js index e2230db9559..1a009c8e7b1 100644 --- a/test/common/grpc-service.js +++ b/test/common/grpc-service.js @@ -1120,13 +1120,13 @@ describe('GrpcService', function() { }); }); - describe('getError_', function() { - it('should retrieve the HTTP error from the gRPC error map', function() { + describe('getStatus_', 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 = GrpcService.getStatus_({ code: code }); assert.notStrictEqual(error, errorMap[code]); assert.deepEqual(error, errorMap[code]); @@ -1141,12 +1141,12 @@ describe('GrpcService', function() { message: errorMessage }; - var error = GrpcService.getError_(err); + var error = GrpcService.getStatus_(err); assert.strictEqual(error.message, errorMessage); }); - it('should return the original object for unknown errors', function() { - var error = GrpcService.getError_({ code: 9999 }); + it('should return null for unknown errors', function() { + var error = GrpcService.getStatus_({ code: 9999 }); assert.strictEqual(error, null); }); From af0055fe0f03cb6aed8487a15778e6825c47ae12 Mon Sep 17 00:00:00 2001 From: Dave Gramlich Date: Fri, 15 Jul 2016 16:06:41 -0400 Subject: [PATCH 4/5] refactored message lookup into separate method --- lib/common/grpc-service.js | 43 ++++++++++++++++++--------- test/common/grpc-service.js | 58 ++++++++++++++++++++++++++++++++----- 2 files changed, 79 insertions(+), 22 deletions(-) diff --git a/lib/common/grpc-service.js b/lib/common/grpc-service.js index 9c20a88060e..e0d295c5205 100644 --- a/lib/common/grpc-service.js +++ b/lib/common/grpc-service.js @@ -244,7 +244,7 @@ GrpcService.prototype.request = function(protoOpts, reqOpts, callback) { service[protoOpts.method](reqOpts, grpcOpts, function(err, resp) { if (err) { - respError = GrpcService.getStatus_(err); + respError = GrpcService.getError_(err); if (respError) { onResponse(null, respError); @@ -326,7 +326,7 @@ GrpcService.prototype.requestStream = function(protoOpts, reqOpts) { request: function() { return service[protoOpts.method](reqOpts, grpcOpts) .on('status', function(status) { - var grcpStatus = GrpcService.getStatus_(status); + var grcpStatus = GrpcService.extendGrpcResponse_({}, status); this.emit('response', grcpStatus || status); }); @@ -335,7 +335,7 @@ GrpcService.prototype.requestStream = function(protoOpts, reqOpts) { return retryRequest(null, retryOpts) .on('error', function(err) { - var grpcError = GrpcService.getStatus_(err); + var grpcError = GrpcService.getError_(err); stream.destroy(grpcError || err); }) @@ -447,27 +447,42 @@ GrpcService.createDeadline_ = function(timeout) { }; /** - * Checks for a grpc status code and extends the status object with additional + * Checks for a grpc status code and extends the supplied object with additional * information. * * @private * - * @param {error|object} status - The original status object. - * @return {error|object|null} + * @param {object} obj - The object to be extended. + * @param {object} response - The grpc response. + * @return {object|null} */ -GrpcService.getStatus_ = function(status) { - if (status && GRPC_ERROR_CODE_TO_HTTP[status.code]) { - var defaultStatusDetails = GRPC_ERROR_CODE_TO_HTTP[status.code]; - var statusObj = is.error(status) ? new Error() : {}; - - return extend(true, statusObj, status, { - code: defaultStatusDetails.code, - message: status.message || defaultStatusDetails.message +GrpcService.extendGrpcResponse_ = 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.getError_ = function(err) { + var errorObj = is.error(err) ? new Error() : {}; + return GrpcService.extendGrpcResponse_(errorObj, err); +}; + /** * Function to decide whether or not a request retry could occur. * diff --git a/test/common/grpc-service.js b/test/common/grpc-service.js index 1a009c8e7b1..7a6aa7369a9 100644 --- a/test/common/grpc-service.js +++ b/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('getStatus_', function() { + describe('extendGrpcResponse_', 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.getStatus_({ code: code }); + var error = new Error(); + var extended = GrpcService.extendGrpcResponse_(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,52 @@ describe('GrpcService', function() { message: errorMessage }; - var error = GrpcService.getStatus_(err); - assert.strictEqual(error.message, errorMessage); + var error = new Error(); + var extended = GrpcService.extendGrpcResponse_(error, err); + + assert.strictEqual(extended.message, errorMessage); }); it('should return null for unknown errors', function() { - var error = GrpcService.getStatus_({ code: 9999 }); + var error = new Error(); + var extended = GrpcService.extendGrpcResponse_(error, { code: 9999 }); + + assert.strictEqual(extended, null); + }); + }); + + describe('getError_', function() { + var fakeError = new Error('err.'); + + beforeEach(function() { + sinon.stub(GrpcService, 'extendGrpcResponse_', function() { + return fakeError; + }); + }); + + it('should call extendGrpcResponse with an error object', function() { + var grpcError = new Error('err.'); + + grpcError.code = 2; + + var error = GrpcService.getError_(grpcError); + var args = GrpcService.extendGrpcResponse_.getCall(0).args; + + assert.strictEqual(fakeError, error); + assert(args[0] instanceof Error); + assert.strictEqual(args[1], grpcError); + }); + + it('should call extendGrpcResponse with a plain object', function() { + var grpcMessage = { code: 2 }; + + var error = GrpcService.getError_(grpcMessage); + var args = GrpcService.extendGrpcResponse_.getCall(0).args; - assert.strictEqual(error, null); + assert.strictEqual(fakeError, error); + assert.deepEqual(args[0], {}); + assert(!(args[0] instanceof Error)); + assert.strictEqual(args[1], grpcMessage); }); }); From ac2ea34ca468ccbb9a5f5c173c3749f3576252fe Mon Sep 17 00:00:00 2001 From: Dave Gramlich Date: Fri, 15 Jul 2016 17:01:40 -0400 Subject: [PATCH 5/5] refactoring yet again! --- lib/common/grpc-service.js | 24 +++++++++++++++----- test/common/grpc-service.js | 45 +++++++++++++++++++++++++++---------- 2 files changed, 51 insertions(+), 18 deletions(-) diff --git a/lib/common/grpc-service.js b/lib/common/grpc-service.js index e0d295c5205..4f50f6eb5c8 100644 --- a/lib/common/grpc-service.js +++ b/lib/common/grpc-service.js @@ -244,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); @@ -326,7 +326,7 @@ GrpcService.prototype.requestStream = function(protoOpts, reqOpts) { request: function() { return service[protoOpts.method](reqOpts, grpcOpts) .on('status', function(status) { - var grcpStatus = GrpcService.extendGrpcResponse_({}, status); + var grcpStatus = GrpcService.decorateStatus_(status); this.emit('response', grcpStatus || status); }); @@ -335,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); }) @@ -456,7 +456,7 @@ GrpcService.createDeadline_ = function(timeout) { * @param {object} response - The grpc response. * @return {object|null} */ -GrpcService.extendGrpcResponse_ = function(obj, response) { +GrpcService.decorateGrpcResponse_ = function(obj, response) { if (response && GRPC_ERROR_CODE_TO_HTTP[response.code]) { var defaultResponseDetails = GRPC_ERROR_CODE_TO_HTTP[response.code]; @@ -478,9 +478,21 @@ GrpcService.extendGrpcResponse_ = function(obj, response) { * @param {error|object} err - The grpc error. * @return {error|null} */ -GrpcService.getError_ = function(err) { +GrpcService.decorateError_ = function(err) { var errorObj = is.error(err) ? new Error() : {}; - return GrpcService.extendGrpcResponse_(errorObj, err); + 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); }; /** diff --git a/test/common/grpc-service.js b/test/common/grpc-service.js index 7a6aa7369a9..d04b039bc4c 100644 --- a/test/common/grpc-service.js +++ b/test/common/grpc-service.js @@ -1122,14 +1122,14 @@ describe('GrpcService', function() { }); }); - describe('extendGrpcResponse_', 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 = new Error(); - var extended = GrpcService.extendGrpcResponse_(error, { code: code }); + var extended = GrpcService.decorateGrpcResponse_(error, { code: code }); assert.notStrictEqual(extended, errorMap[code]); assert.deepEqual(extended, errorMap[code]); @@ -1146,46 +1146,46 @@ describe('GrpcService', function() { }; var error = new Error(); - var extended = GrpcService.extendGrpcResponse_(error, err); + 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.extendGrpcResponse_(error, { code: 9999 }); + var extended = GrpcService.decorateGrpcResponse_(error, { code: 9999 }); assert.strictEqual(extended, null); }); }); - describe('getError_', function() { + describe('decorateError_', function() { var fakeError = new Error('err.'); beforeEach(function() { - sinon.stub(GrpcService, 'extendGrpcResponse_', function() { + sinon.stub(GrpcService, 'decorateGrpcResponse_', function() { return fakeError; }); }); - it('should call extendGrpcResponse with an error object', function() { + it('should call decorateGrpcResponse_ with an error object', function() { var grpcError = new Error('err.'); grpcError.code = 2; - var error = GrpcService.getError_(grpcError); - var args = GrpcService.extendGrpcResponse_.getCall(0).args; + 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 call extendGrpcResponse with a plain object', function() { + it('should call decorateGrpcResponse_ with a plain object', function() { var grpcMessage = { code: 2 }; - var error = GrpcService.getError_(grpcMessage); - var args = GrpcService.extendGrpcResponse_.getCall(0).args; + var error = GrpcService.decorateError_(grpcMessage); + var args = GrpcService.decorateGrpcResponse_.getCall(0).args; assert.strictEqual(fakeError, error); assert.deepEqual(args[0], {}); @@ -1194,6 +1194,27 @@ describe('GrpcService', function() { }); }); + 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(status, fakeStatus); + assert.deepEqual(args[0], {}); + assert.strictEqual(args[1], grpcStatus); + }); + }); + describe('shouldRetryRequest_', function() { it('should retry on 429, 500, 502, and 503', function() { var shouldRetryFn = GrpcService.shouldRetryRequest_;