From 6c112bfd8dc3b6f0b5a8dc1c401c53e99e84d237 Mon Sep 17 00:00:00 2001 From: Stephen Sawchuk Date: Tue, 30 Aug 2016 15:35:27 -0400 Subject: [PATCH 1/6] allow setting gRPC metadata --- packages/common/src/grpc-service.js | 34 +++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/packages/common/src/grpc-service.js b/packages/common/src/grpc-service.js index 4ca68a64abc..9bd41c351ac 100644 --- a/packages/common/src/grpc-service.js +++ b/packages/common/src/grpc-service.js @@ -138,6 +138,7 @@ var GRPC_ERROR_CODE_TO_HTTP = { * * @param {object} config - Configuration object. * @param {string} config.baseUrl - The base URL to make API requests to. + * @param {object} config.grpcMetadata - Metadata to send with every request. * @param {string[]} config.scopes - The scopes required for the request. * @param {string} config.service - The name of the service. * @param {object=} config.protoServices - Directly provide the required proto @@ -185,6 +186,12 @@ function GrpcService(config, options) { service.baseUrl = protoConfig.baseUrl; } }); + + this.callCredentials = []; + + if (config.grpcMetadata) { + this.setGrpcMetadata_(config.grpcMetadata); + } } nodeutil.inherits(GrpcService, Service); @@ -646,18 +653,27 @@ GrpcService.structToObj_ = function(struct) { * @param {?error} callback.err - An error getting an auth client. */ GrpcService.prototype.getGrpcCredentials_ = function(callback) { + var self = this; + this.authClient.getAuthClient(function(err, authClient) { if (err) { callback(err); return; } - var credentials = grpc.credentials.combineChannelCredentials( + var callCredentialObjects = [ grpc.credentials.createSsl(), grpc.credentials.createFromGoogleCredential(authClient) + ]; + + callCredentialObjects = callCredentialObjects.concat(self.callCredentials); + + var grpcCredentials = grpc.credentials.combineChannelCredentials.apply( + null, + callCredentialObjects ); - callback(null, credentials); + callback(null, grpcCredentials); }); }; @@ -728,5 +744,19 @@ GrpcService.prototype.getService_ = function(protoOpts) { return service; }; +GrpcService.prototype.setGrpcMetadata_ = function(metadata) { + var cc = grpc.credentials.createFromMetadataGenerator(function(_, cb) { + var grpcMetadata = new grpc.Metadata(); + + for (var prop in metadata) { + grpcMetadata.add(prop, metadata[prop]); + } + + cb(null, grpcMetadata); + }); + + this.callCredentials.push(cc); +}; + module.exports = GrpcService; module.exports.GRPC_ERROR_CODE_TO_HTTP = GRPC_ERROR_CODE_TO_HTTP; From cbd23f3221af5c2d84532d944fbd49fd30875215 Mon Sep 17 00:00:00 2001 From: Stephen Sawchuk Date: Wed, 31 Aug 2016 08:30:37 -0400 Subject: [PATCH 2/6] set metadata per call --- packages/common/src/grpc-service.js | 51 +++++++++-------------------- 1 file changed, 16 insertions(+), 35 deletions(-) diff --git a/packages/common/src/grpc-service.js b/packages/common/src/grpc-service.js index 9bd41c351ac..1e43acdbb72 100644 --- a/packages/common/src/grpc-service.js +++ b/packages/common/src/grpc-service.js @@ -159,6 +159,7 @@ function GrpcService(config, options) { this.grpcCredentials = grpc.credentials.createInsecure(); } + this.grpcMetadata = config.grpcMetadata; this.maxRetries = options.maxRetries; var apiVersion = config.apiVersion; @@ -186,12 +187,6 @@ function GrpcService(config, options) { service.baseUrl = protoConfig.baseUrl; } }); - - this.callCredentials = []; - - if (config.grpcMetadata) { - this.setGrpcMetadata_(config.grpcMetadata); - } } nodeutil.inherits(GrpcService, Service); @@ -234,8 +229,17 @@ GrpcService.prototype.request = function(protoOpts, reqOpts, callback) { delete reqOpts.autoPaginateVal; var service = this.getService_(protoOpts); - var grpcOpts = {}; + var metadata = null; + if (this.grpcMetadata) { + metadata = new grpc.Metadata(); + + for (var prop in this.grpcMetadata) { + metadata.add(prop, this.grpcMetadata[prop]); + } + } + + var grpcOpts = {}; if (is.number(protoOpts.timeout)) { grpcOpts.deadline = GrpcService.createDeadline_(protoOpts.timeout); } @@ -255,16 +259,16 @@ GrpcService.prototype.request = function(protoOpts, reqOpts, callback) { request: function(_, onResponse) { respError = null; - service[protoOpts.method](reqOpts, grpcOpts, function(err, resp) { - if (err) { - respError = GrpcService.decorateError_(err); + service[protoOpts.method](reqOpts, metadata, grpcOpts, function(e, resp) { + if (e) { + respError = GrpcService.decorateError_(e); if (respError) { onResponse(null, respError); return; } - onResponse(err, resp); + onResponse(e, resp); return; } @@ -653,24 +657,15 @@ GrpcService.structToObj_ = function(struct) { * @param {?error} callback.err - An error getting an auth client. */ GrpcService.prototype.getGrpcCredentials_ = function(callback) { - var self = this; - this.authClient.getAuthClient(function(err, authClient) { if (err) { callback(err); return; } - var callCredentialObjects = [ + var grpcCredentials = grpc.credentials.combineChannelCredentials( grpc.credentials.createSsl(), grpc.credentials.createFromGoogleCredential(authClient) - ]; - - callCredentialObjects = callCredentialObjects.concat(self.callCredentials); - - var grpcCredentials = grpc.credentials.combineChannelCredentials.apply( - null, - callCredentialObjects ); callback(null, grpcCredentials); @@ -744,19 +739,5 @@ GrpcService.prototype.getService_ = function(protoOpts) { return service; }; -GrpcService.prototype.setGrpcMetadata_ = function(metadata) { - var cc = grpc.credentials.createFromMetadataGenerator(function(_, cb) { - var grpcMetadata = new grpc.Metadata(); - - for (var prop in metadata) { - grpcMetadata.add(prop, metadata[prop]); - } - - cb(null, grpcMetadata); - }); - - this.callCredentials.push(cc); -}; - module.exports = GrpcService; module.exports.GRPC_ERROR_CODE_TO_HTTP = GRPC_ERROR_CODE_TO_HTTP; From eff72669208c9c03ba36e0a29fd84174afeee6f6 Mon Sep 17 00:00:00 2001 From: Stephen Sawchuk Date: Thu, 1 Sep 2016 16:37:39 -0400 Subject: [PATCH 3/6] refactor and test --- packages/common/src/grpc-service.js | 28 +++++----- packages/common/test/grpc-service.js | 77 +++++++++++++++++++++++++--- 2 files changed, 85 insertions(+), 20 deletions(-) diff --git a/packages/common/src/grpc-service.js b/packages/common/src/grpc-service.js index 1e43acdbb72..6f6838dff7c 100644 --- a/packages/common/src/grpc-service.js +++ b/packages/common/src/grpc-service.js @@ -159,7 +159,18 @@ function GrpcService(config, options) { this.grpcCredentials = grpc.credentials.createInsecure(); } - this.grpcMetadata = config.grpcMetadata; + this.grpcMetadata = null; + + if (config.grpcMetadata) { + this.grpcMetadata = new grpc.Metadata(); + + for (var prop in config.grpcMetadata) { + if (config.grpcMetadata.hasOwnProperty(prop)) { + this.grpcMetadata.add(prop, config.grpcMetadata[prop]); + } + } + } + this.maxRetries = options.maxRetries; var apiVersion = config.apiVersion; @@ -230,14 +241,7 @@ GrpcService.prototype.request = function(protoOpts, reqOpts, callback) { var service = this.getService_(protoOpts); - var metadata = null; - if (this.grpcMetadata) { - metadata = new grpc.Metadata(); - - for (var prop in this.grpcMetadata) { - metadata.add(prop, this.grpcMetadata[prop]); - } - } + var metadata = this.grpcMetadata; var grpcOpts = {}; if (is.number(protoOpts.timeout)) { @@ -341,7 +345,7 @@ GrpcService.prototype.requestStream = function(protoOpts, reqOpts) { shouldRetryFn: GrpcService.shouldRetryRequest_, request: function() { - return service[protoOpts.method](reqOpts, grpcOpts) + return service[protoOpts.method](reqOpts, self.grpcMetadata, grpcOpts) .on('metadata', function() { // retry-request requires a server response before it starts emitting // data. The closest mechanism grpc provides is a metadata event, but @@ -663,12 +667,12 @@ GrpcService.prototype.getGrpcCredentials_ = function(callback) { return; } - var grpcCredentials = grpc.credentials.combineChannelCredentials( + var credentials = grpc.credentials.combineChannelCredentials( grpc.credentials.createSsl(), grpc.credentials.createFromGoogleCredential(authClient) ); - callback(null, grpcCredentials); + callback(null, credentials); }); }; diff --git a/packages/common/test/grpc-service.js b/packages/common/test/grpc-service.js index 2a91eb7dcae..889066d933c 100644 --- a/packages/common/test/grpc-service.js +++ b/packages/common/test/grpc-service.js @@ -43,8 +43,12 @@ function fakeRetryRequest() { return (retryRequestOverride || retryRequest).apply(null, arguments); } +var grpcMetadataOverride; var grpcLoadOverride; var fakeGrpc = { + Metadata: function() { + return new (grpcMetadataOverride || grpc.Metadata)(); + }, load: function() { return (grpcLoadOverride || grpc.load).apply(null, arguments); }, @@ -84,7 +88,10 @@ describe('GrpcService', function() { var CONFIG = { proto: {}, service: 'Service', - apiVersion: 'v1' + apiVersion: 'v1', + grpcMetadata: { + property: 'value' + } }; var OPTIONS = { @@ -111,6 +118,7 @@ describe('GrpcService', function() { }); beforeEach(function() { + grpcMetadataOverride = null; retryRequestOverride = null; googleProtoFilesOverride = function() { @@ -232,6 +240,30 @@ describe('GrpcService', function() { assert.strictEqual(calledWith[1], OPTIONS); }); + it('should default grpcMetadata to null', function() { + var config = extend({}, CONFIG); + delete config.grpcMetadata; + + var grpcService = new GrpcService(config, OPTIONS); + assert.strictEqual(grpcService.grpcMetadata, null); + }); + + it('should create and localize grpcMetadata', function() { + var fakeGrpcMetadata = { + add: function(prop, value) { + assert.strictEqual(prop, Object.keys(CONFIG.grpcMetadata)[0]); + assert.strictEqual(value, CONFIG.grpcMetadata[prop]); + } + }; + + grpcMetadataOverride = function() { + return fakeGrpcMetadata; + }; + + var grpcService = new GrpcService(CONFIG, OPTIONS); + assert.strictEqual(grpcService.grpcMetadata, fakeGrpcMetadata); + }); + it('should localize maxRetries', function() { assert.strictEqual(grpcService.maxRetries, OPTIONS.maxRetries); }); @@ -738,7 +770,7 @@ describe('GrpcService', function() { grpcService.protos.Service = { service: function() { return { - method: function(reqOpts, grpcOpts, callback) { + method: function(reqOpts, metadata, grpcOpts, callback) { callback(grpcError500); } }; @@ -763,7 +795,7 @@ describe('GrpcService', function() { grpcService.protos.Service = { service: function() { return { - method: function(reqOpts, grpcOpts, callback) { + method: function(reqOpts, metadata, grpcOpts, callback) { callback(grpcError500); } }; @@ -787,7 +819,7 @@ describe('GrpcService', function() { grpcService.protos.Service = { service: function() { return { - method: function(reqOpts, grpcOpts, callback) { + method: function(reqOpts, metadata, grpcOpts, callback) { callback(unknownError, null); } }; @@ -821,6 +853,21 @@ describe('GrpcService', function() { grpcService.request(PROTO_OPTS, REQ_OPTS, assert.ifError); }); + it('should pass the grpc metadata with the request', function(done) { + grpcService.protos.Service = { + service: function() { + return { + method: function(reqOpts, metadata) { + assert.strictEqual(metadata, grpcService.grpcMetadata); + done(); + } + }; + } + }; + + grpcService.request(PROTO_OPTS, REQ_OPTS, assert.ifError); + }); + it('should set a deadline if a timeout is provided', function(done) { var expectedDeadlineRange = [ Date.now() + PROTO_OPTS.timeout - 250, @@ -830,7 +877,7 @@ describe('GrpcService', function() { grpcService.protos.Service = { service: function() { return { - method: function(reqOpts, grpcOpts) { + method: function(reqOpts, metadata, grpcOpts) { assert(is.date(grpcOpts.deadline)); assert(grpcOpts.deadline.getTime() > expectedDeadlineRange[0]); @@ -874,7 +921,7 @@ describe('GrpcService', function() { grpcService.protos.Service = { service: function() { return { - method: function(reqOpts, grpcOpts, callback) { + method: function(reqOpts, metadata, grpcOpts, callback) { callback(grpcError); } }; @@ -896,7 +943,7 @@ describe('GrpcService', function() { grpcService.protos.Service = { service: function() { return { - method: function(reqOpts, grpcOpts, callback) { + method: function(reqOpts, metadata, grpcOpts, callback) { callback(null, RESPONSE); } }; @@ -976,7 +1023,7 @@ describe('GrpcService', function() { return fakeDeadline; }; - ProtoService.prototype.method = function(reqOpts, grpcOpts) { + ProtoService.prototype.method = function(reqOpts, metadata, grpcOpts) { assert.strictEqual(grpcOpts.deadline, fakeDeadline); GrpcService.createDeadline_ = createDeadline; @@ -992,6 +1039,20 @@ describe('GrpcService', function() { grpcService.requestStream(PROTO_OPTS, REQ_OPTS); }); + it('should pass the grpc metadata with the request', function(done) { + ProtoService.prototype.method = function(reqOpts, metadata) { + assert.strictEqual(metadata, grpcService.grpcMetadata); + setImmediate(done); + return through.obj(); + }; + + retryRequestOverride = function(_, retryOpts) { + return retryOpts.request(); + }; + + grpcService.requestStream(PROTO_OPTS, REQ_OPTS); + }); + describe('getting gRPC credentials', function() { beforeEach(function() { delete grpcService.grpcCredentials; From 6cc716b8299c50c0df2e97e7f0a8a49a21803264 Mon Sep 17 00:00:00 2001 From: Stephen Sawchuk Date: Thu, 1 Sep 2016 21:11:54 -0400 Subject: [PATCH 4/6] always default to empty metadata --- packages/common/src/grpc-service.js | 4 +--- packages/common/test/grpc-service.js | 8 +++++++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/common/src/grpc-service.js b/packages/common/src/grpc-service.js index 6f6838dff7c..7919b640ae1 100644 --- a/packages/common/src/grpc-service.js +++ b/packages/common/src/grpc-service.js @@ -159,11 +159,9 @@ function GrpcService(config, options) { this.grpcCredentials = grpc.credentials.createInsecure(); } - this.grpcMetadata = null; + this.grpcMetadata = new grpc.Metadata(); if (config.grpcMetadata) { - this.grpcMetadata = new grpc.Metadata(); - for (var prop in config.grpcMetadata) { if (config.grpcMetadata.hasOwnProperty(prop)) { this.grpcMetadata.add(prop, config.grpcMetadata[prop]); diff --git a/packages/common/test/grpc-service.js b/packages/common/test/grpc-service.js index 889066d933c..177de6e36da 100644 --- a/packages/common/test/grpc-service.js +++ b/packages/common/test/grpc-service.js @@ -241,11 +241,17 @@ describe('GrpcService', function() { }); it('should default grpcMetadata to null', function() { + var fakeGrpcMetadata = {}; + + grpcMetadataOverride = function() { + return fakeGrpcMetadata; + }; + var config = extend({}, CONFIG); delete config.grpcMetadata; var grpcService = new GrpcService(config, OPTIONS); - assert.strictEqual(grpcService.grpcMetadata, null); + assert.strictEqual(grpcService.grpcMetadata, fakeGrpcMetadata); }); it('should create and localize grpcMetadata', function() { From 9cf0c10d4b424d8e5978f674c4b46adcedd4fb9b Mon Sep 17 00:00:00 2001 From: Stephen Sawchuk Date: Thu, 1 Sep 2016 22:09:43 -0400 Subject: [PATCH 5/6] lint issue --- packages/common/test/grpc-service.js | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/packages/common/test/grpc-service.js b/packages/common/test/grpc-service.js index 177de6e36da..ae4e3222056 100644 --- a/packages/common/test/grpc-service.js +++ b/packages/common/test/grpc-service.js @@ -43,11 +43,14 @@ function fakeRetryRequest() { return (retryRequestOverride || retryRequest).apply(null, arguments); } -var grpcMetadataOverride; +var GrpcMetadataOverride; var grpcLoadOverride; var fakeGrpc = { Metadata: function() { - return new (grpcMetadataOverride || grpc.Metadata)(); + if (GrpcMetadataOverride) { + return new GrpcMetadataOverride(); + } + return new grpc.Metadata(); }, load: function() { return (grpcLoadOverride || grpc.load).apply(null, arguments); @@ -118,7 +121,7 @@ describe('GrpcService', function() { }); beforeEach(function() { - grpcMetadataOverride = null; + GrpcMetadataOverride = null; retryRequestOverride = null; googleProtoFilesOverride = function() { @@ -243,7 +246,7 @@ describe('GrpcService', function() { it('should default grpcMetadata to null', function() { var fakeGrpcMetadata = {}; - grpcMetadataOverride = function() { + GrpcMetadataOverride = function() { return fakeGrpcMetadata; }; @@ -262,7 +265,7 @@ describe('GrpcService', function() { } }; - grpcMetadataOverride = function() { + GrpcMetadataOverride = function() { return fakeGrpcMetadata; }; From c68b2604a10094075893f8b67b1715cfa5db2a8d Mon Sep 17 00:00:00 2001 From: Stephen Sawchuk Date: Fri, 2 Sep 2016 10:24:33 -0400 Subject: [PATCH 6/6] Update grpc-service.js --- packages/common/test/grpc-service.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/common/test/grpc-service.js b/packages/common/test/grpc-service.js index ae4e3222056..05392a49ea7 100644 --- a/packages/common/test/grpc-service.js +++ b/packages/common/test/grpc-service.js @@ -243,7 +243,7 @@ describe('GrpcService', function() { assert.strictEqual(calledWith[1], OPTIONS); }); - it('should default grpcMetadata to null', function() { + it('should default grpcMetadata to empty metadata', function() { var fakeGrpcMetadata = {}; GrpcMetadataOverride = function() {