diff --git a/docs/requirements.md b/docs/requirements.md index a70a758..97dad99 100644 --- a/docs/requirements.md +++ b/docs/requirements.md @@ -576,19 +576,23 @@ handoff-14 cleanup; 449 after B2/C2.) - [~] **X3** Accessibility: TUI screen-reader support is best-effort and not a v1 commitment; revisit if user need emerges. -- [~] **X4** Auto-fill semantics differ between simple and advanced - mode — **possible bug, to investigate** (raised 2026-05-26). The - simple-mode insert (`do_insert`, `db.rs`) auto-fills an omitted - **non-PK `serial`** column with `MAX(col)+1` *and* generates - `shortid`s; the advanced-mode SQL insert (`plan_shortid_autofill`) - auto-fills **only `shortid`** and does **not** do non-PK `serial` - MAX+1. The `shortid` distinction is intentional; the non-PK-`serial` - gap looks unintended (no obvious reason advanced mode should skip - filling a `serial`). Decide whether advanced mode should match - simple mode's serial auto-fill (likely yes) or whether the - difference is deliberate, and align ADR-0018 accordingly. Left - untouched by the ADR-0036 value-validation work (which is surgical - and does not change auto-fill). +- [x] **X4** Auto-fill semantics differ between simple and advanced + mode — **resolved 2026-05-27** (raised 2026-05-26). Was: simple-mode + `do_insert` auto-fills an omitted **non-PK `serial`** column with + `MAX(col)+1`, but the advanced-mode SQL insert auto-filled **only + `shortid`**, leaving an omitted non-PK serial **silently NULL** — + violating ADR-0018 §1's "auto-generated on every path" contract (the + column is `INTEGER UNIQUE`, not `NOT NULL`, so SQLite permits the + NULL). Confirmed by characterization, escalated, and fixed (decision: + advanced mode matches simple mode): the advanced-mode auto-fill + reconstruction (`db.rs`, renamed `plan_shortid_autofill` → + **`plan_autogen_autofill`**) now also fills an omitted non-PK serial + with `MAX+1` per row (single- and multi-row), mirroring `do_insert` + and the existing shortid fill. PK serial is excluded (rowid alias); + Form B (no column list) still supplies every column. Covered by + `tests/sql_insert.rs::sql_insert_autofills_omitted_nonpk_serial`. + Honours ADR-0018 §1/§5; no ADR amendment needed (the contract already + said "every path"). ADR-0036 was correct that it did not touch this. - [~] **X5** Framework cohesion / restructuring — **strategic, revisit later** (raised 2026-05-26). The grammar/execution framework (lexer → walker `Node`s → unified grammar tree → typed diff --git a/src/db.rs b/src/db.rs index 52aff3e..40db4ea 100644 --- a/src/db.rs +++ b/src/db.rs @@ -8306,7 +8306,22 @@ fn existing_shortids( /// /// `serial` columns are not handled here — an omitted `serial` /// primary key is filled by the engine's rowid (ADR-0033 §6). -fn plan_shortid_autofill( +/// Auto-fill the omitted **auto-generated** columns of an advanced-mode +/// Form-A `INSERT` (a `(col, …)` list is present) by reconstructing a +/// parameterised statement, honouring ADR-0018 §1's "auto-generated on +/// every path" contract — the same contract simple-mode `do_insert` +/// honours: +/// - **`shortid`** columns omitted from the list get a fresh distinct +/// id per row (ADR-0018 §3). +/// - **non-PK `serial`** columns omitted from the list get `MAX(col)+1`, +/// `MAX+2`, … per row (ADR-0018 §5; the X4 fix — advanced mode used +/// to leave them NULL). PK serial relies on the engine's rowid alias +/// and is never in this set. +/// +/// Returns the original `sql` + an empty param vec when there is nothing +/// to fill (Form B / no omitted auto-gen column / zero rows), so the +/// caller executes the verbatim statement unchanged. +fn plan_autogen_autofill( conn: &Connection, target_table: &str, sql: &str, @@ -8319,18 +8334,29 @@ fn plan_shortid_autofill( } let schema = read_schema(conn, target_table)?; // Identifiers are case-preserving but matched case-insensitively - // (ADR-0009): a shortid column counts as omitted unless the user - // listed a name equal to it ignoring ASCII case. + // (ADR-0009): a column counts as omitted unless the user listed a + // name equal to it ignoring ASCII case. let listed_ci: Vec = listed_columns.iter().map(|c| c.to_ascii_lowercase()).collect(); + let is_omitted = |c: &&ReadColumn| !listed_ci.contains(&c.name.to_ascii_lowercase()); let omitted_shortids: Vec = schema .columns .iter() .filter(|c| c.user_type == Some(Type::ShortId)) - .filter(|c| !listed_ci.contains(&c.name.to_ascii_lowercase())) + .filter(is_omitted) .map(|c| c.name.clone()) .collect(); - if omitted_shortids.is_empty() { + // ADR-0018 §5 + X4: a non-PK serial omitted from the list is + // auto-filled MAX+1 per row (PK serial uses the rowid alias and is + // excluded). Mirrors simple-mode `do_insert`. + let omitted_serials: Vec = schema + .columns + .iter() + .filter(|c| c.user_type == Some(Type::Serial) && !c.primary_key) + .filter(is_omitted) + .map(|c| c.name.clone()) + .collect(); + if omitted_shortids.is_empty() && omitted_serials.is_empty() { return Ok((sql.to_string(), Vec::new())); } @@ -8377,10 +8403,39 @@ fn plan_shortid_autofill( id_batches.push(generate_shortid_batch(n, &existing)?); } - // Reconstruct: listed columns followed by the omitted shortid - // columns; one parameterised tuple per materialised row. - let all_cols: Vec<&String> = - listed_columns.iter().chain(omitted_shortids.iter()).collect(); + // A serial sequence per omitted non-PK serial column: MAX(col)+1 … + // MAX(col)+n. MAX is read once (current table state); the worker- + // thread serialisation (ADR-0010) makes the read-then-insert safe. + // All values exceed the current MAX, so they collide with neither + // existing rows nor each other (the column's UNIQUE holds). MAX+1 + // (gaps jumped, not back-filled) matches `do_insert` and the rowid + // PK case (ADR-0018 §5 / Resolution 2). + let mut serial_batches: Vec> = + Vec::with_capacity(omitted_serials.len()); + for col in &omitted_serials { + let max: i64 = conn + .query_row( + &format!( + "SELECT COALESCE(MAX({col}), 0) FROM {tbl};", + col = quote_ident(col), + tbl = quote_ident(target_table), + ), + [], + |row| row.get(0), + ) + .map_err(DbError::from_rusqlite)?; + serial_batches + .push((1..=n as i64).map(|i| rusqlite::types::Value::Integer(max + i)).collect()); + } + + // Reconstruct: listed columns, then the omitted shortid columns, + // then the omitted non-PK serial columns; one parameterised tuple + // per materialised row. The param push order below matches this. + let all_cols: Vec<&String> = listed_columns + .iter() + .chain(omitted_shortids.iter()) + .chain(omitted_serials.iter()) + .collect(); let cols_csv = all_cols .iter() .map(|c| quote_ident(c)) @@ -8397,6 +8452,9 @@ fn plan_shortid_autofill( for batch in &id_batches { params.push(batch[row_idx].clone()); } + for batch in &serial_batches { + params.push(batch[row_idx].clone()); + } let placeholders = (0..per_tuple) .map(|_| { let s = format!("?{ph}"); @@ -8433,10 +8491,10 @@ fn plan_shortid_autofill( /// persistence failure rolls the insert back), then commit. /// /// Grammar-as-text (ADR-0030 §4): normally the values are literals -/// in `sql` and no parameters are bound. The sub-phase 3d shortid -/// auto-fill path is the exception — it reconstructs a -/// parameterised `INSERT` (see `plan_shortid_autofill`); either -/// way `history.log` records the original `source`, never the +/// in `sql` and no parameters are bound. The auto-fill path (an +/// omitted `shortid` or non-PK `serial` column) is the exception — it +/// reconstructs a parameterised `INSERT` (see `plan_autogen_autofill`); +/// either way `history.log` records the original `source`, never the /// rewritten statement (ADR-0030 §11). FK / UNIQUE / NOT NULL /// engine errors surface enriched via `execute_with_fk_enrichment` /// + the friendly-error layer. @@ -8512,7 +8570,7 @@ fn do_sql_insert( // params vec with the original `sql` means "no auto-fill — // execute verbatim" (the 3b path). let (exec_sql, params) = - plan_shortid_autofill(conn, target_table, sql, listed_columns, row_source, &trailing_tail)?; + plan_autogen_autofill(conn, target_table, sql, listed_columns, row_source, &trailing_tail)?; let tx = conn .unchecked_transaction() .map_err(DbError::from_rusqlite)?; diff --git a/src/dsl/grammar/sql_insert.rs b/src/dsl/grammar/sql_insert.rs index efb75d2..a1dbc5b 100644 --- a/src/dsl/grammar/sql_insert.rs +++ b/src/dsl/grammar/sql_insert.rs @@ -105,7 +105,7 @@ fn fallback_value_list() -> Node { /// `VALUES` position, so it is correctly absent here. /// - **Form B** (no column list): ALL columns in declaration order, /// including `serial` / `shortid` — advanced-mode Form B auto-fills -/// *nothing* (`plan_shortid_autofill` returns early on an empty +/// *nothing* (`plan_autogen_autofill` returns early on an empty /// column list), so the user supplies a value for every column. /// /// Empty when schemaless, the table is unknown, or a Form A list diff --git a/tests/sql_insert.rs b/tests/sql_insert.rs index 1e65fb5..4afe14c 100644 --- a/tests/sql_insert.rs +++ b/tests/sql_insert.rs @@ -707,6 +707,37 @@ fn autofill_does_not_mask_arity_mismatch() { assert!(rows.is_empty(), "no row should land on a rejected insert: {rows:?}"); } +#[test] +fn sql_insert_autofills_omitted_nonpk_serial() { + // ADR-0018 §1/§5 + X4: advanced-mode SQL INSERT auto-fills an omitted + // non-PK `serial` column with MAX+1 (per row), exactly as simple-mode + // `do_insert` does — honouring the "auto-generated on every path" + // contract. (Was: silently inserting NULL.) Mirrors the existing + // shortid auto-fill, which already runs on this path. + let (project, db, _dir) = open_project_db(); + let rt = rt(); + create_cols(&db, &rt, "t", &[("id", Type::Int), ("seq", Type::Serial)], &["id"]); + + // Single row, omitting the non-PK serial `seq`. + run_sqlinsert(&db, &rt, "insert into t (id) values (10)").expect("single-row insert runs"); + // Multi-row, omitting `seq` — each row gets a distinct, increasing + // serial continuing from the current MAX. + run_sqlinsert(&db, &rt, "insert into t (id) values (20), (30)") + .expect("multi-row insert runs"); + + let rows = csv_rows(&project, "t"); + // No NULL serials, and the sequence is 1, 2, 3 across the three rows. + assert_eq!( + rows, + vec![ + vec!["10".to_string(), "1".to_string()], + vec!["20".to_string(), "2".to_string()], + vec!["30".to_string(), "3".to_string()], + ], + "omitted non-PK serial auto-filled MAX+1 per row (no NULLs): {rows:?}", + ); +} + #[test] fn autofill_insert_select_wider_projection_is_rejected() { // SELECT projects more columns than the list: the guard defers