diff --git a/apps/desktop/src/lib/schema/schemaDiff.ts b/apps/desktop/src/lib/schema/schemaDiff.ts index 50db448700..6a5468746f 100644 --- a/apps/desktop/src/lib/schema/schemaDiff.ts +++ b/apps/desktop/src/lib/schema/schemaDiff.ts @@ -112,6 +112,7 @@ export interface ColumnDiff { source?: ColumnInfo; target?: ColumnInfo; changes?: string[]; + addPosition?: "first" | { after: string }; } export interface IndexDiff { diff --git a/crates/dbx-core/src/schema_diff.rs b/crates/dbx-core/src/schema_diff.rs index a91ba85625..89f57ed73f 100644 --- a/crates/dbx-core/src/schema_diff.rs +++ b/crates/dbx-core/src/schema_diff.rs @@ -17,6 +17,13 @@ use crate::types::{ ColumnInfo, ForeignKeyInfo, FunctionInfo, IndexInfo, OwnerInfo, RuleInfo, SequenceInfo, TableInfo, TriggerInfo, }; +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub enum ColumnAddPosition { + First, + After(String), +} + #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct ColumnDiff { @@ -29,6 +36,8 @@ pub struct ColumnDiff { pub target: Option, #[serde(default, skip_serializing_if = "Vec::is_empty")] pub changes: Vec, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub add_position: Option, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -1170,6 +1179,7 @@ impl RollbackGraph { source: c.target.clone(), target: c.source.clone(), changes: c.changes.iter().map(|ch| Self::invert_change_string(ch)).collect(), + add_position: c.add_position.clone(), } }) .collect() @@ -1944,12 +1954,14 @@ fn diff_schema(options: &SchemaDiffPreparationOptions) -> Vec { detail .columns .iter() - .map(|c| ColumnDiff { + .enumerate() + .map(|(index, c)| ColumnDiff { diff_type: "added".to_string(), name: c.name.clone(), source: Some(c.clone()), target: None, changes: vec![], + add_position: Some(column_add_position(&detail.columns, index)), }) .collect() }), @@ -2015,12 +2027,14 @@ fn diff_schema(options: &SchemaDiffPreparationOptions) -> Vec { detail .columns .iter() - .map(|column| ColumnDiff { + .enumerate() + .map(|(index, column)| ColumnDiff { diff_type: "removed".to_string(), name: column.name.clone(), source: None, target: Some(column.clone()), changes: vec![], + add_position: Some(column_add_position(&detail.columns, index)), }) .collect() }), @@ -2262,6 +2276,7 @@ fn diff_columns_with_options( source: Some(source_column.clone()), target: Some((*target_column).clone()), changes, + add_position: None, }); } } else { @@ -2271,11 +2286,12 @@ fn diff_columns_with_options( source: Some(source_column.clone()), target: None, changes: Vec::new(), + add_position: Some(column_add_position(source, source_index)), }); } } - for target_column in target { + for (target_index, target_column) in target.iter().enumerate() { if !source_map.contains_key(target_column.name.as_str()) { diffs.push(ColumnDiff { diff_type: "removed".to_string(), @@ -2283,6 +2299,7 @@ fn diff_columns_with_options( source: None, target: Some(target_column.clone()), changes: Vec::new(), + add_position: Some(column_add_position(target, target_index)), }); } } @@ -2342,6 +2359,7 @@ fn diff_columns_with_options( source: Some(new_col), target: Some(old_col), changes: vec![format!("{} → {}", old_name, new_name)], + add_position: None, }; diffs[*ai] = ColumnDiff { diff_type: "_matched_rename".to_string(), @@ -2349,6 +2367,7 @@ fn diff_columns_with_options( source: None, target: None, changes: Vec::new(), + add_position: None, }; } @@ -2358,6 +2377,14 @@ fn diff_columns_with_options( diffs } +fn column_add_position(columns: &[ColumnInfo], index: usize) -> ColumnAddPosition { + if index == 0 { + ColumnAddPosition::First + } else { + ColumnAddPosition::After(columns[index - 1].name.clone()) + } +} + pub fn diff_indexes(source: &[IndexInfo], target: &[IndexInfo]) -> Vec { let mut diffs = Vec::new(); let target_map: HashMap<&str, &IndexInfo> = target.iter().map(|index| (index.name.as_str(), index)).collect(); @@ -3536,7 +3563,22 @@ fn generate_schema_sync_sql_inner( match column.diff_type.as_str() { "added" => { if let Some(source) = &column.source { - parts.push(format!(" ADD COLUMN {}", column_def(&convert_col(source), db_type))); + let position = if db_type == DatabaseType::Mysql { + match &column.add_position { + Some(ColumnAddPosition::First) => " FIRST".to_string(), + Some(ColumnAddPosition::After(predecessor)) => { + format!(" AFTER {}", quote_id(predecessor, db_type)) + } + None => String::new(), + } + } else { + String::new() + }; + parts.push(format!( + " ADD COLUMN {}{}", + column_def(&convert_col(source), db_type), + position + )); } } "removed" => { @@ -4053,6 +4095,74 @@ mod tests { assert!(!sql.contains("DROP COLUMN"), "MySQL no drop: {sql}"); } + #[test] + fn mysql_add_columns_preserve_source_positions() { + let diffs = make_col_diffs( + &[("first", "int"), ("a", "int"), ("middle", "varchar(32)"), ("next", "int"), ("last", "int")], + &[("a", "int"), ("last", "int")], + false, + ); + + let sql = gen_sql(wrap_table_diff("t", diffs), DatabaseType::Mysql, None); + + assert!(sql.contains("ADD COLUMN `first` int NOT NULL FIRST"), "first position: {sql}"); + assert!(sql.contains("ADD COLUMN `middle` varchar(32) NOT NULL AFTER `a`"), "middle position: {sql}"); + assert!(sql.contains("ADD COLUMN `next` int NOT NULL AFTER `middle`"), "consecutive additions: {sql}"); + } + + #[test] + fn mysql_add_column_quotes_predecessor_and_keeps_trailing_position() { + let diffs = make_col_diffs( + &[("odd`name", "int"), ("middle", "int"), ("tail", "int"), ("new_tail", "int")], + &[("odd`name", "int"), ("tail", "int")], + false, + ); + + let sql = gen_sql(wrap_table_diff("t", diffs), DatabaseType::Mysql, None); + + assert!(sql.contains("ADD COLUMN `middle` int NOT NULL AFTER `odd``name`"), "quoted predecessor: {sql}"); + assert!(sql.contains("ADD COLUMN `new_tail` int NOT NULL AFTER `tail`"), "trailing position: {sql}"); + } + + #[test] + fn non_mysql_add_columns_do_not_emit_mysql_position_clauses() { + let diffs = make_col_diffs(&[("first", "int"), ("a", "int"), ("middle", "int")], &[("a", "int")], false); + + let sql = gen_sql(wrap_table_diff("t", diffs), DatabaseType::Postgres, None); + + assert!(!sql.contains(" FIRST"), "PostgreSQL must not use FIRST: {sql}"); + assert!(!sql.contains(" AFTER "), "PostgreSQL must not use AFTER: {sql}"); + } + + #[test] + fn mysql_add_after_renamed_predecessor_uses_final_source_name() { + let diffs = + make_col_diffs(&[("new_name", "varchar(32)"), ("inserted", "int")], &[("old_name", "varchar(32)")], true); + + let sql = gen_sql(wrap_table_diff("t", diffs), DatabaseType::Mysql, None); + + assert!(sql.contains("ADD COLUMN `inserted` int NOT NULL AFTER `new_name`"), "rename predecessor: {sql}"); + assert!(sql.contains("CHANGE COLUMN `old_name` `new_name`"), "rename remains present: {sql}"); + } + + #[test] + fn mysql_manual_added_diff_without_position_keeps_legacy_sql() { + let diff = ColumnDiff { + diff_type: "added".into(), + name: "legacy".into(), + source: Some(column("legacy", "int", None)), + target: None, + changes: Vec::new(), + add_position: None, + }; + + let sql = gen_sql(wrap_table_diff("t", vec![diff]), DatabaseType::Mysql, None); + + assert!(sql.contains("ADD COLUMN `legacy` int NOT NULL"), "legacy add: {sql}"); + assert!(!sql.contains(" FIRST"), "legacy diff has no position: {sql}"); + assert!(!sql.contains(" AFTER "), "legacy diff has no position: {sql}"); + } + #[test] fn mysql_same_dialect_add_drop_modified() { let diffs = make_col_diffs( @@ -4868,6 +4978,7 @@ mod tests { source: Some(column("name", "varchar(64)", Some("用户姓名"))), target: Some(column("name", "varchar(64)", Some("Name"))), changes: vec!["comment: Name → 用户姓名".to_string()], + add_position: None, }]), indexes: None, foreign_keys: None, @@ -4904,6 +5015,7 @@ mod tests { source: Some(column("config_json", "json", Some("渠道配置"))), target: Some(column("config_json", "json", Some("Config"))), changes: vec!["comment: Config → 渠道配置".to_string()], + add_position: None, }]), indexes: None, foreign_keys: None, @@ -4949,6 +5061,7 @@ mod tests { source: Some(column("config_json", "json", Some("渠道配置"))), target: Some(column("config_json", "json", Some("Config"))), changes: vec!["comment: Config → 渠道配置".to_string()], + add_position: None, }]), indexes: None, foreign_keys: None, @@ -5104,6 +5217,7 @@ mod tests { }), target: None, changes: Vec::new(), + add_position: None, }]), indexes: Some(vec![IndexDiff { diff_type: "added".to_string(), @@ -5969,6 +6083,7 @@ mod tests { source: Some(source_col.clone()), target: Some(target_col.clone()), changes: vec!["type: varchar(50) → varchar(100)".to_string()], + add_position: None, }]), indexes: None, foreign_keys: None, @@ -6618,6 +6733,7 @@ mod tests { source: Some(column("new_name", "varchar(50)", None)), target: Some(column("old_name", "varchar(50)", None)), changes: vec!["old_name → new_name".to_string()], + add_position: None, }]), indexes: None, foreign_keys: None, @@ -6649,6 +6765,7 @@ mod tests { source: Some(column("id", "int(11)", None)), target: None, changes: vec![], + add_position: None, }]), indexes: None, foreign_keys: None, @@ -7659,6 +7776,7 @@ mod tests { }), target: None, changes: vec![], + add_position: None, }, ColumnDiff { diff_type: "added".into(), @@ -7671,6 +7789,7 @@ mod tests { }), target: None, changes: vec![], + add_position: None, }, ColumnDiff { diff_type: "added".into(), @@ -7683,6 +7802,7 @@ mod tests { }), target: None, changes: vec![], + add_position: None, }, ColumnDiff { diff_type: "added".into(), @@ -7695,6 +7815,7 @@ mod tests { }), target: None, changes: vec![], + add_position: None, }, ]; let indexes = vec![IndexDiff { @@ -7805,6 +7926,7 @@ mod tests { source: Some(col_pk("id", "int")), target: None, changes: vec![], + add_position: None, }, ColumnDiff { diff_type: "added".into(), @@ -7817,6 +7939,7 @@ mod tests { }), target: None, changes: vec![], + add_position: None, }, ] } @@ -7877,6 +8000,7 @@ mod tests { source: Some(col_pk("id", "int")), target: None, changes: vec![], + add_position: None, }, ColumnDiff { diff_type: "added".into(), @@ -7889,6 +8013,7 @@ mod tests { }), target: None, changes: vec![], + add_position: None, }, ColumnDiff { diff_type: "added".into(), @@ -7901,6 +8026,7 @@ mod tests { }), target: None, changes: vec![], + add_position: None, }, ColumnDiff { diff_type: "added".into(), @@ -7914,6 +8040,7 @@ mod tests { }), target: None, changes: vec![], + add_position: None, }, ColumnDiff { diff_type: "added".into(), @@ -7926,6 +8053,7 @@ mod tests { }), target: None, changes: vec![], + add_position: None, }, ColumnDiff { diff_type: "added".into(), @@ -7938,6 +8066,7 @@ mod tests { }), target: None, changes: vec![], + add_position: None, }, ]; let idxs = vec![IndexDiff { @@ -8008,6 +8137,7 @@ mod tests { source: Some(col_pk("id", "int")), target: None, changes: vec![], + add_position: None, }, ColumnDiff { diff_type: "added".into(), @@ -8020,6 +8150,7 @@ mod tests { }), target: None, changes: vec![], + add_position: None, }, ]; let fks = vec![ForeignKeyDiff { @@ -8142,6 +8273,7 @@ mod tests { source: Some(col_pk("id", "integer")), target: None, changes: vec![], + add_position: None, }, ColumnDiff { diff_type: "added".into(), @@ -8154,6 +8286,7 @@ mod tests { }), target: None, changes: vec![], + add_position: None, }, ]; let td = TableDiff { @@ -8190,6 +8323,7 @@ mod tests { source: Some(col_pk("id", "int")), target: None, changes: vec![], + add_position: None, }, ColumnDiff { diff_type: "added".into(), @@ -8202,6 +8336,7 @@ mod tests { }), target: None, changes: vec![], + add_position: None, }, ]; let td = TableDiff { @@ -8237,6 +8372,7 @@ mod tests { source: Some(col_pk("id", "Int32")), target: None, changes: vec![], + add_position: None, }, ColumnDiff { diff_type: "added".into(), @@ -8249,6 +8385,7 @@ mod tests { }), target: None, changes: vec![], + add_position: None, }, ]; let td = TableDiff { @@ -8282,6 +8419,7 @@ mod tests { source: Some(col_pk("id", "int")), target: None, changes: vec![], + add_position: None, }, ColumnDiff { diff_type: "added".into(), @@ -8294,6 +8432,7 @@ mod tests { }), target: None, changes: vec![], + add_position: None, }, ]; let table_diff = TableDiff { @@ -8548,6 +8687,7 @@ mod tests { source: Some(column("created_at", "timestamp(0) without time zone", None)), target: Some(column("created_at", "timestamp without time zone", None)), changes: vec!["type: timestamp without time zone → timestamp(0) without time zone".into()], + add_position: None, }]), ..TableDiff::default() }; diff --git a/crates/dbx-core/src/script_generator.rs b/crates/dbx-core/src/script_generator.rs index e520c82e2a..4d11f92b7e 100644 --- a/crates/dbx-core/src/script_generator.rs +++ b/crates/dbx-core/src/script_generator.rs @@ -1706,6 +1706,7 @@ mod tests { source: Some(make_column_info("id", "INT")), target: None, changes: vec![], + add_position: None, }]), indexes: Some(vec![]), foreign_keys: Some(vec![]), @@ -1737,6 +1738,7 @@ mod tests { source: Some(make_column_info("id", "INT")), target: None, changes: vec![], + add_position: None, }]), indexes: Some(vec![]), foreign_keys: Some(vec![]), @@ -2262,6 +2264,7 @@ mod tests { collation: None, }), changes: vec!["data_type BIGINT → INT".to_string()], + add_position: None, }; let table_diff = TableDiff { diff --git a/crates/dbx-core/tests/live_mysql_schema_diff_column_order.rs b/crates/dbx-core/tests/live_mysql_schema_diff_column_order.rs new file mode 100644 index 0000000000..6f582d0299 --- /dev/null +++ b/crates/dbx-core/tests/live_mysql_schema_diff_column_order.rs @@ -0,0 +1,87 @@ +use dbx_core::models::connection::DatabaseType; +use dbx_core::schema_diff::{ + generate_schema_sync_sql, prepare_schema_diff, SchemaDiffPreparationOptions, TableSchemaDetail, +}; +use dbx_core::types::{ColumnInfo, TableInfo}; +use mysql_async::prelude::Queryable; + +fn column(name: &str, data_type: &str) -> ColumnInfo { + ColumnInfo { name: name.to_string(), data_type: data_type.to_string(), ..Default::default() } +} + +fn table(name: &str) -> TableInfo { + TableInfo { + name: name.to_string(), + table_type: "BASE TABLE".to_string(), + comment: None, + parent_schema: None, + parent_name: None, + } +} + +#[tokio::test] +#[ignore = "requires DBX_LIVE_MYSQL_URL pointing to MySQL 8.4.2"] +async fn mysql_schema_diff_add_columns_keep_source_order() { + let url = std::env::var("DBX_LIVE_MYSQL_URL").expect("DBX_LIVE_MYSQL_URL"); + let mut conn = mysql_async::Conn::new(mysql_async::Opts::from_url(&url).expect("valid MySQL URL")) + .await + .expect("connect to live MySQL"); + let version: String = conn.query_first("SELECT VERSION()").await.expect("read version").expect("version row"); + assert!(version.starts_with("8.4.2"), "expected MySQL 8.4.2, got {version}"); + + conn.query_drop("DROP DATABASE IF EXISTS dbx4291_live").await.expect("drop prior live database"); + conn.query_drop("CREATE DATABASE dbx4291_live").await.expect("create live database"); + conn.query_drop("CREATE TABLE dbx4291_live.positional (`a` INT NOT NULL, `last` INT NOT NULL)") + .await + .expect("create target table"); + + let source_columns = vec![ + column("first", "int"), + column("a", "int"), + column("middle", "varchar(32)"), + column("next", "int"), + column("last", "int"), + column("new_tail", "int"), + ]; + let target_columns = vec![column("a", "int"), column("last", "int")]; + let prepared = prepare_schema_diff(SchemaDiffPreparationOptions { + source_tables: vec![table("positional")], + target_tables: vec![table("positional")], + source_details: vec![TableSchemaDetail { + name: "positional".to_string(), + columns: source_columns, + indexes: Vec::new(), + foreign_keys: Vec::new(), + triggers: Vec::new(), + ddl: None, + }], + target_details: vec![TableSchemaDetail { + name: "positional".to_string(), + columns: target_columns, + indexes: Vec::new(), + foreign_keys: Vec::new(), + triggers: Vec::new(), + ddl: None, + }], + database_type: DatabaseType::Mysql, + target_schema: Some("dbx4291_live".to_string()), + ..Default::default() + }); + + conn.query_drop(&prepared.sync_sql).await.expect("execute DBX-generated positional ALTER TABLE"); + let names: Vec = conn + .exec_map( + "SELECT COLUMN_NAME FROM INFORMATION_SCHEMA.COLUMNS \ + WHERE TABLE_SCHEMA = ? AND TABLE_NAME = ? ORDER BY ORDINAL_POSITION", + ("dbx4291_live", "positional"), + |name: String| name, + ) + .await + .expect("read resulting column order"); + assert_eq!(names, vec!["first", "a", "middle", "next", "last", "new_tail"]); + + let postgres_sql = + generate_schema_sync_sql(&prepared.diffs, &[], &[], &[], &[], DatabaseType::Postgres, None, false, None, &[]); + assert!(!postgres_sql.contains(" FIRST"), "PostgreSQL output must not contain MySQL FIRST: {postgres_sql}"); + assert!(!postgres_sql.contains(" AFTER "), "PostgreSQL output must not contain MySQL AFTER: {postgres_sql}"); +}