diff --git a/docs/adr/0035-advanced-mode-sql-ddl.md b/docs/adr/0035-advanced-mode-sql-ddl.md index 6ca60dd..04919e5 100644 --- a/docs/adr/0035-advanced-mode-sql-ddl.md +++ b/docs/adr/0035-advanced-mode-sql-ddl.md @@ -4,14 +4,15 @@ 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 / 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 +4e / 4f / 4g** (`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, +`ALTER TABLE … ALTER COLUMN TYPE`, and `ALTER TABLE` add/drop constraint ++ add foreign key, 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`, `…-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 +`…-4e.md`, `…-4f.md`, `…-4g.md`), so the decision is accepted while the +remaining sub-phases (**4h–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. @@ -456,6 +457,30 @@ ADR-0033's structure: 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.** + *(Implemented 2026-05-25 — plan + `docs/plans/20260525-adr-0035-sql-ddl-4g.md`.)* `ALTER TABLE ADD + [CONSTRAINT ] (CHECK (…) | UNIQUE (…) | FOREIGN KEY (…) + REFERENCES …)` and `DROP CONSTRAINT `. **ADD scope (user- + confirmed):** CHECK + composite UNIQUE + FK; `ADD PRIMARY KEY` is + refused (every table already has a PK) and a **named UNIQUE** is + refused (composite UNIQUE is anonymous in our model — PRAGMA-detected, + §4a.2). Each ADD reuses an existing low-level path: table-CHECK and + composite-UNIQUE rebuild the table (dry-run guards reject existing + rows that would violate), FK decomposes to `add_relationship` (the + same machinery `add 1:n relationship` uses — bare `REFERENCES

` + resolves to the parent's single PK; `create_fk = false` as the column + must exist). **DROP CONSTRAINT (user-confirmed)** resolves the name to + a named table-CHECK then a named FK whose child is ``, else refuses. + **Named table-CHECK round-trip (user-confirmed):** the `CHECK_TABLE` + metadata gains a nullable `name` column (**rebuild-only** arrival — a + pre-4g project gains it on `rebuild`; a named CHECK add on an + un-upgraded project is refused with a friendly "rebuild first" + message), and `project.yaml`'s `check_constraints` is **extended** to + carry the name (`{expr, name}` mapping; the bare-string form still + reads, name = `None`) so a named CHECK survives a rebuild — `rebuild` + reconstructs from the yaml. The internal-`__rdbms_*` guard was folded + into `do_add_constraint` / `do_add_relationship`, completing the + 4d/4e/4f guard class. One undo step per statement. - **4h — `ALTER TABLE … RENAME TO`** (the §6 new low-level op). - **4i — Verification sweep.** Typing-surface + matrix coverage, engine-neutral error pass, undo-parity check (one step per diff --git a/docs/adr/README.md b/docs/adr/README.md index 71a0348..a89b8a0 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 + 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 +- [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` + 4g `ALTER TABLE` add/drop constraint + add FK, implemented 2026-05-25; 4h–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) and **4g** (`ALTER TABLE … ADD [CONSTRAINT ] (CHECK | UNIQUE | FOREIGN KEY)` + `DROP CONSTRAINT `; ADD = CHECK + composite UNIQUE + FK, with `ADD PRIMARY KEY` and a *named* UNIQUE refused — composite UNIQUE is anonymous in our model; each ADD reuses a low-level path (table-CHECK/UNIQUE rebuild with a dry-run guard; FK → `add_relationship`, bare `REFERENCES

` → parent single-PK), DROP CONSTRAINT resolves the name to a table-CHECK then a child-side FK; **named table-CHECKs round-trip** via a nullable `name` column on `__rdbms_playground_table_checks` (**rebuild-only** arrival — pre-4g projects gain it on `rebuild`, a named add on an un-upgraded project is refused with a friendly "rebuild first" message) *and* a `project.yaml` `check_constraints` extension to an `{expr, name}` mapping (the bare-string form still reads); the internal-`__rdbms_*` guard was folded into `do_add_constraint`/`do_add_relationship`, completing that guard class — all 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/plans/20260525-adr-0035-sql-ddl-4g.md b/docs/plans/20260525-adr-0035-sql-ddl-4g.md new file mode 100644 index 0000000..df94881 --- /dev/null +++ b/docs/plans/20260525-adr-0035-sql-ddl-4g.md @@ -0,0 +1,287 @@ +# Plan: ADR-0035 Phase 4, sub-phase 4g — `ALTER TABLE` add/drop constraint + add FK + +Add the advanced-mode SQL forms: + +- `ALTER TABLE ADD [CONSTRAINT ] CHECK ()` — table-level + CHECK, **named or unnamed**. +- `ALTER TABLE ADD UNIQUE (, …)` — composite UNIQUE (unnamed; + see §2.4). +- `ALTER TABLE ADD [CONSTRAINT ] FOREIGN KEY () + REFERENCES

[()] [ON DELETE …] [ON UPDATE …]` — a relationship. +- `ALTER TABLE DROP CONSTRAINT ` — drop a **named CHECK** or a + **named FK** (relationship). + +Plus: fold the internal-`__rdbms_*` guard into `do_add_constraint` / +`do_add_relationship` (the remaining executors of the 4d/4e/4f guard +class). + +**User-confirmed scope (2026-05-25):** full 4g in one slice; ADD = +CHECK + FK + composite UNIQUE (PRIMARY KEY refused); DROP CONSTRAINT = +named CHECK + named FK; named-CHECK round-trip via a **project.yaml +format extension** (the rebuild-only db-column arrival — pre-4g projects +gain the column on `rebuild`). + +## 1. Baseline (at handoff) + +- After 4f: **1865 passing, 0 failed, 0 skipped, 1 ignored**; clippy + clean. Branch `main`, HEAD `5b76315` (4f). + +## 2. Decisions (settled — user-confirmed 2026-05-25) + +1. **ADD scope = CHECK + FOREIGN KEY + composite UNIQUE.** `ADD PRIMARY + KEY` is **refused** with a clear message (every playground table + already has a PK; adding one is near-always invalid). PRIMARY KEY is + not in the grammar's ADD-constraint surface. +2. **CHECK migration = rebuild-only.** The `CHECK_TABLE` + (`__rdbms_playground_table_checks`) CREATE schema gains a nullable + `name` column; fresh + rebuilt databases get it. A pre-4g project on + disk keeps the 3-column table until `rebuild`. Unnamed table-CHECK + creation continues to work on an old DB (its INSERT never names the + column); a **named** CHECK add on an old DB is refused with a + friendly "this project predates named constraints — run `rebuild` + first" message (a single PRAGMA-guarded column-presence check, **not** + an auto-migration). +3. **Named-CHECK round-trip = project.yaml format extension.** Because + `rebuild` reconstructs from `project.yaml` (`do_rebuild_from_text` + parses the yaml, wipes the db, re-emits DDL via `schema_to_ddl`), the + CHECK *name* must live in `project.yaml`, not just the db column. + `check_constraints: Vec` becomes `Vec` where + `TableCheck { name: Option, expr: String }`. The yaml reader + accepts **both** the old bare-string form (`- "expr"`, name = None) + and the new mapping form (`- {expr: "…", name: "…"}`), per the + established "optional on read" backward-compat convention. +4. **DROP CONSTRAINT scope = named CHECK + named FK.** Resolution order: + look up `` in `CHECK_TABLE` (named table-CHECK) → drop it + (rebuild without it + delete the row); else in `REL_TABLE` (named + relationship) → drop it via the existing drop-relationship machinery; + else refuse "no such constraint `` on ``". An unnamed / + column-level / UNIQUE constraint is **not** a DROP CONSTRAINT target. +5. **Composite UNIQUE is unnamed (asymmetry, intentional).** A composite + UNIQUE constraint carries no user-facing name in our model + (PRAGMA-detected via the `origin='u'` auto-index — ADR-0035 §4a.2), so + `ADD UNIQUE (cols)` creates an anonymous constraint and **a + `CONSTRAINT ` prefix on UNIQUE is refused** ("naming a UNIQUE + constraint is not supported — use `alter table add unique + (cols)`"). It therefore cannot be a `DROP CONSTRAINT ` target. + This is consistent: ADD-UNIQUE is in scope, DROP-UNIQUE was never in + scope (no name to target). +6. **FK reuses the existing relationship machinery.** `ADD [CONSTRAINT + ] FOREIGN KEY () REFERENCES

[()] …` decomposes to + `add_relationship` (the same executor `add 1:n relationship` uses): + parent-PK validation, bare-`REFERENCES

` → parent single-PK + resolution, `fk_target_type` compatibility, auto-naming when unnamed, + name-uniqueness, one undo step. No new FK executor. +7. **CREATE TABLE table-CHECKs stay unnamed (out of 4g).** Naming a CHECK + declared *inside* `CREATE TABLE` is a separate consistency item; 4g + only introduces names via `ALTER … ADD CONSTRAINT CHECK`. + `do_create_table` writes `name = NULL` for its table-CHECKs. (Verify + the CREATE grammar does not silently swallow a `CONSTRAINT ` on + a table CHECK; if it parses one, it is currently dropped — leave as a + noted follow-up, do not expand CREATE here.) +8. **Internal-`__rdbms_*` guard** folded into `do_add_constraint` and + `do_add_relationship` (both the `table`/`parent_table` and the + `child_table` for relationships). Closes the 4d/4e/4f guard class. + +## 3. Phase 1 — Requirements checklist (4g) + +### Round-trip backbone (named table-CHECK) +- [ ] `CHECK_TABLE` CREATE schema gains nullable `name TEXT` (rebuild-only + arrival). `read_table_checks` reads `(name, check_expr)` ordered by + `seq`. `ReadSchema.check_constraints: Vec`. +- [ ] `schema_to_ddl` emits `CONSTRAINT CHECK ()` when named, + bare `CHECK ()` when not. +- [ ] `persistence::TableSchema.check_constraints: Vec`; the + db→persistence capture (db.rs:~2549) carries names. +- [ ] yaml **writes** the mapping form for named, bare string for + unnamed; yaml **reads** both old (bare string) and new (mapping) forms. +- [ ] `do_create_table` writes `name = NULL` for its table-CHECKs. +- [ ] Round-trip test: a named table-CHECK survives save→load and + `rebuild`; an old-format yaml (bare strings) still loads. + +### Grammar / dispatch +- [ ] `AlterTableAction` gains `AddTableConstraint { name: Option, + constraint: TableConstraint }` and `DropConstraint { name: String }`. +- [ ] New `TableConstraint` enum: `Check { expr_sql: String }` (raw text + — `sql_expr` is validate-only), `Unique { columns: Vec }`, + `ForeignKey(Box)` (reuse 4b struct). +- [ ] Grammar: `AT_ADD_TABLE_CONSTRAINT` (`add [constraint ] + (check (…) | unique (…) | foreign key (…) references …)`) and + `AT_DROP_CONSTRAINT` (`drop constraint `), added to + `AT_ACTION_CHOICES`. Reuse `sql_create_table`'s table-element nodes for + CHECK / UNIQUE / FK where possible. +- [ ] Builder discrimination order in `build_sql_alter_table`: + `type` → (`column` ⇒ add/rename/drop **column**) → `add` ⇒ + add-table-constraint → `drop` ⇒ drop-constraint. (Checking `column` + before the bare `add`/`drop` keeps `add column … unique`/`… check` + routing to AddColumn.) +- [ ] Sub-discriminate the table-constraint by `check` / `unique` / + `foreign`. A `CONSTRAINT ` on UNIQUE refuses (§2.5). +- [ ] Trailing `;` tolerated; four existing AlterTableAction branches + still route; `alter` stays advanced-only; table slot rejects + `__rdbms_*` at parse. + +### Execution +- [ ] **ADD CHECK** (named/unnamed): dry-run guard (existing rows satisfy + the CHECK — reuse `dry_run_check`), rebuild with the table-CHECK in the + DDL, write the `CHECK_TABLE` row (`table_name, seq=next, check_expr, + name`); auto-show; one undo step. Named add on an old DB (no `name` + column) → friendly rebuild-needed refusal. +- [ ] **ADD UNIQUE (cols)**: dry-run guard (no duplicate tuples — reuse + `dry_run_unique`/the composite equivalent), rebuild adding the + composite UNIQUE; one undo step. Survives rebuild (existing + `unique_constraints` yaml path). +- [ ] **ADD FOREIGN KEY**: decompose to `add_relationship` (name, + parent, child, actions, `create_fk = true`?). Reuse 4b resolution + (bare `REFERENCES

`, self-ref, type compat). One undo step. +- [ ] **DROP CONSTRAINT **: resolve name in `CHECK_TABLE` then + `REL_TABLE`; drop accordingly; refuse unknown. One undo step. +- [ ] Internal-`__rdbms_*` guard in `do_add_constraint` / + `do_add_relationship` (both surfaces) + a test. + +### Testing +- [ ] **Tier 1** (`sql_alter_table_tests` in ddl.rs): parse each new form + → the right `AlterTableAction`; the six-branch dispatch still routes + the four column actions; named-UNIQUE refusal; `ADD PRIMARY KEY` + refusal. +- [ ] **Tier 2/round-trip** (persistence/yaml unit tests): named CHECK + serialize + parse; old bare-string parse; `Vec` save/load. +- [ ] **Tier 3** (`tests/sql_alter_table.rs` via `run_replay`): ADD + named CHECK enforced + survives rebuild with its name; ADD UNIQUE + enforced + survives rebuild; ADD FOREIGN KEY creates the relationship; + DROP CONSTRAINT removes a named CHECK and a named FK; DROP unknown + refused; one undo step each. +- [ ] **Internal guard** (`tests/column_op_guards.rs`): simple + `add_constraint` / `add_relationship` on `__rdbms_*` refused. +- [ ] **Catalog** lockstep + vocab audit for the refreshed + `sql_alter_table` help/usage (now listing add/drop constraint + add FK). + +## 4. Architecture & change list (file by file) + +- **`src/persistence/mod.rs`**: add `pub struct TableCheck { pub name: + Option, pub expr: String }`; `TableSchema.check_constraints: + Vec`. Update the `csv_io.rs` / `mod.rs` constructors that + set `check_constraints: Vec::new()` (type still compiles — empty Vec). +- **`src/persistence/yaml.rs`**: serialize a `TableCheck` as a bare + string when `name` is None, else a `{expr, name}` mapping; parse both + forms (back-compat). Update the parser struct + the round-trip tests. +- **`src/db.rs`**: + - `CHECK_TABLE` CREATE schema += `name TEXT` (nullable). + - `read_table_checks` → `Vec` (read name; tolerate a + missing `name` column on an old DB via a column-presence check → + name = None). + - `ReadSchema.check_constraints: Vec`; `schema_to_ddl` + emits `CONSTRAINT ` when named. + - db→persistence capture (≈2549) maps `Vec`. + - `build_read_schema` (yaml variant, ≈8125) maps persistence + `TableCheck` → `ReadSchema` `TableCheck`. + - `do_create_table` table-CHECK INSERT writes `name = NULL`. + - New executors: `do_alter_add_table_check`, + `do_alter_add_unique`, `do_drop_constraint_by_name`. Worker methods + + `Request` variants + handler dispatch (wrapped in `snapshot_then`). + - `reject_internal_table_name` at the top of `do_add_constraint` / + `do_add_relationship` (+ child_table for the latter). + - A `check_table_has_name_column(conn)` helper for the + rebuild-needed refusal. +- **`src/dsl/command.rs`**: `AlterTableAction::{AddTableConstraint, + DropConstraint}`; `TableConstraint` enum. +- **`src/dsl/grammar/ddl.rs`**: `AT_ADD_TABLE_CONSTRAINT`, + `AT_DROP_CONSTRAINT`, builder branches + sub-discrimination, the + named-UNIQUE / ADD-PRIMARY-KEY refusals. +- **`src/dsl/grammar/sql_create_table.rs`**: expose the table-CHECK / + UNIQUE / FK element nodes for reuse if not already `pub(crate)`. +- **`src/runtime.rs`**: `SqlAlterTable` arm → the new executors; + `AddTableConstraint::ForeignKey` → `add_relationship`. +- **`src/app.rs`**: `build_translate_context` arms for the two new + actions (Operation::AddConstraint / DropConstraint / AddRelationship). +- **`src/friendly/{keys.rs,strings/en-US.yaml}`**: refresh + `sql_alter_table` help/usage; any new refusal message keys. + +## 5. Phase 2 — Candidate approaches (key forks) + +**Round-trip representation.** (R1) `Vec` threaded +through ReadSchema + persistence *(lead — single source of truth, clean +rebuild)*. (R2) parallel `Vec>` names alongside the +existing `Vec` — *rejected* (two vectors to keep aligned, error +prone). (R3) store names only in the db, not yaml — *rejected* (names +lost on rebuild; breaks DROP CONSTRAINT after rebuild). + +**Executor structure.** (E1) one `do_alter_add_table_check` + one +`do_alter_add_unique` + FK via `add_relationship` + one +`do_drop_constraint_by_name` *(lead — each maps to one rebuild, mirrors +the 4e/4f decomposition)*. (E2) a single mega-executor switching on a +constraint enum — *rejected* (a fat function; the three adds have +genuinely different dry-run guards + metadata writes). + +**Grammar.** (G1) separate Choice branches `AT_ADD_TABLE_CONSTRAINT` / +`AT_DROP_CONSTRAINT` added to `AT_ACTION_CHOICES`, builder discriminates +by `column` then `add`/`drop` then the constraint keyword *(lead — +consistent with the existing five branches; reuses the create-table +element nodes)*. (G2) a nested sub-Choice under a single `add` branch — +*rejected* (complicates the builder more than separate branches). + +## 6. Phase 3 — Selection + +R1 + E1 + G1. Satisfies every §3 item with the smallest faithful change: +the round-trip backbone is a typed extension (not a parallel array), the +executors each reduce to one rebuild + one metadata write (one undo +step), and the grammar mirrors the established branch structure. The +named-UNIQUE refusal and ADD-PRIMARY-KEY refusal keep the surface honest +about what the model can persist. + +## 7. Devil's Advocate review of this plan + +- **Forks escalated?** ADD scope, the migration approach, and DROP scope + were put to the user (2026-05-25) and answered (CHECK+FK+UNIQUE / + rebuild-only / CHECK+FK). The newly-discovered yaml-format-change + implication was surfaced and the user chose "Full 4g now". The + named-UNIQUE-refusal and CREATE-CHECK-stays-unnamed micro-decisions are + consequences of the model (anonymous composite UNIQUE; ALTER-only + naming) — noted here, to be confirmed in the combined `/runda`. ✓ +- **Back-compat of the yaml change?** The reader accepts both the bare + string and the mapping form; a test covers an old-format file. The + field stays "optional on read". ✓ +- **Old-DB named-CHECK add?** Guarded by a column-presence check → a + friendly engine-neutral rebuild-needed refusal, not a raw engine error + (ADR-0035 §9). Unnamed CHECK adds keep working on an old DB. ✓ +- **One undo step each?** Each add/drop is one executor call = one + rebuild = one snapshot, like 4e/4f. e2e undo checks. ✓ +- **Grammar trap?** Six concrete-keyword-led branches; the builder keys + on `column` (column ops) then `add`/`drop` then the constraint keyword. + `add column … unique/check` still routes to AddColumn (checked via + `column` first). A parse test for every branch + the discrimination + edges. ✓ +- **Engine neutrality?** New refusal messages say "the database" / + "constraint" in the abstract; vocab audit + catalog lockstep tests + guard it. ✓ +- **Anything dropped?** ADD PRIMARY KEY (refused, stated), named UNIQUE + (refused, stated), CREATE-TABLE CHECK naming (out of scope, noted). No + silent drops. + +## 8. Implementation sequence (test-first) +1. **Internal guards** — `reject_internal_table_name` in + `do_add_constraint` / `do_add_relationship`; `column_op_guards.rs` + tests (red → green). Isolated, lands first. +2. **Round-trip backbone** — `TableCheck` type; `CHECK_TABLE` +`name`; + `read_table_checks` / `ReadSchema` / `schema_to_ddl` / capture / + `do_create_table` / yaml serialize+parse. Persistence/yaml round-trip + tests (incl. old-format read). No behaviour change yet (all CHECKs + still unnamed until the grammar lands) → full suite stays green. +3. **Command + grammar + builder** — the two actions + `TableConstraint`; + `AT_ADD_TABLE_CONSTRAINT` / `AT_DROP_CONSTRAINT`; the discrimination + + refusals; Tier-1 parse tests → exhaustive arms (compiler) → green + (parse only). +4. **Executors + runtime + catalog** — `do_alter_add_table_check`, + `do_alter_add_unique`, `do_drop_constraint_by_name`, FK via + `add_relationship`; wire `SqlAlterTable`; refresh help/usage; Tier-3 + e2e (ADD CHECK/UNIQUE/FK, DROP CHECK/FK, refusals, rebuild survival, + undo) → green. +5. **Full sweep** — `cargo test` (no regression from 1865) + `cargo + clippy --all-targets -- -D warnings`. +6. **Docs** — ADR-0035 Status + §13 4g; README; requirements Q1. Defer + the formal `/runda` to the combined pass (user steer). Propose commit; + wait for approval. + +## 9. Exit gate +- All §3 items satisfied; all tiers green, zero skips; no regression from + 1865; written-DA PASS (combined `/runda` to follow); clippy clean; + ADR-0035 §13 4g + README + requirements.md lockstep. diff --git a/docs/requirements.md b/docs/requirements.md index 55cbaf4..0292400 100644 --- a/docs/requirements.md +++ b/docs/requirements.md @@ -238,9 +238,17 @@ handoff-14 cleanup; 449 after B2/C2.) 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.)* + the internal-`__rdbms_*` guard folded into `do_change_column_type`), + then `ALTER TABLE` add/drop constraint + add FK (4g — `ADD [CONSTRAINT + ] (CHECK | UNIQUE | FOREIGN KEY)` + `DROP CONSTRAINT `; + ADD = CHECK + composite UNIQUE + FK (PRIMARY KEY + named UNIQUE + refused); table-CHECK/UNIQUE rebuild with a dry-run guard, FK reuses + `add_relationship`; named table-CHECKs round-trip via a rebuild-only + `name` column on `__rdbms_playground_table_checks` + a `project.yaml` + `check_constraints` `{expr, name}` extension; the internal-table guard + completed across `do_add_constraint`/`do_add_relationship`)). + Remaining DDL — `ALTER TABLE … RENAME TO` (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 934609e..b5e2f21 100644 --- a/src/app.rs +++ b/src/app.rs @@ -1602,6 +1602,12 @@ impl App { Some(table.as_str()), Some(column.as_str()), ), + AlterTableAction::AddTableConstraint { .. } => { + (Operation::AddConstraint, Some(table.as_str()), None) + } + AlterTableAction::DropConstraint { .. } => { + (Operation::DropConstraint, Some(table.as_str()), None) + } }, C::SqlCreateTable { name, .. } => { (Operation::CreateTable, Some(name.as_str()), None) diff --git a/src/db.rs b/src/db.rs index a8f03c7..e9cc700 100644 --- a/src/db.rs +++ b/src/db.rs @@ -43,7 +43,7 @@ use crate::output_render::{Alignment, render_diagnostic_table}; use crate::type_change; use crate::persistence::{ CellValue, ColumnSchema, IndexSchema, Persistence, PersistenceError, RelationshipSchema, - SchemaSnapshot, TableSchema, TableSnapshot, decode_cell, parse_csv, parse_schema, + SchemaSnapshot, TableCheck, TableSchema, TableSnapshot, decode_cell, parse_csv, parse_schema, }; use crate::project::{DATA_DIR, PROJECT_YAML}; use crate::undo::{DEFAULT_RING_CAPACITY, SnapshotError, SnapshotMeta, SnapshotStore, Staged}; @@ -594,6 +594,40 @@ enum Request { source: Option, reply: oneshot::Sender>, }, + /// `ALTER TABLE … ADD [CONSTRAINT ] CHECK ()` — a + /// table-level CHECK, named or unnamed (ADR-0035 §4g). + AlterAddTableCheck { + table: String, + name: Option, + expr_sql: String, + source: Option, + reply: oneshot::Sender>, + }, + /// `ALTER TABLE … ADD UNIQUE (, …)` — a composite UNIQUE + /// constraint (ADR-0035 §4g). + AlterAddUnique { + table: String, + columns: Vec, + source: Option, + reply: oneshot::Sender>, + }, + /// `ALTER TABLE … DROP CONSTRAINT ` — drop a named table-level + /// CHECK or a named FK (ADR-0035 §4g). + AlterDropConstraint { + table: String, + name: String, + source: Option, + reply: oneshot::Sender, DbError>>, + }, + /// `ALTER TABLE ADD [CONSTRAINT ] FOREIGN KEY (…) + /// REFERENCES …` — a relationship on an existing table (ADR-0035 §4g). + AlterAddForeignKey { + child_table: String, + name: Option, + fk: Box, + source: Option, + reply: oneshot::Sender>, + }, Insert { table: String, columns: Option>, @@ -1075,6 +1109,87 @@ impl Database { recv.await.map_err(|_| DbError::WorkerGone)? } + /// `ALTER TABLE … ADD [CONSTRAINT ] CHECK ()` — a + /// table-level CHECK (ADR-0035 §4g). + pub async fn alter_add_table_check( + &self, + table: String, + name: Option, + expr_sql: String, + source: Option, + ) -> Result { + let (reply, recv) = oneshot::channel(); + self.send(Request::AlterAddTableCheck { + table, + name, + expr_sql, + source, + reply, + }) + .await?; + recv.await.map_err(|_| DbError::WorkerGone)? + } + + /// `ALTER TABLE … ADD UNIQUE (, …)` — a composite UNIQUE + /// constraint (ADR-0035 §4g). + pub async fn alter_add_unique( + &self, + table: String, + columns: Vec, + source: Option, + ) -> Result { + let (reply, recv) = oneshot::channel(); + self.send(Request::AlterAddUnique { + table, + columns, + source, + reply, + }) + .await?; + recv.await.map_err(|_| DbError::WorkerGone)? + } + + /// `ALTER TABLE … DROP CONSTRAINT ` — drop a named table-level + /// CHECK or a named FK (ADR-0035 §4g). + pub async fn alter_drop_constraint( + &self, + table: String, + name: String, + source: Option, + ) -> Result, DbError> { + let (reply, recv) = oneshot::channel(); + self.send(Request::AlterDropConstraint { + table, + name, + source, + reply, + }) + .await?; + recv.await.map_err(|_| DbError::WorkerGone)? + } + + /// `ALTER TABLE ADD [CONSTRAINT ] FOREIGN KEY (…) + /// REFERENCES …` — add a relationship to an existing table (ADR-0035 + /// §4g). + pub async fn alter_add_foreign_key( + &self, + child_table: String, + name: Option, + fk: SqlForeignKey, + source: Option, + ) -> Result { + let (reply, recv) = oneshot::channel(); + self.send(Request::AlterAddForeignKey { + child_table, + name, + fk: Box::new(fk), + source, + reply, + }) + .await?; + recv.await.map_err(|_| DbError::WorkerGone)? + } + pub async fn rename_column( &self, table: String, @@ -1547,6 +1662,7 @@ fn configure_connection(conn: &Connection) -> Result<(), rusqlite::Error> { table_name TEXT NOT NULL,\n\ seq INTEGER NOT NULL,\n\ check_expr TEXT NOT NULL,\n\ + name TEXT,\n\ PRIMARY KEY (table_name, seq)\n\ ) STRICT;\n\ CREATE TABLE IF NOT EXISTS {META_PROJECT_TABLE} (\n\ @@ -2139,6 +2255,62 @@ fn handle_request( kind, )); } + Request::AlterAddTableCheck { + table, + name, + expr_sql, + source, + reply, + } => { + snapshot_then(snap, batch, conn, source.as_deref(), reply, || { + do_alter_add_table_check( + conn, + persistence, + source.as_deref(), + &table, + name.as_deref(), + &expr_sql, + ) + }); + } + Request::AlterAddUnique { + table, + columns, + source, + reply, + } => { + snapshot_then(snap, batch, conn, source.as_deref(), reply, || { + do_alter_add_unique(conn, persistence, source.as_deref(), &table, &columns) + }); + } + Request::AlterDropConstraint { + table, + name, + source, + reply, + } => { + snapshot_then(snap, batch, conn, source.as_deref(), reply, || { + do_drop_constraint_by_name(conn, persistence, source.as_deref(), &table, &name) + }); + } + Request::AlterAddForeignKey { + child_table, + name, + fk, + source, + reply, + } => { + snapshot_then(snap, batch, conn, source.as_deref(), reply, || { + do_alter_add_foreign_key( + conn, + persistence, + source.as_deref(), + &child_table, + name.as_deref(), + &fk, + ) + }); + } Request::Insert { table, columns, @@ -3468,6 +3640,11 @@ fn do_add_constraint( column: &str, constraint: &Constraint, ) -> Result { + // Refuse the internal `__rdbms_*` tables up-front (as "no such + // table"), like the sibling schema-mutation executors. Closes the + // simple `add constraint` exposure and the SQL `ALTER TABLE … ADD + // CONSTRAINT` decomposition target (ADR-0035 §4g). + reject_internal_table_name(table)?; let old_schema = read_schema(conn, table)?; let (col_is_pk, col_user_type) = { let col = old_schema @@ -5107,11 +5284,13 @@ struct ReadSchema { /// read from the UNIQUE-constraint indexes (`origin = 'u'`). /// Single-column UNIQUE rides on `ReadColumn::unique` instead. unique_constraints: Vec>, - /// Table-level CHECK constraints as raw SQL text, in declaration - /// order (ADR-0035 §4a.3). The engine reports no CHECK constraints, - /// so these are read from `__rdbms_playground_table_checks` rather - /// than PRAGMA, and echoed verbatim by `schema_to_ddl` on rebuild. - check_constraints: Vec, + /// Table-level CHECK constraints as raw SQL text with an optional + /// name, in declaration order (ADR-0035 §4a.3, named in §4g). The + /// engine reports no CHECK constraints, so these are read from + /// `__rdbms_playground_table_checks` rather than PRAGMA, and echoed + /// verbatim by `schema_to_ddl` on rebuild (`CONSTRAINT ` when + /// named). + check_constraints: Vec, } #[derive(Debug, Clone)] @@ -5245,19 +5424,32 @@ fn read_schema(conn: &Connection, table: &str) -> Result { }) } -/// Read a table's table-level CHECK constraints (ADR-0035 §4a.3) from -/// `CHECK_TABLE`, in declaration order (`seq`). The engine exposes no -/// PRAGMA for CHECK constraints, so this metadata table is their only -/// source of truth. -fn read_table_checks(conn: &Connection, table: &str) -> Result, DbError> { - let mut stmt = conn - .prepare(&format!( +/// Read a table's table-level CHECK constraints (ADR-0035 §4a.3, named +/// in §4g) from `CHECK_TABLE`, in declaration order (`seq`). The engine +/// exposes no PRAGMA for CHECK constraints, so this metadata table is +/// their only source of truth. Tolerates a pre-4g project whose table +/// predates the `name` column (rebuild-only migration) by reading the +/// name as `None`. +fn read_table_checks(conn: &Connection, table: &str) -> Result, DbError> { + let has_name = check_table_has_name_column(conn)?; + let sql = if has_name { + format!( + "SELECT check_expr, name FROM {CHECK_TABLE} \ + WHERE table_name = ?1 ORDER BY seq;" + ) + } else { + format!( "SELECT check_expr FROM {CHECK_TABLE} \ WHERE table_name = ?1 ORDER BY seq;" - )) - .map_err(DbError::from_rusqlite)?; + ) + }; + let mut stmt = conn.prepare(&sql).map_err(DbError::from_rusqlite)?; let rows = stmt - .query_map([table], |row| row.get::<_, String>(0)) + .query_map([table], |row| { + let expr: String = row.get(0)?; + let name: Option = if has_name { row.get(1)? } else { None }; + Ok(TableCheck { name, expr }) + }) .map_err(DbError::from_rusqlite)?; let mut out = Vec::new(); for row in rows { @@ -5266,6 +5458,23 @@ fn read_table_checks(conn: &Connection, table: &str) -> Result, DbEr Ok(out) } +/// Whether `CHECK_TABLE` carries the `name` column (ADR-0035 §4g). A +/// pre-4g project's metadata table predates it — the column arrives on +/// `rebuild` (the rebuild-only migration, user-confirmed 2026-05-25). +/// Used to read names tolerantly and to refuse a *named* CHECK add on an +/// un-upgraded project with a friendly "rebuild first" message rather +/// than a raw engine error. +fn check_table_has_name_column(conn: &Connection) -> Result { + let count: i64 = conn + .query_row( + &format!("SELECT COUNT(*) FROM pragma_table_info('{CHECK_TABLE}') WHERE name = 'name';"), + [], + |row| row.get(0), + ) + .map_err(DbError::from_rusqlite)?; + Ok(count > 0) +} + /// Whether the raw CHECK expression `check_expr` references the column /// `column` (ADR-0035 §4e — the 4a.3-deferred drop/rename guard). /// @@ -5349,8 +5558,8 @@ fn column_referenced_by_check( column: &str, include_self: bool, ) -> Result { - for expr in read_table_checks(conn, table)? { - if check_references_column(&expr, column) { + for check in read_table_checks(conn, table)? { + if check_references_column(&check.expr, column) { return Ok(true); } } @@ -5561,9 +5770,18 @@ fn schema_to_ddl(table: &str, schema: &ReadSchema) -> String { // Table-level CHECK constraints (ADR-0035 §4a.3) — echoed verbatim // from the raw SQL stored in the metadata table, emitted identically - // to `do_create_table` (the §6.1 two-generators rule). - for expr in &schema.check_constraints { - clauses.push(format!("CHECK ({expr})")); + // to `do_create_table` (the §6.1 two-generators rule). A named CHECK + // (ADR-0035 §4g) re-emits its `CONSTRAINT ` prefix so the name + // round-trips through a rebuild. + for check in &schema.check_constraints { + match &check.name { + Some(name) => clauses.push(format!( + "CONSTRAINT {ident} CHECK ({expr})", + ident = quote_ident(name), + expr = check.expr, + )), + None => clauses.push(format!("CHECK ({expr})", expr = check.expr)), + } } for fk in &schema.foreign_keys { @@ -5981,6 +6199,12 @@ fn do_add_relationship( on_update: ReferentialAction, create_fk: bool, ) -> Result { + // Refuse the internal `__rdbms_*` tables on either endpoint (as "no + // such table"), like the sibling schema-mutation executors. Closes + // the simple `add 1:n relationship` exposure and the SQL `ALTER + // TABLE … ADD FOREIGN KEY` decomposition target (ADR-0035 §4g). + reject_internal_table_name(parent_table)?; + reject_internal_table_name(child_table)?; // 1. Read parent schema; verify the referenced column is a PK. let parent_schema = read_schema(conn, parent_table)?; let parent_col = parent_schema @@ -6180,6 +6404,305 @@ fn do_drop_relationship( Ok(Some(do_describe_table(conn, &parent_table)?)) } +/// `ALTER TABLE ADD [CONSTRAINT ] CHECK ()` (ADR-0035 +/// §4g). A dry-run refuses the add if any existing row fails the +/// predicate; the rebuild then re-emits the table with the new CHECK in +/// its DDL and records it in `CHECK_TABLE`. A *named* CHECK on a pre-4g +/// project (whose metadata table predates the `name` column — the +/// rebuild-only migration) is refused with a friendly "rebuild first" +/// message rather than a raw engine error. +fn do_alter_add_table_check( + conn: &Connection, + persistence: Option<&Persistence>, + source: Option<&str>, + table: &str, + name: Option<&str>, + expr_sql: &str, +) -> Result { + reject_internal_table_name(table)?; + let old_schema = read_schema(conn, table)?; + + if name.is_some() && !check_table_has_name_column(conn)? { + return Err(DbError::Unsupported( + "this project predates named constraints; run `rebuild` to \ + upgrade it, then add the named constraint again." + .to_string(), + )); + } + + // A named CHECK must not collide with an existing CHECK name on this + // table NOR with a relationship name (FKs are also `DROP CONSTRAINT` + // targets) — keeps `drop constraint ` unambiguous. + if let Some(n) = name { + let collides_check = old_schema + .check_constraints + .iter() + .any(|c| c.name.as_deref() == Some(n)); + let collides_rel: i64 = conn + .query_row( + &format!("SELECT COUNT(*) FROM {REL_TABLE} WHERE name = ?1;"), + [n], + |row| row.get(0), + ) + .map_err(DbError::from_rusqlite)?; + if collides_check || collides_rel > 0 { + return Err(DbError::Unsupported(format!( + "a constraint named `{n}` already exists on `{table}`." + ))); + } + } + + // Dry-run: a CHECK passes on TRUE or NULL; only FALSE fails, so + // `WHERE NOT (expr)` counts the genuine violations. + let violating: i64 = conn + .query_row( + &format!( + "SELECT COUNT(*) FROM {tbl} WHERE NOT ({expr_sql});", + tbl = quote_ident(table), + ), + [], + |row| row.get(0), + ) + .map_err(DbError::from_rusqlite)?; + if violating > 0 { + return Err(DbError::Unsupported(format!( + "cannot add CHECK ({expr_sql}) to `{table}`: {violating} existing \ + row(s) do not satisfy it." + ))); + } + + let mut new_schema = old_schema.clone(); + new_schema.check_constraints.push(TableCheck { + name: name.map(ToString::to_string), + expr: expr_sql.to_string(), + }); + + let table_owned = table.to_string(); + let name_owned = name.map(ToString::to_string); + let expr_owned = expr_sql.to_string(); + rebuild_table(conn, table, &old_schema, &new_schema, |tx| { + // MAX(seq)+1 avoids colliding with a gap a prior DROP left. + let next_seq: i64 = tx + .query_row( + &format!( + "SELECT COALESCE(MAX(seq), -1) + 1 FROM {CHECK_TABLE} \ + WHERE table_name = ?1;" + ), + [table_owned.as_str()], + |row| row.get(0), + ) + .map_err(DbError::from_rusqlite)?; + tx.execute( + &format!( + "INSERT INTO {CHECK_TABLE} (table_name, seq, check_expr, name) \ + VALUES (?1, ?2, ?3, ?4);" + ), + rusqlite::params![table_owned, next_seq, expr_owned, name_owned], + ) + .map_err(DbError::from_rusqlite)?; + let changes = Changes { + schema_dirty: true, + rewritten_tables: vec![table_owned.clone()], + ..Changes::default() + }; + finalize_persistence(tx, persistence, source, &changes)?; + Ok(()) + })?; + do_describe_table(conn, table) +} + +/// `ALTER TABLE ADD UNIQUE (, …)` (ADR-0035 §4g) — a composite +/// UNIQUE constraint (anonymous: composite UNIQUE is PRAGMA-detected on +/// read, ADR-0035 §4a.2, so it carries no name). A dry-run refuses the +/// add if existing rows already contain a duplicate non-NULL tuple +/// (NULLs are distinct under SQL's UNIQUE semantics). +fn do_alter_add_unique( + conn: &Connection, + persistence: Option<&Persistence>, + source: Option<&str>, + table: &str, + columns: &[String], +) -> Result { + reject_internal_table_name(table)?; + let old_schema = read_schema(conn, table)?; + for c in columns { + if !old_schema.columns.iter().any(|oc| &oc.name == c) { + return Err(DbError::Sqlite { + message: format!("no such column: {table}.{c}"), + kind: SqliteErrorKind::NoSuchColumn, + }); + } + } + + let non_null = columns + .iter() + .map(|c| format!("{} IS NOT NULL", quote_ident(c))) + .collect::>() + .join(" AND "); + let group = columns + .iter() + .map(|c| quote_ident(c)) + .collect::>() + .join(", "); + let dup_groups: i64 = conn + .query_row( + &format!( + "SELECT COUNT(*) FROM (SELECT 1 FROM {tbl} WHERE {non_null} \ + GROUP BY {group} HAVING COUNT(*) > 1);", + tbl = quote_ident(table), + ), + [], + |row| row.get(0), + ) + .map_err(DbError::from_rusqlite)?; + if dup_groups > 0 { + return Err(DbError::Unsupported(format!( + "cannot add UNIQUE ({}) to `{table}`: existing rows contain \ + duplicate values.", + columns.join(", "), + ))); + } + + let mut new_schema = old_schema.clone(); + new_schema.unique_constraints.push(columns.to_vec()); + let table_owned = table.to_string(); + rebuild_table(conn, table, &old_schema, &new_schema, |tx| { + let changes = Changes { + schema_dirty: true, + rewritten_tables: vec![table_owned.clone()], + ..Changes::default() + }; + finalize_persistence(tx, persistence, source, &changes)?; + Ok(()) + })?; + do_describe_table(conn, table) +} + +/// `ALTER TABLE DROP CONSTRAINT ` (ADR-0035 §4g). Resolves +/// `name` to a named table-level CHECK on `T` (rebuild without it + +/// delete the metadata row), else to a named relationship (FK) whose +/// child is `T` (via `do_drop_relationship`), else refuses. +fn do_drop_constraint_by_name( + conn: &Connection, + persistence: Option<&Persistence>, + source: Option<&str>, + table: &str, + name: &str, +) -> Result, DbError> { + reject_internal_table_name(table)?; + + // 1. A named table-level CHECK on this table? + if check_table_has_name_column(conn)? { + let check_count: i64 = conn + .query_row( + &format!( + "SELECT COUNT(*) FROM {CHECK_TABLE} \ + WHERE table_name = ?1 AND name = ?2;" + ), + [table, name], + |row| row.get(0), + ) + .map_err(DbError::from_rusqlite)?; + if check_count > 0 { + let old_schema = read_schema(conn, table)?; + let mut new_schema = old_schema.clone(); + new_schema + .check_constraints + .retain(|c| c.name.as_deref() != Some(name)); + let (t, n) = (table.to_string(), name.to_string()); + rebuild_table(conn, table, &old_schema, &new_schema, |tx| { + tx.execute( + &format!( + "DELETE FROM {CHECK_TABLE} WHERE table_name = ?1 AND name = ?2;" + ), + [t.as_str(), n.as_str()], + ) + .map_err(DbError::from_rusqlite)?; + let changes = Changes { + schema_dirty: true, + rewritten_tables: vec![t.clone()], + ..Changes::default() + }; + finalize_persistence(tx, persistence, source, &changes)?; + Ok(()) + })?; + return Ok(Some(do_describe_table(conn, table)?)); + } + } + + // 2. A named relationship (FK) whose child is this table? + let rel_count: i64 = conn + .query_row( + &format!("SELECT COUNT(*) FROM {REL_TABLE} WHERE name = ?1 AND child_table = ?2;"), + [name, table], + |row| row.get(0), + ) + .map_err(DbError::from_rusqlite)?; + if rel_count > 0 { + return do_drop_relationship( + conn, + persistence, + source, + &RelationshipSelector::Named { + name: name.to_string(), + }, + ); + } + + // 3. Not a known named constraint on this table. + Err(DbError::Sqlite { + message: format!("no such constraint: {name} on {table}"), + kind: SqliteErrorKind::Other, + }) +} + +/// `ALTER TABLE ADD [CONSTRAINT ] FOREIGN KEY () +/// REFERENCES

[()] [ON …]` (ADR-0035 §4g). Resolves a bare +/// `REFERENCES

` to the parent's single-column PK, then delegates to +/// `do_add_relationship` (the same machinery `add 1:n relationship` +/// uses) with `create_fk = false` — the child column must already exist +/// (an `ALTER … ADD FOREIGN KEY` references an existing column). +fn do_alter_add_foreign_key( + conn: &Connection, + persistence: Option<&Persistence>, + source: Option<&str>, + child_table: &str, + name: Option<&str>, + fk: &SqlForeignKey, +) -> Result { + reject_internal_table_name(child_table)?; + reject_internal_table_name(&fk.parent_table)?; + let parent_column = match &fk.parent_column { + Some(c) => c.clone(), + None => { + let ps = read_schema(conn, &fk.parent_table)?; + if ps.primary_key.len() == 1 { + ps.primary_key[0].clone() + } else { + return Err(DbError::Unsupported(format!( + "`{parent}` has a composite primary key, so a bare reference \ + is ambiguous — name the referenced column, e.g. \ + `REFERENCES {parent}()`.", + parent = fk.parent_table, + ))); + } + } + }; + do_add_relationship( + conn, + persistence, + source, + name, + &fk.parent_table, + &parent_column, + child_table, + &fk.child_column, + fk.on_delete, + fk.on_update, + false, + ) +} + /// Create an index on `table` over `columns` (ADR-0025). /// /// Refuses a redundant index on an already-indexed column set diff --git a/src/dsl/command.rs b/src/dsl/command.rs index df3e93e..f461dfc 100644 --- a/src/dsl/command.rs +++ b/src/dsl/command.rs @@ -739,6 +739,35 @@ pub enum AlterTableAction { /// force flag; static-refused / incompatible still refuse). One undo /// step (the executor's rebuild). ADR-0035 §4f. AlterColumnType { column: String, ty: Type }, + /// `ADD [CONSTRAINT ] (CHECK (…) | UNIQUE (…) | FOREIGN KEY + /// (…) REFERENCES …)` — a table-level constraint (ADR-0035 §4g). The + /// `name` is the `CONSTRAINT ` prefix (the FK carries its own + /// `SqlForeignKey::name`, set from this prefix at build time). CHECK + /// and FOREIGN KEY may be named; UNIQUE may not (composite UNIQUE is + /// anonymous in our model — §4g). Boxed: the FK payload is sizeable + /// (`clippy::large_enum_variant`). + AddTableConstraint { + name: Option, + constraint: Box, + }, + /// `DROP CONSTRAINT ` — drops a named table-level CHECK or a + /// named FK (relationship), resolved by name (ADR-0035 §4g). + DropConstraint { name: String }, +} + +/// A table-level constraint added via `ALTER TABLE … ADD [CONSTRAINT +/// ] …` (ADR-0035 §4g). +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum TableConstraint { + /// `CHECK ()` — the expression as **raw SQL text** (the + /// `sql_expr` grammar is validate-only; the builder captures the + /// matched span — the 4a.2 / 4e mechanism). + Check { expr_sql: String }, + /// `UNIQUE (, …)` — a composite UNIQUE constraint. + Unique { columns: Vec }, + /// `FOREIGN KEY () REFERENCES

[()] [ON …]` — reuses the + /// 4b `SqlForeignKey` shape; decomposed to `add_relationship`. + ForeignKey(SqlForeignKey), } impl std::fmt::Display for IndexSelector { diff --git a/src/dsl/grammar/ddl.rs b/src/dsl/grammar/ddl.rs index 5f7f942..8ce76ff 100644 --- a/src/dsl/grammar/ddl.rs +++ b/src/dsl/grammar/ddl.rs @@ -14,7 +14,7 @@ use crate::dsl::action::ReferentialAction; use crate::dsl::command::{ AlterTableAction, ChangeColumnMode, ColumnSpec, Command, Constraint, ConstraintKind, Expr, - IndexSelector, RelationshipSelector, SqlForeignKey, + IndexSelector, RelationshipSelector, SqlForeignKey, TableConstraint, }; use crate::dsl::value::Value; use crate::dsl::grammar::{ @@ -1880,21 +1880,29 @@ const AT_ADD_CONSTRAINT_SUFFIX: Node = Node::Repeated { min: 0, }; -static AT_ADD_COLUMN_NODES: &[Node] = &[ - Node::Word(Word::keyword("add")), +// The walker's `Choice` selects a branch by its **leading** token and +// does not backtrack into a sibling once a branch's first keyword +// matched. So the action `Choice` keeps ONE branch per leading verb +// (`add`/`drop`/`rename`/`alter`); the `add` and `drop` verbs then +// fan out to an **inner** `Choice` whose branches each lead on a +// *distinct* second keyword (column / constraint / check / unique / +// foreign / primary), so no two same-led branches ever sit in one +// `Choice`. + +// `add column [constraints]` — the column-def tail (the +// leading `add` is consumed by `AT_ADD`). +static AT_ADD_COLUMN_TAIL_NODES: &[Node] = &[ Node::Word(Word::keyword("column")), super::sql_create_table::COL_NAME, super::sql_create_table::SQL_TYPE, AT_ADD_CONSTRAINT_SUFFIX, ]; -const AT_ADD_COLUMN: Node = Node::Seq(AT_ADD_COLUMN_NODES); +const AT_ADD_COLUMN_TAIL: Node = Node::Seq(AT_ADD_COLUMN_TAIL_NODES); -static AT_DROP_COLUMN_NODES: &[Node] = &[ - Node::Word(Word::keyword("drop")), - Node::Word(Word::keyword("column")), - COLUMN_NAME, -]; -const AT_DROP_COLUMN: Node = Node::Seq(AT_DROP_COLUMN_NODES); +// `drop column ` / `drop constraint ` tails (leading `drop` +// consumed by `AT_DROP`). +static AT_DROP_COLUMN_TAIL_NODES: &[Node] = &[Node::Word(Word::keyword("column")), COLUMN_NAME]; +const AT_DROP_COLUMN_TAIL: Node = Node::Seq(AT_DROP_COLUMN_TAIL_NODES); static AT_RENAME_COLUMN_NODES: &[Node] = &[ Node::Word(Word::keyword("rename")), @@ -1918,11 +1926,76 @@ static AT_ALTER_COLUMN_NODES: &[Node] = &[ ]; 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]; +// --- 4g: ADD [CONSTRAINT ] table-constraint / DROP CONSTRAINT --- +// +// `ADD [CONSTRAINT ] (check (…) | unique (…) | foreign key (…) +// references … | primary key (…))` and `DROP CONSTRAINT ` +// (ADR-0035 §4g). The constraint bodies reuse the `sql_create_table` +// table-element nodes; the §4g name comes from the `CONSTRAINT ` +// prefix (a dedicated `constraint`-led inner branch — never a leading +// `Optional`). UNIQUE/PRIMARY KEY may carry a name syntactically but the +// builder refuses it (composite UNIQUE is anonymous; PK is unsupported). +const CONSTRAINT_NAME: Node = Node::Ident { + source: IdentSource::NewName, + role: "constraint_name", + validator: None, + highlight_override: None, + writes_table: false, + writes_column: false, + writes_user_listed_column: false, + writes_table_alias: false, + writes_cte_name: false, + writes_projection_alias: false, +}; +// The constraint bodies — each leads on a distinct concrete keyword. +static AT_CONSTRAINT_BODY_CHOICES: &[Node] = &[ + super::sql_create_table::TABLE_CHECK, + super::sql_create_table::TABLE_UNIQUE, + super::sql_create_table::TABLE_FK, + // `primary key (…)` parses so the builder can refuse it with a + // specific message rather than a generic "unexpected `primary`". + super::sql_create_table::TABLE_PK, +]; +const AT_CONSTRAINT_BODY: Node = Node::Choice(AT_CONSTRAINT_BODY_CHOICES); +// `constraint ` — the named-constraint tail (leads on the +// concrete `constraint` keyword, so it is a safe `Choice` sibling). +static AT_CONSTRAINT_NAMED_NODES: &[Node] = &[ + Node::Word(Word::keyword("constraint")), + CONSTRAINT_NAME, + AT_CONSTRAINT_BODY, +]; +const AT_CONSTRAINT_NAMED: Node = Node::Seq(AT_CONSTRAINT_NAMED_NODES); + +// The `add` tail: a column def, a named constraint, or one of the bare +// (unnamed) constraint bodies — each branch leads on a distinct keyword +// (column / constraint / check / unique / foreign / primary). +static AT_ADD_TAIL_CHOICES: &[Node] = &[ + AT_ADD_COLUMN_TAIL, + AT_CONSTRAINT_NAMED, + super::sql_create_table::TABLE_CHECK, + super::sql_create_table::TABLE_UNIQUE, + super::sql_create_table::TABLE_FK, + super::sql_create_table::TABLE_PK, +]; +const AT_ADD_TAIL: Node = Node::Choice(AT_ADD_TAIL_CHOICES); +static AT_ADD_NODES: &[Node] = &[Node::Word(Word::keyword("add")), AT_ADD_TAIL]; +const AT_ADD: Node = Node::Seq(AT_ADD_NODES); + +// The `drop` tail: a column or a named constraint (distinct second +// keywords `column` / `constraint`). +static AT_DROP_CONSTRAINT_TAIL_NODES: &[Node] = + &[Node::Word(Word::keyword("constraint")), CONSTRAINT_NAME]; +const AT_DROP_CONSTRAINT_TAIL: Node = Node::Seq(AT_DROP_CONSTRAINT_TAIL_NODES); +static AT_DROP_TAIL_CHOICES: &[Node] = &[AT_DROP_COLUMN_TAIL, AT_DROP_CONSTRAINT_TAIL]; +const AT_DROP_TAIL: Node = Node::Choice(AT_DROP_TAIL_CHOICES); +static AT_DROP_NODES: &[Node] = &[Node::Word(Word::keyword("drop")), AT_DROP_TAIL]; +const AT_DROP: Node = Node::Seq(AT_DROP_NODES); + +// One branch per leading verb (`add`/`drop`/`rename`/`alter`) — distinct +// concrete keywords, trap-safe. (The branch's `alter` is the action +// word; the entry-word `alter` was already consumed by dispatch.) The +// second-keyword fan-out happens in `AT_ADD` / `AT_DROP`'s inner Choice. +static AT_ACTION_CHOICES: &[Node] = &[AT_ADD, AT_DROP, AT_RENAME_COLUMN, AT_ALTER_COLUMN]; const AT_ACTION: Node = Node::Choice(AT_ACTION_CHOICES); static SQL_ALTER_TABLE_SHAPE_NODES: &[Node] = &[ @@ -2040,32 +2113,142 @@ fn build_alter_column_type(path: &MatchedPath) -> Result`. fn build_sql_alter_table(path: &MatchedPath, source: &str) -> Result { let table = require_ident(path, "table_name")?; 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 { - old: require_ident(path, "column_name")?, - new: require_ident(path, "new_column_name")?, + } else if path.contains_word("column") { + if path.contains_word("add") { + AlterTableAction::AddColumn(Box::new(build_alter_add_column_spec(path, source)?)) + } else if path.contains_word("rename") { + AlterTableAction::RenameColumn { + old: require_ident(path, "column_name")?, + new: require_ident(path, "new_column_name")?, + } + } else { + AlterTableAction::DropColumn { + column: require_ident(path, "column_name")?, + } } + } else if path.contains_word("add") { + build_alter_add_table_constraint(path, source)? } else { - AlterTableAction::DropColumn { - column: require_ident(path, "column_name")?, + AlterTableAction::DropConstraint { + name: require_ident(path, "constraint_name")?, } }; Ok(Command::SqlAlterTable { table, action }) } +/// Build the `ADD [CONSTRAINT ] (CHECK | UNIQUE | FOREIGN KEY | …)` +/// action (ADR-0035 §4g). The body is discriminated by its leading +/// concrete keyword. The optional `CONSTRAINT ` prefix becomes the +/// action-level `name` (used by CHECK + FK at execution; refused on +/// UNIQUE). `ADD PRIMARY KEY` parses (for a clean message) but is +/// refused — every playground table already has a PK. +fn build_alter_add_table_constraint( + path: &MatchedPath, + source: &str, +) -> Result { + let name = ident(path, "constraint_name").map(str::to_string); + if path.contains_word("primary") { + return Err(ValidationError { + message_key: "parse.custom.alter_add_primary_key", + args: vec![], + }); + } + let constraint = if path.contains_word("check") { + TableConstraint::Check { + expr_sql: capture_table_check_sql(path, source)?, + } + } else if path.contains_word("unique") { + if name.is_some() { + return Err(ValidationError { + message_key: "parse.custom.alter_named_unique", + args: vec![], + }); + } + TableConstraint::Unique { + columns: collect_idents(path, "unique_column"), + } + } else { + // FOREIGN KEY — the §4g name lives at the action level, so the + // FK body itself is parsed unnamed. + TableConstraint::ForeignKey(build_alter_fk(path)) + }; + Ok(AlterTableAction::AddTableConstraint { + name, + constraint: Box::new(constraint), + }) +} + +/// Capture the raw SQL text of an `ADD … CHECK ()` (ADR-0035 §4g). +/// `sql_expr` is validate-only, so the expression is captured by byte +/// span — the 4a.2 / 4e mechanism. +fn capture_table_check_sql( + path: &MatchedPath, + source: &str, +) -> Result { + let mut items = path.items.iter().peekable(); + while let Some(item) = items.next() { + if matches!(item.kind, MatchedKind::Word("check")) + && let Some((start, end)) = capture_parenthesised_span(&mut items) + { + return Ok(source[start..end].trim().to_string()); + } + } + Err(ValidationError { + message_key: "parse.error_wrapper", + args: vec![("detail", "add check needs an expression".to_string())], + }) +} + +/// Build the `SqlForeignKey` for an `ADD [CONSTRAINT ] FOREIGN KEY +/// () REFERENCES

[()] [ON …]` (ADR-0035 §4g). Mirrors the +/// table-level FK walk in `build_sql_create_table`, reusing +/// `consume_fk_reference`. The name is supplied at the action level (so +/// the FK is parsed unnamed here). +fn build_alter_fk(path: &MatchedPath) -> SqlForeignKey { + let mut items = path.items.iter().peekable(); + // Advance to the `foreign` keyword. + while items + .peek() + .is_some_and(|it| !matches!(it.kind, MatchedKind::Word("foreign"))) + { + items.next(); + } + items.next(); // `foreign` + if matches!(items.peek().map(|i| &i.kind), Some(MatchedKind::Word("key"))) { + items.next(); + } + if matches!(items.peek().map(|i| &i.kind), Some(MatchedKind::Punct('('))) { + items.next(); + } + let child_column = items.next().map_or_else(String::new, |it| it.text.clone()); + if matches!(items.peek().map(|i| &i.kind), Some(MatchedKind::Punct(')'))) { + items.next(); + } + if matches!(items.peek().map(|i| &i.kind), Some(MatchedKind::Word("references"))) { + items.next(); + } + consume_fk_reference(&mut items, None, child_column) +} + pub static SQL_ALTER_TABLE: CommandNode = CommandNode { entry: Word::keyword("alter"), shape: SQL_ALTER_TABLE_SHAPE, @@ -2529,7 +2712,7 @@ mod sql_create_index_tests { #[cfg(test)] mod sql_alter_table_tests { - use crate::dsl::command::{AlterTableAction, ColumnSpec, Command}; + use crate::dsl::command::{AlterTableAction, ColumnSpec, Command, TableConstraint}; use crate::dsl::parser::parse_command_in_mode; use crate::mode::Mode; @@ -2696,6 +2879,130 @@ mod sql_alter_table_tests { )); } + #[test] + fn add_table_check_unnamed_and_named() { + // ADR-0035 §4g: table-level CHECK, unnamed and named. + match alter("alter table T add check (a < b)").1 { + AlterTableAction::AddTableConstraint { name, constraint } => { + assert_eq!(name, None); + assert!(matches!(*constraint, TableConstraint::Check { ref expr_sql } if expr_sql == "a < b")); + } + other => panic!("expected AddTableConstraint/Check, got {other:?}"), + } + match alter("alter table T add constraint a_lt_b check (a < b)").1 { + AlterTableAction::AddTableConstraint { name, constraint } => { + assert_eq!(name.as_deref(), Some("a_lt_b")); + assert!(matches!(*constraint, TableConstraint::Check { .. })); + } + other => panic!("expected named AddTableConstraint/Check, got {other:?}"), + } + } + + #[test] + fn add_composite_unique() { + match alter("alter table T add unique (a, b)").1 { + AlterTableAction::AddTableConstraint { name, constraint } => { + assert_eq!(name, None); + assert!(matches!(*constraint, TableConstraint::Unique { ref columns } if columns == &["a".to_string(), "b".to_string()])); + } + other => panic!("expected AddTableConstraint/Unique, got {other:?}"), + } + } + + #[test] + fn named_unique_is_refused() { + // §4g: composite UNIQUE is anonymous in our model — naming it is + // refused by the BUILDER (it parses, then the builder rejects), + // so the error is the friendly message, not a parse error. + let err = parse_command_in_mode( + "alter table T add constraint u unique (a, b)", + Mode::Advanced, + ) + .expect_err("a named UNIQUE constraint is refused"); + assert!( + err.to_string().to_lowercase().contains("unique constraint cannot be named"), + "expected the builder's named-UNIQUE refusal, got: {err}" + ); + } + + #[test] + fn add_primary_key_is_refused() { + // §4g: adding a PK to an existing table is refused by the BUILDER + // (it parses for a clean message, then the builder rejects it). + let err = parse_command_in_mode("alter table T add primary key (id)", Mode::Advanced) + .expect_err("ADD PRIMARY KEY is refused"); + assert!( + err.to_string().to_lowercase().contains("primary key is fixed at creation"), + "expected the builder's ADD-PRIMARY-KEY refusal, got: {err}" + ); + } + + #[test] + fn add_foreign_key_named_and_bare() { + // `add foreign key (col) references P(id)` and the bare + // `references P` form; named via the CONSTRAINT prefix. + match alter("alter table C add foreign key (pid) references P(id)").1 { + AlterTableAction::AddTableConstraint { name, constraint } => { + assert_eq!(name, None); + match *constraint { + TableConstraint::ForeignKey(fk) => { + assert_eq!(fk.child_column, "pid"); + assert_eq!(fk.parent_table, "P"); + assert_eq!(fk.parent_column.as_deref(), Some("id")); + } + other => panic!("expected ForeignKey, got {other:?}"), + } + } + other => panic!("expected AddTableConstraint/FK, got {other:?}"), + } + match alter("alter table C add constraint fk_p foreign key (pid) references P").1 { + AlterTableAction::AddTableConstraint { name, constraint } => { + assert_eq!(name.as_deref(), Some("fk_p")); + match *constraint { + TableConstraint::ForeignKey(fk) => { + assert_eq!(fk.parent_column, None, "bare reference resolves at execution"); + } + other => panic!("expected ForeignKey, got {other:?}"), + } + } + other => panic!("expected named AddTableConstraint/FK, got {other:?}"), + } + } + + #[test] + fn drop_constraint_by_name() { + match alter("alter table T drop constraint a_lt_b").1 { + AlterTableAction::DropConstraint { name } => assert_eq!(name, "a_lt_b"), + other => panic!("expected DropConstraint, got {other:?}"), + } + } + + #[test] + fn six_branch_dispatch_still_routes_column_actions() { + // The two new add/drop-constraint branches do not steal the four + // column actions. + assert!(matches!( + alter("alter table T add column note text").1, + AlterTableAction::AddColumn(_) + )); + assert!(matches!( + alter("alter table T add column code text unique").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 alter_is_advanced_only() { // No simple `alter`; in simple mode it does not parse as a diff --git a/src/dsl/grammar/sql_create_table.rs b/src/dsl/grammar/sql_create_table.rs index 3efbab5..1d0be1c 100644 --- a/src/dsl/grammar/sql_create_table.rs +++ b/src/dsl/grammar/sql_create_table.rs @@ -263,7 +263,10 @@ static TABLE_PK_NODES: &[Node] = &[ }, Node::Punct(')'), ]; -const TABLE_PK: Node = Node::Seq(TABLE_PK_NODES); +// `pub(crate)` so `ALTER TABLE … ADD PRIMARY KEY (…)` (ADR-0035 §4g) +// reuses the body — it parses, then the ALTER builder refuses it with a +// specific message (adding a PK to an existing table is unsupported). +pub(crate) const TABLE_PK: Node = Node::Seq(TABLE_PK_NODES); // Table-level `UNIQUE ( col, … )`. A single column normalises into // that column's `unique` flag (round-trips via the existing @@ -292,7 +295,9 @@ static TABLE_UNIQUE_NODES: &[Node] = &[ }, Node::Punct(')'), ]; -const TABLE_UNIQUE: Node = Node::Seq(TABLE_UNIQUE_NODES); +// `pub(crate)` so `ALTER TABLE … ADD UNIQUE (…)` (ADR-0035 §4g) reuses +// the same composite-UNIQUE body. +pub(crate) const TABLE_UNIQUE: Node = Node::Seq(TABLE_UNIQUE_NODES); // Table-level `CHECK ( )` (ADR-0035 §4a.3) — a multi-column // CHECK referencing several columns. Same paren-bounded shape as the @@ -306,7 +311,9 @@ static TABLE_CHECK_NODES: &[Node] = &[ Node::Subgrammar(&sql_expr::SQL_OR_EXPR), Node::Punct(')'), ]; -const TABLE_CHECK: Node = Node::Seq(TABLE_CHECK_NODES); +// `pub(crate)` so `ALTER TABLE … ADD [CONSTRAINT ] CHECK (…)` +// (ADR-0035 §4g) reuses the same table-CHECK body. +pub(crate) const TABLE_CHECK: Node = Node::Seq(TABLE_CHECK_NODES); // Table-level foreign key (ADR-0035 §5, sub-phase 4b): // `[CONSTRAINT ] FOREIGN KEY ( ) REFERENCES @@ -356,7 +363,10 @@ static FOREIGN_KEY_BODY_NODES: &[Node] = &[ ]; const FOREIGN_KEY_BODY: Node = Node::Seq(FOREIGN_KEY_BODY_NODES); // `FOREIGN KEY (…) …` — the unnamed table-level FK (auto-named). -const TABLE_FK: Node = FOREIGN_KEY_BODY; +// `pub(crate)` so `ALTER TABLE … ADD [CONSTRAINT ] FOREIGN KEY +// (…)` (ADR-0035 §4g) reuses the FK body (the §4g `CONSTRAINT ` +// prefix is supplied by the ALTER grammar, not this body). +pub(crate) const TABLE_FK: Node = FOREIGN_KEY_BODY; // `CONSTRAINT FOREIGN KEY (…) …` — the named table-level FK. static TABLE_FK_NAMED_NODES: &[Node] = &[ Node::Word(Word::keyword("constraint")), diff --git a/src/friendly/keys.rs b/src/friendly/keys.rs index de10dfd..a84cc7a 100644 --- a/src/friendly/keys.rs +++ b/src/friendly/keys.rs @@ -227,6 +227,8 @@ pub const KEYS_AND_PLACEHOLDERS: &[(&str, &[&str])] = &[ ("parse.caret", &["padding"]), // Custom (try_map / source-slice) error messages raised // by the DSL parser. See `parse.custom.*` in the catalog. + ("parse.custom.alter_add_primary_key", &[]), + ("parse.custom.alter_named_unique", &[]), ("parse.custom.bind_type_mismatch", &["found", "expected"]), ("parse.custom.change_column_flags_exclusive", &[]), ("parse.custom.constraint_redundant_on_pk", &["column", "constraint"]), diff --git a/src/friendly/strings/en-US.yaml b/src/friendly/strings/en-US.yaml index f424123..bad5bf4 100644 --- a/src/friendly/strings/en-US.yaml +++ b/src/friendly/strings/en-US.yaml @@ -274,7 +274,9 @@ help: alter table add column [not null] [unique] [default …] [check …] alter table drop column alter table rename column to - alter table alter column type — change a table's columns (advanced SQL) + alter table alter column type + alter table add [constraint ] check () | unique (, …) | foreign key () references

[()] + alter table drop constraint — evolve a table's columns and constraints (advanced SQL) drop: |- drop table — remove a table drop column [from] [table] : [--cascade] — remove a column @@ -425,6 +427,12 @@ parse: expression_too_deep: "expression nested too deeply" on_action_specified_twice: "`on {target}` specified twice" change_column_flags_exclusive: "`--force-conversion` and `--dont-convert` are mutually exclusive — pick one." + # ADR-0035 §4g: adding a primary key to an existing table is not + # supported — every table is created with its primary key. + alter_add_primary_key: "a table's primary key is fixed at creation — `alter table … add primary key` is not supported." + # ADR-0035 §4g: composite UNIQUE constraints are unnamed in this + # tool, so a `constraint ` prefix on UNIQUE has nowhere to go. + alter_named_unique: "a UNIQUE constraint cannot be named — use `alter table add unique (, …)` without `constraint `." unknown_type: "unknown type '{found}' (expected one of: {expected})" unknown_action: "unknown referential action '{found}' (expected one of: {expected})" # Phase D typed-value-slot mismatch (ADR-0024 §Phase D): @@ -475,6 +483,8 @@ parse: alter table drop column alter table
rename column to alter table
alter column type + alter table
add [constraint ] check () | unique (, ...) | foreign key () references [()] + alter table
drop constraint drop_table: "drop table " drop_column: "drop column [from] [table]
: " drop_relationship: |- diff --git a/src/persistence/mod.rs b/src/persistence/mod.rs index c1faeb3..ee09937 100644 --- a/src/persistence/mod.rs +++ b/src/persistence/mod.rs @@ -147,12 +147,37 @@ pub struct TableSchema { /// optional on read. pub unique_constraints: Vec>, /// Table-level `CHECK ()` constraints, in declaration - /// order, as raw SQL text (ADR-0035 §4a.3). The engine reports - /// no CHECK constraints, so these are the source of truth (held - /// in `__rdbms_playground_table_checks`) and echoed verbatim - /// into the rebuilt DDL. Empty for project files written before - /// table-level CHECK existed — the YAML field is optional on read. - pub check_constraints: Vec, + /// order, as raw SQL text with an optional name (ADR-0035 §4a.3, + /// named in §4g). The engine reports no CHECK constraints, so these + /// are the source of truth (held in + /// `__rdbms_playground_table_checks`) and echoed verbatim into the + /// rebuilt DDL. Empty for project files written before table-level + /// CHECK existed — the YAML field is optional on read. + pub check_constraints: Vec, +} + +/// A table-level `CHECK` constraint with an optional name (ADR-0035 §4g). +/// +/// The name is `Some` only for a `CONSTRAINT CHECK (…)` added via +/// `ALTER TABLE` (the source of `DROP CONSTRAINT `); a `CREATE +/// TABLE` table-CHECK and any pre-4g project file are unnamed (`None`). +/// The YAML carries a bare string for the unnamed form (back-compatible) +/// and an `{expr, name}` mapping for the named form. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct TableCheck { + pub name: Option, + pub expr: String, +} + +impl TableCheck { + /// An unnamed table-CHECK (the `CREATE TABLE` / pre-4g form). + #[must_use] + pub fn unnamed(expr: impl Into) -> Self { + Self { + name: None, + expr: expr.into(), + } + } } #[derive(Debug, Clone, PartialEq, Eq)] diff --git a/src/persistence/yaml.rs b/src/persistence/yaml.rs index f46bc51..c5ac1e3 100644 --- a/src/persistence/yaml.rs +++ b/src/persistence/yaml.rs @@ -23,7 +23,9 @@ use serde::Deserialize; use crate::dsl::action::ReferentialAction; use crate::dsl::types::Type; -use super::{ColumnSchema, IndexSchema, RelationshipSchema, SchemaSnapshot, TableSchema}; +use super::{ + ColumnSchema, IndexSchema, RelationshipSchema, SchemaSnapshot, TableCheck, TableSchema, +}; /// Serialize a `SchemaSnapshot` to a `project.yaml` body. #[must_use] @@ -113,11 +115,21 @@ fn write_table(out: &mut String, table: &TableSchema) { } // Table-level CHECK constraints as raw SQL text (ADR-0035 §4a.3) — // double-quoted (an expression like `a < b` is not a bare scalar) - // and emitted only when present. + // and emitted only when present. An unnamed CHECK is a bare string + // (back-compatible); a named CHECK (ADR-0035 §4g) is an `{expr, + // name}` mapping so the name round-trips through a rebuild. if !table.check_constraints.is_empty() { let _ = writeln!(out, " check_constraints:"); - for expr in &table.check_constraints { - let _ = writeln!(out, " - {}", yaml_string(expr)); + for check in &table.check_constraints { + match &check.name { + None => { + let _ = writeln!(out, " - {}", yaml_string(&check.expr)); + } + Some(name) => { + let _ = writeln!(out, " - expr: {}", yaml_string(&check.expr)); + let _ = writeln!(out, " name: {}", yaml_string(name)); + } + } } } } @@ -280,7 +292,7 @@ pub(crate) fn parse_schema(body: &str) -> Result { primary_key: t.primary_key, columns, unique_constraints: t.unique_constraints, - check_constraints: t.check_constraints, + check_constraints: t.check_constraints.into_iter().map(TableCheck::from).collect(), }); } let mut relationships: Vec = Vec::with_capacity(raw.relationships.len()); @@ -394,10 +406,35 @@ struct RawTable { /// Optional on read — older project files omit it. #[serde(default)] unique_constraints: Vec>, - /// Table-level CHECK constraints as raw SQL text (ADR-0035 §4a.3). - /// Optional on read — older project files omit it. + /// Table-level CHECK constraints (ADR-0035 §4a.3, named in §4g). + /// Optional on read — older project files omit it. Each entry is a + /// bare string (unnamed) or an `{expr, name}` mapping (named). #[serde(default)] - check_constraints: Vec, + check_constraints: Vec, +} + +/// A table-CHECK as read from `project.yaml`: a bare string (unnamed — +/// the pre-4g form, back-compatible) or an `{expr, name}` mapping (a +/// named CHECK, ADR-0035 §4g). `#[serde(untagged)]` tries the string +/// form first, then the mapping. +#[derive(Deserialize)] +#[serde(untagged)] +enum RawTableCheck { + Bare(String), + Named { + expr: String, + #[serde(default)] + name: Option, + }, +} + +impl From for TableCheck { + fn from(raw: RawTableCheck) -> Self { + match raw { + RawTableCheck::Bare(expr) => Self { name: None, expr }, + RawTableCheck::Named { expr, name } => Self { name, expr }, + } + } } #[derive(Deserialize)] @@ -714,7 +751,10 @@ indexes: ColumnSchema { name: "c".to_string(), user_type: Type::Int, unique: false, not_null: false, default: None, check: None }, ], unique_constraints: vec![vec!["a".to_string(), "b".to_string()]], - check_constraints: vec!["a < b".to_string(), "b < c".to_string()], + check_constraints: vec![ + TableCheck::unnamed("a < b"), + TableCheck::unnamed("b < c"), + ], }], relationships: vec![], indexes: vec![], @@ -727,6 +767,60 @@ indexes: ); } + #[test] + fn named_check_constraints_round_trip_through_yaml() { + // ADR-0035 §4g: a *named* table-CHECK serializes to the `{expr, + // name}` mapping form and round-trips, mixed with an unnamed one. + let snap = SchemaSnapshot { + created_at: "2026-05-25T00:00:00Z".to_string(), + tables: vec![TableSchema { + name: "T".to_string(), + primary_key: vec!["id".to_string()], + columns: vec![ + ColumnSchema { name: "id".to_string(), user_type: Type::Int, unique: false, not_null: false, default: None, check: None }, + ColumnSchema { name: "qty".to_string(), user_type: Type::Int, unique: false, not_null: false, default: None, check: None }, + ], + unique_constraints: vec![], + check_constraints: vec![ + TableCheck { name: Some("qty_positive".to_string()), expr: "qty >= 0".to_string() }, + TableCheck::unnamed("qty < 1000"), + ], + }], + relationships: vec![], + indexes: vec![], + }; + let body = serialize_schema(&snap); + let parsed = parse_schema(&body).expect("parse schema"); + assert_eq!(parsed, snap, "named + unnamed table-CHECKs survive the yaml round-trip"); + } + + #[test] + fn old_format_bare_string_check_constraints_still_parse() { + // Back-compat: a project file written before §4g (bare-string + // check_constraints) parses with name = None. + let body = "\ +version: 1 +project: + created_at: \"2026-05-25T00:00:00Z\" +tables: + - name: T + primary_key: [id] + columns: + - { name: id, type: int } + - { name: qty, type: int } + check_constraints: + - \"qty >= 0\" +relationships: [] +indexes: [] +"; + let parsed = parse_schema(body).expect("parse old-format schema"); + assert_eq!( + parsed.tables[0].check_constraints, + vec![TableCheck::unnamed("qty >= 0")], + "a bare-string CHECK parses as an unnamed TableCheck" + ); + } + #[test] fn check_constraints_optional_on_read() { // A project file written before table-level CHECK existed (no diff --git a/src/runtime.rs b/src/runtime.rs index 0cf5aed..b0080e0 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -33,6 +33,7 @@ use crate::db::{ Database, DbError, DeleteResult, DropColumnResult, DropIndexOutcome, DropOutcome, InsertResult, QueryPlan, TableDescription, UpdateResult, }; +use crate::dsl::command::TableConstraint; use crate::dsl::{AlterTableAction, ChangeColumnMode, Command, ColumnSpec}; use crate::dsl::walker::Severity; use crate::event::AppEvent; @@ -2124,6 +2125,30 @@ async fn execute_command_typed( .change_column_type(table, column, ty, ChangeColumnMode::ForceConversion, src) .await .map(CommandOutcome::ChangeColumn), + // `ADD [CONSTRAINT ] (CHECK | UNIQUE | FOREIGN KEY)` + // (ADR-0035 §4g) — each reuses an existing low-level executor + // (the FK via the relationship machinery `add 1:n + // relationship` uses); one undo step each. + AlterTableAction::AddTableConstraint { name, constraint } => match *constraint { + TableConstraint::Check { expr_sql } => database + .alter_add_table_check(table, name, expr_sql, src) + .await + .map(|d| CommandOutcome::Schema(Some(d))), + TableConstraint::Unique { columns } => database + .alter_add_unique(table, columns, src) + .await + .map(|d| CommandOutcome::Schema(Some(d))), + TableConstraint::ForeignKey(fk) => database + .alter_add_foreign_key(table, name, fk, src) + .await + .map(|d| CommandOutcome::Schema(Some(d))), + }, + // `DROP CONSTRAINT ` — a named table-CHECK or a named + // FK, resolved by the executor (ADR-0035 §4g). + AlterTableAction::DropConstraint { name } => database + .alter_drop_constraint(table, name, src) + .await + .map(CommandOutcome::Schema), }, Command::AddConstraint { table, diff --git a/tests/column_op_guards.rs b/tests/column_op_guards.rs index 6dfa7f4..a752569 100644 --- a/tests/column_op_guards.rs +++ b/tests/column_op_guards.rs @@ -10,7 +10,8 @@ //! rename-drift bug that would break a later rebuild). use rdbms_playground::db::Database; -use rdbms_playground::dsl::{ChangeColumnMode, ColumnSpec, Type}; +use rdbms_playground::dsl::command::Constraint; +use rdbms_playground::dsl::{ChangeColumnMode, ColumnSpec, ReferentialAction, Type}; use rdbms_playground::persistence::Persistence; use rdbms_playground::project; @@ -89,7 +90,7 @@ fn simple_column_ops_refuse_internal_tables() { // happen to hit. let err = r .block_on(db.change_column_type( - internal, + internal.clone(), "table_name".to_string(), Type::Int, ChangeColumnMode::Default, @@ -100,6 +101,79 @@ fn simple_column_ops_refuse_internal_tables() { format!("{err:?}").contains("NoSuchTable"), "expected a no-such-table refusal from the internal-table guard, got: {err:?}" ); + // `add constraint` (the simple surface; also the SQL `ALTER TABLE … + // ADD CONSTRAINT` decomposition target — ADR-0035 §4g) is refused: + // the guard lives in `do_add_constraint`. + let err = r + .block_on(db.add_constraint( + internal, + "table_name".to_string(), + Constraint::NotNull, + None, + )) + .expect_err("add constraint 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] +fn add_relationship_refuses_internal_tables() { + // The guard lives in `do_add_relationship` (ADR-0035 §4g) and covers + // both the parent and the child endpoint — so the simple `add 1:n + // relationship` and the SQL `ALTER TABLE … ADD FOREIGN KEY` (which + // reaches the same executor) cannot touch an internal table. + let (_p, db, _d) = open(); + let r = rt(); + let internal = "__rdbms_playground_relationships".to_string(); + // Internal *parent* — refused up-front. + let err = r + .block_on(db.add_relationship( + None, + internal.clone(), + "name".to_string(), + "C".to_string(), + "x".to_string(), + ReferentialAction::NoAction, + ReferentialAction::NoAction, + false, + None, + )) + .expect_err("relationship with an internal parent is refused"); + assert!( + format!("{err:?}").contains("NoSuchTable"), + "expected a no-such-table refusal (internal parent), got: {err:?}" + ); + // Internal *child* — also refused (a real parent exists). + r.block_on(db.sql_create_table( + "P".to_string(), + vec![ColumnSpec::new("id", Type::Int)], + vec!["id".to_string()], + vec![], + vec![], + vec![], + false, + Some("create table P (id int primary key)".to_string()), + )) + .expect("create P"); + let err = r + .block_on(db.add_relationship( + None, + "P".to_string(), + "id".to_string(), + internal, + "x".to_string(), + ReferentialAction::NoAction, + ReferentialAction::NoAction, + false, + None, + )) + .expect_err("relationship with an internal child is refused"); + assert!( + format!("{err:?}").contains("NoSuchTable"), + "expected a no-such-table refusal (internal child), got: {err:?}" + ); } #[test] diff --git a/tests/sql_alter_table.rs b/tests/sql_alter_table.rs index 484f873..9f7b2c5 100644 --- a/tests/sql_alter_table.rs +++ b/tests/sql_alter_table.rs @@ -358,3 +358,241 @@ fn e2e_alter_column_type_is_one_undo_step() { ); assert_eq!(col_type(&db, &r, "v"), Some(Type::Real), "one undo restored the pre-conversion type"); } + +// --- 4g: ADD/DROP constraint + ADD foreign key (ADR-0035 §4g) ----------- + +/// True if inserting `(id, qty)` into table `T` succeeds. +fn insert_t_qty_ok(db: &Database, r: &tokio::runtime::Runtime, id: i64, qty: i64) -> bool { + r.block_on(db.insert( + "T".to_string(), + Some(vec!["id".to_string(), "qty".to_string()]), + vec![Value::Number(id.to_string()), Value::Number(qty.to_string())], + Some("insert".to_string()), + )) + .is_ok() +} + +#[test] +fn e2e_add_named_check_enforced_and_survives_rebuild_with_its_name() { + // ADD a named table-CHECK; it is enforced; it round-trips through a + // rebuild *with its name* — proven by DROP CONSTRAINT still + // resolving after the rebuild (the name reached project.yaml and back). + let (project, db, _d) = open(); + let r = rt(); + std::fs::write( + project.path().join("c.commands"), + "create table T with pk id(int)\n\ + add column T: qty (int)\n\ + alter table T add constraint qty_positive check (qty >= 0)\n", + ) + .expect("write"); + let events = r.block_on(run_replay(&db, project.path(), "c.commands")); + assert!( + matches!(events.last(), Some(AppEvent::ReplayCompleted { count, .. }) if *count == 3), + "events: {events:?}" + ); + // Enforced: qty = -1 refused, qty = 5 accepted. + assert!(!insert_t_qty_ok(&db, &r, 1, -1), "the CHECK rejects qty = -1"); + assert!(insert_t_qty_ok(&db, &r, 2, 5), "qty = 5 satisfies the CHECK"); + + // Rebuild from text, then DROP CONSTRAINT by name must still work → + // the name survived the round-trip. + r.block_on(db.rebuild_from_text(project.path().to_path_buf(), Some("rebuild".to_string()))) + .expect("rebuild"); + assert!(!insert_t_qty_ok(&db, &r, 3, -2), "the CHECK is intact after rebuild"); + std::fs::write( + project.path().join("drop.commands"), + "alter table T drop constraint qty_positive\n", + ) + .expect("write"); + let events = r.block_on(run_replay(&db, project.path(), "drop.commands")); + assert!( + matches!(events.last(), Some(AppEvent::ReplayCompleted { count, .. }) if *count == 1), + "DROP CONSTRAINT resolved the name after rebuild; events: {events:?}" + ); + // After the drop the CHECK no longer applies: qty = -1 is accepted. + assert!(insert_t_qty_ok(&db, &r, 4, -1), "the CHECK was dropped"); +} + +#[test] +fn e2e_add_check_with_violating_data_is_refused() { + assert!( + replay_is_refused( + "create table T with pk id(int)\n\ + add column T: qty (int)\n\ + insert into T (id, qty) values (1, -5)\n\ + alter table T add check (qty >= 0)\n", + ), + "adding a CHECK that existing rows violate is refused" + ); +} + +#[test] +fn e2e_add_composite_unique_enforced_and_survives_rebuild() { + let (project, db, _d) = open(); + let r = rt(); + std::fs::write( + project.path().join("u.commands"), + "create table T with pk id(int)\n\ + add column T: a (int)\n\ + add column T: b (int)\n\ + insert into T (id, a, b) values (1, 1, 2)\n\ + alter table T add unique (a, b)\n", + ) + .expect("write"); + let events = r.block_on(run_replay(&db, project.path(), "u.commands")); + assert!( + matches!(events.last(), Some(AppEvent::ReplayCompleted { count, .. }) if *count == 5), + "events: {events:?}" + ); + let dup_ok = |id: i64, a: i64, b: i64| { + r.block_on(db.insert( + "T".to_string(), + Some(vec!["id".to_string(), "a".to_string(), "b".to_string()]), + vec![ + Value::Number(id.to_string()), + Value::Number(a.to_string()), + Value::Number(b.to_string()), + ], + Some("insert".to_string()), + )) + .is_ok() + }; + assert!(!dup_ok(2, 1, 2), "the composite UNIQUE rejects the duplicate (1, 2)"); + assert!(dup_ok(3, 1, 3), "(1, 3) is distinct and accepted"); + + // Survives rebuild (the unique_constraints yaml path). + r.block_on(db.rebuild_from_text(project.path().to_path_buf(), Some("rebuild".to_string()))) + .expect("rebuild"); + assert!(!dup_ok(4, 1, 2), "the composite UNIQUE is intact after rebuild"); +} + +#[test] +fn e2e_add_unique_with_duplicate_data_is_refused() { + assert!( + replay_is_refused( + "create table T with pk id(int)\n\ + add column T: a (int)\n\ + insert into T (id, a) values (1, 7)\n\ + insert into T (id, a) values (2, 7)\n\ + alter table T add unique (a)\n", + ), + "adding a UNIQUE that existing rows violate is refused" + ); +} + +#[test] +fn e2e_add_foreign_key_creates_an_enforced_relationship() { + let (project, db, _d) = open(); + let r = rt(); + std::fs::write( + project.path().join("fk.commands"), + "create table P with pk id(int)\n\ + create table C with pk cid(int)\n\ + add column C: pid (int)\n\ + insert into P (id) values (1)\n\ + alter table C add constraint c_to_p foreign key (pid) references P(id)\n", + ) + .expect("write"); + let events = r.block_on(run_replay(&db, project.path(), "fk.commands")); + assert!( + matches!(events.last(), Some(AppEvent::ReplayCompleted { count, .. }) if *count == 5), + "events: {events:?}" + ); + // FK enforced: a child row referencing a missing parent is rejected; + // one referencing the existing parent (id = 1) is accepted. + let insert_c = |cid: i64, pid: i64| { + r.block_on(db.insert( + "C".to_string(), + Some(vec!["cid".to_string(), "pid".to_string()]), + vec![Value::Number(cid.to_string()), Value::Number(pid.to_string())], + Some("insert".to_string()), + )) + }; + assert!(insert_c(10, 1).is_ok(), "a child referencing parent id=1 is accepted"); + assert!(insert_c(11, 999).is_err(), "a child referencing a missing parent is rejected"); +} + +#[test] +fn e2e_drop_constraint_removes_a_named_foreign_key() { + let (project, db, _d) = open(); + let r = rt(); + std::fs::write( + project.path().join("fk.commands"), + "create table P with pk id(int)\n\ + create table C with pk cid(int)\n\ + add column C: pid (int)\n\ + alter table C add constraint c_to_p foreign key (pid) references P(id)\n\ + alter table C drop constraint c_to_p\n", + ) + .expect("write"); + let events = r.block_on(run_replay(&db, project.path(), "fk.commands")); + assert!( + matches!(events.last(), Some(AppEvent::ReplayCompleted { count, .. }) if *count == 5), + "events: {events:?}" + ); + // The FK is gone: a child referencing a missing parent now succeeds. + assert!( + r.block_on(db.insert( + "C".to_string(), + Some(vec!["cid".to_string(), "pid".to_string()]), + vec![Value::Number("1".to_string()), Value::Number("999".to_string())], + Some("insert".to_string()), + )) + .is_ok(), + "after DROP CONSTRAINT the FK no longer applies" + ); +} + +#[test] +fn e2e_constraint_name_collision_is_refused() { + // A named CHECK cannot reuse a relationship (FK) name on the same + // table — both are `DROP CONSTRAINT ` targets, so a collision + // would make the drop ambiguous. + 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\ + alter table C add constraint dup foreign key (pid) references P(id)\n\ + alter table C add constraint dup check (cid > 0)\n", + ), + "a CHECK reusing an existing FK name on the table is refused" + ); +} + +#[test] +fn e2e_drop_unknown_constraint_is_refused() { + assert!( + replay_is_refused( + "create table T with pk id(int)\n\ + alter table T drop constraint nope\n", + ), + "dropping a non-existent constraint is refused" + ); +} + +#[test] +fn e2e_add_constraint_is_one_undo_step() { + // ADD CONSTRAINT CHECK is one rebuild = one undo step; driven through + // the full SQL pipeline, then undone in one. + let (project, db, _d) = open_with_undo(); + let r = rt(); + std::fs::write( + project.path().join("c.commands"), + "create table T with pk id(int)\n\ + add column T: qty (int)\n\ + insert into T (id, qty) values (1, 5)\n\ + alter table T add constraint qty_positive check (qty >= 0)\n", + ) + .expect("write"); + r.block_on(run_replay(&db, project.path(), "c.commands")); + assert!(!insert_t_qty_ok(&db, &r, 2, -1), "the CHECK is enforced"); + + assert!( + r.block_on(db.undo()).expect("undo").is_some(), + "the ADD CONSTRAINT was one undo step" + ); + // After undo the CHECK is gone: qty = -1 is accepted. + assert!(insert_t_qty_ok(&db, &r, 3, -1), "one undo removed the CHECK"); +}