diff --git a/apps/desktop/src/components/structure/TableStructureEditor.primaryKey.spec.ts b/apps/desktop/src/components/structure/TableStructureEditor.primaryKey.spec.ts index aa957db68f..d9627fed7a 100644 --- a/apps/desktop/src/components/structure/TableStructureEditor.primaryKey.spec.ts +++ b/apps/desktop/src/components/structure/TableStructureEditor.primaryKey.spec.ts @@ -263,7 +263,7 @@ function draft(isPrimaryKey = false, identity?: { seed: number; increment: numbe }; } -async function mountEditor(databaseType: "sqlserver" | "postgres" | "sqlite" | "oracle" | "dameng" | "duckdb" | "informix", isPrimaryKey = false, options: { database?: string; dynamicTypes?: string[]; identity?: { seed: number; increment: number } } = {}) { +async function mountEditor(databaseType: "sqlserver" | "postgres" | "sqlite" | "oracle" | "oceanbase-oracle" | "iris" | "dameng" | "duckdb" | "informix", isPrimaryKey = false, options: { database?: string; dynamicTypes?: string[]; identity?: { seed: number; increment: number } } = {}) { mocks.connection.db_type = databaseType; mocks.connection.name = databaseType; mocks.connection.driver_label = databaseType; @@ -315,10 +315,10 @@ async function mountLoadingEditor(initialTab: "columns" | "indexes" | "foreignKe return root; } -function columnCheckbox(root: HTMLElement, header: string): HTMLInputElement { +function columnCheckbox(root: HTMLElement, header: string, rowIndex = 0): HTMLInputElement { const headerIndex = Array.from(root.querySelectorAll("thead th")).findIndex((cell) => cell.textContent?.trim() === header); if (headerIndex < 0) throw new Error(`Missing ${header} column`); - const row = root.querySelector('[data-column-row-index="0"]'); + const row = root.querySelector(`[data-column-row-index="${rowIndex}"]`); const cell = row?.querySelectorAll("td")[headerIndex]; const checkbox = cell?.querySelector('input[type="checkbox"]'); if (!checkbox) throw new Error(`Missing ${header} checkbox`); @@ -428,8 +428,67 @@ describe("TableStructureEditor primary key editing", () => { ); }); - it("keeps the primary-key checkbox disabled for an existing Oracle column", async () => { + it("enables a primary key on an existing keyless Oracle column and makes it not null", async () => { const root = await mountEditor("oracle"); + const primaryKey = columnCheckbox(root, "structureEditor.primaryKey"); + const nullable = columnCheckbox(root, "structureEditor.nullable"); + + expect(primaryKey.disabled).toBe(false); + expect(nullable.checked).toBe(true); + + primaryKey.checked = true; + primaryKey.dispatchEvent(new Event("change", { bubbles: true })); + await nextTick(); + + expect(primaryKey.checked).toBe(true); + expect(nullable.checked).toBe(false); + await vi.waitFor(() => expect(mocks.buildTableStructureChangeSql).toHaveBeenCalled()); + expect(mocks.buildTableStructureChangeSql).toHaveBeenLastCalledWith( + expect.objectContaining({ + columns: [expect.objectContaining({ isPrimaryKey: true, isNullable: false })], + }), + ); + }); + + it("keeps all primary-key checkboxes enabled while composing a new Oracle key", async () => { + const root = await mountEditor("oracle"); + buttonWithText(root, "structureEditor.addColumn").click(); + await nextTick(); + + const firstPrimaryKey = columnCheckbox(root, "structureEditor.primaryKey", 0); + const secondPrimaryKey = columnCheckbox(root, "structureEditor.primaryKey", 1); + expect(firstPrimaryKey.disabled).toBe(false); + expect(secondPrimaryKey.disabled).toBe(false); + + firstPrimaryKey.checked = true; + firstPrimaryKey.dispatchEvent(new Event("change", { bubbles: true })); + await nextTick(); + expect(secondPrimaryKey.disabled).toBe(false); + + secondPrimaryKey.checked = true; + secondPrimaryKey.dispatchEvent(new Event("change", { bubbles: true })); + await nextTick(); + + await vi.waitFor(() => + expect(mocks.buildTableStructureChangeSql).toHaveBeenLastCalledWith( + expect.objectContaining({ + columns: [expect.objectContaining({ isPrimaryKey: true }), expect.objectContaining({ isPrimaryKey: true })], + }), + ), + ); + }); + + it("keeps an existing Oracle primary key and all replacement choices disabled", async () => { + const root = await mountEditor("oracle", true); + buttonWithText(root, "structureEditor.addColumn").click(); + await nextTick(); + + expect(columnCheckbox(root, "structureEditor.primaryKey", 0).disabled).toBe(true); + expect(columnCheckbox(root, "structureEditor.primaryKey", 1).disabled).toBe(true); + }); + + it.each(["oceanbase-oracle", "iris"] as const)("keeps primary-key creation disabled for existing %s tables", async (databaseType) => { + const root = await mountEditor(databaseType); expect(columnCheckbox(root, "structureEditor.primaryKey").disabled).toBe(true); }); diff --git a/apps/desktop/src/components/structure/TableStructureEditor.vue b/apps/desktop/src/components/structure/TableStructureEditor.vue index cdf4bcaf28..988ff3f59c 100644 --- a/apps/desktop/src/components/structure/TableStructureEditor.vue +++ b/apps/desktop/src/components/structure/TableStructureEditor.vue @@ -2433,8 +2433,9 @@ function isColumnCharsetDisabled(column: EditableStructureColumn): boolean { function isPrimaryKeyDisabled(column: EditableStructureColumn): boolean { if (column.markedForDrop) return true; - if (!column.original) return false; - return !structureCapabilities.value.alterPrimaryKey; + if (isCreateMode.value || structureCapabilities.value.alterPrimaryKey) return false; + if (!structureCapabilities.value.addPrimaryKey) return true; + return columns.value.some((candidate) => candidate.original?.is_primary_key); } function canDropColumn(column: EditableStructureColumn): boolean { diff --git a/apps/desktop/src/lib/__tests__/table/tableStructureCapabilities.spec.ts b/apps/desktop/src/lib/__tests__/table/tableStructureCapabilities.spec.ts index 0756746126..f3a5c6caf0 100644 --- a/apps/desktop/src/lib/__tests__/table/tableStructureCapabilities.spec.ts +++ b/apps/desktop/src/lib/__tests__/table/tableStructureCapabilities.spec.ts @@ -93,10 +93,21 @@ describe("tableStructureCapabilities", () => { expect(getTableStructureCapabilities("dameng", "dameng").comment).toBe(true); }); - it("enables alter primary key for Dameng without enabling it for Oracle", () => { - expect(getTableStructureCapabilities("dameng", "dameng").alterPrimaryKey).toBe(true); - expect(getTableStructureCapabilities("oracle", "oracle").alterPrimaryKey).toBe(false); - expect(getTableStructureCapabilities("oceanbase-oracle", "oceanbase-oracle").alterPrimaryKey).toBe(false); + it("separates adding a primary key from replacing an existing one", () => { + expect(getTableStructureCapabilities("dameng", "dameng")).toMatchObject({ + addPrimaryKey: true, + alterPrimaryKey: true, + }); + expect(getTableStructureCapabilities("oracle", "oracle")).toMatchObject({ + addPrimaryKey: true, + alterPrimaryKey: false, + }); + for (const databaseType of ["oceanbase-oracle", "iris"] as const) { + expect(getTableStructureCapabilities(databaseType, databaseType)).toMatchObject({ + addPrimaryKey: false, + alterPrimaryKey: false, + }); + } }); it("uses local-only column reordering for editable databases without physical reorder support", () => { diff --git a/apps/desktop/src/lib/table/tableStructureCapabilities.ts b/apps/desktop/src/lib/table/tableStructureCapabilities.ts index 250f6d9605..93344f37ba 100644 --- a/apps/desktop/src/lib/table/tableStructureCapabilities.ts +++ b/apps/desktop/src/lib/table/tableStructureCapabilities.ts @@ -15,6 +15,7 @@ export interface TableStructureCapabilities { alterType: boolean; alterNullability: boolean; alterDefault: boolean; + addPrimaryKey: boolean; alterPrimaryKey: boolean; reorderColumn: boolean; comment: boolean; @@ -40,6 +41,7 @@ const unsupportedCapabilities: TableStructureCapabilities = { alterType: false, alterNullability: false, alterDefault: false, + addPrimaryKey: false, alterPrimaryKey: false, reorderColumn: false, comment: false, @@ -59,6 +61,9 @@ function capabilities(overrides: Partial): TableStru if (overrides.alterStrategy === undefined && resolved.alterExistingColumn) { resolved.alterStrategy = "direct"; } + if (resolved.alterPrimaryKey) { + resolved.addPrimaryKey = true; + } return resolved; } @@ -129,6 +134,7 @@ const redshiftCapabilities = capabilities({ indexInclude: false, indexFilter: false, indexComment: false, + addPrimaryKey: false, alterPrimaryKey: false, }); @@ -182,7 +188,7 @@ const sqlserverCapabilities = capabilities({ indexComment: true, }); -const oracleCapabilities = capabilities({ +const oracleCompatibleCapabilities = capabilities({ dialect: "oracle", createTable: true, addColumn: true, @@ -199,15 +205,20 @@ const oracleCapabilities = capabilities({ indexType: true, }); +const oracleCapabilities = capabilities({ + ...oracleCompatibleCapabilities, + addPrimaryKey: true, +}); + // Dameng (DM8): ALTER TABLE ... DROP PRIMARY KEY / ADD PRIMARY KEY is official DDL. -// Keep separate from oracleCapabilities so UI cannot enable PK edit without BE drop SQL. +// Keep separate from Oracle-compatible engines so UI cannot enable PK edits without verified DDL. const damengCapabilities = capabilities({ - ...oracleCapabilities, + ...oracleCompatibleCapabilities, alterPrimaryKey: true, }); const irisCapabilities = capabilities({ - ...oracleCapabilities, + ...oracleCompatibleCapabilities, // IRIS exposes %DESCRIPTION at definition time but cannot alter persisted descriptions. comment: false, }); @@ -340,10 +351,10 @@ const capabilityByType: Partial sqlserver: sqlserverCapabilities, oracle: oracleCapabilities, dameng: damengCapabilities, - "oceanbase-oracle": oracleCapabilities, + "oceanbase-oracle": oracleCompatibleCapabilities, iris: irisCapabilities, - yashandb: oracleCapabilities, - xugu: oracleCapabilities, + yashandb: oracleCompatibleCapabilities, + xugu: oracleCompatibleCapabilities, h2: h2Capabilities, access: accessCapabilities, clickhouse: clickhouseCapabilities, diff --git a/crates/dbx-core/src/table_structure_sql.rs b/crates/dbx-core/src/table_structure_sql.rs index 6d1e8c9bbd..eac4b3d574 100644 --- a/crates/dbx-core/src/table_structure_sql.rs +++ b/crates/dbx-core/src/table_structure_sql.rs @@ -22,7 +22,7 @@ pub use types::*; use crate::models::connection::DatabaseType; -use columns::build_column_sql; +use columns::{build_column_sql, validate_primary_key_change_scope}; use comments::build_table_comment_sql; use foreign_keys::build_foreign_key_sql; use indexes::build_index_sql; @@ -35,6 +35,10 @@ pub fn build_table_structure_change_sql(mut options: TableStructureSqlOptions) - if options.is_gaussdb_m_mode { options.database_type = Some(DatabaseType::Mysql); } + let primary_key_errors = validate_primary_key_change_scope(&options); + if !primary_key_errors.is_empty() { + return TableStructureSqlResult { statements: Vec::new(), warnings: primary_key_errors }; + } // Fail closed: an unsupported concurrent-index request (existing index, or // partitioned parent table) must never degrade into blocking index DDL // behind the caller's back, so the whole plan is refused up front. diff --git a/crates/dbx-core/src/table_structure_sql/columns.rs b/crates/dbx-core/src/table_structure_sql/columns.rs index 43b8a0c991..87866ed188 100644 --- a/crates/dbx-core/src/table_structure_sql/columns.rs +++ b/crates/dbx-core/src/table_structure_sql/columns.rs @@ -346,6 +346,32 @@ fn primary_key_change(options: &TableStructureSqlOptions) -> Option) -> String { + let action = if change.old_ids.is_empty() { "Adding" } else { "Changing" }; + format!("{action} primary keys is not supported for {} from this editor.", database_label(options.database_type)) +} + +pub(super) fn validate_primary_key_change_scope(options: &TableStructureSqlOptions) -> Vec { + if let Some(original) = options + .columns + .iter() + .find(|column| column.marked_for_drop && was_primary_key_column(column)) + .and_then(|column| column.original.as_ref()) + { + return vec![format!("Primary key column \"{}\" cannot be dropped from this editor.", original.name)]; + } + + let Some(change) = primary_key_change(options) else { return Vec::new() }; + let capabilities = capabilities_for(options.database_type); + let supported = + if change.old_ids.is_empty() { capabilities.add_primary_key } else { capabilities.alter_primary_key }; + if supported { + Vec::new() + } else { + vec![unsupported_primary_key_change_warning(options, &change)] + } +} + fn mysql_auto_increment_touches_primary_key(column: &EditableStructureColumn, change: &PrimaryKeyChange<'_>) -> bool { column.extra.as_ref().is_some_and(|extra| extra.auto_increment.unwrap_or(false)) && (change.old_ids.contains(column.id.as_str()) || change.new_ids.contains(column.id.as_str())) @@ -363,11 +389,10 @@ pub(super) fn build_primary_key_sql( // columns is not a PK change. A draft that drops a PK column is also rejected here. let Some(change) = primary_key_change(options) else { return Vec::new() }; - if !capabilities.alter_primary_key { - warnings.push(format!( - "Changing primary keys is not supported for {} from this editor.", - database_label(options.database_type) - )); + let supported = + if change.old_ids.is_empty() { capabilities.add_primary_key } else { capabilities.alter_primary_key }; + if !supported { + warnings.push(unsupported_primary_key_change_warning(options, &change)); return Vec::new(); } diff --git a/crates/dbx-core/src/table_structure_sql/dialect.rs b/crates/dbx-core/src/table_structure_sql/dialect.rs index 6ee6c3d148..1fe2885115 100644 --- a/crates/dbx-core/src/table_structure_sql/dialect.rs +++ b/crates/dbx-core/src/table_structure_sql/dialect.rs @@ -43,6 +43,7 @@ pub(super) struct TableStructureCapabilities { pub(super) index_filter: bool, pub(super) index_comment: bool, pub(super) index_concurrent: bool, + pub(super) add_primary_key: bool, pub(super) alter_primary_key: bool, pub(super) foreign_key: bool, } @@ -65,6 +66,7 @@ impl Default for TableStructureCapabilities { index_filter: false, index_comment: false, index_concurrent: false, + add_primary_key: false, alter_primary_key: false, foreign_key: false, } @@ -85,6 +87,7 @@ pub(super) fn gaussdb_m_capabilities() -> TableStructureCapabilities { rebuild_index: true, index_type: true, index_comment: true, + add_primary_key: true, alter_primary_key: true, ..TableStructureCapabilities::default() } @@ -112,6 +115,7 @@ pub(super) fn capabilities_for(database_type: Option) -> TableStru rebuild_index: true, index_type: true, index_comment: true, + add_primary_key: true, alter_primary_key: true, foreign_key: true, ..base @@ -158,6 +162,7 @@ pub(super) fn capabilities_for(database_type: Option) -> TableStru index_include: true, index_filter: true, index_comment: true, + add_primary_key: true, alter_primary_key: true, foreign_key: true, ..base @@ -188,6 +193,7 @@ pub(super) fn capabilities_for(database_type: Option) -> TableStru index_filter: false, index_comment: false, index_concurrent: false, + add_primary_key: false, alter_primary_key: false, foreign_key: false, }, @@ -249,6 +255,7 @@ pub(super) fn capabilities_for(database_type: Option) -> TableStru drop_index: true, rebuild_index: true, index_type: true, + add_primary_key: true, alter_primary_key: true, ..base }, @@ -265,6 +272,7 @@ pub(super) fn capabilities_for(database_type: Option) -> TableStru drop_index: true, rebuild_index: true, index_type: true, + add_primary_key: true, alter_primary_key: true, ..base }, @@ -293,6 +301,7 @@ pub(super) fn capabilities_for(database_type: Option) -> TableStru drop_index: true, rebuild_index: true, index_type: true, + add_primary_key: true, foreign_key: true, ..base }, diff --git a/crates/dbx-core/src/table_structure_sql/tests.rs b/crates/dbx-core/src/table_structure_sql/tests.rs index 303182ec26..915c826e79 100644 --- a/crates/dbx-core/src/table_structure_sql/tests.rs +++ b/crates/dbx-core/src/table_structure_sql/tests.rs @@ -3522,6 +3522,115 @@ fn dameng_blocks_dropping_former_primary_key_column() { assert!(result.warnings.iter().any(|warning| warning.contains("Primary key column"))); } +#[test] +fn oracle_sets_not_null_before_adding_primary_key() { + let mut id = existing_pk_column("id", "NUMBER", false, true); + id.original.as_mut().unwrap().is_nullable = true; + + let result = + build_table_structure_change_sql(structure_change_options(DatabaseType::Oracle, Some("HR"), "users", vec![id])); + + assert_eq!(result.warnings, Vec::::new()); + assert_eq!( + result.statements, + vec![ + "ALTER TABLE \"HR\".\"users\" MODIFY (\"id\" NUMBER NOT NULL);", + "ALTER TABLE \"HR\".\"users\" ADD PRIMARY KEY (\"id\");", + ] + ); +} + +#[test] +fn oracle_adds_composite_primary_key_in_draft_order() { + let mut tenant_id = existing_pk_column("tenant_id", "NUMBER", false, true); + tenant_id.id = "tenant_id".to_string(); + let mut code = existing_pk_column("code", "VARCHAR2(50)", false, true); + code.id = "code".to_string(); + + let result = build_table_structure_change_sql(structure_change_options( + DatabaseType::Oracle, + Some("HR"), + "users", + vec![code, tenant_id], + )); + + assert_eq!(result.warnings, Vec::::new()); + assert_eq!(result.statements, vec!["ALTER TABLE \"HR\".\"users\" ADD PRIMARY KEY (\"code\", \"tenant_id\");"]); +} + +#[test] +fn oracle_adds_new_primary_key_column_before_adding_constraint() { + let mut code = column("code"); + code.data_type = "VARCHAR2(50)".to_string(); + code.is_nullable = false; + code.is_primary_key = true; + + let result = build_table_structure_change_sql(structure_change_options( + DatabaseType::Oracle, + Some("HR"), + "users", + vec![code], + )); + + assert_eq!(result.warnings, Vec::::new()); + assert_eq!( + result.statements, + vec![ + "ALTER TABLE \"HR\".\"users\" ADD (\"code\" VARCHAR2(50));", + "ALTER TABLE \"HR\".\"users\" ADD PRIMARY KEY (\"code\");", + ] + ); +} + +#[test] +fn oracle_rejects_existing_primary_key_changes_without_partial_sql() { + let uncheck = + vec![existing_pk_column("id", "NUMBER", true, false), existing_pk_column("name", "VARCHAR2(50)", false, false)]; + let replacement = vec![ + existing_pk_column("id", "NUMBER", true, false), + existing_pk_column("code", "VARCHAR2(50)", false, true), + existing_pk_column("name", "VARCHAR2(50)", false, false), + ]; + let second_key = vec![ + existing_pk_column("id", "NUMBER", true, true), + existing_pk_column("code", "VARCHAR2(50)", false, true), + existing_pk_column("name", "VARCHAR2(50)", false, false), + ]; + + for (case, columns) in [("uncheck", uncheck), ("replacement", replacement), ("second key", second_key)] { + let mut options = structure_change_options(DatabaseType::Oracle, Some("HR"), "users", columns); + options.indexes = vec![index("idx_users_name", &["name"])]; + + let result = build_table_structure_change_sql(options); + + assert!(result.statements.is_empty(), "{case} must not emit partial SQL: {:?}", result.statements); + assert_eq!(result.warnings.len(), 1, "unexpected {case} warnings: {:?}", result.warnings); + assert!( + result.warnings[0].contains("Changing primary keys"), + "unexpected {case} warning: {:?}", + result.warnings + ); + } +} + +#[test] +fn oracle_compatible_engines_do_not_inherit_oracle_primary_key_add() { + for database_type in [DatabaseType::OceanbaseOracle, DatabaseType::Iris] { + let columns = vec![ + existing_pk_column("id", "NUMBER", false, true), + existing_pk_column("name", "VARCHAR2(50)", false, false), + ]; + let mut options = structure_change_options(database_type, Some("APP"), "users", columns); + options.indexes = vec![index("idx_users_name", &["name"])]; + + let result = build_table_structure_change_sql(options); + + assert!(result.statements.is_empty(), "{database_type:?} must not emit partial SQL: {:?}", result.statements); + assert_eq!(result.warnings.len(), 1, "unexpected {database_type:?} warnings: {:?}", result.warnings); + assert!(result.warnings[0].contains("Adding primary keys")); + } +} + #[test] fn oracle_uncheck_primary_key_and_drop_column_does_not_emit_drop_column() { // alter_primary_key is false for Oracle: unchecking PK must not unlock DROP COLUMN without a PK drop.