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 exceptions and filter #1953

Open
tychota opened this issue Apr 7, 2019 · 14 comments
Open

gRPC exceptions and filter #1953

tychota opened this issue Apr 7, 2019 · 14 comments

Comments

@tychota
Copy link
Contributor

tychota commented Apr 7, 2019

After discussion the goal is to implement gRPC exceptions and filter, and document it, to improve gRPC integration.

See #1953 (comment)

-- OLD ISSUE --

I'm submitting a...

#1015 report a similar problem but the result did not helped me fixing my issue

See what i tried section.


[ ] Regression 
[ ] Bug report
[ ] Feature request
[x] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

I have an application that expose a public HTTP api.
The "gateway" communicate with microservices through GRPC.

@Injectable()
export class AuthService {
  constructor(private readonly commandBus: CommandBus, private readonly queryBus: QueryBus) {}

  // ...

  async assertEmailDoesNotExist(email: string) {
    const emailAlreadyExist = await this.queryBus.execute(new DoesEmailExistQuery(email));
    if (emailAlreadyExist) {
      // >>>> Here <<<<
      // I would like to display an "ALREADY_EXIST"
      // https://github.com/grpc/grpc/blob/master/doc/statuscodes.md
      const exception = new RpcException({ message: 'Email already exist', status: 6 });
      throw exception;
    }
  }
}

but I get

{
  "level":50,
   /// ...
  "context":"ExceptionsHandler",
  "msg":"2 UNKNOWN: Email already exist",
  "v":1
}

in my log (custom pino logger).

Expected behavior

{
  "level":50,
   /// ...
  "context":"ExceptionsHandler",
  "msg":"6 ALREADY_EXISTS: Email already exist",
  "v":1
}

What i tried

  • setting the code of the exception (like ex.code = xxx)
  • passing an object RpcException({ message: 'Email already exist', status: 6 })
  • doing a rpc filter (as suggested in issue)
import { ExceptionFilter, Catch, ArgumentsHost, HttpException, RpcExceptionFilter } from '@nestjs/common';
import { RpcException, BaseRpcExceptionFilter } from '@nestjs/microservices';
import { Observable, throwError } from 'rxjs';

export class HttpExceptionFilter extends BaseRpcExceptionFilter implements RpcExceptionFilter {
  catch(exception: RpcException, host: ArgumentsHost): Observable<unknown> {
    if (exception instanceof RpcException) {
      return throwError({status: 6})
    }
    return super.catch(exception, host);
  }
}

Ultimatly, without doc, I lack knowledge how to solve my problem. I did try but I guess not in the right direction

Minimal reproduction of the problem with instructions

Not really minimal (but close two, one api gateway, two microservices) but you can found the codebase here: https://github.com/tychota/shitake

To run it:

  • yarn
  • create a db "events" and a db "accounts" in postgres with user "shitake"/"password"
  • (in one terminal) yarn webpack -f webpack.config.js
  • (in an other) node dist/server.js
  • go to swagger: http://localhost:3001/api/#/auth/post_auth_register
  • register twice with same email

What is the motivation / use case for changing the behavior?

In http, it is super nice, you can just throw a ConflictException and boom, you have a 429.

In GRPC world it is not as nice :/

