From 6d4e8deca1effe5c15bfa03862b53dca2010742b Mon Sep 17 00:00:00 2001 From: Alexander Fenster Date: Tue, 14 Apr 2020 12:20:36 -0700 Subject: [PATCH] fix: process messages from all files (#418) * fix: process messages from all files * fix: use local messages for resources --- package.json | 2 + typescript/src/schema/api.ts | 22 +++-- typescript/src/schema/proto.ts | 174 ++++++++++++++++++--------------- typescript/test/unit/api.ts | 45 ++++++++- typescript/test/unit/proto.ts | 22 +++-- 5 files changed, 171 insertions(+), 94 deletions(-) diff --git a/package.json b/package.json index 727dd84de..66b74887c 100644 --- a/package.json +++ b/package.json @@ -59,6 +59,7 @@ "@types/nunjucks": "^3.1.3", "@types/object-hash": "^1.3.1", "@types/rimraf": "^3.0.0", + "@types/sinon": "^9.0.0", "@types/yargs": "^15.0.4", "assert-rejects": "^1.0.0", "c8": "^7.1.0", @@ -68,6 +69,7 @@ "mocha": "^7.1.1", "ncp": "^2.0.0", "rimraf": "^3.0.2", + "sinon": "^9.0.2", "typescript": "~3.8.3" }, "engines": { diff --git a/typescript/src/schema/api.ts b/typescript/src/schema/api.ts index bd674406e..35b2d2090 100644 --- a/typescript/src/schema/api.ts +++ b/typescript/src/schema/api.ts @@ -15,7 +15,7 @@ import * as plugin from '../../../pbjs-genfiles/plugin'; import {Naming, Options as namingOptions} from './naming'; -import {Proto} from './proto'; +import {Proto, MessagesMap} from './proto'; import {ResourceDatabase, ResourceDescriptor} from './resource-database'; export interface ProtosMap { @@ -59,22 +59,32 @@ export class API { // users specify the actual package name, if not, set it to product name. this.publishName = options.publishName || this.naming.productName.toKebabCase(); - // construct resource map + const [allResourceDatabase, resourceDatabase] = getResourceDatabase( fileDescriptors ); - // parse resource map to Proto constructor + + const allMessages: MessagesMap = {}; + for (const fd of fileDescriptors) { + fd.messageType + ?.filter(message => message.name) + .forEach(message => { + allMessages['.' + fd.package! + '.' + message.name!] = message; + }); + } + this.protos = fileDescriptors .filter(fd => fd.name) .filter(fd => !API.isIgnoredService(fd)) .reduce((map, fd) => { - map[fd.name!] = new Proto( + map[fd.name!] = new Proto({ fd, packageName, + allMessages, allResourceDatabase, resourceDatabase, - options - ); + options, + }); return map; }, {} as ProtosMap); diff --git a/typescript/src/schema/proto.ts b/typescript/src/schema/proto.ts index d76b22ce8..5532c5a2b 100644 --- a/typescript/src/schema/proto.ts +++ b/typescript/src/schema/proto.ts @@ -85,10 +85,6 @@ export interface MessagesMap { [name: string]: plugin.google.protobuf.IDescriptorProto; } -export interface EnumsMap { - [name: string]: plugin.google.protobuf.IEnumDescriptorProto; -} - // The following functions are used to add some metadata such as idempotence // flag, long running operation info, pagination, and streaming, to all the // methods of the given service, to use in templates. @@ -291,34 +287,43 @@ function getMethodConfig( return result; } +interface AugmentMethodParameters { + allMessages: MessagesMap; + localMessages: MessagesMap; + service: ServiceDescriptorProto; +} + function augmentMethod( - messages: MessagesMap, - service: ServiceDescriptorProto, + parameters: AugmentMethodParameters, method: MethodDescriptorProto ) { method = Object.assign( { longRunning: longrunning(method), longRunningResponseType: longRunningResponseType( - service.packageName, + parameters.service.packageName, method ), longRunningMetadataType: longRunningMetadataType( - service.packageName, + parameters.service.packageName, method ), streaming: streaming(method), - pagingFieldName: pagingFieldName(messages, method, service), - pagingResponseType: pagingResponseType(messages, method), + pagingFieldName: pagingFieldName( + parameters.allMessages, + method, + parameters.service + ), + pagingResponseType: pagingResponseType(parameters.allMessages, method), inputInterface: method.inputType!, outputInterface: method.outputType!, - comments: service.commentsMap.getMethodComments( - service.name!, + comments: parameters.service.commentsMap.getMethodComments( + parameters.service.name!, method.name! ), methodConfig: getMethodConfig( - service.grpcServiceConfig, - `${service.packageName}.${service.name!}`, + parameters.service.grpcServiceConfig, + `${parameters.service.packageName}.${parameters.service.name!}`, method.name! ), retryableCodesName: defaultNonIdempotentRetryCodesName, @@ -326,11 +331,11 @@ function augmentMethod( }, method ) as MethodDescriptorProto; - const bundleConfigs = service.bundleConfigs; + const bundleConfigs = parameters.service.bundleConfigs; if (bundleConfigs) { for (const bc of bundleConfigs) { if (bc.methodName === method.name) { - const inputType = messages[method.inputType!]; + const inputType = parameters.allMessages[method.inputType!]; const repeatedFields = inputType.field!.filter( field => field.label === @@ -343,12 +348,12 @@ function augmentMethod( } } } - if (method.inputType && messages[method.inputType]?.field) { + if (method.inputType && parameters.allMessages[method.inputType]?.field) { const paramComment: Comment[] = []; - const inputType = messages[method.inputType!]; + const inputType = parameters.allMessages[method.inputType!]; const inputmessageName = toMessageName(method.inputType); for (const field of inputType.field!) { - const comment = service.commentsMap.getParamComments( + const comment = parameters.service.commentsMap.getParamComments( inputmessageName, field.name! ); @@ -357,7 +362,7 @@ function augmentMethod( method.paramComment = paramComment; } if (method.methodConfig.retryPolicy?.retryableStatusCodes) { - method.retryableCodesName = service.retryableCodeMap.getRetryableCodesName( + method.retryableCodesName = parameters.service.retryableCodeMap.getRetryableCodesName( method.methodConfig.retryPolicy.retryableStatusCodes ); } @@ -388,7 +393,7 @@ function augmentMethod( defaultParameters.max_rpc_timeout_millis; retryParams.total_timeout_millis = defaultParameters.total_timeout_millis; - method.retryParamsName = service.retryableCodeMap.getParamsName( + method.retryParamsName = parameters.service.retryableCodeMap.getParamsName( retryParams ); } @@ -433,27 +438,39 @@ export function getHeaderRequestParams( return result; } -function augmentService( - messages: MessagesMap, - packageName: string, - service: plugin.google.protobuf.IServiceDescriptorProto, - commentsMap: CommentsMap, - allResourceDatabase: ResourceDatabase, - resourceDatabase: ResourceDatabase, - options: Options -) { - const augmentedService = service as ServiceDescriptorProto; - augmentedService.packageName = packageName; - augmentedService.iamService = options.iamService ?? false; - augmentedService.comments = commentsMap.getServiceComment(service.name!); - augmentedService.commentsMap = commentsMap; +interface AugmentServiceParameters { + allMessages: MessagesMap; + localMessages: MessagesMap; + packageName: string; + service: plugin.google.protobuf.IServiceDescriptorProto; + commentsMap: CommentsMap; + allResourceDatabase: ResourceDatabase; + resourceDatabase: ResourceDatabase; + options: Options; +} + +function augmentService(parameters: AugmentServiceParameters) { + const augmentedService = parameters.service as ServiceDescriptorProto; + augmentedService.packageName = parameters.packageName; + augmentedService.iamService = parameters.options.iamService ?? false; + augmentedService.comments = parameters.commentsMap.getServiceComment( + parameters.service.name! + ); + augmentedService.commentsMap = parameters.commentsMap; augmentedService.retryableCodeMap = new RetryableCodeMap(); - augmentedService.grpcServiceConfig = options.grpcServiceConfig; - augmentedService.bundleConfigs = options.bundleConfigs?.filter( - bc => bc.serviceName === service.name + augmentedService.grpcServiceConfig = parameters.options.grpcServiceConfig; + augmentedService.bundleConfigs = parameters.options.bundleConfigs?.filter( + bc => bc.serviceName === parameters.service.name ); augmentedService.method = augmentedService.method.map(method => - augmentMethod(messages, augmentedService, method) + augmentMethod( + { + allMessages: parameters.allMessages, + localMessages: parameters.localMessages, + service: augmentedService, + }, + method + ) ); augmentedService.bundleConfigsMethods = augmentedService.method.filter( method => method.bundleConfig @@ -505,21 +522,22 @@ function augmentService( // resourceDatabase: all resources defined by `google.api.resource` or `google.api.resource_definition` const uniqueResources: {[name: string]: ResourceDescriptor} = {}; // Copy all resources in resourceDatabase to uniqueResources - const allPatterns = resourceDatabase.patterns; + const allPatterns = parameters.resourceDatabase.patterns; for (const pattern of Object.keys(allPatterns)) { const resource = allPatterns[pattern]; uniqueResources[resource.name] = resource; } // Copy all resources definination which are referenced into unique resources map. - for (const property of Object.keys(messages)) { - const errorLocation = `service ${service.name} message ${property}`; - for (const fieldDescriptor of messages[property].field ?? []) { + for (const property of Object.keys(parameters.localMessages)) { + const errorLocation = `service ${parameters.service.name} message ${property}`; + for (const fieldDescriptor of parameters.localMessages[property].field ?? + []) { // note: ResourceDatabase can accept `undefined` values, so we happily use optional chaining here. const resourceReference = fieldDescriptor.options?.['.google.api.resourceReference']; // 1. If this resource reference has .child_type, figure out if we have any known parent resources. - const parentResources = allResourceDatabase.getParentResourcesByChildType( + const parentResources = parameters.allResourceDatabase.getParentResourcesByChildType( resourceReference?.childType, errorLocation ); @@ -529,7 +547,7 @@ function augmentService( // 2. If this resource reference has .type, we should have a known resource with this type, check two maps. if (!resourceReference || !resourceReference.type) continue; - const resourceByType = allResourceDatabase.getResourceByType( + const resourceByType = parameters.allResourceDatabase.getResourceByType( resourceReference?.type, errorLocation ); @@ -537,7 +555,7 @@ function augmentService( // For multi pattern resources, we look up the type first, and get the [pattern] from resource, // look up pattern map for all resources. for (const pattern of resourceByType!.pattern!) { - const resourceByPattern = allResourceDatabase.getResourceByPattern( + const resourceByPattern = parameters.allResourceDatabase.getResourceByPattern( pattern ); if (!resourceByPattern) continue; @@ -562,58 +580,54 @@ function augmentService( return augmentedService; } +interface ProtoParameters { + fd: plugin.google.protobuf.IFileDescriptorProto; + packageName: string; + allMessages: MessagesMap; + allResourceDatabase: ResourceDatabase; + resourceDatabase: ResourceDatabase; + options: Options; +} + export class Proto { filePB2: plugin.google.protobuf.IFileDescriptorProto; services: ServicesMap = {}; - messages: MessagesMap = {}; - enums: EnumsMap = {}; + allMessages: MessagesMap = {}; + localMessages: MessagesMap = {}; fileToGenerate: boolean; // TODO: need to store metadata? address? // allResourceDatabase: resources that defined by `google.api.resource` // resourceDatabase: all resources defined by `google.api.resource` or `google.api.resource_definition` - constructor( - fd: plugin.google.protobuf.IFileDescriptorProto, - packageName: string, - allResourceDatabase: ResourceDatabase, - resourceDatabase: ResourceDatabase, - options: Options - ) { - fd.enumType = fd.enumType || []; - fd.messageType = fd.messageType || []; - fd.service = fd.service || []; - - this.filePB2 = fd; + constructor(parameters: ProtoParameters) { + parameters.fd.service = parameters.fd.service || []; + parameters.fd.messageType = parameters.fd.messageType || []; - this.messages = fd.messageType + this.filePB2 = parameters.fd; + this.allMessages = parameters.allMessages; + this.localMessages = parameters.fd.messageType .filter(message => message.name) .reduce((map, message) => { - map['.' + fd.package! + '.' + message.name!] = message; + map[`.${parameters.fd.package!}.${message.name!}`] = message; return map; }, {} as MessagesMap); - - this.enums = fd.enumType - .filter(enum_ => enum_.name) - .reduce((map, enum_) => { - map[enum_.name!] = enum_; - return map; - }, {} as EnumsMap); - this.fileToGenerate = fd.package - ? fd.package.startsWith(packageName) + this.fileToGenerate = parameters.fd.package + ? parameters.fd.package.startsWith(parameters.packageName) : false; - const commentsMap = new CommentsMap(fd); - this.services = fd.service + const commentsMap = new CommentsMap(parameters.fd); + this.services = parameters.fd.service .filter(service => service.name) .map(service => - augmentService( - this.messages, - packageName, + augmentService({ + allMessages: this.allMessages, + localMessages: this.localMessages, + packageName: parameters.packageName, service, commentsMap, - allResourceDatabase, - resourceDatabase, - options - ) + allResourceDatabase: parameters.allResourceDatabase, + resourceDatabase: parameters.resourceDatabase, + options: parameters.options, + }) ) .reduce((map, service) => { map[service.name!] = service; diff --git a/typescript/test/unit/api.ts b/typescript/test/unit/api.ts index 0750dab2e..9d9304f19 100644 --- a/typescript/test/unit/api.ts +++ b/typescript/test/unit/api.ts @@ -15,7 +15,9 @@ import {API} from '../../src/schema/api'; import * as plugin from '../../../pbjs-genfiles/plugin'; import * as assert from 'assert'; -import {describe, it} from 'mocha'; +import {afterEach, describe, it} from 'mocha'; +import * as sinon from 'sinon'; +import * as proto from '../../src/schema/proto'; describe('src/schema/api.ts', () => { it('should construct an API object and return list of protos', () => { @@ -157,4 +159,45 @@ describe('src/schema/api.ts', () => { assert(api); }); }); + + describe('Calling Proto constructor', () => { + afterEach(() => { + sinon.restore(); + }); + + it('should pass all messages to Proto constructor', () => { + const fd1 = new plugin.google.protobuf.FileDescriptorProto(); + fd1.name = 'google/cloud/example/v1/test.proto'; + fd1.package = 'google.cloud.example.v1'; + + fd1.service = [new plugin.google.protobuf.ServiceDescriptorProto()]; + fd1.service[0].name = 'Service'; + fd1.service[0].options = { + '.google.api.defaultHost': 'hostname.example.com:443', + }; + fd1.messageType = [new plugin.google.protobuf.MethodDescriptorProto()]; + fd1.messageType[0].name = 'MessageA'; + + const fd2 = new plugin.google.protobuf.FileDescriptorProto(); + fd2.name = 'google/cloud/example/v1/example.proto'; + fd2.package = 'google.cloud.example.v1'; + fd2.messageType = [new plugin.google.protobuf.MethodDescriptorProto()]; + fd2.messageType[0].name = 'MessageB'; + + const spy = sinon.spy(proto, 'Proto'); + + new API([fd1, fd2], 'google.cloud.example.v1', { + grpcServiceConfig: new plugin.grpc.service_config.ServiceConfig(), + }); + + assert(spy.calledWithNew()); + assert.strictEqual(spy.callCount, 2); // one Proto object created for each fd + const firstCallMessages = spy.getCall(0).args[0].allMessages; + const secondCallMessages = spy.getCall(1).args[0].allMessages; + assert('.google.cloud.example.v1.MessageA' in firstCallMessages); + assert('.google.cloud.example.v1.MessageB' in firstCallMessages); + assert('.google.cloud.example.v1.MessageA' in secondCallMessages); + assert('.google.cloud.example.v1.MessageB' in secondCallMessages); + }); + }); }); diff --git a/typescript/test/unit/proto.ts b/typescript/test/unit/proto.ts index 9119637c5..491f3f424 100644 --- a/typescript/test/unit/proto.ts +++ b/typescript/test/unit/proto.ts @@ -15,7 +15,7 @@ import * as assert from 'assert'; import {describe, it} from 'mocha'; import * as plugin from '../../../pbjs-genfiles/plugin'; -import {getHeaderRequestParams} from '../../src/schema/proto'; +import {getHeaderRequestParams, MessagesMap} from '../../src/schema/proto'; import {Proto} from '../../src/schema/proto'; import {Options} from '../../src/schema/naming'; @@ -96,6 +96,7 @@ describe('src/schema/proto.ts', () => { ); }); }); + describe('special work around for talent API', () => { it('The pagingFieldName should be undefined for SearchJobs & SearchProfiles rpc', () => { const fd = new plugin.google.protobuf.FileDescriptorProto(); @@ -141,13 +142,20 @@ describe('src/schema/proto.ts', () => { const options: Options = { grpcServiceConfig: new plugin.grpc.service_config.ServiceConfig(), }; - const proto = new Proto( + const allMessages: MessagesMap = {}; + fd.messageType + .filter(message => message.name) + .forEach(message => { + allMessages['.' + fd.package! + '.' + message.name!] = message; + }); + const proto = new Proto({ fd, - 'google.cloud.talent.v4beta1', - new ResourceDatabase(), - new ResourceDatabase(), - options - ); + packageName: 'google.cloud.talent.v4beta1', + allMessages, + allResourceDatabase: new ResourceDatabase(), + resourceDatabase: new ResourceDatabase(), + options, + }); assert.deepStrictEqual( proto.services['service'].method[0].pagingFieldName, undefined