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 <corentin@twenty.com>

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)

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<ObjectRecord, IEdge<ObjectRecord>> => {
+  return 'edges' in result && Array.isArray(result.edges);
+};

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);
+};

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);
+};

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;
+};

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<ObjectRecord>
+  | { records: ObjectRecord[] }
+  | ObjectRecord
+  | ObjectRecord[];

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<any>;
+  handle(
+    objectRecord: ObjectRecord,
+    workspaceId: string,
+  ): Promise<ObjectRecord>;
 }

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<string, QueryResultGetterHandlerInterface>;
 
   constructor(private readonly fileService: FileService) {
@@ -30,37 +48,198 @@ export class QueryResultGettersFactory {
     ]);
   }
 
+  private async processConnection(
+    connection: IConnection<ObjectRecord>,
+    objectMetadataItemId: string,
+    objectMetadataMaps: ObjectMetadataMaps,
+    workspaceId: string,
+  ): Promise<IConnection<ObjectRecord>> {
+    return {
+      ...connection,
+      edges: await Promise.all(
+        connection.edges.map(async (edge: IEdge<ObjectRecord>) => ({
+          ...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<ObjectRecord> {
+    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<any> {
-    const handler = this.getHandler(objectMetadataItem.nameSingular);
+    return await this.processQueryResultField(
-
+      result,
-    if (result.edges) {
+      objectMetadataItem.id,
-      return {
+      objectMetadataMaps,
-        ...result,
+      workspaceId,
-        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);
   }
 
   private getHandler(objectType: string): QueryResultGetterHandlerInterface {

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({

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;
     };

packages/twenty-server/src/engine/metadata-modules/relation-metadata/relation-metadata.service.ts

@@ -55,8 +55,8 @@ export class RelationMetadataService extends TypeOrmQueryService<RelationMetadat
     private readonly workspaceMigrationService: WorkspaceMigrationService,
     private readonly workspaceMigrationRunnerService: WorkspaceMigrationRunnerService,
     private readonly workspaceMetadataVersionService: WorkspaceMetadataVersionService,
-    private readonly workspaceCacheStorageService: WorkspaceCacheStorageService,
     private readonly indexMetadataService: IndexMetadataService,
+    private readonly workspaceCacheStorageService: WorkspaceCacheStorageService,
   ) {
     super(relationMetadataRepository);
   }
@@ -483,6 +483,12 @@ export class RelationMetadataService extends TypeOrmQueryService<RelationMetadat
       const objectMetadata =
         objectMetadataMaps.byId[fieldMetadataItem.objectMetadataId];
 
+      if (!objectMetadata) {
+        return new NotFoundException(
+          `Object metadata not found for field ${fieldMetadataItem.id}`,
+        );
+      }
+
       const fieldMetadata = objectMetadata.fieldsById[fieldMetadataItem.id];
 
       const relationMetadata =

packages/twenty-ui/src/utilities/image/getImageAbsoluteURI.ts

@@ -1,11 +1,15 @@
 import { REACT_APP_SERVER_BASE_URL } from '@ui/utilities/config';
 
+// TODO: this is a code smell trying to guess whether it's a relative path or not
+// We should instead put the meaning onto our variables and parameters
+// imageUrl should be either imageAbsoluteURL or imageRelativeServerPath
+// But we need to refactor the chain of calls to this function
 export const getImageAbsoluteURI = (imageUrl?: string | null) => {
   if (!imageUrl) {
     return null;
   }
 
-  if (imageUrl?.startsWith('https:')) {
+  if (imageUrl?.startsWith('http')) {
     return imageUrl;
   }