From dbbe17bf2b45a2f9efd13bf78466e16ddd1a0e16 Mon Sep 17 00:00:00 2001 From: martmull Date: Tue, 14 May 2024 13:21:55 +0200 Subject: [PATCH] Return graphql errors when exists (#5389) - throw badRequest with graphql error messages when graphql request fails - clean some code Before image After ![image](https://github.com/twentyhq/twenty/assets/29927851/6bbaaf7c-1244-473d-9ae5-4fefc6a1b994) --- .../workspace-query-runner.service.ts | 1 + .../api-rest-query-builder.factory.ts | 6 +- .../factories/create-variables.factory.ts | 2 + .../factories/update-variables.factory.ts | 2 + .../engine/api/rest/api-rest.controller.ts | 18 +++-- .../api/rest/api-rest.controller.utils.ts | 18 +---- .../src/engine/api/rest/api-rest.service.ts | 79 ++++++++----------- .../api/rest/metadata-rest.controller.ts | 18 +++-- .../api/rest/types/api-rest-response.type.ts | 5 -- 9 files changed, 68 insertions(+), 81 deletions(-) delete mode 100644 packages/twenty-server/src/engine/api/rest/types/api-rest-response.type.ts diff --git a/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/workspace-query-runner.service.ts b/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/workspace-query-runner.service.ts index 166cb9081eb8..0aa209253b6c 100644 --- a/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/workspace-query-runner.service.ts +++ b/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/workspace-query-runner.service.ts @@ -483,6 +483,7 @@ export class WorkspaceQueryRunnerService { const { workspaceId, userId, objectMetadataItem } = options; assertMutationNotOnRemoteObject(objectMetadataItem); + assertIsValidUuid(args.id); const query = await this.workspaceQueryBuilderFactory.deleteOne( args, diff --git a/packages/twenty-server/src/engine/api/rest/api-rest-query-builder/api-rest-query-builder.factory.ts b/packages/twenty-server/src/engine/api/rest/api-rest-query-builder/api-rest-query-builder.factory.ts index e6df92853c71..3f968e02d39e 100644 --- a/packages/twenty-server/src/engine/api/rest/api-rest-query-builder/api-rest-query-builder.factory.ts +++ b/packages/twenty-server/src/engine/api/rest/api-rest-query-builder/api-rest-query-builder.factory.ts @@ -98,7 +98,7 @@ export class ApiRestQueryBuilderFactory { }; } - async create(request): Promise { + async create(request: Request): Promise { const objectMetadata = await this.getObjectMetadata(request); const depth = computeDepth(request); @@ -109,7 +109,7 @@ export class ApiRestQueryBuilderFactory { }; } - async update(request): Promise { + async update(request: Request): Promise { const objectMetadata = await this.getObjectMetadata(request); const depth = computeDepth(request); @@ -128,7 +128,7 @@ export class ApiRestQueryBuilderFactory { }; } - async get(request): Promise { + async get(request: Request): Promise { const objectMetadata = await this.getObjectMetadata(request); const depth = computeDepth(request); diff --git a/packages/twenty-server/src/engine/api/rest/api-rest-query-builder/factories/create-variables.factory.ts b/packages/twenty-server/src/engine/api/rest/api-rest-query-builder/factories/create-variables.factory.ts index 190d50fd9b3c..9b307e8f8b1f 100644 --- a/packages/twenty-server/src/engine/api/rest/api-rest-query-builder/factories/create-variables.factory.ts +++ b/packages/twenty-server/src/engine/api/rest/api-rest-query-builder/factories/create-variables.factory.ts @@ -1,5 +1,7 @@ import { Injectable } from '@nestjs/common'; +import { Request } from 'express'; + import { ApiRestQueryVariables } from 'src/engine/api/rest/types/api-rest-query-variables.type'; @Injectable() diff --git a/packages/twenty-server/src/engine/api/rest/api-rest-query-builder/factories/update-variables.factory.ts b/packages/twenty-server/src/engine/api/rest/api-rest-query-builder/factories/update-variables.factory.ts index 2d1bca67ef5e..e5231a6663e6 100644 --- a/packages/twenty-server/src/engine/api/rest/api-rest-query-builder/factories/update-variables.factory.ts +++ b/packages/twenty-server/src/engine/api/rest/api-rest-query-builder/factories/update-variables.factory.ts @@ -1,5 +1,7 @@ import { Injectable } from '@nestjs/common'; +import { Request } from 'express'; + import { ApiRestQueryVariables } from 'src/engine/api/rest/types/api-rest-query-variables.type'; @Injectable() diff --git a/packages/twenty-server/src/engine/api/rest/api-rest.controller.ts b/packages/twenty-server/src/engine/api/rest/api-rest.controller.ts index f1eee2cc3066..921cd884178c 100644 --- a/packages/twenty-server/src/engine/api/rest/api-rest.controller.ts +++ b/packages/twenty-server/src/engine/api/rest/api-rest.controller.ts @@ -3,7 +3,7 @@ import { Controller, Delete, Get, Post, Put, Req, Res } from '@nestjs/common'; import { Request, Response } from 'express'; import { ApiRestService } from 'src/engine/api/rest/api-rest.service'; -import { handleResult } from 'src/engine/api/rest/api-rest.controller.utils'; +import { cleanGraphQLResponse } from 'src/engine/api/rest/api-rest.controller.utils'; @Controller('rest/*') export class ApiRestController { @@ -11,21 +11,29 @@ export class ApiRestController { @Get() async handleApiGet(@Req() request: Request, @Res() res: Response) { - handleResult(res, await this.apiRestService.get(request)); + const result = await this.apiRestService.get(request); + + res.send(cleanGraphQLResponse(result.data)); } @Delete() async handleApiDelete(@Req() request: Request, @Res() res: Response) { - handleResult(res, await this.apiRestService.delete(request)); + const result = await this.apiRestService.delete(request); + + res.send(cleanGraphQLResponse(result.data)); } @Post() async handleApiPost(@Req() request: Request, @Res() res: Response) { - handleResult(res, await this.apiRestService.create(request)); + const result = await this.apiRestService.create(request); + + res.send(cleanGraphQLResponse(result.data)); } @Put() async handleApiPut(@Req() request: Request, @Res() res: Response) { - handleResult(res, await this.apiRestService.update(request)); + const result = await this.apiRestService.update(request); + + res.send(cleanGraphQLResponse(result.data)); } } diff --git a/packages/twenty-server/src/engine/api/rest/api-rest.controller.utils.ts b/packages/twenty-server/src/engine/api/rest/api-rest.controller.utils.ts index a45efb8a5fd6..9005d523eb07 100644 --- a/packages/twenty-server/src/engine/api/rest/api-rest.controller.utils.ts +++ b/packages/twenty-server/src/engine/api/rest/api-rest.controller.utils.ts @@ -1,12 +1,8 @@ -import { Response } from 'express'; - -import { ApiRestResponse } from 'src/engine/api/rest/types/api-rest-response.type'; - // https://gist.github.com/ManUtopiK/469aec75b655d6a4d912aeb3b75af3c9 -export const cleanGraphQLResponse = (input) => { +export const cleanGraphQLResponse = (input: any) => { if (!input) return null; const output = {}; - const isObject = (obj) => { + const isObject = (obj: any) => { return obj !== null && typeof obj === 'object' && !Array.isArray(obj); }; @@ -24,13 +20,3 @@ export const cleanGraphQLResponse = (input) => { return output; }; - -export const handleResult = (res: Response, result: ApiRestResponse) => { - if (result.data.error) { - res - .status(result.data.status || 400) - .send({ error: `${result.data.error}` }); - } else { - res.send(cleanGraphQLResponse(result.data)); - } -}; diff --git a/packages/twenty-server/src/engine/api/rest/api-rest.service.ts b/packages/twenty-server/src/engine/api/rest/api-rest.service.ts index 4db2893f3082..3631f4dcdac0 100644 --- a/packages/twenty-server/src/engine/api/rest/api-rest.service.ts +++ b/packages/twenty-server/src/engine/api/rest/api-rest.service.ts @@ -1,87 +1,72 @@ -import { Injectable } from '@nestjs/common'; +import { BadRequestException, Injectable } from '@nestjs/common'; import { HttpService } from '@nestjs/axios'; import { Request } from 'express'; +import { AxiosResponse } from 'axios'; import { EnvironmentService } from 'src/engine/integrations/environment/environment.service'; import { ApiRestQueryBuilderFactory } from 'src/engine/api/rest/api-rest-query-builder/api-rest-query-builder.factory'; -import { TokenService } from 'src/engine/core-modules/auth/services/token.service'; -import { ApiRestResponse } from 'src/engine/api/rest/types/api-rest-response.type'; import { ApiRestQuery } from 'src/engine/api/rest/types/api-rest-query.type'; import { getServerUrl } from 'src/utils/get-server-url'; @Injectable() export class ApiRestService { constructor( - private readonly tokenService: TokenService, private readonly environmentService: EnvironmentService, private readonly apiRestQueryBuilderFactory: ApiRestQueryBuilderFactory, private readonly httpService: HttpService, ) {} - async callGraphql( - request: Request, - data: ApiRestQuery, - ): Promise { + async callGraphql(request: Request, data: ApiRestQuery) { const baseUrl = getServerUrl( request, this.environmentService.get('SERVER_URL'), ); + let response: AxiosResponse; try { - return await this.httpService.axiosRef.post(`${baseUrl}/graphql`, data, { - headers: { - 'Content-Type': 'application/json', - Authorization: request.headers.authorization, + response = await this.httpService.axiosRef.post( + `${baseUrl}/graphql`, + data, + { + headers: { + 'Content-Type': 'application/json', + Authorization: request.headers.authorization, + }, }, - }); + ); } catch (err) { - return { - data: { - error: `${err}. Please check your query.`, - status: err.response.status, - }, - }; + throw new BadRequestException(err.response.data); + } + + if (response.data.errors?.length) { + throw new BadRequestException(response.data); } + + return response; } - async get(request: Request): Promise { - try { - const data = await this.apiRestQueryBuilderFactory.get(request); + async get(request: Request) { + const data = await this.apiRestQueryBuilderFactory.get(request); - return await this.callGraphql(request, data); - } catch (err) { - return { data: { error: err, status: err.status } }; - } + return await this.callGraphql(request, data); } - async delete(request: Request): Promise { - try { - const data = await this.apiRestQueryBuilderFactory.delete(request); + async delete(request: Request) { + const data = await this.apiRestQueryBuilderFactory.delete(request); - return await this.callGraphql(request, data); - } catch (err) { - return { data: { error: err, status: err.status } }; - } + return await this.callGraphql(request, data); } - async create(request: Request): Promise { - try { - const data = await this.apiRestQueryBuilderFactory.create(request); + async create(request: Request) { + const data = await this.apiRestQueryBuilderFactory.create(request); - return await this.callGraphql(request, data); - } catch (err) { - return { data: { error: err, status: err.status } }; - } + return await this.callGraphql(request, data); } - async update(request: Request): Promise { - try { - const data = await this.apiRestQueryBuilderFactory.update(request); + async update(request: Request) { + const data = await this.apiRestQueryBuilderFactory.update(request); - return await this.callGraphql(request, data); - } catch (err) { - return { data: { error: err, status: err.status } }; - } + return await this.callGraphql(request, data); } } diff --git a/packages/twenty-server/src/engine/api/rest/metadata-rest.controller.ts b/packages/twenty-server/src/engine/api/rest/metadata-rest.controller.ts index ea9728b86fc6..98b3b088dc27 100644 --- a/packages/twenty-server/src/engine/api/rest/metadata-rest.controller.ts +++ b/packages/twenty-server/src/engine/api/rest/metadata-rest.controller.ts @@ -2,8 +2,8 @@ import { Controller, Get, Delete, Post, Put, Req, Res } from '@nestjs/common'; import { Request, Response } from 'express'; -import { handleResult } from 'src/engine/api/rest/api-rest.controller.utils'; import { ApiRestMetadataService } from 'src/engine/api/rest/metadata-rest.service'; +import { cleanGraphQLResponse } from 'src/engine/api/rest/api-rest.controller.utils'; @Controller('rest/metadata/*') export class ApiRestMetadataController { @@ -11,21 +11,29 @@ export class ApiRestMetadataController { @Get() async handleApiGet(@Req() request: Request, @Res() res: Response) { - handleResult(res, await this.apiRestService.get(request)); + const result = await this.apiRestService.get(request); + + res.send(cleanGraphQLResponse(result.data)); } @Delete() async handleApiDelete(@Req() request: Request, @Res() res: Response) { - handleResult(res, await this.apiRestService.delete(request)); + const result = await this.apiRestService.delete(request); + + res.send(cleanGraphQLResponse(result.data)); } @Post() async handleApiPost(@Req() request: Request, @Res() res: Response) { - handleResult(res, await this.apiRestService.create(request)); + const result = await this.apiRestService.create(request); + + res.send(cleanGraphQLResponse(result.data)); } @Put() async handleApiPut(@Req() request: Request, @Res() res: Response) { - handleResult(res, await this.apiRestService.update(request)); + const result = await this.apiRestService.update(request); + + res.send(cleanGraphQLResponse(result.data)); } } diff --git a/packages/twenty-server/src/engine/api/rest/types/api-rest-response.type.ts b/packages/twenty-server/src/engine/api/rest/types/api-rest-response.type.ts deleted file mode 100644 index 5f4407b00d3e..000000000000 --- a/packages/twenty-server/src/engine/api/rest/types/api-rest-response.type.ts +++ /dev/null @@ -1,5 +0,0 @@ -import { HttpException } from '@nestjs/common'; - -export type ApiRestResponse = { - data: { error?: HttpException | string; status?: number }; -};