From 70605324ecdd00a05da58438a5f13017a5b5cdf9 Mon Sep 17 00:00:00 2001 From: Guy Ben-Aharon Date: Thu, 18 Dec 2025 20:02:33 +0200 Subject: [PATCH] fix: pk indexes sql export (#1024) --- src/lib/data/import-metadata/import/index.ts | 27 ++- .../sql-export/__tests__/export-sql.test.ts | 200 ++++++++++++++++++ .../data/sql-export/export-per-type/mssql.ts | 6 + .../data/sql-export/export-per-type/mysql.ts | 1 + .../sql-export/export-per-type/postgresql.ts | 1 + src/lib/data/sql-export/export-sql-script.ts | 18 +- src/lib/data/sql-import/index.ts | 22 +- .../__tests__/composite-pk-name.test.ts | 7 +- src/lib/domain/db-index.ts | 3 +- 9 files changed, 259 insertions(+), 26 deletions(-) create mode 100644 src/lib/data/sql-export/__tests__/export-sql.test.ts diff --git a/src/lib/data/import-metadata/import/index.ts b/src/lib/data/import-metadata/import/index.ts index 10e0d9ef..81c09255 100644 --- a/src/lib/data/import-metadata/import/index.ts +++ b/src/lib/data/import-metadata/import/index.ts @@ -1,5 +1,9 @@ import type { DatabaseEdition, Diagram } from '@/lib/domain'; -import { adjustTablePositions, DatabaseType } from '@/lib/domain'; +import { + adjustTablePositions, + DatabaseType, + getTableIndexesWithPrimaryKey, +} from '@/lib/domain'; import { generateDiagramId } from '@/lib/utils'; import type { DatabaseMetadata } from '../metadata-types/database-metadata'; import { createCustomTypesFromMetadata } from './custom-types'; @@ -52,14 +56,19 @@ export const loadFromDatabaseMetadata = async ({ mode: 'perSchema', }); - const sortedTables = adjustedTables.sort((a, b) => { - if (a.isView === b.isView) { - // Both are either tables or views, so sort alphabetically by name - return a.name.localeCompare(b.name); - } - // If one is a view and the other is not, put tables first - return a.isView ? 1 : -1; - }); + const sortedTables = adjustedTables + .map((table) => ({ + ...table, + indexes: getTableIndexesWithPrimaryKey({ table }), + })) + .sort((a, b) => { + if (a.isView === b.isView) { + // Both are either tables or views, so sort alphabetically by name + return a.name.localeCompare(b.name); + } + // If one is a view and the other is not, put tables first + return a.isView ? 1 : -1; + }); const diagram: Diagram = { id: generateDiagramId(), diff --git a/src/lib/data/sql-export/__tests__/export-sql.test.ts b/src/lib/data/sql-export/__tests__/export-sql.test.ts new file mode 100644 index 00000000..d9c72946 --- /dev/null +++ b/src/lib/data/sql-export/__tests__/export-sql.test.ts @@ -0,0 +1,200 @@ +import { describe, it, expect } from 'vitest'; +import { exportBaseSQL } from '../export-sql-script'; +import { exportPostgreSQL } from '../export-per-type/postgresql'; +import { exportMySQL } from '../export-per-type/mysql'; +import { exportMSSQL } from '../export-per-type/mssql'; +import { exportSQLite } from '../export-per-type/sqlite'; +import { DatabaseType } from '@/lib/domain/database-type'; +import type { Diagram } from '@/lib/domain/diagram'; +import type { DBTable } from '@/lib/domain/db-table'; +import type { DBField } from '@/lib/domain/db-field'; + +describe('SQL Export Tests', () => { + let idCounter = 0; + const testId = () => `test-id-${++idCounter}`; + const testTime = Date.now(); + + const createField = (overrides: Partial): DBField => + ({ + id: testId(), + name: 'field', + type: { id: 'text', name: 'text' }, + primaryKey: false, + nullable: true, + unique: false, + createdAt: testTime, + ...overrides, + }) as DBField; + + const createTable = (overrides: Partial): DBTable => + ({ + id: testId(), + name: 'table', + fields: [], + indexes: [], + createdAt: testTime, + x: 0, + y: 0, + width: 200, + ...overrides, + }) as DBTable; + + const createDiagram = (overrides: Partial): Diagram => + ({ + id: testId(), + name: 'diagram', + databaseType: DatabaseType.GENERIC, + tables: [], + relationships: [], + createdAt: testTime, + updatedAt: testTime, + ...overrides, + }) as Diagram; + + const createTestDiagramWithPKIndex = ( + databaseType: DatabaseType + ): { diagram: Diagram; fieldId: string } => { + const fieldId = testId(); + const diagram = createDiagram({ + id: testId(), + name: 'PK Test', + databaseType, + tables: [ + createTable({ + id: testId(), + name: 'table_1', + schema: 'public', + fields: [ + createField({ + id: fieldId, + name: 'id', + type: { id: 'bigint', name: 'bigint' }, + primaryKey: true, + nullable: false, + unique: false, + }), + ], + indexes: [ + { + id: testId(), + name: '', // Empty name indicates auto-generated PK index + unique: true, + fieldIds: [fieldId], + createdAt: testTime, + isPrimaryKey: true, + }, + ], + }), + ], + relationships: [], + }); + return { diagram, fieldId }; + }; + + describe('Primary Key Index Export', () => { + describe('exportBaseSQL', () => { + it('should export PRIMARY KEY without CONSTRAINT for PostgreSQL', () => { + const { diagram } = createTestDiagramWithPKIndex( + DatabaseType.POSTGRESQL + ); + + const sql = exportBaseSQL({ + diagram, + targetDatabaseType: DatabaseType.POSTGRESQL, + }); + + expect(sql).toContain('PRIMARY KEY ("id")'); + expect(sql).not.toContain('CONSTRAINT'); + }); + + it('should export PRIMARY KEY without CONSTRAINT for MySQL', () => { + const { diagram } = createTestDiagramWithPKIndex( + DatabaseType.MYSQL + ); + + const sql = exportBaseSQL({ + diagram, + targetDatabaseType: DatabaseType.MYSQL, + }); + + expect(sql).toContain('PRIMARY KEY'); + expect(sql).not.toContain('CONSTRAINT'); + }); + + it('should export PRIMARY KEY without CONSTRAINT for SQL Server', () => { + const { diagram } = createTestDiagramWithPKIndex( + DatabaseType.SQL_SERVER + ); + + const sql = exportBaseSQL({ + diagram, + targetDatabaseType: DatabaseType.SQL_SERVER, + }); + + expect(sql).toContain('PRIMARY KEY'); + expect(sql).not.toContain('CONSTRAINT'); + }); + + it('should export PRIMARY KEY without CONSTRAINT for SQLite', () => { + const { diagram } = createTestDiagramWithPKIndex( + DatabaseType.SQLITE + ); + + const sql = exportBaseSQL({ + diagram, + targetDatabaseType: DatabaseType.SQLITE, + }); + + expect(sql).toContain('PRIMARY KEY'); + expect(sql).not.toContain('CONSTRAINT'); + }); + }); + + describe('Database-specific exporters', () => { + it('exportPostgreSQL: should export PRIMARY KEY without CONSTRAINT', () => { + const { diagram } = createTestDiagramWithPKIndex( + DatabaseType.POSTGRESQL + ); + + const sql = exportPostgreSQL({ diagram }); + + expect(sql).toContain('PRIMARY KEY ("id")'); + expect(sql).not.toContain('CONSTRAINT'); + }); + + it('exportMySQL: should export PRIMARY KEY without CONSTRAINT', () => { + const { diagram } = createTestDiagramWithPKIndex( + DatabaseType.MYSQL + ); + + const sql = exportMySQL({ diagram }); + + expect(sql).toContain('PRIMARY KEY (`id`)'); + expect(sql).not.toContain('CONSTRAINT'); + }); + + it('exportMSSQL: should export PRIMARY KEY without CONSTRAINT', () => { + const { diagram } = createTestDiagramWithPKIndex( + DatabaseType.SQL_SERVER + ); + + const sql = exportMSSQL({ diagram }); + + expect(sql).toContain('PRIMARY KEY ([id])'); + expect(sql).not.toContain('CONSTRAINT'); + }); + + it('exportSQLite: should export PRIMARY KEY without CONSTRAINT', () => { + const { diagram } = createTestDiagramWithPKIndex( + DatabaseType.SQLITE + ); + + const sql = exportSQLite({ diagram }); + + // SQLite uses inline PRIMARY KEY for single integer columns + expect(sql).toContain('PRIMARY KEY'); + expect(sql).not.toContain('CONSTRAINT'); + }); + }); + }); +}); 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 071a6ab2..1cda3621 100644 --- a/src/lib/data/sql-export/export-per-type/mssql.ts +++ b/src/lib/data/sql-export/export-per-type/mssql.ts @@ -180,6 +180,7 @@ export function exportMSSQL({ 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 ); @@ -194,6 +195,11 @@ export function exportMSSQL({ }\n);\n${(() => { const validIndexes = table.indexes .map((index) => { + // Skip primary key indexes - they're already handled as constraints + if (index.isPrimaryKey) { + return ''; + } + const indexName = table.schema ? `[${table.schema}_${index.name}]` : `[${index.name}]`; 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 c8a75ab6..4da2ec57 100644 --- a/src/lib/data/sql-export/export-per-type/mysql.ts +++ b/src/lib/data/sql-export/export-per-type/mysql.ts @@ -315,6 +315,7 @@ export function exportMySQL({ 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 ); 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 daaaedb1..dc291f2a 100644 --- a/src/lib/data/sql-export/export-per-type/postgresql.ts +++ b/src/lib/data/sql-export/export-per-type/postgresql.ts @@ -331,6 +331,7 @@ export function exportPostgreSQL({ 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 ); diff --git a/src/lib/data/sql-export/export-sql-script.ts b/src/lib/data/sql-export/export-sql-script.ts index e3c102ff..2ab316d7 100644 --- a/src/lib/data/sql-export/export-sql-script.ts +++ b/src/lib/data/sql-export/export-sql-script.ts @@ -469,9 +469,15 @@ export const exportBaseSQL = ({ } } - // Handle PRIMARY KEY constraint - only add inline if no PK index with custom name + // Handle PRIMARY KEY constraint - only add inline if single PK without named constraint const pkIndex = table.indexes.find((idx) => idx.isPrimaryKey); - if (field.primaryKey && !hasCompositePrimaryKey && !pkIndex?.name) { + // Only use CONSTRAINT syntax if PK index has a non-empty name + const useNamedConstraint = !!pkIndex?.name; + if ( + field.primaryKey && + !hasCompositePrimaryKey && + !useNamedConstraint + ) { sqlScript += ' PRIMARY KEY'; // For SQLite with DBML flow, add AUTOINCREMENT after PRIMARY KEY @@ -489,7 +495,7 @@ export const exportBaseSQL = ({ // Add a comma after each field except the last one (or before PK constraint) const needsPKConstraint = hasCompositePrimaryKey || - (primaryKeyFields.length === 1 && pkIndex?.name); + (primaryKeyFields.length === 1 && useNamedConstraint); if (index < table.fields.length - 1 || needsPKConstraint) { sqlScript += ',\n'; } @@ -497,14 +503,16 @@ export const exportBaseSQL = ({ // 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; if ( hasCompositePrimaryKey || - (primaryKeyFields.length === 1 && pkIndex?.name) + (primaryKeyFields.length === 1 && useNamedConstraint) ) { const pkFieldNames = primaryKeyFields .map((f) => getQuotedFieldName(f.name, isDBMLFlow)) .join(', '); - if (pkIndex?.name) { + if (useNamedConstraint) { sqlScript += `\n CONSTRAINT ${pkIndex.name} PRIMARY KEY (${pkFieldNames})`; } else { sqlScript += `\n PRIMARY KEY (${pkFieldNames})`; diff --git a/src/lib/data/sql-import/index.ts b/src/lib/data/sql-import/index.ts index be2b8b81..277918e6 100644 --- a/src/lib/data/sql-import/index.ts +++ b/src/lib/data/sql-import/index.ts @@ -10,6 +10,7 @@ import type { SQLParserResult } from './common'; import { convertToChartDBDiagram } from './common'; import { adjustTablePositions } from '@/lib/domain/db-table'; import { fromMySQL, isMySQLFormat } from './dialect-importers/mysql/mysql'; +import { getTableIndexesWithPrimaryKey } from '@/lib/domain/db-index'; /** * Detect if SQL content is from pg_dump format @@ -235,14 +236,19 @@ export async function sqlImportToDiagram({ mode: 'perSchema', }); - const sortedTables = adjustedTables.sort((a, b) => { - if (a.isView === b.isView) { - // Both are either tables or views, so sort alphabetically by name - return a.name.localeCompare(b.name); - } - // If one is a view and the other is not, put tables first - return a.isView ? 1 : -1; - }); + const sortedTables = adjustedTables + .map((table) => ({ + ...table, + indexes: getTableIndexesWithPrimaryKey({ table }), + })) + .sort((a, b) => { + if (a.isView === b.isView) { + // Both are either tables or views, so sort alphabetically by name + return a.name.localeCompare(b.name); + } + // If one is a view and the other is not, put tables first + return a.isView ? 1 : -1; + }); return { ...diagram, 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 6cc40513..b2f59edf 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 @@ -177,14 +177,15 @@ Table "simple" { expect(diagram.tables).toBeDefined(); const table = diagram.tables![0]; - // PK index should not exist for composite PK without name + // PK index should exist but with empty name (auto-generated) const pkIndex = table.indexes.find((idx) => idx.isPrimaryKey); expect(pkIndex).toBeDefined(); + expect(pkIndex!.name).toBe(''); const sqlScript = exportPostgreSQL({ diagram }); - // Should have unnamed PRIMARY KEY + // Should have unnamed PRIMARY KEY (no CONSTRAINT for auto-generated PK index) expect(sqlScript).toContain('PRIMARY KEY ("x", "y")'); - expect(sqlScript).toContain('CONSTRAINT'); + expect(sqlScript).not.toContain('CONSTRAINT'); }); }); diff --git a/src/lib/domain/db-index.ts b/src/lib/domain/db-index.ts index b9668daf..cebcc295 100644 --- a/src/lib/domain/db-index.ts +++ b/src/lib/domain/db-index.ts @@ -66,9 +66,10 @@ export const getTablePrimaryKeyIndex = ({ }; } else { // Create new PK index for primary key(s) + // Use empty name for auto-generated PK indexes to indicate no CONSTRAINT should be used const pkIndex: DBIndex = { id: generateId(), - name: `pk_${table.name}_${primaryKeyFields.map((f) => f.name).join('_')}`, + name: '', fieldIds: pkFieldIds, unique: true, isPrimaryKey: true,