GraphQL doesn't work well with BaseExceptionFilter

[nirga]

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 inside BaseExceptionFilter.

{
  "errors": [
    {
      "message": "response.status is not a function",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "getUser"
      ],
      "extensions": {
        "code": "INTERNAL_SERVER_ERROR",
        "exception": {
          "stacktrace": [
            "TypeError: response.status is not a function",
            "    at ExpressAdapter.reply (/Users/nirga/tutorials/nest-test/node_modules/@nestjs/platform-express/adapters/express-adapter.js:19:22)",
            "    at CustomExceptionFilter.catch (/Users/nirga/tutorials/nest-test/node_modules/@nestjs/core/exceptions/base-exception-filter.js:26:24)",
            "    at CustomExceptionFilter.catch (/Users/nirga/tutorials/nest-test/dist/src/graphql-exception.filter.js:19:20)",
            "    at ExternalExceptionsHandler.invokeCustomFilters (/Users/nirga/tutorials/nest-test/node_modules/@nestjs/core/exceptions/external-exceptions-handler.js:34:32)",
            "    at ExternalExceptionsHandler.next (/Users/nirga/tutorials/nest-test/node_modules/@nestjs/core/exceptions/external-exceptions-handler.js:13:29)",
            "    at /Users/nirga/tutorials/nest-test/node_modules/@nestjs/core/helpers/external-proxy.js:14:42",
            "    at processTicksAndRejections (internal/process/task_queues.js:93:5)"
          ]
        }
      }
    }
  ],
  "data": {
    "getUser": null
  }
}

Input Code

CustomExceptionFilter

@Catch(HttpException)
export class CustomExceptionFilter extends BaseExceptionFilter {
  catch(exception: HttpException, host: ArgumentsHost) {
    super.catch(exception, host);
  }
}

And then throw an error inside some GraphQL resolver:

@Resolver('User')
export class UserResolver {
  @Query()
  async getUser() {
    throw new UnauthorizedException();
  }
}

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 uses host.getArgByIndex(1) to build the response. However, this may be null for GraphQL queries as their execution context is different.

Environment

Nest version: 7.5.1

 
For Tooling issues:
- Node version: 14  
- Platform: Mac 

Others:

[kamilmysliwiec]

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.

[nirga]

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;
}

[jmcdo29]

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.

[nirga]

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.

[kamilmysliwiec]

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 #5958 (comment), 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 #5972