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

[4/n]: migrate the RESTAPI GET /rest/* to use TwentyORM directly #10372

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

pacyL2K19
Copy link
Contributor

@pacyL2K19 pacyL2K19 commented Feb 20, 2025

This PR

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This PR migrates the GET endpoint in the REST API to use TwentyORM directly instead of going through GraphQL, improving performance by removing the intermediate GraphQL layer.

  • Added new RestApiCoreServiceV2 in /packages/twenty-server/src/engine/api/rest/core/rest-api-core-v2.service.ts to handle direct TwentyORM operations
  • Implemented filter processing system in getWhereFilter method but currently commented out in the GET implementation
  • Added FilterInputFactory to RestApiModule for handling query filter parameters
  • Switched GET endpoint in RestApiCoreController to use restApiCoreServiceV2.get() with RestApiExceptionFilter
  • Commented out RequestMethod.GET from MIGRATED_REST_METHODS in app.module.ts to indicate work in progress

6 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile

@pacyL2K19 pacyL2K19 force-pushed the feat/#4-rest-api-to-twentyorm branch 2 times, most recently from 61edae6 to c573de8 Compare March 3, 2025 12:22
@pacyL2K19 pacyL2K19 force-pushed the feat/#4-rest-api-to-twentyorm branch 3 times, most recently from 319ee5e to e1ac8a3 Compare March 11, 2025 13:30
Copy link
Contributor Author

@pacyL2K19 pacyL2K19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@martmull I think this PR is ready for a review
Screenshot 2025-03-11 at 15 33 06

@pacyL2K19 pacyL2K19 force-pushed the feat/#4-rest-api-to-twentyorm branch from ec2ee2e to b084cab Compare March 11, 2025 20:30
Copy link
Contributor

@martmull martmull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, that's very nice code, concise and clear. I have left 2 some comments
Also, I see that depth query parameter is not taken into account in v2. @Weiko do you have an idea of how we can we do that with the twenty-orm ?

@pacyL2K19 pacyL2K19 force-pushed the feat/#4-rest-api-to-twentyorm branch from b084cab to cf31305 Compare March 12, 2025 11:10
@pacyL2K19
Copy link
Contributor Author

Hey, that's very nice code, concise and clear. I have left 2 some comments Also, I see that depth query parameter is not taken into account in v2. @Weiko do you have an idea of how we can we do that with the twenty-orm ?

Thank you for the review @martmull
We do have a ticket about the depth input, I will have to work on it after migrating the batch endpoint #9992

@Weiko
Copy link
Member

Weiko commented Mar 12, 2025

Hey, that's very nice code, concise and clear. I have left 2 some comments Also, I see that depth query parameter is not taken into account in v2. @Weiko do you have an idea of how we can we do that with the twenty-orm ?

In graphql we use ProcessNestedRelations, we will probably something similar here

@pacyL2K19
Copy link
Contributor Author

Hey, that's very nice code, concise and clear. I have left 2 some comments Also, I see that depth query parameter is not taken into account in v2. @Weiko do you have an idea of how we can we do that with the twenty-orm ?

In graphql we use ProcessNestedRelations, we will probably something similar here

Thanks for the hint @Weiko
I will be working on it later on in the process of this migration, we have a separate ticket already

Copy link
Contributor

@martmull martmull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I still don't see where featureFlagsMap is useful here but it is not that important
Nice work thank you!

@pacyL2K19 pacyL2K19 force-pushed the feat/#4-rest-api-to-twentyorm branch 4 times, most recently from 3ff5102 to 856e19f Compare March 15, 2025 06:55
@martmull martmull force-pushed the feat/#4-rest-api-to-twentyorm branch 2 times, most recently from e2e00a8 to b1b50eb Compare March 18, 2025 10:52
@pacyL2K19 pacyL2K19 force-pushed the feat/#4-rest-api-to-twentyorm branch 3 times, most recently from 5daddfc to a126ff6 Compare March 19, 2025 14:30
@prastoin prastoin force-pushed the feat/#4-rest-api-to-twentyorm branch from a126ff6 to 1af0c15 Compare March 24, 2025 13:09
@prastoin
Copy link
Contributor

prastoin commented Mar 24, 2025

Hey @martmull and @pacyL2K19, should this be merged ?
Rebased in order to manage conflicts with twenty-shared refactor, and also allow to run on latest twenty-server cicd

Remark: OrderByCondition marked as deprecated intypeorm

@pacyL2K19
Copy link
Contributor Author

Hey @martmull and @pacyL2K19, should this be merged ? Rebased in order to manage conflicts with twenty-shared refactor, and also allow to run on latest twenty-server cicd

Remark: OrderByCondition marked as deprecated intypeorm

Hi @prastoin
Please gimme a day to clean up integration tests
I will follow up on this asap

About the depreciation of OrderByCondition, yes, I saw that, may be worth an entire ticket, jic

@prastoin
Copy link
Contributor

Hi @pacyL2K19, obviously take your time no hurry ! Was just doing a tour on opened PRs

About the depreciation of OrderByCondition, yes, I saw that, may be worth an entire ticket, jic

Ok ! sounds good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants