Refactor graphql query runner + fix nested or #6986
Conversation
There was a problem hiding this comment.
PR Summary
This pull request refactors the GraphQL query runner, improving nested 'OR' condition handling and introducing new services for better code organization and maintainability.
- Introduced GraphqlQueryFindOneResolverService and GraphqlQueryFindManyResolverService in separate files, simplifying the main GraphqlQueryRunnerService
- Refactored GraphqlQueryFilterConditionParser to handle nested 'and' and 'or' conditions more efficiently
- Added new exception codes INVALID_ARGS_FIRST and INVALID_ARGS_LAST for improved pagination error handling
- Implemented getObjectMetadata function in convert-object-metadata-to-map.util.ts for robust object metadata retrieval
- Updated exception handling in workspace-query-runner-graphql-api-exception-handler.util.ts to provide more specific error feedback
7 file(s) reviewed, 8 comment(s)
Edit PR Review Bot Settings
...y-runner/graphql-query-parsers/graphql-query-filter/graphql-query-filter-condition.parser.ts
Show resolved
Hide resolved
| const graphqlQueryFindOneResolverService = | ||
| new GraphqlQueryFindOneResolverService(this.twentyORMGlobalManager); |
There was a problem hiding this comment.
style: Consider injecting this service instead of creating a new instance
| const graphqlQueryFindManyResolverService = | ||
| new GraphqlQueryFindManyResolverService(this.twentyORMGlobalManager); |
There was a problem hiding this comment.
style: Consider injecting this service instead of creating a new instance
| if (objectRecords.length > limit) { | ||
| objectRecords.pop(); | ||
| } |
There was a problem hiding this comment.
style: This modifies the original objectRecords array. Consider creating a new array instead
| for (const column of Object.keys(order || {})) { | ||
| if (!select[column]) { | ||
| select[column] = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
style: This mutates the select object. Consider creating a new object instead
| >( | ||
| args: FindOneResolverArgs<Filter>, | ||
| options: WorkspaceQueryRunnerOptions, | ||
| ): Promise<ObjectRecord | undefined> { |
There was a problem hiding this comment.
logic: The return type Promise<ObjectRecord | undefined> doesn't match the implementation, which always returns ObjectRecord or throws an exception.
| objectMetadataItem, | ||
| graphqlFields(info), | ||
| ); | ||
| const where = graphqlQueryParser.parseFilter(args.filter ?? ({} as Filter)); |
There was a problem hiding this comment.
style: Consider adding a type assertion to {} instead of using 'as Filter' to maintain type safety.
| return typeORMObjectRecordsParser.processRecord( | ||
| objectRecord, | ||
| objectMetadataItem.nameSingular, | ||
| 1, | ||
| 1, | ||
| ) as ObjectRecord; |
There was a problem hiding this comment.
style: The hardcoded values 1, 1 passed to processRecord() might need explanation or be replaced with meaningful constants.
No description provided.