Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gRPC proto-loader options are global to the server rather than per-package #12560

Open
3 of 15 tasks
jtimmons opened this issue Oct 12, 2023 · 1 comment
Open
3 of 15 tasks
Labels
needs triage This issue has not been looked into

Comments

@jtimmons
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues

Current behavior

We currently use a global GrpcOptions object to create a gRPC server which includes proto-loader configuration in the GrpcOptions.options.loader field. This is a departure from how grpc-js works in which you would normally:

  1. Create a PackageDefinition from the proto files by loading it via proto-loader
  2. Load that PackageDefinition into a ProtoDescriptor using grpc-js and add that into the server
  3. Repeat for any additional service implementations that you have

By attaching the proto-loader configuration to the server instead of the package definition we lose some flexibility in how people configure their implementation logic. Particularly, this becomes an issue when attempting to use shared modules for common logic (eg. grpc-health-check, reflection-api)

Because the common logic is being loaded into someone else's configuration it has no control over settings which may be important to the implementation. Specifically: defaults for missing fields can be different based on that configuration, while the keepCase option can change entire field names

Minimum reproduction code

https://gitlab.com/jtimmons/nestjs-grpc-reflection-module/-/commits/temp/remove-req-res-conversion

Steps to reproduce

The linked reproduction is to a library that I maintain (nestjs-grpc-reflection-module) where I've had to work around this behavior. The linked branch disables that workaround behavior so that you can demonstrate it in the repo's sample server.

To reproduce:

  1. Startup sample server with npm i && npm run start:dev
  2. Turn keepCase on/off in the sample server's grpc-client options
  3. The sample server will not work when keepCase is set to true

The reflection module code is written to expect the default keepCase option of false and so, with the workaround logic disabled, will break when keepCase is set to true. This is because it will receive messages with field names such as list_services when the code itself is trying to access that field as the camel-cased name of listServices

Expected behavior

I would expect that we would be able to control the proto-loader configuration per module somehow

Package

  • I don't know. Or some 3rd-party package
  • @nestjs/common
  • @nestjs/core
  • @nestjs/microservices
  • @nestjs/platform-express
  • @nestjs/platform-fastify
  • @nestjs/platform-socket.io
  • @nestjs/platform-ws
  • @nestjs/testing
  • @nestjs/websockets
  • Other (see below)

Other package

No response

NestJS version

10.2.6

Packages versions

[System Information]
OS Version     : macOS
NodeJS Version : v18.17.1
NPM Version    : 9.6.7 

[Nest CLI]
Nest CLI Version : 10.1.18 

[Nest Platform Information]
schematics version : 10.0.2
testing version    : 10.2.6
config version     : 3.1.1
cli version        : 10.1.18

Node.js version

v20.8.0

In which operating systems have you tested?

  • macOS
  • Windows
  • Linux

Other

PR #10530 doesn't directly fix this but may offer a good path forward? If a module could add a full PackageDefinition to the list of services to add to a gRPC server then it could solve the problem somewhat

@kamilmysliwiec
Copy link
Member

#10530 has just been merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage This issue has not been looked into
Projects
None yet
Development

No branches or pull requests

2 participants