twentyhq / twenty

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

REST API problems #6640

Closed BOHEUS closed 1 week ago

BOHEUS commented 3 weeks ago

Scenario:

  1. Using any API client, e.g. Postman, send any request to REST API (e.g. GET http://localhost:3000/rest/companies)

Actual: REST API returns 500 error code with no message why request failed image

Expected: REST API should return proper error code with clear message

### Tasks
- [x] Fix examples in the documentation
- [x] Basic fields like createdAt or deletedAt can't be overwritten
- [x] Change error messages to be more human-readable
BOHEUS commented 3 weeks ago

Upon further investigation, it seems there are more problems with REST API such as creating empty objects despite providing proper data, missing error codes when data is invalid or user tries to create a object with data overwriting basic fields such as createdBy or low quality examples in documentation.

@charlesBochet what's the good flow for it? Should I create more issues per problem?

charlesBochet commented 3 weeks ago

@martmull could you take a look here?

@BOHEUS we can use this issue to track all of them down :)

BOHEUS commented 3 weeks ago

Okay, each comment will be one issue to make everything clear and I'll create a task list just in case.

BOHEUS commented 3 weeks ago

Most examples in the documentation are missing proper request body example, here are some examples with other related problems

There is a body with random ID in it, but when I send it, there's 500 error code (why there's an ID in first place and where does it come from?) image

Here secondaryLinks are mentioned they're Object type, but what's the structure of them and what should they contain? image

There's depth but what does it mean for the end user? image

Why this response body says some fields are required? image

Why there are fields like createdAt in request body? Shouldn't this be prohibited? (Somewhat related to #6581) image

Also, schemas are missing proper example body image

BOHEUS commented 3 weeks ago

When creating a object with API, I'm able to override the createdAt and deletedAt fields which shouldn't be possible, here API should return error why such object couldn't be created or create an object nevertheless but do not use values in these fields provided by the user

Also, why the position has negative indexing? image

BOHEUS commented 3 weeks ago

The error message should be done in different way

Here's more extreme example image

charlesBochet commented 1 week ago

@BOHEUS thanks a lot!

@martmull could you prioritize it next sprint. Also note that we will soon migrate the Rest API over TwentyORM (WorkspaceORM) instead of re-using the GraphQL API so do not over invest on the fixes :)

BOHEUS commented 1 week ago

@charlesBochet @martmull why I can't filter records using position? Why position is a float in first place?

{
  "statusCode": 400,
  "message": "Variable \"$filter\" got invalid value \"1\" at \"filter.and[0].position.eq\"; Float cannot represent non numeric value: \"1\"",
  "error": "Bad Request"
}

image

martmull commented 1 week ago

Also, why the position has negative indexing?

@BOHEUS in useComputeNewRowPosition.ts, computeNewPosition can return negative new position when we move to first position if the current first record position has a position <= 0:

...
          if (moveToFirstPosition) {
            if (firstRecordPosition <= 0) {
              return firstRecordPosition - 1;
            } else {
              return firstRecordPosition / 2;
            }
          }
...

Why position is a float in first place?

You can see is that function that position can also be a float as we divide by 2 in certain cases. This allows to avoid updating all record positions when updating only one

BOHEUS commented 1 week ago

@martmull thanks for the answers!