oli/rdbms-playground
1
0
Fork 0
Code Issues 4 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

feat: ADR-0035 4e — ALTER TABLE add/drop/rename column

Browse Source 
Advanced-only `alter` entry word; ALTER TABLE <T> ADD COLUMN <col> <type>
[constraints] | DROP COLUMN <col> | RENAME COLUMN <old> TO <new> ->
SqlAlterTable, runtime-decomposed to the existing column executors
(do_add_column / do_drop_column / do_rename_column) — one undo step each,
no new worker layer. The COLUMN keyword is required (reserves bare
RENAME TO for 4h, ADD CONSTRAINT for 4g).

- ADD COLUMN takes NOT NULL / UNIQUE / DEFAULT / CHECK (no PK / inline
  REFERENCES). do_add_column extended to consume the SQL raw-text
  default_sql / check_sql (sql_expr is validate-only, the 4a.2
  mechanism), reaching parity with CREATE TABLE's column constraints.
- Drop/rename column refuse a column any CHECK references — table-level
  AND column-level (incl. a column's own self-check on rename) — the
  4a.3 deferral, detected up-front by tokenizing the raw CHECK text
  (skipping string literals). In the shared executors, so it guards both
  the simple and SQL surfaces and fixes a latent rename-drift bug that
  desynced the stored CHECK text and broke rebuild.
- SQL DROP COLUMN refuses an index-covered column (no --cascade SQL
  spelling — matches SQLite + the simple default).
- The column executors and do_add_index gained an internal-__rdbms_*
  guard (refuse as "no such table"), closing a pre-existing exposure on
  both surfaces. (do_change_column_type / do_add_constraint /
  do_add_relationship are a tracked follow-up.)
- `alter` is advanced-only; AlterTableAction::AddColumn is boxed
  (clippy::large_enum_variant).

Docs: ADR-0035 status + §13 4e; ADR README; requirements.md Q1. Plan:
docs/plans/20260525-adr-0035-sql-ddl-4e.md.

Tests: 1854 passing / 0 failing / 0 skipped / 1 ignored; clippy clean.
This commit is contained in:
claude@clouddev1
2026-05-25 19:49:13 +00:00
parent 701217d29f
commit bbc2e34b33
17 changed files with 1294 additions and 55 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/db.rs
+184 -27
View File
@@ -3132,10 +3132,12 @@ fn do_add_column(
table: &str,
column: &ColumnSpec,
) -> Result<AddColumnResult, DbError> {
reject_internal_table_name(table)?;
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() {
// (`default_sql` is the advanced-mode raw form — ADR-0035 §4e.)
if column.default.is_some() || column.default_sql.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`.",
@@ -3146,8 +3148,9 @@ fn do_add_column(
// A CHECK on an auto-generated column is supported at
// `create table` time; adding one to a `serial` /
// `shortid` column afterwards is not (the auto-fill
// rebuild path does not thread it).
if column.check.is_some() {
// rebuild path does not thread it). `check_sql` is the
// advanced-mode raw form.
if column.check.is_some() || column.check_sql.is_some() {
return Err(DbError::Unsupported(format!(
"a `check` constraint on the auto-generated column `{}` \
can only be set when the table is created.",
@@ -3158,9 +3161,14 @@ fn do_add_column(
}
// SQLite's `ALTER TABLE ADD COLUMN` cannot express `UNIQUE`
// or `CHECK`, and a `NOT NULL` column added that way must
// carry a default — all route through the rebuild
// primitive instead (ADR-0029 §6).
if column.unique || column.check.is_some() || (column.not_null && column.default.is_none())
// carry a default — all route through the rebuild primitive
// instead (ADR-0029 §6). The advanced-mode raw forms
// (`check_sql` / `default_sql`, ADR-0035 §4e) count alongside
// the typed AST forms.
if column.unique
|| column.check.is_some()
|| column.check_sql.is_some()
|| (column.not_null && column.default.is_none() && column.default_sql.is_none())
{
do_add_constrained_column_via_rebuild(conn, persistence, source, table, column)
} else {
@@ -3370,7 +3378,12 @@ fn do_add_constrained_column_via_rebuild(
// 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 {
// A default may come from the typed AST (`default`, simple mode) or
// the raw advanced-mode text (`default_sql`, ADR-0035 §4e); either
// satisfies the NOT-NULL backfill and triggers the UNIQUE collision
// guard.
let has_default = spec.default.is_some() || spec.default_sql.is_some();
if spec.not_null && !has_default && 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 \
@@ -3378,7 +3391,7 @@ fn do_add_constrained_column_via_rebuild(
spec.name,
)));
}
if spec.unique && spec.default.is_some() && row_count > 1 {
if spec.unique && has_default && 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, \
@@ -3387,6 +3400,12 @@ fn do_add_constrained_column_via_rebuild(
)));
}
// The DEFAULT literal: the raw advanced-mode text wins over the typed
// form (ADR-0035 §4e, mirroring `column_constraints_sql`).
let default_sql = match &spec.default_sql {
Some(raw) => Some(raw.clone()),
None => default_sql_literal(spec)?,
};
// 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();
@@ -3396,16 +3415,17 @@ fn do_add_constrained_column_via_rebuild(
notnull: spec.not_null,
primary_key: false,
unique: spec.unique,
default_sql: default_sql_literal(spec)?,
default_sql,
check: None,
user_type: Some(spec.ty),
});
// The CHECK is compiled against the post-add schema, so it
// may reference the new column itself.
// The CHECK: the raw advanced-mode text (`check_sql`) wins; otherwise
// the typed `check` AST is compiled against the post-add schema (so
// it may reference the new column itself).
let check_sql = spec
.check
.as_ref()
.map(|e| compile_check_sql(e, &new_schema));
.check_sql
.clone()
.or_else(|| spec.check.as_ref().map(|e| compile_check_sql(e, &new_schema)));
if let Some(last) = new_schema.columns.last_mut() {
last.check.clone_from(&check_sql);
}
@@ -4006,6 +4026,7 @@ fn do_drop_column(
column: &str,
cascade: bool,
) -> Result<DropColumnResult, DbError> {
reject_internal_table_name(table)?;
let schema = read_schema(conn, table)?;
let col_info = schema
.columns
@@ -4059,6 +4080,19 @@ fn do_drop_column(
)));
}
// A CHECK (table-level, or a *different* column's column-level CHECK)
// that references this column (ADR-0035 §4e, the 4a.3 deferral): a
// deliberate up-front refusal — dropping the column would break that
// CHECK and the rebuilt DDL would name a missing column. The column's
// own column-level CHECK drops with it, so it does not block.
// Friendly wording is H1. Guards both surfaces.
if column_referenced_by_check(conn, table, &schema, column, false)? {
return Err(DbError::Unsupported(format!(
"cannot drop `{table}.{column}` while a CHECK references it; \
drop the constraint first."
)));
}
let tx = conn
.unchecked_transaction()
.map_err(DbError::from_rusqlite)?;
@@ -4115,6 +4149,7 @@ fn do_rename_column(
old: &str,
new: &str,
) -> Result<TableDescription, DbError> {
reject_internal_table_name(table)?;
let schema = read_schema(conn, table)?;
if !schema.columns.iter().any(|c| c.name == old) {
return Err(DbError::Sqlite {
@@ -4122,6 +4157,19 @@ fn do_rename_column(
kind: SqliteErrorKind::NoSuchColumn,
});
}
// A CHECK that references this column (ADR-0035 §4e): refuse — a
// native RENAME COLUMN rewrites the live CHECK but the stored CHECK
// text (table-level in `__rdbms_playground_table_checks`, or
// column-level in `__rdbms_playground_columns`) keeps the old name,
// drifting the metadata and breaking a later rebuild. A column's
// *own* column-level CHECK drifts too (`include_self = true`).
// Deliberate refusal (friendly wording is H1); guards both surfaces.
if column_referenced_by_check(conn, table, &schema, old, true)? {
return Err(DbError::Unsupported(format!(
"cannot rename `{table}.{old}` while a CHECK references it; \
drop the constraint first."
)));
}
if old == new {
// Nothing to do; refusing keeps behaviour
// predictable rather than appearing to "succeed"
@@ -5213,6 +5261,107 @@ fn read_table_checks(conn: &Connection, table: &str) -> Result<Vec<String>, DbEr
Ok(out)
}
/// Whether the raw CHECK expression `check_expr` references the column
/// `column` (ADR-0035 §4e — the 4a.3-deferred drop/rename guard).
///
/// Tokenizes the expression with the shared lex helpers, **skipping
/// single-quoted string literals** so an identifier that only appears
/// inside a literal does not false-match, and compares each bare
/// identifier case-insensitively. Playground column names are always
/// valid bare identifiers (`validate_user_name` rejects the characters
/// that would force quoting), so bare-identifier scanning is sufficient;
/// the engine's native `DROP`/`RENAME COLUMN` remains the backstop for
/// any miss.
fn check_references_column(check_expr: &str, column: &str) -> bool {
use crate::dsl::walker::lex_helpers::{consume_ident, consume_string_literal, skip_whitespace};
let mut i = 0;
while i < check_expr.len() {
i = skip_whitespace(check_expr, i);
if i >= check_expr.len() {
break;
}
if let Some(((_, end), _)) = consume_string_literal(check_expr, i) {
i = end;
} else if let Some((start, end)) = consume_ident(check_expr, i) {
if check_expr[start..end].eq_ignore_ascii_case(column) {
return true;
}
i = end;
} else {
// Operator / paren / number / punctuation — advance one char.
i += check_expr[i..].chars().next().map_or(1, char::len_utf8);
}
}
false
}
#[cfg(test)]
mod check_references_column_tests {
use super::check_references_column;
#[test]
fn detects_a_bare_identifier() {
assert!(check_references_column("price > 0", "price"));
assert!(check_references_column("a < b AND b < c", "b"));
}
#[test]
fn is_case_insensitive() {
assert!(check_references_column("Price > 0", "price"));
assert!(check_references_column("price > 0", "PRICE"));
}
#[test]
fn ignores_identifier_inside_a_string_literal() {
// `status` appears only inside a literal → not a reference.
assert!(!check_references_column("kind <> 'status'", "status"));
// A genuine reference alongside a literal is still found.
assert!(check_references_column("status <> 'archived'", "status"));
}
#[test]
fn ignores_a_longer_identifier_that_merely_contains_the_name() {
// `price` is a substring of these idents but not a whole match.
assert!(!check_references_column("price_cents > 0", "price"));
assert!(!check_references_column("unit_price > 0", "price"));
}
}
/// Whether any CHECK constraint on `table` references `column` — both the
/// table-level CHECKs (`read_table_checks`) and the column-level CHECKs
/// (`schema.columns[].check`), the guard for drop/rename column
/// (ADR-0035 §4e).
///
/// `include_self` controls whether the column's *own* column-level CHECK
/// counts: a RENAME rewrites the live CHECK but leaves the stored text
/// stale (drift) even for a self-check, so it must be included; a DROP
/// removes the column's own CHECK alongside it, so it is excluded (only a
/// *cross-referencing* CHECK blocks the drop).
fn column_referenced_by_check(
conn: &Connection,
table: &str,
schema: &ReadSchema,
column: &str,
include_self: bool,
) -> Result<bool, DbError> {
for expr in read_table_checks(conn, table)? {
if check_references_column(&expr, column) {
return Ok(true);
}
}
for col in &schema.columns {
if !include_self && col.name == column {
continue;
}
if let Some(check) = &col.check
&& check_references_column(check, column)
{
return Ok(true);
}
}
Ok(false)
}
/// Read the user-created indexes on `table` (ADR-0025).
///
/// `pragma_index_list` reports every index; we keep only those
@@ -6042,6 +6191,23 @@ fn resolve_index_name(name: Option<&str>, table: &str, columns: &[String]) -> St
)
}
/// Refuse an internal `__rdbms_*` table as "no such table" — the same
/// opacity the rest of the app presents (internal tables are filtered
/// from `list_tables` and never offered in completion). Guards the
/// user-facing schema-mutation executors so a deliberately-typed
/// internal name cannot index or alter the metadata tables (ADR-0035
/// §4d/§4e; the grammar's `reject_internal_table` covers only the typed
/// SQL family, not the simple DSL nodes).
fn reject_internal_table_name(table: &str) -> Result<(), DbError> {
if table.to_ascii_lowercase().starts_with("__rdbms_") {
return Err(DbError::Sqlite {
message: format!("no such table: {table}"),
kind: SqliteErrorKind::NoSuchTable,
});
}
Ok(())
}
/// Whether an index named `name` exists (ADR-0035 §4d skip checks).
///
/// `user_only = true` counts only explicit `CREATE INDEX` objects
@@ -6072,19 +6238,10 @@ fn do_add_index(
columns: &[String],
unique: bool,
) -> Result<TableDescription, DbError> {
// 0. Internal `__rdbms_*` tables are not user tables (they are
// filtered from `list_tables` and never offered in completion), so
// indexing one is refused as "no such table" — the same opacity
// the rest of the app presents. Guards BOTH the simple `add index`
// and the SQL `CREATE INDEX` surfaces, since both reach here
// (ADR-0025 / ADR-0035 §4d; the grammar's `reject_internal_table`
// only covers the typed SQL family, not the simple node).
if table.to_ascii_lowercase().starts_with("__rdbms_") {
return Err(DbError::Sqlite {
message: format!("no such table: {table}"),
kind: SqliteErrorKind::NoSuchTable,
});
}
// 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)?;
// 1. Table must exist; gather its columns.
let schema = read_schema(conn, table)?;
// 2. Every indexed column must exist on the table.