From 1b72d901a508b8faf246fa89bde2f7511c5dcec5 Mon Sep 17 00:00:00 2001 From: Marie <51697796+ijreilly@users.noreply.github.com> Date: Mon, 30 Jun 2025 16:01:48 +0200 Subject: [PATCH] Improve RestApiExceptionFilter (#12967) RestApiExceptionFilter is used as an exception filter for the core controller which is used for crud operations on our objects (equivalent of our dynamic queries findManyPeople etc. on the graphql API). Exceptions were leading a 400 / BadRequestException response status which can be confusing to users. By default we should actually throw a 500 if the error was not handled priorily, but we have not implemented input validation for the REST api so we fear to be flooded with errors that should not be 500 but 400 due to user inputs. A solution should be brought [with this ticket](https://github.com/twentyhq/core-team-issues/issues/1027) but it has not been prioritized yet. --- .../handlers/rest-api-create-many.handler.ts | 8 +++- .../handlers/rest-api-create-one.handler.ts | 8 +++- .../handlers/rest-api-update-one.handler.ts | 8 +++- .../api/rest/rest-api-exception.filter.ts | 15 +++++-- .../http-exception-handler.service.ts | 42 ++++++++++++++++--- .../utils/global-exception-handler.util.ts | 4 +- 6 files changed, 69 insertions(+), 16 deletions(-) diff --git a/packages/twenty-server/src/engine/api/rest/core/handlers/rest-api-create-many.handler.ts b/packages/twenty-server/src/engine/api/rest/core/handlers/rest-api-create-many.handler.ts index dc5b1de2d04..4a0c98cda7f 100644 --- a/packages/twenty-server/src/engine/api/rest/core/handlers/rest-api-create-many.handler.ts +++ b/packages/twenty-server/src/engine/api/rest/core/handlers/rest-api-create-many.handler.ts @@ -1,4 +1,8 @@ -import { BadRequestException, Injectable } from '@nestjs/common'; +import { + BadRequestException, + Injectable, + InternalServerErrorException, +} from '@nestjs/common'; import { Request } from 'express'; import { isDefined } from 'twenty-shared/utils'; @@ -72,7 +76,7 @@ export class RestApiCreateManyHandler extends RestApiBaseHandler { }); if (records.length !== body.length) { - throw new Error( + throw new InternalServerErrorException( `Error when creating records. ${body.length - records.length} records are missing after creation.`, ); } diff --git a/packages/twenty-server/src/engine/api/rest/core/handlers/rest-api-create-one.handler.ts b/packages/twenty-server/src/engine/api/rest/core/handlers/rest-api-create-one.handler.ts index 1d99e49ac1f..d69db19dcf8 100644 --- a/packages/twenty-server/src/engine/api/rest/core/handlers/rest-api-create-one.handler.ts +++ b/packages/twenty-server/src/engine/api/rest/core/handlers/rest-api-create-one.handler.ts @@ -1,4 +1,8 @@ -import { BadRequestException, Injectable } from '@nestjs/common'; +import { + BadRequestException, + Injectable, + InternalServerErrorException, +} from '@nestjs/common'; import { Request } from 'express'; import { isDefined } from 'twenty-shared/utils'; @@ -57,7 +61,7 @@ export class RestApiCreateOneHandler extends RestApiBaseHandler { const record = records[0]; if (!isDefined(record)) { - throw new Error('Created record not found'); + throw new InternalServerErrorException('Created record not found'); } return this.formatResult({ diff --git a/packages/twenty-server/src/engine/api/rest/core/handlers/rest-api-update-one.handler.ts b/packages/twenty-server/src/engine/api/rest/core/handlers/rest-api-update-one.handler.ts index 4d236a2c94b..47cef77e897 100644 --- a/packages/twenty-server/src/engine/api/rest/core/handlers/rest-api-update-one.handler.ts +++ b/packages/twenty-server/src/engine/api/rest/core/handlers/rest-api-update-one.handler.ts @@ -1,4 +1,8 @@ -import { BadRequestException, Injectable } from '@nestjs/common'; +import { + BadRequestException, + Injectable, + InternalServerErrorException, +} from '@nestjs/common'; import { Request } from 'express'; import { isDefined } from 'twenty-shared/utils'; @@ -54,7 +58,7 @@ export class RestApiUpdateOneHandler extends RestApiBaseHandler { const record = records[0]; if (!isDefined(record)) { - throw new Error('Updated record not found'); + throw new InternalServerErrorException('Updated record not found'); } return this.formatResult({ diff --git a/packages/twenty-server/src/engine/api/rest/rest-api-exception.filter.ts b/packages/twenty-server/src/engine/api/rest/rest-api-exception.filter.ts index a94dfc57749..b7df941d85c 100644 --- a/packages/twenty-server/src/engine/api/rest/rest-api-exception.filter.ts +++ b/packages/twenty-server/src/engine/api/rest/rest-api-exception.filter.ts @@ -1,9 +1,13 @@ -import { ArgumentsHost, Catch, ExceptionFilter } from '@nestjs/common'; +import { + ArgumentsHost, + Catch, + ExceptionFilter, + HttpException, +} from '@nestjs/common'; import { Response } from 'express'; import { HttpExceptionHandlerService } from 'src/engine/core-modules/exception-handler/http-exception-handler.service'; -import { CustomException } from 'src/utils/custom-exception'; @Catch() export class RestApiExceptionFilter implements ExceptionFilter { @@ -15,10 +19,13 @@ export class RestApiExceptionFilter implements ExceptionFilter { const ctx = host.switchToHttp(); const response = ctx.getResponse(); + const statusCode = + exception instanceof HttpException ? exception.getStatus() : 400; // should actually default to 500 but we dont have input validation yet and dont want to be flooded with errors from input https://github.com/twentyhq/core-team-issues/issues/1027 + return this.httpExceptionHandlerService.handleError( - exception as CustomException, + exception as Error | HttpException, response, - 400, + statusCode, ); } } diff --git a/packages/twenty-server/src/engine/core-modules/exception-handler/http-exception-handler.service.ts b/packages/twenty-server/src/engine/core-modules/exception-handler/http-exception-handler.service.ts index fc118ca5e6b..913b3c38599 100644 --- a/packages/twenty-server/src/engine/core-modules/exception-handler/http-exception-handler.service.ts +++ b/packages/twenty-server/src/engine/core-modules/exception-handler/http-exception-handler.service.ts @@ -1,4 +1,10 @@ -import { BadRequestException, Inject, Injectable, Scope } from '@nestjs/common'; +import { + BadRequestException, + HttpException, + Inject, + Injectable, + Scope, +} from '@nestjs/common'; import { REQUEST } from '@nestjs/core'; import { Response } from 'express'; @@ -16,6 +22,34 @@ interface RequestAndParams { params: any; } +const getErrorNameFromStatusCode = (statusCode: number) => { + switch (statusCode) { + case 400: + return 'BadRequestException'; + case 401: + return 'UnauthorizedException'; + case 403: + return 'ForbiddenException'; + case 404: + return 'NotFoundException'; + case 405: + return 'MethodNotAllowedException'; + case 409: + return 'ConflictException'; + case 422: + return 'UnprocessableEntityException'; + case 500: + return 'InternalServerErrorException'; + default: { + if (statusCode >= 500) { + return 'InternalServerErrorException'; + } + + return 'BadRequestException'; + } + } +}; + @Injectable({ scope: Scope.REQUEST }) export class HttpExceptionHandlerService { constructor( @@ -25,15 +59,13 @@ export class HttpExceptionHandlerService { ) {} handleError = ( - exception: Error, - // eslint-disable-next-line @typescript-eslint/no-explicit-any + exception: Error | HttpException, // eslint-disable-next-line @typescript-eslint/no-explicit-any response: Response>, errorCode?: number, user?: ExceptionHandlerUser, workspace?: ExceptionHandlerWorkspace, // eslint-disable-next-line @typescript-eslint/no-explicit-any - // eslint-disable-next-line @typescript-eslint/no-explicit-any ): Response> | undefined => { const params = this.request?.params; @@ -62,7 +94,7 @@ export class HttpExceptionHandlerService { return response.status(statusCode).send({ statusCode, - error: exception.name || 'BadRequestException', + error: exception.name ?? getErrorNameFromStatusCode(statusCode), messages: [exception?.message], }); }; diff --git a/packages/twenty-server/src/engine/utils/global-exception-handler.util.ts b/packages/twenty-server/src/engine/utils/global-exception-handler.util.ts index 24524a0cf3f..cf094d5063e 100644 --- a/packages/twenty-server/src/engine/utils/global-exception-handler.util.ts +++ b/packages/twenty-server/src/engine/utils/global-exception-handler.util.ts @@ -86,7 +86,9 @@ export const shouldCaptureException = ( return true; }; -export const handleException = ({ +export const handleException = < + T extends Error | CustomException | HttpException, +>({ exception, exceptionHandlerService, user,