feat(@nestjs/graphql): base exception filter for apollo exceptions #1292

nirga · 2020-12-17T16:09:23Z

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?

[X] 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?

Exceptions are built by Apollo server engine, and because it doesn't recognizes Nest.js's exceptions, it always returns a generic HTTP 500 error, even for other errors, specifically UNAUTHORIZED.

Issue Number: #1053

What is the new behavior?

Exceptions are converted explicitly to an ApolloError, possibly to a specific exception (if exists), and then thrown for the Apollo server to catch and format.

Does this PR introduce a breaking change?

[X] Yes
[ ] No

This change might cause breakages if clients used to assume HTTP 500 is always returned. However, this will now be in-line with the rest of Apollo server errors and exceptions so it should work well with compliant clients.

Other information

jmcdo29 · 2020-12-17T16:42:45Z

lib/graphql.module.ts

@@ -46,6 +47,7 @@ import {
    GraphQLTypesLoader,
    GraphQLSchemaBuilder,
    GraphQLSchemaHost,
+    { provide: APP_FILTER, useClass: GraphQLExceptionFilter },


This will bind the filter for the entire application. If someone is using GQL and REST they are now using the GraphQLExceptionFilter for both contexts

I've added a check in GraphQLExceptionFilter to rethrow exceptions that were thrown not in GraphQL context.
If you think it make sense, I can go ahead and add a test for that specifically. I'd probably need to add a REST controller to the code-first test app, does it make sense?

To avoid introducing breaking changes, let's make this optional for now and inform users that they have to explicitly register this filter in their applications. Later (in v8), we can get back to this discussion and think about registering it automatically.

kamilmysliwiec · 2020-12-18T09:30:13Z

lib/graphql-exception.filter.ts

+@Catch(HttpException)
+export class GraphQLExceptionFilter implements ExceptionFilter {
+  catch(exception: HttpException, host: ArgumentsHost) {
+    if (host.getType<GqlContextType>() != 'graphql') {


Suggested change

if (host.getType<GqlContextType>() != 'graphql') {

if (host.getType<GqlContextType>() !== 'graphql') {

kamilmysliwiec · 2020-12-18T09:31:09Z

lib/graphql-exception.filter.ts

+import {
+  ApolloError,
+  AuthenticationError,
+  ForbiddenError,
+} from 'apollo-server-express';


We can't import from the 'apollo-server-express' as this will break Fastify based applications. Can you please import from base/core packages (not sure which one exposes these error classes)?

kamilmysliwiec · 2020-12-18T09:31:34Z

Thanks for your contribution! I've left a few comments/suggestions

faboulaws · 2020-12-23T11:10:43Z

lib/apollo-exception.filter.ts

+export class ApolloExceptionFilter implements GqlExceptionFilter {
+  catch(exception: HttpException, host: ArgumentsHost) {
+    if (host.getType<GqlContextType>() !== 'graphql') {
+      throw exception;


Is it ok to throw an exception here? From what I understand it not ok

The ExceptionFilter is always the last place that gets called before a response is sent out, it is responsible for building the response. You cannot rethrow an exception from within an ExceptionFilter.

also

Note that exceptions cannot be passed from filter to filter

Since this PR was not merged I copied this class to our own project. Our project has both a graphQL and REST routes. This line is crashing the App. I had to make the following change to make it work

if (host.getType<GqlContextType>() !== 'graphql') { const ctx = host.switchToHttp(); const response = ctx.getResponse<Response>(); const status = exception.getStatus(); response .status(status) .json({ statusCode: status, message: exception.message }); return exception; }

I am still new to NestJS. SO am I missing something here?

You're right, thanks for flagging! That's weird though, I'd expect that if an error is not handled by a certain exception filter, it will be "caught" by the next one. Not sure if that's on purpose or not but it seems that there's a difference between how ExternalExceptionsHandler is handling exceptions (where it falls back to the default behavior if the exception filter returned a falsy value), and the http exception handler in ExceptionsHandler (where it falls back to the default behavior only if there was no exception filter found). @kamilmysliwiec WDYT?

faboulaws · 2020-12-23T12:41:30Z

lib/apollo-exception.filter.ts

@@ -0,0 +1,39 @@
+import { Catch, ArgumentsHost, HttpException } from '@nestjs/common';


I am wondering if this class is needed. This feels more like an example on how to implement exception filters for GraphQL. Most people will not use this directly since custom logic such as logging etc might be required depending on use case. Maybe as a base class similar to BaseExceptionFilter.

Another approach is to define a helper to convert HTTPExceptions to Apollo Errors.

import { ApolloError, AuthenticationError, ForbiddenError } from 'apollo-server-errors' import { HttpException } from '@nestjs/common' const apolloPredefinedExceptions: Record<number, typeof ApolloError> = { 401: AuthenticationError, 403: ForbiddenError } export function convertErrorToApolloError(exception: HttpException): ApolloError { let error: ApolloError if (exception.getStatus() in apolloPredefinedExceptions) { error = new apolloPredefinedExceptions[exception.getStatus()]( exception.message ) } else { error = new ApolloError( exception.message, exception.getStatus().toString() ) } error.stack = exception.stack error.extensions['response'] = exception.getResponse() return error }

Then this helper can be used in any custom ExceptionFilter implementing GqlExceptionFilter like below.

@Catch(HttpException) export class MyCustomExceptionFilter implements GqlExceptionFilter { catch(exception: HttpException, host: ArgumentsHost) { const apolloError: ApolloError = convertErrorToApolloError(exception) this.logger.error(apolloError) return apolloError; } }

Just my humble opinion 😃. I might be wrong here. Still new to NestJS.

I think this class should be as part of the infra so you get this behavior "out of the box" without needing to worry about the internals of Nest exceptions vs. Apollo exceptions. And this is indeed the plan (see @kamilmysliwiec comment above), but I'm changing its name to be GraphQLBaseExceptionFilter for now so it'll be clear that you should inherit from it in GraphQL environments.

faboulaws · 2020-12-23T12:55:13Z

lib/apollo-exception.filter.ts

+    error.stack = exception.stack;
+    error.extensions['response'] = exception.getResponse();
+
+    throw error;


Based on the GqlExceptionFilter interface the error should be returned here.

That's on purpose though. I want ApolloServer to catch this exception so it can format it nicely in the response.

Closes nestjs#1053

faboulaws · 2021-01-18T14:55:43Z

lib/graphql-base-exception-filter.ts

+} from 'apollo-server-errors';
+import { GqlContextType } from './services';
+
+const apolloPredefinedExceptions: Record<number, typeof ApolloError> = {


Any reason why UserInputError and other ApolloErrors are not included?

incompletude · 2021-02-16T18:58:17Z

Is there a timeline for this PR? I need it really bad.

kimroen · 2021-04-16T08:47:28Z

To avoid introducing breaking changes, let's make this optional for now and inform users that they have to explicitly register this filter in their applications. Later (in v8), we can get back to this discussion and think about registering it automatically.

@kamilmysliwiec Based on this comment it looks like this was slated for a 7.x release, but due to the recent base change and merge into the nestjs:8.0.0 branch, I'm assuming this is no longer the case.

Would that be accurate? Just want to make sure I'm not just missing this landing in a 7.x release, I'm totally fine with doing workarounds until 8 🙂

kamilmysliwiec · 2021-04-16T09:32:06Z

Correct @kimroen

viztor · 2021-06-11T13:06:41Z

Correct @kimroen

May I suggest v7 to include GraphQLBaseExceptionFilter for developers to register? That could smooth the upgrade to v8 later. It is suggested in the comment that it would be done like that but it seems it's not there yet.

ScreamZ · 2021-08-04T09:43:34Z

Had issue tracking where this code belongs to in v8 now, in order to understand its behavior, for those who get interested in like me just check :

graphql/lib/utils/merge-defaults.util.ts

Line 78 in 83b4919

function wrapFormatErrorFn(options: GqlModuleOptions) {

abouroubi · 2022-01-27T18:44:07Z

Looks like this was removed from version 9.
Is there a new way to handle exception in graphql now ?

JVMartin · 2022-03-03T19:29:44Z

I'm confused... in v8, do I need to register a GraphQLExceptionFilter like this?

app.useGlobalFilters(GraphQLExceptionFilter);

I can't find this exported anywhere, so I'm not sure how to set this up. Trying to throw Apollo exceptions in v8 graphql, and they are logged as errors when they shouldn't be.

kasparszarinovs · 2022-03-16T13:54:58Z

Hi there. I was wondering if there's any movement on this, or similar solutions? Or is there any other go-to way to format GQL errors, except for setting exceptionFactory manually and having a manual formatter on GraphQL module?

Distortedlogic · 2022-03-17T13:40:05Z

Can we get some docs on the built-in nestjs patterns for handling graphql expection filtering? When I google, I dont get much thats informative or get things that are old. And I cannot find anything on doing it for subscriptions.. Trial and error to figure this out isnt preferred and means I potentially miss nestjs stuff that would simplify things.

oonsamyi · 2022-05-27T15:01:44Z

@nirga @kamilmysliwiec Any updates, please?
How to use GraphQLBaseExceptionFilter?

dijonkitchen · 2022-06-07T18:00:59Z

This might have moved to here: https://github.com/nestjs/graphql/blob/master/packages/graphql/lib/interfaces/gql-exception-filter.interface.ts

jmcdo29 reviewed Dec 17, 2020

View reviewed changes

nirga force-pushed the exceptions branch from 6f4c56a to 0e902f8 Compare December 17, 2020 17:34

kamilmysliwiec reviewed Dec 18, 2020

View reviewed changes

nirga force-pushed the exceptions branch from 0e902f8 to 5148eca Compare December 18, 2020 17:14

faboulaws reviewed Dec 23, 2020

View reviewed changes

nirga force-pushed the exceptions branch 2 times, most recently from 8a5c8b3 to 865514c Compare December 27, 2020 09:53

nirga requested a review from kamilmysliwiec December 27, 2020 09:54

nirga changed the title ~~fix(@nestjs/graphql): Convert Nest exceptions to Apollo exceptions~~ fix(@nestjs/graphql): base exception filter for converting nest exceptions to apollo exceptions Dec 27, 2020

nirga changed the title ~~fix(@nestjs/graphql): base exception filter for converting nest exceptions to apollo exceptions~~ feat(@nestjs/graphql): base exception filter for converting nest exceptions to apollo exceptions Dec 27, 2020

feat(@nestjs/graphql): base exception filter for apollo exceptions

422d392

Closes nestjs#1053

nirga changed the title ~~feat(@nestjs/graphql): base exception filter for converting nest exceptions to apollo exceptions~~ feat(@nestjs/graphql): base exception filter for apollo exceptions Dec 27, 2020

nirga force-pushed the exceptions branch from 865514c to 422d392 Compare December 27, 2020 10:40

faboulaws reviewed Jan 18, 2021

View reviewed changes

kamilmysliwiec changed the base branch from master to 8.0.0 March 19, 2021 10:28

kamilmysliwiec merged commit e98e966 into nestjs:8.0.0 Mar 19, 2021

kamilmysliwiec mentioned this pull request Mar 19, 2021

GraphQL error handling #1053

Closed

nestjs locked and limited conversation to collaborators Jun 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(@nestjs/graphql): base exception filter for apollo exceptions #1292

feat(@nestjs/graphql): base exception filter for apollo exceptions #1292

nirga commented Dec 17, 2020

jmcdo29 Dec 17, 2020

nirga Dec 17, 2020

kamilmysliwiec Dec 18, 2020

kamilmysliwiec Dec 18, 2020

kamilmysliwiec Dec 18, 2020 •

edited

Loading

kamilmysliwiec commented Dec 18, 2020

faboulaws Dec 23, 2020 •

edited

Loading

nirga Dec 27, 2020 •

edited

Loading

faboulaws Dec 23, 2020 •

edited

Loading

nirga Dec 27, 2020 •

edited

Loading

faboulaws Dec 23, 2020 •

edited

Loading

nirga Dec 27, 2020

faboulaws Jan 18, 2021

incompletude commented Feb 16, 2021

kimroen commented Apr 16, 2021

kamilmysliwiec commented Apr 16, 2021

viztor commented Jun 11, 2021 •

edited

Loading

ScreamZ commented Aug 4, 2021

abouroubi commented Jan 27, 2022

JVMartin commented Mar 3, 2022

kasparszarinovs commented Mar 16, 2022

Distortedlogic commented Mar 17, 2022

oonsamyi commented May 27, 2022

dijonkitchen commented Jun 7, 2022

	if (host.getType<GqlContextType>() != 'graphql') {
	if (host.getType<GqlContextType>() !== 'graphql') {

		@@ -0,0 +1,39 @@
		import { Catch, ArgumentsHost, HttpException } from '@nestjs/common';

feat(@nestjs/graphql): base exception filter for apollo exceptions #1292

feat(@nestjs/graphql): base exception filter for apollo exceptions #1292

Conversation

nirga commented Dec 17, 2020

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

jmcdo29 Dec 17, 2020

Choose a reason for hiding this comment

nirga Dec 17, 2020

Choose a reason for hiding this comment

kamilmysliwiec Dec 18, 2020

Choose a reason for hiding this comment

kamilmysliwiec Dec 18, 2020

Choose a reason for hiding this comment

kamilmysliwiec Dec 18, 2020 • edited Loading

Choose a reason for hiding this comment

kamilmysliwiec commented Dec 18, 2020

faboulaws Dec 23, 2020 • edited Loading

Choose a reason for hiding this comment

nirga Dec 27, 2020 • edited Loading

Choose a reason for hiding this comment

faboulaws Dec 23, 2020 • edited Loading

Choose a reason for hiding this comment

nirga Dec 27, 2020 • edited Loading

Choose a reason for hiding this comment

faboulaws Dec 23, 2020 • edited Loading

Choose a reason for hiding this comment

nirga Dec 27, 2020

Choose a reason for hiding this comment

faboulaws Jan 18, 2021

Choose a reason for hiding this comment

incompletude commented Feb 16, 2021

kimroen commented Apr 16, 2021

kamilmysliwiec commented Apr 16, 2021

viztor commented Jun 11, 2021 • edited Loading

ScreamZ commented Aug 4, 2021

abouroubi commented Jan 27, 2022

JVMartin commented Mar 3, 2022

kasparszarinovs commented Mar 16, 2022

Distortedlogic commented Mar 17, 2022

oonsamyi commented May 27, 2022

dijonkitchen commented Jun 7, 2022

kamilmysliwiec Dec 18, 2020 •

edited

Loading

faboulaws Dec 23, 2020 •

edited

Loading

nirga Dec 27, 2020 •

edited

Loading

faboulaws Dec 23, 2020 •

edited

Loading

nirga Dec 27, 2020 •

edited

Loading

faboulaws Dec 23, 2020 •

edited

Loading

viztor commented Jun 11, 2021 •

edited

Loading