From b21202e6c7ecc1807a1ef09853532f1282c6f88e Mon Sep 17 00:00:00 2001 From: Jonathan Fishner Date: Tue, 13 Jan 2026 17:53:00 +0200 Subject: [PATCH] fix: enforce NOT NULL for primary key fields (#1061) --- src/hooks/use-update-table-field.ts | 19 +++++++++ src/lib/clone.ts | 3 ++ .../cross-dialect/postgresql/to-mssql.ts | 9 +---- .../cross-dialect/postgresql/to-mysql.ts | 10 +---- .../data/sql-export/export-per-type/mssql.ts | 11 +---- .../data/sql-export/export-per-type/mysql.ts | 11 +---- .../sql-export/export-per-type/postgresql.ts | 11 +---- src/lib/data/sql-export/export-sql-script.ts | 40 +++++-------------- .../__tests__/composite-pk-name.test.ts | 32 +++++++-------- .../table-edit-mode/table-edit-mode-field.tsx | 2 +- .../table-field/table-field.tsx | 6 ++- 11 files changed, 59 insertions(+), 95 deletions(-) diff --git a/src/hooks/use-update-table-field.ts b/src/hooks/use-update-table-field.ts index f25f92b8..e38306a1 100644 --- a/src/hooks/use-update-table-field.ts +++ b/src/hooks/use-update-table-field.ts @@ -103,6 +103,20 @@ export const useUpdateTableField = ( setLocalPrimaryKey(field.primaryKey); }, [field.nullable, field.primaryKey]); + // Auto-correct: Primary key fields must be NOT NULL + // This fixes any existing data where PK columns were incorrectly set as nullable + useEffect(() => { + if (field.primaryKey && field.nullable) { + chartDBUpdateField(table.id, field.id, { nullable: false }); + } + }, [ + field.primaryKey, + field.nullable, + table.id, + field.id, + chartDBUpdateField, + ]); + // Use custom updateField if provided, otherwise use the chartDB one const updateField = useMemo( () => @@ -296,6 +310,7 @@ export const useUpdateTableField = ( // When setting as primary key const updates: Partial = { primaryKey: true, + nullable: false, // Primary keys must be NOT NULL }; // Only auto-set unique if this will be the only primary key if (primaryKeyCount === 0) { @@ -318,6 +333,10 @@ export const useUpdateTableField = ( const handlePrimaryKeyToggle = useCallback( (value: boolean) => { setLocalPrimaryKey(value); + // Primary keys must be NOT NULL - update local state immediately for responsive UI + if (value) { + setLocalNullable(false); + } debouncedPrimaryKeyUpdate(value, primaryKeyCount); }, [primaryKeyCount, debouncedPrimaryKeyUpdate] diff --git a/src/lib/clone.ts b/src/lib/clone.ts index e7163c50..bca48bca 100644 --- a/src/lib/clone.ts +++ b/src/lib/clone.ts @@ -120,6 +120,9 @@ export const cloneTable = ( .map((id) => getNewId(id)) .filter((fieldId): fieldId is string => fieldId !== null), id, + // Clear the name for primary key indexes to avoid duplicate constraint names + // when exporting SQL scripts (database will auto-generate unique names) + name: index.isPrimaryKey ? '' : index.name, }; }) .filter((index): index is DBIndex => index !== null); diff --git a/src/lib/data/sql-export/cross-dialect/postgresql/to-mssql.ts b/src/lib/data/sql-export/cross-dialect/postgresql/to-mssql.ts index 1c28cbe1..5d9d12af 100644 --- a/src/lib/data/sql-export/cross-dialect/postgresql/to-mssql.ts +++ b/src/lib/data/sql-export/cross-dialect/postgresql/to-mssql.ts @@ -473,14 +473,7 @@ export function exportPostgreSQLToMSSQL({ }CREATE TABLE ${tableName} (\n${fieldDefinitions.join('\n')}${ // Add PRIMARY KEY as table constraint primaryKeyFields.length > 0 - ? `\n ${(() => { - const pkIndex = table.indexes.find( - (idx) => idx.isPrimaryKey - ); - return pkIndex?.name - ? `CONSTRAINT [${pkIndex.name}] ` - : ''; - })()}PRIMARY KEY (${primaryKeyFields + ? `\n PRIMARY KEY (${primaryKeyFields .map((f) => `[${f.name}]`) .join( ', ' diff --git a/src/lib/data/sql-export/cross-dialect/postgresql/to-mysql.ts b/src/lib/data/sql-export/cross-dialect/postgresql/to-mysql.ts index e98d87c5..7284b4ac 100644 --- a/src/lib/data/sql-export/cross-dialect/postgresql/to-mysql.ts +++ b/src/lib/data/sql-export/cross-dialect/postgresql/to-mysql.ts @@ -397,15 +397,7 @@ export function exportPostgreSQLToMySQL({ }\nCREATE TABLE IF NOT EXISTS ${tableName} (\n${fieldDefinitions.join('\n')}${ // Add PRIMARY KEY as table constraint primaryKeyFields.length > 0 - ? `\n ${(() => { - // Find PK index to get the constraint name - const pkIndex = table.indexes.find( - (idx) => idx.isPrimaryKey - ); - return pkIndex?.name - ? `CONSTRAINT \`${pkIndex.name}\` ` - : ''; - })()}PRIMARY KEY (${primaryKeyFields + ? `\n PRIMARY KEY (${primaryKeyFields .map((f) => `\`${f.name}\``) .join( ', ' diff --git a/src/lib/data/sql-export/export-per-type/mssql.ts b/src/lib/data/sql-export/export-per-type/mssql.ts index ab30149a..7b4c93b8 100644 --- a/src/lib/data/sql-export/export-per-type/mssql.ts +++ b/src/lib/data/sql-export/export-per-type/mssql.ts @@ -184,16 +184,7 @@ export function exportMSSQL({ }) .join(',\n')}${ table.fields.filter((f) => f.primaryKey).length > 0 - ? `,\n ${(() => { - // Find PK index to get the constraint name - // Only use CONSTRAINT syntax if PK index has a non-empty name - const pkIndex = table.indexes.find( - (idx) => idx.isPrimaryKey - ); - return pkIndex?.name - ? `CONSTRAINT [${pkIndex.name}] ` - : ''; - })()}PRIMARY KEY (${table.fields + ? `,\n PRIMARY KEY (${table.fields .filter((f) => f.primaryKey) .map((f) => `[${f.name}]`) .join(', ')})` diff --git a/src/lib/data/sql-export/export-per-type/mysql.ts b/src/lib/data/sql-export/export-per-type/mysql.ts index f8612709..8be10230 100644 --- a/src/lib/data/sql-export/export-per-type/mysql.ts +++ b/src/lib/data/sql-export/export-per-type/mysql.ts @@ -313,16 +313,7 @@ export function exportMySQL({ .join(',\n')}${ // Add PRIMARY KEY as table constraint primaryKeyFields.length > 0 - ? `,\n ${(() => { - // Find PK index to get the constraint name - // Only use CONSTRAINT syntax if PK index has a non-empty name - const pkIndex = table.indexes.find( - (idx) => idx.isPrimaryKey - ); - return pkIndex?.name - ? `CONSTRAINT \`${pkIndex.name}\` ` - : ''; - })()}PRIMARY KEY (${primaryKeyFields + ? `,\n PRIMARY KEY (${primaryKeyFields .map((f) => `\`${f.name}\``) .join(', ')})` : '' diff --git a/src/lib/data/sql-export/export-per-type/postgresql.ts b/src/lib/data/sql-export/export-per-type/postgresql.ts index 6b7829f9..bf22dcb5 100644 --- a/src/lib/data/sql-export/export-per-type/postgresql.ts +++ b/src/lib/data/sql-export/export-per-type/postgresql.ts @@ -329,16 +329,7 @@ export function exportPostgreSQL({ }) .join(',\n')}${ primaryKeyFields.length > 0 - ? `,\n ${(() => { - // Find PK index to get the constraint name - // Only use CONSTRAINT syntax if PK index has a non-empty name - const pkIndex = table.indexes.find( - (idx) => idx.isPrimaryKey - ); - return pkIndex?.name - ? `CONSTRAINT "${pkIndex.name}" ` - : ''; - })()}PRIMARY KEY (${primaryKeyFields + ? `,\n PRIMARY KEY (${primaryKeyFields .map((f) => `"${f.name}"`) .join(', ')})` : '' diff --git a/src/lib/data/sql-export/export-sql-script.ts b/src/lib/data/sql-export/export-sql-script.ts index 806d6133..f3202934 100644 --- a/src/lib/data/sql-export/export-sql-script.ts +++ b/src/lib/data/sql-export/export-sql-script.ts @@ -511,15 +511,9 @@ export const exportBaseSQL = ({ } } - // Handle PRIMARY KEY constraint - only add inline if single PK without named constraint - const pkIndex = table.indexes.find((idx) => idx.isPrimaryKey); - // Only use CONSTRAINT syntax if PK index has a non-empty name - const useNamedConstraint = !!pkIndex?.name; - if ( - field.primaryKey && - !hasCompositePrimaryKey && - !useNamedConstraint - ) { + // Handle PRIMARY KEY constraint - add inline for single PK fields + // Never use named constraints to avoid duplicate constraint name issues + if (field.primaryKey && !hasCompositePrimaryKey) { sqlScript += ' PRIMARY KEY'; // For SQLite with DBML flow, add AUTOINCREMENT after PRIMARY KEY @@ -534,31 +528,19 @@ export const exportBaseSQL = ({ } } - // Add a comma after each field except the last one (or before PK constraint) - const needsPKConstraint = - hasCompositePrimaryKey || - (primaryKeyFields.length === 1 && useNamedConstraint); - if (index < table.fields.length - 1 || needsPKConstraint) { + // Add a comma after each field except the last one (or before composite PK constraint) + if (index < table.fields.length - 1 || hasCompositePrimaryKey) { sqlScript += ',\n'; } }); - // Add primary key constraint if needed (for composite PKs or single PK with custom name) - const pkIndex = table.indexes.find((idx) => idx.isPrimaryKey); - // Only use CONSTRAINT syntax if PK index has a non-empty name - const useNamedConstraint = !!pkIndex?.name; - const needsPKConstraint = - hasCompositePrimaryKey || - (primaryKeyFields.length === 1 && useNamedConstraint); - if (needsPKConstraint) { + // Add primary key constraint for composite PKs only (single PKs are inline) + // Never use named constraints to avoid duplicate constraint name issues + if (hasCompositePrimaryKey) { const pkFieldNames = primaryKeyFields .map((f) => getQuotedFieldName(f.name, isDBMLFlow)) .join(', '); - if (useNamedConstraint) { - sqlScript += `\n CONSTRAINT ${pkIndex.name} PRIMARY KEY (${pkFieldNames})`; - } else { - sqlScript += `\n PRIMARY KEY (${pkFieldNames})`; - } + sqlScript += `\n PRIMARY KEY (${pkFieldNames})`; } // Add CHECK constraints (only for databases that support them, filter out empty) @@ -568,10 +550,10 @@ export const exportBaseSQL = ({ ); if (validCheckConstraints.length > 0 && dbSupportsChecks) { validCheckConstraints.forEach((checkConstraint, idx) => { - // Add comma if needed (after fields or PK constraint) + // Add comma if needed (after fields or composite PK constraint) if ( idx === 0 && - (table.fields.length > 0 || needsPKConstraint) + (table.fields.length > 0 || hasCompositePrimaryKey) ) { sqlScript += ','; } else if (idx > 0) { diff --git a/src/lib/dbml/dbml-import/__tests__/composite-pk-name.test.ts b/src/lib/dbml/dbml-import/__tests__/composite-pk-name.test.ts index b2f59edf..210f76a0 100644 --- a/src/lib/dbml/dbml-import/__tests__/composite-pk-name.test.ts +++ b/src/lib/dbml/dbml-import/__tests__/composite-pk-name.test.ts @@ -53,12 +53,12 @@ Table "landlord"."users_master_table" { expect(uniqueIndex!.unique).toBe(true); }); - it('should export composite primary key with CONSTRAINT name in PostgreSQL', async () => { + it('should export composite primary key without CONSTRAINT name in PostgreSQL (auto-generated)', async () => { const dbmlContent = ` Table "users" { "id" bigint [not null] "tenant_id" bigint [not null] - + Indexes { (id, tenant_id) [pk, name: "pk_users_composite"] } @@ -71,19 +71,17 @@ Table "users" { const sqlScript = exportPostgreSQL({ diagram }); - // Check that the SQL contains the named constraint - expect(sqlScript).toContain( - 'CONSTRAINT "pk_users_composite" PRIMARY KEY ("id", "tenant_id")' - ); - expect(sqlScript).not.toContain('PRIMARY KEY ("id", "tenant_id"),'); // Should not have unnamed PK + // PK constraint names are auto-generated by the database to avoid duplicates + expect(sqlScript).toContain('PRIMARY KEY ("id", "tenant_id")'); + expect(sqlScript).not.toContain('CONSTRAINT "pk_users_composite"'); }); - it('should export composite primary key with CONSTRAINT name in MySQL', async () => { + it('should export composite primary key without CONSTRAINT name in MySQL (auto-generated)', async () => { const dbmlContent = ` Table "orders" { "order_id" int [not null] "product_id" int [not null] - + Indexes { (order_id, product_id) [pk, name: "orders_order_product_pk"] } @@ -96,18 +94,17 @@ Table "orders" { const sqlScript = exportMySQL({ diagram }); - // Check that the SQL contains the named constraint - expect(sqlScript).toContain( - 'CONSTRAINT `orders_order_product_pk` PRIMARY KEY (`order_id`, `product_id`)' - ); + // PK constraint names are auto-generated by the database to avoid duplicates + expect(sqlScript).toContain('PRIMARY KEY (`order_id`, `product_id`)'); + expect(sqlScript).not.toContain('CONSTRAINT `orders_order_product_pk`'); }); - it('should export composite primary key with CONSTRAINT name in MSSQL', async () => { + it('should export composite primary key without CONSTRAINT name in MSSQL (auto-generated)', async () => { const dbmlContent = ` Table "products" { "category_id" int [not null] "product_id" int [not null] - + Indexes { (category_id, product_id) [pk, name: "pk_products"] } @@ -120,10 +117,11 @@ Table "products" { const sqlScript = exportMSSQL({ diagram }); - // Check that the SQL contains the named constraint + // PK constraint names are auto-generated by the database to avoid duplicates expect(sqlScript).toContain( - 'CONSTRAINT [pk_products] PRIMARY KEY ([category_id], [product_id])' + 'PRIMARY KEY ([category_id], [product_id])' ); + expect(sqlScript).not.toContain('CONSTRAINT [pk_products]'); }); it('should merge duplicate PK index with name', async () => { diff --git a/src/pages/editor-page/canvas/table-node/table-edit-mode/table-edit-mode-field.tsx b/src/pages/editor-page/canvas/table-node/table-edit-mode/table-edit-mode-field.tsx index 3b604127..ba4e4f62 100644 --- a/src/pages/editor-page/canvas/table-node/table-edit-mode/table-edit-mode-field.tsx +++ b/src/pages/editor-page/canvas/table-node/table-edit-mode/table-edit-mode-field.tsx @@ -155,7 +155,7 @@ export const TableEditModeField: React.FC = React.memo( N diff --git a/src/pages/editor-page/side-panel/tables-section/table-list/table-list-item/table-list-item-content/table-field/table-field.tsx b/src/pages/editor-page/side-panel/tables-section/table-list/table-list-item/table-list-item-content/table-field/table-field.tsx index c1af93b6..779f3e6c 100644 --- a/src/pages/editor-page/side-panel/tables-section/table-list/table-list-item/table-list-item-content/table-field/table-field.tsx +++ b/src/pages/editor-page/side-panel/tables-section/table-list/table-list-item/table-list-item-content/table-field/table-field.tsx @@ -161,7 +161,11 @@ export const TableField: React.FC = ({ N