diff --git a/docs/adr/0009-dsl-command-syntax-conventions.md b/docs/adr/0009-dsl-command-syntax-conventions.md index b443699..6e8518c 100644 --- a/docs/adr/0009-dsl-command-syntax-conventions.md +++ b/docs/adr/0009-dsl-command-syntax-conventions.md @@ -66,7 +66,16 @@ DSL commands, and SQL all use plain words. `customers` are different identifiers if a backend would treat them as such (we follow SQLite's case-insensitive identifier rules at the schema level but preserve the user's - written casing in display). + written casing in display). Concretely, a command may name a + table in any capitalization (the engine resolves it + case-insensitively); every executor **canonicalizes a + user-supplied table name to its stored case** before touching + metadata or CSV (`canonical_table_name` in `db.rs`), so the + live schema, the metadata tables, and the `data/.csv` + files stay in step regardless of how the user capitalized the + name. (Column names are matched case-sensitively and a wrong + case is refused as "no such column" — strict, but never + drifting.) - **Whitespace is liberal.** Any amount of horizontal whitespace between tokens is accepted, including around punctuation (`,`, `:`, `(`, `)`). diff --git a/src/db.rs b/src/db.rs index b893308..cbd1792 100644 --- a/src/db.rs +++ b/src/db.rs @@ -2889,6 +2889,51 @@ fn user_table_exists(conn: &Connection, table: &str) -> Result { Ok(count > 0) } +/// An engine-neutral "no such table" error for `name`. +fn no_such_table(name: &str) -> DbError { + DbError::Sqlite { + message: format!("no such table: {name}"), + kind: SqliteErrorKind::NoSuchTable, + } +} + +/// Resolve a user-supplied table name to its **stored (canonical) case**, +/// or `None` if no such user table exists. +/// +/// SQL identifiers are case-insensitive, so a user may type a table name +/// in any capitalization and the engine resolves it. But our metadata +/// tables (keyed by `table_name` / `parent_table` / `child_table`) and the +/// `data/
.csv` files are keyed by the *stored* case, and TEXT `=` +/// is case-sensitive — so an executor that used the as-typed name would +/// drift the metadata/CSV out of step with the live schema. Every executor +/// that names a table canonicalizes first and then operates on the +/// canonical name, keeping the live schema, the metadata, and the CSV in +/// step regardless of how the user capitalised the name. +/// +/// Internal `__rdbms_*` tables are excluded (treated as non-existent), +/// folding the [`reject_internal_table_name`] guard into the same lookup. +fn canonical_table_name(conn: &Connection, name: &str) -> Result, DbError> { + let mut stmt = conn + .prepare( + "SELECT name FROM sqlite_schema \ + WHERE type = 'table' AND name = ?1 COLLATE NOCASE \ + AND name NOT LIKE 'sqlite_%' \ + AND substr(name, 1, 8) != '__rdbms_'", + ) + .map_err(DbError::from_rusqlite)?; + let mut rows = stmt.query([name]).map_err(DbError::from_rusqlite)?; + match rows.next().map_err(DbError::from_rusqlite)? { + Some(row) => Ok(Some(row.get::<_, String>(0).map_err(DbError::from_rusqlite)?)), + None => Ok(None), + } +} + +/// Resolve a table name to its canonical stored case, erroring with a +/// "no such table" if it does not exist (the common executor entry guard). +fn require_canonical_table(conn: &Connection, name: &str) -> Result { + canonical_table_name(conn, name)?.ok_or_else(|| no_such_table(name)) +} + fn row_value_to_cell(row: &rusqlite::Row<'_>, idx: usize) -> Result { use rusqlite::types::ValueRef; let v = row.get_ref(idx).map_err(DbError::from_rusqlite)?; @@ -3269,6 +3314,11 @@ fn do_drop_table( source: Option<&str>, name: &str, ) -> Result<(), DbError> { + // Canonicalize the user-typed name to its stored case (and refuse a + // non-existent / internal table), so the metadata DELETEs and the CSV + // removal target the right name regardless of capitalization. + let canonical_name = require_canonical_table(conn, name)?; + let name = canonical_name.as_str(); // Refuse the drop while any *other* table still has a // relationship pointing at this one — dropping the parent // would leave dangling FK constraints in the children. The @@ -3343,7 +3393,8 @@ fn do_add_column( table: &str, column: &ColumnSpec, ) -> Result { - reject_internal_table_name(table)?; + let canonical_table = require_canonical_table(conn, table)?; + let table = canonical_table.as_str(); if matches!(column.ty, Type::Serial | Type::ShortId) { // ADR-0029 §6: a `serial` / `shortid` column auto-fills // its own values, so a separate `default` is ambiguous. @@ -3679,11 +3730,13 @@ fn do_add_constraint( column: &str, constraint: &Constraint, ) -> Result { - // Refuse the internal `__rdbms_*` tables up-front (as "no such - // table"), like the sibling schema-mutation executors. Closes the - // simple `add constraint` exposure and the SQL `ALTER TABLE … ADD - // CONSTRAINT` decomposition target (ADR-0035 §4g). - reject_internal_table_name(table)?; + // Canonicalize to the stored case (and refuse a non-existent / + // internal `__rdbms_*` table as "no such table"), like the sibling + // schema-mutation executors. Closes the simple `add constraint` + // exposure and the SQL `ALTER TABLE … ADD CONSTRAINT` decomposition + // target (ADR-0035 §4g). + let canonical_table = require_canonical_table(conn, table)?; + let table = canonical_table.as_str(); let old_schema = read_schema(conn, table)?; let (col_is_pk, col_user_type) = { let col = old_schema @@ -3819,6 +3872,8 @@ fn do_drop_constraint( column: &str, kind: ConstraintKind, ) -> Result { + let canonical_table = require_canonical_table(conn, table)?; + let table = canonical_table.as_str(); let old_schema = read_schema(conn, table)?; let (col_is_pk, present) = { let col = old_schema @@ -4242,7 +4297,8 @@ fn do_drop_column( column: &str, cascade: bool, ) -> Result { - reject_internal_table_name(table)?; + let canonical_table = require_canonical_table(conn, table)?; + let table = canonical_table.as_str(); let schema = read_schema(conn, table)?; let col_info = schema .columns @@ -4365,7 +4421,8 @@ fn do_rename_column( old: &str, new: &str, ) -> Result { - reject_internal_table_name(table)?; + let canonical_table = require_canonical_table(conn, table)?; + let table = canonical_table.as_str(); let schema = read_schema(conn, table)?; if !schema.columns.iter().any(|c| c.name == old) { return Err(DbError::Sqlite { @@ -4484,20 +4541,14 @@ fn do_rename_table( old: &str, new: &str, ) -> Result { - reject_internal_table_name(old)?; reject_internal_table_name(new)?; + // Canonicalize the source to its stored case (and refuse a + // non-existent / internal source as "no such table") — so a + // case-variant source name still resolves to the real table and the + // metadata UPDATEs below match the stored case. + let canonical_old = require_canonical_table(conn, old)?; + let old = canonical_old.as_str(); - // Existence + collision: `read_schema` does not error on a missing - // table (`pragma_table_info` returns no rows), so check explicitly — - // and pre-empt the engine's own collision error so the refusal stays - // engine-neutral (ADR-0035 §9). - let tables = do_list_tables(conn)?; - if !tables.iter().any(|t| t == old) { - return Err(DbError::Sqlite { - message: format!("no such table: {old}"), - kind: SqliteErrorKind::NoSuchTable, - }); - } if old == new { return Err(DbError::Unsupported(format!( "rename: new name is identical to the existing one (`{old}`)." @@ -4515,6 +4566,7 @@ fn do_rename_table( treats them as the same table, so there is nothing to rename." ))); } + let tables = do_list_tables(conn)?; if tables.iter().any(|t| t.eq_ignore_ascii_case(new)) { return Err(DbError::Unsupported(format!( "table `{new}` already exists; pick a different name." @@ -4677,11 +4729,13 @@ fn do_change_column_type( ty: Type, mode: ChangeColumnMode, ) -> Result { - // Refuse the internal `__rdbms_*` tables up-front (as "no such - // table"), like the sibling column executors. Closes the simple - // `change column` exposure and the SQL `ALTER COLUMN TYPE` - // decomposition target (ADR-0035 §4f); user-confirmed 2026-05-25. - reject_internal_table_name(table)?; + // Canonicalize to the stored case (and refuse a non-existent / + // internal `__rdbms_*` table as "no such table"), like the sibling + // column executors. Closes the simple `change column` exposure and + // the SQL `ALTER COLUMN TYPE` decomposition target (ADR-0035 §4f); + // user-confirmed 2026-05-25. + let canonical_table = require_canonical_table(conn, table)?; + let table = canonical_table.as_str(); let old_schema = read_schema(conn, table)?; let col_info = old_schema .columns @@ -6555,12 +6609,16 @@ fn do_add_relationship( on_update: ReferentialAction, create_fk: bool, ) -> Result { - // Refuse the internal `__rdbms_*` tables on either endpoint (as "no - // such table"), like the sibling schema-mutation executors. Closes - // the simple `add 1:n relationship` exposure and the SQL `ALTER + // Canonicalize both endpoints to their stored case (and refuse a + // non-existent / internal `__rdbms_*` table as "no such table"), like + // the sibling schema-mutation executors — so the relationship metadata + // stores the stored-case names and `describe` / rebuild match them. + // Closes the simple `add 1:n relationship` exposure and the SQL `ALTER // TABLE … ADD FOREIGN KEY` decomposition target (ADR-0035 §4g). - reject_internal_table_name(parent_table)?; - reject_internal_table_name(child_table)?; + let canonical_parent = require_canonical_table(conn, parent_table)?; + let parent_table = canonical_parent.as_str(); + let canonical_child = require_canonical_table(conn, child_table)?; + let child_table = canonical_child.as_str(); // 1. Read parent schema; verify the referenced column is a PK. let parent_schema = read_schema(conn, parent_table)?; let parent_col = parent_schema @@ -6775,7 +6833,8 @@ fn do_alter_add_table_check( name: Option<&str>, expr_sql: &str, ) -> Result { - reject_internal_table_name(table)?; + let canonical_table = require_canonical_table(conn, table)?; + let table = canonical_table.as_str(); let old_schema = read_schema(conn, table)?; if name.is_some() && !check_table_has_name_column(conn)? { @@ -6879,7 +6938,8 @@ fn do_alter_add_unique( table: &str, columns: &[String], ) -> Result { - reject_internal_table_name(table)?; + let canonical_table = require_canonical_table(conn, table)?; + let table = canonical_table.as_str(); let old_schema = read_schema(conn, table)?; for c in columns { if !old_schema.columns.iter().any(|oc| &oc.name == c) { @@ -6945,7 +7005,8 @@ fn do_drop_constraint_by_name( table: &str, name: &str, ) -> Result, DbError> { - reject_internal_table_name(table)?; + let canonical_table = require_canonical_table(conn, table)?; + let table = canonical_table.as_str(); // 1. A named table-level CHECK on this table? if check_table_has_name_column(conn)? { @@ -7122,10 +7183,12 @@ fn do_add_index( columns: &[String], unique: bool, ) -> Result { - // 0. Internal tables are not user tables (ADR-0025 / ADR-0035 §4d) — - // refused on both the simple `add index` and SQL `CREATE INDEX` - // surfaces, which both reach here. - reject_internal_table_name(table)?; + // 0. Canonicalize to the stored case (and refuse a non-existent / + // internal `__rdbms_*` table) — both the simple `add index` and SQL + // `CREATE INDEX` surfaces reach here, and the auto-index name embeds + // the table name, so it must use the stored case. + let canonical_table = require_canonical_table(conn, table)?; + let table = canonical_table.as_str(); // 1. Table must exist; gather its columns. let schema = read_schema(conn, table)?; // 2. Every indexed column must exist on the table. @@ -7649,6 +7712,8 @@ fn do_insert( user_columns: Option<&[String]>, user_values: &[Value], ) -> Result { + let canonical_table = require_canonical_table(conn, table)?; + let table = canonical_table.as_str(); let schema = read_schema(conn, table)?; // Resolve which columns the user is providing values for. @@ -7809,6 +7874,8 @@ fn do_update( "UPDATE requires at least one assignment".to_string(), )); } + let canonical_table = require_canonical_table(conn, table)?; + let table = canonical_table.as_str(); let schema = read_schema(conn, table)?; // Capture rowids of matching rows up front so we can fetch @@ -7901,6 +7968,8 @@ fn do_delete( table: &str, filter: &RowFilter, ) -> Result { + let canonical_table = require_canonical_table(conn, table)?; + let table = canonical_table.as_str(); let schema = read_schema(conn, table)?; // Snapshot child-table row counts before the delete so we @@ -8208,6 +8277,8 @@ fn do_sql_insert( returning: bool, ) -> Result { debug!(sql = %sql, table = %target_table, returning, "sql_insert"); + let canonical_table = require_canonical_table(conn, target_table)?; + let target_table = canonical_table.as_str(); // The `shortid` auto-fill rewrite reconstructs only `INSERT … // VALUES …` and would drop any trailing clause — `ON CONFLICT …` // (3h) and/or `RETURNING …` (3g). `row_source` is the clean @@ -8294,6 +8365,8 @@ fn do_sql_update( returning: bool, ) -> Result { debug!(sql = %sql, table = %target_table, returning, "sql_update"); + let canonical_table = require_canonical_table(conn, target_table)?; + let target_table = canonical_table.as_str(); let tx = conn .unchecked_transaction() .map_err(DbError::from_rusqlite)?; @@ -8365,6 +8438,8 @@ fn do_sql_delete( returning: bool, ) -> Result { debug!(sql = %sql, table = %target_table, returning, "sql_delete"); + let canonical_table = require_canonical_table(conn, target_table)?; + let target_table = canonical_table.as_str(); // Snapshot child-table row counts before the delete so cascade // effects can be detected by diffing afterwards (Amendment 2; diff --git a/tests/case_insensitive_names.rs b/tests/case_insensitive_names.rs new file mode 100644 index 0000000..6b644d6 --- /dev/null +++ b/tests/case_insensitive_names.rs @@ -0,0 +1,235 @@ +//! Regression: SQL identifiers are case-insensitive, so a user may refer +//! to a table by any capitalization. The engine resolves the name +//! case-insensitively, but our metadata tables and CSV files are keyed by +//! the *stored* case — so before the fix, an operation naming the table in +//! a different case drifted the metadata / silently skipped the CSV write +//! (losing data on reload). Every table-naming executor now canonicalizes +//! the name to its stored case first. These tests pin that behaviour +//! across schema and data operations, including fresh-rebuild round-trips +//! (which reconstruct purely from the text artifacts, so any drift shows). + +use rdbms_playground::db::Database; +use rdbms_playground::dsl::{ColumnSpec, Type, Value}; +use rdbms_playground::event::AppEvent; +use rdbms_playground::persistence::Persistence; +use rdbms_playground::project; +use rdbms_playground::runtime::run_replay; + +fn rt() -> tokio::runtime::Runtime { + tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("tokio rt") +} + +fn open() -> (project::Project, Database, tempfile::TempDir) { + let dir = tempfile::tempdir().expect("create tempdir"); + let project = + project::open_or_create(None, Some(dir.path())).expect("open or create project"); + let db = Database::open_with_persistence( + project.db_path(), + Persistence::new(project.path().to_path_buf()), + ) + .expect("db"); + (project, db, dir) +} + +/// Drop the db handle, delete the `.db`, reopen, and rebuild purely from +/// the text artifacts (`project.yaml` + CSVs) — where any metadata/CSV +/// drift from a case-variant operation would surface. +fn fresh_rebuild( + old: Database, + project: &project::Project, + r: &tokio::runtime::Runtime, +) -> Database { + use rdbms_playground::project::PLAYGROUND_DB; + drop(old); + std::fs::remove_file(project.path().join(PLAYGROUND_DB)).expect("remove db"); + let db = Database::open_with_persistence( + project.db_path(), + Persistence::new(project.path().to_path_buf()), + ) + .expect("db"); + r.block_on(db.rebuild_from_text(project.path().to_path_buf(), None)) + .expect("rebuild"); + db +} + +fn replay(project: &project::Project, db: &Database, r: &tokio::runtime::Runtime, script: &str) { + std::fs::write(project.path().join("ci.commands"), script).expect("write script"); + let events = r.block_on(run_replay(db, project.path(), "ci.commands")); + assert!( + matches!(events.last(), Some(AppEvent::ReplayCompleted { .. })), + "script replayed cleanly; events: {events:?}" + ); +} + +fn tables(db: &Database, r: &tokio::runtime::Runtime) -> Vec { + r.block_on(db.list_tables()).expect("list_tables") +} + +#[test] +fn rename_column_with_case_variant_table_keeps_metadata_in_step() { + // The engine renames the column on the real table; the user-type + // metadata must follow even when the table is named in a different + // case (without canonicalization the metadata UPDATE misses and + // `amount` loses its `int` user-type). + let (_p, db, _d) = open(); + let r = rt(); + r.block_on(db.create_table( + "Items".to_string(), + vec![ColumnSpec::new("id", Type::Int), ColumnSpec::new("qty", Type::Int)], + vec!["id".to_string()], + Some("create".to_string()), + )) + .expect("create Items"); + + r.block_on(db.rename_column( + "items".to_string(), // ← case variant of `Items` + "qty".to_string(), + "amount".to_string(), + Some("rename".to_string()), + )) + .expect("rename column via a case-variant table name"); + + let desc = r + .block_on(db.describe_table("Items".to_string(), None)) + .expect("describe Items"); + let amount = desc + .columns + .iter() + .find(|c| c.name == "amount") + .expect("the column was renamed to `amount`"); + assert_eq!( + amount.user_type, + Some(Type::Int), + "the user-type metadata followed the case-variant rename (no drift)" + ); +} + +#[test] +fn insert_with_case_variant_table_persists_and_survives_rebuild() { + // The data-loss case: a wrong-case INSERT executes on the real table + // (engine is case-insensitive), but the CSV write must target the + // stored case — otherwise the row is silently absent from the CSV and + // lost on a fresh rebuild. + let (project, db, _d) = open(); + let r = rt(); + replay( + &project, + &db, + &r, + "create table Items with pk id(int)\n\ + add column Items: note (text)\n\ + insert into items (id, note) values (1, 'kept')\n", + ); + + let db = fresh_rebuild(db, &project, &r); + let rows = r + .block_on(db.query_data("Items".to_string(), None, None, None)) + .expect("query") + .rows; + assert_eq!(rows.len(), 1, "the wrong-case insert survived the rebuild (no data loss)"); + assert_eq!(rows[0][1].as_deref(), Some("kept")); +} + +#[test] +fn add_column_with_case_variant_table_survives_rebuild() { + let (project, db, _d) = open(); + let r = rt(); + replay( + &project, + &db, + &r, + "create table Items with pk id(int)\n\ + alter table items add column qty int check (qty >= 0)\n", + ); + + let db = fresh_rebuild(db, &project, &r); + let desc = r.block_on(db.describe_table("Items".to_string(), None)).expect("describe"); + let qty = desc.columns.iter().find(|c| c.name == "qty").expect("qty added"); + assert_eq!(qty.user_type, Some(Type::Int), "qty's user-type survived the rebuild"); + // The CHECK is intact too (a negative qty is refused under the real table). + assert!( + r.block_on(db.insert( + "Items".to_string(), + Some(vec!["id".into(), "qty".into()]), + vec![Value::Number("1".into()), Value::Number("-3".into())], + Some("i".into()), + )) + .is_err(), + "the CHECK added via a case-variant ALTER is enforced" + ); +} + +#[test] +fn drop_table_with_case_variant_name_clears_table_and_csv() { + let (project, db, _d) = open(); + let r = rt(); + replay( + &project, + &db, + &r, + "create table Items with pk id(int)\n\ + add column Items: note (text)\n\ + insert into Items (id, note) values (1, 'x')\n\ + drop table items\n", + ); + assert!(!tables(&db, &r).contains(&"Items".to_string()), "the table was dropped"); + let csv = project.path().join(project::DATA_DIR).join("Items.csv"); + assert!(!csv.exists(), "the CSV was removed despite the case-variant drop"); + + // A fresh rebuild yields no Items (the metadata/yaml has no orphan). + let db = fresh_rebuild(db, &project, &r); + assert!(!tables(&db, &r).contains(&"Items".to_string())); +} + +#[test] +fn rename_table_accepts_case_variant_source() { + // `alter table orders rename to Sales` when the table is stored as + // `Orders` now resolves the source case-insensitively and renames it. + let (project, db, _d) = open(); + let r = rt(); + replay( + &project, + &db, + &r, + "create table Orders with pk id(int)\n\ + insert into Orders (id) values (1)\n\ + alter table orders rename to Sales\n", + ); + let t = tables(&db, &r); + assert!( + t.contains(&"Sales".to_string()) && !t.contains(&"Orders".to_string()), + "the case-variant source resolved and the table was renamed: {t:?}" + ); + let db = fresh_rebuild(db, &project, &r); + assert!(tables(&db, &r).contains(&"Sales".to_string())); +} + +#[test] +fn add_relationship_with_case_variant_tables_survives_rebuild() { + // The relationship metadata must store the canonical table names, or + // `describe` (which matches by stored case) would not show it, and a + // rebuild would emit a relationship against the wrong-case name. + let (project, db, _d) = open(); + let r = rt(); + replay( + &project, + &db, + &r, + "create table Parent with pk id(int)\n\ + create table Child with pk id(int)\n\ + add column Child: parent_id (int)\n\ + add 1:n relationship from parent.id to child.parent_id\n", + ); + // The parent's inbound relationship is visible under the stored case. + let p = r.block_on(db.describe_table("Parent".to_string(), None)).expect("describe Parent"); + assert_eq!(p.inbound_relationships.len(), 1, "relationship recorded under the stored case"); + assert_eq!(p.inbound_relationships[0].other_table, "Child"); + + let db = fresh_rebuild(db, &project, &r); + let p = r.block_on(db.describe_table("Parent".to_string(), None)).expect("describe Parent"); + assert_eq!(p.inbound_relationships.len(), 1, "relationship survived the rebuild"); + assert_eq!(p.inbound_relationships[0].other_table, "Child"); +}