refactor(grpc-reflection): file cleanup and enabled ts strict mode

jtimmons · jtimmons · commit 3b4f92ee6201 · 2023-11-08T22:43:18.000-05:00
diff --git a/packages/grpc-reflection/example/server.ts b/packages/grpc-reflection/example/server.ts
@@ -9,14 +9,9 @@ const INCLUDE_PATH = path.join(__dirname, '../proto/sample/vendor');
 
 const server = new grpc.Server();
 const packageDefinition = protoLoader.loadSync(PROTO_PATH, { includeDirs: [INCLUDE_PATH] });
-const reflection = new ReflectionService(packageDefinition);
+const reflection = new ReflectionService(packageDefinition, { services: ['sample.SampleService'] });
 reflection.addToServer(server);
 
 server.bindAsync('localhost:5000', grpc.ServerCredentials.createInsecure(), () => {
   server.start();
 });
-
-// const protoDescriptor = grpc.loadPackageDefinition(packageDefinition);
-
-
-
diff --git a/packages/grpc-reflection/proto/sample/sample.proto b/packages/grpc-reflection/proto/sample/sample.proto
@@ -9,6 +9,10 @@ service SampleService {
   rpc Hello2 (HelloRequest) returns (CommonMessage) {}
 }
 
+service IgnoreService {
+  rpc Hello (HelloRequest) returns (HelloResponse) {}
+}
+
 message HelloRequest {
   string hello = 1;
   HelloNested nested = 2;
diff --git a/packages/grpc-reflection/src/implementations/common/constants.ts b/packages/grpc-reflection/src/implementations/common/constants.ts
diff --git a/packages/grpc-reflection/src/implementations/common/interfaces.ts b/packages/grpc-reflection/src/implementations/common/interfaces.ts
@@ -0,0 +1,5 @@
+/** Options to create a reflection server */
+export interface ReflectionServerOptions {
+  /** whitelist of fully-qualified service names to expose. (Default: expose all) */
+  services?: string[];
+}
diff --git a/packages/grpc-reflection/src/implementations/common/protobuf-visitor.ts b/packages/grpc-reflection/src/implementations/common/protobuf-visitor.ts
@@ -101,7 +101,7 @@ export const visit = (file: FileDescriptorProto, visitor: Visitor): void => {
     });
   };
 
-  const packageName = file.getPackage();
+  const packageName = file.getPackage() || '';
   file.getEnumTypeList().forEach((type) => processEnum(packageName, file, type));
   file.getMessageTypeList().forEach((type) => processMessage(packageName, file, type));
   file.getServiceList().forEach((service) => processService(packageName, file, service));
