From 3fe12cd8b5feb96a66cf480f34a08def70f5e764 Mon Sep 17 00:00:00 2001 From: Weiko Date: Thu, 17 Apr 2025 18:34:31 +0200 Subject: [PATCH] Fix select default value not in options (#11622) Also fixing a bunch of places where validation exceptions were not properly handled --- .../field-metadata-validation.service.ts | 45 +++++++++++++++ .../field-metadata/field-metadata.resolver.ts | 9 ++- .../field-metadata/field-metadata.service.ts | 19 +++++-- .../before-update-one-field.hook.spec.ts | 21 ++++--- .../hooks/before-update-one-field.hook.ts | 18 +++--- ...ate-one-field-metadata.integration-spec.ts | 55 +++++++++++++++++++ 6 files changed, 141 insertions(+), 26 deletions(-) diff --git a/packages/twenty-server/src/engine/metadata-modules/field-metadata/field-metadata-validation.service.ts b/packages/twenty-server/src/engine/metadata-modules/field-metadata/field-metadata-validation.service.ts index 0df92d220..4628fbe85 100644 --- a/packages/twenty-server/src/engine/metadata-modules/field-metadata/field-metadata-validation.service.ts +++ b/packages/twenty-server/src/engine/metadata-modules/field-metadata/field-metadata-validation.service.ts @@ -11,6 +11,8 @@ import { } from 'class-validator'; import { FieldMetadataType } from 'twenty-shared/types'; +import { FieldMetadataDefaultValue } from 'src/engine/metadata-modules/field-metadata/interfaces/field-metadata-default-value.interface'; +import { FieldMetadataOptions } from 'src/engine/metadata-modules/field-metadata/interfaces/field-metadata-options.interface'; import { FieldMetadataSettings } from 'src/engine/metadata-modules/field-metadata/interfaces/field-metadata-settings.interface'; import { @@ -86,4 +88,47 @@ export class FieldMetadataValidationService< ); } } + + async validateDefaultValueOrThrow({ + fieldType, + defaultValue, + options, + }: { + fieldType: FieldMetadataType; + defaultValue: FieldMetadataDefaultValue; + options: FieldMetadataOptions; + }) { + if ( + fieldType === FieldMetadataType.SELECT || + fieldType === FieldMetadataType.MULTI_SELECT + ) { + this.validateEnumDefaultValue(options, defaultValue); + } + } + + private validateEnumDefaultValue( + options: FieldMetadataOptions, + defaultValue: FieldMetadataDefaultValue, + ) { + if (typeof defaultValue === 'string') { + const formattedDefaultValue = defaultValue.replace( + /^['"](.*)['"]$/, + '$1', + ); + + const enumOptions = options.map((option) => option.value); + + if ( + enumOptions && + (enumOptions.includes(formattedDefaultValue) || + enumOptions.some((option) => option.to === formattedDefaultValue)) + ) { + return; + } + } + throw new FieldMetadataException( + `Default value for existing options is invalid: ${defaultValue}`, + FieldMetadataExceptionCode.INVALID_FIELD_INPUT, + ); + } } diff --git a/packages/twenty-server/src/engine/metadata-modules/field-metadata/field-metadata.resolver.ts b/packages/twenty-server/src/engine/metadata-modules/field-metadata/field-metadata.resolver.ts index 640a8d7db..44940636c 100644 --- a/packages/twenty-server/src/engine/metadata-modules/field-metadata/field-metadata.resolver.ts +++ b/packages/twenty-server/src/engine/metadata-modules/field-metadata/field-metadata.resolver.ts @@ -1,4 +1,4 @@ -import { UnauthorizedException, UseFilters, UseGuards } from '@nestjs/common'; +import { UseFilters, UseGuards } from '@nestjs/common'; import { Args, Context, @@ -12,7 +12,10 @@ import { FieldMetadataType } from 'twenty-shared/types'; import { FeatureFlagKey } from 'src/engine/core-modules/feature-flag/enums/feature-flag-key.enum'; import { FeatureFlagService } from 'src/engine/core-modules/feature-flag/services/feature-flag.service'; -import { ValidationError } from 'src/engine/core-modules/graphql/utils/graphql-errors.util'; +import { + ForbiddenError, + ValidationError, +} from 'src/engine/core-modules/graphql/utils/graphql-errors.util'; import { I18nContext } from 'src/engine/core-modules/i18n/types/i18n-context.type'; import { Workspace } from 'src/engine/core-modules/workspace/workspace.entity'; import { IDataloaders } from 'src/engine/dataloaders/dataloader.interface'; @@ -132,7 +135,7 @@ export class FieldMetadataResolver { @AuthWorkspace() { id: workspaceId }: Workspace, ) { if (!workspaceId) { - throw new UnauthorizedException(); + throw new ForbiddenError('Could not retrieve workspace ID'); } const fieldMetadata = 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 0f10319d9..221951498 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 @@ -213,7 +213,7 @@ export class FieldMetadataService extends TypeOrmQueryService( + await this.validateFieldMetadata( existingFieldMetadata.type, fieldMetadataForUpdate, objectMetadata, @@ -545,11 +545,13 @@ export class FieldMetadataService extends TypeOrmQueryService( + private async validateFieldMetadata< + T extends UpdateFieldInput | CreateFieldInput, + >( fieldMetadataType: FieldMetadataType, fieldMetadataInput: T, objectMetadata: ObjectMetadataEntity, - ): T { + ): Promise { if (fieldMetadataInput.name) { try { validateMetadataNameOrThrow(fieldMetadataInput.name); @@ -599,10 +601,17 @@ export class FieldMetadataService extends TypeOrmQueryService( + await this.validateFieldMetadata( fieldMetadataForCreate.type, fieldMetadataForCreate, objectMetadata, diff --git a/packages/twenty-server/src/engine/metadata-modules/field-metadata/hooks/__tests__/before-update-one-field.hook.spec.ts b/packages/twenty-server/src/engine/metadata-modules/field-metadata/hooks/__tests__/before-update-one-field.hook.spec.ts index 250d3a545..6f4a682e7 100644 --- a/packages/twenty-server/src/engine/metadata-modules/field-metadata/hooks/__tests__/before-update-one-field.hook.spec.ts +++ b/packages/twenty-server/src/engine/metadata-modules/field-metadata/hooks/__tests__/before-update-one-field.hook.spec.ts @@ -1,9 +1,12 @@ -import { BadRequestException, UnauthorizedException } from '@nestjs/common'; import { Test, TestingModule } from '@nestjs/testing'; import { i18n } from '@lingui/core'; import { UpdateOneInputType } from '@ptc-org/nestjs-query-graphql'; +import { + ForbiddenError, + ValidationError, +} from 'src/engine/core-modules/graphql/utils/graphql-errors.util'; import { UpdateFieldInput } from 'src/engine/metadata-modules/field-metadata/dtos/update-field.input'; import { FieldMetadataEntity } from 'src/engine/metadata-modules/field-metadata/field-metadata.entity'; import { FieldMetadataService } from 'src/engine/metadata-modules/field-metadata/field-metadata.service'; @@ -48,7 +51,7 @@ describe('BeforeUpdateOneField', () => { jest.clearAllMocks(); }); - it('should throw UnauthorizedException if workspaceId is not provided', async () => { + it('should throw ForbiddenError if workspaceId is not provided', async () => { const instance: UpdateOneInputType = { id: mockFieldId, update: { @@ -61,10 +64,10 @@ describe('BeforeUpdateOneField', () => { workspaceId: '', locale: undefined, }), - ).rejects.toThrow(UnauthorizedException); + ).rejects.toThrow(ForbiddenError); }); - it('should throw BadRequestException if field does not exist', async () => { + it('should throw ValidationError if field does not exist', async () => { const instance: UpdateOneInputType = { id: mockFieldId, update: { @@ -81,7 +84,7 @@ describe('BeforeUpdateOneField', () => { workspaceId: mockWorkspaceId, locale: undefined, }), - ).rejects.toThrow(BadRequestException); + ).rejects.toThrow(ValidationError); }); it('should not affect custom fields', async () => { @@ -113,7 +116,7 @@ describe('BeforeUpdateOneField', () => { expect(result).toEqual(instance); }); - it('should throw BadRequestException when trying to update non-updatable fields on standard fields', async () => { + it('should throw ValidationError when trying to update non-updatable fields on standard fields', async () => { const instance: UpdateOneInputType = { id: mockFieldId, update: { @@ -135,10 +138,10 @@ describe('BeforeUpdateOneField', () => { workspaceId: mockWorkspaceId, locale: undefined, }), - ).rejects.toThrow(BadRequestException); + ).rejects.toThrow(ValidationError); }); - it('should throw BadRequestException when trying to update label when it is synced with name', async () => { + it('should throw ValidationError when trying to update label when it is synced with name', async () => { const instance: UpdateOneInputType = { id: mockFieldId, update: { @@ -162,7 +165,7 @@ describe('BeforeUpdateOneField', () => { workspaceId: mockWorkspaceId, locale: undefined, }), - ).rejects.toThrow(BadRequestException); + ).rejects.toThrow(ValidationError); }); it('should handle isActive updates for standard fields', async () => { diff --git a/packages/twenty-server/src/engine/metadata-modules/field-metadata/hooks/before-update-one-field.hook.ts b/packages/twenty-server/src/engine/metadata-modules/field-metadata/hooks/before-update-one-field.hook.ts index 5eca92327..97f6d3a2b 100644 --- a/packages/twenty-server/src/engine/metadata-modules/field-metadata/hooks/before-update-one-field.hook.ts +++ b/packages/twenty-server/src/engine/metadata-modules/field-metadata/hooks/before-update-one-field.hook.ts @@ -1,8 +1,4 @@ -import { - BadRequestException, - Injectable, - UnauthorizedException, -} from '@nestjs/common'; +import { Injectable } from '@nestjs/common'; import { i18n } from '@lingui/core'; import { @@ -12,6 +8,10 @@ import { import { APP_LOCALES, SOURCE_LOCALE } from 'twenty-shared/translations'; import { isDefined } from 'twenty-shared/utils'; +import { + ForbiddenError, + ValidationError, +} from 'src/engine/core-modules/graphql/utils/graphql-errors.util'; import { generateMessageId } from 'src/engine/core-modules/i18n/utils/generateMessageId'; import { FieldStandardOverridesDTO } from 'src/engine/metadata-modules/field-metadata/dtos/field-standard-overrides.dto'; import { UpdateFieldInput } from 'src/engine/metadata-modules/field-metadata/dtos/update-field.input'; @@ -39,7 +39,7 @@ export class BeforeUpdateOneField }, ): Promise> { if (!workspaceId) { - throw new UnauthorizedException(); + throw new ForbiddenError('Could not retrieve workspace ID'); } const fieldMetadata = await this.getFieldMetadata(instance, workspaceId); @@ -63,7 +63,7 @@ export class BeforeUpdateOneField }); if (!fieldMetadata) { - throw new BadRequestException('Field does not exist'); + throw new ValidationError('Field does not exist'); } return fieldMetadata; @@ -96,13 +96,13 @@ export class BeforeUpdateOneField instance.update.label !== fieldMetadata.label; if (isUpdatingLabelWhenSynced) { - throw new BadRequestException( + throw new ValidationError( 'Cannot update label when it is synced with name', ); } if (nonUpdatableFields.length > 0) { - throw new BadRequestException( + throw new ValidationError( `Only isActive, isLabelSyncedWithName, label, icon, description and defaultValue fields can be updated for standard fields. Invalid fields: ${nonUpdatableFields.join(', ')}`, ); } diff --git a/packages/twenty-server/test/integration/metadata/suites/field-metadata/update-one-field-metadata.integration-spec.ts b/packages/twenty-server/test/integration/metadata/suites/field-metadata/update-one-field-metadata.integration-spec.ts index 34ae3b08d..ac073a149 100644 --- a/packages/twenty-server/test/integration/metadata/suites/field-metadata/update-one-field-metadata.integration-spec.ts +++ b/packages/twenty-server/test/integration/metadata/suites/field-metadata/update-one-field-metadata.integration-spec.ts @@ -116,4 +116,59 @@ describe('updateOne', () => { ); }); }); + + describe('FieldMetadataService Enum Default Value Validation', () => { + it('should throw an error if the default value is not in the options', async () => { + const { data: listingObjectMetadata } = await createOneObjectMetadata({ + input: { + labelSingular: LISTING_NAME_SINGULAR, + labelPlural: LISTING_NAME_PLURAL, + nameSingular: LISTING_NAME_SINGULAR, + namePlural: LISTING_NAME_PLURAL, + icon: 'IconBuildingSkyscraper', + isLabelSyncedWithName: true, + }, + }); + + const { data: createdFieldMetadata } = await createOneFieldMetadata({ + input: { + objectMetadataId: listingObjectMetadata.createOneObject.id, + type: FieldMetadataType.SELECT, + name: 'testName', + label: 'Test name', + isLabelSyncedWithName: true, + options: [ + { + label: 'Option 1', + value: 'option1', + color: 'green', + position: 1, + }, + ], + }, + }); + + const { errors } = await updateOneFieldMetadata({ + input: { + idToUpdate: createdFieldMetadata.createOneField.id, + updatePayload: { + defaultValue: 'option2', + }, + }, + gqlFields: ` + id + name + label + isLabelSyncedWithName + `, + expectToFail: true, + }); + + expect(errors[0].message).toBe('Invalid default value "option2"'); + + await deleteOneObjectMetadata({ + input: { idToDelete: listingObjectMetadata.createOneObject.id }, + }); + }); + }); });