From 59e4ed715a5af263dac250797f30714e1ad50346 Mon Sep 17 00:00:00 2001 From: Nathan Nguyen <146415969+NathanDrake2406@users.noreply.github.com> Date: Sat, 18 Apr 2026 05:36:43 +1000 Subject: [PATCH] fix(server): normalize empty composite phone sub-fields to NULL (#19775) Fixed using Opus 4.7, I wanted to test this model out and in this repo I know you guys care about quality, pls let me know if this is good code. It looks good to me Fixes #19740. ## Summary PostgreSQL UNIQUE indexes treat two `''` values as duplicates but two `NULL`s as distinct. `validateAndInferPhoneInput` was persisting blank `primaryPhoneNumber` as `''` instead of `NULL`, so a second record with an empty unique phone failed with a constraint violation. The sibling composite transforms (`transformEmailsValue`, `removeEmptyLinks`, `transformTextField`) already canonicalize null-equivalent values; phones was the outlier. - Empty-string phone sub-fields now normalize to `null`. `undefined` is preserved so partial updates leave columns the user did not touch alone. - `PhonesFieldGraphQLInput` drops the aspirational `CountryCode` brand on input. GraphQL delivers raw strings at the boundary; branding happens during validation. --------- Co-authored-by: Charles Bochet --- .../transform-phones-value.util.spec.ts | 83 +++++++++ .../utils/transform-phones-value.util.ts | 50 +++--- ...field-null-equivalence.integration-spec.ts | 159 ++++++++++++++++++ 3 files changed, 271 insertions(+), 21 deletions(-) create mode 100644 packages/twenty-server/src/engine/core-modules/record-transformer/utils/__tests__/transform-phones-value.util.spec.ts create mode 100644 packages/twenty-server/test/integration/graphql/suites/unique-field/unique-phones-field-null-equivalence.integration-spec.ts diff --git a/packages/twenty-server/src/engine/core-modules/record-transformer/utils/__tests__/transform-phones-value.util.spec.ts b/packages/twenty-server/src/engine/core-modules/record-transformer/utils/__tests__/transform-phones-value.util.spec.ts new file mode 100644 index 0000000000..aa3cd85293 --- /dev/null +++ b/packages/twenty-server/src/engine/core-modules/record-transformer/utils/__tests__/transform-phones-value.util.spec.ts @@ -0,0 +1,83 @@ +import { transformPhonesValue } from 'src/engine/core-modules/record-transformer/utils/transform-phones-value.util'; + +describe('transformPhonesValue', () => { + it('should return null when input is null', () => { + const result = transformPhonesValue({ input: null }); + + expect(result).toBeNull(); + }); + + it('should normalize all empty primary sub-fields to null', () => { + const result = transformPhonesValue({ + input: { + primaryPhoneNumber: '', + primaryPhoneCallingCode: '', + primaryPhoneCountryCode: '', + }, + }); + + expect(result).toEqual({ + additionalPhones: null, + primaryPhoneNumber: null, + primaryPhoneCallingCode: null, + primaryPhoneCountryCode: null, + }); + }); + + it('should normalize empty number inside an additionalPhones entry to null', () => { + const result = transformPhonesValue({ + input: { + primaryPhoneNumber: '', + additionalPhones: JSON.stringify([{ number: '' }]), + }, + }); + + expect(result?.additionalPhones).toBe(JSON.stringify([{ number: null }])); + }); + + it('should parse a valid international phone number into its canonical parts', () => { + const result = transformPhonesValue({ + input: { primaryPhoneNumber: '+14155552671' }, + }); + + expect(result).toEqual({ + additionalPhones: null, + primaryPhoneNumber: '4155552671', + primaryPhoneCallingCode: '+1', + primaryPhoneCountryCode: 'US', + }); + }); + + it('should infer callingCode from the number when callingCode is an empty string', () => { + const result = transformPhonesValue({ + input: { + primaryPhoneNumber: '+14155552671', + primaryPhoneCallingCode: '', + }, + }); + + expect(result).toEqual({ + additionalPhones: null, + primaryPhoneNumber: '4155552671', + primaryPhoneCallingCode: '+1', + primaryPhoneCountryCode: 'US', + }); + }); + + it('should accept additionalPhones as an array of phone objects', () => { + const result = transformPhonesValue({ + input: { + primaryPhoneNumber: '+14155552671', + additionalPhones: [ + { number: '+442071838750', callingCode: '+44', countryCode: 'GB' }, + ], + }, + }); + + expect(result?.additionalPhones).toBe( + JSON.stringify([ + { countryCode: 'GB', callingCode: '+44', number: '2071838750' }, + ]), + ); + }); +}); diff --git a/packages/twenty-server/src/engine/core-modules/record-transformer/utils/transform-phones-value.util.ts b/packages/twenty-server/src/engine/core-modules/record-transformer/utils/transform-phones-value.util.ts index 4f97bd160e..819bd0241d 100644 --- a/packages/twenty-server/src/engine/core-modules/record-transformer/utils/transform-phones-value.util.ts +++ b/packages/twenty-server/src/engine/core-modules/record-transformer/utils/transform-phones-value.util.ts @@ -5,10 +5,7 @@ import { parsePhoneNumberWithError, } from 'libphonenumber-js'; import isEmpty from 'lodash.isempty'; -import { - type AdditionalPhoneMetadata, - type PhonesMetadata, -} from 'twenty-shared/types'; +import { type AdditionalPhoneMetadata } from 'twenty-shared/types'; import { getCountryCodesForCallingCode, isDefined, @@ -23,11 +20,12 @@ import { } from 'src/engine/core-modules/record-transformer/record-transformer.exception'; export type PhonesFieldGraphQLInput = - | Partial< - Omit & { - additionalPhones: string | null; - } - > + | { + primaryPhoneNumber?: string | null; + primaryPhoneCountryCode?: string | null; + primaryPhoneCallingCode?: string | null; + additionalPhones?: string | Partial[] | null; + } | null | undefined; @@ -36,10 +34,16 @@ type AdditionalPhoneMetadataWithNumber = Partial & const removePlusFromString = (str: string) => str.replace(/\+/g, ''); +const nullIfEmptyString = (value: string | null | undefined) => + !isDefined(value) ? value : isNonEmptyString(value) ? value : null; + const validatePrimaryPhoneCountryCodeAndCallingCode = ({ callingCode, countryCode, -}: Partial>) => { +}: { + callingCode?: string | null; + countryCode?: string | null; +}) => { if (isNonEmptyString(countryCode) && !isValidCountryCode(countryCode)) { throw new RecordTransformerException( `Invalid country code ${countryCode}`, @@ -154,24 +158,28 @@ const validateAndInferPhoneInput = ({ callingCode, countryCode, number, -}: Partial) => { - validatePrimaryPhoneCountryCodeAndCallingCode({ - callingCode, - countryCode, - }); +}: { + callingCode?: string | null; + countryCode?: string | null; + number?: string | null; +}) => { + validatePrimaryPhoneCountryCodeAndCallingCode({ callingCode, countryCode }); - if (isDefined(number) && isNonEmptyString(number)) { + if (isNonEmptyString(number)) { return validateAndInferMetadataFromPrimaryPhoneNumber({ number, - callingCode, - countryCode, + callingCode: isNonEmptyString(callingCode) ? callingCode : undefined, + countryCode: + isNonEmptyString(countryCode) && isValidCountryCode(countryCode) + ? countryCode + : undefined, }); } return { - callingCode, - countryCode, - number, + callingCode: nullIfEmptyString(callingCode), + countryCode: nullIfEmptyString(countryCode), + number: nullIfEmptyString(number), }; }; diff --git a/packages/twenty-server/test/integration/graphql/suites/unique-field/unique-phones-field-null-equivalence.integration-spec.ts b/packages/twenty-server/test/integration/graphql/suites/unique-field/unique-phones-field-null-equivalence.integration-spec.ts new file mode 100644 index 0000000000..5b576c3f52 --- /dev/null +++ b/packages/twenty-server/test/integration/graphql/suites/unique-field/unique-phones-field-null-equivalence.integration-spec.ts @@ -0,0 +1,159 @@ +import { faker } from '@faker-js/faker'; +import { createOneOperation } from 'test/integration/graphql/utils/create-one-operation.util'; +import { createOneFieldMetadata } from 'test/integration/metadata/suites/field-metadata/utils/create-one-field-metadata.util'; +import { createOneObjectMetadata } from 'test/integration/metadata/suites/object-metadata/utils/create-one-object-metadata.util'; +import { deleteOneObjectMetadata } from 'test/integration/metadata/suites/object-metadata/utils/delete-one-object-metadata.util'; +import { updateOneObjectMetadata } from 'test/integration/metadata/suites/object-metadata/utils/update-one-object-metadata.util'; +import { deleteRecordsByIds } from 'test/integration/utils/delete-records-by-ids'; +import { FieldMetadataType } from 'twenty-shared/types'; + +const OBJECT_SINGULAR = 'uniquePhonesTestObject'; +const OBJECT_PLURAL = 'uniquePhonesTestObjects'; +const FIELD_NAME = 'phone'; + +describe('unique PHONES field with empty values', () => { + let createdObjectMetadataId: string; + let createdRecordIdsForCleaning: string[] = []; + + beforeAll(async () => { + const { data } = await createOneObjectMetadata({ + input: { + nameSingular: OBJECT_SINGULAR, + namePlural: OBJECT_PLURAL, + labelSingular: 'Unique Phones Test Object', + labelPlural: 'Unique Phones Test Objects', + icon: 'IconPhone', + isLabelSyncedWithName: false, + }, + }); + + createdObjectMetadataId = data.createOneObject.id; + + await createOneFieldMetadata({ + input: { + name: FIELD_NAME, + label: 'Phone', + type: FieldMetadataType.PHONES, + objectMetadataId: createdObjectMetadataId, + isUnique: true, + isLabelSyncedWithName: false, + }, + gqlFields: ` + id + name + isUnique + `, + }); + }); + + afterEach(async () => { + if (createdRecordIdsForCleaning.length > 0) { + await deleteRecordsByIds(OBJECT_SINGULAR, createdRecordIdsForCleaning); + createdRecordIdsForCleaning = []; + } + }); + + afterAll(async () => { + await updateOneObjectMetadata({ + expectToFail: false, + input: { + idToUpdate: createdObjectMetadataId, + updatePayload: { isActive: false }, + }, + }); + await deleteOneObjectMetadata({ + input: { idToDelete: createdObjectMetadataId }, + }); + }); + + it('should allow creating two records with empty primaryPhoneNumber on a unique PHONES field', async () => { + const firstId = faker.string.uuid(); + const secondId = faker.string.uuid(); + + const firstResponse = await createOneOperation({ + objectMetadataSingularName: OBJECT_SINGULAR, + input: { + id: firstId, + [FIELD_NAME]: { primaryPhoneNumber: '' }, + }, + gqlFields: `id`, + }); + + expect(firstResponse.errors).toBeUndefined(); + expect(firstResponse.data.createOneResponse.id).toBe(firstId); + createdRecordIdsForCleaning.push(firstId); + + const secondResponse = await createOneOperation({ + objectMetadataSingularName: OBJECT_SINGULAR, + input: { + id: secondId, + [FIELD_NAME]: { primaryPhoneNumber: '' }, + }, + gqlFields: `id`, + }); + + expect(secondResponse.errors).toBeUndefined(); + expect(secondResponse.data.createOneResponse.id).toBe(secondId); + createdRecordIdsForCleaning.push(secondId); + }); + + it('should allow creating two records with no PHONES payload at all', async () => { + const firstId = faker.string.uuid(); + const secondId = faker.string.uuid(); + + const firstResponse = await createOneOperation({ + objectMetadataSingularName: OBJECT_SINGULAR, + input: { id: firstId }, + gqlFields: `id`, + }); + + expect(firstResponse.errors).toBeUndefined(); + createdRecordIdsForCleaning.push(firstId); + + const secondResponse = await createOneOperation({ + objectMetadataSingularName: OBJECT_SINGULAR, + input: { id: secondId }, + gqlFields: `id`, + }); + + expect(secondResponse.errors).toBeUndefined(); + createdRecordIdsForCleaning.push(secondId); + }); + + it('should still enforce uniqueness when two records share the same non-empty primaryPhoneNumber', async () => { + const firstId = faker.string.uuid(); + const secondId = faker.string.uuid(); + + const firstResponse = await createOneOperation({ + objectMetadataSingularName: OBJECT_SINGULAR, + input: { + id: firstId, + [FIELD_NAME]: { + primaryPhoneNumber: '4155552671', + primaryPhoneCallingCode: '+1', + primaryPhoneCountryCode: 'US', + }, + }, + gqlFields: `id`, + }); + + expect(firstResponse.errors).toBeUndefined(); + createdRecordIdsForCleaning.push(firstId); + + const secondResponse = await createOneOperation({ + objectMetadataSingularName: OBJECT_SINGULAR, + input: { + id: secondId, + [FIELD_NAME]: { + primaryPhoneNumber: '4155552671', + primaryPhoneCallingCode: '+1', + primaryPhoneCountryCode: 'US', + }, + }, + gqlFields: `id`, + expectToFail: true, + }); + + expect(secondResponse.errors).toBeDefined(); + }); +});