twentyhq / twenty

Building a modern alternative to Salesforce, powered by the community.
https://twenty.com
GNU Affero General Public License v3.0
15.7k stars 1.7k forks source link

400 instead of 500 for query read timeout #6300

Open FelixMalfait opened 1 month ago

FelixMalfait commented 1 month ago

A client reported that they ran the following request:

{
  "url": "https://api.twenty.com/rest/companies?filter=updatedAt%5Bgte%5D%3A%222021-06-01T00%3A00%3A00.000000Z%22",
  "headers": {
    "User-Agent": "python-requests/2.32.3",
    "Accept-Encoding": "gzip, deflate",
    "Accept": "*/*",
    "Connection": "keep-alive",
    "Authorization": "Bearer ****"
  },
  "http_method": "GET",
  "body": ""
}

And got the following error:

{
  "status": 400,
  "body": {
    "statusCode": 400,
    "message": "Query read timeout",
    "error": "Bad Request"
  },
  "headers": {
    "Date": "Wed, 10 Jul 2024 05:26:42 GMT",
    "Content-Type": "application/json; charset=utf-8",
    "Content-Length": "71",
    "Connection": "keep-alive",
    "X-Powered-By": "Express",
    "Access-Control-Allow-Origin": "*",
    "Access-Control-Allow-Methods": "GET,HEAD,PUT,PATCH,POST,DELETE",
    "Access-Control-Allow-Headers": "Origin, X-Requested-With, Content-Type, Accept",
    "ETag": "W/\"47-XdAOEIsvUz8giAnu75HoLvsXodw\"",
    "CF-Cache-Status": "DYNAMIC",
    "Server": "cloudflare",
    "CF-RAY": "8a0e2394bee75309-SLC",
    "alt-svc": "h3=\":443\"; ma=86400"
  }
}

This didn't appear in Sentry. This should have been a 500 not a 400 as the request is correct. Where could this come from @charlesBochet?

greptile-apps[bot] commented 1 month ago

To resolve the issue of returning a 400 status code instead of a 500 for query read timeouts, follow these steps:

  1. Update RestApiException:

    • File: /packages/twenty-server/src/engine/api/rest/errors/RestApiException.ts
    • Modify the constructor to handle query read timeouts as a 500 error.
      
      import { InternalServerErrorException } from '@nestjs/common';

    export class RestApiException extends InternalServerErrorException { constructor(errors: BaseGraphQLError[]) { super({ statusCode: 500, message: errors.length === 1 ? formatMessage(errors[0]) : JSON.stringify(errors.map((error) => formatMessage(error))), error: 'Internal Server Error', }); } }

  2. Update OpenAPI Utils:

    • File: /packages/twenty-server/src/engine/core-modules/open-api/utils/get-error-responses.utils.ts
    • Add a function to handle 500 error responses.
      export const get500ErrorResponses = (): OpenAPIV3_1.ResponseObject => {
      return {
      description: 'Internal Server Error',
      content: {
       'application/json': {
         schema: {
           type: 'object',
           properties: {
             statusCode: { type: 'number' },
             message: { type: 'string' },
             error: { type: 'string' },
           },
           example: {
             statusCode: 500,
             message: 'Query read timeout',
             error: 'Internal Server Error',
           },
         },
       },
      },
      };
      };

References

/packages/twenty-server/src/engine/core-modules/open-api/utils/get-error-responses.utils.ts /packages/twenty-server/src/engine/api/rest/errors/RestApiException.ts /packages/twenty-server/src/core/api-rest/api-rest.controller.utils.ts /packages/twenty-server/src/core/api-rest/api-rest.controller.utils.spec.ts

#### About Greptile This response provides a starting point for your research, not a precise solution. Help us improve! Please leave a 👍 if this is helpful and 👎 if it is irrelevant. [Ask Greptile](https://app.greptile.com/chat/github/twentyhq/twenty/main) · [Edit Issue Bot Settings](https://app.greptile.com/apps/github)
Murkyskate commented 1 month ago

idk but is there any user rate limit on the server side? although i have seen rate limit on the application level for some end points

charlesBochet commented 3 weeks ago

There is one but it's quite high right now. We will lower it as user will start spamming the platform :p