add column: column constraints — NOT NULL / UNIQUE / DEFAULT (ADR-0029 §6)
`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.
This commit is contained in:
@@ -1796,16 +1796,26 @@ fn column_constraints_sql(spec: &ColumnSpec) -> Result<String, DbError> {
|
||||
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<Option<String>, 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<AddColumnResult, DbError> {
|
||||
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<AddColumnResult, DbError> {
|
||||
// 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<AddColumnResult, DbError> {
|
||||
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();
|
||||
|
||||
Reference in New Issue
Block a user