nestjs / nest

A progressive Node.js framework for building efficient, scalable, and enterprise-grade server-side applications with TypeScript/JavaScript 🚀
https://nestjs.com
MIT License
67.28k stars 7.59k forks source link

deserialize RpcException #5694

Closed TrejGun closed 3 years ago

TrejGun commented 3 years ago

Feature Request

Is your feature request related to a problem? Please describe.

RpcException class is lost when passing exception around

Describe the solution you'd like

Make ProxyClient.send method deserialize RpcException from json object. Make deserialization extensible so developer can pass more than just error message (error code for example)

Teachability, Documentation, Adoption, Migration Strategy

for now I have to manually re-throw exception

return this.client.send(MESSAGE, { payload })
      .toPromise()
      .catch(error => {
        throw new HttpException(error.message, 500)
      })

or handle it in ExceptionFilter

export class ExtendedRpcExceptionsFilter extends BaseExceptionFilter implements ExceptionFilter {
  catch(exception: unknown, host: ArgumentsHost): void {
    if (exception instanceof HttpException) {
      return super.catch(exception, host);
    } else { // pure json object
      // RpcException
      // @ts-ignore
      return super.catch(new HttpException(exception.message, 500), host);
    }
  }
}

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

1 I'm unable to filter incoming exceptions using @Catch decorator 2 current solution is not extensible, I can't pass error code together with error message 3 first workaround introduces huge code overhead 4 if I have custom exceptions my second workaround does not really work

jmcdo29 commented 3 years ago

@kamilmysliwiec this is something I think I can work on as an enhancement. After looking through the code, I think we can make client-proxy's class have a public deserializeError method that will allow devs to customize the error handling logic in a central location so that errors that come back turn into proper RcpExceptions instead of the base Error instance. Let me know what you think.

kamilmysliwiec commented 3 years ago

@jmcdo29 this sounds quite useful. Would you be able to create a PR with an example public API?

jmcdo29 commented 3 years ago

Absolutely! I'll try to bang it out this weekend

jmcdo29 commented 3 years ago

So, I'm thinking about how this can be done. Currently, the proper class is being created based on the transport type of ClientsModule.register. This would mean that it's not possible to change the class created to make use of OO practices. However, what if we add a customClass to the options and make a check if customClass exists? If so, then pass the options to this custom class, as we already are for the built in classes. Then, this error deserializer can be taken advantage of not only in the ClientProxy's createObserver method, but in other methods like emit and send.

TrejGun commented 3 years ago

I personally dont need yet but what if i want to throw exceptions of more than one type how can i specify which class is that? I prefer to serrialize class name in json object of message and try to create class in switch case in deserealizer func so all absent classes will end up in default clouse. unless it is RpcException which is handled by framework itself

kamilmysliwiec commented 3 years ago

@jmcdo29 what about adding a new configuration property (to the microservice options) named, for example, transformError which would be evaluated right before the observer.error(error) call allowing any object transformation?

jmcdo29 commented 3 years ago

@kamilmysliwiec that would be an option, it crossed my mind as well.

My thought with allowing for a custom class is that it would allow for extension of all the microservice client proxies and not just the options that we allow for customization with. In the ClientProxyFactory we're creating each ClientProxy type based on the transport option. We could do the same with customClass and do

export class ClientProxyFactory {
  public static create(
    clientOptions: { transport: Transport.GRPC } & ClientOptions,
  ): ClientGrpcProxy;
  public static create(clientOptions: ClientOptions): ClientProxy & Closeable;
  public static create(clientOptions: ClientOptions): ClientProxy & Closeable {
    const { transport, options, customClass } = clientOptions;
    if (customClass) {
      return new customClass(options);
    }
    switch (transport) {
      case Transport.REDIS:
        return new ClientRedis(options as RedisOptions['options']);
      case Transport.NATS:
        return new ClientNats(options as NatsOptions['options']);
      case Transport.MQTT:
        return new ClientMqtt(options as MqttOptions['options']);
      case Transport.GRPC:
        return new ClientGrpcProxy(options as GrpcOptions['options']);
      case Transport.RMQ:
        return new ClientRMQ(options as RmqOptions['options']);
      case Transport.KAFKA:
        return new ClientKafka(options as KafkaOptions['options']);
      default:
        return new ClientTCP(options as TcpClientOptions['options']);
    }
  }
}

Which would make sure we don't impact any current functionality, but also allows for full customization of not just ClientProxy, but also our other clients like ClientRmq and ClientKafka. This would in theory allow for even greater extensibility of Nest's microservice ecosystem

kamilmysliwiec commented 3 years ago

Looks good too @jmcdo29! Would you like to create a PR with this feature?

jmcdo29 commented 3 years ago

Absolutely! I'll get going on that sooon

kamilmysliwiec commented 3 years ago

Let's track this here https://github.com/nestjs/nest/pull/5710