From fab0358df51f0b7934bd4e5761528df618a9f420 Mon Sep 17 00:00:00 2001 From: Weiko Date: Wed, 1 Jul 2026 13:31:22 +0200 Subject: [PATCH] Handle field isNullable update (#22362) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Context Setting isNullable on a field via the app SDK manifest was silently ignored when re-syncing an existing field. The first sync that creates a field honored isNullable correctly, but any later manifest change to isNullable had no effect, neither on the field metadata nor on the underlying Postgres column. Two compounding gaps caused this: The diff never detected the change. isNullable was configured with toCompare: false, so compareTwoFlatEntity excluded it from the diff and no update action was ever generated. There was no DDL to apply it. Even if detected, the update field action handler only altered name, options, defaultValue, and settings. The column manager had no way to alter a column's NOT NULL constraint. ## Fix - Set isNullable.toCompare: true so manifest changes are detected and persisted to the field metadata (via the existing executeForMetadata path). - Add WorkspaceSchemaColumnManagerService.alterColumnNullable(): emits SET NOT NULL / DROP NOT NULL, with an optional pre-serialized backfill (UPDATE … WHERE col IS NULL) applied only on the nullable → non-nullable transition. - Add handleFieldNullableUpdate() to the update field action handler, dispatched after the defaultValue block so the default is in place before NOT NULL is enforced. It is composite-aware (mirrors the per-sub-column parentIsNullable || !property.isRequired rule used at column creation) and skips relation/morph join columns and TS_VECTOR, which are always nullable by design. Review in cubic --- .../developers/extend/apps/data/objects.mdx | 21 +++ ...ompare-and-stringify.constant.spec.ts.snap | 1 + ...configuration-by-metadata-name.constant.ts | 2 +- .../__tests__/flat-entity-update.type-test.ts | 1 + ...workspace-schema-column-manager.service.ts | 33 ++++ .../universal-flat-entity-update.test-type.ts | 1 + ...-flat-entity-properties-to-compare.type.ts | 1 + .../update-field-action-handler.service.ts | 100 +++++++++++ ...-manifest-update-field.integration-spec.ts | 165 +++++++++++++++++- 9 files changed, 323 insertions(+), 2 deletions(-) diff --git a/packages/twenty-docs/developers/extend/apps/data/objects.mdx b/packages/twenty-docs/developers/extend/apps/data/objects.mdx index 6f466443d6..4976cef49e 100644 --- a/packages/twenty-docs/developers/extend/apps/data/objects.mdx +++ b/packages/twenty-docs/developers/extend/apps/data/objects.mdx @@ -97,6 +97,27 @@ Unquoted strings are reserved for computed defaults, evaluated when a record is The same convention applies to string sub-fields of composite defaults (e.g. `{ source: "'MANUAL'" }` on an `ACTOR` field) and to `SELECT`/`MULTI_SELECT` values. A literal string default left unquoted raises a warning when your app is built. +## Nullability + +`isNullable` controls whether a field accepts `NULL`. It defaults to `true` — omit it for optional fields. Set `isNullable: false` to make a field required at the database level. + +Changes to `isNullable` are applied on every sync, including syncs that update an existing field — so you can flip a field's nullability by editing the manifest and re-syncing. + + +**Making an existing field non-nullable requires a default value.** When you change a field to `isNullable: false`, you must also provide a non-null `defaultValue`. The default backfills any existing `NULL` rows before the `NOT NULL` constraint is applied; without it the sync fails with `Default value cannot be null for non-nullable fields`. Relation fields and `TS_VECTOR` fields are always nullable, so `isNullable` has no effect on them. + + +```ts +{ + universalIdentifier: 'b1a7c0de-1234-4f00-9abc-000000000000', + name: 'reference', + type: FieldType.TEXT, + label: 'Reference', + isNullable: false, + defaultValue: "'N/A'", +} +``` + ## What's next - **Connect this object to others** — see [Relations](/developers/extend/apps/data/relations) for the bidirectional relation pattern. diff --git a/packages/twenty-server/src/engine/metadata-modules/flat-entity/constant/__tests__/__snapshots__/all-universal-flat-entity-properties-to-compare-and-stringify.constant.spec.ts.snap b/packages/twenty-server/src/engine/metadata-modules/flat-entity/constant/__tests__/__snapshots__/all-universal-flat-entity-properties-to-compare-and-stringify.constant.spec.ts.snap index dd16e025d4..eb12f595f0 100644 --- a/packages/twenty-server/src/engine/metadata-modules/flat-entity/constant/__tests__/__snapshots__/all-universal-flat-entity-properties-to-compare-and-stringify.constant.spec.ts.snap +++ b/packages/twenty-server/src/engine/metadata-modules/flat-entity/constant/__tests__/__snapshots__/all-universal-flat-entity-properties-to-compare-and-stringify.constant.spec.ts.snap @@ -75,6 +75,7 @@ exports[`ALL_UNIVERSAL_FLAT_ENTITY_PROPERTIES_TO_COMPARE_AND_STRINGIFY should ma "standardOverrides", "universalSettings", "isUIEditable", + "isNullable", ], "propertiesToStringify": [ "defaultValue", diff --git a/packages/twenty-server/src/engine/metadata-modules/flat-entity/constant/all-entity-properties-configuration-by-metadata-name.constant.ts b/packages/twenty-server/src/engine/metadata-modules/flat-entity/constant/all-entity-properties-configuration-by-metadata-name.constant.ts index b622cf67e9..08b2010f13 100644 --- a/packages/twenty-server/src/engine/metadata-modules/flat-entity/constant/all-entity-properties-configuration-by-metadata-name.constant.ts +++ b/packages/twenty-server/src/engine/metadata-modules/flat-entity/constant/all-entity-properties-configuration-by-metadata-name.constant.ts @@ -124,7 +124,7 @@ export const ALL_ENTITY_PROPERTIES_CONFIGURATION_BY_METADATA_NAME = { universalProperty: undefined, }, isNullable: { - toCompare: false, + toCompare: true, toStringify: false, universalProperty: undefined, }, diff --git a/packages/twenty-server/src/engine/metadata-modules/flat-entity/types/__tests__/flat-entity-update.type-test.ts b/packages/twenty-server/src/engine/metadata-modules/flat-entity/types/__tests__/flat-entity-update.type-test.ts index 63cdfa0a30..09ddf0f3f4 100644 --- a/packages/twenty-server/src/engine/metadata-modules/flat-entity/types/__tests__/flat-entity-update.type-test.ts +++ b/packages/twenty-server/src/engine/metadata-modules/flat-entity/types/__tests__/flat-entity-update.type-test.ts @@ -23,6 +23,7 @@ type Assertions = [ | 'isUnique' | 'isLabelSyncedWithName' | 'isUIEditable' + | 'isNullable' | 'universalSettings' > >, diff --git a/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/services/workspace-schema-column-manager.service.ts b/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/services/workspace-schema-column-manager.service.ts index 6022e611e5..943237274a 100644 --- a/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/services/workspace-schema-column-manager.service.ts +++ b/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/services/workspace-schema-column-manager.service.ts @@ -104,4 +104,37 @@ export class WorkspaceSchemaColumnManagerService { await queryRunner.query(sql); } + + async alterColumnNullable({ + queryRunner, + schemaName, + tableName, + columnName, + isNullable, + backfillValue, + }: { + queryRunner: QueryRunner; + schemaName: string; + tableName: string; + columnName: string; + isNullable: boolean; + backfillValue?: string; + }): Promise { + const tableRef = `${escapeIdentifier(schemaName)}.${escapeIdentifier(tableName)}`; + const columnRef = escapeIdentifier(columnName); + + if ( + !isNullable && + backfillValue !== undefined && + backfillValue !== 'NULL' + ) { + await queryRunner.query( + `UPDATE ${tableRef} SET ${columnRef} = ${backfillValue} WHERE ${columnRef} IS NULL`, + ); + } + + await queryRunner.query( + `ALTER TABLE ${tableRef} ALTER COLUMN ${columnRef} ${isNullable ? 'DROP NOT NULL' : 'SET NOT NULL'}`, + ); + } } diff --git a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/universal-flat-entity/types/__tests__/universal-flat-entity-update.test-type.ts b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/universal-flat-entity/types/__tests__/universal-flat-entity-update.test-type.ts index 48f9ab013d..a4bc67a971 100644 --- a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/universal-flat-entity/types/__tests__/universal-flat-entity-update.test-type.ts +++ b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/universal-flat-entity/types/__tests__/universal-flat-entity-update.test-type.ts @@ -18,6 +18,7 @@ type Assertions = [ | 'isUnique' | 'isLabelSyncedWithName' | 'isUIEditable' + | 'isNullable' | 'universalSettings' > >, diff --git a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/universal-flat-entity/types/metadata-universal-flat-entity-properties-to-compare.type.ts b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/universal-flat-entity/types/metadata-universal-flat-entity-properties-to-compare.type.ts index 6df02f9173..db67349ea1 100644 --- a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/universal-flat-entity/types/metadata-universal-flat-entity-properties-to-compare.type.ts +++ b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/universal-flat-entity/types/metadata-universal-flat-entity-properties-to-compare.type.ts @@ -41,6 +41,7 @@ type Assertions = [ | 'isUnique' | 'isLabelSyncedWithName' | 'isUIEditable' + | 'isNullable' | 'universalSettings' > >, diff --git a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-runner/action-handlers/field/services/update-field-action-handler.service.ts b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-runner/action-handlers/field/services/update-field-action-handler.service.ts index fb5b86cfde..48c42e4206 100644 --- a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-runner/action-handlers/field/services/update-field-action-handler.service.ts +++ b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-runner/action-handlers/field/services/update-field-action-handler.service.ts @@ -76,6 +76,13 @@ type DefaultValueUpdateHandlerArgs< toDefaultValue: FlatFieldMetadata['defaultValue']; }; +type NullableUpdateHandlerArgs = Omit< + UpdateFieldPropertyHandlerArgs, + 'update' +> & { + toIsNullable: boolean; +}; + type OptionsUpdateHandlerArgs = UpdateFieldPropertyHandlerArgs & { toOptions: FlatFieldMetadata['options']; @@ -248,6 +255,19 @@ export class UpdateFieldActionHandlerService extends WorkspaceMigrationRunnerAct } } + if (update.isNullable !== undefined) { + const toIsNullable = update.isNullable ?? true; + + await this.handleFieldNullableUpdate({ + queryRunner, + schemaName, + tableName, + flatFieldMetadata: optimisticFlatFieldMetadata, + toIsNullable, + }); + optimisticFlatFieldMetadata.isNullable = toIsNullable; + } + if (isDefined(update.settings)) { // Handle onDelete change (for morph/relation fields) order matters if (isMorphOrRelationFlatFieldMetadata(optimisticFlatFieldMetadata)) { @@ -547,6 +567,86 @@ export class UpdateFieldActionHandlerService extends WorkspaceMigrationRunnerAct ); } + private async handleFieldNullableUpdate({ + flatFieldMetadata, + queryRunner, + schemaName, + tableName, + toIsNullable, + }: NullableUpdateHandlerArgs) { + if ( + isMorphOrRelationFlatFieldMetadata(flatFieldMetadata) || + isFlatFieldMetadataOfType(flatFieldMetadata, FieldMetadataType.TS_VECTOR) + ) { + return; + } + + if (isCompositeFlatFieldMetadata(flatFieldMetadata)) { + const compositeType = getCompositeTypeOrThrow(flatFieldMetadata.type); + + for (const property of compositeType.properties) { + if (isMorphOrRelationFieldMetadataType(property.type)) { + throw new WorkspaceMigrationActionExecutionException({ + message: + 'Relation field metadata in composite type is not supported yet', + code: WorkspaceMigrationActionExecutionExceptionCode.NOT_SUPPORTED, + }); + } + + const compositeColumnName = computeCompositeColumnName( + flatFieldMetadata.name, + property, + ); + const propertyIsNullable = toIsNullable || !property.isRequired; + const fieldDefaultValue = flatFieldMetadata.defaultValue; + // @ts-expect-error - composite default value is keyed by property name + const compositeDefaultValue = fieldDefaultValue?.[property.name]; + + await this.workspaceSchemaManagerService.columnManager.alterColumnNullable( + { + queryRunner, + schemaName, + tableName, + columnName: compositeColumnName, + isNullable: propertyIsNullable, + backfillValue: propertyIsNullable + ? undefined + : serializeDefaultValue({ + columnName: compositeColumnName, + schemaName, + tableName, + columnType: fieldMetadataTypeToColumnType( + property.type, + ) as ColumnType, + defaultValue: compositeDefaultValue, + }), + }, + ); + } + + return; + } + + await this.workspaceSchemaManagerService.columnManager.alterColumnNullable({ + queryRunner, + schemaName, + tableName, + columnName: flatFieldMetadata.name, + isNullable: toIsNullable, + backfillValue: toIsNullable + ? undefined + : serializeDefaultValue({ + columnName: flatFieldMetadata.name, + schemaName, + tableName, + columnType: fieldMetadataTypeToColumnType( + flatFieldMetadata.type, + ) as ColumnType, + defaultValue: flatFieldMetadata.defaultValue, + }), + }); + } + private async handleFieldOptionsUpdate({ flatFieldMetadata, queryRunner, diff --git a/packages/twenty-server/test/integration/metadata/suites/application/successful-manifest-update-field.integration-spec.ts b/packages/twenty-server/test/integration/metadata/suites/application/successful-manifest-update-field.integration-spec.ts index d1f8d97e03..3e47c2ebf9 100644 --- a/packages/twenty-server/test/integration/metadata/suites/application/successful-manifest-update-field.integration-spec.ts +++ b/packages/twenty-server/test/integration/metadata/suites/application/successful-manifest-update-field.integration-spec.ts @@ -3,15 +3,21 @@ import { buildDefaultObjectManifest } from 'test/integration/metadata/suites/app import { cleanupApplicationAndAppRegistration } from 'test/integration/metadata/suites/application/utils/cleanup-application-and-app-registration.util'; import { setupApplicationForSync } from 'test/integration/metadata/suites/application/utils/setup-application-for-sync.util'; import { syncApplication } from 'test/integration/metadata/suites/application/utils/sync-application.util'; +import { createOneOperationFactory } from 'test/integration/graphql/utils/create-one-operation-factory.util'; +import { findOneOperationFactory } from 'test/integration/graphql/utils/find-one-operation-factory.util'; +import { makeGraphqlAPIRequest } from 'test/integration/graphql/utils/make-graphql-api-request.util'; +import { findManyObjectMetadata } from 'test/integration/metadata/suites/object-metadata/utils/find-many-object-metadata.util'; import { findManyObjectMetadataWithIndexes } from 'test/integration/metadata/suites/object-metadata/utils/find-many-object-metadata-with-indexes.util'; -import { type Manifest } from 'twenty-shared/application'; +import { type FieldManifest, type Manifest } from 'twenty-shared/application'; import { FieldMetadataType } from 'twenty-shared/types'; +import { capitalize, isDefined } from 'twenty-shared/utils'; import { v4 as uuidv4 } from 'uuid'; const TEST_APP_ID = uuidv4(); const TEST_ROLE_ID = uuidv4(); const TEST_FIELD_ID = uuidv4(); const TEST_SECOND_FIELD_ID = uuidv4(); +const TEST_NUMBER_FIELD_ID = uuidv4(); const TEST_OBJECT = buildDefaultObjectManifest({ nameSingular: 'ticket', @@ -41,6 +47,94 @@ const findObjectFields = async () => { return object?.fieldsList ?? []; }; +const findFieldWithNullable = async (fieldName: string) => { + const { objects } = await findManyObjectMetadata({ + expectToFail: false, + input: { + filter: {}, + paging: { first: 100 }, + }, + gqlFields: ` + id + universalIdentifier + fieldsList { + name + isNullable + } + `, + }); + + const object = objects.find( + (o) => o.universalIdentifier === TEST_OBJECT.universalIdentifier, + ); + + return object?.fieldsList?.find((field) => field.name === fieldName); +}; + +const buildReferenceFieldManifest = (isNullable: boolean): FieldManifest => ({ + universalIdentifier: TEST_FIELD_ID, + type: FieldMetadataType.TEXT, + name: 'reference', + label: 'Reference', + description: 'Ticket reference', + icon: 'IconFileDescription', + isNullable, + defaultValue: "'N/A'", + objectUniversalIdentifier: TEST_OBJECT.universalIdentifier, +}); + +// NUMBER is used here (rather than a TEXT field) because the data API +// coerces null/omitted TEXT values to '' via the null-equivalent processor, +// so a TEXT column can never actually hold NULL. NUMBER preserves NULL, which +// is what the nullable -> non-nullable backfill needs to act on. +const buildEstimateFieldManifest = ({ + isNullable, + defaultValue, +}: { + isNullable: boolean; + defaultValue?: number; +}): FieldManifest => ({ + universalIdentifier: TEST_NUMBER_FIELD_ID, + type: FieldMetadataType.NUMBER, + name: 'estimate', + label: 'Estimate', + description: 'Ticket estimate', + icon: 'IconNumber', + isNullable, + ...(isDefined(defaultValue) ? { defaultValue } : {}), + objectUniversalIdentifier: TEST_OBJECT.universalIdentifier, +}); + +const createTicketRecord = async (data: Record) => { + const response = await makeGraphqlAPIRequest( + createOneOperationFactory({ + objectMetadataSingularName: TEST_OBJECT.nameSingular, + gqlFields: ` + id + estimate + `, + data, + }), + ); + + return response.body.data?.[`create${capitalize(TEST_OBJECT.nameSingular)}`]; +}; + +const findTicketRecordById = async (recordId: string) => { + const response = await makeGraphqlAPIRequest( + findOneOperationFactory({ + objectMetadataSingularName: TEST_OBJECT.nameSingular, + gqlFields: ` + id + estimate + `, + filter: { id: { eq: recordId } }, + }), + ); + + return response.body.data?.[TEST_OBJECT.nameSingular]; +}; + describe('Manifest update - fields', () => { beforeEach(async () => { await setupApplicationForSync({ @@ -240,6 +334,75 @@ describe('Manifest update - fields', () => { ).toBeUndefined(); }, 60000); + it('should update isNullable when changed in manifest on second sync', async () => { + await syncApplication({ + manifest: buildManifest({ fields: [buildReferenceFieldManifest(true)] }), + expectToFail: false, + }); + + const fieldAfterFirstSync = await findFieldWithNullable('reference'); + + expect(fieldAfterFirstSync).toBeDefined(); + expect(fieldAfterFirstSync?.isNullable).toBe(true); + + await syncApplication({ + manifest: buildManifest({ fields: [buildReferenceFieldManifest(false)] }), + expectToFail: false, + }); + + const fieldAfterSecondSync = await findFieldWithNullable('reference'); + + expect(fieldAfterSecondSync?.isNullable).toBe(false); + + await syncApplication({ + manifest: buildManifest({ fields: [buildReferenceFieldManifest(true)] }), + expectToFail: false, + }); + + const fieldAfterThirdSync = await findFieldWithNullable('reference'); + + expect(fieldAfterThirdSync?.isNullable).toBe(true); + }, 60000); + + it('should backfill existing null rows when a field becomes non-nullable on second sync', async () => { + // First sync creates a nullable field with no default. + await syncApplication({ + manifest: buildManifest({ + fields: [buildEstimateFieldManifest({ isNullable: true })], + }), + expectToFail: false, + }); + + // Persist a record whose estimate is NULL on the underlying column. + const recordId = uuidv4(); + const createdRecord = await createTicketRecord({ + id: recordId, + estimate: null, + }); + + expect(createdRecord?.id).toBe(recordId); + expect(createdRecord?.estimate).toBeNull(); + + // Second sync makes the field non-nullable with a default value, which + // must backfill the existing NULL row before SET NOT NULL is enforced. + await syncApplication({ + manifest: buildManifest({ + fields: [ + buildEstimateFieldManifest({ isNullable: false, defaultValue: 42 }), + ], + }), + expectToFail: false, + }); + + const fieldAfterSecondSync = await findFieldWithNullable('estimate'); + + expect(fieldAfterSecondSync?.isNullable).toBe(false); + + const backfilledRecord = await findTicketRecordById(recordId); + + expect(backfilledRecord?.estimate).toBe(42); + }, 60000); + it('should create a unique index when field has isUnique set to true', async () => { await syncApplication({ manifest: buildManifest({