From 3acec7731c95e84d9aeb9b9c63565b1e5e3465a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9my=20M?= Date: Tue, 26 Mar 2024 16:16:29 +0100 Subject: [PATCH] Fix/enum bug (#4659) * fix: sever not throwing when enum contains two identical values * fix: enum column name cannot be change * fix: put field create/update inside transactions * fix: check for options duplicate values front-end * fix: missing commit transaction --- .../utils/formatFieldMetadataItemInput.ts | 16 + .../field-metadata/field-metadata.service.ts | 469 ++++++++++-------- .../utils/validate-options-for-type.util.ts | 9 +- .../is-field-metadata-options.validator.ts | 14 +- .../workspace-migration-enum.service.ts | 27 + 5 files changed, 329 insertions(+), 206 deletions(-) diff --git a/packages/twenty-front/src/modules/object-metadata/utils/formatFieldMetadataItemInput.ts b/packages/twenty-front/src/modules/object-metadata/utils/formatFieldMetadataItemInput.ts index 805726352..1174b5e0b 100644 --- a/packages/twenty-front/src/modules/object-metadata/utils/formatFieldMetadataItemInput.ts +++ b/packages/twenty-front/src/modules/object-metadata/utils/formatFieldMetadataItemInput.ts @@ -26,6 +26,22 @@ export const formatFieldMetadataItemInput = ( ) => { const defaultOption = input.options?.find((option) => option.isDefault); + // Check if options has unique values + if (input.options !== undefined) { + // Compute the values based on the label + const values = input.options.map((option) => + getOptionValueFromLabel(option.label), + ); + + if (new Set(values).size !== input.options.length) { + throw new Error( + `Options must have unique values, but contains the following duplicates ${values.join( + ', ', + )}`, + ); + } + } + return { defaultValue: defaultOption ? getOptionValueFromLabel(defaultOption.label) diff --git a/packages/twenty-server/src/engine/metadata-modules/field-metadata/field-metadata.service.ts b/packages/twenty-server/src/engine/metadata-modules/field-metadata/field-metadata.service.ts index de36dd302..d36c7cc97 100644 --- a/packages/twenty-server/src/engine/metadata-modules/field-metadata/field-metadata.service.ts +++ b/packages/twenty-server/src/engine/metadata-modules/field-metadata/field-metadata.service.ts @@ -4,10 +4,10 @@ import { Injectable, NotFoundException, } from '@nestjs/common'; -import { InjectRepository } from '@nestjs/typeorm'; +import { InjectDataSource, InjectRepository } from '@nestjs/typeorm'; import { v4 as uuidV4 } from 'uuid'; -import { FindOneOptions, Repository } from 'typeorm'; +import { DataSource, FindOneOptions, Repository } from 'typeorm'; import { TypeOrmQueryService } from '@ptc-org/nestjs-query-typeorm'; import { WorkspaceMigrationRunnerService } from 'src/engine/workspace-manager/workspace-migration-runner/workspace-migration-runner.service'; @@ -48,6 +48,8 @@ import { generateDefaultValue } from './utils/generate-default-value'; @Injectable() export class FieldMetadataService extends TypeOrmQueryService { constructor( + @InjectDataSource('metadata') + private readonly metadataDataSource: DataSource, @InjectRepository(FieldMetadataEntity, 'metadata') private readonly fieldMetadataRepository: Repository, @InjectRepository(RelationMetadataEntity, 'metadata') @@ -65,231 +67,290 @@ export class FieldMetadataService extends TypeOrmQueryService { - const objectMetadata = - await this.objectMetadataService.findOneWithinWorkspace( - fieldMetadataInput.workspaceId, - { - where: { - id: fieldMetadataInput.objectMetadataId, + const queryRunner = this.metadataDataSource.createQueryRunner(); + + await queryRunner.connect(); + await queryRunner.startTransaction(); + + try { + const fieldMetadataRepository = + queryRunner.manager.getRepository>( + FieldMetadataEntity, + ); + const objectMetadata = + await this.objectMetadataService.findOneWithinWorkspace( + fieldMetadataInput.workspaceId, + { + where: { + id: fieldMetadataInput.objectMetadataId, + }, }, - }, - ); + ); - if (!objectMetadata) { - throw new NotFoundException('Object does not exist'); - } - - // Double check in case the service is directly called - if (isEnumFieldMetadataType(fieldMetadataInput.type)) { - if ( - !fieldMetadataInput.options && - fieldMetadataInput.type !== FieldMetadataType.RATING - ) { - throw new BadRequestException('Options are required for enum fields'); + if (!objectMetadata) { + throw new NotFoundException('Object does not exist'); } - } - // Generate options for rating fields - if (fieldMetadataInput.type === FieldMetadataType.RATING) { - fieldMetadataInput.options = generateRatingOptions(); - } - - const fieldAlreadyExists = await this.fieldMetadataRepository.findOne({ - where: { - name: fieldMetadataInput.name, - objectMetadataId: fieldMetadataInput.objectMetadataId, - workspaceId: fieldMetadataInput.workspaceId, - }, - }); - - if (fieldAlreadyExists) { - throw new ConflictException('Field already exists'); - } - - const createdFieldMetadata = await super.createOne({ - ...fieldMetadataInput, - targetColumnMap: generateTargetColumnMap( - fieldMetadataInput.type, - true, - fieldMetadataInput.name, - ), - isNullable: generateNullable( - fieldMetadataInput.type, - fieldMetadataInput.isNullable, - ), - defaultValue: - fieldMetadataInput.defaultValue ?? - generateDefaultValue(fieldMetadataInput.type), - options: fieldMetadataInput.options - ? fieldMetadataInput.options.map((option) => ({ - ...option, - id: uuidV4(), - })) - : undefined, - isActive: true, - isCustom: true, - }); - - await this.workspaceMigrationService.createCustomMigration( - generateMigrationName(`create-${createdFieldMetadata.name}`), - fieldMetadataInput.workspaceId, - [ - { - name: computeObjectTargetTable(objectMetadata), - action: 'alter', - columns: this.workspaceMigrationFactory.createColumnActions( - WorkspaceMigrationColumnActionType.CREATE, - createdFieldMetadata, - ), - } satisfies WorkspaceMigrationTableAction, - ], - ); - - await this.workspaceMigrationRunnerService.executeMigrationFromPendingMigrations( - fieldMetadataInput.workspaceId, - ); - - // TODO: Move viewField creation to a cdc scheduler - const dataSourceMetadata = - await this.dataSourceService.getLastDataSourceMetadataFromWorkspaceIdOrFail( - fieldMetadataInput.workspaceId, - ); - - const workspaceDataSource = - await this.typeORMService.connectToDataSource(dataSourceMetadata); - - // TODO: use typeorm repository - const view = await workspaceDataSource?.query( - `SELECT id FROM ${dataSourceMetadata.schema}."view" - WHERE "objectMetadataId" = '${createdFieldMetadata.objectMetadataId}'`, - ); - - const existingViewFields = await workspaceDataSource?.query( - `SELECT * FROM ${dataSourceMetadata.schema}."viewField" - WHERE "viewId" = '${view[0].id}'`, - ); - - const lastPosition = existingViewFields - .map((viewField) => viewField.position) - .reduce((acc, position) => { - if (position > acc) { - return position; - } - - return acc; - }, -1); - - await workspaceDataSource?.query( - `INSERT INTO ${dataSourceMetadata.schema}."viewField" - ("fieldMetadataId", "position", "isVisible", "size", "viewId") - VALUES ('${createdFieldMetadata.id}', '${lastPosition + 1}', true, 180, '${ - view[0].id - }')`, - ); - - return createdFieldMetadata; - } - - override async updateOne( - id: string, - fieldMetadataInput: UpdateFieldInput, - ): Promise { - const existingFieldMetadata = await this.fieldMetadataRepository.findOne({ - where: { - id, - workspaceId: fieldMetadataInput.workspaceId, - }, - }); - - if (!existingFieldMetadata) { - throw new NotFoundException('Field does not exist'); - } - - const objectMetadata = - await this.objectMetadataService.findOneWithinWorkspace( - fieldMetadataInput.workspaceId, - { - where: { - id: existingFieldMetadata?.objectMetadataId, - }, - }, - ); - - if (!objectMetadata) { - throw new NotFoundException('Object does not exist'); - } - - if ( - objectMetadata.labelIdentifierFieldMetadataId === - existingFieldMetadata.id && - fieldMetadataInput.isActive === false - ) { - throw new BadRequestException('Cannot deactivate label identifier field'); - } - - if (fieldMetadataInput.options) { - for (const option of fieldMetadataInput.options) { - if (!option.id) { - throw new BadRequestException('Option id is required'); + // Double check in case the service is directly called + if (isEnumFieldMetadataType(fieldMetadataInput.type)) { + if ( + !fieldMetadataInput.options && + fieldMetadataInput.type !== FieldMetadataType.RATING + ) { + throw new BadRequestException('Options are required for enum fields'); } } - } - const updatableFieldInput = - existingFieldMetadata.isCustom === false - ? this.buildUpdatableStandardFieldInput( - fieldMetadataInput, - existingFieldMetadata, - ) - : fieldMetadataInput; + // Generate options for rating fields + if (fieldMetadataInput.type === FieldMetadataType.RATING) { + fieldMetadataInput.options = generateRatingOptions(); + } - const updatedFieldMetadata = await super.updateOne(id, { - ...updatableFieldInput, - defaultValue: - // Todo: we need to handle default value for all field types. Right now we are only allowing update for SELECt - existingFieldMetadata.type !== FieldMetadataType.SELECT - ? existingFieldMetadata.defaultValue - : updatableFieldInput.defaultValue - ? // Todo: we need to rework DefaultValue typing and format to be simpler, there is no need to have this complexity - { value: updatableFieldInput.defaultValue as unknown as string } - : null, - // If the name is updated, the targetColumnMap should be updated as well - targetColumnMap: updatableFieldInput.name - ? generateTargetColumnMap( - existingFieldMetadata.type, - existingFieldMetadata.isCustom, - updatableFieldInput.name, - ) - : existingFieldMetadata.targetColumnMap, - }); + const fieldAlreadyExists = await fieldMetadataRepository.findOne({ + where: { + name: fieldMetadataInput.name, + objectMetadataId: fieldMetadataInput.objectMetadataId, + workspaceId: fieldMetadataInput.workspaceId, + }, + }); + + if (fieldAlreadyExists) { + throw new ConflictException('Field already exists'); + } + + const createdFieldMetadata = await fieldMetadataRepository.save({ + ...fieldMetadataInput, + targetColumnMap: generateTargetColumnMap( + fieldMetadataInput.type, + true, + fieldMetadataInput.name, + ), + isNullable: generateNullable( + fieldMetadataInput.type, + fieldMetadataInput.isNullable, + ), + defaultValue: + fieldMetadataInput.defaultValue ?? + generateDefaultValue(fieldMetadataInput.type), + options: fieldMetadataInput.options + ? fieldMetadataInput.options.map((option) => ({ + ...option, + id: uuidV4(), + })) + : undefined, + isActive: true, + isCustom: true, + }); - if ( - fieldMetadataInput.name || - updatableFieldInput.options || - updatableFieldInput.defaultValue - ) { await this.workspaceMigrationService.createCustomMigration( - generateMigrationName(`update-${updatedFieldMetadata.name}`), - existingFieldMetadata.workspaceId, + generateMigrationName(`create-${createdFieldMetadata.name}`), + fieldMetadataInput.workspaceId, [ { name: computeObjectTargetTable(objectMetadata), action: 'alter', columns: this.workspaceMigrationFactory.createColumnActions( - WorkspaceMigrationColumnActionType.ALTER, - existingFieldMetadata, - updatedFieldMetadata, + WorkspaceMigrationColumnActionType.CREATE, + createdFieldMetadata, ), } satisfies WorkspaceMigrationTableAction, ], ); await this.workspaceMigrationRunnerService.executeMigrationFromPendingMigrations( - updatedFieldMetadata.workspaceId, + fieldMetadataInput.workspaceId, ); - } - return updatedFieldMetadata; + // TODO: Move viewField creation to a cdc scheduler + const dataSourceMetadata = + await this.dataSourceService.getLastDataSourceMetadataFromWorkspaceIdOrFail( + fieldMetadataInput.workspaceId, + ); + + const workspaceDataSource = + await this.typeORMService.connectToDataSource(dataSourceMetadata); + + const workspaceQueryRunner = workspaceDataSource?.createQueryRunner(); + + if (!workspaceQueryRunner) { + throw new Error('Could not create workspace query runner'); + } + + await workspaceQueryRunner.connect(); + await workspaceQueryRunner.startTransaction(); + + try { + // TODO: use typeorm repository + const view = await workspaceQueryRunner?.query( + `SELECT id FROM ${dataSourceMetadata.schema}."view" + WHERE "objectMetadataId" = '${createdFieldMetadata.objectMetadataId}'`, + ); + + const existingViewFields = await workspaceQueryRunner?.query( + `SELECT * FROM ${dataSourceMetadata.schema}."viewField" + WHERE "viewId" = '${view[0].id}'`, + ); + + const lastPosition = existingViewFields + .map((viewField) => viewField.position) + .reduce((acc, position) => { + if (position > acc) { + return position; + } + + return acc; + }, -1); + + await workspaceQueryRunner?.query( + `INSERT INTO ${dataSourceMetadata.schema}."viewField" + ("fieldMetadataId", "position", "isVisible", "size", "viewId") + VALUES ('${createdFieldMetadata.id}', '${lastPosition + 1}', true, 180, '${ + view[0].id + }')`, + ); + } catch (error) { + await workspaceQueryRunner.rollbackTransaction(); + throw error; + } finally { + await workspaceQueryRunner.release(); + } + + await queryRunner.commitTransaction(); + + return createdFieldMetadata; + } catch (error) { + await queryRunner.rollbackTransaction(); + throw error; + } finally { + await queryRunner.release(); + } + } + + override async updateOne( + id: string, + fieldMetadataInput: UpdateFieldInput, + ): Promise { + const queryRunner = this.metadataDataSource.createQueryRunner(); + + await queryRunner.connect(); + await queryRunner.startTransaction(); + + try { + const fieldMetadataRepository = + queryRunner.manager.getRepository>( + FieldMetadataEntity, + ); + + const existingFieldMetadata = await fieldMetadataRepository.findOne({ + where: { + id, + workspaceId: fieldMetadataInput.workspaceId, + }, + }); + + if (!existingFieldMetadata) { + throw new NotFoundException('Field does not exist'); + } + + const objectMetadata = + await this.objectMetadataService.findOneWithinWorkspace( + fieldMetadataInput.workspaceId, + { + where: { + id: existingFieldMetadata?.objectMetadataId, + }, + }, + ); + + if (!objectMetadata) { + throw new NotFoundException('Object does not exist'); + } + + if ( + objectMetadata.labelIdentifierFieldMetadataId === + existingFieldMetadata.id && + fieldMetadataInput.isActive === false + ) { + throw new BadRequestException( + 'Cannot deactivate label identifier field', + ); + } + + if (fieldMetadataInput.options) { + for (const option of fieldMetadataInput.options) { + if (!option.id) { + throw new BadRequestException('Option id is required'); + } + } + } + + const updatableFieldInput = + existingFieldMetadata.isCustom === false + ? this.buildUpdatableStandardFieldInput( + fieldMetadataInput, + existingFieldMetadata, + ) + : fieldMetadataInput; + + // We're running field update under a transaction, so we can rollback if migration fails + await fieldMetadataRepository.update(id, { + ...updatableFieldInput, + defaultValue: + // Todo: we need to handle default value for all field types. Right now we are only allowing update for SELECt + existingFieldMetadata.type !== FieldMetadataType.SELECT + ? existingFieldMetadata.defaultValue + : updatableFieldInput.defaultValue + ? // Todo: we need to rework DefaultValue typing and format to be simpler, there is no need to have this complexity + { value: updatableFieldInput.defaultValue as unknown as string } + : null, + // If the name is updated, the targetColumnMap should be updated as well + targetColumnMap: updatableFieldInput.name + ? generateTargetColumnMap( + existingFieldMetadata.type, + existingFieldMetadata.isCustom, + updatableFieldInput.name, + ) + : existingFieldMetadata.targetColumnMap, + }); + const updatedFieldMetadata = await fieldMetadataRepository.findOneOrFail({ + where: { id }, + }); + + if ( + fieldMetadataInput.name || + updatableFieldInput.options || + updatableFieldInput.defaultValue + ) { + await this.workspaceMigrationService.createCustomMigration( + generateMigrationName(`update-${updatedFieldMetadata.name}`), + existingFieldMetadata.workspaceId, + [ + { + name: computeObjectTargetTable(objectMetadata), + action: 'alter', + columns: this.workspaceMigrationFactory.createColumnActions( + WorkspaceMigrationColumnActionType.ALTER, + existingFieldMetadata, + updatedFieldMetadata, + ), + } satisfies WorkspaceMigrationTableAction, + ], + ); + + await this.workspaceMigrationRunnerService.executeMigrationFromPendingMigrations( + updatedFieldMetadata.workspaceId, + ); + } + + await queryRunner.commitTransaction(); + + return updatedFieldMetadata; + } catch (error) { + await queryRunner.rollbackTransaction(); + throw error; + } finally { + await queryRunner.release(); + } } public async findOneOrFail( diff --git a/packages/twenty-server/src/engine/metadata-modules/field-metadata/utils/validate-options-for-type.util.ts b/packages/twenty-server/src/engine/metadata-modules/field-metadata/utils/validate-options-for-type.util.ts index f671d0ea0..7e119d594 100644 --- a/packages/twenty-server/src/engine/metadata-modules/field-metadata/utils/validate-options-for-type.util.ts +++ b/packages/twenty-server/src/engine/metadata-modules/field-metadata/utils/validate-options-for-type.util.ts @@ -24,7 +24,7 @@ export const validateOptionsForType = ( if (options === null) return true; if (!Array.isArray(options)) { - return false; + throw new Error('Options must be an array'); } if (!isEnumFieldMetadataType(type)) { @@ -35,6 +35,13 @@ export const validateOptionsForType = ( return true; } + const values = options.map(({ value }) => value); + + // Check if all options are unique + if (new Set(values).size !== options.length) { + throw new Error('Options must be unique'); + } + const validators = optionsValidatorsMap[type]; if (!validators) return false; diff --git a/packages/twenty-server/src/engine/metadata-modules/field-metadata/validators/is-field-metadata-options.validator.ts b/packages/twenty-server/src/engine/metadata-modules/field-metadata/validators/is-field-metadata-options.validator.ts index ecbb09153..110660d1e 100644 --- a/packages/twenty-server/src/engine/metadata-modules/field-metadata/validators/is-field-metadata-options.validator.ts +++ b/packages/twenty-server/src/engine/metadata-modules/field-metadata/validators/is-field-metadata-options.validator.ts @@ -14,6 +14,8 @@ import { validateOptionsForType } from 'src/engine/metadata-modules/field-metada @Injectable() @ValidatorConstraint({ name: 'isFieldMetadataOptions', async: true }) export class IsFieldMetadataOptions { + private validationErrors: string[] = []; + constructor(private readonly fieldMetadataService: FieldMetadataService) {} async validate( @@ -42,10 +44,20 @@ export class IsFieldMetadataOptions { type = fieldMetadata.type; } - return validateOptionsForType(type, value); + try { + return validateOptionsForType(type, value); + } catch (err) { + this.validationErrors.push(err.message); + + return false; + } } defaultMessage(): string { + if (this.validationErrors.length > 0) { + return this.validationErrors.join(', '); + } + return 'FieldMetadataOptions is not valid'; } } diff --git a/packages/twenty-server/src/engine/workspace-manager/workspace-migration-runner/services/workspace-migration-enum.service.ts b/packages/twenty-server/src/engine/workspace-manager/workspace-migration-runner/services/workspace-migration-enum.service.ts index 5d8692c13..e3432577b 100644 --- a/packages/twenty-server/src/engine/workspace-manager/workspace-migration-runner/services/workspace-migration-enum.service.ts +++ b/packages/twenty-server/src/engine/workspace-manager/workspace-migration-runner/services/workspace-migration-enum.service.ts @@ -13,6 +13,20 @@ export class WorkspaceMigrationEnumService { tableName: string, migrationColumn: WorkspaceMigrationColumnAlter, ) { + // Rename column name + if ( + migrationColumn.currentColumnDefinition.columnName !== + migrationColumn.alteredColumnDefinition.columnName + ) { + await this.renameColumn( + queryRunner, + schemaName, + tableName, + migrationColumn.currentColumnDefinition.columnName, + migrationColumn.alteredColumnDefinition.columnName, + ); + } + const columnDefinition = migrationColumn.alteredColumnDefinition; const oldEnumTypeName = `${tableName}_${columnDefinition.columnName}_enum`; const newEnumTypeName = `${tableName}_${columnDefinition.columnName}_enum_new`; @@ -82,6 +96,19 @@ export class WorkspaceMigrationEnumService { ); } + private async renameColumn( + queryRunner: QueryRunner, + schemaName: string, + tableName: string, + oldColumnName: string, + newColumnName: string, + ) { + await queryRunner.query(` + ALTER TABLE "${schemaName}"."${tableName}" + RENAME COLUMN "${oldColumnName}" TO "${newColumnName}" + `); + } + private async createNewEnumType( name: string, queryRunner: QueryRunner,