graphql
graphql copied to clipboard
GraphQL errors are logged, but they shouldn't be
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 HttpException
s 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).
Totally agree. It needs to be fixed.
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.
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?
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,
};
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 :(