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.

Sign up for GitHub

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

Jump to bottom

[1/n]: Migrate deleteOne Rest API to use TwentyORM directly #9784

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

[1/n]: Migrate deleteOne Rest API to use TwentyORM directly #9784

wants to merge 6 commits into from

Conversation

pacyL2K19
Copy link
Contributor

@pacyL2K19 pacyL2K19 commented Jan 22, 2025 •
edited
Loading

This PR

  • Addressing Make REST API use Twenty ORM direclty #3644

  • Migrates the DELETE /rest/* endpoint to use TwentyORM

  • Factorizes common middleware logic into a common module

  • [WIP] 🚧 Writing integration tests for the endpoint updated

greptile-apps[bot]
greptile-apps bot reviewed Jan 22, 2025
View reviewed changes
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 introduces direct TwentyORM integration for the DELETE REST API endpoint, replacing the previous GraphQL-based implementation with a more efficient direct database access approach.

  • Added new RestApiCoreServiceV2 in /packages/twenty-server/src/engine/api/rest/core/rest-api-core-v2.service.ts for direct TwentyORM operations
  • Implemented RestCoreMiddleware in /packages/twenty-server/src/engine/middlewares/rest-core-middleware.ts for authentication and metadata validation
  • Created CommonMiddlewareOperationsService in /packages/twenty-server/src/engine/middlewares/common/common.service.ts to share middleware logic between REST and GraphQL
  • Modified RestApiCoreController to use RestApiCoreServiceV2 for DELETE operations while maintaining backward compatibility for other endpoints
  • Added utility function extractObjectIdFromPath in /packages/twenty-server/src/engine/api/rest/core/utils/extract-id-from-path.utils.ts for UUID validation

12 file(s) reviewed, 14 comment(s)
Edit PR Review Bot Settings | Greptile

packages/twenty-server/src/engine/api/rest/core/controllers/rest-api-core.controller.ts Show resolved Hide resolved
packages/twenty-server/src/app.module.ts Show resolved Hide resolved
packages/twenty-server/src/engine/api/rest/core/rest-api-core-v2.service.ts Outdated Show resolved Hide resolved
packages/twenty-server/src/engine/api/rest/core/rest-api-core-v2.service.ts Outdated Show resolved Hide resolved
packages/twenty-server/src/engine/api/rest/core/utils/extract-id-from-path.utils.ts Outdated Show resolved Hide resolved
packages/twenty-server/src/engine/middlewares/common/common.service.ts Outdated Show resolved Hide resolved
packages/twenty-server/src/engine/middlewares/rest-core-middleware.ts Outdated Show resolved Hide resolved
packages/twenty-server/src/engine/middlewares/rest-core-middleware.ts Outdated Show resolved Hide resolved
packages/twenty-server/src/engine/middlewares/rest-core-middleware.ts Outdated Show resolved Hide resolved
packages/twenty-server/src/engine/middlewares/rest-core-middleware.ts Outdated Show resolved Hide resolved
martmull
martmull reviewed Jan 22, 2025
View reviewed changes
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, very nice code and strategy. Like the way you factorized middlewares and the new rest-api-core-v2 service is a great seed for the migration
I put some comments about code standard we have at twenty and some simplification suggestions. Feel free to comment. Thank you

packages/twenty-server/src/engine/middlewares/common/common-middleware-operations.module.ts Outdated Show resolved Hide resolved
packages/twenty-server/src/engine/middlewares/common/common.service.ts Outdated Show resolved Hide resolved
packages/twenty-server/src/engine/middlewares/rest-core-middleware.ts Outdated Show resolved Hide resolved
packages/twenty-server/src/engine/middlewares/common/common.service.ts Outdated Show resolved Hide resolved
packages/twenty-server/src/engine/middlewares/common/common-middleware-operations.module.ts Outdated Show resolved Hide resolved
packages/twenty-server/src/engine/api/rest/core/rest-api-core-v2.service.ts Outdated Show resolved Hide resolved
packages/twenty-server/src/engine/api/rest/core/rest-api-core-v2.service.ts Outdated Show resolved Hide resolved
packages/twenty-server/src/engine/api/rest/core/rest-api-core-v2.service.ts Outdated Show resolved Hide resolved
packages/twenty-server/src/engine/api/rest/core/rest-api-core-v2.service.ts Outdated Show resolved Hide resolved
packages/twenty-server/src/engine/api/rest/core/utils/extract-id-from-path.utils.ts Outdated Show resolved Hide resolved
@pacyL2K19 pacyL2K19 force-pushed the feat/rest-api-on-twenty-orm branch from 32469c4 to 7a2a2a6 Compare January 22, 2025 15:18
@prastoin prastoin self-requested a review January 22, 2025 15:36
prastoin
prastoin reviewed Jan 22, 2025
View reviewed changes
Copy link
Contributor

@prastoin prastoin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hello there ! Hope you're doing great
GG for this great PR
I've just reviewed the integrations tests left few comments, no big deal !
About to review main logic afterwards !
If I can provide any further information of course let me know !

packages/twenty-server/test/integration/rest/suites/rest-api-core.integration-spec.ts Show resolved Hide resolved
packages/twenty-server/test/integration/rest/suites/rest-api-core.integration-spec.ts Show resolved Hide resolved
packages/twenty-server/test/integration/rest/suites/rest-api-core.integration-spec.ts Show resolved Hide resolved
packages/twenty-server/test/integration/rest/suites/rest-api-core.integration-spec.ts Show resolved Hide resolved
packages/twenty-server/test/integration/rest/suites/rest-api-core.integration-spec.ts Show resolved Hide resolved
packages/twenty-server/test/integration/rest/utils/make-rest-api-request.util.ts Show resolved Hide resolved
@pacyL2K19
Copy link
Contributor Author

Hello there ! Hope you're doing great GG for this great PR I've left few comments, no big deal ! If I can provide any further information of course let me know !

Thank you for the review @prastoin, great suggestions
I'll be following up on this asap tomorrow morning

prastoin
prastoin reviewed Jan 22, 2025
View reviewed changes
Copy link
Contributor

@prastoin prastoin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Second review on the logic this time !
Indeed there're some very neat refactor out there, good scoping !
Sorry for the long review, I'm available whenever you are tomorrow for peer review session if you wish to !
Please let me know, have a great evening

packages/twenty-e2e-testing/tests/authentication/fixture.ts Show resolved Hide resolved
packages/twenty-server/src/app.module.ts Show resolved Hide resolved
...es/twenty-server/src/engine/middlewares/constants/excluded-middleware-operations.constant.ts Show resolved Hide resolved
packages/twenty-server/src/engine/middlewares/graphql-hydrate-request-from-token.middleware.ts Show resolved Hide resolved
packages/twenty-server/src/engine/middlewares/graphql-hydrate-request-from-token.middleware.ts Show resolved Hide resolved
packages/twenty-server/src/engine/middlewares/middleware.service.ts Show resolved Hide resolved
packages/twenty-server/src/engine/middlewares/middleware.service.ts Show resolved Hide resolved
packages/twenty-server/src/engine/middlewares/rest-core.middleware.ts Show resolved Hide resolved
packages/twenty-server/src/engine/middlewares/rest-core.middleware.ts Show resolved Hide resolved
@FelixMalfait
Copy link
Member

Nice!

@pacyL2K19 pacyL2K19 force-pushed the feat/rest-api-on-twenty-orm branch 2 times, most recently from 904f938 to 40a828e Compare January 23, 2025 10:38
martmull
martmull reviewed Jan 23, 2025
View reviewed changes
packages/twenty-server/src/engine/api/rest/core/rest-api-core-v2.service.ts Show resolved Hide resolved
packages/twenty-server/src/engine/middlewares/middleware.service.ts
Comment on lines +29 to +31
: error instanceof Error
? 500
: error.status || 500,
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we should not set 500 status for all Error, we should

Suggested change
: error instanceof Error
? 500
: error.status || 500,
: error.status || 500,

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion
I got it, pushing the latest changes asap

@pacyL2K19 pacyL2K19 force-pushed the feat/rest-api-on-twenty-orm branch from 40a828e to aa4be76 Compare January 24, 2025 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments

@prastoin prastoin prastoin left review comments

@martmull martmull martmull left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
-PR: awaiting review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pacyL2K19 @FelixMalfait @martmull @prastoin @charlesBochet