You have to play with message string (hardcoded stuff) and ultimatly not every status code are equal (https://github.com/grpc/grpc/blob/master/doc/statuscodes.md) (like a 401 in HTTp may be expected but a 500 isn't). Here you have just the equivalence of 500, with all what is implied (logging of a 500).

Environment


Nest version:
    "@nestjs/common": "^6.0.5",
    "@nestjs/cqrs": "^6.0.0",
 
For Tooling issues:
- Node version: v8.12.0
- Platform:  Linux, Gentoo

Closing

Let me know if it is clear enough.

@tychota
Copy link
Contributor Author

tychota commented Apr 7, 2019

Applying #764 (comment) solve part of my problem.

The GRPC status is now correct but i can't try catch in the gateway:

Expected

@Controller('auth')
export class AuthGatewayController implements OnModuleInit {
  @Client(authClientOptions)
  private readonly authClient!: ClientGrpc;

  private authCommand!: AuthCommand;

  onModuleInit() {
    this.authCommand = this.authClient.getService<AuthCommand>('Command');
  }


  @Post('/register')
  // @UseFilters(new GrpcExceptionFilter())
  @UsePipes(new ValidationPipe({ transform: true }))
  async register(@Body() dto: AuthClearTextCredentialsDto) {
    try {
      return this.authCommand.register(dto);
   } catch (e) {
       // do dmthg with e and raise HTPP status code accordingly
   }
  }
}

@kamilmysliwiec
Copy link
Member

Please, use StackOverflow for such questions.

Btw, we will add dedicated exceptions for gRPC shortly.

@tychota
Copy link
Contributor Author

tychota commented Apr 8, 2019

Please, use StackOverflow for such questions.

Don't you think this issue was fitting the specific case:

[x] Documentation issue or request

(Goal is not to criticise the issue being closed but to understand a bit more as without explanation it just feel a bit harsh: "such questions" is a bit condescending, at least I understand it like so, and that does not reflect the time I put filling the issue).

Don't get me wrong, I love Nest and most of the lib is super well designed and documented. I'm also not mad at you and I know maintaining open source is involving and tiresome sometimes.

That being said, I still think GRPC is hard to get right with the current doc and tooling in Nest.

My goal is to help Nest and do some PR in th doc so it get easier in the future.
Thus I wanted to discuss a real use case to figure out the good way to answer this problem:

When doing a microservice with nest that use GRPC, it is hard to make a micro service fail and get another status than 500 in the gateway

Should I open another issue without the complex example to track that use case ?

How would you like go forward on this ?

Btw, we will add dedicated exceptions for gRPC shortly.

That is nice 😍

I started something like this here : https://github.com/tychota/shitake/blob/master/packages/utils-grpc/exception.ts
I can PR if needed.

I also created a mapping https://github.com/tychota/shitake/blob/master/packages/utils-grpc/mapping.ts so it is easy to raise appropriate HTTP exception based on GRPC status.

Look at https://github.com/tychota/shitake/blob/master/packages/utils-grpc/formatHttpResponse.ts and usage here: https://github.com/tychota/shitake/blob/master/packages/api-gateway/controllers/auth.controller.ts#L55-L63

@kamilmysliwiec
Copy link
Member

(Goal is not to criticise the issue being closed but to understand a bit more as without explanation it just feel a bit harsh: "such questions" is a bit condescending, at least I understand it like so, and that does not reflect the time I put filling the issue).
Don't get me wrong, I love Nest and most of the lib is super well designed and documented. I'm also not mad at you and I know maintaining open source is involving and tiresome sometimes.

It's all about time @tychota. I'd love to write long posts. I can't do it though :) Anyway, thanks for getting me full context.

This: https://github.com/tychota/shitake/blob/master/packages/utils-grpc/exception.ts is awesome.
And this: https://github.com/tychota/shitake/blob/master/packages/utils-grpc/mapping.ts would be useful as well.

It would be great to create a filter that maps these exceptions to gRPC error objects.

Would you like to create a PR with your files? :)

@kamilmysliwiec kamilmysliwiec reopened this Apr 8, 2019
@kamilmysliwiec kamilmysliwiec changed the title Set custom statusCode for a GRPC Exception gRPC exceptions and filter Apr 8, 2019
@kamilmysliwiec
Copy link
Member

Let's track this here

@tychota
Copy link
Contributor Author

tychota commented Apr 8, 2019

Thank for answering and reopening: I understand and my issue was not super quality anyway.

I will create the PR with the above file and then we can see how to improve the doc.

@tychota
Copy link
Contributor Author

tychota commented Apr 8, 2019

Shall I edit the description of first post to reflect new title ?

@kamilmysliwiec
Copy link
Member

kamilmysliwiec commented Apr 8, 2019

I will create the PR with the above file and then we can see how to improve the doc.

Amazing.

Shall I edit the description of first post to reflect new title ?

This one is fine - at least outlines the problem and lack of the functionality very well :)

In order to make the integration better, we would need:

  • a dedicated set of gRPC exceptions
  • GrpcExceptionFiler that would catch all these exceptions and serialize to gRPC error objects
  • and + additionally, we can provide this mapping (HTTP -> gRPC utils)

@tychota
Copy link
Contributor Author

tychota commented Apr 8, 2019

Here you are: the wip start of the PR #1957.

@raikusy
Copy link

raikusy commented Mar 20, 2020

Any update on this?

@sadhakbj
Copy link

Any updates on this?

@moshest
Copy link

moshest commented Jun 30, 2022

If anyone needs a quick mapping from HttpException to gRPC:

import {
  Catch,
  ExceptionFilter,
  HttpException,
  HttpStatus,
} from '@nestjs/common';
import { Observable, throwError } from 'rxjs';
import { status } from '@grpc/grpc-js';

@Catch(HttpException)
export class HttpExceptionFilter implements ExceptionFilter {
  static HttpStatusCode: Record<number, number> = {
    // standard gRPC error mapping
    // https://cloud.google.com/apis/design/errors#handling_errors
    [HttpStatus.BAD_REQUEST]: status.INVALID_ARGUMENT,
    [HttpStatus.UNAUTHORIZED]: status.UNAUTHENTICATED,
    [HttpStatus.FORBIDDEN]: status.PERMISSION_DENIED,
    [HttpStatus.NOT_FOUND]: status.NOT_FOUND,
    [HttpStatus.CONFLICT]: status.ALREADY_EXISTS,
    [HttpStatus.GONE]: status.ABORTED,
    [HttpStatus.TOO_MANY_REQUESTS]: status.RESOURCE_EXHAUSTED,
    499: status.CANCELLED,
    [HttpStatus.INTERNAL_SERVER_ERROR]: status.INTERNAL,
    [HttpStatus.NOT_IMPLEMENTED]: status.UNIMPLEMENTED,
    [HttpStatus.BAD_GATEWAY]: status.UNKNOWN,
    [HttpStatus.SERVICE_UNAVAILABLE]: status.UNAVAILABLE,
    [HttpStatus.GATEWAY_TIMEOUT]: status.DEADLINE_EXCEEDED,

    // additional built-in http exceptions
    // https://docs.nestjs.com/exception-filters#built-in-http-exceptions
    [HttpStatus.HTTP_VERSION_NOT_SUPPORTED]: status.UNAVAILABLE,
    [HttpStatus.PAYLOAD_TOO_LARGE]: status.OUT_OF_RANGE,
    [HttpStatus.UNSUPPORTED_MEDIA_TYPE]: status.CANCELLED,
    [HttpStatus.UNPROCESSABLE_ENTITY]: status.CANCELLED,
    [HttpStatus.I_AM_A_TEAPOT]: status.UNKNOWN,
    [HttpStatus.METHOD_NOT_ALLOWED]: status.CANCELLED,
    [HttpStatus.PRECONDITION_FAILED]: status.FAILED_PRECONDITION,
  };

  catch(exception: HttpException): Observable<never> | void {
    const httpStatus = exception.getStatus();
    const httpRes = exception.getResponse() as { details?: unknown };

    return throwError(() => ({
      code: HttpExceptionFilter.HttpStatusCode[httpStatus] ?? status.UNKNOWN,
      message: exception.message,
      details: Array.isArray(httpRes.details) ? httpRes.details : undefined,
    }));
  }
}

Then connect it as a global filter on the microservice:

const microservice = await NestFactory.createMicroservice<MicroserviceOptions>(...);

microservice.useGlobalFilters(new HttpExceptionFilter());

@single9
Copy link

