From 58d895882292ffa4a8cc7c282528ae4531c73674 Mon Sep 17 00:00:00 2001 From: "claude@clouddev1" Date: Tue, 19 May 2026 14:50:19 +0000 Subject: [PATCH] =?UTF-8?q?add=20column:=20column=20constraints=20?= =?UTF-8?q?=E2=80=94=20NOT=20NULL=20/=20UNIQUE=20/=20DEFAULT=20(ADR-0029?= =?UTF-8?q?=20=C2=A76)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `add column` now accepts the shared constraint suffix and the worker honours it — the surface where NOT NULL / UNIQUE actually matter, on non-PK columns. - Grammar: `ADD_COLUMN_NODES` gains the constraint-suffix fragment; `collect_column_constraints` folds it into `Command::AddColumn`. - `do_add_column` routes per ADR-0029 §6: SQLite's `ALTER TABLE ADD COLUMN` cannot express `UNIQUE` and requires a default for `NOT NULL`, so those go through the rebuild primitive (`do_add_constrained_column_via_rebuild`); plain cases keep the ALTER path with the constraint suffix appended. - Pre-flight refusals, before any SQL write: a NOT NULL column with no default added to a populated table; a UNIQUE column with a default added to a multi-row table; a default on a `serial` / `shortid` column. CHECK is still deferred to the next commit. 1193 tests pass (+9); clippy clean. --- src/db.rs | 262 +++++++++++++++++++++++++++++++++++++++-- src/dsl/grammar/ddl.rs | 85 ++++++++++++- 2 files changed, 330 insertions(+), 17 deletions(-) diff --git a/src/db.rs b/src/db.rs index 28a3b82..6435fd3 100644 --- a/src/db.rs +++ b/src/db.rs @@ -1796,16 +1796,26 @@ fn column_constraints_sql(spec: &ColumnSpec) -> Result { if spec.unique { sql.push_str(" UNIQUE"); } - if let Some(value) = &spec.default { - let bound = value - .bind_for_column(&spec.name, spec.ty) - .map_err(|e| DbError::InvalidValue(e.to_string()))?; + if let Some(literal) = default_sql_literal(spec)? { sql.push_str(" DEFAULT "); - sql.push_str(&sql_literal(&bound_to_sqlite_value(&bound))); + sql.push_str(&literal); } Ok(sql) } +/// The SQL literal for a column's `DEFAULT` value, bound +/// against the column's user-facing type (ADR-0029). `None` +/// when the column carries no default. +fn default_sql_literal(spec: &ColumnSpec) -> Result, DbError> { + let Some(value) = &spec.default else { + return Ok(None); + }; + let bound = value + .bind_for_column(&spec.name, spec.ty) + .map_err(|e| DbError::InvalidValue(e.to_string()))?; + Ok(Some(sql_literal(&bound_to_sqlite_value(&bound)))) +} + fn do_create_table( conn: &Connection, persistence: Option<&Persistence>, @@ -1971,11 +1981,28 @@ fn do_add_column( table: &str, column: &ColumnSpec, ) -> Result { - let auto_generated = matches!(column.ty, Type::Serial | Type::ShortId); - if !auto_generated { - return do_add_plain_column(conn, persistence, source, table, column); + 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. + if column.default.is_some() { + return Err(DbError::Unsupported(format!( + "`{name}` is a {ty} column — it auto-fills its own values, \ + so it cannot also carry a `default`.", + name = column.name, + ty = column.ty.keyword(), + ))); + } + return do_add_auto_generated_column(conn, persistence, source, table, column); + } + // SQLite's `ALTER TABLE ADD COLUMN` cannot express `UNIQUE`, + // and a `NOT NULL` column added that way must carry a + // default — both route through the rebuild primitive + // instead (ADR-0029 §6). + if column.unique || (column.not_null && column.default.is_none()) { + do_add_constrained_column_via_rebuild(conn, persistence, source, table, column) + } else { + do_add_plain_column(conn, persistence, source, table, column) } - do_add_auto_generated_column(conn, persistence, source, table, column) } /// Plain ALTER-TABLE path for non-auto-generated types. @@ -1986,15 +2013,18 @@ fn do_add_plain_column( table: &str, spec: &ColumnSpec, ) -> Result { - // ADR-0029: the constraint DDL suffix is emitted here once - // the constraint grammar lands; for now name + type only. + // The plain `ALTER TABLE ADD COLUMN` path. `do_add_column` + // only routes here when the constraints are ALTER-expressible + // (no UNIQUE; NOT NULL only alongside a default), so the + // ADR-0029 suffix appends cleanly. let ty = spec.ty; let column = spec.name.as_str(); let ddl = format!( - "ALTER TABLE {tbl} ADD COLUMN {col} {sqlite_type};", + "ALTER TABLE {tbl} ADD COLUMN {col} {sqlite_type}{constraints};", tbl = quote_ident(table), col = quote_ident(column), sqlite_type = ty.sqlite_strict_type(), + constraints = column_constraints_sql(spec)?, ); debug!(ddl = %ddl, "add_column"); let tx = conn @@ -2153,6 +2183,85 @@ fn do_add_auto_generated_column( }) } +/// Add a plain column whose constraints `ALTER TABLE ADD +/// COLUMN` cannot express — `UNIQUE`, or `NOT NULL` with no +/// default — through the rebuild primitive (ADR-0029 §6). A +/// pre-flight check refuses, with a friendly message, the +/// cases the table's existing rows would violate. +fn do_add_constrained_column_via_rebuild( + conn: &Connection, + persistence: Option<&Persistence>, + source: Option<&str>, + table: &str, + spec: &ColumnSpec, +) -> Result { + let old_schema = read_schema(conn, table)?; + if old_schema.columns.iter().any(|c| c.name == spec.name) { + return Err(DbError::Unsupported(format!( + "column `{table}.{}` already exists.", + spec.name, + ))); + } + let row_count = count_rows(conn, table)?; + + // ADR-0029 §6 pre-flight refusals — caught before any SQL + // write, surfaced as friendly messages. + if spec.not_null && spec.default.is_none() && row_count > 0 { + return Err(DbError::Unsupported(format!( + "adding the NOT NULL column `{}` to `{table}`, which already \ + has {row_count} row(s), needs a `default` — every existing \ + row would otherwise be null.", + spec.name, + ))); + } + if spec.unique && spec.default.is_some() && row_count > 1 { + return Err(DbError::Unsupported(format!( + "adding the UNIQUE column `{}` with a default to `{table}` \ + would give all {row_count} existing rows the same value, \ + breaking uniqueness.", + spec.name, + ))); + } + + // Append the new column to the schema; the rebuild's + // column-by-name copy leaves it at its DEFAULT (or NULL). + let mut new_schema = old_schema.clone(); + new_schema.columns.push(ReadColumn { + name: spec.name.clone(), + sqlite_type: spec.ty.sqlite_strict_type().to_string(), + notnull: spec.not_null, + primary_key: false, + unique: spec.unique, + default_sql: default_sql_literal(spec)?, + user_type: Some(spec.ty), + }); + + let metadata_updates = |tx: &rusqlite::Transaction<'_>| -> Result<(), DbError> { + tx.execute( + &format!( + "INSERT INTO {META_TABLE} (table_name, column_name, user_type) \ + VALUES (?1, ?2, ?3);" + ), + [table, spec.name.as_str(), spec.ty.keyword()], + ) + .map_err(DbError::from_rusqlite)?; + let changes = Changes { + schema_dirty: true, + rewritten_tables: vec![table.to_string()], + ..Changes::default() + }; + finalize_persistence(tx, persistence, source, &changes)?; + Ok(()) + }; + + rebuild_table(conn, table, &old_schema, &new_schema, metadata_updates)?; + + Ok(AddColumnResult { + description: do_describe_table(conn, table)?, + client_side_notes: Vec::new(), + }) +} + /// Generate `count` shortid values that don't collide with each /// other or with `existing` (a slice of currently-stored /// shortid values, used during change-column-to-shortid). Up to @@ -8457,6 +8566,135 @@ mod tests { ); } + // --- column constraints at add-column (ADR-0029 §6) ----- + + #[tokio::test] + async fn add_column_with_default_fills_existing_rows() { + let db = db(); + people_table(&db).await; // 4 rows + db.add_column( + "People".to_string(), + col_c("tier", Type::Int, false, false, Some(Value::Number("1".to_string()))), + None, + ) + .await + .unwrap(); + let data = db + .query_data("People".to_string(), None, None, None) + .await + .unwrap(); + let idx = data.columns.iter().position(|c| c == "tier").unwrap(); + assert!( + data.rows.iter().all(|r| r[idx].as_deref() == Some("1")), + "every existing row took the new column's DEFAULT", + ); + } + + #[tokio::test] + async fn add_not_null_column_without_default_to_populated_table_is_refused() { + let db = db(); + people_table(&db).await; + let result = db + .add_column( + "People".to_string(), + col_c("x", Type::Int, true, false, None), + None, + ) + .await; + assert!( + result.is_err(), + "a NOT NULL column with no default cannot be added to a table with rows", + ); + } + + #[tokio::test] + async fn add_not_null_column_without_default_to_empty_table_succeeds() { + let db = db(); + db.create_table( + "T".to_string(), + vec![col("id", Type::Serial)], + vec!["id".to_string()], + None, + ) + .await + .unwrap(); + db.add_column( + "T".to_string(), + col_c("x", Type::Int, true, false, None), + None, + ) + .await + .expect("NOT NULL with no default is fine on an empty table"); + let desc = db.describe_table("T".to_string(), None).await.unwrap(); + assert!(desc.columns.iter().find(|c| c.name == "x").unwrap().notnull); + } + + #[tokio::test] + async fn add_not_null_column_with_a_default_succeeds_on_a_populated_table() { + let db = db(); + people_table(&db).await; + db.add_column( + "People".to_string(), + col_c("tier", Type::Int, true, false, Some(Value::Number("0".to_string()))), + None, + ) + .await + .unwrap(); + let desc = db.describe_table("People".to_string(), None).await.unwrap(); + let tier = desc.columns.iter().find(|c| c.name == "tier").unwrap(); + assert!(tier.notnull); + assert_eq!(tier.default.as_deref(), Some("0")); + } + + #[tokio::test] + async fn add_unique_column_applies_the_constraint_via_rebuild() { + let db = db(); + people_table(&db).await; // 4 rows; the new column is all-NULL + db.add_column( + "People".to_string(), + col_c("badge", Type::Text, false, true, None), + None, + ) + .await + .expect("a UNIQUE column with no default is fine — NULLs do not collide"); + let desc = db.describe_table("People".to_string(), None).await.unwrap(); + assert!(desc.columns.iter().find(|c| c.name == "badge").unwrap().unique); + } + + #[tokio::test] + async fn add_unique_column_with_default_to_a_multi_row_table_is_refused() { + let db = db(); + people_table(&db).await; // 4 rows + let result = db + .add_column( + "People".to_string(), + col_c("badge", Type::Text, false, true, Some(Value::Text("X".to_string()))), + None, + ) + .await; + assert!( + result.is_err(), + "a UNIQUE column with a default would give every row the same value", + ); + } + + #[tokio::test] + async fn add_serial_column_with_a_default_is_refused() { + let db = db(); + people_table(&db).await; + let result = db + .add_column( + "People".to_string(), + col_c("seq", Type::Serial, false, false, Some(Value::Number("1".to_string()))), + None, + ) + .await; + assert!( + result.is_err(), + "a serial column auto-fills its own values — a default is ambiguous", + ); + } + #[tokio::test] async fn update_with_all_rows_affects_everything() { let db = db(); diff --git a/src/dsl/grammar/ddl.rs b/src/dsl/grammar/ddl.rs index 82b8c24..3075b64 100644 --- a/src/dsl/grammar/ddl.rs +++ b/src/dsl/grammar/ddl.rs @@ -15,6 +15,7 @@ use crate::dsl::action::ReferentialAction; use crate::dsl::command::{ ChangeColumnMode, ColumnSpec, Command, IndexSelector, RelationshipSelector, }; +use crate::dsl::value::Value; use crate::dsl::grammar::{ CommandNode, HintMode, IdentSource, Node, ValidationError, Word, shared::{REFERENTIAL_CLAUSES, TYPE_SLOT, TYPE_VALIDATOR}, @@ -299,6 +300,9 @@ const ADD_COLUMN_NODES: &[Node] = &[ Node::Punct('('), TYPE_SLOT, Node::Punct(')'), + // ADR-0029: the constraint suffix — shared with `create + // table`'s column spec. + COLUMN_CONSTRAINT_SUFFIX, ]; const ADD_COLUMN: Node = Node::Seq(ADD_COLUMN_NODES); @@ -612,15 +616,15 @@ fn build_add(path: &MatchedPath) -> Result { message_key: "parse.error_wrapper", args: vec![("detail", "unknown type".to_string())], })?; + let (not_null, unique, default) = collect_column_constraints(path)?; Ok(Command::AddColumn { table: require_ident(path, "table_name")?, column: require_ident(path, "column_name")?, ty, - // Constraint suffix is wired in once the - // constraint grammar lands (ADR-0029). - not_null: false, - unique: false, - default: None, + not_null, + unique, + default, + // CHECK joins in a later ADR-0029 step. check: None, }) } @@ -890,6 +894,47 @@ const CREATE_TABLE_NODES: &[Node] = &[ ]; const CREATE_TABLE: Node = Node::Seq(CREATE_TABLE_NODES); +/// Collect the ADR-0029 constraint suffix from a +/// single-column command's matched path (`add column`), +/// returning the `(not_null, unique, default)` triple. The +/// scan reacts only to the four constraint keywords, so +/// passing the whole path is safe. (`create table`'s +/// multi-column collection is inline in `build_create_table`.) +fn collect_column_constraints( + path: &MatchedPath, +) -> Result<(bool, bool, Option), ValidationError> { + let mut not_null = false; + let mut unique = false; + let mut default = None; + let mut items = path.items.iter().peekable(); + while let Some(item) = items.next() { + match &item.kind { + MatchedKind::Word("not") => { + if matches!( + items.peek().map(|i| &i.kind), + Some(MatchedKind::Word("null")) + ) { + items.next(); + not_null = true; + } + } + MatchedKind::Word("unique") => unique = true, + MatchedKind::Word("default") => { + let value = items + .next() + .and_then(crate::dsl::grammar::data::item_to_value) + .ok_or_else(|| ValidationError { + message_key: "parse.error_wrapper", + args: vec![("detail", "default needs a value".to_string())], + })?; + default = Some(value); + } + _ => {} + } + } + Ok((not_null, unique, default)) +} + /// The friendly error for declaring a constraint a /// primary-key column already implies (ADR-0029 §9). fn redundant_pk_constraint(column: &str, constraint: &str) -> ValidationError { @@ -1077,4 +1122,34 @@ mod constraint_tests { assert_eq!(cols.len(), 2); assert!(cols.iter().all(|c| !c.not_null && !c.unique && c.default.is_none())); } + + #[test] + fn add_column_parses_its_constraint_suffix() { + match parse_command("add column to T: tier (int) not null default 0") + .expect("add column should parse") + { + Command::AddColumn { + not_null, + unique, + default, + .. + } => { + assert!(not_null); + assert!(!unique); + assert_eq!(default, Some(Value::Number("0".to_string()))); + } + other => panic!("expected AddColumn, got {other:?}"), + } + } + + #[test] + fn add_column_parses_a_unique_constraint() { + match parse_command("add column to T: email (text) unique").expect("parse") { + Command::AddColumn { unique, not_null, .. } => { + assert!(unique); + assert!(!not_null); + } + other => panic!("expected AddColumn, got {other:?}"), + } + } }