GraphQL errors are logged, but they shouldn't be

[JVMartin]

Is there an existing issue that is already proposing this?

• [x] I have searched the existing issues

Is your feature request related to a problem? Please describe it

✅ In a REST application, we throw HttpException errors, and they are not logged. ❌ In a GraphQL application, we throw ApolloError errors (or any other GraphQL provider errors) and they should not be logged, but they are.

Throwing any ApolloError from any resolver results in errors incorrectly being logged to console:

The logging is happening here:

https://github.com/nestjs/nest/blob/db3654a8919512e21c5bc5f512e753f3c4009a9a/packages/core/exceptions/external-exception-filter.ts#L8

It's already excluding HttpException instances, but it also needs to of course exclude ApolloError instances.

Nobody should be throwing HttpExceptions in a GraphQL project, as it results in ridiculous response JSON that contains http status codes in .extensions, even though the real http status code is 200. GraphQL is a protocol on top of http and should not have various http codes embedded in responses. Instead, we should all be throwing ApolloError instances, like UserInputError and AuthenticationError, etc. More info here.

There are other people complaining about the same issue:

https://stackoverflow.com/questions/64583235/how-do-i-prevent-nest-js-from-logging-an-exception-to-the-console

Describe the solution you'd like

Errors should not be logged when instances of ApolloError are thrown in a GraphQL application, just like errors are not logged when instances of HttpException are thrown in a REST application.

Not sure if we need to fix @nestjs/core, or if there is a way we can fix it from this module.

What is the motivation / use case for changing the behavior?

The behavior for HttpException errors is already totally correct, so it's possible to build RESTful applications with proper logging with Nest, but it isn't possible to create GraphQL applications without being forced to incorrectly use HttpException errors, which is a bad and confusing practice that should be replaced ASAP with the ability to use ApolloError errors (or any other hierarchy of errors for other non-Apollo GraphQL providers).

[akim-bow]

Totally agree. It needs to be fixed.

[akim-bow]

For now i'm able to fix it using the following code:

@Catch()
export class AllExceptionFilter implements ExceptionFilter {
    catch(error: any): any {
        if (error instanceof ApolloError) {
            throw error;
        }
    }
}

The idea is to make global exception filter which rethrow ApolloServer exceptions. I don't know why but it prevents logging. Another option is to override default logging and cancel log calls which impose ApolloServer exception logging.

[JVMartin]

To add more clarification to my logic here:

The reason we need Nest not to log these exceptions, is because the only exceptions that should be logged are exceptions that require developer intervention.

This is how Nest was designed with regards to Controllers and HttpException instances. These exceptions are NOT logged, because they are actually happy paths like validations, things the client/user did wrong, NOT the server.

We need the same exact functionality for Resolvers and ApolloError instances. These exceptions should NOT be logged, because they are actually happy paths like validations, things the client/user did wrong, NOT the server.

@kamilmysliwiec This should be a super easy huge win, yes?

[iamolegga]

One more workaround is to create separate filter for ApolloError:

import {
  ArgumentsHost,
  Catch,
  ClassProvider,
  ExceptionFilter,
  Logger,
} from '@nestjs/common';
import { APP_FILTER } from '@nestjs/core';
import { ApolloError } from 'apollo-server-express';

@Catch(ApolloError)
export class ApolloErrorsFilter implements ExceptionFilter {
  private readonly logger = new Logger(ApolloErrorsFilter.name);
  catch(err: ApolloError, _host: ArgumentsHost) {
    if (!err.extensions.code || err.extensions.code === 'INTERNAL_SERVER_ERROR')
      this.logger.error({ err }, 'Graphql internal error');
    throw err;
  }
}

export const ApolloErrorsFilterProvider: ClassProvider = {
  provide: APP_FILTER,
  useClass: ApolloErrorsFilter,
};

[julestruong]

For now i'm able to fix it using the following code:

@Catch()
export class AllExceptionFilter implements ExceptionFilter {
  catch(error: any): any {
      if (error instanceof ApolloError) {
          throw error;
      }
  }
}

The idea is to make global exception filter which rethrow ApolloServer exceptions. I don't know why but it prevents logging. Another option is to override default logging and cancel log calls which impose ApolloServer exception logging.

It would be nice to understand why doing this prevents logging :(

[BernalCarlos]

For now i'm able to fix it using the following code:

@Catch()
export class AllExceptionFilter implements ExceptionFilter {
    catch(error: any): any {
        if (error instanceof ApolloError) {
            throw error;
        }
    }
}

The idea is to make global exception filter which rethrow ApolloServer exceptions. I don't know why but it prevents logging. Another option is to override default logging and cancel log calls which impose ApolloServer exception logging.

It would be nice to understand why doing this prevents logging :(

@julestruong This approach prevents logging because what you're doing is creating an exception filter that is, in simple terms, overriding the NestJS default global exception filter, which is the one actually logging any exception that is different from HttpException (See: https://docs.nestjs.com/exception-filters).

Since you're now using a customer filter that only re-trows the exception and doesn't log anything, we're effectively preventing logging.

As a side note, I used the same approach as @iamolegga, since I wanted to override NestJS default behavior ONLY for GraphQL errors.

[kamilmysliwiec]

https://github.com/nestjs/nest/pull/14182