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

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)

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<FieldMetadataEntity> {
   constructor(
+    @InjectDataSource('metadata')
+    private readonly metadataDataSource: DataSource,
     @InjectRepository(FieldMetadataEntity, 'metadata')
     private readonly fieldMetadataRepository: Repository<FieldMetadataEntity>,
     @InjectRepository(RelationMetadataEntity, 'metadata')
@@ -65,6 +67,16 @@ export class FieldMetadataService extends TypeOrmQueryService<FieldMetadataEntit
   override async createOne(
     fieldMetadataInput: CreateFieldInput,
   ): Promise<FieldMetadataEntity> {
+    const queryRunner = this.metadataDataSource.createQueryRunner();
+
+    await queryRunner.connect();
+    await queryRunner.startTransaction();
+
+    try {
+      const fieldMetadataRepository =
+        queryRunner.manager.getRepository<FieldMetadataEntity<'default'>>(
+          FieldMetadataEntity,
+        );
       const objectMetadata =
         await this.objectMetadataService.findOneWithinWorkspace(
           fieldMetadataInput.workspaceId,
@@ -94,7 +106,7 @@ export class FieldMetadataService extends TypeOrmQueryService<FieldMetadataEntit
         fieldMetadataInput.options = generateRatingOptions();
       }
 
-    const fieldAlreadyExists = await this.fieldMetadataRepository.findOne({
+      const fieldAlreadyExists = await fieldMetadataRepository.findOne({
         where: {
           name: fieldMetadataInput.name,
           objectMetadataId: fieldMetadataInput.objectMetadataId,
@@ -106,7 +118,7 @@ export class FieldMetadataService extends TypeOrmQueryService<FieldMetadataEntit
         throw new ConflictException('Field already exists');
       }
 
-    const createdFieldMetadata = await super.createOne({
+      const createdFieldMetadata = await fieldMetadataRepository.save({
         ...fieldMetadataInput,
         targetColumnMap: generateTargetColumnMap(
           fieldMetadataInput.type,
@@ -158,13 +170,23 @@ export class FieldMetadataService extends TypeOrmQueryService<FieldMetadataEntit
       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 workspaceDataSource?.query(
+        const view = await workspaceQueryRunner?.query(
           `SELECT id FROM ${dataSourceMetadata.schema}."view"
       WHERE "objectMetadataId" = '${createdFieldMetadata.objectMetadataId}'`,
         );
 
-    const existingViewFields = await workspaceDataSource?.query(
+        const existingViewFields = await workspaceQueryRunner?.query(
           `SELECT * FROM ${dataSourceMetadata.schema}."viewField"
       WHERE "viewId" = '${view[0].id}'`,
         );
@@ -179,22 +201,47 @@ export class FieldMetadataService extends TypeOrmQueryService<FieldMetadataEntit
             return acc;
           }, -1);
 
-    await workspaceDataSource?.query(
+        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<FieldMetadataEntity> {
-    const existingFieldMetadata = await this.fieldMetadataRepository.findOne({
+    const queryRunner = this.metadataDataSource.createQueryRunner();
+
+    await queryRunner.connect();
+    await queryRunner.startTransaction();
+
+    try {
+      const fieldMetadataRepository =
+        queryRunner.manager.getRepository<FieldMetadataEntity<'default'>>(
+          FieldMetadataEntity,
+        );
+
+      const existingFieldMetadata = await fieldMetadataRepository.findOne({
         where: {
           id,
           workspaceId: fieldMetadataInput.workspaceId,
@@ -224,7 +271,9 @@ export class FieldMetadataService extends TypeOrmQueryService<FieldMetadataEntit
           existingFieldMetadata.id &&
         fieldMetadataInput.isActive === false
       ) {
-      throw new BadRequestException('Cannot deactivate label identifier field');
+        throw new BadRequestException(
+          'Cannot deactivate label identifier field',
+        );
       }
 
       if (fieldMetadataInput.options) {
@@ -243,7 +292,8 @@ export class FieldMetadataService extends TypeOrmQueryService<FieldMetadataEntit
             )
           : fieldMetadataInput;
 
-    const updatedFieldMetadata = await super.updateOne(id, {
+      // 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
@@ -262,6 +312,9 @@ export class FieldMetadataService extends TypeOrmQueryService<FieldMetadataEntit
             )
           : existingFieldMetadata.targetColumnMap,
       });
+      const updatedFieldMetadata = await fieldMetadataRepository.findOneOrFail({
+        where: { id },
+      });
 
       if (
         fieldMetadataInput.name ||
@@ -289,7 +342,15 @@ export class FieldMetadataService extends TypeOrmQueryService<FieldMetadataEntit
         );
       }
 
+      await queryRunner.commitTransaction();
+
       return updatedFieldMetadata;
+    } catch (error) {
+      await queryRunner.rollbackTransaction();
+      throw error;
+    } finally {
+      await queryRunner.release();
+    }
   }
 
   public async findOneOrFail(

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;

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

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,