Closed nirga closed 3 years ago
BaseExceptionFilter
is not supposed to be used in GraphQL applications. In fact, all GQL apps are using this https://github.com/nestjs/nest/blob/master/packages/core/exceptions/external-exception-filter.ts ExternalExceptionFilter
class by default, which (as you can see from the codebase) simply re-throws the exception. Perhaps, we should mention that in the docs to make sure nobody else will run into a similar issue.
What I wanted to change here is to make BaseExceptionFilter
be more aware of the context and not assume that it's always HTTP. Thus hybrid applications that have both REST and GQL can still inherit from it. So something like this snippet for example:
if (host.getType() == 'http') {
const ctx = host.switchToHttp();
applicationRef.reply(ctx.getResponse(), message, statusCode);
} else {
// Let the RPC / GraphQL framework handle building the response.
throw exception;
}
The problem with making the BaseExceptionFilter
be 100% context aware is that
1) the number of cases to even try to catch is pretty ridiculous for a single class. gRPC has a different error notation than REST which has a different error notation than other RPC transports which differs from GQL which differs from WS (you get the idea)
2) for the case of GQL it would require bringing in the GqlExectuionContext
which would in turn create a circular dependency between the packages
3) any slight change to any of the error formats would require a change on our side as well to keep the "expected" return from changing, which could mean a lot more code and upkeep on our side
Your suggestion of throwing the exception again is actually what's currently happening, as Kamil mentioned with the ExternalExceptionFilter
and it's the reason for the INTERNAL_SERVER_ERROR
because the Apollo server doesn't understand Nest's HttpException
class as a recognized Error.
Not exactly, this is a different one than the GraphQL error we were also discussing. Here, if there's an exception in a graph QL context, the BaseExceptionFilter
will fail which will cause the actual exception returned to be the one from BaseExceptionFilter
and not the one that was originally thrown.
What I'm suggesting is just to make sure that host is of type HTTP before doing host.getArgByIndex(1)
since this will fail in a GQL or a GRPC context.
Here, if there's an exception in a graph QL context, the BaseExceptionFilter will fail which will cause the actual exception returned to be the one from BaseExceptionFilter and not the one that was originally thrown.
As I've said here https://github.com/nestjs/nest/issues/5958#issuecomment-747483125, BaseExceptionFilter
shouldn't be used in GraphQL applications so there's literally no reason to make it context-aware. GraphQL apps are using ExternalExceptionFilter
by default (instead of the BaseExceptionFilter
), and this class isn't even publicly exposed (exported from the root of the package) because it simply re-throws the error (there's no logic inside). Let's continue here https://github.com/nestjs/nest/pull/5972
Bug Report
Current behavior
When throwing an exception within a GraphQL resolver, and then using a global Exception filter that extends
BaseExceptionFilter
, there's an exception insideBaseExceptionFilter
.Input Code
CustomExceptionFilter
And then throw an error inside some GraphQL resolver:
Expected behavior
The original error should have been the one returned and not the exception from within
BaseExceptionFilter
.Possible Solution
When
BaseExceptionFilter
handles unknown exceptions, it useshost.getArgByIndex(1)
to build the response. However, this may be null for GraphQL queries as their execution context is different.Environment