From c60a3e49cda73688679c99be0e8bb514c2ad768a Mon Sep 17 00:00:00 2001 From: Thomas Trompette Date: Fri, 31 May 2024 10:39:35 +0200 Subject: [PATCH] Catch query timeout exceptions (#5680) Query read timeouts happen when a remote server is not available. It breaks: - the remote server show page - the record table page of imported remote tables This PR will catch the exception so it does not go to Sentry in both cases. Also did 2 renaming. --- packages/twenty-server/src/command/command.ts | 4 +- .../typeorm/metadata/metadata.datasource.ts | 3 + .../src/database/typeorm/typeorm.service.ts | 3 + ...d.util.ts => assert-is-valid-uuid.util.ts} | 0 .../workspace-query-runner.service.ts | 44 +++++----- .../hooks/use-exception-handler.hook.ts | 4 +- .../distant-table/distant-table.service.ts | 85 +++++++++++-------- .../utils/global-exception-handler.util.ts | 6 +- .../src/engine/utils/graphql-errors.util.ts | 8 ++ .../src/engine/utils/query-timeout.util.ts | 3 + .../src/queue-worker/queue-worker.ts | 4 +- 11 files changed, 101 insertions(+), 63 deletions(-) rename packages/twenty-server/src/engine/api/graphql/workspace-query-runner/utils/{assertIsValidUuid.util.ts => assert-is-valid-uuid.util.ts} (100%) create mode 100644 packages/twenty-server/src/engine/utils/query-timeout.util.ts diff --git a/packages/twenty-server/src/command/command.ts b/packages/twenty-server/src/command/command.ts index a099c5943a..257d24e240 100644 --- a/packages/twenty-server/src/command/command.ts +++ b/packages/twenty-server/src/command/command.ts @@ -1,6 +1,6 @@ import { CommandFactory } from 'nest-commander'; -import { filterException } from 'src/engine/utils/global-exception-handler.util'; +import { shouldFilterException } from 'src/engine/utils/global-exception-handler.util'; import { ExceptionHandlerService } from 'src/engine/integrations/exception-handler/exception-handler.service'; import { LoggerService } from 'src/engine/integrations/logger/logger.service'; @@ -10,7 +10,7 @@ async function bootstrap() { const errorHandler = (err: Error) => { loggerService.error(err?.message, err?.name); - if (filterException(err)) { + if (shouldFilterException(err)) { return; } diff --git a/packages/twenty-server/src/database/typeorm/metadata/metadata.datasource.ts b/packages/twenty-server/src/database/typeorm/metadata/metadata.datasource.ts index 89bc837d99..7aa3ed1ab7 100644 --- a/packages/twenty-server/src/database/typeorm/metadata/metadata.datasource.ts +++ b/packages/twenty-server/src/database/typeorm/metadata/metadata.datasource.ts @@ -20,6 +20,9 @@ export const typeORMMetadataModuleOptions: TypeOrmModuleOptions = { rejectUnauthorized: false, } : undefined, + extra: { + query_timeout: 10000, + }, }; export const connectionSource = new DataSource( typeORMMetadataModuleOptions as DataSourceOptions, diff --git a/packages/twenty-server/src/database/typeorm/typeorm.service.ts b/packages/twenty-server/src/database/typeorm/typeorm.service.ts index a3cedb8397..ce986c2ac8 100644 --- a/packages/twenty-server/src/database/typeorm/typeorm.service.ts +++ b/packages/twenty-server/src/database/typeorm/typeorm.service.ts @@ -38,6 +38,9 @@ export class TypeORMService implements OnModuleInit, OnModuleDestroy { rejectUnauthorized: false, } : undefined, + extra: { + query_timeout: 10000, + }, }); } diff --git a/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/utils/assertIsValidUuid.util.ts b/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/utils/assert-is-valid-uuid.util.ts similarity index 100% rename from packages/twenty-server/src/engine/api/graphql/workspace-query-runner/utils/assertIsValidUuid.util.ts rename to packages/twenty-server/src/engine/api/graphql/workspace-query-runner/utils/assert-is-valid-uuid.util.ts diff --git a/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/workspace-query-runner.service.ts b/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/workspace-query-runner.service.ts index bb922291eb..a1cab54bee 100644 --- a/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/workspace-query-runner.service.ts +++ b/packages/twenty-server/src/engine/api/graphql/workspace-query-runner/workspace-query-runner.service.ts @@ -3,6 +3,7 @@ import { Inject, Injectable, Logger, + RequestTimeoutException, } from '@nestjs/common'; import { EventEmitter2 } from '@nestjs/event-emitter'; @@ -49,7 +50,8 @@ import { QueryRunnerArgsFactory } from 'src/engine/api/graphql/workspace-query-r import { QueryResultGettersFactory } from 'src/engine/api/graphql/workspace-query-runner/factories/query-result-getters.factory'; import { assertMutationNotOnRemoteObject } from 'src/engine/metadata-modules/object-metadata/utils/assert-mutation-not-on-remote-object.util'; import { STANDARD_OBJECT_IDS } from 'src/engine/workspace-manager/workspace-sync-metadata/constants/standard-object-ids'; -import { assertIsValidUuid } from 'src/engine/api/graphql/workspace-query-runner/utils/assertIsValidUuid.util'; +import { assertIsValidUuid } from 'src/engine/api/graphql/workspace-query-runner/utils/assert-is-valid-uuid.util'; +import { isQueryTimeoutError } from 'src/engine/utils/query-timeout.util'; import { WorkspaceQueryRunnerOptions } from './interfaces/query-runner-option.interface'; import { @@ -577,28 +579,30 @@ export class WorkspaceQueryRunnerService { workspaceId, ); - await workspaceDataSource?.query(` - SET search_path TO ${this.workspaceDataSourceService.getSchemaName( - workspaceId, - )}; - `); + try { + return await workspaceDataSource?.transaction( + async (transactionManager) => { + await transactionManager.query(` + SET search_path TO ${this.workspaceDataSourceService.getSchemaName( + workspaceId, + )}; + `); - return await workspaceDataSource?.transaction( - async (transactionManager) => { - await transactionManager.query(` - SET search_path TO ${this.workspaceDataSourceService.getSchemaName( - workspaceId, - )}; - `); + const results = transactionManager.query( + `SELECT graphql.resolve($1);`, + [query], + ); - const results = transactionManager.query( - `SELECT graphql.resolve($1);`, - [query], - ); + return results; + }, + ); + } catch (error) { + if (isQueryTimeoutError(error)) { + throw new RequestTimeoutException(error.message); + } - return results; - }, - ); + throw error; + } } private async parseResult( diff --git a/packages/twenty-server/src/engine/integrations/exception-handler/hooks/use-exception-handler.hook.ts b/packages/twenty-server/src/engine/integrations/exception-handler/hooks/use-exception-handler.hook.ts index 3e72c95047..cdfae3bf76 100644 --- a/packages/twenty-server/src/engine/integrations/exception-handler/hooks/use-exception-handler.hook.ts +++ b/packages/twenty-server/src/engine/integrations/exception-handler/hooks/use-exception-handler.hook.ts @@ -11,7 +11,7 @@ import { GraphQLContext } from 'src/engine/api/graphql/graphql-config/interfaces import { ExceptionHandlerService } from 'src/engine/integrations/exception-handler/exception-handler.service'; import { convertExceptionToGraphQLError, - filterException, + shouldFilterException, } from 'src/engine/utils/global-exception-handler.util'; export type ExceptionHandlerPluginOptions = { @@ -69,7 +69,7 @@ export const useExceptionHandler = ( }>( (acc, err) => { // Filter out exceptions that we don't want to be captured by exception handler - if (filterException(err?.originalError ?? err)) { + if (shouldFilterException(err?.originalError ?? err)) { acc.filtered.push(err); } else { acc.unfiltered.push(err); diff --git a/packages/twenty-server/src/engine/metadata-modules/remote-server/remote-table/distant-table/distant-table.service.ts b/packages/twenty-server/src/engine/metadata-modules/remote-server/remote-table/distant-table/distant-table.service.ts index 64d15785ae..d0e5fdadeb 100644 --- a/packages/twenty-server/src/engine/metadata-modules/remote-server/remote-table/distant-table/distant-table.service.ts +++ b/packages/twenty-server/src/engine/metadata-modules/remote-server/remote-table/distant-table/distant-table.service.ts @@ -1,4 +1,8 @@ -import { BadRequestException, Injectable } from '@nestjs/common'; +import { + BadRequestException, + Injectable, + RequestTimeoutException, +} from '@nestjs/common'; import { InjectRepository } from '@nestjs/typeorm'; import { EntityManager, Repository } from 'typeorm'; @@ -12,6 +16,7 @@ import { WorkspaceDataSourceService } from 'src/engine/workspace-datasource/work import { DistantTables } from 'src/engine/metadata-modules/remote-server/remote-table/distant-table/types/distant-table'; import { STRIPE_DISTANT_TABLES } from 'src/engine/metadata-modules/remote-server/remote-table/distant-table/utils/stripe-distant-tables.util'; import { PostgresTableSchemaColumn } from 'src/engine/metadata-modules/remote-server/types/postgres-table-schema-column'; +import { isQueryTimeoutError } from 'src/engine/utils/query-timeout.util'; @Injectable() export class DistantTableService { @@ -70,44 +75,54 @@ export class DistantTableService { workspaceId, ); - const distantTables = await workspaceDataSource.transaction( - async (entityManager: EntityManager) => { - await entityManager.query(`CREATE SCHEMA "${tmpSchemaName}"`); + try { + const distantTables = await workspaceDataSource.transaction( + async (entityManager: EntityManager) => { + await entityManager.query(`CREATE SCHEMA "${tmpSchemaName}"`); - const tableLimitationsOptions = tableName - ? ` LIMIT TO ("${tableName}")` - : ''; + const tableLimitationsOptions = tableName + ? ` LIMIT TO ("${tableName}")` + : ''; - await entityManager.query( - `IMPORT FOREIGN SCHEMA "${remoteServer.schema}"${tableLimitationsOptions} FROM SERVER "${remoteServer.foreignDataWrapperId}" INTO "${tmpSchemaName}"`, + await entityManager.query( + `IMPORT FOREIGN SCHEMA "${remoteServer.schema}"${tableLimitationsOptions} FROM SERVER "${remoteServer.foreignDataWrapperId}" INTO "${tmpSchemaName}"`, + ); + + const createdForeignTableNames = await entityManager.query( + `SELECT table_name, column_name, data_type, udt_name FROM information_schema.columns WHERE table_schema = '${tmpSchemaName}'`, + ); + + await entityManager.query(`DROP SCHEMA "${tmpSchemaName}" CASCADE`); + + return createdForeignTableNames.reduce( + (acc, { table_name, column_name, data_type, udt_name }) => { + if (!acc[table_name]) { + acc[table_name] = []; + } + + acc[table_name].push({ + columnName: column_name, + dataType: data_type, + udtName: udt_name, + }); + + return acc; + }, + {}, + ); + }, + ); + + return distantTables; + } catch (error) { + if (isQueryTimeoutError(error)) { + throw new RequestTimeoutException( + `Could not find distant tables: ${error.message}`, ); + } - const createdForeignTableNames = await entityManager.query( - `SELECT table_name, column_name, data_type, udt_name FROM information_schema.columns WHERE table_schema = '${tmpSchemaName}'`, - ); - - await entityManager.query(`DROP SCHEMA "${tmpSchemaName}" CASCADE`); - - return createdForeignTableNames.reduce( - (acc, { table_name, column_name, data_type, udt_name }) => { - if (!acc[table_name]) { - acc[table_name] = []; - } - - acc[table_name].push({ - columnName: column_name, - dataType: data_type, - udtName: udt_name, - }); - - return acc; - }, - {}, - ); - }, - ); - - return distantTables; + throw error; + } } private getDistantTablesFromStaticSchema( diff --git a/packages/twenty-server/src/engine/utils/global-exception-handler.util.ts b/packages/twenty-server/src/engine/utils/global-exception-handler.util.ts index 4a6dac8d81..66878862ed 100644 --- a/packages/twenty-server/src/engine/utils/global-exception-handler.util.ts +++ b/packages/twenty-server/src/engine/utils/global-exception-handler.util.ts @@ -10,6 +10,7 @@ import { NotFoundError, ConflictError, MethodNotAllowedError, + TimeoutError, } from 'src/engine/utils/graphql-errors.util'; import { ExceptionHandlerService } from 'src/engine/integrations/exception-handler/exception-handler.service'; @@ -19,6 +20,7 @@ const graphQLPredefinedExceptions = { 403: ForbiddenError, 404: NotFoundError, 405: MethodNotAllowedError, + 408: TimeoutError, 409: ConflictError, }; @@ -32,7 +34,7 @@ export const handleExceptionAndConvertToGraphQLError = ( return convertExceptionToGraphQLError(exception); }; -export const filterException = (exception: Error): boolean => { +export const shouldFilterException = (exception: Error): boolean => { if (exception instanceof HttpException && exception.getStatus() < 500) { return true; } @@ -45,7 +47,7 @@ export const handleException = ( exceptionHandlerService: ExceptionHandlerService, user?: ExceptionHandlerUser, ): void => { - if (filterException(exception)) { + if (shouldFilterException(exception)) { return; } diff --git a/packages/twenty-server/src/engine/utils/graphql-errors.util.ts b/packages/twenty-server/src/engine/utils/graphql-errors.util.ts index 22733566de..d198a9e6ee 100644 --- a/packages/twenty-server/src/engine/utils/graphql-errors.util.ts +++ b/packages/twenty-server/src/engine/utils/graphql-errors.util.ts @@ -157,3 +157,11 @@ export class ConflictError extends BaseGraphQLError { Object.defineProperty(this, 'name', { value: 'ConflictError' }); } } + +export class TimeoutError extends BaseGraphQLError { + constructor(message: string, extensions?: Record) { + super(message, 'TIMEOUT', extensions); + + Object.defineProperty(this, 'name', { value: 'TimeoutError' }); + } +} diff --git a/packages/twenty-server/src/engine/utils/query-timeout.util.ts b/packages/twenty-server/src/engine/utils/query-timeout.util.ts new file mode 100644 index 0000000000..9e0a108133 --- /dev/null +++ b/packages/twenty-server/src/engine/utils/query-timeout.util.ts @@ -0,0 +1,3 @@ +export const isQueryTimeoutError = (error: Error) => { + return error.message.includes('Query read timeout'); +}; diff --git a/packages/twenty-server/src/queue-worker/queue-worker.ts b/packages/twenty-server/src/queue-worker/queue-worker.ts index 9fae386968..fd62872d47 100644 --- a/packages/twenty-server/src/queue-worker/queue-worker.ts +++ b/packages/twenty-server/src/queue-worker/queue-worker.ts @@ -5,7 +5,7 @@ import { MessageQueueJobData, } from 'src/engine/integrations/message-queue/interfaces/message-queue-job.interface'; -import { filterException } from 'src/engine/utils/global-exception-handler.util'; +import { shouldFilterException } from 'src/engine/utils/global-exception-handler.util'; import { ExceptionHandlerService } from 'src/engine/integrations/exception-handler/exception-handler.service'; import { LoggerService } from 'src/engine/integrations/logger/logger.service'; import { JobsModule } from 'src/engine/integrations/message-queue/jobs.module'; @@ -54,7 +54,7 @@ async function bootstrap() { } catch (err) { loggerService?.error(err?.message, err?.name); - if (!filterException(err)) { + if (!shouldFilterException(err)) { exceptionHandlerService?.captureExceptions([err]); }