Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make REST API use Twenty ORM direclty #3644

Open
FelixMalfait opened this issue Jan 26, 2024 · 31 comments
Open

Make REST API use Twenty ORM direclty #3644

FelixMalfait opened this issue Jan 26, 2024 · 31 comments
Assignees

Comments

@FelixMalfait
Copy link
Member

FelixMalfait commented Jan 26, 2024

Objective: Enhance the reliability and performance of the REST API.

Technical Context

Currently, our REST API (documentation here) is exposed via two dynamic endpoint sets:

  • rest/...
  • rest/batch/...
    These endpoints are handled by two controllers: rest-api-core.controller and rest-api-batch-core.controller, both backed by RestApiCoreService.

The RestApiCoreService currently parses REST payloads, converts them into GraphQL payloads, and calls the GraphQL API—an inefficient process. This approach was initially necessary because querying workspaceSchemas in Twenty required pg_graphql.

However, we recently introduced TwentyORM (soon to be renamed WorkspaceORM), a lightweight layer on top of TypeORM, which can directly query workspaceSchemas.

Goal: Refactor the REST API to leverage TwentyORM directly.

Note: The GraphQL API is also being migrated to TwentyORM (replacing pg_graphql), so you can draw inspiration from the GraphqlQueryRunnerService.

Key Goals:

Refactor the REST API to use TwentyORM directly.
Ensure the REST API is fully covered by integration tests.

Technical inputs

We should take a lot of inspiration from what has been done in the new GraphqlQueryRunnerService. Advices:

  1. split the code in services / utils to keep small and well scoped functional pieces
  2. Overall logic: a) parse REST payload into TypeORM format, b) send TypeORM query, c) parse result back into REST response
  3. All the pieces are already there: the current code is able to parse REST into graphql and Graphql into twentyORM, we "only" need to skip one step. It will likely mean to write everything from scratch but this should still speed a lot the process
@charlesBochet
Copy link
Member

charlesBochet commented Sep 14, 2024

Yes please!

If any experienced contributor wants to tackle this task, I'm happy to give a hand here. There is a lot of work but the engineering and the product is very well defined so it should be doable by the community

@charlesBochet charlesBochet added for experienced contributor scope: backend Issues that are affecting the backend side only prio: med labels Sep 14, 2024
@charlesBochet
Copy link
Member

@FelixMalfait It's not that difficult anymore I think as most of the complexity is within twenty-orm and we have quite good example of parsing in the current rest api and in our graphql implementation (we know how to parse rest input to graphql and graphql to orm already, we need to do rest input to orm direclty)

@charlesBochet
Copy link
Member

I'm editing the description to add more details

@charlesBochet charlesBochet changed the title Improve REST API performance by bypassing the HTTP Layer Make REST API use Twenty ORM direclty Sep 15, 2024
@Faisal-imtiyaz123
Copy link
Contributor

@charlesBochet I am ready to work. You may please assign me.

@FelixMalfait
Copy link
Member Author

@Faisal-imtiyaz123 let me know if we should unassign you. Thanks!

@Faisal-imtiyaz123
Copy link
Contributor

@FelixMalfait Yes you may unassign me. I talked to @charlesBochet regarding this.

@charlesBochet
Copy link
Member

/oss.gg 3000

Copy link

oss-gg bot commented Oct 13, 2024

Thanks for opening an issue! It's live on oss.gg!

@DeepaPrasanna
Copy link

I have working experience in nestjs, May I work on this? @charlesBochet

@charlesBochet
Copy link
Member

sure, thank you @DeepaPrasanna!
You can type: /assign to get assigned to it

@charlesBochet
Copy link
Member

It's not an easy one, feel free to ping me if you need help!

@DeepaPrasanna
Copy link

sure! Thank you

@DeepaPrasanna
Copy link

/assign

Copy link

oss-gg bot commented Oct 13, 2024

Assigned to @DeepaPrasanna! Please open a draft PR linking this issue within 48h ⚠️ If we can't detect a PR from you linking this issue in 48h, you'll be unassigned automatically 🕹️ Excited to have you ship this 🚀

@Tanmayshi
Copy link

/assign

Copy link

oss-gg bot commented Oct 13, 2024

This issue is already assigned to another person. Please find more issues here.

Copy link

oss-gg bot commented Oct 16, 2024

@DeepaPrasanna, Just a little reminder: Please open a draft PR linking this issue within 12 hours. If we can't detect a PR in 12h, you will be unassigned automatically.

@charlesBochet
Copy link
Member

/unassign

Copy link

oss-gg bot commented Oct 16, 2024

Issue unassigned.

@DeepaPrasanna
Copy link

hello @charlesBochet , I am working on this. I have an interview scheduled for today, that is why I paused for some time. I plan to resume my work again tomorrow.

@DeepaPrasanna
Copy link

/assign

Copy link

oss-gg bot commented Oct 16, 2024

Assigned to @DeepaPrasanna! Please open a draft PR linking this issue within 48h ⚠️ If we can't detect a PR from you linking this issue in 48h, you'll be unassigned automatically 🕹️ Excited to have you ship this 🚀

@devhiteshk
Copy link

/assign

Copy link

oss-gg bot commented Oct 17, 2024

This issue is already assigned to another person. Please find more issues here.

@DeepaPrasanna
Copy link

I will be creating a draft PR today. Pls don't unassign me.

@FelixMalfait
Copy link
Member Author

Thanks @DeepaPrasanna, great to create a PR to get intermediate feedback and make sure you go in the right direction since this is a big PR, thanks a lot!

@DeepaPrasanna
Copy link

Hello @charlesBochet @FelixMalfait , I have tried to refactor the rest/batch/... endpoint first. I have raised a draft PR. If I am moving in the right direction, I will refactor the remaining APIs soon. Requesting feedback :) Thank u

@charlesBochet
Copy link
Member

Thank you @DeepaPrasanna, I will take a look today

@DeepaPrasanna
Copy link

Just curious, how the distribution of the points will work if I am unable to complete it everything before 31st. I do understand and respect your time for reviews. :)

@charlesBochet
Copy link
Member

Awarded points on the PR :) We will take a deep look at your PR shortly, we are a bit overwhelmed by the volume of PRs right now!

@DeepaPrasanna
Copy link

Awarded points on the PR :) We will take a deep look at your PR shortly, we are a bit overwhelmed by the volume of PRs right now!

No worries! Thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🆕 New
Development

Successfully merging a pull request may close this issue.

7 participants