From a0a1bd6637441210d394c2509ebed192c109e86c Mon Sep 17 00:00:00 2001 From: Jussi Saurio Date: Wed, 19 Nov 2025 10:43:39 +0200 Subject: [PATCH 1/7] Triggers: fix issues with ALTER TABLE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Disallow DROP COLUMN on columns referenced in triggers - Propagate RENAME COLUMN to trigger SQL definitions DROP COLUMN is not allowed when the column is mentioned in a trigger on the table the column is dropped from, eg: ``` turso> CREATE TABLE t(x,y); turso> CREATE TRIGGER foo BEFORE INSERT ON t BEGIN INSERT INTO t VALUES (NEW.x); END; turso> ALTER TABLE t DROP COLUMN x; × Parse error: cannot drop column "x": it is referenced in trigger foo ``` However, it is allowed if the trigger is on another table: ``` turso> CREATE TABLE t(x,y); turso> CREATE TABLE u(x,y); turso> CREATE TRIGGER bar BEFORE INSERT ON t BEGIN INSERT INTO u(y) VALUES (NEW.x); END; turso> ALTER TABLE u DROP COLUMN y; turso> INSERT INTO t VALUES (1,1); × Parse error: table u has no column named y ``` Nearly all of the code here is vibecoded. I first asked Cursor Composer to create an initial implementation. Then, I asked it to try to discover edge cases using the `turso` and `sqlite3` CLIs, and write tests+fixes for the edge cases found. The code is a bit slop, but this isn't a particularly performance-critical use case and it should solve most of the issues with RENAME and DROP COLUMN. --- core/translate/alter.rs | 1305 ++++++++++++++++++++++++++++++++++ tests/integration/trigger.rs | 944 ++++++++++++++++++++++++ 2 files changed, 2249 insertions(+) diff --git a/core/translate/alter.rs b/core/translate/alter.rs index 2505bc273..1fcfbfbc3 100644 --- a/core/translate/alter.rs +++ b/core/translate/alter.rs @@ -201,6 +201,16 @@ pub fn translate_alter_table( // References in VIEWs are checked in the VDBE layer op_drop_column instruction. + // Check if any trigger owned by this table references the column being dropped + for trigger in resolver.schema.get_triggers_for_table(table_name) { + if trigger_references_column(trigger, &btree, column_name)? { + return Err(LimboError::ParseError(format!( + "cannot drop column \"{column_name}\": it is referenced in trigger {}", + trigger.name + ))); + } + } + btree.columns.remove(dropped_index); let sql = btree.to_sql().replace('\'', "''"); @@ -579,6 +589,68 @@ pub fn translate_alter_table( )); } + // If renaming, rewrite trigger SQL for all triggers that reference this column + // We'll collect the triggers to rewrite and update them in sqlite_schema + let mut triggers_to_rewrite: Vec<(String, String)> = Vec::new(); + if rename { + // Find all triggers that might reference this column + 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); + let mut needs_rewrite = false; + + // Check if trigger references the column being renamed + // This includes: + // 1. References from the trigger's owning table (NEW.x, OLD.x, unqualified x) + // 2. References in INSERT column lists targeting the table being renamed + // 3. References in UPDATE SET column lists targeting the table being renamed + if trigger_table_name_norm == target_table_name_norm { + // Trigger is on the table being renamed - check for references + if let Some(trigger_table) = + resolver.schema.get_btree_table(&trigger_table_name_norm) + { + if let Ok(refs) = + trigger_references_column(trigger, &trigger_table, from) + { + needs_rewrite = refs; + } + } + } + + // Also check if trigger references the column in INSERT/UPDATE targeting other tables + // Parse the trigger to check INSERT column lists and UPDATE SET column lists + if !needs_rewrite { + needs_rewrite = trigger_references_column_in_other_tables( + trigger, + &target_table_name_norm, + from, + resolver, + )?; + } + + if needs_rewrite { + match rewrite_trigger_sql_for_column_rename( + &trigger.sql, + table_name, + from, + col_name, + resolver, + ) { + Ok(new_sql) => { + triggers_to_rewrite.push((trigger.name.clone(), new_sql)); + } + Err(e) => { + // If we can't rewrite the trigger, fail the ALTER TABLE operation + return Err(LimboError::ParseError(format!( + "error in trigger {} after rename column: {}", + trigger.name, e + ))); + } + } + } + } + } + let sqlite_schema = resolver .schema .get_btree_table(SQLITE_TABLEID) @@ -646,6 +718,35 @@ pub fn translate_alter_table( }); }); + // Update trigger SQL for renamed columns + for (trigger_name, new_sql) in triggers_to_rewrite { + let escaped_sql = new_sql.replace('\'', "''"); + let update_stmt = format!( + r#" + UPDATE {SQLITE_TABLEID} + SET sql = '{escaped_sql}' + WHERE name = '{trigger_name}' COLLATE NOCASE AND type = 'trigger' + "#, + ); + + let mut parser = Parser::new(update_stmt.as_bytes()); + let Some(ast::Cmd::Stmt(ast::Stmt::Update(update))) = parser.next_cmd().unwrap() + else { + return Err(LimboError::ParseError(format!( + "failed to parse trigger update SQL for {trigger_name}", + ))); + }; + + program = translate_update_for_schema_change( + update, + resolver, + program, + connection, + input, + |_program| {}, + )?; + } + program.emit_insn(Insn::SetCookie { db: 0, cookie: Cookie::SchemaVersion, @@ -762,3 +863,1207 @@ fn translate_rename_virtual_table( Ok(program) } + +/* Triggers must be rewritten when a column is renamed, and DROP COLUMN on table T must be forbidden if any trigger on T references the column. +Here are some helpers related to that: */ + +/// Check if a trigger references a specific column from its owning table. +/// Returns true if the column is referenced as old.x, new.x, or unqualified x. +fn trigger_references_column( + trigger: &crate::schema::Trigger, + table: &crate::schema::BTreeTable, + column_name: &str, +) -> Result { + let column_name_norm = normalize_ident(column_name); + let mut found = false; + + // Check when_clause + if let Some(ref when_expr) = trigger.when_clause { + walk_expr(when_expr, &mut |e: &ast::Expr| -> Result { + 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 = true; + return Ok(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 table.get_column(&col_norm).is_some() { + found = true; + return Ok(WalkControl::SkipChildren); + } + } + } + _ => {} + } + Ok(WalkControl::Continue) + })?; + } + + if found { + return Ok(true); + } + + // Check all trigger commands + for cmd in &trigger.commands { + match cmd { + ast::TriggerCmd::Update { + sets, where_clause, .. + } => { + // Check SET assignments + 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; + break; + } + } + // Check SELECT/VALUES expressions + walk_select_expressions(select, table, &column_name_norm, &mut found)?; + } + ast::TriggerCmd::Delete { 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::Select(select) => { + walk_select_expressions(select, table, &column_name_norm, &mut found)?; + } + } + if found { + break; + } + } + + Ok(found) +} + +/// Helper to check if an expression references a column +fn check_column_ref( + e: &ast::Expr, + table: &crate::schema::BTreeTable, + column_name_norm: &str, + found: &mut bool, +) -> Result<()> { + 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 = true; + } + } + 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 table.get_column(&col_norm).is_some() { + *found = true; + } + } + } + _ => {} + } + Ok(()) +} + +/// Check if a trigger references a column from a table other than its owning table. +/// This checks INSERT column lists and UPDATE SET column lists that target the table being renamed. +fn trigger_references_column_in_other_tables( + trigger: &crate::schema::Trigger, + target_table_name: &str, + column_name: &str, + _resolver: &Resolver, +) -> Result { + let column_name_norm = normalize_ident(column_name); + let target_table_name_norm = normalize_ident(target_table_name); + + // Check all trigger commands for INSERT/UPDATE targeting the table being renamed + for cmd in &trigger.commands { + match cmd { + ast::TriggerCmd::Insert { + tbl_name, + col_names, + .. + } => { + // Check if INSERT targets the table being renamed + let insert_table_name_norm = normalize_ident(tbl_name.as_str()); + if insert_table_name_norm == target_table_name_norm { + // Check if column name appears in INSERT column list + for col_name in col_names { + let col_norm = normalize_ident(col_name.as_str()); + if col_norm == column_name_norm { + return Ok(true); + } + } + } + } + ast::TriggerCmd::Update { tbl_name, sets, .. } => { + // Check if UPDATE targets the table being renamed + let update_table_name_norm = normalize_ident(tbl_name.as_str()); + if update_table_name_norm == target_table_name_norm { + // Check if column name appears in UPDATE SET column list + for set in sets { + for col_name in &set.col_names { + let col_norm = normalize_ident(col_name.as_str()); + if col_norm == column_name_norm { + return Ok(true); + } + } + } + } + } + _ => {} + } + } + + Ok(false) +} + +/// Walk through all expressions in a SELECT statement +fn walk_select_expressions( + select: &ast::Select, + table: &crate::schema::BTreeTable, + column_name_norm: &str, + found: &mut bool, +) -> Result<()> { + // Check WITH clause (CTEs) + if let Some(ref with_clause) = select.with { + for cte in &with_clause.ctes { + walk_select_expressions(&cte.select, table, column_name_norm, found)?; + if *found { + return Ok(()); + } + } + } + + // Check main SELECT body + walk_one_select_expressions(&select.body.select, table, column_name_norm, found)?; + if *found { + return Ok(()); + } + + // Check compound SELECTs (UNION, EXCEPT, INTERSECT) + for compound in &select.body.compounds { + walk_one_select_expressions(&compound.select, table, column_name_norm, found)?; + if *found { + return Ok(()); + } + } + + // Check ORDER BY + for sorted_col in &select.order_by { + walk_expr( + &sorted_col.expr, + &mut |e: &ast::Expr| -> Result { + check_column_ref(e, table, column_name_norm, found)?; + Ok(WalkControl::Continue) + }, + )?; + if *found { + return Ok(()); + } + } + + // Check LIMIT + if let Some(ref limit) = select.limit { + walk_expr(&limit.expr, &mut |e: &ast::Expr| -> Result { + check_column_ref(e, table, column_name_norm, found)?; + Ok(WalkControl::Continue) + })?; + if *found { + return Ok(()); + } + if let Some(ref offset) = limit.offset { + walk_expr(offset, &mut |e: &ast::Expr| -> Result { + check_column_ref(e, table, column_name_norm, found)?; + Ok(WalkControl::Continue) + })?; + if *found { + return Ok(()); + } + } + } + + Ok(()) +} + +/// Walk through all expressions in a OneSelect +fn walk_one_select_expressions( + one_select: &ast::OneSelect, + table: &crate::schema::BTreeTable, + column_name_norm: &str, + found: &mut bool, +) -> Result<()> { + match one_select { + ast::OneSelect::Select { + columns, + from, + where_clause, + group_by, + window_clause, + .. + } => { + // Check columns + for col in columns { + if let ast::ResultColumn::Expr(expr, _) = col { + walk_expr(expr, &mut |e: &ast::Expr| -> Result { + check_column_ref(e, table, column_name_norm, found)?; + Ok(WalkControl::Continue) + })?; + if *found { + return Ok(()); + } + } + } + + // Check FROM clause and JOIN conditions + if let Some(ref from_clause) = from { + walk_from_clause_expressions(from_clause, table, column_name_norm, found)?; + if *found { + return Ok(()); + } + } + + // 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, found)?; + Ok(WalkControl::Continue) + })?; + if *found { + return Ok(()); + } + } + + // Check GROUP BY and HAVING + if let Some(ref group_by) = group_by { + for expr in &group_by.exprs { + walk_expr(expr, &mut |e: &ast::Expr| -> Result { + check_column_ref(e, table, column_name_norm, found)?; + Ok(WalkControl::Continue) + })?; + if *found { + return Ok(()); + } + } + if let Some(ref having_expr) = group_by.having { + walk_expr(having_expr, &mut |e: &ast::Expr| -> Result { + check_column_ref(e, table, column_name_norm, found)?; + Ok(WalkControl::Continue) + })?; + if *found { + return Ok(()); + } + } + } + + // Check WINDOW clause + for window_def in window_clause { + walk_window_expressions(&window_def.window, table, column_name_norm, found)?; + if *found { + return Ok(()); + } + } + } + ast::OneSelect::Values(values) => { + for row in values { + for expr in row { + walk_expr(expr, &mut |e: &ast::Expr| -> Result { + check_column_ref(e, table, column_name_norm, found)?; + Ok(WalkControl::Continue) + })?; + if *found { + return Ok(()); + } + } + } + } + } + Ok(()) +} + +/// Walk through expressions in a FROM clause (including JOIN conditions) +fn walk_from_clause_expressions( + from_clause: &ast::FromClause, + table: &crate::schema::BTreeTable, + column_name_norm: &str, + found: &mut bool, +) -> Result<()> { + // Check main table (could be a subquery) + walk_select_table_expressions(&from_clause.select, table, column_name_norm, found)?; + if *found { + return Ok(()); + } + + // Check JOIN conditions + for join in &from_clause.joins { + walk_select_table_expressions(&join.table, table, column_name_norm, found)?; + if *found { + return Ok(()); + } + if let Some(ref constraint) = join.constraint { + match constraint { + ast::JoinConstraint::On(expr) => { + walk_expr(expr, &mut |e: &ast::Expr| -> Result { + check_column_ref(e, table, column_name_norm, found)?; + Ok(WalkControl::Continue) + })?; + if *found { + return Ok(()); + } + } + ast::JoinConstraint::Using(_) => { + // USING clause contains column names, not expressions + } + } + } + } + Ok(()) +} + +/// Walk through expressions in a SelectTable (table, subquery, or table function) +fn walk_select_table_expressions( + select_table: &ast::SelectTable, + table: &crate::schema::BTreeTable, + column_name_norm: &str, + found: &mut bool, +) -> Result<()> { + match select_table { + ast::SelectTable::Select(select, _) => { + walk_select_expressions(select, table, column_name_norm, found)?; + } + ast::SelectTable::Sub(from_clause, _) => { + walk_from_clause_expressions(from_clause, table, column_name_norm, found)?; + } + ast::SelectTable::TableCall(_, args, _) => { + for arg in args { + walk_expr(arg, &mut |e: &ast::Expr| -> Result { + check_column_ref(e, table, column_name_norm, found)?; + Ok(WalkControl::Continue) + })?; + if *found { + return Ok(()); + } + } + } + ast::SelectTable::Table(_, _, _) => { + // Table reference, no expressions + } + } + Ok(()) +} + +/// Walk through expressions in a Window definition +fn walk_window_expressions( + window: &ast::Window, + table: &crate::schema::BTreeTable, + column_name_norm: &str, + found: &mut bool, +) -> Result<()> { + // Check PARTITION BY expressions + for expr in &window.partition_by { + walk_expr(expr, &mut |e: &ast::Expr| -> Result { + check_column_ref(e, table, column_name_norm, found)?; + Ok(WalkControl::Continue) + })?; + if *found { + return Ok(()); + } + } + + // Check ORDER BY expressions + for sorted_col in &window.order_by { + walk_expr( + &sorted_col.expr, + &mut |e: &ast::Expr| -> Result { + check_column_ref(e, table, column_name_norm, found)?; + Ok(WalkControl::Continue) + }, + )?; + if *found { + return Ok(()); + } + } + + // TODO: FrameClause can also contain expressions, but they're more complex + // For now, we'll skip them as they're less common in triggers + Ok(()) +} + +/// Rewrite trigger SQL to replace old column name with new column name. +/// This handles old.x, new.x, and unqualified x references. +fn rewrite_trigger_sql_for_column_rename( + trigger_sql: &str, + table_name: &str, + old_column_name: &str, + new_column_name: &str, + resolver: &Resolver, +) -> Result { + use turso_parser::parser::Parser; + + // Parse the trigger SQL + let mut parser = Parser::new(trigger_sql.as_bytes()); + let Some(ast::Cmd::Stmt(ast::Stmt::CreateTrigger { + temporary, + if_not_exists, + trigger_name, + time, + event, + tbl_name, + for_each_row, + when_clause, + commands, + })) = parser.next_cmd().unwrap() + else { + return Err(LimboError::ParseError(format!( + "failed to parse trigger SQL: {trigger_sql}" + ))); + }; + + let old_col_norm = normalize_ident(old_column_name); + let new_col_norm = normalize_ident(new_column_name); + + // Get the trigger's owning table to check unqualified column references + let trigger_table_name = normalize_ident(tbl_name.name.as_str()); + let trigger_table = resolver + .schema + .get_btree_table(&trigger_table_name) + .ok_or_else(|| { + LimboError::ParseError(format!("trigger table not found: {trigger_table_name}")) + })?; + + // Check if this trigger references the column being renamed + // We need to check if the column exists in the table being renamed + let target_table_name = normalize_ident(table_name); + let target_table = resolver + .schema + .get_btree_table(&target_table_name) + .ok_or_else(|| { + LimboError::ParseError(format!("target table not found: {target_table_name}")) + })?; + + // Rewrite UPDATE OF column list if renaming a column in the trigger's owning table + let is_renaming_trigger_table = trigger_table_name == target_table_name; + let new_event = if is_renaming_trigger_table { + match event { + ast::TriggerEvent::UpdateOf(mut cols) => { + // Rewrite column names in UPDATE OF list + for col in &mut cols { + let col_norm = normalize_ident(col.as_str()); + if col_norm == old_col_norm { + *col = ast::Name::from_string(new_col_norm.clone()); + } + } + ast::TriggerEvent::UpdateOf(cols) + } + other => other, + } + } else { + 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, + ) + .unwrap() + }); + + let mut new_commands = Vec::new(); + for cmd in commands { + let new_cmd = rewrite_trigger_cmd_for_column_rename( + cmd, + &trigger_table, + &target_table, + &trigger_table_name, + &target_table_name, + &old_col_norm, + &new_col_norm, + resolver, + )?; + new_commands.push(new_cmd); + } + + // Reconstruct the SQL + use crate::translate::trigger::create_trigger_to_sql; + let new_sql = create_trigger_to_sql( + temporary, + if_not_exists, + &trigger_name, + time, + &new_event, + &tbl_name, + for_each_row, + new_when_clause.as_ref(), + &new_commands, + ); + + Ok(new_sql) +} + +/// Rewrite an expression to replace column references +/// +/// `context_table_name` is used for UPDATE/DELETE WHERE clauses where unqualified column +/// references refer to the UPDATE/DELETE target table, not the trigger's owning table. +/// If `None`, unqualified references refer to the trigger's owning table. +#[allow(clippy::too_many_arguments)] +fn rewrite_expr_for_column_rename( + expr: &ast::Expr, + trigger_table: &crate::schema::BTreeTable, + trigger_table_name: &str, + target_table_name: &str, + old_col_norm: &str, + new_col_norm: &str, + context_table_name: Option<&str>, + resolver: &Resolver, +) -> Result { + use crate::translate::expr::walk_expr_mut; + + let trigger_table_name_norm = normalize_ident(trigger_table_name); + let target_table_name_norm = normalize_ident(target_table_name); + let is_renaming_trigger_table = trigger_table_name_norm == target_table_name_norm; + + // Get context table if provided (for UPDATE/DELETE WHERE clauses) + let context_table_info: Option<(std::sync::Arc, String, bool)> = + if let Some(ctx_name) = context_table_name { + let ctx_name_norm = normalize_ident(ctx_name); + let is_renaming = ctx_name_norm == target_table_name_norm; + let table = resolver + .schema + .get_btree_table(&ctx_name_norm) + .ok_or_else(|| { + LimboError::ParseError(format!("context table not found: {ctx_name_norm}")) + })?; + Some((table, ctx_name_norm, is_renaming)) + } else { + None + }; + + let mut expr = expr.clone(); + walk_expr_mut(&mut expr, &mut |e: &mut ast::Expr| -> Result { + rewrite_expr_column_ref_with_context( + e, + trigger_table, + old_col_norm, + new_col_norm, + is_renaming_trigger_table, + context_table_info + .as_ref() + .map(|(t, n, r)| (t.as_ref(), n, *r)), + )?; + Ok(WalkControl::Continue) + })?; + + Ok(expr) +} + +/// Rewrite a trigger command to replace column references +#[allow(clippy::too_many_arguments)] +fn rewrite_trigger_cmd_for_column_rename( + cmd: ast::TriggerCmd, + trigger_table: &crate::schema::BTreeTable, + target_table: &crate::schema::BTreeTable, + trigger_table_name: &str, + target_table_name: &str, + old_col_norm: &str, + new_col_norm: &str, + resolver: &Resolver, +) -> Result { + use crate::translate::expr::walk_expr_mut; + + match cmd { + ast::TriggerCmd::Update { + or_conflict, + tbl_name, + mut sets, + from, + where_clause, + } => { + // Get the UPDATE target table to check if we're renaming a column in it + let update_table_name_norm = normalize_ident(tbl_name.as_str()); + let is_renaming_update_table = update_table_name_norm == *target_table_name; + + // Rewrite SET column names if renaming a column in the UPDATE target table + if is_renaming_update_table { + for set in &mut sets { + for col_name in &mut set.col_names { + let col_norm = normalize_ident(col_name.as_str()); + if col_norm == *old_col_norm { + *col_name = ast::Name::from_string(new_col_norm); + } + } + } + } + + // Rewrite SET expressions + for set in &mut sets { + walk_expr_mut( + &mut set.expr, + &mut |e: &mut ast::Expr| -> Result { + rewrite_expr_column_ref( + e, + trigger_table, + trigger_table_name, + target_table_name, + old_col_norm, + new_col_norm, + )?; + Ok(WalkControl::Continue) + }, + )?; + } + + // Rewrite WHERE clause - unqualified column references refer to UPDATE target table + let new_where = where_clause.map(|e| { + Box::new( + rewrite_expr_for_column_rename( + &e, + trigger_table, + trigger_table_name, + target_table_name, + old_col_norm, + new_col_norm, + Some(&update_table_name_norm), // UPDATE WHERE: unqualified refs refer to UPDATE target + resolver, + ) + .unwrap(), + ) + }); + Ok(ast::TriggerCmd::Update { + or_conflict, + tbl_name, + sets, + from, + where_clause: new_where, + }) + } + ast::TriggerCmd::Insert { + or_conflict, + tbl_name, + mut col_names, + select, + upsert, + returning, + } => { + // Rewrite column names in INSERT column list + // Check if the INSERT is targeting the table being renamed + let insert_table_name_norm = normalize_ident(tbl_name.as_str()); + if insert_table_name_norm == *target_table_name { + // This INSERT targets the table being renamed, so rewrite column names + for col_name in &mut col_names { + let col_norm = normalize_ident(col_name.as_str()); + if col_norm == *old_col_norm { + *col_name = ast::Name::from_string(new_col_norm); + } + } + } + // Rewrite SELECT expressions + let new_select = rewrite_select_for_column_rename( + select, + trigger_table, + target_table, + trigger_table_name, + target_table_name, + old_col_norm, + new_col_norm, + )?; + Ok(ast::TriggerCmd::Insert { + or_conflict, + tbl_name, + col_names, + select: new_select, + upsert, + returning, + }) + } + ast::TriggerCmd::Delete { + tbl_name, + where_clause, + } => { + // Get the DELETE target table to check if we're renaming a column in it + let delete_table_name_norm = normalize_ident(tbl_name.as_str()); + + // Rewrite WHERE clause - unqualified column references refer to DELETE target table + let new_where = where_clause.map(|e| { + Box::new( + rewrite_expr_for_column_rename( + &e, + trigger_table, + trigger_table_name, + target_table_name, + old_col_norm, + new_col_norm, + Some(&delete_table_name_norm), // DELETE WHERE: unqualified refs refer to DELETE target + resolver, + ) + .unwrap(), + ) + }); + Ok(ast::TriggerCmd::Delete { + tbl_name, + where_clause: new_where, + }) + } + ast::TriggerCmd::Select(select) => { + let new_select = rewrite_select_for_column_rename( + select, + trigger_table, + target_table, + trigger_table_name, + target_table_name, + old_col_norm, + new_col_norm, + )?; + Ok(ast::TriggerCmd::Select(new_select)) + } + } +} + +/// Rewrite a SELECT statement to replace column references +fn rewrite_select_for_column_rename( + select: ast::Select, + trigger_table: &crate::schema::BTreeTable, + _target_table: &crate::schema::BTreeTable, + trigger_table_name: &str, + target_table_name: &str, + old_col_norm: &str, + new_col_norm: &str, +) -> Result { + use crate::translate::expr::walk_expr_mut; + + let mut rewrite_cb = |e: &mut ast::Expr| -> Result { + rewrite_expr_column_ref( + e, + trigger_table, + trigger_table_name, + target_table_name, + old_col_norm, + new_col_norm, + )?; + Ok(WalkControl::Continue) + }; + + let mut select = select; + + // Rewrite WITH clause (CTEs) + if let Some(ref mut with_clause) = select.with { + for cte in &mut with_clause.ctes { + cte.select = rewrite_select_for_column_rename( + cte.select.clone(), + trigger_table, + _target_table, + trigger_table_name, + target_table_name, + old_col_norm, + new_col_norm, + )?; + } + } + + // Rewrite main SELECT body + rewrite_one_select_for_column_rename( + &mut select.body.select, + trigger_table, + trigger_table_name, + target_table_name, + old_col_norm, + new_col_norm, + )?; + + // Rewrite compound SELECTs (UNION, EXCEPT, INTERSECT) + for compound in &mut select.body.compounds { + rewrite_one_select_for_column_rename( + &mut compound.select, + trigger_table, + trigger_table_name, + target_table_name, + old_col_norm, + new_col_norm, + )?; + } + + // Rewrite ORDER BY + for sorted_col in &mut select.order_by { + walk_expr_mut(&mut sorted_col.expr, &mut rewrite_cb)?; + } + + // Rewrite LIMIT + if let Some(ref mut limit) = select.limit { + walk_expr_mut(&mut limit.expr, &mut rewrite_cb)?; + if let Some(ref mut offset) = limit.offset { + walk_expr_mut(offset, &mut rewrite_cb)?; + } + } + + Ok(select) +} + +/// Rewrite a OneSelect to replace column references +fn rewrite_one_select_for_column_rename( + one_select: &mut ast::OneSelect, + trigger_table: &crate::schema::BTreeTable, + trigger_table_name: &str, + target_table_name: &str, + old_col_norm: &str, + new_col_norm: &str, +) -> Result<()> { + use crate::translate::expr::walk_expr_mut; + + let mut rewrite_cb = |e: &mut ast::Expr| -> Result { + rewrite_expr_column_ref( + e, + trigger_table, + trigger_table_name, + target_table_name, + old_col_norm, + new_col_norm, + )?; + Ok(WalkControl::Continue) + }; + + match one_select { + ast::OneSelect::Select { + columns, + from, + where_clause, + group_by, + window_clause, + .. + } => { + // Rewrite columns + for col in columns { + if let ast::ResultColumn::Expr(expr, _) = col { + walk_expr_mut(expr, &mut rewrite_cb)?; + } + } + + // Rewrite FROM clause and JOIN conditions + if let Some(ref mut from_clause) = from { + rewrite_from_clause_for_column_rename( + from_clause, + trigger_table, + trigger_table_name, + target_table_name, + old_col_norm, + new_col_norm, + )?; + } + + // Rewrite WHERE clause + if let Some(ref mut where_expr) = where_clause { + walk_expr_mut(where_expr, &mut rewrite_cb)?; + } + + // Rewrite GROUP BY and HAVING + if let Some(ref mut group_by) = group_by { + for expr in &mut group_by.exprs { + walk_expr_mut(expr, &mut rewrite_cb)?; + } + if let Some(ref mut having_expr) = group_by.having { + walk_expr_mut(having_expr, &mut rewrite_cb)?; + } + } + + // Rewrite WINDOW clause + for window_def in window_clause { + rewrite_window_for_column_rename( + &mut window_def.window, + trigger_table, + trigger_table_name, + target_table_name, + old_col_norm, + new_col_norm, + )?; + } + } + ast::OneSelect::Values(values) => { + for row in values { + for expr in row { + walk_expr_mut(expr, &mut rewrite_cb)?; + } + } + } + } + Ok(()) +} + +/// Rewrite expressions in a FROM clause (including JOIN conditions) +fn rewrite_from_clause_for_column_rename( + from_clause: &mut ast::FromClause, + trigger_table: &crate::schema::BTreeTable, + trigger_table_name: &str, + target_table_name: &str, + old_col_norm: &str, + new_col_norm: &str, +) -> Result<()> { + use crate::translate::expr::walk_expr_mut; + + let mut rewrite_cb = |e: &mut ast::Expr| -> Result { + rewrite_expr_column_ref( + e, + trigger_table, + trigger_table_name, + target_table_name, + old_col_norm, + new_col_norm, + )?; + Ok(WalkControl::Continue) + }; + + // Rewrite main table (could be a subquery) + rewrite_select_table_for_column_rename( + &mut from_clause.select, + trigger_table, + trigger_table_name, + target_table_name, + old_col_norm, + new_col_norm, + )?; + + // Rewrite JOIN conditions + for join in &mut from_clause.joins { + rewrite_select_table_for_column_rename( + &mut join.table, + trigger_table, + trigger_table_name, + target_table_name, + old_col_norm, + new_col_norm, + )?; + if let Some(ref mut constraint) = join.constraint { + match constraint { + ast::JoinConstraint::On(expr) => { + walk_expr_mut(expr, &mut rewrite_cb)?; + } + ast::JoinConstraint::Using(_) => { + // USING clause contains column names, not expressions + } + } + } + } + Ok(()) +} + +/// Rewrite expressions in a SelectTable (table, subquery, or table function) +fn rewrite_select_table_for_column_rename( + select_table: &mut ast::SelectTable, + trigger_table: &crate::schema::BTreeTable, + trigger_table_name: &str, + target_table_name: &str, + old_col_norm: &str, + new_col_norm: &str, +) -> Result<()> { + use crate::translate::expr::walk_expr_mut; + + let mut rewrite_cb = |e: &mut ast::Expr| -> Result { + rewrite_expr_column_ref( + e, + trigger_table, + trigger_table_name, + target_table_name, + old_col_norm, + new_col_norm, + )?; + Ok(WalkControl::Continue) + }; + + match select_table { + ast::SelectTable::Select(select, _) => { + *select = rewrite_select_for_column_rename( + select.clone(), + trigger_table, + trigger_table, // target_table not needed for subqueries + trigger_table_name, + target_table_name, + old_col_norm, + new_col_norm, + )?; + } + ast::SelectTable::Sub(from_clause, _) => { + rewrite_from_clause_for_column_rename( + from_clause, + trigger_table, + trigger_table_name, + target_table_name, + old_col_norm, + new_col_norm, + )?; + } + ast::SelectTable::TableCall(_, args, _) => { + for arg in args { + walk_expr_mut(arg, &mut rewrite_cb)?; + } + } + ast::SelectTable::Table(_, _, _) => { + // Table reference, no expressions + } + } + Ok(()) +} + +/// Rewrite expressions in a Window definition +fn rewrite_window_for_column_rename( + window: &mut ast::Window, + trigger_table: &crate::schema::BTreeTable, + trigger_table_name: &str, + target_table_name: &str, + old_col_norm: &str, + new_col_norm: &str, +) -> Result<()> { + use crate::translate::expr::walk_expr_mut; + + let mut rewrite_cb = |e: &mut ast::Expr| -> Result { + rewrite_expr_column_ref( + e, + trigger_table, + trigger_table_name, + target_table_name, + old_col_norm, + new_col_norm, + )?; + Ok(WalkControl::Continue) + }; + + // Rewrite PARTITION BY expressions + for expr in &mut window.partition_by { + walk_expr_mut(expr, &mut rewrite_cb)?; + } + + // Rewrite ORDER BY expressions + for sorted_col in &mut window.order_by { + walk_expr_mut(&mut sorted_col.expr, &mut rewrite_cb)?; + } + + // TODO: FrameClause can also contain expressions, but they're more complex + // For now, we'll skip them as they're less common in triggers + Ok(()) +} + +/// Rewrite a single expression's column reference +/// +/// `context_table` is used for UPDATE/DELETE WHERE clauses where unqualified column +/// references refer to the UPDATE/DELETE target table. If `None`, unqualified references +/// refer to the trigger's owning table. +fn rewrite_expr_column_ref_with_context( + e: &mut ast::Expr, + trigger_table: &crate::schema::BTreeTable, + old_col_norm: &str, + new_col_norm: &str, + is_renaming_trigger_table: bool, + context_table: Option<(&crate::schema::BTreeTable, &String, bool)>, +) -> Result<()> { + 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 if this is NEW.column or OLD.column + if (ns_norm.eq_ignore_ascii_case("new") || ns_norm.eq_ignore_ascii_case("old")) + && col_norm == *old_col_norm + { + // NEW.x and OLD.x always refer to the trigger's owning table + if is_renaming_trigger_table && trigger_table.get_column(&col_norm).is_some() { + *col = ast::Name::from_string(new_col_norm); + } + } else if col_norm == *old_col_norm { + // This is a qualified column reference like u.x + // Check if it refers to the context table (UPDATE/DELETE target) + if let Some((_, ctx_name_norm, is_renaming_ctx)) = context_table { + if ns_norm == *ctx_name_norm && is_renaming_ctx { + *col = ast::Name::from_string(new_col_norm); + } + } + } + } + ast::Expr::Id(col) => { + // Unqualified column reference + let col_norm = normalize_ident(col.as_str()); + if col_norm == *old_col_norm { + // Check context table first (for UPDATE/DELETE WHERE clauses) + if let Some((ctx_table, _, is_renaming_ctx)) = context_table { + if ctx_table.get_column(&col_norm).is_some() { + // This refers to the context table (UPDATE/DELETE target) + if is_renaming_ctx { + *e = ast::Expr::Id(ast::Name::from_string(new_col_norm)); + } + return Ok(()); + } + } + // Otherwise, check trigger's owning table + if is_renaming_trigger_table && trigger_table.get_column(&col_norm).is_some() { + *e = ast::Expr::Id(ast::Name::from_string(new_col_norm)); + } + } + } + _ => {} + } + Ok(()) +} + +/// Rewrite a single expression's column reference (convenience wrapper for non-context cases) +fn rewrite_expr_column_ref( + e: &mut ast::Expr, + trigger_table: &crate::schema::BTreeTable, + trigger_table_name: &str, + target_table_name: &str, + old_col_norm: &str, + new_col_norm: &str, +) -> Result<()> { + let trigger_table_name_norm = normalize_ident(trigger_table_name); + let target_table_name_norm = normalize_ident(target_table_name); + let is_renaming_trigger_table = trigger_table_name_norm == target_table_name_norm; + + rewrite_expr_column_ref_with_context( + e, + trigger_table, + old_col_norm, + new_col_norm, + is_renaming_trigger_table, + None, + ) +} diff --git a/tests/integration/trigger.rs b/tests/integration/trigger.rs index 22122e0b7..e51787fc7 100644 --- a/tests/integration/trigger.rs +++ b/tests/integration/trigger.rs @@ -886,3 +886,947 @@ fn test_trigger_with_multiple_statements() { assert_eq!(audit_results[0], "Balance changed for account 1"); assert_eq!(audit_results[1], "Balance changed for account 2"); } + +#[test] +fn test_alter_table_drop_column_fails_when_trigger_references_new_column() { + let db = TempDatabase::new_empty(); + let conn = db.connect_limbo(); + + // Create table with columns + conn.execute("CREATE TABLE t (x INTEGER PRIMARY KEY, y INTEGER)") + .unwrap(); + + // Create trigger that references y via NEW.y + conn.execute( + "CREATE TRIGGER tu BEFORE INSERT ON t BEGIN + INSERT INTO t VALUES (NEW.x, NEW.y); + END", + ) + .unwrap(); + + // Attempting to drop column y should fail because trigger references it + let result = conn.execute("ALTER TABLE t DROP COLUMN y"); + assert!( + result.is_err(), + "Dropping column y should fail when trigger references NEW.y" + ); + + let error_msg = result.unwrap_err().to_string(); + assert!( + error_msg.contains("cannot drop column") && error_msg.contains("trigger"), + "Error should mention column drop and trigger: {error_msg}", + ); +} + +#[test] +fn test_alter_table_drop_column_fails_when_trigger_references_old_column() { + let db = TempDatabase::new_empty(); + let conn = db.connect_limbo(); + + // Create table with columns + conn.execute("CREATE TABLE t (x INTEGER PRIMARY KEY, y INTEGER)") + .unwrap(); + + // Create trigger that references y via OLD.y + conn.execute( + "CREATE TRIGGER tu AFTER UPDATE ON t BEGIN + INSERT INTO t VALUES (OLD.x, OLD.y); + END", + ) + .unwrap(); + + // Attempting to drop column y should fail because trigger references it + let result = conn.execute("ALTER TABLE t DROP COLUMN y"); + assert!( + result.is_err(), + "Dropping column y should fail when trigger references OLD.y" + ); + + let error_msg = result.unwrap_err().to_string(); + assert!( + error_msg.contains("cannot drop column") && error_msg.contains("trigger"), + "Error should mention column drop and trigger: {error_msg}", + ); +} + +#[test] +fn test_alter_table_drop_column_fails_when_trigger_references_unqualified_column() { + let db = TempDatabase::new_empty(); + let conn = db.connect_limbo(); + + // Create table with columns + conn.execute("CREATE TABLE t (x INTEGER PRIMARY KEY, y INTEGER)") + .unwrap(); + + // Create trigger that references y as unqualified column (in WHEN clause) + conn.execute( + "CREATE TRIGGER tu BEFORE INSERT ON t WHEN y > 10 BEGIN + INSERT INTO t VALUES (NEW.x, 100); + END", + ) + .unwrap(); + + // Attempting to drop column y should fail because trigger references it + let result = conn.execute("ALTER TABLE t DROP COLUMN y"); + assert!( + result.is_err(), + "Dropping column y should fail when trigger references it in WHEN clause" + ); + + let error_msg = result.unwrap_err().to_string(); + assert!( + error_msg.contains("cannot drop column") && error_msg.contains("trigger"), + "Error should mention column drop and trigger: {error_msg}", + ); +} + +#[test] +fn test_alter_table_drop_column_succeeds_when_trigger_references_other_table() { + let db = TempDatabase::new_empty(); + let conn = db.connect_limbo(); + + // Create two tables + conn.execute("CREATE TABLE t (x INTEGER PRIMARY KEY, y INTEGER)") + .unwrap(); + conn.execute("CREATE TABLE u (z INTEGER)").unwrap(); + + // Create trigger on t that references column from u (not t) + conn.execute( + "CREATE TRIGGER tu BEFORE INSERT ON t BEGIN + INSERT INTO u(z) VALUES (NEW.x); + END", + ) + .unwrap(); + + // Dropping column y from t should succeed because trigger doesn't reference it + conn.execute("ALTER TABLE t DROP COLUMN y").unwrap(); + + // Verify column was dropped + let mut stmt = conn.prepare("PRAGMA table_info(t)").unwrap(); + let mut columns = Vec::new(); + loop { + match stmt.step().unwrap() { + turso_core::StepResult::Row => { + let row = stmt.row().unwrap(); + columns.push(row.get_value(1).cast_text().unwrap().to_string()); + } + turso_core::StepResult::Done => break, + turso_core::StepResult::IO => { + stmt.run_once().unwrap(); + } + _ => panic!("Unexpected step result"), + } + } + + // Should only have x column now + assert_eq!(columns.len(), 1); + assert_eq!(columns[0], "x"); +} + +#[test] +fn test_alter_table_drop_column_from_other_table_causes_parse_error_when_trigger_fires() { + let db = TempDatabase::new_empty(); + let conn = db.connect_limbo(); + + // Create two tables + conn.execute("CREATE TABLE t (x INTEGER PRIMARY KEY)") + .unwrap(); + conn.execute("CREATE TABLE u (z INTEGER, zer INTEGER)") + .unwrap(); + + // Create trigger on t that references column zer from u + conn.execute( + "CREATE TRIGGER tu BEFORE INSERT ON t BEGIN + INSERT INTO u(z, zer) VALUES (NEW.x, NEW.x); + END", + ) + .unwrap(); + + // Dropping column zer from u should succeed (trigger is on t, not u) + conn.execute("ALTER TABLE u DROP COLUMN zer").unwrap(); + + // Verify column was dropped + let mut stmt = conn.prepare("PRAGMA table_info(u)").unwrap(); + let mut columns = Vec::new(); + loop { + match stmt.step().unwrap() { + turso_core::StepResult::Row => { + let row = stmt.row().unwrap(); + columns.push(row.get_value(1).cast_text().unwrap().to_string()); + } + turso_core::StepResult::Done => break, + turso_core::StepResult::IO => { + stmt.run_once().unwrap(); + } + _ => panic!("Unexpected step result"), + } + } + + // Should only have z column now + assert_eq!(columns.len(), 1); + assert_eq!(columns[0], "z"); + + // Now trying to insert into t should fail because trigger references non-existent column zer + let result = conn.execute("INSERT INTO t VALUES (1)"); + assert!( + result.is_err(), + "Insert should fail because trigger references non-existent column zer" + ); + + let error_msg = result.unwrap_err().to_string(); + assert!( + error_msg.contains("no column named") || error_msg.contains("zer"), + "Error should mention missing column: {error_msg}", + ); +} + +#[test] +fn test_alter_table_rename_column_propagates_to_trigger_on_owning_table() { + let db = TempDatabase::new_empty(); + let conn = db.connect_limbo(); + + // Create table + conn.execute("CREATE TABLE t (x INTEGER PRIMARY KEY, y INTEGER)") + .unwrap(); + + // Create trigger on t that references y via NEW.y + conn.execute( + "CREATE TRIGGER tu BEFORE INSERT ON t BEGIN + INSERT INTO t VALUES (NEW.x, NEW.y); + END", + ) + .unwrap(); + + // Rename column y to y_new + conn.execute("ALTER TABLE t RENAME COLUMN y TO y_new") + .unwrap(); + + // 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 should reference y_new instead of y + let normalized_sql = trigger_sql.split_whitespace().collect::>().join(" "); + assert_eq!( + normalized_sql, + "CREATE TRIGGER tu BEFORE INSERT ON t BEGIN INSERT INTO t VALUES (NEW.x, NEW.y_new); END" + ); +} + +#[test] +fn test_alter_table_rename_column_propagates_to_trigger_referencing_other_table() { + let db = TempDatabase::new_empty(); + let conn = db.connect_limbo(); + + // Create two tables + conn.execute("CREATE TABLE t (x INTEGER PRIMARY KEY)") + .unwrap(); + conn.execute("CREATE TABLE u (z INTEGER, zer INTEGER)") + .unwrap(); + + // Create trigger on t that references column z from u + conn.execute( + "CREATE TRIGGER tu BEFORE INSERT ON t BEGIN + INSERT INTO u(z, zer) VALUES (NEW.x, NEW.x); + END", + ) + .unwrap(); + + // Rename column z to zoo in table u + conn.execute("ALTER TABLE u RENAME COLUMN z TO zoo") + .unwrap(); + + // Verify trigger SQL was updated to reference zoo + 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 should reference zoo instead of z + let normalized_sql = trigger_sql.split_whitespace().collect::>().join(" "); + assert_eq!( + normalized_sql, + "CREATE TRIGGER tu BEFORE INSERT ON t BEGIN INSERT INTO u (zoo, zer) VALUES (NEW.x, NEW.x); END" + ); +} + +#[test] +fn test_alter_table_rename_column_propagates_to_trigger_with_multiple_references() { + let db = TempDatabase::new_empty(); + let conn = db.connect_limbo(); + + // Create two tables + conn.execute("CREATE TABLE t (x INTEGER PRIMARY KEY, y INTEGER)") + .unwrap(); + conn.execute("CREATE TABLE u (z INTEGER, zer INTEGER)") + .unwrap(); + + // Create trigger on t that references y multiple times and z from u + conn.execute( + "CREATE TRIGGER tu BEFORE INSERT ON t BEGIN + INSERT INTO u(z, zer) VALUES (NEW.y, NEW.y); + END", + ) + .unwrap(); + + // Rename column y to y_new in table t + conn.execute("ALTER TABLE t RENAME COLUMN y TO y_new") + .unwrap(); + + // 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 should reference y_new instead of y + let normalized_sql = trigger_sql.split_whitespace().collect::>().join(" "); + assert_eq!( + normalized_sql, + "CREATE TRIGGER tu BEFORE INSERT ON t BEGIN INSERT INTO u (z, zer) VALUES (NEW.y_new, NEW.y_new); END" + ); +} + +#[test] +fn test_alter_table_rename_column_propagates_to_trigger_when_clause() { + let db = TempDatabase::new_empty(); + let conn = db.connect_limbo(); + + // Create table + conn.execute("CREATE TABLE t (x INTEGER PRIMARY KEY, y INTEGER)") + .unwrap(); + + // Create trigger with WHEN clause referencing y + conn.execute( + "CREATE TRIGGER tu BEFORE INSERT ON t WHEN y > 10 BEGIN + INSERT INTO t VALUES (NEW.x, 100); + END", + ) + .unwrap(); + + // Rename column y to y_new + conn.execute("ALTER TABLE t RENAME COLUMN y TO y_new") + .unwrap(); + + // 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" + ); +} + +#[test] +fn test_alter_table_rename_column_propagates_to_multiple_triggers() { + let db = TempDatabase::new_empty(); + let conn = db.connect_limbo(); + + // Create table + conn.execute("CREATE TABLE t (x INTEGER PRIMARY KEY, y INTEGER)") + .unwrap(); + + // Create multiple triggers referencing y + conn.execute( + "CREATE TRIGGER t1 BEFORE INSERT ON t BEGIN + INSERT INTO t VALUES (NEW.x, NEW.y); + END", + ) + .unwrap(); + + conn.execute( + "CREATE TRIGGER t2 AFTER UPDATE ON t BEGIN + INSERT INTO t VALUES (OLD.x, OLD.y); + END", + ) + .unwrap(); + + // Rename column y to y_new + conn.execute("ALTER TABLE t RENAME COLUMN y TO y_new") + .unwrap(); + + // Verify both triggers were updated + let mut stmt = conn + .prepare("SELECT sql FROM sqlite_schema WHERE type='trigger' ORDER BY name") + .unwrap(); + let mut trigger_sqls = Vec::new(); + loop { + match stmt.step().unwrap() { + turso_core::StepResult::Row => { + let row = stmt.row().unwrap(); + trigger_sqls.push(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"), + } + } + + // Both triggers should reference y_new + assert_eq!(trigger_sqls.len(), 2); + let normalized_t1 = trigger_sqls[0] + .split_whitespace() + .collect::>() + .join(" "); + let normalized_t2 = trigger_sqls[1] + .split_whitespace() + .collect::>() + .join(" "); + assert_eq!( + normalized_t1, + "CREATE TRIGGER t1 BEFORE INSERT ON t BEGIN INSERT INTO t VALUES (NEW.x, NEW.y_new); END" + ); + assert_eq!( + normalized_t2, + "CREATE TRIGGER t2 AFTER UPDATE ON t BEGIN INSERT INTO t VALUES (OLD.x, OLD.y_new); END" + ); +} + +#[test] +fn test_alter_table_drop_column_fails_with_old_reference_in_update_trigger() { + let db = TempDatabase::new_empty(); + let conn = db.connect_limbo(); + + // Create table + conn.execute("CREATE TABLE t (x INTEGER PRIMARY KEY, y INTEGER)") + .unwrap(); + + // Create UPDATE trigger that references OLD.y + conn.execute( + "CREATE TRIGGER tu AFTER UPDATE ON t BEGIN + INSERT INTO t VALUES (OLD.x, OLD.y); + END", + ) + .unwrap(); + + // Attempting to drop column y should fail + let result = conn.execute("ALTER TABLE t DROP COLUMN y"); + assert!( + result.is_err(), + "Dropping column y should fail when UPDATE trigger references OLD.y" + ); +} + +#[test] +fn test_alter_table_rename_column_in_insert_column_list() { + let db = TempDatabase::new_empty(); + let conn = db.connect_limbo(); + + // Create two tables + conn.execute("CREATE TABLE t (x INTEGER PRIMARY KEY)") + .unwrap(); + conn.execute("CREATE TABLE u (z INTEGER, zer INTEGER)") + .unwrap(); + + // Create trigger that inserts into u with explicit column list + conn.execute( + "CREATE TRIGGER tu BEFORE INSERT ON t BEGIN + INSERT INTO u(z, zer) VALUES (NEW.x, NEW.x); + END", + ) + .unwrap(); + + // Rename column zer to zercher in table u + conn.execute("ALTER TABLE u RENAME COLUMN zer TO zercher") + .unwrap(); + + // 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 should reference zercher in INSERT column list + let normalized_sql = trigger_sql.split_whitespace().collect::>().join(" "); + assert_eq!( + normalized_sql, + "CREATE TRIGGER tu BEFORE INSERT ON t BEGIN INSERT INTO u (z, zercher) VALUES (NEW.x, NEW.x); END" + ); +} + +#[test] +fn test_alter_table_rename_column_in_trigger_table_does_not_rewrite_other_table_column() { + let db = TempDatabase::new_empty(); + let conn = db.connect_limbo(); + + // Create two tables, both with column 'x' + conn.execute("CREATE TABLE t (x INTEGER PRIMARY KEY, y INTEGER)") + .unwrap(); + conn.execute("CREATE TABLE u (x INTEGER, z INTEGER)") + .unwrap(); + + // Create trigger on t that references both t.x (via NEW.x) and u.x (in INSERT column list) + conn.execute( + "CREATE TRIGGER tu BEFORE INSERT ON t BEGIN + INSERT INTO u(x, z) VALUES (NEW.x, NEW.y); + END", + ) + .unwrap(); + + // Rename column x to x_new in table t (the trigger's owning table) + conn.execute("ALTER TABLE t RENAME COLUMN x TO x_new") + .unwrap(); + + // Verify trigger SQL was updated correctly: + // - NEW.x should become NEW.x_new (refers to table t) + // - INSERT INTO u(x, ...) should remain as x (refers to table u, not t) + 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"), + } + } + + let normalized_sql = trigger_sql.split_whitespace().collect::>().join(" "); + // NEW.x should be rewritten to NEW.x_new + assert!( + normalized_sql.contains("NEW.x_new"), + "Trigger SQL should contain NEW.x_new: {normalized_sql}", + ); + // INSERT INTO u (x, ...) should still have x (not x_new) because it refers to table u's column + assert!( + normalized_sql.contains("INSERT INTO u (x,") || normalized_sql.contains("INSERT INTO u(x,"), + "Trigger SQL should contain INSERT INTO u (x, (not u (x_new,): {normalized_sql}", + ); + assert!( + !normalized_sql.contains("INSERT INTO u (x_new,") && !normalized_sql.contains("INSERT INTO u(x_new,"), + "Trigger SQL should NOT contain INSERT INTO u (x_new, (x refers to table u): {normalized_sql}", + ); +} + +#[test] +fn test_alter_table_rename_column_in_insert_target_table_does_not_rewrite_trigger_table_column() { + let db = TempDatabase::new_empty(); + let conn = db.connect_limbo(); + + // Create two tables, both with column 'x' + conn.execute("CREATE TABLE t (x INTEGER PRIMARY KEY, y INTEGER)") + .unwrap(); + conn.execute("CREATE TABLE u (x INTEGER, z INTEGER)") + .unwrap(); + + // Create trigger on t that references both t.x (via NEW.x) and u.x (in INSERT column list) + conn.execute( + "CREATE TRIGGER tu BEFORE INSERT ON t BEGIN + INSERT INTO u(x, z) VALUES (NEW.x, NEW.y); + END", + ) + .unwrap(); + + // Rename column x to x_new in table u (the INSERT target table) + conn.execute("ALTER TABLE u RENAME COLUMN x TO x_new") + .unwrap(); + + // Verify trigger SQL was updated correctly: + // - NEW.x should remain as NEW.x (refers to table t, not u) + // - INSERT INTO u(x, ...) should become u(x_new, ...) (refers to table u) + 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"), + } + } + + let normalized_sql = trigger_sql.split_whitespace().collect::>().join(" "); + // NEW.x should remain as NEW.x (not rewritten because it refers to table t) + assert!( + normalized_sql.contains("NEW.x"), + "Trigger SQL should contain NEW.x (not NEW.x_new): {normalized_sql}", + ); + assert!( + !normalized_sql.contains("NEW.x_new"), + "Trigger SQL should NOT contain NEW.x_new (x refers to table t, not u): {normalized_sql}", + ); + // INSERT INTO u (x_new, ...) should have x_new (rewritten because it refers to table u) + assert!( + normalized_sql.contains("INSERT INTO u (x_new,") + || normalized_sql.contains("INSERT INTO u(x_new,"), + "Trigger SQL should contain INSERT INTO u (x_new, (not u (x,): {normalized_sql}", + ); + assert!( + !normalized_sql.contains("INSERT INTO u (x,") && !normalized_sql.contains("INSERT INTO u(x,"), + "Trigger SQL should NOT contain INSERT INTO u (x, (x was renamed to x_new in table u): {normalized_sql}", + ); +} + +#[test] +fn test_alter_table_rename_column_update_where_clause_does_not_rewrite_target_table_column() { + 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, with WHERE clause referencing u.x (unqualified) + conn.execute( + "CREATE TRIGGER tu AFTER UPDATE ON t BEGIN + UPDATE u SET z = NEW.x WHERE x = OLD.x; + END", + ) + .unwrap(); + + // Rename column x to x_new in table t (the trigger's owning table) + conn.execute("ALTER TABLE t RENAME COLUMN x TO x_new") + .unwrap(); + + // Verify trigger SQL: WHERE x should remain as x (refers to table u, not t) + 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"), + } + } + + let normalized_sql = trigger_sql.split_whitespace().collect::>().join(" "); + // NEW.x and OLD.x should be rewritten to x_new + assert!( + normalized_sql.contains("NEW.x_new") && normalized_sql.contains("OLD.x_new"), + "Trigger SQL should contain NEW.x_new and OLD.x_new: {normalized_sql}", + ); + // WHERE x should remain as x (not x_new) because it refers to table u's column + assert!( + normalized_sql.contains("WHERE x =") || normalized_sql.contains("WHERE x="), + "Trigger SQL should contain WHERE x = (not WHERE x_new =): {normalized_sql}", + ); + assert!( + !normalized_sql.contains("WHERE x_new =") && !normalized_sql.contains("WHERE x_new="), + "Trigger SQL should NOT contain WHERE x_new = (x refers to table u): {normalized_sql}", + ); +} + +#[test] +fn test_alter_table_rename_column_update_set_column_name_rewritten() { + 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, setting u.x + conn.execute( + "CREATE TRIGGER tu AFTER UPDATE ON t BEGIN + UPDATE u SET x = NEW.x WHERE u.x = OLD.x; + END", + ) + .unwrap(); + + // Rename column x to x_new in table u (the UPDATE target table) + conn.execute("ALTER TABLE u RENAME COLUMN x TO x_new") + .unwrap(); + + // Verify trigger SQL: SET x should become SET x_new, WHERE u.x should become u.x_new + 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"), + } + } + + let normalized_sql = trigger_sql.split_whitespace().collect::>().join(" "); + // SET x should become SET x_new + assert!( + normalized_sql.contains("SET x_new =") || normalized_sql.contains("SET x_new="), + "Trigger SQL should contain SET x_new =: {normalized_sql}", + ); + assert!( + !normalized_sql.contains("SET x =") && !normalized_sql.contains("SET x="), + "Trigger SQL should NOT contain SET x = (x was renamed to x_new): {normalized_sql}", + ); + // WHERE u.x should become u.x_new + assert!( + normalized_sql.contains("u.x_new =") || normalized_sql.contains("u.x_new="), + "Trigger SQL should contain u.x_new =: {normalized_sql}", + ); +} + +#[test] +fn test_alter_table_rename_column_delete_where_clause_does_not_rewrite_target_table_column() { + 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 deletes from u, with WHERE clause referencing u.x (unqualified) + conn.execute( + "CREATE TRIGGER tu AFTER DELETE ON t BEGIN + DELETE FROM u WHERE x = OLD.x; + END", + ) + .unwrap(); + + // Rename column x to x_new in table t (the trigger's owning table) + conn.execute("ALTER TABLE t RENAME COLUMN x TO x_new") + .unwrap(); + + // Verify trigger SQL: WHERE x should remain as x (refers to table u, not t) + 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"), + } + } + + let normalized_sql = trigger_sql.split_whitespace().collect::>().join(" "); + // OLD.x should be rewritten to OLD.x_new + assert!( + normalized_sql.contains("OLD.x_new"), + "Trigger SQL should contain OLD.x_new: {normalized_sql}", + ); + // WHERE x should remain as x (not x_new) because it refers to table u's column + assert!( + normalized_sql.contains("WHERE x =") || normalized_sql.contains("WHERE x="), + "Trigger SQL should contain WHERE x = (not WHERE x_new =): {normalized_sql}", + ); + assert!( + !normalized_sql.contains("WHERE x_new =") && !normalized_sql.contains("WHERE x_new="), + "Trigger SQL should NOT contain WHERE x_new = (x refers to table u): {normalized_sql}", + ); +} + +#[test] +fn test_alter_table_rename_column_update_of_column_list_rewritten() { + let db = TempDatabase::new_empty(); + let conn = db.connect_limbo(); + + // Create table + conn.execute("CREATE TABLE t (x INTEGER, y INTEGER)") + .unwrap(); + conn.execute("CREATE TABLE u (x INTEGER, z INTEGER)") + .unwrap(); + + // Create trigger with UPDATE OF x + conn.execute( + "CREATE TRIGGER tu AFTER UPDATE OF x ON t BEGIN + UPDATE u SET z = NEW.x WHERE x = OLD.x; + END", + ) + .unwrap(); + + // Rename column x to x_new in table t + conn.execute("ALTER TABLE t RENAME COLUMN x TO x_new") + .unwrap(); + + // Verify trigger SQL: UPDATE OF x should become UPDATE OF x_new + 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"), + } + } + + let normalized_sql = trigger_sql.split_whitespace().collect::>().join(" "); + // UPDATE OF x should become UPDATE OF x_new + assert!( + normalized_sql.contains("UPDATE OF x_new"), + "Trigger SQL should contain UPDATE OF x_new: {normalized_sql}", + ); + assert!( + !normalized_sql.contains("UPDATE OF x,") && !normalized_sql.contains("UPDATE OF x "), + "Trigger SQL should NOT contain UPDATE OF x (x was renamed to x_new): {normalized_sql}", + ); +} + +#[test] +fn test_alter_table_rename_column_update_of_multiple_columns_rewritten() { + let db = TempDatabase::new_empty(); + let conn = db.connect_limbo(); + + // Create table + conn.execute("CREATE TABLE t (x INTEGER, y INTEGER)") + .unwrap(); + conn.execute("CREATE TABLE u (x INTEGER, z INTEGER)") + .unwrap(); + + // Create trigger with UPDATE OF x, y + conn.execute( + "CREATE TRIGGER tu AFTER UPDATE OF x, y ON t BEGIN + UPDATE u SET z = NEW.x WHERE x = OLD.x; + END", + ) + .unwrap(); + + // Rename column x to x_new in table t + conn.execute("ALTER TABLE t RENAME COLUMN x TO x_new") + .unwrap(); + + // Verify trigger SQL: UPDATE OF x, y should become UPDATE OF x_new, y + 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"), + } + } + + let normalized_sql = trigger_sql.split_whitespace().collect::>().join(" "); + // UPDATE OF x, y should become UPDATE OF x_new, y + assert!( + normalized_sql.contains("UPDATE OF x_new, y") + || normalized_sql.contains("UPDATE OF x_new,y"), + "Trigger SQL should contain UPDATE OF x_new, y: {normalized_sql}", + ); + assert!( + !normalized_sql.contains("UPDATE OF x,") && !normalized_sql.contains("UPDATE OF x "), + "Trigger SQL should NOT contain UPDATE OF x (x was renamed to x_new): {normalized_sql}", + ); +} From 5b1c69a9d0a14c6800a7c77fd10760ae5ee3f543 Mon Sep 17 00:00:00 2001 From: Jussi Saurio Date: Wed, 19 Nov 2025 12:33:13 +0200 Subject: [PATCH 2/7] fix ai slop with more ai slop --- core/translate/alter.rs | 123 ++++++++++++++++++++++++++-------------- 1 file changed, 80 insertions(+), 43 deletions(-) diff --git a/core/translate/alter.rs b/core/translate/alter.rs index 1fcfbfbc3..41000e768 100644 --- a/core/translate/alter.rs +++ b/core/translate/alter.rs @@ -166,7 +166,11 @@ pub fn translate_alter_table( ); let where_copy = index .bind_where_expr(Some(&mut table_references), connection) - .expect("where clause to exist"); + .ok_or_else(|| { + LimboError::ParseError( + "index where clause unexpectedly missing".to_string(), + ) + })?; let mut column_referenced = false; walk_expr( &where_copy, @@ -224,8 +228,13 @@ pub fn translate_alter_table( ); let mut parser = Parser::new(stmt.as_bytes()); - let Some(ast::Cmd::Stmt(ast::Stmt::Update(update))) = parser.next_cmd().unwrap() else { - unreachable!(); + let cmd = parser.next_cmd().map_err(|e| { + LimboError::ParseError(format!("failed to parse generated UPDATE statement: {e}")) + })?; + let Some(ast::Cmd::Stmt(ast::Stmt::Update(update))) = cmd else { + return Err(LimboError::ParseError( + "generated UPDATE statement did not parse as expected".to_string(), + )); }; translate_update_for_schema_change( @@ -332,7 +341,11 @@ pub fn translate_alter_table( } } - let new_column_name = column.name.as_ref().unwrap(); + let new_column_name = column.name.as_ref().ok_or_else(|| { + LimboError::ParseError( + "column name is missing in ALTER TABLE ADD COLUMN".to_string(), + ) + })?; if btree.get_column(new_column_name).is_some() { return Err(LimboError::ParseError( "duplicate column name: ".to_string() + new_column_name, @@ -413,8 +426,13 @@ pub fn translate_alter_table( ); let mut parser = Parser::new(stmt.as_bytes()); - let Some(ast::Cmd::Stmt(ast::Stmt::Update(update))) = parser.next_cmd().unwrap() else { - unreachable!(); + let cmd = parser.next_cmd().map_err(|e| { + LimboError::ParseError(format!("failed to parse generated UPDATE statement: {e}")) + })?; + let Some(ast::Cmd::Stmt(ast::Stmt::Update(update))) = cmd else { + return Err(LimboError::ParseError( + "generated UPDATE statement did not parse as expected".to_string(), + )); }; translate_update_for_schema_change( @@ -453,10 +471,15 @@ pub fn translate_alter_table( ))); }; - let sqlite_schema = resolver - .schema - .get_btree_table(SQLITE_TABLEID) - .expect("sqlite_schema should be on schema"); + let sqlite_schema = + resolver + .schema + .get_btree_table(SQLITE_TABLEID) + .ok_or_else(|| { + LimboError::ParseError( + "sqlite_schema table not found in schema".to_string(), + ) + })?; let cursor_id = program.alloc_cursor_id(CursorType::BTreeTable(sqlite_schema.clone())); @@ -624,7 +647,6 @@ pub fn translate_alter_table( trigger, &target_table_name_norm, from, - resolver, )?; } @@ -651,10 +673,15 @@ pub fn translate_alter_table( } } - let sqlite_schema = resolver - .schema - .get_btree_table(SQLITE_TABLEID) - .expect("sqlite_schema should be on schema"); + let sqlite_schema = + resolver + .schema + .get_btree_table(SQLITE_TABLEID) + .ok_or_else(|| { + LimboError::ParseError( + "sqlite_schema table not found in schema".to_string(), + ) + })?; let cursor_id = program.alloc_cursor_id(CursorType::BTreeTable(sqlite_schema.clone())); @@ -730,8 +757,12 @@ pub fn translate_alter_table( ); let mut parser = Parser::new(update_stmt.as_bytes()); - let Some(ast::Cmd::Stmt(ast::Stmt::Update(update))) = parser.next_cmd().unwrap() - else { + let cmd = parser.next_cmd().map_err(|e| { + LimboError::ParseError(format!( + "failed to parse trigger update SQL for {trigger_name}: {e}" + )) + })?; + let Some(ast::Cmd::Stmt(ast::Stmt::Update(update))) = cmd else { return Err(LimboError::ParseError(format!( "failed to parse trigger update SQL for {trigger_name}", ))); @@ -787,7 +818,9 @@ fn translate_rename_virtual_table( let sqlite_schema = resolver .schema .get_btree_table(SQLITE_TABLEID) - .expect("sqlite_schema should be on schema"); + .ok_or_else(|| { + LimboError::ParseError("sqlite_schema table not found in schema".to_string()) + })?; let schema_cur = program.alloc_cursor_id(CursorType::BTreeTable(sqlite_schema.clone())); program.emit_insn(Insn::OpenWrite { @@ -1009,7 +1042,6 @@ fn trigger_references_column_in_other_tables( trigger: &crate::schema::Trigger, target_table_name: &str, column_name: &str, - _resolver: &Resolver, ) -> Result { let column_name_norm = normalize_ident(column_name); let target_table_name_norm = normalize_ident(target_table_name); @@ -1340,6 +1372,9 @@ fn rewrite_trigger_sql_for_column_rename( // Parse the trigger SQL let mut parser = Parser::new(trigger_sql.as_bytes()); + let cmd = parser + .next_cmd() + .map_err(|e| LimboError::ParseError(format!("failed to parse trigger SQL: {e}")))?; let Some(ast::Cmd::Stmt(ast::Stmt::CreateTrigger { temporary, if_not_exists, @@ -1350,7 +1385,7 @@ fn rewrite_trigger_sql_for_column_rename( for_each_row, when_clause, commands, - })) = parser.next_cmd().unwrap() + })) = cmd else { return Err(LimboError::ParseError(format!( "failed to parse trigger SQL: {trigger_sql}" @@ -1400,19 +1435,21 @@ fn rewrite_trigger_sql_for_column_rename( }; // 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, - ) - .unwrap() - }); + 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()?; let mut new_commands = Vec::new(); for cmd in commands { @@ -1559,8 +1596,8 @@ fn rewrite_trigger_cmd_for_column_rename( } // Rewrite WHERE clause - unqualified column references refer to UPDATE target table - let new_where = where_clause.map(|e| { - Box::new( + let new_where = where_clause + .map(|e| { rewrite_expr_for_column_rename( &e, trigger_table, @@ -1571,9 +1608,9 @@ fn rewrite_trigger_cmd_for_column_rename( Some(&update_table_name_norm), // UPDATE WHERE: unqualified refs refer to UPDATE target resolver, ) - .unwrap(), - ) - }); + .map(Box::new) + }) + .transpose()?; Ok(ast::TriggerCmd::Update { or_conflict, tbl_name, @@ -1629,8 +1666,8 @@ fn rewrite_trigger_cmd_for_column_rename( let delete_table_name_norm = normalize_ident(tbl_name.as_str()); // Rewrite WHERE clause - unqualified column references refer to DELETE target table - let new_where = where_clause.map(|e| { - Box::new( + let new_where = where_clause + .map(|e| { rewrite_expr_for_column_rename( &e, trigger_table, @@ -1641,9 +1678,9 @@ fn rewrite_trigger_cmd_for_column_rename( Some(&delete_table_name_norm), // DELETE WHERE: unqualified refs refer to DELETE target resolver, ) - .unwrap(), - ) - }); + .map(Box::new) + }) + .transpose()?; Ok(ast::TriggerCmd::Delete { tbl_name, where_clause: new_where, From 745cdc3aa2b1c15d43e5b9e4ee3d5269e91c9a24 Mon Sep 17 00:00:00 2001 From: Jussi Saurio Date: Wed, 19 Nov 2025 12:55:25 +0200 Subject: [PATCH 3/7] Align trigger sql rewrite behavior with sqlite SQLite doesn't rewrite INSERT lists or WHEN clause, it instead lets the trigger go "stale" and will cause runtime errors. This may not be great behavior, but it's compatible... --- core/translate/alter.rs | 122 +++++++++++++++++++++--------- tests/integration/trigger.rs | 143 ++++++++++++++++++++++++++++------- 2 files changed, 201 insertions(+), 64 deletions(-) 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" + ); +} From dbdf60a628798347433145cbc44d0418f2291c5a Mon Sep 17 00:00:00 2001 From: Jussi Saurio Date: Wed, 19 Nov 2025 13:10:23 +0200 Subject: [PATCH 4/7] extract common functionality --- core/translate/alter.rs | 125 ++++++++++++++++++---------------------- 1 file changed, 56 insertions(+), 69 deletions(-) diff --git a/core/translate/alter.rs b/core/translate/alter.rs index 2a6c56c70..076fd8b3c 100644 --- a/core/translate/alter.rs +++ b/core/translate/alter.rs @@ -635,45 +635,11 @@ pub fn translate_alter_table( )) })?; - 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 { + if expr_references_trigger_column( + when_expr, + &trigger_table, + &column_name_norm, + )? { return Err(LimboError::ParseError(format!( "error in trigger {}: no such column: {}", trigger.name, from @@ -962,6 +928,50 @@ fn translate_rename_virtual_table( /* Triggers must be rewritten when a column is renamed, and DROP COLUMN on table T must be forbidden if any trigger on T references the column. Here are some helpers related to that: */ +/// Check if an expression references a specific column from a trigger's owning table. +/// Checks for NEW.column, OLD.column, and unqualified column references. +/// Returns true if the column is found, false otherwise. +fn expr_references_trigger_column( + expr: &ast::Expr, + table: &crate::schema::BTreeTable, + column_name_norm: &str, +) -> Result { + use crate::translate::expr::walk_expr; + let mut found = false; + walk_expr(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 = 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 table.get_column(&col_norm).is_some() { + found = true; + return Ok(crate::translate::expr::WalkControl::SkipChildren); + } + } + } + _ => {} + } + Ok(crate::translate::expr::WalkControl::Continue) + })?; + Ok(found) +} + /// Check if a trigger references a specific column from its owning table. /// Returns true if the column is referenced as old.x, new.x, or unqualified x. fn trigger_references_column( @@ -974,35 +984,7 @@ fn trigger_references_column( // Check when_clause if let Some(ref when_expr) = trigger.when_clause { - walk_expr(when_expr, &mut |e: &ast::Expr| -> Result { - 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 = true; - return Ok(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 table.get_column(&col_norm).is_some() { - found = true; - return Ok(WalkControl::SkipChildren); - } - } - } - _ => {} - } - Ok(WalkControl::Continue) - })?; + found = expr_references_trigger_column(when_expr, table, &column_name_norm)?; } if found { @@ -1064,7 +1046,8 @@ fn trigger_references_column( Ok(found) } -/// Helper to check if an expression references a column +/// Helper to check if an expression references a column. +/// Used as a callback within walk_expr, so it mutates a found flag. fn check_column_ref( e: &ast::Expr, table: &crate::schema::BTreeTable, @@ -1778,6 +1761,8 @@ fn rewrite_select_for_column_rename( let mut select = select; // Rewrite WITH clause (CTEs) + // Note: We clone here because Select doesn't implement Default, so we can't use mem::take. + // The clone is necessary to move ownership to rewrite_select_for_column_rename. if let Some(ref mut with_clause) = select.with { for cte in &mut with_clause.ctes { cte.select = rewrite_select_for_column_rename( @@ -2001,6 +1986,8 @@ fn rewrite_select_table_for_column_rename( match select_table { ast::SelectTable::Select(select, _) => { + // Note: We clone here because Select doesn't implement Default, so we can't use mem::take. + // The clone is necessary to move ownership to rewrite_select_for_column_rename. *select = rewrite_select_for_column_rename( select.clone(), trigger_table, From 5d9a0b15f8b789c7f77d883d26f0ecec6e928ce6 Mon Sep 17 00:00:00 2001 From: Jussi Saurio Date: Wed, 19 Nov 2025 13:21:29 +0200 Subject: [PATCH 5/7] Handle qualified column references in triggers wrt ALTER TABLE --- core/translate/alter.rs | 714 +++++++++++++++++++++++++++++++---- tests/integration/trigger.rs | 36 ++ 2 files changed, 683 insertions(+), 67 deletions(-) diff --git a/core/translate/alter.rs b/core/translate/alter.rs index 076fd8b3c..5c7302f29 100644 --- a/core/translate/alter.rs +++ b/core/translate/alter.rs @@ -622,22 +622,24 @@ pub fn translate_alter_table( 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 + // or if trigger commands contain qualified references to the trigger table (e.g., t.x) 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 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}" + )) + })?; + // Check WHEN clause + if let Some(ref when_expr) = trigger.when_clause { if expr_references_trigger_column( when_expr, &trigger_table, + &trigger_table_name_norm, &column_name_norm, )? { return Err(LimboError::ParseError(format!( @@ -646,26 +648,479 @@ pub fn translate_alter_table( ))); } } - } - let mut needs_rewrite = false; + // Check trigger commands for qualified references to trigger table (e.g., t.x) + // SQLite fails RENAME COLUMN if triggers contain qualified references like t.x + // We check all expressions in trigger commands for qualified references + use crate::translate::expr::walk_expr; + let mut found_qualified_ref = false; + for cmd in &trigger.commands { + match cmd { + ast::TriggerCmd::Update { + sets, where_clause, .. + } => { + for set in sets { + walk_expr( + &set.expr, + &mut |e: &ast::Expr| -> Result< + crate::translate::expr::WalkControl, + > { + if let ast::Expr::Qualified(ns, col) + | ast::Expr::DoublyQualified(_, ns, col) = e + { + let ns_norm = normalize_ident(ns.as_str()); + let col_norm = normalize_ident(col.as_str()); + // Check if it's a qualified reference to trigger table (not NEW/OLD) + if !ns_norm.eq_ignore_ascii_case("new") + && !ns_norm.eq_ignore_ascii_case("old") + && ns_norm == trigger_table_name_norm + && col_norm == column_name_norm + { + if trigger_table + .get_column(&col_norm) + .is_some() + { + found_qualified_ref = true; + return Ok(crate::translate::expr::WalkControl::SkipChildren); + } + } + } + Ok(crate::translate::expr::WalkControl::Continue) + }, + )?; + if found_qualified_ref { + break; + } + } + if !found_qualified_ref { + if let Some(ref where_expr) = where_clause { + walk_expr(where_expr, &mut |e: &ast::Expr| -> Result { + if let ast::Expr::Qualified(ns, col) | ast::Expr::DoublyQualified(_, ns, col) = e { + let ns_norm = normalize_ident(ns.as_str()); + let col_norm = normalize_ident(col.as_str()); + if !ns_norm.eq_ignore_ascii_case("new") + && !ns_norm.eq_ignore_ascii_case("old") + && ns_norm == trigger_table_name_norm + && col_norm == column_name_norm + { + if trigger_table.get_column(&col_norm).is_some() { + found_qualified_ref = true; + return Ok(crate::translate::expr::WalkControl::SkipChildren); + } + } + } + Ok(crate::translate::expr::WalkControl::Continue) + })?; + } + } + } + ast::TriggerCmd::Insert { select, .. } + | ast::TriggerCmd::Select(select) => { + // Walk through all expressions in SELECT and check ONLY for qualified refs to trigger table + // We need to check specifically for qualified refs (t.x), not NEW.x, OLD.x, or unqualified x + // Use walk_select_expressions but check only for qualified refs manually + use crate::translate::expr::walk_expr; + // Helper function to check for qualified refs to trigger table + fn check_qualified_ref_to_trigger_table( + e: &ast::Expr, + trigger_table: &crate::schema::BTreeTable, + trigger_table_name_norm: &str, + column_name_norm: &str, + found: &mut bool, + ) -> Result + { + if let ast::Expr::Qualified(ns, col) + | ast::Expr::DoublyQualified(_, ns, col) = e + { + let ns_norm = normalize_ident(ns.as_str()); + let col_norm = normalize_ident(col.as_str()); + // Only check for qualified refs to trigger table (not NEW/OLD) + if !ns_norm.eq_ignore_ascii_case("new") + && !ns_norm.eq_ignore_ascii_case("old") + && ns_norm == trigger_table_name_norm + && col_norm == column_name_norm + { + if trigger_table.get_column(&col_norm).is_some() { + *found = true; + return Ok(crate::translate::expr::WalkControl::SkipChildren); + } + } + } + Ok(crate::translate::expr::WalkControl::Continue) + } + // Check WITH clause + if let Some(ref with_clause) = select.with { + for cte in &with_clause.ctes { + walk_one_select_expressions( + &cte.select.body.select, + &trigger_table, + &trigger_table_name_norm, + &column_name_norm, + &mut found_qualified_ref, + )?; + if found_qualified_ref { + break; + } + } + } + // Check main SELECT body + if !found_qualified_ref { + match &select.body.select { + ast::OneSelect::Select { + columns, + from, + where_clause, + group_by, + window_clause, + .. + } => { + // Check columns + for col in columns { + if let ast::ResultColumn::Expr(expr, _) = col { + walk_expr(expr, &mut |e| { + check_qualified_ref_to_trigger_table( + e, + &trigger_table, + &trigger_table_name_norm, + &column_name_norm, + &mut found_qualified_ref, + ) + })?; + if found_qualified_ref { + break; + } + } + } + // Check FROM clause - use walk_from_clause_expressions but it uses check_column_ref + // which checks for all refs. We need to manually check for qualified refs only. + // Actually, walk_from_clause_expressions will set found_qualified_ref if it finds + // qualified refs via check_column_ref, so we can use it. + if !found_qualified_ref { + if let Some(ref from_clause) = from { + walk_from_clause_expressions( + from_clause, + &trigger_table, + &trigger_table_name_norm, + &column_name_norm, + &mut found_qualified_ref, + )?; + } + } + // Check WHERE clause + if !found_qualified_ref { + if let Some(ref where_expr) = where_clause { + walk_expr(where_expr, &mut |e| { + check_qualified_ref_to_trigger_table( + e, + &trigger_table, + &trigger_table_name_norm, + &column_name_norm, + &mut found_qualified_ref, + ) + })?; + } + } + // Check GROUP BY + if !found_qualified_ref { + if let Some(ref group_by) = group_by { + for expr in &group_by.exprs { + walk_expr(expr, &mut |e| { + check_qualified_ref_to_trigger_table( + e, + &trigger_table, + &trigger_table_name_norm, + &column_name_norm, + &mut found_qualified_ref, + ) + })?; + if found_qualified_ref { + break; + } + } + if !found_qualified_ref { + if let Some(ref having_expr) = + group_by.having + { + walk_expr(having_expr, &mut |e| { + check_qualified_ref_to_trigger_table( + e, &trigger_table, &trigger_table_name_norm, &column_name_norm, &mut found_qualified_ref + ) + })?; + } + } + } + } + // Check WINDOW clause + if !found_qualified_ref { + for window_def in window_clause { + walk_window_expressions( + &window_def.window, + &trigger_table, + &trigger_table_name_norm, + &column_name_norm, + &mut found_qualified_ref, + )?; + if found_qualified_ref { + break; + } + } + } + } + ast::OneSelect::Values(values) => { + for row in values { + for expr in row { + walk_expr(expr, &mut |e| { + check_qualified_ref_to_trigger_table( + e, + &trigger_table, + &trigger_table_name_norm, + &column_name_norm, + &mut found_qualified_ref, + ) + })?; + if found_qualified_ref { + break; + } + } + if found_qualified_ref { + break; + } + } + } + } + } + // Check compound SELECTs + if !found_qualified_ref { + for compound in &select.body.compounds { + match &compound.select { + ast::OneSelect::Select { + columns, + from, + where_clause, + group_by, + window_clause, + .. + } => { + for col in columns { + if let ast::ResultColumn::Expr(expr, _) = + col + { + walk_expr(expr, &mut |e| { + check_qualified_ref_to_trigger_table( + e, + &trigger_table, + &trigger_table_name_norm, + &column_name_norm, + &mut found_qualified_ref, + ) + })?; + if found_qualified_ref { + break; + } + } + } + if !found_qualified_ref { + if let Some(ref from_clause) = from { + walk_from_clause_expressions( + from_clause, + &trigger_table, + &trigger_table_name_norm, + &column_name_norm, + &mut found_qualified_ref, + )?; + } + } + if !found_qualified_ref { + if let Some(ref where_expr) = where_clause { + walk_expr(where_expr, &mut |e| { + check_qualified_ref_to_trigger_table( + e, + &trigger_table, + &trigger_table_name_norm, + &column_name_norm, + &mut found_qualified_ref, + ) + })?; + } + } + if !found_qualified_ref { + if let Some(ref group_by) = group_by { + for expr in &group_by.exprs { + walk_expr(expr, &mut |e| { + check_qualified_ref_to_trigger_table( + e, &trigger_table, &trigger_table_name_norm, &column_name_norm, &mut found_qualified_ref + ) + })?; + if found_qualified_ref { + break; + } + } + if !found_qualified_ref { + if let Some(ref having_expr) = + group_by.having + { + walk_expr( + having_expr, + &mut |e| { + check_qualified_ref_to_trigger_table( + e, &trigger_table, &trigger_table_name_norm, &column_name_norm, &mut found_qualified_ref + ) + }, + )?; + } + } + } + } + if !found_qualified_ref { + for window_def in window_clause { + walk_window_expressions( + &window_def.window, + &trigger_table, + &trigger_table_name_norm, + &column_name_norm, + &mut found_qualified_ref, + )?; + if found_qualified_ref { + break; + } + } + } + } + ast::OneSelect::Values(values) => { + for row in values { + for expr in row { + walk_expr(expr, &mut |e| { + check_qualified_ref_to_trigger_table( + e, + &trigger_table, + &trigger_table_name_norm, + &column_name_norm, + &mut found_qualified_ref, + ) + })?; + if found_qualified_ref { + break; + } + } + if found_qualified_ref { + break; + } + } + } + } + if found_qualified_ref { + break; + } + } + } + // Check ORDER BY + if !found_qualified_ref { + for sorted_col in &select.order_by { + walk_expr(&sorted_col.expr, &mut |e| { + check_qualified_ref_to_trigger_table( + e, + &trigger_table, + &trigger_table_name_norm, + &column_name_norm, + &mut found_qualified_ref, + ) + })?; + if found_qualified_ref { + break; + } + } + } + // Check LIMIT + if !found_qualified_ref { + if let Some(ref limit) = select.limit { + walk_expr(&limit.expr, &mut |e| { + check_qualified_ref_to_trigger_table( + e, + &trigger_table, + &trigger_table_name_norm, + &column_name_norm, + &mut found_qualified_ref, + ) + })?; + if !found_qualified_ref { + if let Some(ref offset) = limit.offset { + walk_expr(offset, &mut |e| { + check_qualified_ref_to_trigger_table( + e, + &trigger_table, + &trigger_table_name_norm, + &column_name_norm, + &mut found_qualified_ref, + ) + })?; + } + } + } + } + } + ast::TriggerCmd::Delete { where_clause, .. } => { + if let Some(ref where_expr) = where_clause { + walk_expr( + where_expr, + &mut |e: &ast::Expr| -> Result< + crate::translate::expr::WalkControl, + > { + if let ast::Expr::Qualified(ns, col) + | ast::Expr::DoublyQualified(_, ns, col) = e + { + let ns_norm = normalize_ident(ns.as_str()); + let col_norm = normalize_ident(col.as_str()); + if !ns_norm.eq_ignore_ascii_case("new") + && !ns_norm.eq_ignore_ascii_case("old") + && ns_norm == trigger_table_name_norm + && col_norm == column_name_norm + { + if trigger_table + .get_column(&col_norm) + .is_some() + { + found_qualified_ref = true; + return Ok(crate::translate::expr::WalkControl::SkipChildren); + } + } + } + Ok(crate::translate::expr::WalkControl::Continue) + }, + )?; + } + } + } + if found_qualified_ref { + break; + } + } + if found_qualified_ref { + return Err(LimboError::ParseError(format!( + "error in trigger {}: no such column: {}", + trigger.name, from + ))); + } + } // Check if trigger references the column being renamed // This includes: // 1. References from the trigger's owning table (NEW.x, OLD.x, unqualified x) // 2. References in INSERT column lists targeting the table being renamed // 3. References in UPDATE SET column lists targeting the table being renamed + let mut needs_rewrite = false; + if trigger_table_name_norm == target_table_name_norm { // Trigger is on the table being renamed - check for references - if let Some(trigger_table) = - resolver.schema.get_btree_table(&trigger_table_name_norm) - { - if let Ok(refs) = - trigger_references_column(trigger, &trigger_table, from) - { - needs_rewrite = refs; - } - } + 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}" + )) + })?; + + needs_rewrite = trigger_references_column(trigger, &trigger_table, from)?; } // Also check if trigger references the column in INSERT/UPDATE targeting other tables @@ -929,14 +1384,17 @@ fn translate_rename_virtual_table( Here are some helpers related to that: */ /// Check if an expression references a specific column from a trigger's owning table. -/// Checks for NEW.column, OLD.column, and unqualified column references. +/// Checks for NEW.column, OLD.column, unqualified column references, and qualified +/// references to the trigger table (e.g., t.x in a trigger on table t). /// Returns true if the column is found, false otherwise. fn expr_references_trigger_column( expr: &ast::Expr, table: &crate::schema::BTreeTable, + trigger_table_name: &str, column_name_norm: &str, ) -> Result { use crate::translate::expr::walk_expr; + let trigger_table_name_norm = normalize_ident(trigger_table_name); let mut found = false; walk_expr(expr, &mut |e: &ast::Expr| -> Result< crate::translate::expr::WalkControl, @@ -953,6 +1411,13 @@ fn expr_references_trigger_column( found = true; return Ok(crate::translate::expr::WalkControl::SkipChildren); } + // Check qualified reference to trigger table (e.g., t.x) + if ns_norm == trigger_table_name_norm && col_norm == *column_name_norm { + if table.get_column(&col_norm).is_some() { + found = true; + return Ok(crate::translate::expr::WalkControl::SkipChildren); + } + } } ast::Expr::Id(col) => { // Unqualified column reference - check if it matches @@ -984,7 +1449,14 @@ fn trigger_references_column( // Check when_clause if let Some(ref when_expr) = trigger.when_clause { - found = expr_references_trigger_column(when_expr, table, &column_name_norm)?; + // Get trigger table name for checking qualified references + let trigger_table_name = normalize_ident(&trigger.table_name); + found = expr_references_trigger_column( + when_expr, + table, + &trigger_table_name, + &column_name_norm, + )?; } if found { @@ -1003,39 +1475,61 @@ fn trigger_references_column( sets, where_clause, .. } => { // Check SET expressions (not column names in SET clause) + let trigger_table_name = normalize_ident(&trigger.table_name); 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) - })?; - if found { + if expr_references_trigger_column( + &set.expr, + table, + &trigger_table_name, + &column_name_norm, + )? { + found = true; break; } } // 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) - })?; + found = expr_references_trigger_column( + where_expr, + table, + &trigger_table_name, + &column_name_norm, + )?; } } } ast::TriggerCmd::Insert { select, .. } => { // Check SELECT/VALUES expressions (not column names in INSERT clause) - walk_select_expressions(select, table, &column_name_norm, &mut found)?; + let trigger_table_name = normalize_ident(&trigger.table_name); + walk_select_expressions( + select, + table, + &trigger_table_name, + &column_name_norm, + &mut found, + )?; } ast::TriggerCmd::Delete { 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) - })?; + let trigger_table_name = normalize_ident(&trigger.table_name); + found = expr_references_trigger_column( + where_expr, + table, + &trigger_table_name, + &column_name_norm, + )?; } } ast::TriggerCmd::Select(select) => { - walk_select_expressions(select, table, &column_name_norm, &mut found)?; + let trigger_table_name = normalize_ident(&trigger.table_name); + walk_select_expressions( + select, + table, + &trigger_table_name, + &column_name_norm, + &mut found, + )?; } } if found { @@ -1048,12 +1542,15 @@ fn trigger_references_column( /// Helper to check if an expression references a column. /// Used as a callback within walk_expr, so it mutates a found flag. +/// Also checks for qualified references to the trigger table (e.g., t.x). fn check_column_ref( e: &ast::Expr, table: &crate::schema::BTreeTable, + trigger_table_name: &str, column_name_norm: &str, found: &mut bool, ) -> Result<()> { + let trigger_table_name_norm = normalize_ident(trigger_table_name); match e { ast::Expr::Qualified(ns, col) | ast::Expr::DoublyQualified(_, ns, col) => { let ns_norm = normalize_ident(ns.as_str()); @@ -1065,6 +1562,12 @@ fn check_column_ref( { *found = true; } + // Check qualified reference to trigger table (e.g., t.x) + else if ns_norm == trigger_table_name_norm && col_norm == *column_name_norm { + if table.get_column(&col_norm).is_some() { + *found = true; + } + } } ast::Expr::Id(col) => { // Unqualified column reference - check if it matches @@ -1137,13 +1640,20 @@ fn trigger_references_column_in_other_tables( fn walk_select_expressions( select: &ast::Select, table: &crate::schema::BTreeTable, + trigger_table_name: &str, column_name_norm: &str, found: &mut bool, ) -> Result<()> { // Check WITH clause (CTEs) if let Some(ref with_clause) = select.with { for cte in &with_clause.ctes { - walk_select_expressions(&cte.select, table, column_name_norm, found)?; + walk_select_expressions( + &cte.select, + table, + trigger_table_name, + column_name_norm, + found, + )?; if *found { return Ok(()); } @@ -1151,14 +1661,26 @@ fn walk_select_expressions( } // Check main SELECT body - walk_one_select_expressions(&select.body.select, table, column_name_norm, found)?; + walk_one_select_expressions( + &select.body.select, + table, + trigger_table_name, + column_name_norm, + found, + )?; if *found { return Ok(()); } // Check compound SELECTs (UNION, EXCEPT, INTERSECT) for compound in &select.body.compounds { - walk_one_select_expressions(&compound.select, table, column_name_norm, found)?; + walk_one_select_expressions( + &compound.select, + table, + trigger_table_name, + column_name_norm, + found, + )?; if *found { return Ok(()); } @@ -1169,7 +1691,7 @@ fn walk_select_expressions( walk_expr( &sorted_col.expr, &mut |e: &ast::Expr| -> Result { - check_column_ref(e, table, column_name_norm, found)?; + check_column_ref(e, table, trigger_table_name, column_name_norm, found)?; Ok(WalkControl::Continue) }, )?; @@ -1181,7 +1703,7 @@ fn walk_select_expressions( // Check LIMIT if let Some(ref limit) = select.limit { walk_expr(&limit.expr, &mut |e: &ast::Expr| -> Result { - check_column_ref(e, table, column_name_norm, found)?; + check_column_ref(e, table, trigger_table_name, column_name_norm, found)?; Ok(WalkControl::Continue) })?; if *found { @@ -1189,7 +1711,7 @@ fn walk_select_expressions( } if let Some(ref offset) = limit.offset { walk_expr(offset, &mut |e: &ast::Expr| -> Result { - check_column_ref(e, table, column_name_norm, found)?; + check_column_ref(e, table, trigger_table_name, column_name_norm, found)?; Ok(WalkControl::Continue) })?; if *found { @@ -1205,6 +1727,7 @@ fn walk_select_expressions( fn walk_one_select_expressions( one_select: &ast::OneSelect, table: &crate::schema::BTreeTable, + trigger_table_name: &str, column_name_norm: &str, found: &mut bool, ) -> Result<()> { @@ -1221,7 +1744,7 @@ fn walk_one_select_expressions( for col in columns { if let ast::ResultColumn::Expr(expr, _) = col { walk_expr(expr, &mut |e: &ast::Expr| -> Result { - check_column_ref(e, table, column_name_norm, found)?; + check_column_ref(e, table, trigger_table_name, column_name_norm, found)?; Ok(WalkControl::Continue) })?; if *found { @@ -1232,7 +1755,13 @@ fn walk_one_select_expressions( // Check FROM clause and JOIN conditions if let Some(ref from_clause) = from { - walk_from_clause_expressions(from_clause, table, column_name_norm, found)?; + walk_from_clause_expressions( + from_clause, + table, + trigger_table_name, + column_name_norm, + found, + )?; if *found { return Ok(()); } @@ -1241,7 +1770,7 @@ fn walk_one_select_expressions( // 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, found)?; + check_column_ref(e, table, trigger_table_name, column_name_norm, found)?; Ok(WalkControl::Continue) })?; if *found { @@ -1253,7 +1782,7 @@ fn walk_one_select_expressions( if let Some(ref group_by) = group_by { for expr in &group_by.exprs { walk_expr(expr, &mut |e: &ast::Expr| -> Result { - check_column_ref(e, table, column_name_norm, found)?; + check_column_ref(e, table, trigger_table_name, column_name_norm, found)?; Ok(WalkControl::Continue) })?; if *found { @@ -1262,7 +1791,7 @@ fn walk_one_select_expressions( } if let Some(ref having_expr) = group_by.having { walk_expr(having_expr, &mut |e: &ast::Expr| -> Result { - check_column_ref(e, table, column_name_norm, found)?; + check_column_ref(e, table, trigger_table_name, column_name_norm, found)?; Ok(WalkControl::Continue) })?; if *found { @@ -1273,7 +1802,13 @@ fn walk_one_select_expressions( // Check WINDOW clause for window_def in window_clause { - walk_window_expressions(&window_def.window, table, column_name_norm, found)?; + walk_window_expressions( + &window_def.window, + table, + trigger_table_name, + column_name_norm, + found, + )?; if *found { return Ok(()); } @@ -1283,7 +1818,7 @@ fn walk_one_select_expressions( for row in values { for expr in row { walk_expr(expr, &mut |e: &ast::Expr| -> Result { - check_column_ref(e, table, column_name_norm, found)?; + check_column_ref(e, table, trigger_table_name, column_name_norm, found)?; Ok(WalkControl::Continue) })?; if *found { @@ -1300,18 +1835,31 @@ fn walk_one_select_expressions( fn walk_from_clause_expressions( from_clause: &ast::FromClause, table: &crate::schema::BTreeTable, + trigger_table_name: &str, column_name_norm: &str, found: &mut bool, ) -> Result<()> { // Check main table (could be a subquery) - walk_select_table_expressions(&from_clause.select, table, column_name_norm, found)?; + walk_select_table_expressions( + &from_clause.select, + table, + trigger_table_name, + column_name_norm, + found, + )?; if *found { return Ok(()); } // Check JOIN conditions for join in &from_clause.joins { - walk_select_table_expressions(&join.table, table, column_name_norm, found)?; + walk_select_table_expressions( + &join.table, + table, + trigger_table_name, + column_name_norm, + found, + )?; if *found { return Ok(()); } @@ -1319,7 +1867,7 @@ fn walk_from_clause_expressions( match constraint { ast::JoinConstraint::On(expr) => { walk_expr(expr, &mut |e: &ast::Expr| -> Result { - check_column_ref(e, table, column_name_norm, found)?; + check_column_ref(e, table, trigger_table_name, column_name_norm, found)?; Ok(WalkControl::Continue) })?; if *found { @@ -1339,20 +1887,27 @@ fn walk_from_clause_expressions( fn walk_select_table_expressions( select_table: &ast::SelectTable, table: &crate::schema::BTreeTable, + trigger_table_name: &str, column_name_norm: &str, found: &mut bool, ) -> Result<()> { match select_table { ast::SelectTable::Select(select, _) => { - walk_select_expressions(select, table, column_name_norm, found)?; + walk_select_expressions(select, table, trigger_table_name, column_name_norm, found)?; } ast::SelectTable::Sub(from_clause, _) => { - walk_from_clause_expressions(from_clause, table, column_name_norm, found)?; + walk_from_clause_expressions( + from_clause, + table, + trigger_table_name, + column_name_norm, + found, + )?; } ast::SelectTable::TableCall(_, args, _) => { for arg in args { walk_expr(arg, &mut |e: &ast::Expr| -> Result { - check_column_ref(e, table, column_name_norm, found)?; + check_column_ref(e, table, trigger_table_name, column_name_norm, found)?; Ok(WalkControl::Continue) })?; if *found { @@ -1371,13 +1926,14 @@ fn walk_select_table_expressions( fn walk_window_expressions( window: &ast::Window, table: &crate::schema::BTreeTable, + trigger_table_name: &str, column_name_norm: &str, found: &mut bool, ) -> Result<()> { // Check PARTITION BY expressions for expr in &window.partition_by { walk_expr(expr, &mut |e: &ast::Expr| -> Result { - check_column_ref(e, table, column_name_norm, found)?; + check_column_ref(e, table, trigger_table_name, column_name_norm, found)?; Ok(WalkControl::Continue) })?; if *found { @@ -1390,7 +1946,7 @@ fn walk_window_expressions( walk_expr( &sorted_col.expr, &mut |e: &ast::Expr| -> Result { - check_column_ref(e, table, column_name_norm, found)?; + check_column_ref(e, table, trigger_table_name, column_name_norm, found)?; Ok(WalkControl::Continue) }, )?; @@ -1441,7 +1997,8 @@ fn rewrite_trigger_sql_for_column_rename( let new_col_norm = normalize_ident(new_column_name); // Get the trigger's owning table to check unqualified column references - let trigger_table_name = normalize_ident(tbl_name.name.as_str()); + let trigger_table_name_raw = tbl_name.name.as_str(); + let trigger_table_name = normalize_ident(trigger_table_name_raw); let trigger_table = resolver .schema .get_btree_table(&trigger_table_name) @@ -1490,7 +2047,7 @@ fn rewrite_trigger_sql_for_column_rename( cmd, &trigger_table, &target_table, - &trigger_table_name, + trigger_table_name_raw, &target_table_name, &old_col_norm, &new_col_norm, @@ -1559,6 +2116,7 @@ fn rewrite_expr_for_column_rename( rewrite_expr_column_ref_with_context( e, trigger_table, + trigger_table_name, old_col_norm, new_col_norm, is_renaming_trigger_table, @@ -2060,12 +2618,22 @@ fn rewrite_window_for_column_rename( /// Rewrite a single expression's column reference /// -/// `context_table` is used for UPDATE/DELETE WHERE clauses where unqualified column -/// references refer to the UPDATE/DELETE target table. If `None`, unqualified references -/// refer to the trigger's owning table. +/// Handles column references in trigger expressions: +/// - NEW.column and OLD.column: Always refer to the trigger's owning table +/// - Qualified references (e.g., u.x): Refer to the specified table +/// - Unqualified references (e.g., x): Resolution order: +/// 1. If `context_table` is provided (UPDATE/DELETE WHERE clauses), check the context table first +/// 2. Otherwise, check the trigger's owning table +/// +/// This matches SQLite's column resolution order where unqualified columns in UPDATE/DELETE +/// WHERE clauses refer to the target table, not the trigger's owning table. +/// +/// `context_table`: Optional tuple of (table, normalized_name, is_renaming) for UPDATE/DELETE +/// target tables. If `None`, unqualified references refer to the trigger's owning table. fn rewrite_expr_column_ref_with_context( e: &mut ast::Expr, trigger_table: &crate::schema::BTreeTable, + trigger_table_name: &str, old_col_norm: &str, new_col_norm: &str, is_renaming_trigger_table: bool, @@ -2085,10 +2653,21 @@ fn rewrite_expr_column_ref_with_context( *col = ast::Name::from_string(new_col_norm); } } else if col_norm == *old_col_norm { - // This is a qualified column reference like u.x - // Check if it refers to the context table (UPDATE/DELETE target) + // This is a qualified column reference like u.x or t.x + // Check if it refers to the context table (UPDATE/DELETE target) or trigger table if let Some((_, ctx_name_norm, is_renaming_ctx)) = context_table { if ns_norm == *ctx_name_norm && is_renaming_ctx { + // Qualified reference to context table (e.g., u.x where u is UPDATE target) + *col = ast::Name::from_string(new_col_norm); + } + } + // Also check if it's a qualified reference to the trigger's owning table + // (e.g., t.x in a trigger on table t) + if is_renaming_trigger_table { + let trigger_table_name_norm = normalize_ident(trigger_table_name); + if ns_norm == trigger_table_name_norm + && trigger_table.get_column(&col_norm).is_some() + { *col = ast::Name::from_string(new_col_norm); } } @@ -2135,6 +2714,7 @@ fn rewrite_expr_column_ref( rewrite_expr_column_ref_with_context( e, trigger_table, + trigger_table_name, old_col_norm, new_col_norm, is_renaming_trigger_table, diff --git a/tests/integration/trigger.rs b/tests/integration/trigger.rs index 5cd3a5b40..1898de5da 100644 --- a/tests/integration/trigger.rs +++ b/tests/integration/trigger.rs @@ -1917,3 +1917,39 @@ fn test_alter_table_drop_column_allows_when_update_set_targets_owning_table() { "INSERT should fail because trigger references dropped column" ); } + +#[test] +fn test_alter_table_rename_column_qualified_reference_to_trigger_table() { + let db = TempDatabase::new_empty(); + let conn = db.connect_limbo(); + + // Create two tables + conn.execute("CREATE TABLE t (x INTEGER, y INTEGER)") + .unwrap(); + conn.execute("CREATE TABLE u (z INTEGER)").unwrap(); + + // Note: SQLite doesn't support qualified references to the trigger's owning table (t.x). + // SQLite fails the RENAME COLUMN operation with "error in trigger tu: no such column: t.x". + // We match SQLite's behavior - the rename should fail. + + // Create trigger on t that uses qualified reference t.x (invalid in SQLite) + conn.execute( + "CREATE TRIGGER tu AFTER UPDATE ON t BEGIN + UPDATE u SET z = t.x WHERE z = 1; + END", + ) + .unwrap(); + + // Rename column x to x_new in table t should fail (SQLite fails with "no such column: t.x") + let result = conn.execute("ALTER TABLE t RENAME COLUMN x TO x_new"); + assert!( + result.is_err(), + "RENAME COLUMN should fail when trigger uses qualified reference to trigger table" + ); + + 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 or column: {error_msg}", + ); +} From fddcea788bd81ed7613efe1698c591e032219752 Mon Sep 17 00:00:00 2001 From: Jussi Saurio Date: Wed, 19 Nov 2025 14:20:18 +0200 Subject: [PATCH 6/7] refactor --- core/translate/alter.rs | 712 ++++++++++++++-------------------------- 1 file changed, 254 insertions(+), 458 deletions(-) diff --git a/core/translate/alter.rs b/core/translate/alter.rs index 5c7302f29..f58b4c5aa 100644 --- a/core/translate/alter.rs +++ b/core/translate/alter.rs @@ -651,450 +651,12 @@ pub fn translate_alter_table( // Check trigger commands for qualified references to trigger table (e.g., t.x) // SQLite fails RENAME COLUMN if triggers contain qualified references like t.x - // We check all expressions in trigger commands for qualified references - use crate::translate::expr::walk_expr; - let mut found_qualified_ref = false; - for cmd in &trigger.commands { - match cmd { - ast::TriggerCmd::Update { - sets, where_clause, .. - } => { - for set in sets { - walk_expr( - &set.expr, - &mut |e: &ast::Expr| -> Result< - crate::translate::expr::WalkControl, - > { - if let ast::Expr::Qualified(ns, col) - | ast::Expr::DoublyQualified(_, ns, col) = e - { - let ns_norm = normalize_ident(ns.as_str()); - let col_norm = normalize_ident(col.as_str()); - // Check if it's a qualified reference to trigger table (not NEW/OLD) - if !ns_norm.eq_ignore_ascii_case("new") - && !ns_norm.eq_ignore_ascii_case("old") - && ns_norm == trigger_table_name_norm - && col_norm == column_name_norm - { - if trigger_table - .get_column(&col_norm) - .is_some() - { - found_qualified_ref = true; - return Ok(crate::translate::expr::WalkControl::SkipChildren); - } - } - } - Ok(crate::translate::expr::WalkControl::Continue) - }, - )?; - if found_qualified_ref { - break; - } - } - if !found_qualified_ref { - if let Some(ref where_expr) = where_clause { - walk_expr(where_expr, &mut |e: &ast::Expr| -> Result { - if let ast::Expr::Qualified(ns, col) | ast::Expr::DoublyQualified(_, ns, col) = e { - let ns_norm = normalize_ident(ns.as_str()); - let col_norm = normalize_ident(col.as_str()); - if !ns_norm.eq_ignore_ascii_case("new") - && !ns_norm.eq_ignore_ascii_case("old") - && ns_norm == trigger_table_name_norm - && col_norm == column_name_norm - { - if trigger_table.get_column(&col_norm).is_some() { - found_qualified_ref = true; - return Ok(crate::translate::expr::WalkControl::SkipChildren); - } - } - } - Ok(crate::translate::expr::WalkControl::Continue) - })?; - } - } - } - ast::TriggerCmd::Insert { select, .. } - | ast::TriggerCmd::Select(select) => { - // Walk through all expressions in SELECT and check ONLY for qualified refs to trigger table - // We need to check specifically for qualified refs (t.x), not NEW.x, OLD.x, or unqualified x - // Use walk_select_expressions but check only for qualified refs manually - use crate::translate::expr::walk_expr; - // Helper function to check for qualified refs to trigger table - fn check_qualified_ref_to_trigger_table( - e: &ast::Expr, - trigger_table: &crate::schema::BTreeTable, - trigger_table_name_norm: &str, - column_name_norm: &str, - found: &mut bool, - ) -> Result - { - if let ast::Expr::Qualified(ns, col) - | ast::Expr::DoublyQualified(_, ns, col) = e - { - let ns_norm = normalize_ident(ns.as_str()); - let col_norm = normalize_ident(col.as_str()); - // Only check for qualified refs to trigger table (not NEW/OLD) - if !ns_norm.eq_ignore_ascii_case("new") - && !ns_norm.eq_ignore_ascii_case("old") - && ns_norm == trigger_table_name_norm - && col_norm == column_name_norm - { - if trigger_table.get_column(&col_norm).is_some() { - *found = true; - return Ok(crate::translate::expr::WalkControl::SkipChildren); - } - } - } - Ok(crate::translate::expr::WalkControl::Continue) - } - // Check WITH clause - if let Some(ref with_clause) = select.with { - for cte in &with_clause.ctes { - walk_one_select_expressions( - &cte.select.body.select, - &trigger_table, - &trigger_table_name_norm, - &column_name_norm, - &mut found_qualified_ref, - )?; - if found_qualified_ref { - break; - } - } - } - // Check main SELECT body - if !found_qualified_ref { - match &select.body.select { - ast::OneSelect::Select { - columns, - from, - where_clause, - group_by, - window_clause, - .. - } => { - // Check columns - for col in columns { - if let ast::ResultColumn::Expr(expr, _) = col { - walk_expr(expr, &mut |e| { - check_qualified_ref_to_trigger_table( - e, - &trigger_table, - &trigger_table_name_norm, - &column_name_norm, - &mut found_qualified_ref, - ) - })?; - if found_qualified_ref { - break; - } - } - } - // Check FROM clause - use walk_from_clause_expressions but it uses check_column_ref - // which checks for all refs. We need to manually check for qualified refs only. - // Actually, walk_from_clause_expressions will set found_qualified_ref if it finds - // qualified refs via check_column_ref, so we can use it. - if !found_qualified_ref { - if let Some(ref from_clause) = from { - walk_from_clause_expressions( - from_clause, - &trigger_table, - &trigger_table_name_norm, - &column_name_norm, - &mut found_qualified_ref, - )?; - } - } - // Check WHERE clause - if !found_qualified_ref { - if let Some(ref where_expr) = where_clause { - walk_expr(where_expr, &mut |e| { - check_qualified_ref_to_trigger_table( - e, - &trigger_table, - &trigger_table_name_norm, - &column_name_norm, - &mut found_qualified_ref, - ) - })?; - } - } - // Check GROUP BY - if !found_qualified_ref { - if let Some(ref group_by) = group_by { - for expr in &group_by.exprs { - walk_expr(expr, &mut |e| { - check_qualified_ref_to_trigger_table( - e, - &trigger_table, - &trigger_table_name_norm, - &column_name_norm, - &mut found_qualified_ref, - ) - })?; - if found_qualified_ref { - break; - } - } - if !found_qualified_ref { - if let Some(ref having_expr) = - group_by.having - { - walk_expr(having_expr, &mut |e| { - check_qualified_ref_to_trigger_table( - e, &trigger_table, &trigger_table_name_norm, &column_name_norm, &mut found_qualified_ref - ) - })?; - } - } - } - } - // Check WINDOW clause - if !found_qualified_ref { - for window_def in window_clause { - walk_window_expressions( - &window_def.window, - &trigger_table, - &trigger_table_name_norm, - &column_name_norm, - &mut found_qualified_ref, - )?; - if found_qualified_ref { - break; - } - } - } - } - ast::OneSelect::Values(values) => { - for row in values { - for expr in row { - walk_expr(expr, &mut |e| { - check_qualified_ref_to_trigger_table( - e, - &trigger_table, - &trigger_table_name_norm, - &column_name_norm, - &mut found_qualified_ref, - ) - })?; - if found_qualified_ref { - break; - } - } - if found_qualified_ref { - break; - } - } - } - } - } - // Check compound SELECTs - if !found_qualified_ref { - for compound in &select.body.compounds { - match &compound.select { - ast::OneSelect::Select { - columns, - from, - where_clause, - group_by, - window_clause, - .. - } => { - for col in columns { - if let ast::ResultColumn::Expr(expr, _) = - col - { - walk_expr(expr, &mut |e| { - check_qualified_ref_to_trigger_table( - e, - &trigger_table, - &trigger_table_name_norm, - &column_name_norm, - &mut found_qualified_ref, - ) - })?; - if found_qualified_ref { - break; - } - } - } - if !found_qualified_ref { - if let Some(ref from_clause) = from { - walk_from_clause_expressions( - from_clause, - &trigger_table, - &trigger_table_name_norm, - &column_name_norm, - &mut found_qualified_ref, - )?; - } - } - if !found_qualified_ref { - if let Some(ref where_expr) = where_clause { - walk_expr(where_expr, &mut |e| { - check_qualified_ref_to_trigger_table( - e, - &trigger_table, - &trigger_table_name_norm, - &column_name_norm, - &mut found_qualified_ref, - ) - })?; - } - } - if !found_qualified_ref { - if let Some(ref group_by) = group_by { - for expr in &group_by.exprs { - walk_expr(expr, &mut |e| { - check_qualified_ref_to_trigger_table( - e, &trigger_table, &trigger_table_name_norm, &column_name_norm, &mut found_qualified_ref - ) - })?; - if found_qualified_ref { - break; - } - } - if !found_qualified_ref { - if let Some(ref having_expr) = - group_by.having - { - walk_expr( - having_expr, - &mut |e| { - check_qualified_ref_to_trigger_table( - e, &trigger_table, &trigger_table_name_norm, &column_name_norm, &mut found_qualified_ref - ) - }, - )?; - } - } - } - } - if !found_qualified_ref { - for window_def in window_clause { - walk_window_expressions( - &window_def.window, - &trigger_table, - &trigger_table_name_norm, - &column_name_norm, - &mut found_qualified_ref, - )?; - if found_qualified_ref { - break; - } - } - } - } - ast::OneSelect::Values(values) => { - for row in values { - for expr in row { - walk_expr(expr, &mut |e| { - check_qualified_ref_to_trigger_table( - e, - &trigger_table, - &trigger_table_name_norm, - &column_name_norm, - &mut found_qualified_ref, - ) - })?; - if found_qualified_ref { - break; - } - } - if found_qualified_ref { - break; - } - } - } - } - if found_qualified_ref { - break; - } - } - } - // Check ORDER BY - if !found_qualified_ref { - for sorted_col in &select.order_by { - walk_expr(&sorted_col.expr, &mut |e| { - check_qualified_ref_to_trigger_table( - e, - &trigger_table, - &trigger_table_name_norm, - &column_name_norm, - &mut found_qualified_ref, - ) - })?; - if found_qualified_ref { - break; - } - } - } - // Check LIMIT - if !found_qualified_ref { - if let Some(ref limit) = select.limit { - walk_expr(&limit.expr, &mut |e| { - check_qualified_ref_to_trigger_table( - e, - &trigger_table, - &trigger_table_name_norm, - &column_name_norm, - &mut found_qualified_ref, - ) - })?; - if !found_qualified_ref { - if let Some(ref offset) = limit.offset { - walk_expr(offset, &mut |e| { - check_qualified_ref_to_trigger_table( - e, - &trigger_table, - &trigger_table_name_norm, - &column_name_norm, - &mut found_qualified_ref, - ) - })?; - } - } - } - } - } - ast::TriggerCmd::Delete { where_clause, .. } => { - if let Some(ref where_expr) = where_clause { - walk_expr( - where_expr, - &mut |e: &ast::Expr| -> Result< - crate::translate::expr::WalkControl, - > { - if let ast::Expr::Qualified(ns, col) - | ast::Expr::DoublyQualified(_, ns, col) = e - { - let ns_norm = normalize_ident(ns.as_str()); - let col_norm = normalize_ident(col.as_str()); - if !ns_norm.eq_ignore_ascii_case("new") - && !ns_norm.eq_ignore_ascii_case("old") - && ns_norm == trigger_table_name_norm - && col_norm == column_name_norm - { - if trigger_table - .get_column(&col_norm) - .is_some() - { - found_qualified_ref = true; - return Ok(crate::translate::expr::WalkControl::SkipChildren); - } - } - } - Ok(crate::translate::expr::WalkControl::Continue) - }, - )?; - } - } - } - if found_qualified_ref { - break; - } - } - if found_qualified_ref { + if check_trigger_has_qualified_ref_to_column( + trigger, + &trigger_table, + &trigger_table_name_norm, + &column_name_norm, + )? { return Err(LimboError::ParseError(format!( "error in trigger {}: no such column: {}", trigger.name, from @@ -1383,6 +945,243 @@ fn translate_rename_virtual_table( /* Triggers must be rewritten when a column is renamed, and DROP COLUMN on table T must be forbidden if any trigger on T references the column. Here are some helpers related to that: */ +/// Check if a trigger contains qualified references to a specific column in its owning table. +/// This is used to detect cases like `t.x` in a trigger on table `t`, which SQLite fails on RENAME COLUMN. +/// Only checks for qualified table references (e.g., t.x), not NEW.x, OLD.x, or unqualified x. +fn check_trigger_has_qualified_ref_to_column( + trigger: &crate::schema::Trigger, + trigger_table: &crate::schema::BTreeTable, + trigger_table_name_norm: &str, + column_name_norm: &str, +) -> Result { + use crate::translate::expr::walk_expr; + use std::cell::Cell; + + let found = Cell::new(false); + + // Helper callback to check for qualified references to trigger table + let mut check_qualified_ref = |e: &ast::Expr| -> Result { + if let ast::Expr::Qualified(ns, col) | ast::Expr::DoublyQualified(_, ns, col) = e { + let ns_norm = normalize_ident(ns.as_str()); + let col_norm = normalize_ident(col.as_str()); + // Only check for qualified refs to trigger table (not NEW/OLD) + if !ns_norm.eq_ignore_ascii_case("new") + && !ns_norm.eq_ignore_ascii_case("old") + && ns_norm == trigger_table_name_norm + && col_norm == column_name_norm + && trigger_table.get_column(&col_norm).is_some() + { + found.set(true); + return Ok(crate::translate::expr::WalkControl::SkipChildren); + } + } + Ok(crate::translate::expr::WalkControl::Continue) + }; + + for cmd in &trigger.commands { + match cmd { + ast::TriggerCmd::Update { + sets, where_clause, .. + } => { + for set in sets { + walk_expr(&set.expr, &mut check_qualified_ref)?; + if found.get() { + return Ok(true); + } + } + if let Some(ref where_expr) = where_clause { + walk_expr(where_expr, &mut check_qualified_ref)?; + if found.get() { + return Ok(true); + } + } + } + ast::TriggerCmd::Insert { select, .. } | ast::TriggerCmd::Select(select) => { + // Walk through all expressions in SELECT checking for qualified refs + if let Some(ref with_clause) = select.with { + for cte in &with_clause.ctes { + if walk_one_select_expressions_for_qualified_ref( + &cte.select.body.select, + &mut check_qualified_ref, + )? { + return Ok(true); + } + } + } + if walk_one_select_expressions_for_qualified_ref( + &select.body.select, + &mut check_qualified_ref, + )? { + return Ok(true); + } + for compound in &select.body.compounds { + if walk_one_select_expressions_for_qualified_ref( + &compound.select, + &mut check_qualified_ref, + )? { + return Ok(true); + } + } + for sorted_col in &select.order_by { + walk_expr(&sorted_col.expr, &mut check_qualified_ref)?; + if found.get() { + return Ok(true); + } + } + if let Some(ref limit) = select.limit { + walk_expr(&limit.expr, &mut check_qualified_ref)?; + if found.get() { + return Ok(true); + } + if let Some(ref offset) = limit.offset { + walk_expr(offset, &mut check_qualified_ref)?; + if found.get() { + return Ok(true); + } + } + } + } + ast::TriggerCmd::Delete { where_clause, .. } => { + if let Some(ref where_expr) = where_clause { + walk_expr(where_expr, &mut check_qualified_ref)?; + if found.get() { + return Ok(true); + } + } + } + } + } + + Ok(false) +} + +/// Helper to walk OneSelect expressions checking for qualified references +/// Uses a callback that can mutate the found flag through a closure +fn walk_one_select_expressions_for_qualified_ref( + one_select: &ast::OneSelect, + check_fn: &mut F, +) -> Result +where + F: FnMut(&ast::Expr) -> Result, +{ + use crate::translate::expr::walk_expr; + + match one_select { + ast::OneSelect::Select { + columns, + from, + where_clause, + group_by, + window_clause, + .. + } => { + for col in columns { + if let ast::ResultColumn::Expr(expr, _) = col { + if matches!( + walk_expr(expr, check_fn)?, + crate::translate::expr::WalkControl::SkipChildren + ) { + return Ok(true); + } + } + } + if let Some(ref from_clause) = from { + // Walk FROM clause expressions manually using walk_expr + match from_clause.select.as_ref() { + ast::SelectTable::Select(select, _) => { + // Recursively check the subquery + if walk_one_select_expressions_for_qualified_ref( + &select.body.select, + check_fn, + )? { + return Ok(true); + } + } + ast::SelectTable::TableCall(_, args, _) => { + for arg in args { + if matches!( + walk_expr(arg, check_fn)?, + crate::translate::expr::WalkControl::SkipChildren + ) { + return Ok(true); + } + } + } + _ => {} + } + for join in &from_clause.joins { + if let Some(ast::JoinConstraint::On(ref expr)) = join.constraint { + let skip_children = matches!( + walk_expr(expr, check_fn)?, + crate::translate::expr::WalkControl::SkipChildren + ); + if skip_children { + return Ok(true); + } + } + } + } + if let Some(ref where_expr) = where_clause { + if matches!( + walk_expr(where_expr, check_fn)?, + crate::translate::expr::WalkControl::SkipChildren + ) { + return Ok(true); + } + } + if let Some(ref group_by) = group_by { + for expr in &group_by.exprs { + if matches!( + walk_expr(expr, check_fn)?, + crate::translate::expr::WalkControl::SkipChildren + ) { + return Ok(true); + } + } + if let Some(ref having_expr) = group_by.having { + if matches!( + walk_expr(having_expr, check_fn)?, + crate::translate::expr::WalkControl::SkipChildren + ) { + return Ok(true); + } + } + } + for window_def in window_clause { + for expr in &window_def.window.partition_by { + if matches!( + walk_expr(expr, check_fn)?, + crate::translate::expr::WalkControl::SkipChildren + ) { + return Ok(true); + } + } + for sorted_col in &window_def.window.order_by { + if matches!( + walk_expr(&sorted_col.expr, check_fn)?, + crate::translate::expr::WalkControl::SkipChildren + ) { + return Ok(true); + } + } + } + } + ast::OneSelect::Values(values) => { + for row in values { + for expr in row { + if matches!( + walk_expr(expr, check_fn)?, + crate::translate::expr::WalkControl::SkipChildren + ) { + return Ok(true); + } + } + } + } + } + Ok(false) +} + /// Check if an expression references a specific column from a trigger's owning table. /// Checks for NEW.column, OLD.column, unqualified column references, and qualified /// references to the trigger table (e.g., t.x in a trigger on table t). @@ -1412,11 +1211,12 @@ fn expr_references_trigger_column( return Ok(crate::translate::expr::WalkControl::SkipChildren); } // Check qualified reference to trigger table (e.g., t.x) - if ns_norm == trigger_table_name_norm && col_norm == *column_name_norm { - if table.get_column(&col_norm).is_some() { - found = true; - return Ok(crate::translate::expr::WalkControl::SkipChildren); - } + if ns_norm == trigger_table_name_norm + && col_norm == *column_name_norm + && table.get_column(&col_norm).is_some() + { + found = true; + return Ok(crate::translate::expr::WalkControl::SkipChildren); } } ast::Expr::Id(col) => { @@ -1556,18 +1356,14 @@ fn check_column_ref( 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")) + let new_or_old = ns_norm.eq_ignore_ascii_case("new") + || ns_norm.eq_ignore_ascii_case("old") && col_norm == *column_name_norm; + let qualified_ref = ns_norm == trigger_table_name_norm && col_norm == *column_name_norm - { + && table.get_column(&col_norm).is_some(); + if new_or_old || qualified_ref { *found = true; } - // Check qualified reference to trigger table (e.g., t.x) - else if ns_norm == trigger_table_name_norm && col_norm == *column_name_norm { - if table.get_column(&col_norm).is_some() { - *found = true; - } - } } ast::Expr::Id(col) => { // Unqualified column reference - check if it matches From 32063334f9473f78f907d7d2a99b2302a143d1b3 Mon Sep 17 00:00:00 2001 From: Jussi Saurio Date: Wed, 19 Nov 2025 14:28:05 +0200 Subject: [PATCH 7/7] fix operator precedence bug --- core/translate/alter.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/core/translate/alter.rs b/core/translate/alter.rs index f58b4c5aa..95aecc3d5 100644 --- a/core/translate/alter.rs +++ b/core/translate/alter.rs @@ -1356,8 +1356,9 @@ fn check_column_ref( let ns_norm = normalize_ident(ns.as_str()); let col_norm = normalize_ident(col.as_str()); - let new_or_old = ns_norm.eq_ignore_ascii_case("new") - || ns_norm.eq_ignore_ascii_case("old") && col_norm == *column_name_norm; + let new_or_old = (ns_norm.eq_ignore_ascii_case("new") + || ns_norm.eq_ignore_ascii_case("old")) + && col_norm == *column_name_norm; let qualified_ref = ns_norm == trigger_table_name_norm && col_norm == *column_name_norm && table.get_column(&col_norm).is_some();