From 5bb0a147f0c38fec482a66320f2b0b271397c3d6 Mon Sep 17 00:00:00 2001 From: "claude@clouddev1" Date: Fri, 8 May 2026 14:32:19 +0000 Subject: [PATCH] ADR-0018 implementation: auto-fill contracts for serial and shortid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Generalises serial and shortid beyond their previous restricted forms: - `serial` is no longer restricted to single-column PK. Non-PK serial columns get an emitted UNIQUE constraint and use application-side MAX(col)+1 at INSERT time (rowid alias still drives the PK case for free; per ADR-0010 worker-thread serialisation, the read-then-insert sequence is safe). - `shortid` columns auto-fill existing null cells when the column is materialised — `add column T: x (shortid)` on a non-empty table no longer leaves rows in a not-really-valid NULL state. - `int -> serial` joins the type-change matrix as always-clean identity (closes the asymmetry vs `text -> shortid`); other sources are refused with a route-via-int hint. - `change column T: x (serial|shortid)` fills null source cells with sequence / generated values in the same rebuild transaction. Internal infrastructure: - ReadColumn gains `unique: bool`; read_schema detects single- column UNIQUE indexes via pragma_index_list / pragma_index_info; schema_to_ddl emits inline UNIQUE for non-PK columns. - ColumnSchema (persistence) gains `unique: bool` so the flag survives YAML round-trip and rebuild-from-text reconstructs it faithfully — preserves the "serial -> int leaves UNIQUE in place" promise across save/load cycles. - ChangeColumnTypeResult.client_side now carries `auto_filled` + `auto_fill_kind` alongside `transformed` + `lossy`; the app handler renders separate note lines when both apply. - AddColumnResult is a new return type carrying pre-rendered [client-side] note lines for the auto-fill paths. Tests: 519 -> 534 (+15). Clippy clean. --- ...o-fill-contracts-for-serial-and-shortid.md | 385 ++++++ docs/adr/README.md | 1 + src/app.rs | 88 +- src/db.rs | 1217 ++++++++++++++++- src/event.rs | 11 +- src/persistence/csv_io.rs | 2 +- src/persistence/mod.rs | 8 + src/persistence/yaml.rs | 40 +- src/runtime.rs | 11 +- src/type_change.rs | 52 +- 10 files changed, 1718 insertions(+), 97 deletions(-) create mode 100644 docs/adr/0018-auto-fill-contracts-for-serial-and-shortid.md diff --git a/docs/adr/0018-auto-fill-contracts-for-serial-and-shortid.md b/docs/adr/0018-auto-fill-contracts-for-serial-and-shortid.md new file mode 100644 index 0000000..3092ad3 --- /dev/null +++ b/docs/adr/0018-auto-fill-contracts-for-serial-and-shortid.md @@ -0,0 +1,385 @@ +# ADR-0018: Auto-fill contracts for `serial` and `shortid` columns + +## Status + +Accepted. + +Amends ADR-0005 (column type vocabulary), ADR-0014 (data +operations and value model), and ADR-0017 (column type-change +compatibility). Pulls part of C3's UNIQUE-constraint emission +forward as internal infrastructure. + +## Context + +`serial` and `shortid` are the two auto-generated types in +ADR-0005's vocabulary. Today they have asymmetric and +under-specified semantics: + +1. **`serial` only on PK.** `Type::Serial.sqlite_strict_extra()` + returns `" PRIMARY KEY"`, and `do_add_column` explicitly + refuses serial. The implicit user-facing model is "serial = + auto-incrementing PK". This is an artefact of SQLite's only + free auto-increment mechanism (the rowid alias on `INTEGER + PRIMARY KEY`); other RDBMS — PostgreSQL's `SEQUENCE`, MySQL's + `AUTO_INCREMENT` — let auto-incrementing columns exist + anywhere. Our pedagogical intent is the broader model; the + restriction is incidental to our backend choice and leaks + that choice into the user-facing surface (against ADR-0002). + +2. **`int → serial` is statically refused** in ADR-0017's + transformer matrix, while `text → shortid` is per-cell- + classified. Yet both target types are equally + "auto-generated with a uniqueness contract" — the asymmetry + isn't principled. + +3. **`add column T: x (shortid)` on a non-empty table leaves + existing rows NULL.** Per the design contract, shortids are + unique non-null identifiers — so the column ends up in a + not-really-valid state until the user issues UPDATEs. The + auto-fill logic that runs at INSERT time for omitted shortid + values doesn't run at column-materialisation time. + +4. **No UNIQUE constraint emission today.** A non-PK serial + column would need a UNIQUE constraint to enforce its contract + (the rowid trick isn't available off the PK). The same + applies to non-PK shortid: today it relies on a probabilistic + "won't collide" argument, not a database-enforced + guarantee. `schema_to_ddl` only emits NOT NULL inline plus PK + inline / table-level. No path emits UNIQUE. + +This ADR resolves all four gaps with a single unifying +principle. + +## Decision + +### 1. The unifying principle + +> Auto-generated column types honour their generation contract +> on every path that creates or transitions the column. + +Concretely: a column declared (or converted to be) `serial` or +`shortid` always satisfies its contract — non-null, +auto-generated, unique — by the time the operation completes. +The user does not have to issue a follow-up UPDATE. The +mechanism is hidden; the user-facing model is "values appear +automatically". + +### 2. `serial`: dual-implementation, single semantic + +`serial` is generalised from "auto-incrementing PK" to +"auto-incrementing integer column". The column may be the table +PK or any non-PK column; the user-facing semantic is identical. + +The implementation switches transparently: + +- **PK case** (single-column PK on this column): rowid alias. + `INTEGER PRIMARY KEY` in DDL; SQLite's free auto-increment + applies. Unchanged from today. +- **Non-PK case**: app-level `MAX(col) + 1` lookup at INSERT + time, plus an emitted UNIQUE constraint on the column. The + worker-thread serialisation (ADR-0010) makes the read-then- + insert sequence safe without explicit locking — only one + INSERT runs at a time on the connection. + +User-visible help, error messages, and `[client-side]` notes +refer to `serial` columns as "auto-incrementing" or +"auto-generated". The PK / non-PK distinction is an internal +implementation detail (ADR-0002 user-facing posture). + +### 3. `shortid`: tighten the contract at column materialisation + +Today: shortid generation runs only when an INSERT omits the +value. Rows existing at the moment a shortid column is created +remain NULL until the user issues an UPDATE. + +Going forward: any null cell in a shortid column gets a freshly- +generated value at the operation that creates that condition: + +- `add column T: x (shortid)` on a non-empty table fills every + existing row's `x` with a generated shortid before the + operation completes. +- `change column T: x (shortid)` from `text` (or any other + matrix-permitted source) fills any null cells with generated + shortids in the same rebuild transaction. + +Generator collisions (vanishingly rare given the 10⁷–10⁸ +namespace; see ADR-0014 §"shortid auto-generation") trigger up +to 5 retries per cell. Exhausting retries fails the operation +with a friendly diagnostic; in practice this indicates either a +generator-state bug or a pathological RNG and is not user- +recoverable. + +### 4. UNIQUE story + +Auto-generated non-PK columns gain an emitted UNIQUE constraint +to enforce their contract: + +- Non-PK `serial`: gains UNIQUE on creation / conversion-to- + serial. Required for the contract; the rowid trick isn't + available off the PK. +- Non-PK `shortid`: gains UNIQUE on creation / conversion-to- + shortid. Strengthens today's probabilistic guarantee into a + database-enforced one. +- PK case for either type: PK already implies UNIQUE+NOT NULL. + No additional constraint needed. + +The reverse direction (`serial → int`, `shortid → text`) leaves +the UNIQUE constraint **in place**. The user has not signalled +intent to drop the uniqueness guarantee; only the auto- +generation contract was dropped. When constraint-management +lands as a user-facing feature (C3-track), the user can +explicitly drop the UNIQUE if desired. + +This ADR pulls forward the **internal infrastructure** to emit +and read UNIQUE constraints — `schema_to_ddl` gains UNIQUE- +column-clause emission; `read_schema` gains UNIQUE detection +via `pragma_index_list` + `pragma_index_info`; `ReadColumn` +gains a `unique: bool` field. The **user-facing constraint +surface** (declaring UNIQUE in `with pk … unique …` or via +`add unique`, dropping UNIQUE, naming UNIQUE constraints) is +not in scope here and remains C3-track work. + +### 5. INSERT-path changes + +For non-PK `serial` columns, when the column is omitted from +an INSERT (the existing skip-list at db.rs:3111 already covers +serial and shortid identically), the executor: + +1. Queries `SELECT COALESCE(MAX(col), 0) + 1 FROM T` inside the + same transaction. +2. Binds the result as the column's value. + +The MAX-based seeding mirrors SQLite's rowid behaviour: gaps +left by user-supplied explicit values are jumped over (the next +auto-fill is `MAX + 1`, not "the smallest available integer"). + +Worker-thread serialisation (ADR-0010) prevents the classic +read-modify-write race; the pattern is safe for our single- +writer model. + +### 6. `add_column` changes + +`do_add_column` lifts its blanket serial refusal (db.rs:1374). +The new behaviour is determined by the source table's state: + +- **`add column T: x (serial)` on an empty table**: emit + `ALTER TABLE T ADD COLUMN x INTEGER UNIQUE`. Every table has + a PK by construction (the parser refuses `create table` + without `with pk`), so the "no PK" branch doesn't arise — + the new column joins as a non-PK serial. +- **`add column T: x (serial)` on a non-empty table**: route + through the rebuild-table primitive (ADR-0013). Create new + table with `x INTEGER UNIQUE`. Copy rows, filling `x` with + values 1..N in declaration order. Emit a `[client-side]` note + (§7). +- **`add column T: x (shortid)` on a non-empty table**: route + through the rebuild-table primitive. Create new table with + `x TEXT UNIQUE`. Copy rows, generating a fresh shortid for + each (collision-retried per §3). Emit a `[client-side]` note. + +The empty-table path can stay on `ALTER TABLE ADD COLUMN` for +efficiency; the non-empty path needs the rebuild because we +need to populate the new column atomically with table +creation. + +### 7. `change column` to `serial` / `shortid` + +`change column T: x (serial)` from any matrix-permitted source +type (today: `int`; future expansions follow the same rule): + +1. Run the per-cell dry-run (ADR-0017 §2). For non-null cells, + classify via the transformer matrix: source must produce an + integer (the existing serial pre-condition). +2. Refuse if existing non-null values have duplicates + (uniqueness collision, ADR-0017 §4.3). +3. Auto-fill any null cells with sequential values continuing + from `MAX(non-null values) + 1` (or starting at 1 if none). +4. Refuse if the auto-fill would itself produce a collision — + in practice, this can only happen if the user supplied + non-null values that already overlap the would-be sequence + (e.g., existing values [1, 2, 5] with two nulls — fill would + be 6 and 7, no collision; existing values [1, 2, 6] with + nulls — fill would be 3 and 4, no collision; the sequence + uses MAX+1, not gap-filling, so this case doesn't actually + arise — but state the rule defensively). +5. Rebuild the table with the new column type plus UNIQUE (per + §4) plus the transformed + auto-filled values. +6. Emit `[client-side]` notes (§7). + +`change column T: x (shortid)` from `text`: + +1. Run the per-cell dry-run. Non-null cells classify via the + text → shortid transformer (ADR-0017 §3) — must match the + shortid grammar. +2. Refuse if existing non-null shortid-valid values have + duplicates. +3. Auto-fill null cells with generated shortids (collision- + retried per §3, including against the existing values). +4. Rebuild with TEXT + UNIQUE + the validated + auto-filled + values. +5. Emit `[client-side]` notes. + +### 8. Conversion matrix amendments to ADR-0017 + +ADR-0017 §3 "Statically refused" is amended: + +- `int → serial` is **removed** from the static refusal list and + added as a **per-cell-classified** matrix entry: clean for + non-null integers (with the post-transformation uniqueness + check from §4.3), with null-cell auto-fill per §7 above. +- The general "Anything → `serial`" refusal is replaced with a + more specific list: `text → serial`, `real → serial`, etc. + remain refused for v1 (route via int first); `bool → serial` + remains refused (cross-domain). +- `text → shortid` is unchanged from ADR-0017 (still per-cell- + classified). The contract enforcement at column-materialisation + is new. + +ADR-0017 §4.3 (uniqueness check) is amended to apply to +"PK columns and shortid columns and any column that gains a +UNIQUE constraint as part of the operation" — i.e., non-PK +serial / shortid targets are uniqueness-checked. + +### 9. Client-side notes + +ADR-0017 §6 introduced the `[client-side]` pattern: when the +playground rewrote any cell value, the success summary tells +the learner "the tool did this for you; raw SQL would need a +`CAST` or application-level code." This ADR extends the pattern +to auto-fill operations: + +- **`add column T: x (serial)` on non-empty table**: + > [client-side] N row(s) given auto-generated serial values + > 1..N. In raw SQL this would need an explicit UPDATE to + > populate. + +- **`add column T: x (shortid)` on non-empty table**: + > [client-side] N row(s) given auto-generated shortid values. + > In raw SQL this would need an explicit UPDATE to populate. + +- **`change column T: x (serial)` with M null cells**: + > [client-side] M null cell(s) given auto-generated serial + > values. In raw SQL this would need an explicit UPDATE to + > populate. + +- **`change column T: x (shortid)` with M null cells**: + > [client-side] M null cell(s) given auto-generated shortid + > values. In raw SQL this would need an explicit UPDATE to + > populate. + +When both an ADR-0017 transformation note AND an ADR-0018 +auto-fill note apply to the same operation (e.g., `change +column T: x (shortid)` from text where some cells need +validation and others need auto-fill), both notes are emitted +on separate lines. The success path emits them after the `[ok]` +summary and before the structure-render block. + +### 10. Engine-vocabulary cleanup + +While here, fix the existing user-facing string in +`do_add_column`'s serial refusal (db.rs:1374): the message +names "SQLite's ALTER TABLE" — an ADR-0002 user-facing posture +violation. This message is being replaced anyway as part of +lifting the refusal; the replacement uses abstract "the +database" / "the engine" phrasing. + +## Resolutions + +Three points called out as "open" during drafting, resolved +before acceptance: + +1. **No-PK empty-table case**: not reachable. Every table has + a PK by construction — the `create table` parser refuses + input that produces an empty PK list. `add column T: x + (serial)` on an empty table therefore always lands on a + table that already has a PK, and the new `x` column is a + non-PK serial (gains UNIQUE per §4). + +2. **Serial sequencing under explicit user inserts**: MAX+1. + If the user explicitly inserts `id = 100`, the next auto- + fill yields 101. Gappy sequences are accepted (e.g., if + the user later inserts `id = 200`, the next auto-fill is + 201; the gap 102..199 is not back-filled). MAX+1 matches + SQLite's rowid behaviour for the PK case, so both + implementation paths feel uniform to the user, and gap- + detection is more expensive than its pedagogical value. + +3. **UNIQUE emission style**: inline column constraint + (`x INTEGER UNIQUE`). Cleaner DDL while we don't have a + user-facing constraint surface that would benefit from + named, separately-managed indexes. Revisitable when C3 + lands the user-facing constraint feature; the + `read_schema` detection via `pragma_index_list` works for + either form. + +## Out of scope + +- **OOS-1.** User-facing UNIQUE constraint surface (`add + unique : `, `drop unique`, naming, multi-column unique). + Stays as C3-track work. +- **OOS-2.** Strict-monotonic AUTOINCREMENT semantics — we + retain plain `INTEGER PRIMARY KEY` for PK serial. Rebuild- + reset of the high-water mark is acceptable for a teaching + tool; users who care can be taught the distinction in a + later iteration. +- **OOS-3.** Custom serial start values, custom step sizes, or + multi-column composite serial. +- **OOS-4.** Non-PK serial when the table has no PK at all + (caught by Open Question 1's resolution). +- **OOS-5.** A `[client-side]` note on the empty-table case + (`add column` on an empty table). No rows means nothing to + auto-fill — the operation is a structural change with no + pedagogical "the tool did this for you" content. +- **OOS-6.** Reading and emitting CHECK constraints — only + UNIQUE is required for this ADR. + +## Consequences + +- The "serial only on PK" mental model is replaced with + "serial works anywhere". Pedagogically richer: students see + auto-incrementing columns as a general feature, not as a + special PK-only quirk. +- One internal mechanism the user doesn't see (rowid alias vs + application MAX+1). The two paths converge to identical + user-facing behaviour, honouring ADR-0002's posture. +- `schema_to_ddl` and `read_schema` gain UNIQUE handling — a + partial pull-forward of C3 work. The user-facing constraint + surface stays deferred; this ADR only lands the internal + infrastructure required by serial / shortid contracts. +- `[client-side]` notes proliferate to cover auto-fill cases. + Strengthens the pedagogical lens: every place the playground + goes beyond what raw SQL does, the user is told. +- All four user-observed gaps from §Context closed. The + `int → serial → int` round-trip works (matching the existing + `text → shortid → text` round-trip from ADR-0017). +- Add-column-with-shortid producing a "valid" state aligns + with ADR-0005's design contract that shortids are unique + non-null identifiers. + +## Relationship to earlier ADRs + +- **ADR-0002** — User-facing posture honoured: the dual + serial implementation is hidden; the existing engine-name + leak in `do_add_column`'s refusal message is fixed + opportunistically. +- **ADR-0005** — Type vocabulary unchanged; `serial` definition + generalised. The keyword and the user model stay the same; + the implementation broadens. +- **ADR-0010** — Worker-thread serialisation is what makes the + non-PK serial MAX+1 path safe without explicit locks. +- **ADR-0011** — `fk_target_type` for serial unchanged + (`Serial → Int`); FK target compatibility remains as-is. +- **ADR-0013** — Rebuild-table primitive carries the auto-fill + cases for non-empty `add column` and `change column to + serial/shortid`. +- **ADR-0014** — INSERT-time auto-fill semantics extended to + non-PK serial. ADR-0014's auto-fill skip-list (which already + covers both serial and shortid symmetrically) is reused. +- **ADR-0015** — The text-format round-trip carries the new + UNIQUE constraints in metadata so a rebuild from + `project.yaml` reconstructs the database faithfully. Likely + needs a `__rdbms_playground_columns` schema additon (a + `unique` bool) — to be confirmed during implementation. +- **ADR-0017** — §3 transformer matrix amended: `int → serial` + joins per-cell-classified. §4.3 uniqueness check extended to + cover non-PK serial / shortid targets. diff --git a/docs/adr/README.md b/docs/adr/README.md index f684bd0..3f20739 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -23,3 +23,4 @@ This directory contains the project's ADRs, recorded per - [ADR-0015 — Project storage runtime](0015-project-storage-runtime.md) - [ADR-0016 — Pretty table rendering for data and structure views](0016-pretty-table-rendering.md) - [ADR-0017 — Column type-change compatibility](0017-column-type-change-compatibility.md) +- [ADR-0018 — Auto-fill contracts for `serial` and `shortid` columns](0018-auto-fill-contracts-for-serial-and-shortid.md) diff --git a/src/app.rs b/src/app.rs index bd6650c..733bc6b 100644 --- a/src/app.rs +++ b/src/app.rs @@ -13,8 +13,8 @@ use tracing::{trace, warn}; use crate::action::Action; use crate::db::{ - CascadeEffect, ChangeColumnTypeResult, DataResult, DeleteResult, InsertResult, - TableDescription, UpdateResult, + AddColumnResult, CascadeEffect, ChangeColumnTypeResult, DataResult, DeleteResult, + InsertResult, TableDescription, UpdateResult, }; use crate::dsl::{Command, ParseError, parse_command}; use crate::event::AppEvent; @@ -317,6 +317,10 @@ impl App { self.handle_dsl_change_column_success(&command, result); Vec::new() } + AppEvent::DslAddColumnSucceeded { command, result } => { + self.handle_dsl_add_column_success(&command, result); + Vec::new() + } AppEvent::DslFailed { command, error } => { self.handle_dsl_failure(&command, &error); Vec::new() @@ -782,6 +786,26 @@ impl App { } } + fn handle_dsl_add_column_success( + &mut self, + command: &Command, + result: AddColumnResult, + ) { + let summary = format!("[ok] {} {}", command.verb(), command.display_subject()); + self.note_system(summary); + // ADR-0018 §9: emit auto-fill note(s) before the + // structure render, so the pedagogical "the tool did + // this for you" line is in the user's eye-line next to + // the success summary. + for note in result.client_side_notes { + self.note_system(note); + } + for line in crate::output_render::render_structure(&result.description) { + self.note_system(line); + } + self.current_table = Some(result.description); + } + fn handle_dsl_change_column_success( &mut self, command: &Command, @@ -790,28 +814,44 @@ impl App { let summary = format!("[ok] {} {}", command.verb(), command.display_subject()); self.note_system(summary); if let Some(note) = result.client_side { - // ADR-0017 §6: pedagogical hook telling the learner - // "the tool did this for you; raw SQL would need a - // CAST or application-level code." Lossy variant - // adds the lossy count when --force-conversion was - // used. - let line = if note.lossy > 0 { - format!( - "[client-side] {n} row(s) transformed; {l} of those discarded \ - information (lossy). In raw SQL this would need an explicit \ - `CAST` or application-level code.", - n = note.transformed, - l = note.lossy - ) - } else { - format!( - "[client-side] {n} row(s) were transformed before being stored. \ - In raw SQL this would need an explicit `CAST` or \ - application-level code.", - n = note.transformed - ) - }; - self.note_system(line); + // ADR-0017 §6 + ADR-0018 §9: pedagogical hook + // telling the learner "the tool did this for you; + // raw SQL would need explicit CAST / UPDATE / etc." + // + // When both transformations and auto-fills happen + // in the same operation, both note lines are + // emitted in order (ADR-0018 §9 explicit rule). + if note.transformed > 0 { + let line = if note.lossy > 0 { + format!( + "[client-side] {n} row(s) transformed; {l} of those discarded \ + information (lossy). In raw SQL this would need an explicit \ + `CAST` or application-level code.", + n = note.transformed, + l = note.lossy + ) + } else { + format!( + "[client-side] {n} row(s) were transformed before being stored. \ + In raw SQL this would need an explicit `CAST` or \ + application-level code.", + n = note.transformed + ) + }; + self.note_system(line); + } + if note.auto_filled > 0 { + let kind = match note.auto_fill_kind { + Some(crate::db::AutoFillKind::Serial) => "serial", + Some(crate::db::AutoFillKind::ShortId) => "shortid", + None => "auto-generated", + }; + self.note_system(format!( + "[client-side] {m} null cell(s) given auto-generated {kind} values. \ + In raw SQL this would need an explicit UPDATE to populate.", + m = note.auto_filled, + )); + } } for line in crate::output_render::render_structure(&result.description) { self.note_system(line); diff --git a/src/db.rs b/src/db.rs index b0d49d5..61df977 100644 --- a/src/db.rs +++ b/src/db.rs @@ -23,7 +23,6 @@ //! (H1). For now `friendly_message()` is a passthrough; when H1 //! lands the body of that method becomes the translation table. -use std::fmt::Write as _; use std::path::Path; use std::thread; @@ -162,6 +161,19 @@ pub struct InsertResult { pub data: DataResult, } +/// Outcome of a successful `add column …`. +/// +/// Carries the post-add structure (used for the auto-show that +/// follows DDL) plus zero or one pre-rendered `[client-side]` +/// note lines (ADR-0018 §9). Only the auto-fill paths +/// (`add column T: x (serial|shortid)` on a non-empty table) +/// produce notes. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct AddColumnResult { + pub description: TableDescription, + pub client_side_notes: Vec, +} + /// Outcome of a successful `change column …` (ADR-0017 §6). /// /// `description` is the post-rebuild table structure (used for @@ -183,11 +195,31 @@ pub struct ClientSideNote { /// Cells whose stored value differs from what the user /// originally inserted (any non-identity transformation, /// including pure storage-class changes like - /// `Text("42")` → `Integer(42)`). + /// `Text("42")` → `Integer(42)`). Auto-filled cells (per + /// ADR-0018) are NOT counted here — they're tracked in + /// `auto_filled` separately so the success summary can + /// distinguish "the tool transformed your data" from "the + /// tool generated values where you had nulls." pub transformed: usize, /// Subset of `transformed` where information was discarded /// (only non-zero when `--force-conversion` was used). pub lossy: usize, + /// Null cells filled with auto-generated values when the + /// target type carries an auto-generation contract + /// (`serial` / `shortid`). ADR-0018 §3 / §7. + pub auto_filled: usize, + /// What kind of value was auto-generated; drives the note + /// wording (per ADR-0018 §9). `None` when `auto_filled` is + /// zero. + pub auto_fill_kind: Option, +} + +/// Whether an auto-fill emitted serial sequence values or +/// generated shortids. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum AutoFillKind { + Serial, + ShortId, } /// Outcome of a successful UPDATE — a count plus the rows that @@ -306,7 +338,7 @@ enum Request { column: String, ty: Type, source: Option, - reply: oneshot::Sender>, + reply: oneshot::Sender>, }, DropColumn { table: String, @@ -468,7 +500,7 @@ impl Database { column: String, ty: Type, source: Option, - ) -> Result { + ) -> Result { let (reply, recv) = oneshot::channel(); self.send(Request::AddColumn { table, @@ -1075,6 +1107,7 @@ fn read_schema_snapshot(conn: &Connection) -> Result { .iter() .map(|c| ColumnSchema { name: c.name.clone(), + unique: c.unique, // user_type is always populated for tables we // created; the fallback is defensive. user_type: c.user_type.unwrap_or(Type::Text), @@ -1156,6 +1189,7 @@ fn read_table_snapshot( .map(|c| ColumnSchema { name: c.name.clone(), user_type: c.user_type.unwrap_or(Type::Text), + unique: c.unique, }) .collect(); let column_idents: Vec = read @@ -1363,6 +1397,25 @@ fn do_drop_table( Ok(()) } +/// Add a column to an existing table. +/// +/// Two execution paths per ADR-0018 §6: +/// +/// - **Plain types** (`text`, `int`, `real`, `decimal`, `bool`, +/// `date`, `datetime`): `ALTER TABLE ADD COLUMN`. Existing +/// rows get NULL in the new column — that's the SQL standard +/// behaviour and acceptable for non-auto-generated types. +/// - **Auto-generated types** (`serial`, `shortid`): route +/// through the rebuild-table primitive so we can populate +/// the new column atomically with table creation. Existing +/// rows receive a generated value (1..N for serial, fresh +/// shortids for shortid) and the column gains UNIQUE + +/// NOT NULL. +/// +/// `blob` is statically refused as a column-add target by the +/// existing infrastructure (no DSL literal, downstream INSERT +/// would fail), but the path itself is allowed via the plain +/// branch — same as today. fn do_add_column( conn: &Connection, persistence: Option<&Persistence>, @@ -1370,25 +1423,29 @@ fn do_add_column( table: &str, column: &str, ty: Type, -) -> Result { - if ty == Type::Serial { - return Err(DbError::Unsupported( - "the 'serial' type carries auto-increment primary-key semantics \ - that SQLite's ALTER TABLE ADD COLUMN cannot apply. Specify \ - `serial` at create-table time via `with pk` instead." - .to_string(), - )); +) -> Result { + let auto_generated = matches!(ty, Type::Serial | Type::ShortId); + if !auto_generated { + return do_add_plain_column(conn, persistence, source, table, column, ty); } - let mut ddl = String::new(); - write!( - ddl, - "ALTER TABLE {tbl} ADD COLUMN {col} {sqlite_type}{extra};", + do_add_auto_generated_column(conn, persistence, source, table, column, ty) +} + +/// Plain ALTER-TABLE path for non-auto-generated types. +fn do_add_plain_column( + conn: &Connection, + persistence: Option<&Persistence>, + source: Option<&str>, + table: &str, + column: &str, + ty: Type, +) -> Result { + let ddl = format!( + "ALTER TABLE {tbl} ADD COLUMN {col} {sqlite_type};", tbl = quote_ident(table), col = quote_ident(column), sqlite_type = ty.sqlite_strict_type(), - extra = ty.sqlite_strict_extra(), - ) - .expect("write to String never fails"); + ); debug!(ddl = %ddl, "add_column"); let tx = conn .unchecked_transaction() @@ -1410,7 +1467,192 @@ fn do_add_column( }; finalize_persistence(conn, persistence, source, &changes)?; tx.commit().map_err(DbError::from_rusqlite)?; - Ok(description) + Ok(AddColumnResult { + description, + client_side_notes: Vec::new(), + }) +} + +/// Auto-fill path for `serial` / `shortid` (ADR-0018 §6 + §9). +/// +/// Empty table: the new column is added through the rebuild +/// primitive too, but no auto-fill / `[client-side]` note is +/// produced — there are no rows to populate. +fn do_add_auto_generated_column( + conn: &Connection, + persistence: Option<&Persistence>, + source: Option<&str>, + table: &str, + column: &str, + ty: Type, +) -> Result { + use rusqlite::types::Value as RV; + + let old_schema = read_schema(conn, table)?; + if old_schema.columns.iter().any(|c| c.name == column) { + return Err(DbError::Unsupported(format!( + "column `{table}.{column}` already exists." + ))); + } + let row_count = count_rows(conn, table)? as usize; + + // Build the new schema with the auto-generated column + // appended. UNIQUE + NOT NULL emit per ADR-0018 §4. + let mut new_schema = old_schema.clone(); + new_schema.columns.push(ReadColumn { + name: column.to_string(), + sqlite_type: ty.sqlite_strict_type().to_string(), + notnull: true, + primary_key: false, + unique: true, + user_type: Some(ty), + }); + + // Generate auto-fill values for every existing row. + let auto_fill_values: Vec = match ty { + Type::Serial => (1..=row_count as i64).map(RV::Integer).collect(), + Type::ShortId => generate_shortid_batch(row_count, &[])?, + _ => unreachable!("guarded by `auto_generated` flag"), + }; + + let old_columns: Vec = old_schema.columns.iter().map(|c| c.name.clone()).collect(); + let new_columns: Vec = new_schema.columns.iter().map(|c| c.name.clone()).collect(); + + let copy_data = |tx: &rusqlite::Transaction<'_>, + temp_name: &str, + orig: &str| + -> Result<(), DbError> { + if row_count == 0 { + return Ok(()); + } + // Read all rows from old, append the auto-fill value, + // INSERT into temp. + let select_cols = old_columns + .iter() + .map(|c| quote_ident(c)) + .collect::>() + .join(", "); + let select_sql = format!( + "SELECT {select_cols} FROM {orig};", + orig = quote_ident(orig), + ); + let cols_csv = new_columns + .iter() + .map(|c| quote_ident(c)) + .collect::>() + .join(", "); + let placeholders = (1..=new_columns.len()) + .map(|i| format!("?{i}")) + .collect::>() + .join(", "); + let insert_sql = format!( + "INSERT INTO {temp} ({cols_csv}) VALUES ({placeholders});", + temp = quote_ident(temp_name), + ); + + let mut select_stmt = tx.prepare(&select_sql).map_err(DbError::from_rusqlite)?; + let mut rows = select_stmt.query([]).map_err(DbError::from_rusqlite)?; + let mut insert_stmt = tx.prepare(&insert_sql).map_err(DbError::from_rusqlite)?; + let mut row_idx = 0usize; + while let Some(r) = rows.next().map_err(DbError::from_rusqlite)? { + let mut values: Vec = Vec::with_capacity(new_columns.len()); + for i in 0..old_columns.len() { + values.push(r.get(i).map_err(DbError::from_rusqlite)?); + } + values.push(auto_fill_values[row_idx].clone()); + let params: Vec<&dyn rusqlite::ToSql> = + values.iter().map(|v| v as &dyn rusqlite::ToSql).collect(); + insert_stmt + .execute(rusqlite::params_from_iter(params)) + .map_err(DbError::from_rusqlite)?; + row_idx += 1; + } + Ok(()) + }; + + 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, column, 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_with_copy(conn, table, &new_schema, copy_data, metadata_updates)?; + + let description = do_describe_table(conn, table)?; + let mut client_side_notes = Vec::new(); + if row_count > 0 { + client_side_notes.push(format_auto_fill_add_note(ty, row_count)); + } + Ok(AddColumnResult { + description, + client_side_notes, + }) +} + +/// 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 +/// 5 retries per cell per ADR-0018 §3. +fn generate_shortid_batch( + count: usize, + existing: &[String], +) -> Result, DbError> { + use std::collections::HashSet; + let mut taken: HashSet = existing.iter().cloned().collect(); + let mut out: Vec = Vec::with_capacity(count); + for _ in 0..count { + let mut generated: Option = None; + for _ in 0..5 { + let candidate = shortid::generate(); + if !taken.contains(&candidate) { + taken.insert(candidate.clone()); + generated = Some(candidate); + break; + } + } + match generated { + Some(v) => out.push(rusqlite::types::Value::Text(v)), + None => { + return Err(DbError::Unsupported( + "could not generate a unique shortid after 5 attempts; \ + this typically indicates a generator-state issue, not \ + a recoverable user error." + .to_string(), + )); + } + } + } + Ok(out) +} + +/// `[client-side]` note line for `add column T: x (serial|shortid)` +/// on a non-empty table per ADR-0018 §9. +fn format_auto_fill_add_note(ty: Type, row_count: usize) -> String { + match ty { + Type::Serial => format!( + "[client-side] {row_count} row(s) given auto-generated serial \ + values 1..{row_count}. In raw SQL this would need an explicit \ + UPDATE to populate." + ), + Type::ShortId => format!( + "[client-side] {row_count} row(s) given auto-generated shortid \ + values. In raw SQL this would need an explicit UPDATE to \ + populate." + ), + _ => unreachable!("called only for serial/shortid"), + } } /// Drop a column from a table. @@ -1688,8 +1930,14 @@ fn do_change_column_type( } // Build new_schema: same as old, but the target column's - // user_type / sqlite_type are updated. PK and notnull - // flags carry over unchanged. + // user_type / sqlite_type are updated. PK / notnull flags + // carry over unchanged. UNIQUE handling per ADR-0018 §4: + // + // - Non-PK serial / shortid target: gain UNIQUE. + // - Reverse direction (e.g. serial → int): keep whatever + // UNIQUE flag was already present (ADR-0018 §4 "leaves + // UNIQUE in place"), which the clone() above already + // does. let mut new_schema = old_schema.clone(); { let target = new_schema @@ -1699,6 +1947,13 @@ fn do_change_column_type( .expect("column existence checked above"); target.user_type = Some(ty); target.sqlite_type = ty.sqlite_strict_type().to_string(); + if matches!(ty, Type::Serial | Type::ShortId) && !target.primary_key { + target.unique = true; + // Auto-generated columns are non-null by contract + // (ADR-0018 §3). Set the flag here so schema_to_ddl + // emits NOT NULL during the rebuild. + target.notnull = true; + } } let ty_keyword = ty.keyword(); @@ -1744,11 +1999,12 @@ fn do_change_column_type( )?, ), }; - // Client-side note fires only when at least one cell was - // materially transformed. A pure metadata change (e.g. - // identity-clean transformer over zero rows, or zero - // non-identity outcomes) emits no note (ADR-0017 §6). - let client_side = client_side.filter(|note| note.transformed > 0); + // Client-side note fires when at least one cell was + // materially transformed (ADR-0017 §6) OR at least one + // null cell was auto-filled (ADR-0018 §9). A pure metadata + // change (zero non-identity outcomes, no auto-fill) emits + // no note. + let client_side = client_side.filter(|note| note.transformed > 0 || note.auto_filled > 0); let description = do_describe_table(conn, table)?; Ok(ChangeColumnTypeResult { @@ -1839,19 +2095,39 @@ where } } - // Classify every row's target cell. + // Classify every row's target cell. Null cells targeting + // an auto-generated type get a placeholder outcome here + // and are filled in below once the full set of non-null + // values is known (so serial sequencing can start at + // `MAX + 1`, and shortid generation can avoid collisions + // against existing values). + let auto_filling = matches!(target_ty, Type::Serial | Type::ShortId); let mut outcomes: Vec = Vec::with_capacity(rows.len()); for row in &rows { let pk_values: Vec = pk_indices.iter().map(|i| row[*i].clone()).collect(); let original = row[target_idx].clone(); - let outcome = type_change::transform_cell(src_ty, target_ty, &original); + let (outcome, is_auto_fill) = if auto_filling && matches!(original, RV::Null) { + // Placeholder; replaced after the loop. + (CellOutcome::Clean(RV::Null), true) + } else { + (type_change::transform_cell(src_ty, target_ty, &original), false) + }; outcomes.push(Outcome { pk_values, original, outcome, + is_auto_fill, }); } + // Auto-fill pass for serial / shortid targets per ADR-0018 + // §3 / §7. Runs before the incompatible / lossy / collision + // checks so the filled values participate in those checks + // as if they were ordinary classified outcomes. + if auto_filling { + fill_auto_generated_cells(target_ty, &mut outcomes)?; + } + // Refuse on incompatibles. let incompatibles: Vec<&Outcome> = outcomes .iter() @@ -1886,13 +2162,18 @@ where ))); } - // Uniqueness check for PK / shortid columns. - let target_col_info = old_schema + // Uniqueness check for any target column that will carry + // a uniqueness constraint in the new schema. ADR-0017 §4.3 + // covered PK + shortid; ADR-0018 §4.3-amendment extends + // this to "any column that gains a UNIQUE constraint as + // part of the operation" — i.e., the new schema's target + // column has primary_key OR unique set. + let new_target = new_schema .columns .iter() .find(|c| c.name == column) - .expect("target column exists"); - let uniqueness_required = target_col_info.primary_key || target_ty == Type::ShortId; + .expect("target column exists in new schema"); + let uniqueness_required = new_target.primary_key || new_target.unique; if uniqueness_required && let Some(error_msg) = check_uniqueness_collisions( table, @@ -1956,22 +2237,113 @@ where rebuild_table_with_copy(conn, table, new_schema, copy_data, metadata_updates)?; - // Tally counts for the [client-side] note. A cell counts - // as "transformed" if its stored value materially differs - // from the original (per ADR-0017 §6 / DA #5: storage class - // changes count). Lossy is a strict subset. + // Tally counts for the [client-side] note. Three kinds of + // cells of interest (ADR-0017 §6 + ADR-0018 §9): + // + // - auto-filled: original was NULL, target type + // auto-generates. Counted under `auto_filled`, NOT + // `transformed` (different user-facing wording). + // - transformed: original was non-null and the stored + // value materially differs (storage class change + // counts). + // - lossy: subset of transformed; only when + // --force-conversion was used. let mut transformed = 0usize; let mut lossy = 0usize; + let mut auto_filled = 0usize; for (idx, o) in outcomes.iter().enumerate() { let new = &transformed_values[idx]; - if type_change::is_non_identity(&o.original, new) { + if o.is_auto_fill { + auto_filled += 1; + } else if type_change::is_non_identity(&o.original, new) { transformed += 1; } if matches!(o.outcome, CellOutcome::Lossy { .. }) { lossy += 1; } } - Ok(ClientSideNote { transformed, lossy }) + let auto_fill_kind = if auto_filled > 0 { + Some(match target_ty { + Type::Serial => AutoFillKind::Serial, + Type::ShortId => AutoFillKind::ShortId, + _ => unreachable!("auto-fill only fires for Serial / ShortId targets"), + }) + } else { + None + }; + Ok(ClientSideNote { + transformed, + lossy, + auto_filled, + auto_fill_kind, + }) +} + +/// Replace the placeholder `Clean(Null)` outcomes for null +/// source cells with auto-generated values. +/// +/// For `Serial` targets: continue the integer sequence from +/// `MAX(non-null value) + 1`. For `ShortId` targets: generate +/// fresh shortids that don't collide with existing values or +/// with one another (5-retry budget per cell, ADR-0018 §3). +fn fill_auto_generated_cells( + target_ty: Type, + outcomes: &mut [Outcome], +) -> Result<(), DbError> { + use rusqlite::types::Value as RV; + use type_change::CellOutcome; + + match target_ty { + Type::Serial => { + let max_existing: i64 = outcomes + .iter() + .filter_map(|o| { + if o.is_auto_fill { + return None; + } + match &o.outcome { + CellOutcome::Clean(RV::Integer(i)) => Some(*i), + _ => None, + } + }) + .max() + .unwrap_or(0); + let mut next = max_existing.saturating_add(1); + for o in outcomes.iter_mut().filter(|o| o.is_auto_fill) { + o.outcome = CellOutcome::Clean(RV::Integer(next)); + next = next.saturating_add(1); + } + } + Type::ShortId => { + let existing: Vec = outcomes + .iter() + .filter_map(|o| { + if o.is_auto_fill { + return None; + } + match &o.outcome { + CellOutcome::Clean(RV::Text(s)) => Some(s.clone()), + _ => None, + } + }) + .collect(); + let auto_fill_count = outcomes.iter().filter(|o| o.is_auto_fill).count(); + let new_values = generate_shortid_batch(auto_fill_count, &existing)?; + for (idx, o) in outcomes + .iter_mut() + .filter(|o| o.is_auto_fill) + .enumerate() + { + o.outcome = CellOutcome::Clean(new_values[idx].clone()); + } + } + _ => { + // Caller filters to Serial / ShortId; the + // unreachable branch is defensive. + unreachable!("fill_auto_generated_cells called with non-auto-gen target"); + } + } + Ok(()) } /// Wrap an engine-level error from the `--dont-convert` path @@ -2221,6 +2593,11 @@ struct Outcome { pk_values: Vec, original: rusqlite::types::Value, outcome: type_change::CellOutcome, + /// `true` when the original cell was NULL and the target + /// type carries an auto-generation contract; the `outcome` + /// gets filled in with the generated value during the + /// post-classification auto-fill pass. + is_auto_fill: bool, } fn pk_header_cells(pk_columns: &[String]) -> Vec { @@ -2315,6 +2692,14 @@ struct ReadColumn { sqlite_type: String, notnull: bool, primary_key: bool, + /// `true` when this column carries a single-column UNIQUE + /// constraint detected via `pragma_index_list` / + /// `pragma_index_info` (origin = "u"). PK columns are not + /// marked unique here even though PK implies UNIQUE — the + /// `primary_key` flag covers that, and `schema_to_ddl` + /// avoids double-emitting. Compound UNIQUE is out of scope + /// for v1 (ADR-0018 OOS-6 / future C3 work). + unique: bool, user_type: Option, } @@ -2347,6 +2732,7 @@ fn read_schema(conn: &Connection, table: &str) -> Result { sqlite_type: row.get(1)?, notnull: row.get::<_, i64>(2)? != 0, primary_key: row.get::<_, i64>(3)? != 0, + unique: false, // filled in below from pragma_index_list user_type, }) }) @@ -2367,6 +2753,19 @@ fn read_schema(conn: &Connection, table: &str) -> Result { .map(|c| c.name.clone()) .collect(); + // Detect single-column UNIQUE constraints (ADR-0018 §4). + // pragma_index_list returns one row per index; we filter to + // unique indexes whose origin is "u" (a UNIQUE constraint, + // as opposed to "pk" or "c"). For each, pragma_index_info + // gives the constituent column(s); we only mark single- + // column unique here. Compound UNIQUE is out of scope. + let unique_columns = read_unique_columns(conn, table)?; + for col in &mut columns { + if unique_columns.contains(&col.name) { + col.unique = true; + } + } + // Foreign keys from pragma_foreign_key_list. let mut fk_stmt = conn .prepare( @@ -2407,6 +2806,54 @@ fn parse_action_from_sqlite(s: &str) -> ReferentialAction { .unwrap_or(ReferentialAction::NoAction) } +/// Read the set of column names carrying a single-column UNIQUE +/// constraint on `table`, via `pragma_index_list` + +/// `pragma_index_info`. Filters to indexes whose `origin` is +/// `"u"` (a UNIQUE constraint, not a PK-implied or CHECK +/// auto-index) and which cover exactly one column. Compound +/// UNIQUE is deferred to a future ADR (out of scope for ADR-0018). +fn read_unique_columns( + conn: &Connection, + table: &str, +) -> Result, DbError> { + let mut out: std::collections::HashSet = std::collections::HashSet::new(); + let mut idx_stmt = conn + .prepare( + "SELECT name, \"unique\", origin \ + FROM pragma_index_list(?1);", + ) + .map_err(DbError::from_rusqlite)?; + let idx_rows = idx_stmt + .query_map([table], |row| { + Ok(( + row.get::<_, String>(0)?, + row.get::<_, i64>(1)? != 0, + row.get::<_, String>(2)?, + )) + }) + .map_err(DbError::from_rusqlite)?; + let indexes: Vec<(String, bool, String)> = idx_rows + .collect::, _>>() + .map_err(DbError::from_rusqlite)?; + for (idx_name, is_unique, origin) in indexes { + if !is_unique || origin != "u" { + continue; + } + let mut info_stmt = conn + .prepare("SELECT name FROM pragma_index_info(?1);") + .map_err(DbError::from_rusqlite)?; + let cols: Vec = info_stmt + .query_map([&idx_name], |row| row.get::<_, String>(0)) + .map_err(DbError::from_rusqlite)? + .collect::, _>>() + .map_err(DbError::from_rusqlite)?; + if cols.len() == 1 { + out.insert(cols.into_iter().next().expect("len 1")); + } + } + Ok(out) +} + /// Generate the CREATE TABLE DDL from a `ReadSchema`. Used during /// the rebuild dance. fn schema_to_ddl(table: &str, schema: &ReadSchema) -> String { @@ -2433,6 +2880,13 @@ fn schema_to_ddl(table: &str, schema: &ReadSchema) -> String { if single_inline_pk && col.primary_key { clause.push_str(" PRIMARY KEY"); } + // Inline UNIQUE for non-PK columns flagged unique + // (ADR-0018 §4). PK columns get UNIQUE implicitly via + // PRIMARY KEY; double-emitting would still be valid SQL + // but generates a redundant index. + if col.unique && !col.primary_key { + clause.push_str(" UNIQUE"); + } clauses.push(clause); } @@ -2659,6 +3113,7 @@ fn do_add_relationship( sqlite_type: expected_child_type.sqlite_strict_type().to_string(), notnull: false, primary_key: false, + unique: false, user_type: Some(expected_child_type), }); } else { @@ -3137,6 +3592,34 @@ fn do_insert( } } + // Auto-fill any non-PK serial columns the user didn't list + // (ADR-0018 §5). PK serial columns rely on SQLite's rowid + // alias (omitting the column from the INSERT yields the + // next rowid for free); non-PK serial needs explicit + // MAX(col)+1 application-side because there's no engine- + // level auto-increment for non-PK columns. The worker- + // thread serialisation (ADR-0010) makes this read-then- + // write sequence safe without explicit locking. + for c in &schema.columns { + if c.user_type == Some(Type::Serial) + && !c.primary_key + && !provided.contains(&c.name) + { + let next: i64 = conn + .query_row( + &format!( + "SELECT COALESCE(MAX({col}), 0) + 1 FROM {tbl};", + col = quote_ident(&c.name), + tbl = quote_ident(table), + ), + [], + |row| row.get(0), + ) + .map_err(DbError::from_rusqlite)?; + bindings.push((c.name.clone(), Bound::Integer(next))); + } + } + if bindings.is_empty() { return Err(DbError::InvalidValue( "INSERT requires at least one column value".to_string(), @@ -3698,6 +4181,7 @@ fn build_read_schema(table: &TableSchema, relationships: &[RelationshipSchema]) sqlite_type: c.user_type.sqlite_strict_type().to_string(), notnull: false, primary_key: table.primary_key.contains(&c.name), + unique: c.unique, user_type: Some(c.user_type), }) .collect(); @@ -3950,15 +4434,17 @@ mod tests { async fn add_column_appends_to_existing_table() { let db = db(); make_id_table(&db, "Customers").await; - let desc = db + let result = db .add_column("Customers".to_string(), "Name".to_string(), Type::Text, None) .await .unwrap(); + let desc = &result.description; let names: Vec<_> = desc.columns.iter().map(|c| c.name.as_str()).collect(); assert_eq!(names, vec!["id", "Name"]); let name_col = desc.columns.iter().find(|c| c.name == "Name").unwrap(); assert_eq!(name_col.user_type, Some(Type::Text)); assert_eq!(name_col.sqlite_type.to_uppercase(), "TEXT"); + assert!(result.client_side_notes.is_empty(), "no auto-fill for plain types"); } #[tokio::test] @@ -4041,14 +4527,219 @@ mod tests { } #[tokio::test] - async fn add_column_rejects_serial_with_unsupported_error() { + async fn add_column_serial_to_empty_table_succeeds() { + // ADR-0018 §6: serial / shortid via add_column are now + // allowed. Empty-table case: column added, no auto-fill + // [client-side] note (nothing to populate). let db = db(); make_id_table(&db, "T").await; + let result = db + .add_column("T".to_string(), "code".to_string(), Type::Serial, None) + .await + .unwrap(); + let code = result + .description + .columns + .iter() + .find(|c| c.name == "code") + .expect("code column added"); + assert_eq!(code.user_type, Some(Type::Serial)); + assert!( + !code.primary_key, + "non-PK serial: column is not the PK (id remains the PK)" + ); + assert!( + result.client_side_notes.is_empty(), + "empty table: no auto-fill note" + ); + } + + /// Helper: build a table with `id` (serial PK) + `Name` + /// (text) and insert N rows, populating just `Name`. + async fn make_table_with_n_rows(db: &Database, table: &str, count: usize) { + make_id_table(db, table).await; + db.add_column(table.to_string(), "Name".to_string(), Type::Text, None) + .await + .unwrap(); + for i in 0..count { + db.insert( + table.to_string(), + Some(vec!["Name".to_string()]), + vec![Value::Text(format!("row{i}"))], + None, + ) + .await + .unwrap(); + } + } + + #[tokio::test] + async fn add_column_serial_to_non_empty_table_auto_fills() { + let db = db(); + make_table_with_n_rows(&db, "T", 3).await; + let result = db + .add_column("T".to_string(), "seq".to_string(), Type::Serial, None) + .await + .unwrap(); + let seq = result + .description + .columns + .iter() + .find(|c| c.name == "seq") + .unwrap(); + assert_eq!(seq.user_type, Some(Type::Serial)); + assert!(!result.client_side_notes.is_empty(), "auto-fill note expected"); + assert!( + result.client_side_notes[0].contains("3 row(s) given auto-generated serial"), + "unexpected note: {:?}", + result.client_side_notes + ); + // Verify the column is populated 1..3. + let data = db.query_data("T".to_string(), None).await.unwrap(); + let seq_idx = data.columns.iter().position(|c| c == "seq").unwrap(); + let mut filled: Vec = data + .rows + .iter() + .filter_map(|r| r[seq_idx].as_ref().and_then(|s| s.parse::().ok())) + .collect(); + filled.sort(); + assert_eq!(filled, vec![1, 2, 3]); + } + + #[tokio::test] + async fn add_column_shortid_to_non_empty_table_auto_fills() { + let db = db(); + make_table_with_n_rows(&db, "T", 3).await; + let result = db + .add_column("T".to_string(), "tag".to_string(), Type::ShortId, None) + .await + .unwrap(); + let tag = result + .description + .columns + .iter() + .find(|c| c.name == "tag") + .unwrap(); + assert_eq!(tag.user_type, Some(Type::ShortId)); + assert!(!result.client_side_notes.is_empty(), "auto-fill note expected"); + assert!( + result.client_side_notes[0].contains("3 row(s) given auto-generated shortid"), + "unexpected note: {:?}", + result.client_side_notes + ); + // Verify each row has a non-null shortid value. + let data = db.query_data("T".to_string(), None).await.unwrap(); + let tag_idx = data.columns.iter().position(|c| c == "tag").unwrap(); + for row in &data.rows { + let v = row[tag_idx].as_ref().expect("non-null shortid auto-filled"); + assert!( + v.len() >= 10 && v.len() <= 12, + "shortid length out of range: {v}" + ); + } + } + + #[tokio::test] + async fn insert_auto_fills_non_pk_serial_column_sequentially() { + // ADR-0018 §5: non-PK serial columns get MAX(col)+1 + // automatically when the user omits them. PK serial + // columns continue to use SQLite's rowid alias. + let db = db(); + make_table_with_n_rows(&db, "T", 0).await; + db.add_column("T".to_string(), "seq".to_string(), Type::Serial, None) + .await + .unwrap(); + // Insert three rows providing only `Name`. The seq + // column should auto-fill 1, 2, 3. + for n in ["a", "b", "c"] { + db.insert( + "T".to_string(), + Some(vec!["Name".to_string()]), + vec![Value::Text(n.to_string())], + None, + ) + .await + .unwrap(); + } + let data = db.query_data("T".to_string(), None).await.unwrap(); + let seq_idx = data.columns.iter().position(|c| c == "seq").unwrap(); + let mut values: Vec = data + .rows + .iter() + .filter_map(|r| r[seq_idx].as_ref().and_then(|s| s.parse::().ok())) + .collect(); + values.sort(); + assert_eq!(values, vec![1, 2, 3]); + } + + #[tokio::test] + async fn insert_non_pk_serial_continues_past_explicit_value() { + // If the user explicitly inserts a high value, MAX+1 + // sequencing jumps past it. Gappy sequences are + // accepted (ADR-0018 Resolution 2). + let db = db(); + make_table_with_n_rows(&db, "T", 0).await; + db.add_column("T".to_string(), "seq".to_string(), Type::Serial, None) + .await + .unwrap(); + // Insert with explicit seq=100. + db.insert( + "T".to_string(), + Some(vec!["Name".to_string(), "seq".to_string()]), + vec![Value::Text("a".to_string()), Value::Number("100".to_string())], + None, + ) + .await + .unwrap(); + // Next omitted-seq insert should auto-fill 101. + db.insert( + "T".to_string(), + Some(vec!["Name".to_string()]), + vec![Value::Text("b".to_string())], + None, + ) + .await + .unwrap(); + let data = db.query_data("T".to_string(), None).await.unwrap(); + let seq_idx = data.columns.iter().position(|c| c == "seq").unwrap(); + let mut values: Vec = data + .rows + .iter() + .filter_map(|r| r[seq_idx].as_ref().and_then(|s| s.parse::().ok())) + .collect(); + values.sort(); + assert_eq!(values, vec![100, 101]); + } + + #[tokio::test] + async fn add_column_serial_emits_unique_constraint() { + // Non-PK serial gains UNIQUE per ADR-0018 §4. Verify by + // attempting to UPDATE all rows to the same value and + // confirming the engine refuses. + let db = db(); + make_table_with_n_rows(&db, "T", 2).await; + db.add_column("T".to_string(), "seq".to_string(), Type::Serial, None) + .await + .unwrap(); + // Attempt to UPDATE one row to have the same `seq` value + // as the other — should violate UNIQUE. let err = db - .add_column("T".to_string(), "id2".to_string(), Type::Serial, None) + .update( + "T".to_string(), + vec![("seq".to_string(), Value::Number("1".to_string()))], + RowFilter::AllRows, + None, + ) .await .unwrap_err(); - assert!(matches!(err, DbError::Unsupported(_)), "got {err:?}"); + // SQLite reports as a constraint violation; classified + // as UniqueViolation. + match err { + DbError::Sqlite { kind, .. } => { + assert_eq!(kind, SqliteErrorKind::UniqueViolation); + } + other => panic!("unexpected error: {other:?}"), + } } #[tokio::test] @@ -4782,17 +5473,238 @@ mod tests { } #[tokio::test] - async fn change_column_type_refuses_serial_target() { + async fn change_column_type_int_to_serial_succeeds() { + // ADR-0018 §8: int → serial joins the matrix. The + // column gains UNIQUE (since it's non-PK) and the + // existing values are preserved. + let db = db(); + make_table_with_n_rows(&db, "T", 0).await; + db.add_column("T".to_string(), "code".to_string(), Type::Int, None) + .await + .unwrap(); + // Insert a few rows with explicit code values. + for (i, code) in [(1, 10), (2, 20), (3, 30)] { + db.insert( + "T".to_string(), + Some(vec!["Name".to_string(), "code".to_string()]), + vec![ + Value::Text(format!("row{i}")), + Value::Number(code.to_string()), + ], + None, + ) + .await + .unwrap(); + } + let result = db + .change_column_type( + "T".to_string(), + "code".to_string(), + Type::Serial, + ChangeColumnMode::Default, + None, + ) + .await + .expect("int -> serial should succeed"); + let code = result + .description + .columns + .iter() + .find(|c| c.name == "code") + .unwrap(); + assert_eq!(code.user_type, Some(Type::Serial)); + // No null cells, no transformation: no [client-side] + // note (storage-class change for int -> serial is + // identity at the value level). + assert!( + result.client_side.is_none(), + "int -> serial with all non-null values is a metadata-only change" + ); + } + + #[tokio::test] + async fn change_column_type_int_to_serial_refuses_duplicate_values() { + // Duplicate non-null values would violate the UNIQUE + // contract serial gains. Refused via the existing + // uniqueness-collision diagnostic. + let db = db(); + make_table_with_n_rows(&db, "T", 0).await; + db.add_column("T".to_string(), "code".to_string(), Type::Int, None) + .await + .unwrap(); + // Two rows with the same code. + for i in 0..2 { + db.insert( + "T".to_string(), + Some(vec!["Name".to_string(), "code".to_string()]), + vec![Value::Text(format!("row{i}")), Value::Number("7".to_string())], + None, + ) + .await + .unwrap(); + } + let err = db + .change_column_type( + "T".to_string(), + "code".to_string(), + Type::Serial, + ChangeColumnMode::Default, + None, + ) + .await + .unwrap_err(); + match err { + DbError::Unsupported(message) => { + assert!( + message.contains("collision") || message.contains("uniqueness"), + "expected uniqueness diagnostic: {message}" + ); + } + other => panic!("unexpected: {other:?}"), + } + } + + #[tokio::test] + async fn change_column_type_text_to_serial_routes_via_int_hint() { + // Per ADR-0018 §8, only int → serial is supported + // directly. Other source types are refused with a hint + // to route through int. let db = db(); make_id_table(&db, "T").await; - db.add_column("T".to_string(), "A".to_string(), Type::Int, None) + db.add_column("T".to_string(), "A".to_string(), Type::Text, None) .await .unwrap(); let err = db - .change_column_type("T".to_string(), "A".to_string(), Type::Serial, ChangeColumnMode::Default, None) + .change_column_type( + "T".to_string(), + "A".to_string(), + Type::Serial, + ChangeColumnMode::Default, + None, + ) .await .unwrap_err(); - assert!(matches!(err, DbError::Unsupported(_)), "got {err:?}"); + match err { + DbError::Unsupported(message) => { + assert!( + message.contains("int"), + "expected hint about routing via int: {message}" + ); + } + other => panic!("unexpected: {other:?}"), + } + } + + #[tokio::test] + async fn change_column_type_int_to_serial_with_nulls_auto_fills() { + // Per ADR-0018 §3 / §7, null cells get sequence values + // continuing from MAX of non-null values. The + // [client-side] note reports the auto-fill count. + let db = db(); + make_table_with_n_rows(&db, "T", 0).await; + db.add_column("T".to_string(), "code".to_string(), Type::Int, None) + .await + .unwrap(); + // Three rows: one with code=5, two with NULL. + db.insert( + "T".to_string(), + Some(vec!["Name".to_string(), "code".to_string()]), + vec![Value::Text("a".to_string()), Value::Number("5".to_string())], + None, + ) + .await + .unwrap(); + for n in ["b", "c"] { + db.insert( + "T".to_string(), + Some(vec!["Name".to_string()]), + vec![Value::Text(n.to_string())], + None, + ) + .await + .unwrap(); + } + let result = db + .change_column_type( + "T".to_string(), + "code".to_string(), + Type::Serial, + ChangeColumnMode::Default, + None, + ) + .await + .expect("int -> serial with auto-fill should succeed"); + let note = result + .client_side + .expect("auto-fill should produce a client-side note"); + assert_eq!(note.auto_filled, 2); + assert_eq!(note.auto_fill_kind, Some(AutoFillKind::Serial)); + // Confirm the filled values: existing 5, fills are 6 + // and 7 (continue sequence from MAX+1). + let data = db.query_data("T".to_string(), None).await.unwrap(); + let code_idx = data.columns.iter().position(|c| c == "code").unwrap(); + let mut values: Vec = data + .rows + .iter() + .filter_map(|r| r[code_idx].as_ref().and_then(|s| s.parse::().ok())) + .collect(); + values.sort(); + assert_eq!(values, vec![5, 6, 7]); + } + + #[tokio::test] + async fn change_column_type_text_to_shortid_with_nulls_auto_fills() { + // text → shortid is in the matrix; null cells in the + // text column get fresh shortids (ADR-0018 §3). + let db = db(); + make_table_with_n_rows(&db, "T", 0).await; + db.add_column("T".to_string(), "tag".to_string(), Type::Text, None) + .await + .unwrap(); + // One row with a valid shortid value, two with NULL. + db.insert( + "T".to_string(), + Some(vec!["Name".to_string(), "tag".to_string()]), + vec![ + Value::Text("a".to_string()), + Value::Text("23456789Ab".to_string()), + ], + None, + ) + .await + .unwrap(); + for n in ["b", "c"] { + db.insert( + "T".to_string(), + Some(vec!["Name".to_string()]), + vec![Value::Text(n.to_string())], + None, + ) + .await + .unwrap(); + } + let result = db + .change_column_type( + "T".to_string(), + "tag".to_string(), + Type::ShortId, + ChangeColumnMode::Default, + None, + ) + .await + .expect("text -> shortid with auto-fill should succeed"); + let note = result + .client_side + .expect("auto-fill should produce a client-side note"); + assert_eq!(note.auto_filled, 2); + assert_eq!(note.auto_fill_kind, Some(AutoFillKind::ShortId)); + // All three rows now have valid shortids. + let data = db.query_data("T".to_string(), None).await.unwrap(); + let tag_idx = data.columns.iter().position(|c| c == "tag").unwrap(); + for row in &data.rows { + let v = row[tag_idx].as_ref().expect("non-null shortid after fill"); + assert!(v.len() >= 10 && v.len() <= 12, "len out of range: {v}"); + } } #[tokio::test] @@ -5616,6 +6528,217 @@ mod tests { assert_eq!(data.rows[0][1], None); } + // ---- ADR-0018 UNIQUE infrastructure ---- + + #[test] + fn read_schema_detects_single_column_unique() { + // Create a raw connection (no Database wrapper) so we + // can drop arbitrary DDL containing UNIQUE — the DSL + // doesn't yet expose UNIQUE as a user-controlled + // constraint (C3-track). + let conn = Connection::open_in_memory().unwrap(); + configure_connection(&conn).unwrap(); + conn.execute_batch( + "CREATE TABLE T (id INTEGER PRIMARY KEY, code TEXT UNIQUE) STRICT;", + ) + .unwrap(); + let schema = read_schema(&conn, "T").unwrap(); + let id = schema.columns.iter().find(|c| c.name == "id").unwrap(); + let code = schema.columns.iter().find(|c| c.name == "code").unwrap(); + assert!(id.primary_key, "id should still be PK"); + assert!( + !id.unique, + "PK columns are not separately marked unique \ + (PK already implies it; double-marking would lead to \ + redundant DDL)" + ); + assert!(code.unique, "code should be detected as unique"); + } + + #[test] + fn read_schema_ignores_compound_unique_constraints() { + // Multi-column UNIQUE is out of scope for ADR-0018; the + // detection helper filters it out so schema_to_ddl does + // not accidentally emit a column-level UNIQUE for one + // half of a compound constraint. + let conn = Connection::open_in_memory().unwrap(); + configure_connection(&conn).unwrap(); + conn.execute_batch( + "CREATE TABLE T (id INTEGER PRIMARY KEY, a TEXT, b TEXT, UNIQUE(a, b)) STRICT;", + ) + .unwrap(); + let schema = read_schema(&conn, "T").unwrap(); + for col in &schema.columns { + assert!( + !col.unique, + "column {} should not be marked unique under compound UNIQUE", + col.name + ); + } + } + + #[test] + fn schema_to_ddl_emits_unique_for_marked_column() { + // Construct a ReadSchema with a unique non-PK column and + // confirm the round-trip through schema_to_ddl produces + // valid DDL containing UNIQUE. + let schema = ReadSchema { + columns: vec![ + ReadColumn { + name: "id".to_string(), + sqlite_type: "INTEGER".to_string(), + notnull: false, + primary_key: true, + unique: false, + user_type: Some(Type::Serial), + }, + ReadColumn { + name: "code".to_string(), + sqlite_type: "TEXT".to_string(), + notnull: false, + primary_key: false, + unique: true, + user_type: Some(Type::Text), + }, + ], + primary_key: vec!["id".to_string()], + foreign_keys: vec![], + }; + let ddl = schema_to_ddl("T", &schema); + assert!( + ddl.contains("\"id\" INTEGER PRIMARY KEY"), + "PK emission preserved: {ddl}" + ); + assert!( + ddl.contains("\"code\" TEXT UNIQUE"), + "UNIQUE emitted on non-PK column: {ddl}" + ); + assert!( + !ddl.contains("\"id\" INTEGER PRIMARY KEY UNIQUE"), + "PK column should not double-emit UNIQUE: {ddl}" + ); + // The DDL is valid SQL — apply it and read it back. + let conn = Connection::open_in_memory().unwrap(); + configure_connection(&conn).unwrap(); + conn.execute_batch(&ddl).unwrap(); + let round = read_schema(&conn, "T").unwrap(); + let code = round.columns.iter().find(|c| c.name == "code").unwrap(); + assert!(code.unique, "round-tripped column retains unique flag"); + } + + #[tokio::test] + async fn unique_flag_persisted_in_yaml_for_non_pk_serial() { + // ADR-0018 §4: a non-PK serial column gains UNIQUE, + // and the flag must be recorded in `project.yaml` so + // that a rebuild-from-text reconstructs the constraint + // faithfully (preserves the "serial -> int leaves + // UNIQUE in place" promise across save/load cycles). + use crate::persistence::Persistence; + let dir = tempfile::tempdir().unwrap(); + let persistence = Persistence::new(dir.path().to_path_buf()); + let db_path = dir.path().join("playground.db"); + let db = Database::open_with_persistence(&db_path, persistence).unwrap(); + db.create_table( + "T".to_string(), + vec![col("id", Type::Serial), col("Name", Type::Text)], + vec!["id".to_string()], + None, + ) + .await + .unwrap(); + db.add_column("T".to_string(), "seq".to_string(), Type::Serial, None) + .await + .unwrap(); + // Read the persisted YAML straight from disk. + let yaml = std::fs::read_to_string(dir.path().join(PROJECT_YAML)).unwrap(); + assert!( + yaml.contains("seq") && yaml.contains("unique: true"), + "yaml should record the unique flag for `seq`: {yaml}" + ); + // PK column not separately marked unique — PK already + // implies UNIQUE, double-marking would emit redundant + // DDL on rebuild. + let lines_with_id: Vec<&str> = yaml + .lines() + .filter(|l| l.contains("name: id")) + .collect(); + for line in &lines_with_id { + assert!( + !line.contains("unique: true"), + "PK column should not be marked unique in YAML: {line}" + ); + } + } + + #[tokio::test] + async fn unique_flag_round_trips_through_rebuild() { + // End-to-end: create a table with a non-PK serial, + // then rebuild from text. The rebuild should re-emit + // UNIQUE on the column, restoring the constraint + // exactly as the original. + use crate::persistence::Persistence; + let dir = tempfile::tempdir().unwrap(); + let persistence = Persistence::new(dir.path().to_path_buf()); + let db_path = dir.path().join("playground.db"); + let db = Database::open_with_persistence(&db_path, persistence).unwrap(); + db.create_table( + "T".to_string(), + vec![col("id", Type::Serial), col("Name", Type::Text)], + vec!["id".to_string()], + None, + ) + .await + .unwrap(); + db.add_column("T".to_string(), "seq".to_string(), Type::Serial, None) + .await + .unwrap(); + // Tear down the .db file and rebuild from yaml + csvs. + drop(db); + std::fs::remove_file(&db_path).unwrap(); + let persistence2 = Persistence::new(dir.path().to_path_buf()); + let db = Database::open_with_persistence(&db_path, persistence2).unwrap(); + db.rebuild_from_text(dir.path().to_path_buf(), None) + .await + .unwrap(); + // Read the schema and confirm `seq` is still unique. + let desc = db.describe_table("T".to_string(), None).await.unwrap(); + let seq = desc + .columns + .iter() + .find(|c| c.name == "seq") + .expect("seq column rebuilt"); + assert_eq!(seq.user_type, Some(Type::Serial)); + // Confirm the UNIQUE constraint is enforced post- + // rebuild by attempting to UPDATE all rows to the same + // value... but the table is empty. Instead, insert two + // rows and then UPDATE. + for n in ["a", "b"] { + db.insert( + "T".to_string(), + Some(vec!["Name".to_string()]), + vec![Value::Text(n.to_string())], + None, + ) + .await + .unwrap(); + } + let err = db + .update( + "T".to_string(), + vec![("seq".to_string(), Value::Number("1".to_string()))], + RowFilter::AllRows, + None, + ) + .await + .unwrap_err(); + match err { + DbError::Sqlite { kind, .. } => { + assert_eq!(kind, SqliteErrorKind::UniqueViolation); + } + other => panic!("unexpected: {other:?}"), + } + } + #[tokio::test] async fn quoted_table_names_round_trip() { let db = db(); diff --git a/src/event.rs b/src/event.rs index 6f67a15..60f49c4 100644 --- a/src/event.rs +++ b/src/event.rs @@ -8,8 +8,8 @@ use crossterm::event::KeyEvent; use crate::db::{ - ChangeColumnTypeResult, DataResult, DeleteResult, InsertResult, TableDescription, - UpdateResult, + AddColumnResult, ChangeColumnTypeResult, DataResult, DeleteResult, InsertResult, + TableDescription, UpdateResult, }; use crate::dsl::Command; @@ -49,6 +49,13 @@ pub enum AppEvent { command: Command, result: ChangeColumnTypeResult, }, + /// An `add column …` succeeded. `result` carries the + /// post-add description plus any `[client-side]` notes + /// from the auto-fill paths (ADR-0018 §9). + DslAddColumnSucceeded { + command: Command, + result: AddColumnResult, + }, /// A DSL command failed. `error` is already a friendly /// message produced via `DbError::friendly_message`. DslFailed { diff --git a/src/persistence/csv_io.rs b/src/persistence/csv_io.rs index 3edfea6..88690a2 100644 --- a/src/persistence/csv_io.rs +++ b/src/persistence/csv_io.rs @@ -360,7 +360,7 @@ mod tests { use crate::persistence::ColumnSchema; fn col(name: &str, ty: Type) -> ColumnSchema { - ColumnSchema { name: name.to_string(), user_type: ty } + ColumnSchema { name: name.to_string(), user_type: ty, unique: false } } #[test] diff --git a/src/persistence/mod.rs b/src/persistence/mod.rs index 6cd718d..e0cc6e6 100644 --- a/src/persistence/mod.rs +++ b/src/persistence/mod.rs @@ -100,6 +100,13 @@ pub struct TableSchema { pub struct ColumnSchema { pub name: String, pub user_type: Type, + /// Whether this column carries a single-column UNIQUE + /// constraint (ADR-0018 §4). Stored explicitly in the + /// project YAML so that a `serial → int` round-trip + /// (which leaves UNIQUE in place) is preserved across a + /// save/load cycle. Defaults to `false` when missing in + /// older project files. + pub unique: bool, } #[derive(Debug, Clone, PartialEq, Eq)] @@ -318,6 +325,7 @@ mod tests { columns: vec![ColumnSchema { name: "Name".to_string(), user_type: Type::Text, + unique: false, }], rows: vec![vec![CellValue::Text("Alice".to_string())]], }; diff --git a/src/persistence/yaml.rs b/src/persistence/yaml.rs index 9b9c256..89a935e 100644 --- a/src/persistence/yaml.rs +++ b/src/persistence/yaml.rs @@ -71,12 +71,21 @@ fn write_table(out: &mut String, table: &TableSchema) { } fn write_column(out: &mut String, col: &ColumnSchema) { - let _ = writeln!( - out, - " - {{ name: {}, type: {} }}", - quote_if_needed(&col.name), - col.user_type.keyword(), - ); + if col.unique { + let _ = writeln!( + out, + " - {{ name: {}, type: {}, unique: true }}", + quote_if_needed(&col.name), + col.user_type.keyword(), + ); + } else { + let _ = writeln!( + out, + " - {{ name: {}, type: {} }}", + quote_if_needed(&col.name), + col.user_type.keyword(), + ); + } } fn write_relationship(out: &mut String, rel: &RelationshipSchema) { @@ -181,6 +190,7 @@ pub(crate) fn parse_schema(body: &str) -> Result { columns.push(ColumnSchema { name: c.name, user_type, + unique: c.unique, }); } tables.push(TableSchema { @@ -265,6 +275,11 @@ struct RawColumn { name: String, #[serde(rename = "type")] user_type: String, + /// Optional flag introduced in ADR-0018 for single-column + /// UNIQUE constraints. Older project files without this + /// field default to `false`. + #[serde(default)] + unique: bool, } #[derive(Deserialize)] @@ -295,16 +310,16 @@ mod tests { name: "Customers".to_string(), primary_key: vec!["id".to_string()], columns: vec![ - ColumnSchema { name: "id".to_string(), user_type: Type::Serial }, - ColumnSchema { name: "Name".to_string(), user_type: Type::Text }, + ColumnSchema { name: "id".to_string(), user_type: Type::Serial, unique: false }, + ColumnSchema { name: "Name".to_string(), user_type: Type::Text, unique: false }, ], }, TableSchema { name: "Orders".to_string(), primary_key: vec!["id".to_string()], columns: vec![ - ColumnSchema { name: "id".to_string(), user_type: Type::Serial }, - ColumnSchema { name: "CustId".to_string(), user_type: Type::Int }, + ColumnSchema { name: "id".to_string(), user_type: Type::Serial, unique: false }, + ColumnSchema { name: "CustId".to_string(), user_type: Type::Int, unique: false }, ], }, ], @@ -359,6 +374,7 @@ mod tests { columns: vec![ColumnSchema { name: "yes".to_string(), user_type: Type::Bool, + unique: false, }], }], relationships: vec![], @@ -452,8 +468,8 @@ relationships: name: "Items".to_string(), primary_key: vec!["a".to_string(), "b".to_string()], columns: vec![ - ColumnSchema { name: "a".to_string(), user_type: Type::Int }, - ColumnSchema { name: "b".to_string(), user_type: Type::Int }, + ColumnSchema { name: "a".to_string(), user_type: Type::Int, unique: false }, + ColumnSchema { name: "b".to_string(), user_type: Type::Int, unique: false }, ], }], relationships: vec![], diff --git a/src/runtime.rs b/src/runtime.rs index 9341599..91cdc2d 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -29,8 +29,8 @@ use crate::action::Action; use crate::app::App; use crate::cli::Args; use crate::db::{ - ChangeColumnTypeResult, DataResult, Database, DbError, DeleteResult, InsertResult, - TableDescription, UpdateResult, + AddColumnResult, ChangeColumnTypeResult, DataResult, Database, DbError, DeleteResult, + InsertResult, TableDescription, UpdateResult, }; use crate::dsl::Command; use crate::event::AppEvent; @@ -951,6 +951,10 @@ fn spawn_dsl_dispatch( command: command.clone(), result, }, + Ok(CommandOutcome::AddColumn(result)) => AppEvent::DslAddColumnSucceeded { + command: command.clone(), + result, + }, Err(DbError::PersistenceFatal { operation, path, @@ -988,6 +992,7 @@ enum CommandOutcome { Update(UpdateResult), Delete(DeleteResult), ChangeColumn(ChangeColumnTypeResult), + AddColumn(AddColumnResult), } /// Execute a parsed user command and return either a typed @@ -1016,7 +1021,7 @@ async fn execute_command_typed( Command::AddColumn { table, column, ty } => database .add_column(table, column, ty, src) .await - .map(|d| CommandOutcome::Schema(Some(d))), + .map(CommandOutcome::AddColumn), Command::DropColumn { table, column } => database .drop_column(table, column, src) .await diff --git a/src/type_change.rs b/src/type_change.rs index 6e8d6f5..e4360ac 100644 --- a/src/type_change.rs +++ b/src/type_change.rs @@ -54,13 +54,14 @@ pub fn static_refusal(src: Type, target: Type) -> Option { if src == target { return Some(format!("column is already `{src}`.")); } - if matches!(target, Type::Serial) { - return Some( - "the 'serial' type carries auto-increment primary-key semantics \ - that only apply at create-table time; pick a different target \ - type (e.g. `int`)." - .to_string(), - ); + // ADR-0018 §8: `int → serial` is allowed via the matrix. + // Other sources to serial are refused — the user routes + // via int first if needed. + if matches!(target, Type::Serial) && !matches!(src, Type::Int) { + return Some(format!( + "to convert from `{src}` to `serial`, change the column to `int` \ + first; only `int → serial` is supported directly." + )); } if matches!(src, Type::Blob) || matches!(target, Type::Blob) { return Some(format!( @@ -98,6 +99,14 @@ const fn is_in_matrix(src: Type, target: Type) -> bool { // stored values"). Storage stays INTEGER; we treat // it as an identity transformer. | (Serial, Int) + // int -> serial: ADR-0018 §8. Storage stays + // INTEGER, the metadata flips to "auto-generated" + // and the column gains UNIQUE if non-PK. The + // matrix entry is identity at the value level; + // uniqueness and auto-fill of nulls happen at the + // change-column orchestration layer, not in the + // per-cell transformer. + | (Int, Serial) | (Bool, Int | Real | Decimal | Text) | (Decimal | Date | DateTime | ShortId | Real, Text) // Per-cell-classified @@ -141,6 +150,16 @@ pub fn transform_cell(src: Type, target: Type, value: &Value) -> CellOutcome { Value::Integer(i) => CellOutcome::Clean(Value::Integer(*i)), other => unexpected_storage("serial", other), }, + // int -> serial: identity at the storage class level + // (both INTEGER); the conversion adds the auto- + // generated contract and (for non-PK columns) UNIQUE. + // Per-cell transformer is identity; the change-column + // orchestrator handles null auto-fill and uniqueness. + // ADR-0018 §8. + (Int, Serial) => match value { + Value::Integer(i) => CellOutcome::Clean(Value::Integer(*i)), + other => unexpected_storage("int", other), + }, // ---- Always-clean: bool source (stored as INTEGER 0/1) ---- (Bool, Int) => match value { @@ -568,12 +587,29 @@ mod tests { } #[test] - fn anything_to_serial_is_statically_refused() { + fn non_int_sources_to_serial_are_statically_refused() { + // ADR-0018 §8: only `int → serial` is allowed directly. + // Other sources have to route via int. (Same-type + // `serial → serial` is the no-op identity refusal.) for &src in Type::all() { + if matches!(src, Type::Int | Type::Serial) { + continue; + } assert!(static_refusal(src, Type::Serial).is_some(), "{src:?}"); } } + #[test] + fn int_to_serial_is_allowed() { + // ADR-0018 §8: `int → serial` joins the matrix as + // always-clean identity. Non-null values pass through. + assert!(static_refusal(Type::Int, Type::Serial).is_none()); + assert_eq!( + transform_cell(Type::Int, Type::Serial, &int(42)), + CellOutcome::Clean(int(42)) + ); + } + #[test] fn anything_involving_blob_is_statically_refused() { for &other in Type::all() {