diff --git a/docs/adr/0035-advanced-mode-sql-ddl.md b/docs/adr/0035-advanced-mode-sql-ddl.md index d2e62c3..6ca60dd 100644 --- a/docs/adr/0035-advanced-mode-sql-ddl.md +++ b/docs/adr/0035-advanced-mode-sql-ddl.md @@ -4,14 +4,14 @@ Accepted. Design agreed with the user (2026-05-24); the approach is **validated end-to-end by sub-phases 4a / 4a.2 / 4a.3 / 4b / 4c / 4d / -4e** (`CREATE TABLE` with column- and table-level constraints and foreign -keys, `DROP TABLE [IF EXISTS]`, `CREATE [UNIQUE] INDEX` / -`DROP INDEX [IF EXISTS]`, and `ALTER TABLE` add/drop/rename column, -implemented 2026-05-25 — plans +4e / 4f** (`CREATE TABLE` with column- and table-level constraints and +foreign keys, `DROP TABLE [IF EXISTS]`, `CREATE [UNIQUE] INDEX` / +`DROP INDEX [IF EXISTS]`, `ALTER TABLE` add/drop/rename column, and +`ALTER TABLE … ALTER COLUMN TYPE`, implemented 2026-05-25 — plans `docs/plans/20260524-adr-0035-sql-ddl-4a.md`, `…-4a2.md`, `…-4a3.md`, `docs/plans/20260525-adr-0035-sql-ddl-4b.md`, `…-4c.md`, `…-4d.md`, -`…-4e.md`), so the decision is accepted while the remaining sub-phases -(**4f–4i**, §13) continue. This is **Phase 4** of the ADR-0030 roadmap (the +`…-4e.md`, `…-4f.md`), so the decision is accepted while the remaining +sub-phases (**4g–4i**, §13) continue. This is **Phase 4** of the ADR-0030 roadmap (the advanced-mode SQL surface), the peer of ADR-0031 (expression grammar), ADR-0032 (`SELECT`), and ADR-0033 (DML). It **clarifies ADR-0030 §4** on how DDL is represented and executed. @@ -436,7 +436,25 @@ ADR-0033's structure: internal-table guard on `do_change_column_type` / `do_add_constraint` / `do_add_relationship` is a tracked follow-up.)* - **4f — `ALTER TABLE … ALTER COLUMN TYPE`** (the §7 conversion - model + the lossy-with-note path). + model + the lossy-with-note path). *(Implemented 2026-05-25 — plan + `docs/plans/20260525-adr-0035-sql-ddl-4f.md`.)* A fourth + `AlterTableAction::AlterColumnType`, runtime-decomposed to the existing + `change_column_type` executor with `ChangeColumnMode::ForceConversion` + — which **is** the §7 advanced policy: lossy cells are *performed* and + counted (the engine-neutral `client_side.transformed_lossy` note + fires), incompatible cells refuse, and the ADR-0017 static refusals + (`↔ blob`, same-type, `date ↔ datetime`, non-`int → serial`) refuse in + both modes. **`int → serial` is *allowed*** (auto-fills nulls, adds + UNIQUE if non-PK — ADR-0018 §8; the §7 "static-refused →serial" + summary is looser than the code). No force flag, no `USING`, no + `SET DATA TYPE` synonym (§7/§12); `undo` is the advanced safety net. + The grammar adds a fourth action branch leading on `alter`, + discriminated in the builder by the **`type` keyword** (unique — ADD + COLUMN's type is an ident); the type slot reuses `SQL_TYPE`. The + internal-`__rdbms_*` guard was folded into `do_change_column_type` + (user-confirmed 2026-05-25), closing the simple `change column` + exposure too. *(The remaining internal-table guard on + `do_add_constraint` / `do_add_relationship` rides in 4g.)* - **4g — `ALTER TABLE` add/drop constraint, add foreign key.** - **4h — `ALTER TABLE … RENAME TO`** (the §6 new low-level op). - **4i — Verification sweep.** Typing-surface + matrix coverage, diff --git a/docs/adr/README.md b/docs/adr/README.md index 36c9157..71a0348 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -40,4 +40,4 @@ This directory contains the project's ADRs, recorded per - [ADR-0032 — The full SQL `SELECT` grammar](0032-sql-select-grammar.md) — **Accepted**, the Phase-2 grammar commissioned by ADR-0030 §3: full `SELECT` with `INNER`/`LEFT`/`RIGHT`/`FULL OUTER`/`CROSS` joins, `GROUP BY`/`HAVING`, all four set ops (`UNION`/`UNION ALL`/`INTERSECT`/`EXCEPT`), `WITH` and `WITH RECURSIVE` CTEs, `LIMIT … OFFSET`, `DISTINCT`, `t.*`, and bare-alias projection (lifting Phase-1 §4.2); additive extensions to ADR-0031's `sql_expr` for scalar subqueries, `IN (SELECT …)`, `[NOT] EXISTS`, and qualified column refs (redeeming ADR-0031 §7 OOS-1/OOS-2); grammar-recursion via `Subgrammar(&SQL_SELECT_COMPOUND)` reuses ADR-0026's `MAX_SUBGRAMMAR_DEPTH = 64` cap unchanged; **softens ADR-0030 §8's "ambient assistance comes for free" claim**: completion scope needs new `WalkContext` accumulators (a `from_scope_stack` of `ScopeFrame`s holding `from_scope` / `cte_bindings` / `projection_aliases`), a **new walker node variant `Node::ScopedSubgrammar(&Node)`** as the push/pop trigger (existing `Node::Subgrammar` unchanged so DSL `Expr` and `sql_expr` recursion are unaffected), qualified-prefix completion narrowing, body-projection-derived CTE column resolution (so `SELECT *` and explicit-projection CTE bodies both yield real column completion past `cte_alias.|`), and a **post-walk fixup pass** that re-resolves projection-list identifier highlighting/validity once `FROM` is parsed (the projection-before-FROM problem); classifies every Phase-2 validation case against ADR-0027's ERROR/WARNING guideline (§11): five new `diagnostic.*` keys for parse-time-detectable cases (unknown qualifier, ambiguous column, projection-alias misplaced, CTE/compound arity mismatch) plus eight `engine.*` translation keys; a MatchedPath-walking predicate-warnings variant that closes the Phase-1 gap where SQL `WHERE` expressions emitted no `LIKE`-on-numeric / `= NULL` / type-mismatch warnings (ADR-0027 Amendment 1 finally extends to the SQL surface); adds a worker-side post-prepare type-resolution pass via engine column-origin metadata so bare column refs recover their playground type (partially lifting Phase-1 §4.5, the bool→0/1 case) — `Cargo.toml` gains `column_metadata` to rusqlite features (verified against pinned 0.39.0); `__rdbms_*` rejection extended to every new table-source slot; Amendment 1 narrows §12's resolution rule from a grammar-side structural classification to "trust the engine's column-origin metadata verbatim" after an empirical probe showed origin metadata follows through non-recursive CTEs, scalar subqueries, derived tables, set ops, and joins — the one structural exception is recursive CTE result columns, which return None and stay typeless; Amendment 2 records that §10.6's "rewrite the highlight class" prescription is realised via the two-pass schema-existence diagnostic + the renderer's diagnostic-overlay path (no separate per-byte rewrite step needed; no new HighlightClass variant), and that the projection-before-FROM completion narrowing has been improved by an `src/completion.rs` look-ahead probe when the leading walk's `from_scope` is empty but the full input parses - [ADR-0033 — The full SQL DML grammar (`INSERT` / `UPDATE` / `DELETE`)](0033-sql-dml-grammar.md) — **Accepted** (implemented + verified through sub-phase 3k, 2026-05-23; phase-exit report `docs/handoff/20260523-phase-3-verification.md`), the Phase-3 grammar commissioned by ADR-0030 §3: single- and multi-row `INSERT` (incl. `INSERT … SELECT` recursing through ADR-0032's `SQL_SELECT_COMPOUND`), `UPDATE` with `SET` assignment list, `DELETE`, all three optionally followed by `RETURNING projection_list`, plus full `ON CONFLICT … DO NOTHING / DO UPDATE` UPSERT on INSERT; **fixes the DSL-vs-SQL dispatch architecture for shared entry words (`insert`/`update`/`delete`)**: SQL-first / DSL-fallback in Advanced mode via a `Choice(SQL_shape, DSL_shape)` per shape, gated by a new walker capability `Node::Guard(fn)` — a zero-byte-consumption gating node that fails the enclosing Seq with a `ValidationError`; carries `Command::SqlInsert` / `SqlUpdate` / `SqlDelete` variants and `do_sql_*` worker handlers each of which knows the target table (for re-persistence) and the `returning: bool` flag (for DataResult routing); `shortid` auto-fill mirrors the DSL `do_insert` mechanism via worker post-fill; SQL DELETE produces the same per-relationship cascade summary the DSL DELETE does (ADR-0014 parity); three new walker diagnostics (`insert_arity_mismatch` ERROR, `auto_column_overridden` WARNING, `not_null_missing` WARNING) with positive + negative tests each; OOS list explicitly carves out `DEFAULT VALUES` (the project's planned seed feature), SQLite-specific `OR REPLACE` / `OR IGNORE` / `OR ABORT` / `OR FAIL` / `OR ROLLBACK` prefixes, `UPDATE FROM` multi-table updates, and WITH-prefixed DML; the `excluded` keyword inside `ON CONFLICT DO UPDATE` is a deliberate carve-out from ADR-0030 §7's engine-neutral posture (no standard-SQL UPSERT spelling exists that SQLite and PostgreSQL share); eleven phased sub-phases each with explicit exit gates + written DA gate, opening with the dispatch mechanism before any DML grammar lands; initial DA review recorded seven critiques that were resolved before status moved to Proposed; **Amendment 1 supersedes §2's dispatch mechanism**: the originally-chosen `Node::Guard(fn)` + `Choice(SQL_shape, DSL_shape)` was found during 3a to be unworkable as framed (any guard-in-`Choice` mechanism forces a `walk_choice` change — `walk_choice` only falls through on `NoMatch`, so Simple-mode valid-DSL would wrongly surface "this is SQL", and `walk_seq` treats a `NoMatch` past `idx 0` as a hard `Failed`, breaking Advanced-mode DSL fall-through); replaced by **category-grouped, mode-aware dispatch** in `walker::walk` (each `REGISTRY` entry tagged `CommandCategory::{Simple, Advanced}`, generalising the existing whole-command `is_advanced_only` gate), shared entry words carrying a node in both groups, no `Node::Guard` and no `walk_choice`/`walk_seq` change, advanced-mode completion SQL-first with DSL as a full-line fallback; **Amendment 2 (sub-phase 3f) supersedes §7's cascade mechanism**: the WHERE-injected per-child pre-count rested on a premise that was factually wrong about the DSL handler (which detects cascades by before/after row-count diffing inside a transaction, not by `Expr`-derived pre-count subqueries) and would have broken the §2 parity promise by reporting `SET NULL` the DSL path doesn't; replaced by mirroring `do_delete`'s count-diff exactly (verbatim DELETE executes, child-count diff observes the cascade — `ON DELETE CASCADE` row removals only, SET NULL deferred for both paths to preserve parity), which shares the render-layer formatter for free via `CommandOutcome::Delete` and **withdraws risk R2** (no WHERE-byte extraction, no N+1 subquery); **Amendment 3 (sub-phase 3j) records the command-identity model and defers the execution-mode side-channel**: a command is the typed outcome of a *mode-rooted* grammar path and its identity is intrinsic (Advanced mode tries SQL first, falls back to the *Simple* DSL command when no SQL branch matches a token, e.g. `delete … --all-rows`; note `update … --all-rows` does *not* fall back — the SQL `SET` expression eats `--all-rows`, harmless since the engine treats it as a comment); **Simple mode commits the DSL candidate for shared words** so the *real* DSL error surfaces, and when that line would also run in advanced mode the rendering layer **combines** them — DSL error **plus** an `advanced_mode.also_valid_sql` pointer ("… (valid as SQL in advanced mode)") — keeping the actionable DSL fix while pointing at advanced mode; bare "this is SQL" is reserved for entry words with no DSL form (`select`/`with`); a fully-overlapping input (`insert … values …`) legitimately yields *two distinct commands* (`Command::Insert` typed-AST vs `Command::SqlInsert` validated-text) that do the same thing but execute differently (ADR-0030 §4), so each is tested in the mode that produces it; **corrects the plan's 3j exit-gate premise** that the DSL DML tests run in Simple mode (they call `parse_command`, which defaults to Advanced) — the real invariant is "Simple-mode behaviour unchanged, Advanced mode SQL-first, DSL grammar tested in Simple mode, both variants tested in their producing mode", with §6/§7 parity keeping the paths observably equivalent; and **defers to its own future ADR** the execution-time mode side-channel (three-way `Mode`: simple/advanced/advanced-one-shot threaded through `Action`→worker, for mode-dependent *output* like echoing generated SQL) — today only the *rendering* side-channel `OutputLine.mode_at_submission` exists, and the three-way distinction is not required for Phase 3 dispatch correctness - [ADR-0034 — `history.log` as a complete command journal; replay reads success-only](0034-history-journal-and-replay-filter.md) — **Accepted**, resolves a three-way tension in `history.log`'s roles found while implementing ADR-0033 3f: (1) the persistent log is success-only while the in-memory Up/Down recall ring records *every* submission (success or failure, "so users can recall and edit typo'd commands"), and the ring is re-seeded from the log on project open — so **failed commands are recallable within a session but silently lost across sessions**; (2) replay wants the state-building (successful) commands while recall wants everything typed, which one success-only file cannot serve; (3) `replay history.log` never actually worked — `run_replay` parses each whole line through the DSL parser with no understanding of the `||` record shape, so a real log fails on line 1, and **no test ever fed the pipe format to replay** (the `replay_history_log_records_subcommands_only` test only checks what replay *writes*, never replays the log as input). Decision: `history.log` becomes a **complete journal** — every submission recorded, tagged `ok`/`err` via the status field the format already reserved (ADR-0015 §5) — and **each consumer filters**: hydration reads all records (cross-session recall matches in-session), replay reads `ok` only (and learns the journal format, while still accepting bare-command `.commands` scripts; detection by the leading timestamp+status prefix so a `|` inside a bare command isn't misread). Successful commands stay journalled transactionally by the worker; failed commands are journalled `err` best-effort from the runtime/app error path (a parse failure never reaches the worker). Amends ADR-0006's "successfully executed" wording and ADR-0015 §5 ("status always `ok`") / §12 (hydration). Code deferred to two tracked test-first sub-tasks (journal-failures+filtering; replay-parses-journal-format); existing all-`ok` logs need no migration; **implemented 2026-05-24** (plan `docs/plans/20260524-adr-0034-history-journal.md`); **Amendment 1 (2026-05-24): replay filters out app-lifecycle commands** — a working `replay history.log` (the §3 fix) exposed that the journal also records `save as`/`load`/`new`/`export`/`import`/`rebuild`/`mode` (which would panic the worker dispatch or abort the replay), so replay now re-applies **only** schema/data write commands and **skips** every `Command::App` + nested `Command::Replay`; **all skips continue** (never abort — reversing the prior nested-`replay` refusal, so a journal containing a once-run `replay` needs no hand-editing, and the infinite-loop footgun is closed by construction), with a `[skip]` **warning** on `import` and nested-`replay` skips (their omission can leave replayed state incomplete) and silent skips for the rest; `replay.error_nested` removed, `replay.skipped_import`/`replay.skipped_replay` added, `ReplayCompleted` carries `warnings` -- [ADR-0035 — Advanced-mode SQL DDL](0035-advanced-mode-sql-ddl.md) — **Accepted** (design agreed 2026-05-24; validated end-to-end by sub-phases 4a/4a.2/4a.3/4b `CREATE TABLE` (incl. foreign keys) + 4c `DROP TABLE [IF EXISTS]` + 4d `CREATE [UNIQUE] INDEX` / `DROP INDEX [IF EXISTS]` + 4e `ALTER TABLE` add/drop/rename column, implemented 2026-05-25; 4f–4i pending), **Phase 4** of the ADR-0030 roadmap (peer of 0031/0032/0033) and **clarifies ADR-0030 §4**. Advanced-mode `CREATE`/`DROP`/`ALTER TABLE` + `CREATE`/`DROP INDEX` get their **own per-statement commands** (`SqlCreateTable`/`SqlAlterTable`/`SqlDropTable`/`SqlCreateIndex`/`SqlDropIndex`), like DML's `Sql*` set — but unlike DML they **execute *structurally*, not verbatim** (raw execution would lose the playground's types, named relationships, and `STRICT`; "verbatim" was a DML convenience, not a rule). Handlers **reuse the low-level schema/metadata helpers** where the operation matches simple mode and **stand alone where the SQL surface is richer** (clarity over forced refactoring); simple mode is untouched (additive). Dispatch: `create`/`drop` reuse ADR-0033 Amendment 1's category-grouped mode-aware dispatch (SQL-first, simple fallback); `alter` is a new advanced-only entry word. Full surface (no pre-emptive cuts, `Q4`): `CREATE TABLE` with column + table constraints, single/compound `PRIMARY KEY`, inline + table-level `FOREIGN KEY` → **named relationships** (one statement = one command = **one undo step**, ADR-0006); `ALTER TABLE` add/drop/rename column, `ALTER COLUMN TYPE`, add/drop constraint, add FK, **`RENAME TO`** (advanced-only table rename — new low-level op renaming the table + its CSV + the relationship and table-CHECK metadata, closing the rename half of `C1`); `CREATE [UNIQUE] INDEX` / `DROP INDEX`. Type slot accepts the ten playground keywords **and** standard-SQL aliases (`integer`→`int`, `varchar`→`text`, `timestamp`→`datetime`, …; length args accepted-and-ignored; no engine type names in/out — ADR-0030 §5). `CHECK`/`DEFAULT` reuse ADR-0031 `sql_expr`. **Pre-implementation `/runda` refinements (2026-05-24, user-confirmed):** `CREATE TABLE`/`DROP TABLE` **admit `IF [NOT] EXISTS`** (no-op-that-succeeds-with-a-note — a near-universal cross-vendor idiom, reclassified *into* scope, not engine-specific); `INTEGER PRIMARY KEY` maps to a **plain `int`** PK, *not* auto-increment (`serial` stays the sole auto-increment type). **Column-type-conversion is unified** (ADR-0017 engine, mode-appropriate policy): clean auto-converts and incompatible/own-type-static cases refuse in both modes, but a **lossy** change refuses-by-default in simple mode (`--force-conversion` opts in) while advanced mode **performs it with a loss note** and relies on **`undo` as the safety net** — no force flag, no dropping to simple mode (a payoff of shipping ADR-0006 first). OOS: views/triggers/txn-control/PRAGMA/etc. (ADR-0030 §3), the Postgres `USING` clause, and the DSL→SQL teaching echo (ADR-0030 Phase 5). Sub-phases 4a–4i, plus **4a.2** (per-column `CHECK`/`DEFAULT` via raw `sql_expr` text — `sql_expr` is validate-only, no `Expr` AST — + composite `UNIQUE(a,b)`; no new internal table) and **4a.3** (table-level/multi-column `CHECK`, landed via the new `__rdbms_playground_table_checks` metadata table because SQLite has no PRAGMA for CHECK; the builder tells a table-level CHECK from a column-level one by element position) and **4b** (foreign keys — inline `REFERENCES` + table-level `FOREIGN KEY` → ADR-0013 named relationships in the create transaction, one undo step; self-references + bare `REFERENCES ` supported, user-confirmed) and **4c** (`DROP TABLE [IF EXISTS]` → `SqlDropTable`, reusing `do_drop_table`; `IF EXISTS` is a no-op-with-note via `DropOutcome::Skipped`) and **4d** (`CREATE [UNIQUE] INDEX [IF NOT EXISTS] [] ON (cols)` → `SqlCreateIndex` and `DROP INDEX [IF EXISTS] ` → `SqlDropIndex`, reusing `do_add_index`/`do_drop_index`; **`CREATE UNIQUE INDEX` admitted** — ADR-0025 **Amendment 1** — via an additive `IndexSchema.unique` flag that round-trips through `project.yaml` and rebuild, with `[unique]` markers in the structure view + items panel, while simple-mode `add unique index` stays deferred; `IF [NOT] EXISTS` reuses the 4c skip path; `create`/`drop` each gain a *second* advanced node, exercising the all-candidates dispatch) and **4e** (`ALTER TABLE` add/drop/rename column → `SqlAlterTable`; `alter` is a new advanced-**only** entry word, runtime-decomposed to the existing `do_add_column`/`do_drop_column`/`do_rename_column` — no new worker layer; `do_add_column` extended to consume raw `default_sql`/`check_sql` so ADD COLUMN reaches CREATE-TABLE constraint parity; drop/rename refuse a column any CHECK references (table-level AND column-level, incl. a column's own self-check on rename) — the 4a.3 deferral, via a raw-CHECK-text tokenizer in the shared executors, so it guards both surfaces and fixes a latent rename-drift bug; SQL DROP COLUMN refuses an index-covered column with no `--cascade` spelling; the column executors + `do_add_index` gained an internal-`__rdbms_*`-table guard — all user-confirmed); the remaining DDL forms stay "not yet supported" until their sub-phases land. Each sub-phase has exit + DA gates +- [ADR-0035 — Advanced-mode SQL DDL](0035-advanced-mode-sql-ddl.md) — **Accepted** (design agreed 2026-05-24; validated end-to-end by sub-phases 4a/4a.2/4a.3/4b `CREATE TABLE` (incl. foreign keys) + 4c `DROP TABLE [IF EXISTS]` + 4d `CREATE [UNIQUE] INDEX` / `DROP INDEX [IF EXISTS]` + 4e `ALTER TABLE` add/drop/rename column + 4f `ALTER TABLE … ALTER COLUMN TYPE`, implemented 2026-05-25; 4g–4i pending), **Phase 4** of the ADR-0030 roadmap (peer of 0031/0032/0033) and **clarifies ADR-0030 §4**. Advanced-mode `CREATE`/`DROP`/`ALTER TABLE` + `CREATE`/`DROP INDEX` get their **own per-statement commands** (`SqlCreateTable`/`SqlAlterTable`/`SqlDropTable`/`SqlCreateIndex`/`SqlDropIndex`), like DML's `Sql*` set — but unlike DML they **execute *structurally*, not verbatim** (raw execution would lose the playground's types, named relationships, and `STRICT`; "verbatim" was a DML convenience, not a rule). Handlers **reuse the low-level schema/metadata helpers** where the operation matches simple mode and **stand alone where the SQL surface is richer** (clarity over forced refactoring); simple mode is untouched (additive). Dispatch: `create`/`drop` reuse ADR-0033 Amendment 1's category-grouped mode-aware dispatch (SQL-first, simple fallback); `alter` is a new advanced-only entry word. Full surface (no pre-emptive cuts, `Q4`): `CREATE TABLE` with column + table constraints, single/compound `PRIMARY KEY`, inline + table-level `FOREIGN KEY` → **named relationships** (one statement = one command = **one undo step**, ADR-0006); `ALTER TABLE` add/drop/rename column, `ALTER COLUMN TYPE`, add/drop constraint, add FK, **`RENAME TO`** (advanced-only table rename — new low-level op renaming the table + its CSV + the relationship and table-CHECK metadata, closing the rename half of `C1`); `CREATE [UNIQUE] INDEX` / `DROP INDEX`. Type slot accepts the ten playground keywords **and** standard-SQL aliases (`integer`→`int`, `varchar`→`text`, `timestamp`→`datetime`, …; length args accepted-and-ignored; no engine type names in/out — ADR-0030 §5). `CHECK`/`DEFAULT` reuse ADR-0031 `sql_expr`. **Pre-implementation `/runda` refinements (2026-05-24, user-confirmed):** `CREATE TABLE`/`DROP TABLE` **admit `IF [NOT] EXISTS`** (no-op-that-succeeds-with-a-note — a near-universal cross-vendor idiom, reclassified *into* scope, not engine-specific); `INTEGER PRIMARY KEY` maps to a **plain `int`** PK, *not* auto-increment (`serial` stays the sole auto-increment type). **Column-type-conversion is unified** (ADR-0017 engine, mode-appropriate policy): clean auto-converts and incompatible/own-type-static cases refuse in both modes, but a **lossy** change refuses-by-default in simple mode (`--force-conversion` opts in) while advanced mode **performs it with a loss note** and relies on **`undo` as the safety net** — no force flag, no dropping to simple mode (a payoff of shipping ADR-0006 first). OOS: views/triggers/txn-control/PRAGMA/etc. (ADR-0030 §3), the Postgres `USING` clause, and the DSL→SQL teaching echo (ADR-0030 Phase 5). Sub-phases 4a–4i, plus **4a.2** (per-column `CHECK`/`DEFAULT` via raw `sql_expr` text — `sql_expr` is validate-only, no `Expr` AST — + composite `UNIQUE(a,b)`; no new internal table) and **4a.3** (table-level/multi-column `CHECK`, landed via the new `__rdbms_playground_table_checks` metadata table because SQLite has no PRAGMA for CHECK; the builder tells a table-level CHECK from a column-level one by element position) and **4b** (foreign keys — inline `REFERENCES` + table-level `FOREIGN KEY` → ADR-0013 named relationships in the create transaction, one undo step; self-references + bare `REFERENCES ` supported, user-confirmed) and **4c** (`DROP TABLE [IF EXISTS]` → `SqlDropTable`, reusing `do_drop_table`; `IF EXISTS` is a no-op-with-note via `DropOutcome::Skipped`) and **4d** (`CREATE [UNIQUE] INDEX [IF NOT EXISTS] [] ON (cols)` → `SqlCreateIndex` and `DROP INDEX [IF EXISTS] ` → `SqlDropIndex`, reusing `do_add_index`/`do_drop_index`; **`CREATE UNIQUE INDEX` admitted** — ADR-0025 **Amendment 1** — via an additive `IndexSchema.unique` flag that round-trips through `project.yaml` and rebuild, with `[unique]` markers in the structure view + items panel, while simple-mode `add unique index` stays deferred; `IF [NOT] EXISTS` reuses the 4c skip path; `create`/`drop` each gain a *second* advanced node, exercising the all-candidates dispatch) and **4e** (`ALTER TABLE` add/drop/rename column → `SqlAlterTable`; `alter` is a new advanced-**only** entry word, runtime-decomposed to the existing `do_add_column`/`do_drop_column`/`do_rename_column` — no new worker layer; `do_add_column` extended to consume raw `default_sql`/`check_sql` so ADD COLUMN reaches CREATE-TABLE constraint parity; drop/rename refuse a column any CHECK references (table-level AND column-level, incl. a column's own self-check on rename) — the 4a.3 deferral, via a raw-CHECK-text tokenizer in the shared executors, so it guards both surfaces and fixes a latent rename-drift bug; SQL DROP COLUMN refuses an index-covered column with no `--cascade` spelling; the column executors + `do_add_index` gained an internal-`__rdbms_*`-table guard — all user-confirmed) and **4f** (`ALTER TABLE … ALTER COLUMN TYPE` → a fourth `AlterTableAction`, runtime-decomposed to the existing `change_column_type` with `ChangeColumnMode::ForceConversion` — which **is** the §7 advanced policy: lossy converts *with a note* (no force flag), incompatible + ADR-0017 static refusals (`↔ blob`, same-type, `date ↔ datetime`, non-`int → serial`) still refuse, while **`int → serial` is allowed** (auto-fills nulls + UNIQUE, ADR-0018 §8 — the §7 "→serial refused" summary is looser than the code); the builder discriminates the fourth branch by the **`type` keyword** (unique — ADD COLUMN's type is an ident), the type slot reuses `SQL_TYPE`; the internal-`__rdbms_*` guard was folded into `do_change_column_type`, closing the simple `change column` exposure too — user-confirmed); the remaining DDL forms stay "not yet supported" until their sub-phases land. Each sub-phase has exit + DA gates diff --git a/docs/requirements.md b/docs/requirements.md index 6747458..55cbaf4 100644 --- a/docs/requirements.md +++ b/docs/requirements.md @@ -234,9 +234,13 @@ handoff-14 cleanup; 449 after B2/C2.) then `ALTER TABLE` add/drop/rename column (4e — `alter` is advanced-only, runtime-decomposed to the existing column executors; ADD COLUMN reaches CREATE-TABLE constraint parity; drop/rename refuse a table-CHECK- - referenced column)). - Remaining DDL — `ALTER TABLE … ALTER COLUMN TYPE` / add-drop-constraint / - add-FK / `RENAME TO` (4f–4h) — is phased per ADR-0035 §13.)* + referenced column), then `ALTER TABLE … ALTER COLUMN TYPE` (4f — + runtime-decomposed to `change_column_type` with `ForceConversion`, the + §7 advanced policy: lossy converts with a note, incompatible + static + refusals (`↔ blob`, non-`int → serial`) refuse, `int → serial` allowed; + the internal-`__rdbms_*` guard folded into `do_change_column_type`)). + Remaining DDL — `ALTER TABLE` add-drop-constraint / add-FK / `RENAME TO` + (4g–4h) — is phased per ADR-0035 §13.)* - [ ] **Q2** Non-standard syntax rejected with a clear message pointing at the supported subset. *(Design done — ADR-0030 §8: out-of-subset statements are diff --git a/src/app.rs b/src/app.rs index a024984..934609e 100644 --- a/src/app.rs +++ b/src/app.rs @@ -1597,6 +1597,11 @@ impl App { Some(table.as_str()), Some(old.as_str()), ), + AlterTableAction::AlterColumnType { column, .. } => ( + Operation::ChangeColumnType, + Some(table.as_str()), + Some(column.as_str()), + ), }, C::SqlCreateTable { name, .. } => { (Operation::CreateTable, Some(name.as_str()), None) diff --git a/src/db.rs b/src/db.rs index 38d27d1..a8f03c7 100644 --- a/src/db.rs +++ b/src/db.rs @@ -4279,6 +4279,11 @@ fn do_change_column_type( ty: Type, mode: ChangeColumnMode, ) -> Result { + // Refuse the internal `__rdbms_*` tables up-front (as "no such + // table"), like the sibling column executors. Closes the simple + // `change column` exposure and the SQL `ALTER COLUMN TYPE` + // decomposition target (ADR-0035 §4f); user-confirmed 2026-05-25. + reject_internal_table_name(table)?; let old_schema = read_schema(conn, table)?; let col_info = old_schema .columns diff --git a/src/dsl/command.rs b/src/dsl/command.rs index 4edef10..df3e93e 100644 --- a/src/dsl/command.rs +++ b/src/dsl/command.rs @@ -714,9 +714,11 @@ pub enum IndexSelector { Columns { table: String, columns: Vec }, } -/// The action of an advanced-mode `ALTER TABLE` (ADR-0035 §4). Sub-phase -/// 4e carries the column actions; 4f/4g/4h add `AlterColumnType`, -/// `AddConstraint`/`AddForeignKey`/`DropConstraint`, and `RenameTo`. +/// The action of an advanced-mode `ALTER TABLE` (ADR-0035 §4). +/// +/// Sub-phase 4e carries the column actions; 4f adds `AlterColumnType`; +/// 4g/4h add `AddConstraint`/`AddForeignKey`/`DropConstraint`, and +/// `RenameTo`. #[derive(Debug, Clone, PartialEq, Eq)] pub enum AlterTableAction { /// `ADD COLUMN [NOT NULL] [UNIQUE] [DEFAULT …] @@ -731,6 +733,12 @@ pub enum AlterTableAction { DropColumn { column: String }, /// `RENAME COLUMN TO ` — reuses `do_rename_column`. RenameColumn { old: String, new: String }, + /// `ALTER COLUMN TYPE ` — reuses `do_change_column_type` + /// with `ChangeColumnMode::ForceConversion`, which is the ADR-0035 §7 + /// advanced-mode policy (lossy cells are *performed* with a note, no + /// force flag; static-refused / incompatible still refuse). One undo + /// step (the executor's rebuild). ADR-0035 §4f. + AlterColumnType { column: String, ty: Type }, } impl std::fmt::Display for IndexSelector { diff --git a/src/dsl/grammar/ddl.rs b/src/dsl/grammar/ddl.rs index 3b62475..5f7f942 100644 --- a/src/dsl/grammar/ddl.rs +++ b/src/dsl/grammar/ddl.rs @@ -1905,9 +1905,24 @@ static AT_RENAME_COLUMN_NODES: &[Node] = &[ ]; const AT_RENAME_COLUMN: Node = Node::Seq(AT_RENAME_COLUMN_NODES); -// Each action branch leads on a concrete keyword (`add`/`drop`/ -// `rename`) — trap-safe. -static AT_ACTION_CHOICES: &[Node] = &[AT_ADD_COLUMN, AT_DROP_COLUMN, AT_RENAME_COLUMN]; +// `ALTER COLUMN TYPE ` (ADR-0035 §4f). The type slot reuses +// SQL_TYPE (the same alias map + `double precision` pair the CREATE +// TABLE / ADD COLUMN forms use). The builder keys on the `type` keyword +// — unique to this action (ADD COLUMN's type is a `col_type` ident). +static AT_ALTER_COLUMN_NODES: &[Node] = &[ + Node::Word(Word::keyword("alter")), + Node::Word(Word::keyword("column")), + COLUMN_NAME, + Node::Word(Word::keyword("type")), + super::sql_create_table::SQL_TYPE, +]; +const AT_ALTER_COLUMN: Node = Node::Seq(AT_ALTER_COLUMN_NODES); + +// Each action branch leads on a concrete keyword (`add`/`drop`/`rename`/ +// `alter`) — trap-safe. (The branch's `alter` is the action word; the +// entry-word `alter` was already consumed by dispatch.) +static AT_ACTION_CHOICES: &[Node] = + &[AT_ADD_COLUMN, AT_DROP_COLUMN, AT_RENAME_COLUMN, AT_ALTER_COLUMN]; const AT_ACTION: Node = Node::Choice(AT_ACTION_CHOICES); static SQL_ALTER_TABLE_SHAPE_NODES: &[Node] = &[ @@ -1989,12 +2004,54 @@ fn build_alter_add_column_spec( }) } -/// Build `Command::SqlAlterTable` (ADR-0035 §4e). The action is the -/// leading concrete keyword (`add`/`drop`/`rename` — exactly one matches -/// per the action `Choice`). +/// Extract the `ALTER COLUMN TYPE ` action (ADR-0035 §4f). +/// The type slot reuses SQL_TYPE, so the target-type extraction mirrors +/// `build_alter_add_column_spec`'s: a `col_type` ident via +/// `Type::from_sql_name` (alias map applied), or the two-word +/// `double precision` → `Type::Real`. +fn build_alter_column_type(path: &MatchedPath) -> Result { + let column = require_ident(path, "column_name")?; + let mut ty: Option = None; + let mut items = path.items.iter().peekable(); + while let Some(item) = items.next() { + match &item.kind { + MatchedKind::Ident { role: "col_type", .. } => { + ty = Some(Type::from_sql_name(&item.text).ok_or_else(|| ValidationError { + message_key: "parse.error_wrapper", + args: vec![("detail", "unknown type".to_string())], + })?); + } + MatchedKind::Word("double") => { + if matches!( + items.peek().map(|i| &i.kind), + Some(MatchedKind::Word("precision")) + ) { + items.next(); + } + ty = Some(Type::Real); + } + _ => {} + } + } + let ty = ty.ok_or_else(|| ValidationError { + message_key: "parse.error_wrapper", + args: vec![("detail", "alter column needs a target type".to_string())], + })?; + Ok(AlterTableAction::AlterColumnType { column, ty }) +} + +/// Build `Command::SqlAlterTable` (ADR-0035 §4e/§4f). The action is the +/// leading concrete keyword (`add`/`drop`/`rename`/`alter` — exactly one +/// matches per the action `Choice`). The `type` keyword is checked +/// **first**: it is unique to ALTER COLUMN TYPE (ADD COLUMN's type is a +/// `col_type` *ident*, not the literal word), and an `alter column …` +/// input contains none of add/drop/rename, so without this it would fall +/// through to the DropColumn arm. fn build_sql_alter_table(path: &MatchedPath, source: &str) -> Result { let table = require_ident(path, "table_name")?; - let action = if path.contains_word("add") { + let action = if path.contains_word("type") { + build_alter_column_type(path)? + } else if path.contains_word("add") { AlterTableAction::AddColumn(Box::new(build_alter_add_column_spec(path, source)?)) } else if path.contains_word("rename") { AlterTableAction::RenameColumn { @@ -2551,6 +2608,94 @@ mod sql_alter_table_tests { )); } + #[test] + fn alter_column_type_parses() { + // ADR-0035 §4f: the fourth action, discriminated by the `type` + // keyword (ADD COLUMN's type is an ident, not the literal word). + let (table, action) = alter("alter table T alter column qty type int"); + assert_eq!(table, "T"); + match action { + AlterTableAction::AlterColumnType { column, ty } => { + assert_eq!(column, "qty"); + assert_eq!(ty, crate::dsl::types::Type::Int); + } + other => panic!("expected AlterColumnType, got {other:?}"), + } + // trailing semicolon tolerated + assert!(matches!( + alter("alter table T alter column qty type int;").1, + AlterTableAction::AlterColumnType { .. } + )); + } + + #[test] + fn alter_column_type_accepts_sql_type_alias() { + // `integer` → int, `double precision` → real (ADR-0035 §3), + // reusing SQL_TYPE for the type slot. + match alter("alter table T alter column n type integer").1 { + AlterTableAction::AlterColumnType { ty, .. } => { + assert_eq!(ty, crate::dsl::types::Type::Int); + } + other => panic!("expected AlterColumnType, got {other:?}"), + } + match alter("alter table T alter column n type double precision").1 { + AlterTableAction::AlterColumnType { ty, .. } => { + assert_eq!(ty, crate::dsl::types::Type::Real); + } + other => panic!("expected AlterColumnType, got {other:?}"), + } + } + + #[test] + fn four_branch_dispatch_still_routes_the_column_actions() { + // The new `alter column type` branch does not steal add/drop/ + // rename: each still routes to its own action. + assert!(matches!( + alter("alter table T add column note text").1, + AlterTableAction::AddColumn(_) + )); + assert!(matches!( + alter("alter table T drop column note").1, + AlterTableAction::DropColumn { .. } + )); + assert!(matches!( + alter("alter table T rename column a to b").1, + AlterTableAction::RenameColumn { .. } + )); + assert!(matches!( + alter("alter table T alter column a type text").1, + AlterTableAction::AlterColumnType { .. } + )); + } + + #[test] + fn type_discriminator_probe_column_named_type() { + // PROBE (DA): the `type`-keyword discriminator keys on the literal + // `type` *keyword* node, which only the ALTER COLUMN TYPE branch + // carries. Verify a column whose *name* is `type` does not get + // misrouted (it is an ident, not a Word). If `type` is reserved + // and rejected as an ident, the parse errors — either outcome is + // fine; the failure we guard against is silent misrouting to + // AlterColumnType (which would then error on a missing type). + let dropped = parse_command_in_mode("alter table T drop column type", Mode::Advanced); + match dropped { + Ok(Command::SqlAlterTable { + action: AlterTableAction::DropColumn { column }, + .. + }) => assert_eq!(column, "type", "a column named `type` drops correctly"), + Ok(other) => panic!("`drop column type` misrouted to {other:?}"), + Err(_) => { /* `type` rejected as an ident — acceptable, no misroute */ } + } + // And the real ALTER COLUMN TYPE still routes (sanity). + assert!(matches!( + parse_command_in_mode("alter table T alter column c type int", Mode::Advanced), + Ok(Command::SqlAlterTable { + action: AlterTableAction::AlterColumnType { .. }, + .. + }) + )); + } + #[test] fn alter_is_advanced_only() { // No simple `alter`; in simple mode it does not parse as a diff --git a/src/friendly/strings/en-US.yaml b/src/friendly/strings/en-US.yaml index 8aa09c5..f424123 100644 --- a/src/friendly/strings/en-US.yaml +++ b/src/friendly/strings/en-US.yaml @@ -273,7 +273,8 @@ help: sql_alter_table: |- alter table add column [not null] [unique] [default …] [check …] alter table drop column - alter table rename column to — change a table's columns (advanced SQL) + alter table rename column to + alter table alter column type — change a table's columns (advanced SQL) drop: |- drop table — remove a table drop column [from] [table] : [--cascade] — remove a column @@ -473,6 +474,7 @@ parse: alter table add column [not null] [unique] [default ] [check ()] alter table
drop column alter table
rename column to + alter table
alter column type drop_table: "drop table " drop_column: "drop column [from] [table]
: " drop_relationship: |- @@ -871,8 +873,10 @@ ok: client_side: # Per-cell transformation notice when `change column ...` rewrote # at least one stored value (storage-class change or non-identity - # mapping). `lossy` variant fires under --force-conversion when - # information was discarded. + # mapping). `lossy` variant fires when information was discarded — + # under simple-mode `--force-conversion`, and under advanced-mode + # `alter table … alter column … type …`, which always converts + # (ADR-0035 §7). transformed: |- [client-side] {count} row(s) were transformed before being stored. In raw SQL this would need an explicit `CAST` or application-level code. transformed_lossy: |- diff --git a/src/runtime.rs b/src/runtime.rs index afb2a58..0cf5aed 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -33,7 +33,7 @@ use crate::db::{ Database, DbError, DeleteResult, DropColumnResult, DropIndexOutcome, DropOutcome, InsertResult, QueryPlan, TableDescription, UpdateResult, }; -use crate::dsl::{AlterTableAction, Command, ColumnSpec}; +use crate::dsl::{AlterTableAction, ChangeColumnMode, Command, ColumnSpec}; use crate::dsl::walker::Severity; use crate::event::AppEvent; use crate::project::{ @@ -2114,6 +2114,16 @@ async fn execute_command_typed( .rename_column(table, old, new, src) .await .map(|d| CommandOutcome::Schema(Some(d))), + // `ALTER COLUMN … TYPE` reuses the simple `change column` + // executor with `ForceConversion` — the ADR-0035 §7 + // advanced policy (lossy converts with a note; no force + // flag; static-refused / incompatible still refuse). The + // ChangeColumn outcome surfaces the client-side lossy note, + // shared with simple mode. + AlterTableAction::AlterColumnType { column, ty } => database + .change_column_type(table, column, ty, ChangeColumnMode::ForceConversion, src) + .await + .map(CommandOutcome::ChangeColumn), }, Command::AddConstraint { table, diff --git a/tests/column_op_guards.rs b/tests/column_op_guards.rs index 7ad51f4..6dfa7f4 100644 --- a/tests/column_op_guards.rs +++ b/tests/column_op_guards.rs @@ -10,7 +10,7 @@ //! rename-drift bug that would break a later rebuild). use rdbms_playground::db::Database; -use rdbms_playground::dsl::{ColumnSpec, Type}; +use rdbms_playground::dsl::{ChangeColumnMode, ColumnSpec, Type}; use rdbms_playground::persistence::Persistence; use rdbms_playground::project; @@ -72,10 +72,34 @@ fn simple_column_ops_refuse_internal_tables() { "drop column on an internal table is refused" ); assert!( - r.block_on(db.rename_column(internal, "table_name".to_string(), "tn".to_string(), None)) - .is_err(), + r.block_on(db.rename_column( + internal.clone(), + "table_name".to_string(), + "tn".to_string(), + None + )) + .is_err(), "rename column on an internal table is refused" ); + // `change column` (the simple surface; also the SQL `ALTER COLUMN + // TYPE` decomposition target — ADR-0035 §4f) is refused too: the + // guard lives in `do_change_column_type`. It refuses up-front as + // "no such table" (the sibling-executor contract), not via the + // incidental "no user-facing type metadata" path internal tables + // happen to hit. + let err = r + .block_on(db.change_column_type( + internal, + "table_name".to_string(), + Type::Int, + ChangeColumnMode::Default, + None, + )) + .expect_err("change column type on an internal table is refused"); + assert!( + format!("{err:?}").contains("NoSuchTable"), + "expected a no-such-table refusal from the internal-table guard, got: {err:?}" + ); } #[test] diff --git a/tests/sql_alter_table.rs b/tests/sql_alter_table.rs index 444a5e3..484f873 100644 --- a/tests/sql_alter_table.rs +++ b/tests/sql_alter_table.rs @@ -1,17 +1,21 @@ -//! Sub-phase 4e Tier-3 end-to-end tests for advanced-mode SQL -//! `ALTER TABLE` add/drop/rename column (ADR-0035 §4e). +//! Sub-phase 4e/4f Tier-3 end-to-end tests for advanced-mode SQL +//! `ALTER TABLE` (ADR-0035 §4e + §4f). //! //! These drive the **full advanced-mode pipeline** via `run_replay`: a //! literal `alter table …` line is parsed in Advanced mode, routed to //! `Command::SqlAlterTable`, decomposed by the runtime to the existing -//! column executor, and persisted. They prove the decomposition for all -//! three actions and the **raw-text DEFAULT/CHECK ADD COLUMN** path (the -//! 4e executor extension). The drop/rename refusals (PK / FK / index / -//! table-CHECK) live in the shared executors and are covered by -//! `tests/column_op_guards.rs` — the SQL surface reaches the same code. +//! column executor, and persisted. 4e proves the decomposition for +//! add/drop/rename column and the **raw-text DEFAULT/CHECK ADD COLUMN** +//! path; 4f adds `ALTER COLUMN TYPE `, decomposed to +//! `change_column_type` with `ChangeColumnMode::ForceConversion` — the +//! §7 advanced policy (lossy converts with a note, no force flag; +//! static-refused / incompatible still refuse). The drop/rename refusals +//! (PK / FK / index / table-CHECK) and the internal-table guard live in +//! the shared executors and are covered by `tests/column_op_guards.rs` — +//! the SQL surface reaches the same code. use rdbms_playground::db::Database; -use rdbms_playground::dsl::Value; +use rdbms_playground::dsl::{Type, Value}; use rdbms_playground::event::AppEvent; use rdbms_playground::persistence::Persistence; use rdbms_playground::project; @@ -36,6 +40,41 @@ fn open() -> (project::Project, Database, tempfile::TempDir) { (project, db, dir) } +fn open_with_undo() -> (project::Project, Database, tempfile::TempDir) { + let dir = tempfile::tempdir().expect("create tempdir"); + let project = + project::open_or_create(None, Some(dir.path())).expect("open or create project"); + let db = Database::open_with_persistence_and_undo( + project.db_path(), + Persistence::new(project.path().to_path_buf()), + true, + ) + .expect("db"); + (project, db, dir) +} + +/// Run a single-conversion script through the full pipeline and report +/// whether it aborted with a `ReplayFailed` (i.e. the command was +/// refused). Used to assert the SQL `ALTER COLUMN TYPE` path reaches the +/// shared executor's static / incompatible refusals. +fn replay_is_refused(script: &str) -> bool { + let (project, db, _d) = open(); + let r = rt(); + std::fs::write(project.path().join("conv.commands"), script).expect("write script"); + let events = r.block_on(run_replay(&db, project.path(), "conv.commands")); + matches!(events.last(), Some(AppEvent::ReplayFailed { .. })) +} + +/// The current user-facing type of column `name` in table `T`. +fn col_type(db: &Database, r: &tokio::runtime::Runtime, name: &str) -> Option { + r.block_on(db.describe_table("T".to_string(), None)) + .expect("describe") + .columns + .into_iter() + .find(|c| c.name == name) + .and_then(|c| c.user_type) +} + fn column_names(db: &Database, r: &tokio::runtime::Runtime) -> Vec { r.block_on(db.describe_table("T".to_string(), None)) .expect("describe") @@ -138,3 +177,184 @@ fn e2e_alter_add_column_survives_rebuild() { "the ALTER-added CHECK is intact after rebuild" ); } + +// --- 4f: ALTER COLUMN … TYPE (ADR-0035 §4f) ----------------------------- + +#[test] +fn e2e_alter_column_type_clean_and_lossy_convert() { + // The key 4f assertion: the SQL ALTER COLUMN TYPE path wires + // `ForceConversion`. A lossy `real → int` (3.7 → 3) is therefore + // *performed*, not refused — under `Default` mode the replay line + // would refuse and abort (count < 6). A clean `int → text` stringifies. + let (project, db, _d) = open(); + let r = rt(); + std::fs::write( + project.path().join("conv.commands"), + "create table T with pk id(int)\n\ + add column T: v (real)\n\ + add column T: w (int)\n\ + insert into T (id, v, w) values (1, 3.7, 42)\n\ + alter table T alter column v type int\n\ + alter table T alter column w type text\n", + ) + .expect("write script"); + + let events = r.block_on(run_replay(&db, project.path(), "conv.commands")); + match events.last().expect("at least one event") { + AppEvent::ReplayCompleted { count, .. } => { + assert_eq!(*count, 6, "all six lines replayed; events: {events:?}"); + } + other => panic!("expected ReplayCompleted, got {other:?} (events: {events:?})"), + } + + let rows = r + .block_on(db.query_data("T".to_string(), None, None, None)) + .expect("query") + .rows; + assert_eq!(rows.len(), 1); + // v (col 1): lossy real→int performed → 3.7 stored as 3. + assert_eq!(rows[0][1].as_deref(), Some("3"), "lossy real→int performed (3.7→3)"); + // w (col 2): clean int→text stringified → "42". + assert_eq!(rows[0][2].as_deref(), Some("42"), "clean int→text stringified"); + + // The columns now carry the new user-facing types (round-tripped + // through the metadata). + assert_eq!(col_type(&db, &r, "v"), Some(Type::Int)); + assert_eq!(col_type(&db, &r, "w"), Some(Type::Text)); +} + +#[test] +fn e2e_alter_column_type_int_to_serial_is_allowed() { + // ADR-0035 §7's "static-refused (→serial …)" summary is looser than + // the code: `int → serial` IS allowed (ADR-0018 §8 — auto-fills nulls, + // adds UNIQUE on a non-PK column). The SQL path reaches that supported + // conversion; the pre-existing non-null value is preserved. + let (project, db, _d) = open(); + let r = rt(); + std::fs::write( + project.path().join("conv.commands"), + "create table T with pk id(int)\n\ + add column T: n (int)\n\ + insert into T (id, n) values (1, 100)\n\ + alter table T alter column n type serial\n", + ) + .expect("write script"); + let events = r.block_on(run_replay(&db, project.path(), "conv.commands")); + match events.last().expect("at least one event") { + AppEvent::ReplayCompleted { count, .. } => { + assert_eq!(*count, 4, "all four lines replayed; events: {events:?}"); + } + other => panic!("expected ReplayCompleted, got {other:?} (events: {events:?})"), + } + assert_eq!(col_type(&db, &r, "n"), Some(Type::Serial), "int→serial converted the column"); + let rows = r + .block_on(db.query_data("T".to_string(), None, None, None)) + .expect("query") + .rows; + assert_eq!(rows[0][1].as_deref(), Some("100"), "the existing value is preserved"); +} + +#[test] +fn e2e_alter_column_type_incompatible_is_refused() { + // text "abc" → int has no valid per-cell conversion → refused (no + // force flag overrides incompatibles). The SQL path reaches the + // shared executor's incompatible refusal. + assert!( + replay_is_refused( + "create table T with pk id(int)\n\ + add column T: v (text)\n\ + insert into T (id, v) values (1, 'abc')\n\ + alter table T alter column v type int\n", + ), + "an incompatible text→int conversion is refused via the SQL path" + ); +} + +#[test] +fn e2e_alter_column_type_static_refusals() { + // Static refusals are shared by both modes (ADR-0017 §3); the SQL + // ALTER COLUMN TYPE path reaches them. + assert!( + replay_is_refused( + "create table T with pk id(int)\n\ + add column T: v (text)\n\ + alter table T alter column v type serial\n", + ), + "text→serial is refused (only int→serial is allowed)" + ); + assert!( + replay_is_refused( + "create table T with pk id(int)\n\ + add column T: v (text)\n\ + alter table T alter column v type blob\n", + ), + "↔ blob is statically refused" + ); +} + +#[test] +fn e2e_alter_column_type_on_fk_column_is_refused() { + // The column is the child side of a relationship (outbound FK); + // changing its type is refused for v1 (ADR-0017 §4.2). The SQL ALTER + // COLUMN TYPE path reaches the same executor precondition. + assert!( + replay_is_refused( + "create table P with pk id(int)\n\ + create table C with pk cid(int)\n\ + add column C: pid (int)\n\ + add 1:n relationship from P.id to C.pid\n\ + alter table C alter column pid type text\n", + ), + "changing the type of a child-side FK column is refused via the SQL path" + ); +} + +#[test] +fn e2e_alter_column_type_survives_rebuild() { + // The user_type metadata update is the existing path, so the + // converted type round-trips through the text artifacts and survives + // a rebuild. + let (project, db, _d) = open(); + let r = rt(); + std::fs::write( + project.path().join("conv.commands"), + "create table T with pk id(int)\n\ + add column T: v (real)\n\ + alter table T alter column v type int\n", + ) + .expect("write script"); + r.block_on(run_replay(&db, project.path(), "conv.commands")); + assert_eq!(col_type(&db, &r, "v"), Some(Type::Int), "converted before rebuild"); + + r.block_on(db.rebuild_from_text(project.path().to_path_buf(), Some("rebuild".to_string()))) + .expect("rebuild"); + assert_eq!(col_type(&db, &r, "v"), Some(Type::Int), "the converted type survives rebuild"); +} + +#[test] +fn e2e_alter_column_type_is_one_undo_step() { + // The runtime decomposes SqlAlterTable::AlterColumnType into ONE + // change_column_type call, so the whole conversion is one undo step + // (the executor's rebuild is one snapshot) — like the simple + // `change column`. Driven through the full SQL pipeline (run_replay + // fires the worker snapshot hook per command), then undone in one. + let (project, db, _d) = open_with_undo(); + let r = rt(); + std::fs::write( + project.path().join("conv.commands"), + "create table T with pk id(int)\n\ + add column T: v (real)\n\ + insert into T (id, v) values (1, 3.7)\n\ + alter table T alter column v type int\n", + ) + .expect("write script"); + r.block_on(run_replay(&db, project.path(), "conv.commands")); + assert_eq!(col_type(&db, &r, "v"), Some(Type::Int), "the SQL ALTER COLUMN TYPE converted v"); + + // A single undo reverts the whole conversion. + assert!( + r.block_on(db.undo()).expect("undo").is_some(), + "the conversion was one undo step" + ); + assert_eq!(col_type(&db, &r, "v"), Some(Type::Real), "one undo restored the pre-conversion type"); +}