From 756de8a31b5f74ff790fcea7e464a4a9cea2c3ec Mon Sep 17 00:00:00 2001 From: Thomas Trompette Date: Mon, 15 Apr 2024 14:09:01 +0200 Subject: [PATCH] Add connection failed status (#4939) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1/ When the user inputs wrong connection informations, we do not inform him. He will only see that no tables are available. We will display a connection failed status if an error is raised testing the connection 2/ If the connection fails, it should still be possible to delete the server. Today, since we try first to delete the tables, the connection failure throws an error that will prevent server deletion. Using the foreign tables instead of calling the distant DB. 3/ Redirect to connection show page instead of connection list after creation 4/ Today, foreign tables are fetched without the server name. This is a mistake because we need to know which foreign table is linked with which server. Updating the associated query. Capture d’écran 2024-04-12 à 10 52 49 --------- Co-authored-by: Thomas Trompette --- .../hooks/useGetDatabaseConnectionTables.ts | 3 +- .../object-record/components/RecordChip.tsx | 12 +- ...ntegrationDatabaseConnectionSyncStatus.tsx | 7 +- .../object-record/RecordIndexPageHeader.tsx | 5 +- ...ttingsIntegrationNewDatabaseConnection.tsx | 8 +- .../object-metadata.service.ts | 2 +- .../remote-server/remote-server.service.ts | 22 +-- .../remote-table/remote-table.service.ts | 179 ++++++++++-------- ...ts => get-remote-table-local-name.util.ts} | 2 +- .../utils/validate-remote-server-input.ts | 2 +- .../workspace-migration.entity.ts | 2 +- .../display/chip/components/EntityChip.tsx | 1 - 12 files changed, 144 insertions(+), 101 deletions(-) rename packages/twenty-server/src/engine/metadata-modules/remote-server/remote-table/utils/{get-remote-table-name.util.ts => get-remote-table-local-name.util.ts} (57%) diff --git a/packages/twenty-front/src/modules/databases/hooks/useGetDatabaseConnectionTables.ts b/packages/twenty-front/src/modules/databases/hooks/useGetDatabaseConnectionTables.ts index 6548222e6..dd50582bb 100644 --- a/packages/twenty-front/src/modules/databases/hooks/useGetDatabaseConnectionTables.ts +++ b/packages/twenty-front/src/modules/databases/hooks/useGetDatabaseConnectionTables.ts @@ -18,7 +18,7 @@ export const useGetDatabaseConnectionTables = ({ }: UseGetDatabaseConnectionTablesParams) => { const apolloMetadataClient = useApolloMetadataClient(); - const { data } = useQuery< + const { data, error } = useQuery< GetManyRemoteTablesQuery, GetManyRemoteTablesQueryVariables >(GET_MANY_REMOTE_TABLES, { @@ -33,5 +33,6 @@ export const useGetDatabaseConnectionTables = ({ return { tables: data?.findAvailableRemoteTablesByServerId || [], + error, }; }; diff --git a/packages/twenty-front/src/modules/object-record/components/RecordChip.tsx b/packages/twenty-front/src/modules/object-record/components/RecordChip.tsx index 1c167e194..60eeea769 100644 --- a/packages/twenty-front/src/modules/object-record/components/RecordChip.tsx +++ b/packages/twenty-front/src/modules/object-record/components/RecordChip.tsx @@ -1,6 +1,8 @@ +import { useContext } from 'react'; import { EntityChip } from 'twenty-ui'; import { useMapToObjectRecordIdentifier } from '@/object-metadata/hooks/useMapToObjectRecordIdentifier'; +import { RecordTableRowContext } from '@/object-record/record-table/contexts/RecordTableRowContext'; import { ObjectRecord } from '@/object-record/types/ObjectRecord'; import { getImageAbsoluteURIOrBase64 } from '~/utils/image/getImageAbsoluteURIOrBase64'; @@ -21,8 +23,16 @@ export const RecordChip = ({ objectNameSingular, }); + // Will only exists if the chip is inside a record table. + // This is temporary until we have the show page for remote objects. + const { isReadOnly } = useContext(RecordTableRowContext); + const objectRecordIdentifier = mapToObjectRecordIdentifier(record); + const linkToEntity = isReadOnly + ? undefined + : objectRecordIdentifier.linkToShowPage; + return ( diff --git a/packages/twenty-front/src/modules/settings/integrations/components/SettingsIntegrationDatabaseConnectionSyncStatus.tsx b/packages/twenty-front/src/modules/settings/integrations/components/SettingsIntegrationDatabaseConnectionSyncStatus.tsx index a13aede90..b50a048a2 100644 --- a/packages/twenty-front/src/modules/settings/integrations/components/SettingsIntegrationDatabaseConnectionSyncStatus.tsx +++ b/packages/twenty-front/src/modules/settings/integrations/components/SettingsIntegrationDatabaseConnectionSyncStatus.tsx @@ -1,6 +1,7 @@ import { useGetDatabaseConnectionTables } from '@/databases/hooks/useGetDatabaseConnectionTables'; import { Status } from '@/ui/display/status/components/Status'; import { RemoteTableStatus } from '~/generated-metadata/graphql'; +import { isDefined } from '~/utils/isDefined'; type SettingsIntegrationDatabaseConnectionSyncStatusProps = { connectionId: string; @@ -11,11 +12,15 @@ export const SettingsIntegrationDatabaseConnectionSyncStatus = ({ connectionId, skip, }: SettingsIntegrationDatabaseConnectionSyncStatusProps) => { - const { tables } = useGetDatabaseConnectionTables({ + const { tables, error } = useGetDatabaseConnectionTables({ connectionId, skip, }); + if (isDefined(error)) { + return ; + } + const syncedTables = tables.filter( (table) => table.status === RemoteTableStatus.Synced, ); diff --git a/packages/twenty-front/src/pages/object-record/RecordIndexPageHeader.tsx b/packages/twenty-front/src/pages/object-record/RecordIndexPageHeader.tsx index f92acab85..5039ec7a3 100644 --- a/packages/twenty-front/src/pages/object-record/RecordIndexPageHeader.tsx +++ b/packages/twenty-front/src/pages/object-record/RecordIndexPageHeader.tsx @@ -35,8 +35,11 @@ export const RecordIndexPageHeader = ({ const canAddRecord = recordIndexViewType === ViewType.Table && !objectMetadataItem?.isRemote; + const pageHeaderTitle = + objectMetadataItem?.labelPlural ?? capitalize(objectNamePlural); + return ( - + {canAddRecord && } diff --git a/packages/twenty-front/src/pages/settings/integrations/SettingsIntegrationNewDatabaseConnection.tsx b/packages/twenty-front/src/pages/settings/integrations/SettingsIntegrationNewDatabaseConnection.tsx index d8b27084d..fba7e9885 100644 --- a/packages/twenty-front/src/pages/settings/integrations/SettingsIntegrationNewDatabaseConnection.tsx +++ b/packages/twenty-front/src/pages/settings/integrations/SettingsIntegrationNewDatabaseConnection.tsx @@ -95,14 +95,18 @@ export const SettingsIntegrationNewDatabaseConnection = () => { const formValues = formConfig.getValues(); try { - await createOneDatabaseConnection( + const createdConnection = await createOneDatabaseConnection( createRemoteServerInputSchema.parse({ ...formValues, foreignDataWrapperType: getForeignDataWrapperType(databaseKey), }), ); - navigate(`${settingsIntegrationsPagePath}/${databaseKey}`); + const connectionId = createdConnection.data?.createOneRemoteServer.id; + + navigate( + `${settingsIntegrationsPagePath}/${databaseKey}/${connectionId}`, + ); } catch (error) { enqueueSnackBar((error as Error).message, { variant: 'error', diff --git a/packages/twenty-server/src/engine/metadata-modules/object-metadata/object-metadata.service.ts b/packages/twenty-server/src/engine/metadata-modules/object-metadata/object-metadata.service.ts index f9e92d284..1f9f239ec 100644 --- a/packages/twenty-server/src/engine/metadata-modules/object-metadata/object-metadata.service.ts +++ b/packages/twenty-server/src/engine/metadata-modules/object-metadata/object-metadata.service.ts @@ -361,7 +361,7 @@ export class ObjectMetadataService extends TypeOrmQueryService { @@ -117,21 +116,18 @@ export class RemoteServerService { throw new NotFoundException('Object does not exist'); } - const remoteTablesToRemove = ( - await this.remoteTableService.findAvailableRemoteTablesByServerId( - id, + const foreignTablesToRemove = + await this.remoteTableService.fetchForeignTableNamesWithinWorkspace( workspaceId, - ) - ).filter((remoteTable) => remoteTable.status === RemoteTableStatus.SYNCED); + remoteServer.foreignDataWrapperId, + ); - if (remoteTablesToRemove.length) { - for (const remoteTable of remoteTablesToRemove) { - await this.remoteTableService.unsyncRemoteTable( - { - remoteServerId: id, - name: remoteTable.name, - }, + if (foreignTablesToRemove.length) { + for (const foreignTableName of foreignTablesToRemove) { + await this.remoteTableService.removeForeignTableAndMetadata( + foreignTableName, workspaceId, + remoteServer, ); } } diff --git a/packages/twenty-server/src/engine/metadata-modules/remote-server/remote-table/remote-table.service.ts b/packages/twenty-server/src/engine/metadata-modules/remote-server/remote-table/remote-table.service.ts index ce9a8a392..d2279380d 100644 --- a/packages/twenty-server/src/engine/metadata-modules/remote-server/remote-table/remote-table.service.ts +++ b/packages/twenty-server/src/engine/metadata-modules/remote-server/remote-table/remote-table.service.ts @@ -26,7 +26,7 @@ import { RemotePostgresTableService } from 'src/engine/metadata-modules/remote-s import { WorkspaceCacheVersionService } from 'src/engine/metadata-modules/workspace-cache-version/workspace-cache-version.service'; import { camelCase } from 'src/utils/camel-case'; import { camelToTitleCase } from 'src/utils/camel-to-title-case'; -import { getRemoteTableName } from 'src/engine/metadata-modules/remote-server/remote-table/utils/get-remote-table-name.util'; +import { getRemoteTableLocalName } from 'src/engine/metadata-modules/remote-server/remote-table/utils/get-remote-table-local-name.util'; import { WorkspaceMigrationService } from 'src/engine/metadata-modules/workspace-migration/workspace-migration.service'; import { WorkspaceMigrationRunnerService } from 'src/engine/workspace-manager/workspace-migration-runner/workspace-migration-runner.service'; import { generateMigrationName } from 'src/engine/metadata-modules/workspace-migration/utils/generate-migration-name.util'; @@ -73,7 +73,10 @@ export class RemoteTableService { } const currentForeignTableNames = - await this.fetchForeignTableNamesWithinWorkspace(workspaceId); + await this.fetchForeignTableNamesWithinWorkspace( + workspaceId, + remoteServer.foreignDataWrapperId, + ); const tableInRemoteSchema = await this.fetchTablesFromRemoteSchema(remoteServer); @@ -82,7 +85,7 @@ export class RemoteTableService { name: remoteTable.tableName, schema: remoteTable.tableSchema, status: currentForeignTableNames.includes( - getRemoteTableName(remoteTable.tableName), + getRemoteTableLocalName(remoteTable.tableName), ) ? RemoteTableStatus.SYNCED : RemoteTableStatus.NOT_SYNCED, @@ -107,20 +110,95 @@ export class RemoteTableService { workspaceId, ); - await this.workspaceCacheVersionService.incrementVersion(workspaceId); - return remoteTable; } public async unsyncRemoteTable(input: RemoteTableInput, workspaceId: string) { - const remoteTable = await this.removeForeignTableAndMetadata( - input, + const remoteServer = await this.remoteServerRepository.findOne({ + where: { + id: input.remoteServerId, + workspaceId, + }, + }); + + if (!remoteServer) { + throw new NotFoundException('Remote server does not exist'); + } + + const remoteTableLocalName = getRemoteTableLocalName(input.name); + + await this.removeForeignTableAndMetadata( + remoteTableLocalName, + workspaceId, + remoteServer, + ); + + return { + name: input.name, + schema: input.schema, + status: RemoteTableStatus.NOT_SYNCED, + }; + } + + public async fetchForeignTableNamesWithinWorkspace( + workspaceId: string, + foreignDataWrapperId: string, + ): Promise { + const workspaceDataSource = + await this.workspaceDataSourceService.connectToWorkspaceDataSource( + workspaceId, + ); + + return ( + await workspaceDataSource.query( + `SELECT foreign_table_name, foreign_server_name FROM information_schema.foreign_tables WHERE foreign_server_name = '${foreignDataWrapperId}'`, + ) + ).map((foreignTable) => foreignTable.foreign_table_name); + } + + public async removeForeignTableAndMetadata( + remoteTableLocalName: string, + workspaceId: string, + remoteServer: RemoteServerEntity, + ) { + const currentForeignTableNames = + await this.fetchForeignTableNamesWithinWorkspace( + workspaceId, + remoteServer.foreignDataWrapperId, + ); + + if (!currentForeignTableNames.includes(remoteTableLocalName)) { + throw new Error('Remote table does not exist'); + } + + const objectMetadata = + await this.objectMetadataService.findOneWithinWorkspace(workspaceId, { + where: { nameSingular: remoteTableLocalName }, + }); + + if (objectMetadata) { + await this.objectMetadataService.deleteOneObject( + { id: objectMetadata.id }, + workspaceId, + ); + } + + await this.workspaceMigrationService.createCustomMigration( + generateMigrationName(`drop-foreign-table-${remoteTableLocalName}`), + workspaceId, + [ + { + name: remoteTableLocalName, + action: WorkspaceMigrationTableActionType.DROP_FOREIGN_TABLE, + }, + ], + ); + + await this.workspaceMigrationRunnerService.executeMigrationFromPendingMigrations( workspaceId, ); await this.workspaceCacheVersionService.incrementVersion(workspaceId); - - return remoteTable; } private async createForeignTableAndMetadata( @@ -133,9 +211,14 @@ export class RemoteTableService { } const currentForeignTableNames = - await this.fetchForeignTableNamesWithinWorkspace(workspaceId); + await this.fetchForeignTableNamesWithinWorkspace( + workspaceId, + remoteServer.foreignDataWrapperId, + ); - if (currentForeignTableNames.includes(getRemoteTableName(input.name))) { + if ( + currentForeignTableNames.includes(getRemoteTableLocalName(input.name)) + ) { throw new Error('Remote table already exists'); } @@ -145,8 +228,8 @@ export class RemoteTableService { input.schema, ); - const remoteTableName = getRemoteTableName(input.name); - const remoteTableLabel = camelToTitleCase(remoteTableName); + const remoteTableLocalName = getRemoteTableLocalName(input.name); + const remoteTableLabel = camelToTitleCase(remoteTableLocalName); // We only support remote tables with an id column for now. const remoteTableIdColumn = remoteTableColumns.filter( @@ -163,11 +246,11 @@ export class RemoteTableService { ); await this.workspaceMigrationService.createCustomMigration( - generateMigrationName(`create-foreign-table-${remoteTableName}`), + generateMigrationName(`create-foreign-table-${remoteTableLocalName}`), workspaceId, [ { - name: remoteTableName, + name: remoteTableLocalName, action: WorkspaceMigrationTableActionType.CREATE_FOREIGN_TABLE, foreignTable: { columns: remoteTableColumns.map( @@ -190,8 +273,8 @@ export class RemoteTableService { ); const objectMetadata = await this.objectMetadataService.createOne({ - nameSingular: remoteTableName, - namePlural: `${remoteTableName}s`, + nameSingular: remoteTableLocalName, + namePlural: `${remoteTableLocalName}s`, labelSingular: remoteTableLabel, labelPlural: `${remoteTableLabel}s`, description: 'Remote table', @@ -223,6 +306,8 @@ export class RemoteTableService { } } + await this.workspaceCacheVersionService.incrementVersion(workspaceId); + return { name: input.name, schema: input.schema, @@ -230,53 +315,6 @@ export class RemoteTableService { }; } - private async removeForeignTableAndMetadata( - input: RemoteTableInput, - workspaceId: string, - ): Promise { - const remoteTableName = getRemoteTableName(input.name); - - const currentForeignTableNames = - await this.fetchForeignTableNamesWithinWorkspace(workspaceId); - - if (!currentForeignTableNames.includes(remoteTableName)) { - throw new Error('Remote table does not exist'); - } - - const objectMetadata = - await this.objectMetadataService.findOneWithinWorkspace(workspaceId, { - where: { nameSingular: remoteTableName }, - }); - - if (objectMetadata) { - await this.objectMetadataService.deleteOneObject( - { id: objectMetadata.id }, - workspaceId, - ); - } - - await this.workspaceMigrationService.createCustomMigration( - generateMigrationName(`drop-foreign-table-${input.name}`), - workspaceId, - [ - { - name: remoteTableName, - action: WorkspaceMigrationTableActionType.DROP_FOREIGN_TABLE, - }, - ], - ); - - await this.workspaceMigrationRunnerService.executeMigrationFromPendingMigrations( - workspaceId, - ); - - return { - name: input.name, - schema: input.schema, - status: RemoteTableStatus.NOT_SYNCED, - }; - } - private async fetchTableColumnsSchema( remoteServer: RemoteServerEntity, tableName: string, @@ -299,19 +337,6 @@ export class RemoteTableService { } } - private async fetchForeignTableNamesWithinWorkspace(workspaceId: string) { - const workspaceDataSource = - await this.workspaceDataSourceService.connectToWorkspaceDataSource( - workspaceId, - ); - - return ( - await workspaceDataSource.query( - `SELECT foreign_table_name FROM information_schema.foreign_tables`, - ) - ).map((foreignTable) => foreignTable.foreign_table_name); - } - private async fetchTablesFromRemoteSchema( remoteServer: RemoteServerEntity, ): Promise { diff --git a/packages/twenty-server/src/engine/metadata-modules/remote-server/remote-table/utils/get-remote-table-name.util.ts b/packages/twenty-server/src/engine/metadata-modules/remote-server/remote-table/utils/get-remote-table-local-name.util.ts similarity index 57% rename from packages/twenty-server/src/engine/metadata-modules/remote-server/remote-table/utils/get-remote-table-name.util.ts rename to packages/twenty-server/src/engine/metadata-modules/remote-server/remote-table/utils/get-remote-table-local-name.util.ts index bcc7a2123..a7162c4c5 100644 --- a/packages/twenty-server/src/engine/metadata-modules/remote-server/remote-table/utils/get-remote-table-name.util.ts +++ b/packages/twenty-server/src/engine/metadata-modules/remote-server/remote-table/utils/get-remote-table-local-name.util.ts @@ -1,4 +1,4 @@ import { camelCase } from 'src/utils/camel-case'; -export const getRemoteTableName = (distantTableName: string) => +export const getRemoteTableLocalName = (distantTableName: string) => `${camelCase(distantTableName)}Remote`; diff --git a/packages/twenty-server/src/engine/metadata-modules/remote-server/utils/validate-remote-server-input.ts b/packages/twenty-server/src/engine/metadata-modules/remote-server/utils/validate-remote-server-input.ts index 5c043f2a9..e9cc3e542 100644 --- a/packages/twenty-server/src/engine/metadata-modules/remote-server/utils/validate-remote-server-input.ts +++ b/packages/twenty-server/src/engine/metadata-modules/remote-server/utils/validate-remote-server-input.ts @@ -1,4 +1,4 @@ -const INPUT_REGEX = /^([A-Za-z0-9\-\_\.]+)$/; +const INPUT_REGEX = /^([A-Za-z0-9\-_.@]+)$/; export const validateObject = (input: object) => { for (const [key, value] of Object.entries(input)) { diff --git a/packages/twenty-server/src/engine/metadata-modules/workspace-migration/workspace-migration.entity.ts b/packages/twenty-server/src/engine/metadata-modules/workspace-migration/workspace-migration.entity.ts index 6acee6df2..224eaf4e3 100644 --- a/packages/twenty-server/src/engine/metadata-modules/workspace-migration/workspace-migration.entity.ts +++ b/packages/twenty-server/src/engine/metadata-modules/workspace-migration/workspace-migration.entity.ts @@ -88,7 +88,7 @@ export enum WorkspaceMigrationTableActionType { ALTER = 'alter', DROP = 'drop', CREATE_FOREIGN_TABLE = 'create_foreign_table', - DROP_FOREIGN_TABLE = 'drop_foreign_table' + DROP_FOREIGN_TABLE = 'drop_foreign_table', } export type WorkspaceMigrationTableAction = { diff --git a/packages/twenty-ui/src/display/chip/components/EntityChip.tsx b/packages/twenty-ui/src/display/chip/components/EntityChip.tsx index 246782496..740875fe5 100644 --- a/packages/twenty-ui/src/display/chip/components/EntityChip.tsx +++ b/packages/twenty-ui/src/display/chip/components/EntityChip.tsx @@ -38,7 +38,6 @@ export const EntityChip = ({ maxWidth, }: EntityChipProps) => { const navigate = useNavigate(); - const theme = useTheme(); const handleLinkClick = (event: React.MouseEvent) => {