fix(core): Change BaseExceptionFilter to only handle REST calls

[nirga]

Fixes #5958

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[ ] No

Other information

[jmcdo29]

What happens if there is no error handler? Say for TCP, if you just throw and error, how does the server handle it? What's the response to the client?

[nirga]

Basically this was the behavior so far. Because the original code called applicationRef.reply with host.getArgByIndex(1) which resolves to a context object on RPC, a client object on WS, and args on GQL. Then, for example the express implementation of .reply would simply invoke .send which will throw an exception for everything except for REST (IIUC).

[kamilmysliwiec]

@jmcdo29 TCP (and any other microservices) uses BaseRpcExceptionFilter instead so this class won't be (and shouldn't be used) by any other transport strategy (including GQL btw)

[kamilmysliwiec]

@nirga what's the reason for making BaseRpcExceptionFilter context-aware and adding an additional if condition that will run for every error thrown from the HTTP app (which has a very little impact on the performance, but still), when:

• WS apps should use BaseWsExceptionFilter instead (https://docs.nestjs.com/websockets/exception-filters#inheritance
• Microservices (including gRPC) should use BaseRpcExceptionFilter as mentioned here https://docs.nestjs.com/microservices/exception-filters#inheritance

and GraphQL shouldn't inherit from ANY class as it doesn't bring any benefit (there's no logic in the GQL exception filter, it simply re-throws an error down to the Apollo server - as I've mentioned here #5958 (comment))

[nirga]

Thanks @kamilmysliwiec! I actually didn't realize there are multiple different base filters for each protocol.
The reason for me to write this was basically to support a use case we're having now - where we have an app that communicates in both GraphQL and REST, and we don't want to interfere with the normal exception treatment of Nest, just add some logging of the fact that an error occurred. So extending BaseExceptionFilter seemed to be the right thing to do, and I guess that given the multi protocols we have in the server (and I'm not sure if it's a common use case or a really strange thing to do), we actually won't be able to use any Exception Filter.

I'd guess that then maybe it's better to rename this filter to BaseHttpExceptionFilter or something like that so it'll be clearer? And what about creating some "catch all" base exception filter that support the default behavior for all types of protocols (not necessarily just one)?

[kamilmysliwiec]

I'd guess that then maybe it's better to rename this filter to BaseHttpExceptionFilter or something like that so it'll be clearer?

I'd prefer using a BaseHttpExceptionFiltername instead as well, but now it's too late to introduce a breaking change (and force people to update their code) just to change a class name, unfortunately.

And what about creating some "catch all" base exception filter that support the default behavior for all types of protocols (not necessarily just one)?

This would require providing a separate package. Adding this to the core would create circular dependencies with @nestjs/websockets and @nestjs/microservices.

[all2pie]

Regarding #5958

I have both REST and GraphQL APIs.

Here is a solution that worked for me:

const type = host.getType();

if (type === 'http') {
  return super.catch(exception, host);
} else {
  throw exception;
}

[jtomaszewski]

I've just read the whole conversation above. And I don't understand — why exactly did this PR not get merged?

It forces us still to write this boilerplate (in apps that use both REST and GraphQL). Why won't the framework do it for us?