Refactored query result getter handlers to support recursivity #8497
Conversation
TODOs/FIXMEs:
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR refactors query result getter handlers to support recursive processing of nested data structures, with improvements to image handling and metadata caching.
- Added recursive processing in
QueryResultGettersFactoryto handle nested relations, connections, and record arrays - Introduced type guards in
isResultAConnection.tsto safely check query result shapes - Added
objectMetadataMapparameter toqueryResultGettersFactory.create()for metadata-aware processing - Enhanced
LogExecutionTimedecorator with optional labels for better observability - Improved caching with new
getWorkspaceObjectMetadataMapmethod inWorkspaceMetadataCacheService
11 file(s) reviewed, 7 comment(s)
Edit PR Review Bot Settings | Greptile
| async handle( | ||
| person: PersonWorkspaceEntity, | ||
| workspaceId: string, | ||
| ): Promise<PersonWorkspaceEntity> { | ||
| console.log('PersonQueryResultGetterHandler.handle'); | ||
|
|
||
| if (!person.id || !person?.avatarUrl) { |
There was a problem hiding this comment.
style: Optional chaining (?.) is redundant here since person.id is already checked
| async handle( | ||
| person: PersonWorkspaceEntity, | ||
| workspaceId: string, | ||
| ): Promise<PersonWorkspaceEntity> { | ||
| console.log('PersonQueryResultGetterHandler.handle'); |
There was a problem hiding this comment.
style: Consider removing this console.log or replacing with proper logger since execution time is already being logged via decorator
| const objectRecordProcessedWithoutRelationFields = await handler.handle( | ||
| record, | ||
| workspaceId, | ||
| ); | ||
|
|
||
| const newRecord = { | ||
| ...objectRecordProcessedWithoutRelationFields, | ||
| ...relationFieldsProcessedMap, | ||
| }; |
There was a problem hiding this comment.
logic: Handler results are being overridden by relationFieldsProcessedMap spread. This could cause loss of handler-specific processing for relation fields.
| async processConnection( | ||
| connection: IConnection<ObjectRecord>, | ||
| objectMetadataItemId: string, | ||
| objectMetadataMap: ObjectMetadataMap, | ||
| workspaceId: string, | ||
| ): Promise<any> { |
There was a problem hiding this comment.
style: processConnection returns Promise while having strongly typed input parameters. Consider using concrete return type.
| export const isPossibleFieldValueAConnection = ( | ||
| result: PossibleQueryResultFieldValue, | ||
| ): result is IConnection<ObjectRecord, IEdge<ObjectRecord>> => { | ||
| return isDefined((result as any).edges); |
There was a problem hiding this comment.
logic: Using (result as any).edges bypasses type safety. Consider checking if result is an object first with typeof result === 'object' && result !== null
| /** | ||
| * A decorator function that logs the execution time of the decorated method. | ||
| * | ||
| * @returns The modified property descriptor with the execution time logging functionality. | ||
| */ | ||
| export function LogExecutionTime() { | ||
| export function LogExecutionTime(label?: string | undefined) { |
There was a problem hiding this comment.
style: The type annotation string | undefined is redundant when using optional parameter syntax ?
| @@ -21,7 +23,11 @@ export function LogExecutionTime() { | |||
| const end = performance.now(); | |||
| const executionTime = end - start; | |||
|
|
|||
| logger.log(`Execution time: ${executionTime.toFixed(2)}ms`); | |||
| if (isDefined(label)) { | |||
There was a problem hiding this comment.
logic: isDefined() will return true for empty strings - consider using a more strict check like label?.length > 0
Weiko
force-pushed
the
fix/upload-picture-bugs
branch
from
cd0efaa
to
d83400b
Compare
The
QueryResultGettersFactorythat is called on every query return to was called only on the first level of relations because recursivity wasn't implemented.In this PR I implement recursivity and add some typing for the possible forms a GraphQL query field can take.
This PR will fix any issue we have with pictures that were losing their token (here for person.avatarUrl)
Fixes #8425
Fixes #8498