refactor: rest APIs to use twenty orm #7988
Conversation
There was a problem hiding this comment.
PR Summary
This pull request refactors the REST APIs to utilize the Twenty ORM, focusing on the 'rest/batch/...' endpoint. Key changes include:
- Added RestMetadataMiddleware for token validation and metadata handling in app.module.ts
- Implemented createMany method in CoreQueryBuilderFactory to use Twenty ORM for batch operations
- Removed cleanGraphQLResponse utility in rest-api-core-batch.controller.ts, potentially affecting response structure
- Modified createMany method in rest-api-core.service.ts to return data directly and added LogExecutionTime decorator
- Introduced new WorkspaceMetadataCacheModule and WorkspaceCacheStorageModule in core-query-builder.module.ts
7 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile
| if (metadataVersion === undefined) { | ||
| await this.workspaceMetadataCacheService.recomputeMetadataCache({ | ||
| workspaceId: data.workspace.id, | ||
| }); | ||
| throw new Error('Metadata cache version not found'); | ||
| } |
There was a problem hiding this comment.
logic: Recomputing metadata cache and then throwing an error may lead to unnecessary work. Consider reordering these operations.
| req.workspaceMetadataVersion = metadataVersion; | ||
| req.workspaceMemberId = data.workspaceMemberId; | ||
| } catch (error) { | ||
| res.writeHead(200, { 'Content-Type': 'application/json' }); |
There was a problem hiding this comment.
style: Using status code 200 for errors is unconventional. Consider using appropriate error status codes.
|
Hello @charlesBochet , may I get your feedback? As this is an incomplete PR, I would love to work on that after the feedback. Thank you for reviewing :) |
|
/award 1500 sorry we couldn't give more feedback earlier |
|
Awarding DeepaPrasanna: 1500 points 🕹️ Well done! Check out your new contribution on oss.gg/DeepaPrasanna |
No worries! Thank you :) I would like to continue this later after feedback |
|
You are not allowed to award points! Please contact an admin. |
| const excludedOperations = [ | ||
| 'GetClientConfig', | ||
| 'GetCurrentUser', | ||
| 'GetWorkspaceFromInviteHash', | ||
| 'Track', | ||
| 'CheckUserExists', | ||
| 'Challenge', | ||
| 'Verify', | ||
| 'SignUp', | ||
| 'RenewToken', | ||
| 'EmailPasswordResetLink', | ||
| 'ValidatePasswordResetToken', | ||
| 'UpdatePasswordViaResetToken', | ||
| 'IntrospectionQuery', | ||
| 'ExchangeAuthorizationCode', | ||
| 'GetAuthorizationUrl', | ||
| 'FindAvailableSSOIdentityProviders', | ||
| ]; |
There was a problem hiding this comment.
Please refactor this list with the list located in packages/twenty-server/src/engine/middlewares/graphql-hydrate-request-from-token.middleware.ts
| try { | ||
| data = await this.tokenService.validateToken(req); | ||
| const metadataVersion = | ||
| await this.workspaceStorageCacheService.getMetadataVersion( | ||
| data.workspace.id, | ||
| ); | ||
|
|
||
| if (metadataVersion === undefined) { | ||
| await this.workspaceMetadataCacheService.recomputeMetadataCache({ | ||
| workspaceId: data.workspace.id, | ||
| }); | ||
| throw new Error('Metadata cache version not found'); | ||
| } | ||
|
|
||
| const dataSourcesMetadata = | ||
| await this.dataSourceService.getDataSourcesMetadataFromWorkspaceId( | ||
| data.workspace.id, | ||
| ); | ||
|
|
||
| if (!dataSourcesMetadata || dataSourcesMetadata.length === 0) { | ||
| throw new Error('No data sources found'); | ||
| } | ||
|
|
There was a problem hiding this comment.
Lot of code in this new file is shared with packages/twenty-server/src/engine/middlewares/graphql-hydrate-request-from-token.middleware.ts. We should factorize this logic and create a base HydrateRequestFromTokenMiddleware middleware
| req.workspaceMetadataVersion = metadataVersion; | ||
| req.workspaceMemberId = data.workspaceMemberId; | ||
| } catch (error) { | ||
| res.writeHead(200, { 'Content-Type': 'application/json' }); |
There was a problem hiding this comment.
for rest api, we don't want 200 error indeed
There was a problem hiding this comment.
yes indeed! I really missed that
| if (!objectMetadataMap) { | ||
| await this.workspaceMetadataCacheService.recomputeMetadataCache({ | ||
| workspaceId: context.workspace.id, | ||
| }); | ||
| throw new Error('Object metadata collection not found'); | ||
| } | ||
|
|
||
| const objectMetadataMapItem = | ||
| objectMetadataMap[objectMetadata.objectMetadataItem.nameSingular]; | ||
|
|
||
| const dataSource = | ||
| await this.twentyORMGlobalManager.getDataSourceForWorkspace( | ||
| context.workspace.id, | ||
| ); | ||
|
|
||
| const repository = dataSource.getRepository( | ||
| objectMetadataMapItem.nameSingular, | ||
| ); | ||
|
|
||
| const objectRecords: InsertResult = !request.body.upsert | ||
| ? await repository.insert(request.body) | ||
| : await repository.upsert(request.body, { | ||
| conflictPaths: ['id'], | ||
| skipUpdateIfNoValuesChanged: true, | ||
| }); | ||
|
|
||
| const queryBuilder = repository.createQueryBuilder( | ||
| objectMetadataMapItem.nameSingular, | ||
| ); | ||
|
|
||
| const nonFormattedUpsertedRecords = await queryBuilder | ||
| .where({ | ||
| id: In(objectRecords.generatedMaps.map((record) => record.id)), | ||
| }) | ||
| .take(QUERY_MAX_RECORDS) | ||
| .getMany(); | ||
|
|
||
| const upsertedRecords = formatResult( | ||
| nonFormattedUpsertedRecords, | ||
| objectMetadataMapItem, | ||
| objectMetadataMap, | ||
| ); |
There was a problem hiding this comment.
This code shared lot of code with packages/twenty-server/src/engine/api/graphql/graphql-query-runner/resolvers/graphql-query-create-many-resolver.service.ts
We must factorize that shared code
|
Hey @DeepaPrasanna, added some comments on your PR. I think this is the good way to go. I just see duplicated code that we must factorize |
|
Closing this PR to keep history clear :) |
fixes #3644
This isn't the whole refactoring yet. I tried to refactor the
rest/batch/...endpoint first. I added comments too for the part that might have to be included which were inGraphqlQueryRunnerService. Please let me know if I am moving in the right direction and If there is anything that I might have missed.Note: I will split the code in well scoped functionalities after knowing that I am moving in the right direction. Please excuse me until then.