ChilliCream / graphql-platform

Welcome to the home of the Hot Chocolate GraphQL server for .NET, the Strawberry Shake GraphQL client for .NET and Banana Cake Pop the awesome Monaco based GraphQL IDE.
https://chillicream.com
MIT License
5.23k stars 745 forks source link

Add IErrorFilterAsync #4826

Closed iSeiryu closed 2 years ago

iSeiryu commented 2 years ago

Is your feature request related to a problem?

Sometimes we need to run additional logic inside IError OnError(IError error); method and that logic may involve IO meaning it will need to be async.

The solution you'd like

Either add IErrorFilterAsync interface or Task<IError> OnErrorAsync(IError error); inside the existing IErrorFilter interface.

Product

Hot Chocolate

michaelstaib commented 2 years ago

We designed the error filters to explicitly have no IO interaction in order to not slow down execution. If you need this for logging IErrorFilter is actually the wrong thing to use. Look at something like IDiagnosticEventListeners for instance or the new open tracing integration.

The error filter is meant to enrich the error object with extra data from custom exceptions.

I am closing this issue since we do not intend to have async logic in this component.

EvilVir commented 1 year ago

You will hate me for that, but I actually would like to ask you to reconsider this one :)

You said The error filter is meant to enrich the error object with extra data from custom exceptions. and that's exactly what we're facing right now and can't use IErrorFilter for. We're throwing UserNotFoundException from one of our microservices' business layer and that exception is carrying id of said user. Then, for API layer we would like to enhance that exception with user's first and last names but in order to do that we need to either fetch data from database or reach out to remote microservice via HTTP call.

Of course straightforward approach is to just try-catch in the resolver/mutation and do all the translation there but that's not an universal approach. Other approach we consider is to write custom middleware, which on the otherhand looks like overkill and replacement of already existing feature. One can ask why we don't put that demographic data in the exception at the place where it's throwed and the answer to that is that it's only meaningful and needed for API/UI layer, any other components that uses that service doesn't care about that human-facing info so there's no point in doing extra I/O in the BLL just for having nice popup in UI use case.

Having IErrorFilterAsync handlers would fit nicelly there.