From 77165a280e94d53438a0a6a659ec409c24d7424c Mon Sep 17 00:00:00 2001 From: Lucas Bordeau Date: Fri, 15 Nov 2024 16:34:58 +0100 Subject: [PATCH] Refactored query result getter handlers to support recursivity (#8497) The `QueryResultGettersFactory` that 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 https://github.com/twentyhq/twenty/issues/8425 Fixes https://github.com/twentyhq/twenty/issues/8498 --------- Co-authored-by: Weiko --- .../graphql-query-runner.service.ts | 1 + ...y-result-field-value-a-connection.guard.ts | 10 + ...field-value-a-nested-record-array.guard.ts | 8 + ...result-field-value-a-record-array.guard.ts | 8 + ...query-result-field-value-a-record.guard.ts | 8 + .../interfaces/query-result-field-value.ts | 8 + .../query-result-getter-handler.interface.ts | 7 +- .../query-result-getters.factory.ts | 235 +++++++++++++++--- .../core-modules/logger/logger.module.ts | 6 +- .../log-execution-time.decorator.ts | 10 +- .../relation-metadata.service.ts | 8 +- .../utilities/image/getImageAbsoluteURI.ts | 6 +- 12 files changed, 279 insertions(+), 36 deletions(-) create mode 100644 packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/guards/is-query-result-field-value-a-connection.guard.ts create mode 100644 packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/guards/is-query-result-field-value-a-nested-record-array.guard.ts create mode 100644 packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/guards/is-query-result-field-value-a-record-array.guard.ts create mode 100644 packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/guards/is-query-result-field-value-a-record.guard.ts create mode 100644 packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/interfaces/query-result-field-value.ts diff --git a/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/graphql-query-runner.service.ts b/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/graphql-query-runner.service.ts index 0e0220106..b79137ab8 100644 --- a/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/graphql-query-runner.service.ts +++ b/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/graphql-query-runner.service.ts @@ -343,6 +343,7 @@ export class GraphqlQueryRunnerService { results, objectMetadataItemWithFieldMaps, authContext.workspace.id, + options.objectMetadataMaps, ); const resultWithGettersArray = Array.isArray(resultWithGetters) diff --git a/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/guards/is-query-result-field-value-a-connection.guard.ts b/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/guards/is-query-result-field-value-a-connection.guard.ts new file mode 100644 index 000000000..e6861f360 --- /dev/null +++ b/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/guards/is-query-result-field-value-a-connection.guard.ts @@ -0,0 +1,10 @@ +import { ObjectRecord } from 'src/engine/api/graphql/workspace-query-builder/interfaces/object-record.interface'; +import { QueryResultFieldValue } from 'src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/interfaces/query-result-field-value'; +import { IConnection } from 'src/engine/api/graphql/workspace-query-runner/interfaces/connection.interface'; +import { IEdge } from 'src/engine/api/graphql/workspace-query-runner/interfaces/edge.interface'; + +export const isQueryResultFieldValueAConnection = ( + result: QueryResultFieldValue, +): result is IConnection> => { + return 'edges' in result && Array.isArray(result.edges); +}; diff --git a/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/guards/is-query-result-field-value-a-nested-record-array.guard.ts b/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/guards/is-query-result-field-value-a-nested-record-array.guard.ts new file mode 100644 index 000000000..cac0b4178 --- /dev/null +++ b/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/guards/is-query-result-field-value-a-nested-record-array.guard.ts @@ -0,0 +1,8 @@ +import { ObjectRecord } from 'src/engine/api/graphql/workspace-query-builder/interfaces/object-record.interface'; +import { QueryResultFieldValue } from 'src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/interfaces/query-result-field-value'; + +export const isQueryResultFieldValueANestedRecordArray = ( + result: QueryResultFieldValue, +): result is { records: ObjectRecord[] } => { + return 'records' in result && Array.isArray(result.records); +}; diff --git a/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/guards/is-query-result-field-value-a-record-array.guard.ts b/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/guards/is-query-result-field-value-a-record-array.guard.ts new file mode 100644 index 000000000..1ffc4a7fa --- /dev/null +++ b/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/guards/is-query-result-field-value-a-record-array.guard.ts @@ -0,0 +1,8 @@ +import { ObjectRecord } from 'src/engine/api/graphql/workspace-query-builder/interfaces/object-record.interface'; +import { QueryResultFieldValue } from 'src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/interfaces/query-result-field-value'; + +export const isQueryResultFieldValueARecordArray = ( + result: QueryResultFieldValue, +): result is ObjectRecord[] => { + return Array.isArray(result); +}; diff --git a/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/guards/is-query-result-field-value-a-record.guard.ts b/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/guards/is-query-result-field-value-a-record.guard.ts new file mode 100644 index 000000000..dc83cf7e8 --- /dev/null +++ b/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/guards/is-query-result-field-value-a-record.guard.ts @@ -0,0 +1,8 @@ +import { ObjectRecord } from 'src/engine/api/graphql/workspace-query-builder/interfaces/object-record.interface'; +import { QueryResultFieldValue } from 'src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/interfaces/query-result-field-value'; + +export const isQueryResultFieldValueARecord = ( + result: QueryResultFieldValue, +): result is ObjectRecord => { + return 'id' in result; +}; diff --git a/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/interfaces/query-result-field-value.ts b/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/interfaces/query-result-field-value.ts new file mode 100644 index 000000000..5a15e7369 --- /dev/null +++ b/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/interfaces/query-result-field-value.ts @@ -0,0 +1,8 @@ +import { ObjectRecord } from 'src/engine/api/graphql/workspace-query-builder/interfaces/object-record.interface'; +import { IConnection } from 'src/engine/api/graphql/workspace-query-runner/interfaces/connection.interface'; + +export type QueryResultFieldValue = + | IConnection + | { records: ObjectRecord[] } + | ObjectRecord + | ObjectRecord[]; diff --git a/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/interfaces/query-result-getter-handler.interface.ts b/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/interfaces/query-result-getter-handler.interface.ts index 2218afcde..4e1edc25d 100644 --- a/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/interfaces/query-result-getter-handler.interface.ts +++ b/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/interfaces/query-result-getter-handler.interface.ts @@ -1,3 +1,8 @@ +import { ObjectRecord } from 'src/engine/api/graphql/workspace-query-builder/interfaces/object-record.interface'; + export interface QueryResultGetterHandlerInterface { - handle(result: any, workspaceId: string): Promise; + handle( + objectRecord: ObjectRecord, + workspaceId: string, + ): Promise; } diff --git a/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/query-result-getters.factory.ts b/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/query-result-getters.factory.ts index 1f3b72b76..3602fb235 100644 --- a/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/query-result-getters.factory.ts +++ b/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/query-result-getters.factory.ts @@ -1,16 +1,34 @@ -import { Injectable } from '@nestjs/common'; +import { Injectable, Logger } from '@nestjs/common'; +import { ObjectRecord } from 'src/engine/api/graphql/workspace-query-builder/interfaces/object-record.interface'; +import { QueryResultFieldValue } from 'src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/interfaces/query-result-field-value'; import { QueryResultGetterHandlerInterface } from 'src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/interfaces/query-result-getter-handler.interface'; +import { IConnection } from 'src/engine/api/graphql/workspace-query-runner/interfaces/connection.interface'; +import { IEdge } from 'src/engine/api/graphql/workspace-query-runner/interfaces/edge.interface'; import { ObjectMetadataInterface } from 'src/engine/metadata-modules/field-metadata/interfaces/object-metadata.interface'; +import { isQueryResultFieldValueAConnection } from 'src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/guards/is-query-result-field-value-a-connection.guard'; +import { isQueryResultFieldValueANestedRecordArray } from 'src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/guards/is-query-result-field-value-a-nested-record-array.guard'; +import { isQueryResultFieldValueARecordArray } from 'src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/guards/is-query-result-field-value-a-record-array.guard'; +import { isQueryResultFieldValueARecord } from 'src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/guards/is-query-result-field-value-a-record.guard'; import { ActivityQueryResultGetterHandler } from 'src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/handlers/activity-query-result-getter.handler'; import { AttachmentQueryResultGetterHandler } from 'src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/handlers/attachment-query-result-getter.handler'; import { PersonQueryResultGetterHandler } from 'src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/handlers/person-query-result-getter.handler'; import { WorkspaceMemberQueryResultGetterHandler } from 'src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/handlers/workspace-member-query-result-getter.handler'; +import { CompositeInputTypeDefinitionFactory } from 'src/engine/api/graphql/workspace-schema-builder/factories/composite-input-type-definition.factory'; import { FileService } from 'src/engine/core-modules/file/services/file.service'; +import { ObjectMetadataMaps } from 'src/engine/metadata-modules/types/object-metadata-maps'; +import { isRelationFieldMetadataType } from 'src/engine/utils/is-relation-field-metadata-type.util'; +import { isDefined } from 'src/utils/is-defined'; +// TODO: find a way to prevent conflict between handlers executing logic on object relations +// And this factory that is also executing logic on object relations +// Right now the factory will override any change made on relations by the handlers @Injectable() export class QueryResultGettersFactory { + private readonly logger = new Logger( + CompositeInputTypeDefinitionFactory.name, + ); private handlers: Map; constructor(private readonly fileService: FileService) { @@ -30,37 +48,198 @@ export class QueryResultGettersFactory { ]); } + private async processConnection( + connection: IConnection, + objectMetadataItemId: string, + objectMetadataMaps: ObjectMetadataMaps, + workspaceId: string, + ): Promise> { + return { + ...connection, + edges: await Promise.all( + connection.edges.map(async (edge: IEdge) => ({ + ...edge, + node: await this.processRecord( + edge.node, + objectMetadataItemId, + objectMetadataMaps, + workspaceId, + ), + })), + ), + }; + } + + private async processNestedRecordArray( + result: { records: ObjectRecord[] }, + objectMetadataItemId: string, + objectMetadataMaps: ObjectMetadataMaps, + workspaceId: string, + ) { + return { + ...result, + records: await Promise.all( + result.records.map( + async (record: ObjectRecord) => + await this.processRecord( + record, + objectMetadataItemId, + objectMetadataMaps, + workspaceId, + ), + ), + ), + }; + } + + private async processRecordArray( + recordArray: ObjectRecord[], + objectMetadataItemId: string, + objectMetadataMaps: ObjectMetadataMaps, + workspaceId: string, + ) { + return await Promise.all( + recordArray.map( + async (record: ObjectRecord) => + await this.processRecord( + record, + objectMetadataItemId, + objectMetadataMaps, + workspaceId, + ), + ), + ); + } + + private async processRecord( + record: ObjectRecord, + objectMetadataItemId: string, + objectMetadataMaps: ObjectMetadataMaps, + workspaceId: string, + ): Promise { + const objectMetadataMapItem = objectMetadataMaps.byId[objectMetadataItemId]; + + const handler = this.getHandler(objectMetadataMapItem.nameSingular); + + const relationFields = Object.keys(record) + .map( + (recordFieldName) => + objectMetadataMapItem.fieldsByName[recordFieldName], + ) + .filter(isDefined) + .filter((fieldMetadata) => + isRelationFieldMetadataType(fieldMetadata.type), + ); + + const relationFieldsProcessedMap = {} as Record< + string, + QueryResultFieldValue + >; + + for (const relationField of relationFields) { + const relationMetadata = + relationField.fromRelationMetadata ?? relationField.toRelationMetadata; + + if (!isDefined(relationMetadata)) { + throw new Error('Relation metadata is not defined'); + } + + // TODO: computing this by taking the opposite of the current object metadata id + // is really less than ideal. This should be computed based on the relation metadata + // But right now it is too complex with the current structure and / or lack of utils + // around the possible combinations with relation metadata from / to + MANY_TO_ONE / ONE_TO_MANY + const relationObjectMetadataItemId = + relationMetadata.fromObjectMetadataId === objectMetadataItemId + ? relationMetadata.toObjectMetadataId + : relationMetadata.fromObjectMetadataId; + + const relationObjectMetadataItem = + objectMetadataMaps.byId[relationObjectMetadataItemId]; + + if (!isDefined(relationObjectMetadataItem)) { + throw new Error( + `Object metadata not found for id ${relationObjectMetadataItemId}`, + ); + } + + relationFieldsProcessedMap[relationField.name] = + await this.processQueryResultField( + record[relationField.name], + relationObjectMetadataItem.id, + objectMetadataMaps, + workspaceId, + ); + } + + const objectRecordProcessedWithoutRelationFields = await handler.handle( + record, + workspaceId, + ); + + const processedRecord = { + ...objectRecordProcessedWithoutRelationFields, + ...relationFieldsProcessedMap, + }; + + return processedRecord; + } + + private async processQueryResultField( + queryResultField: QueryResultFieldValue, + objectMetadataItemId: string, + objectMetadataMaps: ObjectMetadataMaps, + workspaceId: string, + ) { + if (isQueryResultFieldValueAConnection(queryResultField)) { + return await this.processConnection( + queryResultField, + objectMetadataItemId, + objectMetadataMaps, + workspaceId, + ); + } else if (isQueryResultFieldValueANestedRecordArray(queryResultField)) { + return await this.processNestedRecordArray( + queryResultField, + objectMetadataItemId, + objectMetadataMaps, + workspaceId, + ); + } else if (isQueryResultFieldValueARecordArray(queryResultField)) { + return await this.processRecordArray( + queryResultField, + objectMetadataItemId, + objectMetadataMaps, + workspaceId, + ); + } else if (isQueryResultFieldValueARecord(queryResultField)) { + return await this.processRecord( + queryResultField, + objectMetadataItemId, + objectMetadataMaps, + workspaceId, + ); + } else { + this.logger.warn( + `Query result field is not a record, connection, nested record array or record array. + This is an undetected case in query result getter that should be implemented !!`, + ); + + return queryResultField; + } + } + async create( - result: any, + result: QueryResultFieldValue, objectMetadataItem: ObjectMetadataInterface, workspaceId: string, + objectMetadataMaps: ObjectMetadataMaps, ): Promise { - const handler = this.getHandler(objectMetadataItem.nameSingular); - - if (result.edges) { - return { - ...result, - edges: await Promise.all( - result.edges.map(async (edge: any) => ({ - ...edge, - node: await handler.handle(edge.node, workspaceId), - })), - ), - }; - } - - if (result.records) { - return { - ...result, - records: await Promise.all( - result.records.map( - async (item: any) => await handler.handle(item, workspaceId), - ), - ), - }; - } - - return await handler.handle(result, workspaceId); + return await this.processQueryResultField( + result, + objectMetadataItem.id, + objectMetadataMaps, + workspaceId, + ); } private getHandler(objectType: string): QueryResultGetterHandlerInterface { diff --git a/packages/twenty-server/src/engine/core-modules/logger/logger.module.ts b/packages/twenty-server/src/engine/core-modules/logger/logger.module.ts index 4822a780b..fba6a3a5f 100644 --- a/packages/twenty-server/src/engine/core-modules/logger/logger.module.ts +++ b/packages/twenty-server/src/engine/core-modules/logger/logger.module.ts @@ -1,13 +1,13 @@ -import { DynamicModule, Global, ConsoleLogger, Module } from '@nestjs/common'; +import { ConsoleLogger, DynamicModule, Global, Module } from '@nestjs/common'; -import { LoggerService } from 'src/engine/core-modules/logger/logger.service'; +import { LoggerDriverType } from 'src/engine/core-modules/logger/interfaces'; import { LOGGER_DRIVER } from 'src/engine/core-modules/logger/logger.constants'; import { ASYNC_OPTIONS_TYPE, ConfigurableModuleClass, OPTIONS_TYPE, } from 'src/engine/core-modules/logger/logger.module-definition'; -import { LoggerDriverType } from 'src/engine/core-modules/logger/interfaces'; +import { LoggerService } from 'src/engine/core-modules/logger/logger.service'; @Global() @Module({ diff --git a/packages/twenty-server/src/engine/decorators/observability/log-execution-time.decorator.ts b/packages/twenty-server/src/engine/decorators/observability/log-execution-time.decorator.ts index 6f9fbe560..5625b98cf 100644 --- a/packages/twenty-server/src/engine/decorators/observability/log-execution-time.decorator.ts +++ b/packages/twenty-server/src/engine/decorators/observability/log-execution-time.decorator.ts @@ -1,11 +1,13 @@ import { Logger } from '@nestjs/common'; +import { isDefined } from 'class-validator'; + /** * 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) { return function ( target: any, propertyKey: string, @@ -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)) { + logger.log(`${label} execution time: ${executionTime.toFixed(2)}ms`); + } else { + logger.log(`Execution time: ${executionTime.toFixed(2)}ms`); + } return result; }; diff --git a/packages/twenty-server/src/engine/metadata-modules/relation-metadata/relation-metadata.service.ts b/packages/twenty-server/src/engine/metadata-modules/relation-metadata/relation-metadata.service.ts index 19daa9c46..a98f2f002 100644 --- a/packages/twenty-server/src/engine/metadata-modules/relation-metadata/relation-metadata.service.ts +++ b/packages/twenty-server/src/engine/metadata-modules/relation-metadata/relation-metadata.service.ts @@ -55,8 +55,8 @@ export class RelationMetadataService extends TypeOrmQueryService { if (!imageUrl) { return null; } - if (imageUrl?.startsWith('https:')) { + if (imageUrl?.startsWith('http')) { return imageUrl; }