diff --git a/crates/dbx-core/src/table_structure_sql/column_alter.rs b/crates/dbx-core/src/table_structure_sql/column_alter.rs index cf4019953e..00065e45b3 100644 --- a/crates/dbx-core/src/table_structure_sql/column_alter.rs +++ b/crates/dbx-core/src/table_structure_sql/column_alter.rs @@ -85,6 +85,7 @@ pub fn build_single_column_alter_sql(options: SingleColumnAlterSqlOptions) -> Ta &options.column, options.schema.as_deref(), &options.table_name, + &mut warnings, )), StructureDialect::Sqlite => { statements.extend(build_sqlite_existing_column_sql(&table, &options.column, &mut warnings)) @@ -117,6 +118,66 @@ fn original_manticore_extra_flags(extra: &str) -> (bool, bool, bool, bool) { ) } +fn original_has_auto_increment(extra: &str) -> bool { + let lower = extra.to_lowercase(); + lower.contains("auto_increment") || lower.contains("autoincrement") +} + +fn original_has_identity(extra: &str) -> bool { + extra.to_lowercase().contains("identity") +} + +fn sqlserver_identity_values(extra: &str) -> Option<(Option, Option)> { + let lower = extra.to_lowercase(); + let identity_index = lower.find("identity")?; + let after_identity = extra.get(identity_index + "identity".len()..)?.trim_start(); + if !after_identity.starts_with('(') { + return Some((None, None)); + } + let close_index = after_identity.find(')')?; + let args = &after_identity[1..close_index]; + let mut parts = args.split(',').map(|part| part.trim().parse::().ok()); + Some((parts.next().flatten(), parts.next().flatten())) +} + +fn sqlserver_identity_matches_original(identity: &super::types::ColumnIdentity, original_extra: &str) -> bool { + let Some((original_seed, original_increment)) = sqlserver_identity_values(original_extra) else { + return false; + }; + identity.seed.unwrap_or(1) == original_seed.unwrap_or(1) + && identity.increment.unwrap_or(1) == original_increment.unwrap_or(1) +} + +fn parse_i64_after_phrase(value: &str, lower: &str, phrase: &str) -> Option { + let start = lower.find(phrase)? + phrase.len(); + let rest = value.get(start..)?.trim_start(); + let token = rest.split(|ch: char| ch.is_whitespace() || ch == ')' || ch == ',').find(|part| !part.is_empty())?; + token.parse::().ok() +} + +fn postgres_identity_values(extra: &str) -> Option<(String, Option, Option)> { + let lower = extra.to_lowercase(); + let generation = if lower.contains("generated always as identity") { + "ALWAYS".to_string() + } else if lower.contains("generated by default as identity") { + "BY DEFAULT".to_string() + } else { + return None; + }; + let seed = parse_i64_after_phrase(extra, &lower, "start with"); + let increment = parse_i64_after_phrase(extra, &lower, "increment by"); + Some((generation, seed, increment)) +} + +fn identity_matches_original(identity: &super::types::ColumnIdentity, original_extra: &str) -> bool { + if let Some((generation, original_seed, original_increment)) = postgres_identity_values(original_extra) { + return identity.generation.as_deref().unwrap_or("BY DEFAULT").eq_ignore_ascii_case(&generation) + && identity.seed.unwrap_or(1) == original_seed.unwrap_or(1) + && identity.increment.unwrap_or(1) == original_increment.unwrap_or(1); + } + sqlserver_identity_matches_original(identity, original_extra) +} + pub(super) fn has_column_extra_change(column: &EditableStructureColumn) -> bool { let Some(original) = &column.original else { return false }; let current_extra = column.extra.as_ref(); @@ -128,8 +189,9 @@ pub(super) fn has_column_extra_change(column: &EditableStructureColumn) -> bool (Some(curr), Some(orig)) if is_column_extra_empty(curr) => { let (indexed, stored, attribute, secondary_index) = original_manticore_extra_flags(orig); let orig_lower = orig.to_lowercase(); - orig_lower.contains("auto_increment") + original_has_auto_increment(orig) || orig_lower.contains("on update") + || original_has_identity(orig) || indexed || stored || attribute @@ -142,10 +204,16 @@ pub(super) fn has_column_extra_change(column: &EditableStructureColumn) -> bool (Some(curr), Some(orig)) => { let orig_lower = orig.to_lowercase(); let curr_has_ai = curr.auto_increment.unwrap_or(false); - let orig_has_ai = orig_lower.contains("auto_increment"); + let orig_has_identity = original_has_identity(orig); + let orig_has_ai = original_has_auto_increment(orig) || (orig_has_identity && curr_has_ai); let curr_has_on_update = curr.on_update_current_timestamp.unwrap_or(false); let orig_has_on_update = orig_lower.contains("on update"); - let curr_has_identity = curr.identity.is_some(); + let identity_changed = match (&curr.identity, orig_has_identity) { + (Some(identity), true) => !identity_matches_original(identity, orig), + (Some(_), false) => true, + (None, true) => !curr_has_ai, + (None, false) => false, + }; let curr_manticore = ( curr.manticore_indexed.unwrap_or(false), curr.manticore_stored.unwrap_or(false), @@ -153,10 +221,9 @@ pub(super) fn has_column_extra_change(column: &EditableStructureColumn) -> bool curr.manticore_secondary_index.unwrap_or(false), ); let orig_manticore = original_manticore_extra_flags(orig); - // identity is harder to detect in free-form original.extra, so treat it as changed if present curr_has_ai != orig_has_ai || curr_has_on_update != orig_has_on_update - || curr_has_identity + || identity_changed || curr_manticore != orig_manticore } } @@ -322,6 +389,7 @@ pub(super) fn build_sqlserver_existing_column_sql( column: &EditableStructureColumn, schema: Option<&str>, table_name: &str, + warnings: &mut Vec, ) -> Vec { let Some(original) = &column.original else { return Vec::new(); @@ -330,6 +398,13 @@ pub(super) fn build_sqlserver_existing_column_sql( let mut statements = Vec::new(); let mut current_name = original.name.clone(); + if has_sqlserver_identity_change(column) { + warnings.push(format!( + "Changing SQL Server IDENTITY for existing column \"{}\" is not supported from this editor.", + original.name + )); + } + // Rename column via sp_rename if column.name != original.name { let full_obj_path = format!("{table}.{}", quote_ident(dialect, &original.name)); @@ -409,6 +484,28 @@ pub(super) fn build_sqlserver_existing_column_sql( statements } +fn has_sqlserver_identity_change(column: &EditableStructureColumn) -> bool { + let Some(original) = &column.original else { + return false; + }; + let original_extra = original.extra.as_deref().unwrap_or(""); + let original_identity = original_has_identity(original_extra); + let current_extra = column.extra.as_ref(); + let current_identity = + current_extra.is_some_and(|extra| extra.auto_increment.unwrap_or(false) || extra.identity.is_some()); + + if current_identity != original_identity { + return true; + } + if !current_identity { + return false; + } + + current_extra + .and_then(|extra| extra.identity.as_ref()) + .is_some_and(|identity| !sqlserver_identity_matches_original(identity, original_extra)) +} + pub(super) fn build_h2_existing_column_sql(table: &str, column: &EditableStructureColumn) -> Vec { let Some(original) = &column.original else { return Vec::new(); diff --git a/crates/dbx-core/src/table_structure_sql/columns.rs b/crates/dbx-core/src/table_structure_sql/columns.rs index ac3e7c8a54..1a3e7fae6f 100644 --- a/crates/dbx-core/src/table_structure_sql/columns.rs +++ b/crates/dbx-core/src/table_structure_sql/columns.rs @@ -127,6 +127,7 @@ pub(super) fn build_column_sql(options: &TableStructureSqlOptions, warnings: &mu column, options.schema.as_deref(), &options.table_name, + warnings, )), StructureDialect::Sqlite => statements.extend(build_sqlite_existing_column_sql(&table, column, warnings)), StructureDialect::Questdb => statements.extend(build_questdb_existing_column_sql(&table, column)), diff --git a/crates/dbx-core/src/table_structure_sql/foreign_keys.rs b/crates/dbx-core/src/table_structure_sql/foreign_keys.rs index 22668af5dd..ade00f71a3 100644 --- a/crates/dbx-core/src/table_structure_sql/foreign_keys.rs +++ b/crates/dbx-core/src/table_structure_sql/foreign_keys.rs @@ -11,7 +11,9 @@ pub(super) fn build_foreign_key_sql(options: &TableStructureSqlOptions, warnings let capabilities = super::dialect::capabilities_for(options.database_type); let dialect = capabilities.dialect; if !capabilities.foreign_key { - warnings.push(format!("Editing foreign keys is not supported for {database_label} from this editor.")); + if has_foreign_key_edits(&options.foreign_keys) { + warnings.push(format!("Editing foreign keys is not supported for {database_label} from this editor.")); + } return Vec::new(); } @@ -41,6 +43,29 @@ pub(super) fn build_foreign_key_sql(options: &TableStructureSqlOptions, warnings statements } +fn has_foreign_key_edits(foreign_keys: &[EditableStructureForeignKey]) -> bool { + foreign_keys.iter().any(|foreign_key| { + if foreign_key.marked_for_drop { + return foreign_key.original.is_some(); + } + + match &foreign_key.original { + Some(original) => has_foreign_key_change(foreign_key, original), + None => has_foreign_key_definition(foreign_key), + } + }) +} + +fn has_foreign_key_definition(foreign_key: &EditableStructureForeignKey) -> bool { + !clean(&foreign_key.name).is_empty() + || !clean(&foreign_key.column).is_empty() + || !clean(&foreign_key.ref_schema).is_empty() + || !clean(&foreign_key.ref_table).is_empty() + || !clean(&foreign_key.ref_column).is_empty() + || !clean(&foreign_key.on_update).is_empty() + || !clean(&foreign_key.on_delete).is_empty() +} + fn has_foreign_key_change(foreign_key: &EditableStructureForeignKey, original: &ForeignKeyInfo) -> bool { clean(&foreign_key.name) != clean(&original.name) || clean(&foreign_key.column) != clean(&original.column) diff --git a/crates/dbx-core/src/table_structure_sql/tests.rs b/crates/dbx-core/src/table_structure_sql/tests.rs index 70300c1563..1f59dbd4f9 100644 --- a/crates/dbx-core/src/table_structure_sql/tests.rs +++ b/crates/dbx-core/src/table_structure_sql/tests.rs @@ -1000,6 +1000,159 @@ fn builds_sql_server_quoted_column_and_index_statements() { ); } +#[test] +fn sqlserver_unchanged_foreign_key_does_not_warn_when_saving_other_changes() { + let mut email = column("email"); + email.data_type = "nvarchar(255)".to_string(); + email.is_nullable = false; + + let mut user_fk = foreign_key("fk_orders_user_id", "user_id", "users", "id"); + user_fk.ref_schema = "dbo".to_string(); + user_fk.original = Some(ForeignKeyInfo { + name: "fk_orders_user_id".to_string(), + column: "user_id".to_string(), + ref_schema: Some("dbo".to_string()), + ref_table: "users".to_string(), + ref_column: "id".to_string(), + on_update: None, + on_delete: None, + }); + + let result = build_table_structure_change_sql(TableStructureSqlOptions { + database_type: Some(DatabaseType::SqlServer), + schema: Some("dbo".to_string()), + table_name: "orders".to_string(), + columns: vec![email], + indexes: Vec::new(), + foreign_keys: vec![user_fk], + triggers: Vec::new(), + table_comment: None, + original_table_comment: None, + }); + + assert_eq!(result.warnings, Vec::::new()); + assert_eq!(result.statements, vec!["ALTER TABLE [dbo].[orders] ADD [email] nvarchar(255) NOT NULL;"]); +} + +#[test] +fn sqlserver_changed_foreign_key_still_warns_as_unsupported() { + let mut user_fk = foreign_key("fk_orders_user_id", "user_id", "accounts", "id"); + user_fk.ref_schema = "dbo".to_string(); + user_fk.original = Some(ForeignKeyInfo { + name: "fk_orders_user_id".to_string(), + column: "user_id".to_string(), + ref_schema: Some("dbo".to_string()), + ref_table: "users".to_string(), + ref_column: "id".to_string(), + on_update: None, + on_delete: None, + }); + + let result = build_table_structure_change_sql(TableStructureSqlOptions { + database_type: Some(DatabaseType::SqlServer), + schema: Some("dbo".to_string()), + table_name: "orders".to_string(), + columns: Vec::new(), + indexes: Vec::new(), + foreign_keys: vec![user_fk], + triggers: Vec::new(), + table_comment: None, + original_table_comment: None, + }); + + assert_eq!(result.statements, Vec::::new()); + assert_eq!(result.warnings, vec!["Editing foreign keys is not supported for sqlserver from this editor."]); +} + +#[test] +fn sqlserver_unchanged_identity_extra_does_not_mark_existing_column_changed() { + let mut id = column("id"); + id.data_type = "int".to_string(); + id.is_nullable = false; + id.is_primary_key = true; + id.extra = Some(ColumnExtra { + auto_increment: Some(true), + identity: Some(ColumnIdentity { generation: None, seed: Some(1), increment: Some(1) }), + ..Default::default() + }); + id.original = Some(ColumnInfo { + name: "id".to_string(), + data_type: "int".to_string(), + is_nullable: false, + column_default: None, + is_primary_key: true, + extra: Some("IDENTITY(1,1)".to_string()), + comment: None, + }); + + let result = build_table_structure_change_sql(TableStructureSqlOptions { + database_type: Some(DatabaseType::SqlServer), + schema: Some("dbo".to_string()), + table_name: "orders".to_string(), + columns: vec![id], + indexes: Vec::new(), + foreign_keys: Vec::new(), + triggers: Vec::new(), + table_comment: None, + original_table_comment: None, + }); + + assert_eq!(result.warnings, Vec::::new()); + assert_eq!(result.statements, Vec::::new()); +} + +#[test] +fn sqlserver_existing_column_identity_change_warns_without_unchanged_foreign_key_warning() { + let mut id = column("id"); + id.data_type = "int".to_string(); + id.is_nullable = false; + id.is_primary_key = true; + id.extra = Some(ColumnExtra { + auto_increment: Some(true), + identity: Some(ColumnIdentity { generation: None, seed: Some(1), increment: Some(1) }), + ..Default::default() + }); + id.original = Some(ColumnInfo { + name: "id".to_string(), + data_type: "int".to_string(), + is_nullable: false, + column_default: None, + is_primary_key: true, + extra: None, + comment: None, + }); + + let mut user_fk = foreign_key("fk_orders_user_id", "user_id", "users", "id"); + user_fk.ref_schema = "dbo".to_string(); + user_fk.original = Some(ForeignKeyInfo { + name: "fk_orders_user_id".to_string(), + column: "user_id".to_string(), + ref_schema: Some("dbo".to_string()), + ref_table: "users".to_string(), + ref_column: "id".to_string(), + on_update: None, + on_delete: None, + }); + + let result = build_table_structure_change_sql(TableStructureSqlOptions { + database_type: Some(DatabaseType::SqlServer), + schema: Some("dbo".to_string()), + table_name: "orders".to_string(), + columns: vec![id], + indexes: Vec::new(), + foreign_keys: vec![user_fk], + triggers: Vec::new(), + table_comment: None, + original_table_comment: None, + }); + + assert_eq!(result.statements, Vec::::new()); + assert_eq!( + result.warnings, + vec!["Changing SQL Server IDENTITY for existing column \"id\" is not supported from this editor."] + ); +} + #[cfg(feature = "duckdb-bundled")] #[test] fn builds_duckdb_create_table_statements() {