diff --git a/core/translate/alter.rs b/core/translate/alter.rs index 41000e768..2a6c56c70 100644 --- a/core/translate/alter.rs +++ b/core/translate/alter.rs @@ -620,6 +620,68 @@ pub fn translate_alter_table( let target_table_name_norm = normalize_ident(table_name); for trigger in resolver.schema.triggers.values().flatten() { let trigger_table_name_norm = normalize_ident(&trigger.table_name); + + // SQLite fails RENAME COLUMN if a trigger's WHEN clause references the column + // Check for WHEN clause references first + if trigger_table_name_norm == target_table_name_norm { + if let Some(ref when_expr) = trigger.when_clause { + let column_name_norm = normalize_ident(from); + let trigger_table = resolver + .schema + .get_btree_table(&trigger_table_name_norm) + .ok_or_else(|| { + LimboError::ParseError(format!( + "trigger table not found: {trigger_table_name_norm}" + )) + })?; + + let mut found_in_when = false; + use crate::translate::expr::walk_expr; + walk_expr(when_expr, &mut |e: &ast::Expr| -> Result< + crate::translate::expr::WalkControl, + > { + match e { + ast::Expr::Qualified(ns, col) + | ast::Expr::DoublyQualified(_, ns, col) => { + let ns_norm = normalize_ident(ns.as_str()); + let col_norm = normalize_ident(col.as_str()); + + // Check NEW.column or OLD.column + if (ns_norm.eq_ignore_ascii_case("new") + || ns_norm.eq_ignore_ascii_case("old")) + && col_norm == column_name_norm + { + found_in_when = true; + return Ok( + crate::translate::expr::WalkControl::SkipChildren, + ); + } + } + ast::Expr::Id(col) => { + // Unqualified column reference - check if it matches + let col_norm = normalize_ident(col.as_str()); + if col_norm == column_name_norm { + // Verify this column exists in the trigger's owning table + if trigger_table.get_column(&col_norm).is_some() { + found_in_when = true; + return Ok(crate::translate::expr::WalkControl::SkipChildren); + } + } + } + _ => {} + } + Ok(crate::translate::expr::WalkControl::Continue) + })?; + + if found_in_when { + return Err(LimboError::ParseError(format!( + "error in trigger {}: no such column: {}", + trigger.name, from + ))); + } + } + } + let mut needs_rewrite = false; // Check if trigger references the column being renamed @@ -948,38 +1010,38 @@ fn trigger_references_column( } // Check all trigger commands + // Note: We only check NEW.x, OLD.x, and unqualified x references in expressions. + // INSERT column lists and UPDATE SET column lists are NOT checked here because + // SQLite allows DROP COLUMN even when triggers reference columns in INSERT/UPDATE + // column lists targeting the owning table. The error only occurs when the trigger + // is actually executed. for cmd in &trigger.commands { match cmd { ast::TriggerCmd::Update { sets, where_clause, .. } => { - // Check SET assignments + // Check SET expressions (not column names in SET clause) for set in sets { walk_expr(&set.expr, &mut |e: &ast::Expr| -> Result { check_column_ref(e, table, &column_name_norm, &mut found)?; Ok(WalkControl::Continue) })?; - } - // Check WHERE clause - if let Some(ref where_expr) = where_clause { - walk_expr(where_expr, &mut |e: &ast::Expr| -> Result { - check_column_ref(e, table, &column_name_norm, &mut found)?; - Ok(WalkControl::Continue) - })?; - } - } - ast::TriggerCmd::Insert { - select, col_names, .. - } => { - // Check column names in INSERT - for col_name in col_names { - let col_norm = normalize_ident(col_name.as_str()); - if col_norm == column_name_norm { - found = true; + if found { break; } } - // Check SELECT/VALUES expressions + // Check WHERE clause + if !found { + if let Some(ref where_expr) = where_clause { + walk_expr(where_expr, &mut |e: &ast::Expr| -> Result { + check_column_ref(e, table, &column_name_norm, &mut found)?; + Ok(WalkControl::Continue) + })?; + } + } + } + ast::TriggerCmd::Insert { select, .. } => { + // Check SELECT/VALUES expressions (not column names in INSERT clause) walk_select_expressions(select, table, &column_name_norm, &mut found)?; } ast::TriggerCmd::Delete { where_clause, .. } => { @@ -1434,22 +1496,10 @@ fn rewrite_trigger_sql_for_column_rename( event }; - // Clone and rewrite expressions - let new_when_clause = when_clause - .as_ref() - .map(|e| { - rewrite_expr_for_column_rename( - e, - &trigger_table, - &trigger_table_name, - &target_table_name, - &old_col_norm, - &new_col_norm, - None, // WHEN clause: unqualified refs refer to trigger's owning table - resolver, - ) - }) - .transpose()?; + // Note: SQLite fails RENAME COLUMN if a trigger's WHEN clause references the column. + // We check for this earlier and fail the operation immediately, matching SQLite. + // If we reach here, the WHEN clause doesn't reference the column, so we keep it unchanged. + let new_when_clause = when_clause.clone(); let mut new_commands = Vec::new(); for cmd in commands { @@ -1476,7 +1526,7 @@ fn rewrite_trigger_sql_for_column_rename( &new_event, &tbl_name, for_each_row, - new_when_clause.as_ref(), + new_when_clause.as_deref(), &new_commands, ); diff --git a/tests/integration/trigger.rs b/tests/integration/trigger.rs index e51787fc7..5cd3a5b40 100644 --- a/tests/integration/trigger.rs +++ b/tests/integration/trigger.rs @@ -1229,7 +1229,7 @@ fn test_alter_table_rename_column_propagates_to_trigger_with_multiple_references } #[test] -fn test_alter_table_rename_column_propagates_to_trigger_when_clause() { +fn test_alter_table_rename_column_fails_when_trigger_when_clause_references_column() { let db = TempDatabase::new_empty(); let conn = db.connect_limbo(); @@ -1245,34 +1245,17 @@ fn test_alter_table_rename_column_propagates_to_trigger_when_clause() { ) .unwrap(); - // Rename column y to y_new - conn.execute("ALTER TABLE t RENAME COLUMN y TO y_new") - .unwrap(); + // Rename column y to y_new should fail (SQLite fails if WHEN clause references the column) + let result = conn.execute("ALTER TABLE t RENAME COLUMN y TO y_new"); + assert!( + result.is_err(), + "RENAME COLUMN should fail when trigger WHEN clause references the column" + ); - // Verify trigger SQL was updated - let mut stmt = conn - .prepare("SELECT sql FROM sqlite_schema WHERE type='trigger' AND name='tu'") - .unwrap(); - let mut trigger_sql = String::new(); - loop { - match stmt.step().unwrap() { - turso_core::StepResult::Row => { - let row = stmt.row().unwrap(); - trigger_sql = row.get_value(0).cast_text().unwrap().to_string(); - } - turso_core::StepResult::Done => break, - turso_core::StepResult::IO => { - stmt.run_once().unwrap(); - } - _ => panic!("Unexpected step result"), - } - } - - // Trigger SQL WHEN clause should reference y_new - let normalized_sql = trigger_sql.split_whitespace().collect::>().join(" "); - assert_eq!( - normalized_sql, - "CREATE TRIGGER tu BEFORE INSERT ON t WHEN y_new > 10 BEGIN INSERT INTO t VALUES (NEW.x, 100); END" + let error_msg = result.unwrap_err().to_string(); + assert!( + error_msg.contains("error in trigger") && error_msg.contains("no such column"), + "Error should mention trigger and column: {error_msg}", ); } @@ -1830,3 +1813,107 @@ fn test_alter_table_rename_column_update_of_multiple_columns_rewritten() { "Trigger SQL should NOT contain UPDATE OF x (x was renamed to x_new): {normalized_sql}", ); } + +#[test] +fn test_alter_table_drop_column_allows_when_insert_targets_other_table() { + let db = TempDatabase::new_empty(); + let conn = db.connect_limbo(); + + // Create two tables, both with column 'x' + conn.execute("CREATE TABLE t (x INTEGER, y INTEGER)") + .unwrap(); + conn.execute("CREATE TABLE u (x INTEGER, z INTEGER)") + .unwrap(); + + // Create trigger on t that inserts into u(x) - this should NOT prevent dropping x from t + conn.execute( + "CREATE TRIGGER tu BEFORE INSERT ON t BEGIN + INSERT INTO u(x) VALUES (NEW.y); + END", + ) + .unwrap(); + + // Dropping column x from table t should succeed (INSERT INTO u(x) refers to u.x, not t.x) + conn.execute("ALTER TABLE t DROP COLUMN x").unwrap(); +} + +#[test] +fn test_alter_table_drop_column_allows_when_update_targets_other_table() { + let db = TempDatabase::new_empty(); + let conn = db.connect_limbo(); + + // Create two tables, both with column 'x' + conn.execute("CREATE TABLE t (x INTEGER, y INTEGER)") + .unwrap(); + conn.execute("CREATE TABLE u (x INTEGER, z INTEGER)") + .unwrap(); + + // Create trigger on t that updates u SET x = ... - this should NOT prevent dropping x from t + conn.execute( + "CREATE TRIGGER tu BEFORE INSERT ON t BEGIN + UPDATE u SET x = NEW.y WHERE z = 1; + END", + ) + .unwrap(); + + // Dropping column x from table t should succeed (UPDATE u SET x refers to u.x, not t.x) + conn.execute("ALTER TABLE t DROP COLUMN x").unwrap(); +} + +#[test] +fn test_alter_table_drop_column_allows_when_insert_targets_owning_table() { + let db = TempDatabase::new_empty(); + let conn = db.connect_limbo(); + + // Create table + conn.execute("CREATE TABLE t (x INTEGER, y INTEGER)") + .unwrap(); + + // Create trigger on t that inserts into t(x) - SQLite allows DROP COLUMN here + // The error only occurs when the trigger is actually executed + conn.execute( + "CREATE TRIGGER tu BEFORE INSERT ON t BEGIN + INSERT INTO t(x) VALUES (NEW.y); + END", + ) + .unwrap(); + + // Dropping column x from table t should succeed (SQLite allows this) + conn.execute("ALTER TABLE t DROP COLUMN x").unwrap(); + + // Verify that executing the trigger now causes an error + let result = conn.execute("INSERT INTO t VALUES (5)"); + assert!( + result.is_err(), + "INSERT should fail because trigger references dropped column" + ); +} + +#[test] +fn test_alter_table_drop_column_allows_when_update_set_targets_owning_table() { + let db = TempDatabase::new_empty(); + let conn = db.connect_limbo(); + + // Create table + conn.execute("CREATE TABLE t (x INTEGER, y INTEGER)") + .unwrap(); + + // Create trigger on t that updates t SET x = ... - SQLite allows DROP COLUMN here + // The error only occurs when the trigger is actually executed + conn.execute( + "CREATE TRIGGER tu BEFORE INSERT ON t BEGIN + UPDATE t SET x = NEW.y WHERE y = 1; + END", + ) + .unwrap(); + + // Dropping column x from table t should succeed (SQLite allows this) + conn.execute("ALTER TABLE t DROP COLUMN x").unwrap(); + + // Verify that executing the trigger now causes an error + let result = conn.execute("INSERT INTO t VALUES (5)"); + assert!( + result.is_err(), + "INSERT should fail because trigger references dropped column" + ); +}