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

@nestjs/microservices can't handle gRPC package versioning. #13719

Closed
3 of 15 tasks
mishio-n opened this issue Jun 24, 2024 · 3 comments
Closed
3 of 15 tasks

@nestjs/microservices can't handle gRPC package versioning. #13719

mishio-n opened this issue Jun 24, 2024 · 3 comments
Labels
needs triage This issue has not been looked into

Comments

@mishio-n
Copy link

mishio-n commented Jun 24, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Current behavior

As shown in this document, gRPC allows version control by including the version number in the "package".

https://learn.microsoft.com/en-us/aspnet/core/grpc/versioning?view=aspnetcore-3.1#version-number-services

Normally, each implementation should be called according to the requested version.

However, in "@nestjs/microservices", the wrong handler is invoked when a gRPC package with a different version is registered.

Minimum reproduction code

https://github.com/mishio-n/nest-grpc-multi-versioning/

├── package.json
├── pnpm-lock.yaml
├── proto
│   └── hello
│       ├── v1
│       │   └── hello.proto
│       └── v2
│           └── hello.proto
├── src
│   ├── app.module.ts
│   ├── hello
│   │   ├── hello.module.ts
│   │   ├── v1
│   │   │   ├── hello.v1.controller.ts
│   │   │   └── hello_pb.ts
│   │   └── v2
│   │       ├── hello.v2.controller.ts
│   │       └── hello_pb.ts
│   └── main.ts

controller for v1

import { Controller } from '@nestjs/common';
import { GrpcMethod } from '@nestjs/microservices';
import {
  type SayHelloRequest,
  SayHelloResponse,
  VersionResponse,
} from './hello_pb';

@Controller()
export class HelloV1Controller {
  @GrpcMethod('HelloService', 'SayHello')
  sayHello(data: SayHelloRequest): SayHelloResponse {
    return new SayHelloResponse({
      message: `Hello, ${data.name}`,
    });
  }

  @GrpcMethod('HelloService', 'Version')
  version(): VersionResponse {
    return new VersionResponse({
      version: 'v1',
    });
  }
}

controller for v2

import { Controller } from '@nestjs/common';
import { GrpcMethod } from '@nestjs/microservices';
import {
  type SayHelloRequest,
  SayHelloResponse,
  VersionResponse,
} from './hello_pb';

@Controller()
export class HelloV2Controller {
  @GrpcMethod('HelloService', 'SayHello')
  sayHello(data: SayHelloRequest): SayHelloResponse {
    return new SayHelloResponse({
      message: `Hello, ${data.name}`,
    });
  }

  @GrpcMethod('HelloService', 'Version')
  version(): VersionResponse {
    return new VersionResponse({
      version: 'v2',
    });
  }
}

Steps to reproduce

I'm using Buf CLI for gRPC request.

  1. pnpm start:dev
  2. gRPC call for hello.v1.HelloService/Version
buf curl --schema proto --protocol grpc --http2-prior-knowledge http://localhost:3000/hello.v1.HelloService/Version
  1. get response
{
  "version": "2"
}

Since the request is for v1, "v1" is expected to be returned.
https://github.com/mishio-n/nest-grpc-multi-versioning/blob/main/src/hello/v1/hello.v1.controller.ts#L21

Expected behavior

After checking the source code of "@nestjs/microservices", I believe this behavior is caused by the following code.

https://github.com/nestjs/nest/blob/master/packages/microservices/server/server-grpc.ts#L447-L455

Here, "route" contains the following information.

"{ service: 'HelloService', rpc: 'SayHello', streaming: 'no_stream' }"

The information comes from the "@GrpcMethod" decorator, and I see no way to include the package name here.
So, implementations between different packages will be registered overwriting.

export function GrpcMethod(service?: string): MethodDecorator;
export function GrpcMethod(service: string, method?: string): MethodDecorator;
export function GrpcMethod(service: string, method?: string): MethodDecorator {

I may be mistaken, though,
How can I handle versioning?

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.0.0

Packages versions

  "dependencies": {
    "@bufbuild/protobuf": "^1.10.0",
    "@grpc/grpc-js": "^1.10.9",
    "@grpc/proto-loader": "^0.7.13",
    "@nestjs/common": "^10.0.0",
    "@nestjs/core": "^10.0.0",
    "@nestjs/microservices": "^10.3.9",
    "@nestjs/platform-express": "^10.0.0",
    "reflect-metadata": "^0.2.0",
    "rxjs": "^7.8.1"
  },
  "devDependencies": {
    "@nestjs/cli": "^10.0.0",
    "@nestjs/schematics": "^10.0.0",
    "@nestjs/testing": "^10.0.0",
    "@types/express": "^4.17.17",
    "@types/jest": "^29.5.2",
    "@types/node": "^20.3.1",
    "@types/supertest": "^6.0.0",
    "@typescript-eslint/eslint-plugin": "^6.0.0",
    "@typescript-eslint/parser": "^6.0.0",
    "eslint": "^8.42.0",
    "eslint-config-prettier": "^9.0.0",
    "eslint-plugin-prettier": "^5.0.0",
    "jest": "^29.5.0",
    "prettier": "^3.0.0",
    "source-map-support": "^0.5.21",
    "supertest": "^6.3.3",
    "ts-jest": "^29.1.0",
    "ts-loader": "^9.4.3",
    "ts-node": "^10.9.1",
    "tsconfig-paths": "^4.2.0",
    "typescript": "^5.1.3"
  },

Node.js version

20.11.1

In which operating systems have you tested?

  • macOS
  • Windows
  • Linux

Other

No response

@JadenKim-dev
Copy link

I think it would be good if the @GrpcMethod decorator could specify the package name along with the service name, like below.

  @GrpcMethod('hello.v2.HelloService', 'Version')
  version(): VersionResponse {
    return new VersionResponse({
      version: 'v1',
    });
  }

I have worked on it in the #13731, so please review it.

@mishio-n
Copy link
Author

@JadenKim-dev

Cool!

This is exactly the code I was thinking of.
I got it running in my hand and it worked as expected.

I hope this PR gets merged.

@kamilmysliwiec
Copy link
Member

Let's track this here #13731

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

3 participants