single9 commented Jul 19, 2022

@moshest You save my time!

@Farenheith
Copy link

I have a hybrid http1/grpc application. The grpc app have either unary and stream server methods, and returning throwError wasn't working to treat the stream server errors, so, I created this base class to do all the exception filters, and it worked in every scenario:

import { throwError } from 'rxjs';
import { FastifyReply } from 'fastify';
import { ArgumentsHost, ExceptionFilter, HttpStatus } from '@nestjs/common';
import { ServerUnaryCall, ServerWritableStream, status } from '@grpc/grpc-js';

const httpToGrpc: Record<number, number> = {
	[HttpStatus.BAD_REQUEST]: status.INVALID_ARGUMENT,
	[HttpStatus.UNAUTHORIZED]: status.UNAUTHENTICATED,
	[HttpStatus.FORBIDDEN]: status.PERMISSION_DENIED,
	[HttpStatus.NOT_FOUND]: status.NOT_FOUND,
	[HttpStatus.CONFLICT]: status.ALREADY_EXISTS,
	[HttpStatus.GONE]: status.ABORTED,
	[HttpStatus.TOO_MANY_REQUESTS]: status.RESOURCE_EXHAUSTED,
	499: status.CANCELLED,
	[HttpStatus.INTERNAL_SERVER_ERROR]: status.INTERNAL,
	[HttpStatus.NOT_IMPLEMENTED]: status.UNIMPLEMENTED,
	[HttpStatus.BAD_GATEWAY]: status.UNKNOWN,
	[HttpStatus.SERVICE_UNAVAILABLE]: status.UNAVAILABLE,
	[HttpStatus.GATEWAY_TIMEOUT]: status.DEADLINE_EXCEEDED,

	// additional built-in http exceptions
	// https://docs.nestjs.com/exception-filters#built-in-http-exceptions
	[HttpStatus.HTTP_VERSION_NOT_SUPPORTED]: status.UNAVAILABLE,
	[HttpStatus.PAYLOAD_TOO_LARGE]: status.OUT_OF_RANGE,
	[HttpStatus.UNSUPPORTED_MEDIA_TYPE]: status.CANCELLED,
	[HttpStatus.UNPROCESSABLE_ENTITY]: status.CANCELLED,
	[HttpStatus.I_AM_A_TEAPOT]: status.UNKNOWN,
	[HttpStatus.METHOD_NOT_ALLOWED]: status.CANCELLED,
	[HttpStatus.PRECONDITION_FAILED]: status.FAILED_PRECONDITION,
};
export function getStatusCode(host: ArgumentsHost, code: number) {
	return host.getType() === 'http' ? code : httpToGrpc[code];
}

function isGrpcStream(call: ServerWritableStream<unknown, unknown> | ServerUnaryCall<unknown, unknown>): call is ServerWritableStream<unknown, unknown> {
  return call.constructor.name.includes('Stream');
}

const RESULT_INDEX = 2;

export abstract class BaseCustomExceptionFilter implements ExceptionFilter {
	constructor(private statusCode: number) {}

	catch(error: Error, host: ArgumentsHost) {
		const { message } = error;
		const ctx = host.switchToHttp();
		const statusCode =
			getStatusCode(host, this.statusCode) ?? HttpStatus.INTERNAL_SERVER_ERROR;
		if (host.getType() === 'http') {
			return ctx.getResponse<FastifyReply>().status(statusCode).send({
				statusCode,
				message,
			});
		}
		const call =
			host.getArgByIndex<ServerWritableStream<unknown, unknown> | ServerUnaryCall<unknown, unknown>>(RESULT_INDEX);

		if (isGrpcStream(call)) {
			return call.emit('error', {
				code: statusCode,
				message,
			});
		}

		return throwError(() => ({
			code: statusCode,
			message,
			type: error.constructor.name,
		}));
	}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants
@moshest @single9 @raikusy @Farenheith @sadhakbj @tychota @kamilmysliwiec and others