diff --git a/packages/grpc-reflection/src/implementations/common/utils.ts b/packages/grpc-reflection/src/implementations/common/utils.ts
diff --git a/packages/grpc-reflection/src/implementations/reflection-v1.ts b/packages/grpc-reflection/src/implementations/reflection-v1.ts
@@ -7,14 +7,15 @@ import {
 import * as grpc from '@grpc/grpc-js';
 import * as protoLoader from '@grpc/proto-loader';
 
-import { ExtensionNumberResponse__Output } from './generated/grpc/reflection/v1/ExtensionNumberResponse';
-import { FileDescriptorResponse__Output } from './generated/grpc/reflection/v1/FileDescriptorResponse';
-import { ListServiceResponse__Output } from './generated/grpc/reflection/v1/ListServiceResponse';
-import { ServerReflectionRequest } from './generated/grpc/reflection/v1/ServerReflectionRequest';
-import { ServerReflectionResponse } from './generated/grpc/reflection/v1/ServerReflectionResponse';
-import { visit } from './protobuf-visitor';
-import { scope } from './utils';
-import { PROTO_LOADER_OPTS } from './constants';
+import { ExtensionNumberResponse__Output } from '../generated/grpc/reflection/v1/ExtensionNumberResponse';
+import { FileDescriptorResponse__Output } from '../generated/grpc/reflection/v1/FileDescriptorResponse';
+import { ListServiceResponse__Output } from '../generated/grpc/reflection/v1/ListServiceResponse';
+import { ServerReflectionRequest } from '../generated/grpc/reflection/v1/ServerReflectionRequest';
+import { ServerReflectionResponse } from '../generated/grpc/reflection/v1/ServerReflectionResponse';
+import { visit } from './common/protobuf-visitor';
+import { scope } from './common/utils';
+import { PROTO_LOADER_OPTS } from './common/constants';
+import { ReflectionServerOptions } from './common/interfaces';
 
 export class ReflectionError extends Error {
   constructor(
@@ -37,22 +38,27 @@ export class ReflectionError extends Error {
 export class ReflectionV1Implementation {
 
   /** The full list of proto files (including imported deps) that the gRPC server includes */
-  private fileDescriptorSet = new FileDescriptorSet();
+  private readonly fileDescriptorSet = new FileDescriptorSet();
 
   /** An index of proto files by file name (eg. 'sample.proto') */
-  private fileNameIndex: Record<string, FileDescriptorProto> = {};
+  private readonly fileNameIndex: Record<string, FileDescriptorProto> = {};
 
   /** An index of proto files by type extension relationship
    *
    * extensionIndex[<pkg>.<msg>][<field#>] contains a reference to the file containing an
    * extension for the type "<pkg>.<msg>" and field number "<field#>"
    */
-  private extensionIndex: Record<string, Record<number, FileDescriptorProto>> = {};
+  private readonly extensionIndex: Record<string, Record<number, FileDescriptorProto>> = {};
 
   /** An index of fully qualified symbol names (eg. 'sample.Message') to the files that contain them */
-  private symbolMap: Record<string, FileDescriptorProto> = {};
+  private readonly symbolMap: Record<string, FileDescriptorProto> = {};
+
+  /** Options that the user provided for this service */
+  private readonly options?: ReflectionServerOptions;
+
+  constructor(root: protoLoader.PackageDefinition, options?: ReflectionServerOptions) {
+    this.options = options;
 
-  constructor(root: protoLoader.PackageDefinition) {
     Object.values(root).forEach(({ fileDescriptorProtos }) => {
       // Add file descriptors to the FileDescriptorSet.
       // We use the Array check here because a ServiceDefinition could have a method named the same thing
@@ -88,10 +94,10 @@ export class ReflectionV1Implementation {
         extension: (fqn, file, ext) => {
           index(fqn, file);
 
-          const extendeeName = ext.getExtendee();
+          const extendeeName = ext.getExtendee() || '';
           this.extensionIndex[extendeeName] = {
             ...(this.extensionIndex[extendeeName] || {}),
-            [ext.getNumber()]: file,
+            [ext.getNumber() || -1]: file,
           };
         },
       }),
@@ -126,25 +132,26 @@ export class ReflectionV1Implementation {
         return;
       }
 
-      if (referencedFile !== sourceFile) {
-        sourceFile.addDependency(referencedFile.getName());
+      const fname = referencedFile.getName();
+      if (referencedFile !== sourceFile && fname) {
+        sourceFile.addDependency(fname);
       }
     };
 
     this.fileDescriptorSet.getFileList().forEach((file) =>
       visit(file, {
-        field: (fqn, file, field) => addReference(field.getTypeName(), file, scope(fqn)),
-        extension: (fqn, file, ext) => addReference(ext.getTypeName(), file, scope(fqn)),
+        field: (fqn, file, field) => addReference(field.getTypeName() || '', file, scope(fqn)),
+        extension: (fqn, file, ext) => addReference(ext.getTypeName() || '', file, scope(fqn)),
         method: (fqn, file, method) => {
-          addReference(method.getInputType(), file, scope(fqn));
-          addReference(method.getOutputType(), file, scope(fqn));
+          addReference(method.getInputType() || '', file, scope(fqn));
+          addReference(method.getOutputType() || '', file, scope(fqn));
         },
       }),
     );
   }
 
   addToServer(server: Pick<grpc.Server, 'addService'>) {
-    const protoPath = path.join(__dirname, '../proto/grpc/reflection/v1/reflection.proto');
+    const protoPath = path.join(__dirname, '../../proto/grpc/reflection/v1/reflection.proto');
     const pkgDefinition = protoLoader.loadSync(protoPath, PROTO_LOADER_OPTS);
     const pkg = grpc.loadPackageDefinition(pkgDefinition) as any;
 
@@ -180,8 +187,10 @@ export class ReflectionV1Implementation {
       } else if (message.fileByFilename !== undefined) {
         response.fileDescriptorResponse = this.fileByFilename(message.fileByFilename);
       } else if (message.fileContainingExtension !== undefined) {
-        const { containingType, extensionNumber } = message.fileContainingExtension;
-        response.fileDescriptorResponse = this.fileContainingExtension(containingType, extensionNumber);
+        response.fileDescriptorResponse = this.fileContainingExtension(
+          message.fileContainingExtension?.containingType || '',
+          message.fileContainingExtension?.extensionNumber || -1
+        );
       } else if (message.allExtensionNumbersOfType) {
         response.allExtensionNumbersResponse = this.allExtensionNumbersOfType(message.allExtensionNumbersOfType);
       } else {
@@ -224,7 +233,12 @@ export class ReflectionV1Implementation {
       )
       .flat();
 
-    return { service: services.map((service) => ({ name: service })) };
+    const whitelist = new Set(this.options?.services ?? undefined);
+    const exposedServices = this.options?.services ?
+      services.filter(service => whitelist.has(service))
+      : services;
+
+    return { service: exposedServices.map((service) => ({ name: service })) };
   }
 
   /** Find the proto file(s) that declares the given fully-qualified symbol name
diff --git a/packages/grpc-reflection/src/implementations/reflection-v1alpha.ts b/packages/grpc-reflection/src/implementations/reflection-v1alpha.ts
@@ -3,10 +3,10 @@ import * as path from 'path';
 import * as grpc from '@grpc/grpc-js';
 import * as protoLoader from '@grpc/proto-loader';
 
-import { ServerReflectionRequest } from './generated/grpc/reflection/v1/ServerReflectionRequest';
-import { ServerReflectionResponse } from './generated/grpc/reflection/v1/ServerReflectionResponse';
-import { PROTO_LOADER_OPTS } from './constants';
-import { ReflectionV1Implementation } from './reflection-v1-implementation';
+import { ServerReflectionRequest } from '../generated/grpc/reflection/v1/ServerReflectionRequest';
+import { ServerReflectionResponse } from '../generated/grpc/reflection/v1/ServerReflectionResponse';
+import { PROTO_LOADER_OPTS } from './common/constants';
+import { ReflectionV1Implementation } from './reflection-v1';
 
 
 /** Analyzes a gRPC server and exposes methods to reflect on it
@@ -18,12 +18,12 @@ import { ReflectionV1Implementation } from './reflection-v1-implementation';
  * For example: if files 'a.proto' and 'b.proto' are both for the same package 'c' then
  * we will always return a reference to a combined 'c.proto' instead of the 2 files.
  *
- * @remarks as the v1 and v1alpha specs are identical, this implementation extends v1
- * and just exposes it at the v1alpha package instead
+ * @privateRemarks as the v1 and v1alpha specs are identical, this implementation extends
+ * reflection-v1 and exposes it at the v1alpha package instead
  */
 export class ReflectionV1AlphaImplementation extends ReflectionV1Implementation {
   addToServer(server: Pick<grpc.Server, 'addService'>) {
-    const protoPath = path.join(__dirname, '../proto/grpc/reflection/v1alpha/reflection.proto');
+    const protoPath = path.join(__dirname, '../../proto/grpc/reflection/v1alpha/reflection.proto');
     const pkgDefinition = protoLoader.loadSync(protoPath, PROTO_LOADER_OPTS);
     const pkg = grpc.loadPackageDefinition(pkgDefinition) as any;
 
diff --git a/packages/grpc-reflection/src/service.ts b/packages/grpc-reflection/src/service.ts
@@ -1,13 +1,9 @@
 import * as grpc from '@grpc/grpc-js';
 import * as protoLoader from '@grpc/proto-loader';
 
-import { ReflectionV1Implementation } from './reflection-v1-implementation';
-import { ReflectionV1AlphaImplementation } from './reflection-v1alpha';
-
-interface ReflectionServerOptions {
-  /** whitelist of fully-qualified service names to expose. (Default: expose all) */
-  services?: string[];
-}
+import { ReflectionV1Implementation } from './implementations/reflection-v1';
+import { ReflectionV1AlphaImplementation } from './implementations/reflection-v1alpha';
+import { ReflectionServerOptions } from './implementations/common/interfaces';
 
 /** Analyzes a gRPC package and exposes endpoints providing information about
  *  it according to the gRPC Server Reflection API Specification
@@ -31,21 +27,8 @@ export class ReflectionService {
   private readonly v1Alpha: ReflectionV1AlphaImplementation;
   
   constructor(pkg: protoLoader.PackageDefinition, options?: ReflectionServerOptions) {
-
-    if (options.services) {
-      const whitelist = new Set(options.services);
-
-      for (const key in Object.keys(pkg)) {
-        const value = pkg[key];
-        const isService = value.format !== 'Protocol Buffer 3 DescriptorProto' && value.format !== 'Protocol Buffer 3 EnumDescriptorProto';
-        if (isService && !whitelist.has(key)) {
-          delete pkg[key];
-        }
-      }
-    }
-
-    this.v1 = new ReflectionV1Implementation(pkg);
-    this.v1Alpha = new ReflectionV1AlphaImplementation(pkg);
+    this.v1 = new ReflectionV1Implementation(pkg, options);
+    this.v1Alpha = new ReflectionV1AlphaImplementation(pkg, options);
   }
 
   addToServer(server: Pick<grpc.Server, 'addService'>) {
diff --git a/packages/grpc-reflection/test/test-reflection-v1-implementation.ts b/packages/grpc-reflection/test/test-reflection-v1-implementation.ts
@@ -3,14 +3,12 @@ import * as path from 'path';
 import { FileDescriptorProto } from 'google-protobuf/google/protobuf/descriptor_pb';
 import * as protoLoader from '@grpc/proto-loader';
 
-import { ReflectionV1Implementation } from '../src/reflection-v1-implementation';
+import { ReflectionV1Implementation } from '../src/implementations/reflection-v1';
 
 describe('GrpcReflectionService', () => {
   let reflectionService: ReflectionV1Implementation;
 
   beforeEach(async () => {
-    console.log(path.join(__dirname, '../proto/sample/sample.proto'));
-    console.log([path.join(__dirname, '../proto/sample/vendor')]);
     const root = protoLoader.loadSync(path.join(__dirname, '../proto/sample/sample.proto'), {
       includeDirs: [path.join(__dirname, '../proto/sample/vendor')]
     });
@@ -20,6 +18,18 @@ describe('GrpcReflectionService', () => {
 
   describe('listServices()', () => {
     it('lists all services', () => {
+      const { service: services } = reflectionService.listServices('*');
+      assert.equal(services.length, 2);
+      assert(services.find((s) => s.name === 'sample.SampleService'));
+    });
+
+    it('whitelists services properly', () => {
+      const root = protoLoader.loadSync(path.join(__dirname, '../proto/sample/sample.proto'), {
+        includeDirs: [path.join(__dirname, '../proto/sample/vendor')]
+      });
+
+      reflectionService = new ReflectionV1Implementation(root, { services: ['sample.SampleService'] });
+
       const { service: services } = reflectionService.listServices('*');
       assert.equal(services.length, 1);
       assert(services.find((s) => s.name === 'sample.SampleService'));
diff --git a/packages/grpc-reflection/test/test-utils.ts b/packages/grpc-reflection/test/test-utils.ts
@@ -1,6 +1,6 @@
 import * as assert from 'assert';
 
-import { scope } from '../src/utils';
+import { scope } from '../src/implementations/common/utils';
 
 describe('scope', () => {
   it('traverses upwards in the package scope', () => {
diff --git a/packages/grpc-reflection/tsconfig.json b/packages/grpc-reflection/tsconfig.json
@@ -9,7 +9,7 @@
     "noImplicitReturns": true,
     "pretty": true,
     "sourceMap": true,
-    "strictNullChecks": false,
+    "strict": true,
     "lib": ["es2017"],
     "outDir": "build",
     "target": "es2017",
