diff --git a/docs/adr/0035-advanced-mode-sql-ddl.md b/docs/adr/0035-advanced-mode-sql-ddl.md index e044eea..3bc1dda 100644 --- a/docs/adr/0035-advanced-mode-sql-ddl.md +++ b/docs/adr/0035-advanced-mode-sql-ddl.md @@ -17,7 +17,11 @@ the CREATE-TABLE help/usage refresh — implemented 2026-05-25/26 — plans `…-4e.md`, `…-4f.md`, `…-4g.md`, `docs/plans/20260526-adr-0035-sql-ddl-4h.md`, `docs/plans/20260526-adr-0035-sql-ddl-4i.md`). **Phase 4 is complete** -(4a–4i all shipped). This is **Phase 4** of the ADR-0030 roadmap (the +(4a–4i all shipped). **Amendment 1 (2026-05-26)** adds a way to **drop a +composite UNIQUE** via a derived, engine-neutral name (`unique_`) +that reuses the existing `DROP CONSTRAINT ` grammar — no new +syntax, no metadata, the §4g anonymity decision intact (see the amendment +below). 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. @@ -551,6 +555,67 @@ ADR-0033's structure: read-side (completion / diagnostics / describe / help), so no undo steps are introduced. +## Amendment 1 — Dropping a composite UNIQUE constraint (2026-05-26) + +A whole-Phase-4 `/runda` surfaced that a composite `UNIQUE(a,b)` — kept +**anonymous** by design (§4a.2, and §4g refused a *named* UNIQUE add) — +has **no way to be dropped**: `DROP CONSTRAINT ` (§4g) resolves +only a named table-CHECK or a named FK, so recreating the table was the +only escape. This amendment adds a drop path. Written with explicit user +approval; the plan is +`docs/plans/20260526-adr-0035-composite-unique-drop-f1f2f3.md`. +**Implemented 2026-05-26.** + +**It does not reverse the §4g anonymity decision.** Storage stays a bare +column-list (`unique_constraints: Vec>`, PRAGMA-detected) and +a UNIQUE still **cannot be named on `ADD`**. The addition is purely a +**derived, engine-neutral name** used to *display* and *address* the +constraint on drop. + +### The derived name (user-decided: derived, no storage; `unique_`) + +The name is a deterministic function of the column list — +`unique__…` — recomputed live wherever it is shown or +matched. Nothing is persisted: the constraint remains a bare column-list, +so the name round-trips for free and needs no metadata table and no +rebuild-arrival migration (the cost §4a.3 deliberately avoided). If a +column in the UNIQUE is later renamed, the displayed name tracks it — +arguably more correct than a frozen stored name. Alternatives weighed and +rejected: naming UNIQUEs with a user-supplied name + new metadata table +(reverses §4g; heaviest), and a positional `drop … unique (cols)` form +(needs new grammar). The derived name **reuses the existing `DROP +CONSTRAINT ` grammar — no new syntax**. + +### Surfaces + +- **`describe` / structure view.** The "Table constraints:" section (4i b) + annotates each composite UNIQUE with its name: `unique_b_c: UNIQUE + (b, c)`. +- **`ALTER TABLE DROP CONSTRAINT `** (advanced-SQL only, matching + the §4g `ADD` form). `do_drop_constraint_by_name` gains a **third + resolution step** after named table-CHECK and named FK: it recomputes + the derived name of each composite UNIQUE on `` and matches. On a + single match it rebuilds the table without that entry (the + `do_alter_add_unique` rebuild in reverse). **A name matching more than + one UNIQUE is refused as ambiguous** (e.g. a column literally named + `b_c` colliding with `UNIQUE (b, c)`) — it never guesses which to drop. + **Resolution order** means a user-named CHECK/FK with the same string + shadows a derived UNIQUE name; the distinctive `unique_` prefix makes + this unlikely and it is documented, not guarded. + +### Dropping a *column* a composite UNIQUE covers (F1) + +`do_drop_column` gains an up-front guard (alongside the index-covering +and CHECK guards): a column participating in any composite UNIQUE is +refused with the constraint's derived name and the actionable drop +command — `cannot drop \`T.c\` … part of the UNIQUE constraint +\`unique_b_c\` (b, c); drop that constraint first (\`alter table T drop +constraint unique_b_c\`)`. The refusal itself is unchanged (the engine +already refuses it); the message becomes engine-neutral and actionable. +Single-column UNIQUE column drops are a **parallel** gap (different +mechanism — ADR-0029 column-level `drop constraint`) and are **out of +scope** here. + ## Consequences - Advanced mode reaches DDL parity with simple mode and adds diff --git a/docs/adr/README.md b/docs/adr/README.md index 506881a..df7c0a0 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` + 4g `ALTER TABLE` add/drop constraint + add FK + 4h `ALTER TABLE … RENAME TO` + 4i verification sweep (completion merge + simple/advanced completion colour + describe of table-level constraints + self-ref FK indicator + CREATE-TABLE help/usage), implemented 2026-05-25/26 — **Phase 4 complete**), **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) and **4h** (`ALTER TABLE … RENAME TO` — the one genuinely new low-level op, `do_rename_table`: a native engine rename plus one-transaction reconciliation of every metadata row naming the table (`__rdbms_playground_columns`, **both ends** of `__rdbms_playground_relationships`, `__rdbms_playground_table_checks`), the CSV file (the existing rewrite+delete path — no new persistence method), and **CHECK text that qualifies a column with the old table name** (`T.age`→`U.age`, a planning-`/runda` finding — the engine rewrites the live CHECK but the stored text would drift and break a fresh rebuild; `rewrite_check_table_qualifier` keeps them in step); grammar splits the `rename` verb into one branch with an inner Choice on a distinct second keyword (`column` vs `to`), the new-name slot mirroring the `CREATE TABLE` name slot; refuses same-name / existing-target / `__rdbms_*` / non-existent, with **case-insensitive** collision checks behind an engine-neutral pre-check (a finished-slice `/runda` finding — the engine matches names case-insensitively); auto-named indexes *and* relationships keep their stale names (only table-name columns update — §6 scope); one undo step; advanced-only, closing the rename half of `C1` — all user-confirmed) and **4i** (the verification sweep that completes Phase 4: the shared-entry-word completion merge + the simple-vs-advanced completion colour-when-mixed with Both→Advanced→Simple block ordering; `describe` of table-level composite UNIQUE + table CHECK; the self-ref FK pre-submit indicator fix; and the CREATE-TABLE help/usage skeleton refresh). **All of Phase 4 (4a–4i) is shipped.** 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 + 4h `ALTER TABLE … RENAME TO` + 4i verification sweep (completion merge + simple/advanced completion colour + describe of table-level constraints + self-ref FK indicator + CREATE-TABLE help/usage), implemented 2026-05-25/26 — **Phase 4 complete**; **Amendment 1, 2026-05-26**: drop a composite UNIQUE via a derived, engine-neutral `unique_` name that reuses the existing `DROP CONSTRAINT ` grammar — no new syntax, no metadata, §4g anonymity intact; `describe` shows the name; dropping a UNIQUE-covered *column* now refuses with that name + the drop command), **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) and **4h** (`ALTER TABLE … RENAME TO` — the one genuinely new low-level op, `do_rename_table`: a native engine rename plus one-transaction reconciliation of every metadata row naming the table (`__rdbms_playground_columns`, **both ends** of `__rdbms_playground_relationships`, `__rdbms_playground_table_checks`), the CSV file (the existing rewrite+delete path — no new persistence method), and **CHECK text that qualifies a column with the old table name** (`T.age`→`U.age`, a planning-`/runda` finding — the engine rewrites the live CHECK but the stored text would drift and break a fresh rebuild; `rewrite_check_table_qualifier` keeps them in step); grammar splits the `rename` verb into one branch with an inner Choice on a distinct second keyword (`column` vs `to`), the new-name slot mirroring the `CREATE TABLE` name slot; refuses same-name / existing-target / `__rdbms_*` / non-existent, with **case-insensitive** collision checks behind an engine-neutral pre-check (a finished-slice `/runda` finding — the engine matches names case-insensitively); auto-named indexes *and* relationships keep their stale names (only table-name columns update — §6 scope); one undo step; advanced-only, closing the rename half of `C1` — all user-confirmed) and **4i** (the verification sweep that completes Phase 4: the shared-entry-word completion merge + the simple-vs-advanced completion colour-when-mixed with Both→Advanced→Simple block ordering; `describe` of table-level composite UNIQUE + table CHECK; the self-ref FK pre-submit indicator fix; and the CREATE-TABLE help/usage skeleton refresh). **All of Phase 4 (4a–4i) is shipped.** Each sub-phase has exit + DA gates diff --git a/docs/plans/20260526-adr-0035-composite-unique-drop-f1f2f3.md b/docs/plans/20260526-adr-0035-composite-unique-drop-f1f2f3.md new file mode 100644 index 0000000..d357e36 --- /dev/null +++ b/docs/plans/20260526-adr-0035-composite-unique-drop-f1f2f3.md @@ -0,0 +1,110 @@ +# Plan — ADR-0035 Phase-4 `/runda` follow-ups F1 / F2 / F3 (2026-05-26) + +Bundle of the three error-message / capability follow-ups surfaced by the +whole-Phase-4 `/runda` (handoff-42 §3). All three live on the **safe** +composite-UNIQUE edge (dropping a UNIQUE-covered column is correctly +*refused* today — no corruption); the work improves messaging and adds a +way to drop the constraint itself. + +## Phase 1 — requirements + +- **F1 — friendly refusal for dropping a composite-UNIQUE column.** + `do_drop_column`'s covering-index guard reads `read_table_indexes`, + which filters to `origin='c'` (explicit `CREATE INDEX`) and excludes + the UNIQUE-constraint auto-index (`origin='u'`). So `drop column c` + when `unique (b, c)` spans `c` skips the guard, reaches the engine, and + is refused with an unhelpful generic message. Add an up-front guard + detecting the column in `schema.unique_constraints` (composite only — + `read_unique_constraints` routes single-column UNIQUEs to the column + flag, multi-column to `unique_constraints`), refusing with the + constraint's **derived name** (F3) + the drop command. Behaviour stays + "refused"; only the message improves. **Message-only — no `--cascade` + extension** (the SQL drop-column has no `--cascade` spelling; dropping + a constraint via cascade is a larger semantic change, out of scope + unless the user asks). + +- **F2 — literal `{table}` leak in contextless `friendly_message()`.** + `Verbosity` defaults to `Verbose`, so `friendly_message()` (which uses + `TranslateContext::default()`, no table) renders the generic hint + `"…current state of `{table}`."` with the literal placeholder via + `ctx_table()`'s `"{table}"` fallback. Hits every contextless + `friendly_message()` callsite whose error lands in the **generic + bucket**: replay, undo, rebuild-from-text, export. Fix: a tableless + generic-hint variant selected when `ctx.table` is `None`. **Broader + finding** (DA): the same `{name}`-marker fallbacks leak in *other* + templates (e.g. a replayed UNIQUE violation → `error.unique.*`) when + reached contextless. The documented F2 is the generic case; the broader + leak is surfaced for the user to scope, not silently expanded/narrowed. + +- **F3 — a way to drop an anonymous composite UNIQUE (user-raised).** + By design (§4a.2/§4g) a composite `UNIQUE(a,b)` is anonymous — + PRAGMA-detected, a bare column-list, no name — so `DROP CONSTRAINT + ` can't target it and recreating the table is the only escape. + Add a way to drop it. **(Amends ADR-0035 — see Amendment 1.)** + +Baseline: `cargo test` → 1917 pass / 0 fail / 0 skip / 1 ignored doctest; +`cargo clippy --all-targets -- -D warnings` clean. + +## Phase 2/3 — F3 design (the genuine fork; user-decided) + +Composite UNIQUE has no name. Options considered: + +- **A — name composite UNIQUEs (user-supplied):** reverse the §4g + anonymity decision; needs a new `__rdbms_*` table + YAML round-trip + + rebuild-arrival migration (the cost §4a.3 deliberately avoided). Most + SQL-standard, largest. +- **B — positional drop by column-list** (`drop … unique (cols)`): + preserves anonymity, no metadata, but needs a *new* grammar form. +- **C — auto-assigned, engine-neutral *derived* name (chosen).** The + name is a deterministic function of the columns (`unique_`), + recomputed live wherever shown or matched. Storage stays a bare + column-list (anonymity preserved); the name is purely a + presentation/addressing label. **Reuses the existing `DROP CONSTRAINT + ` grammar — no new syntax at all.** Zero metadata, zero + migration, round-trips for free. Tracks column renames. + +**User decisions (2026-05-26):** approach **C / derived (no storage)**; +name format **`unique_`**; doc vehicle **amend ADR-0035**; scope +**advanced-SQL only** (matching the 4g `ADD` form — no simple-mode verb). + +### DA critique (written down) + +1. **Ambiguous derived names** (e.g. a column literally named `b_c` vs + `UNIQUE (b, c)`): drop-by-name must **detect ambiguity and refuse**, + never guess. *In scope.* +2. **Collision with a user-named CHECK/FK** of the same string: the + `do_drop_constraint_by_name` order is CHECK → FK → UNIQUE, so a + CHECK/FK shadows a derived UNIQUE name. Acceptable given the + distinctive `unique_` prefix; **document the order**. +3. **F1 `--cascade`**: not extended to drop a covering UNIQUE + (constraint, not index). Refuse-only. *Flagged.* +4. **F2 breadth**: the leak is broader than `error.generic.hint`. Fix the + documented generic case; **surface** the broader leak. *Flagged.* +5. **Single-column UNIQUE column drop**: a parallel gap (a single-column + UNIQUE column drop also reaches the engine with a poor message) exists + but is **outside the documented F1 scope** (different mechanism — + ADR-0029 column-level `drop constraint`). Noted, not fixed here. + +## Phase 4 — execution (order: F3 → F1 → F2) + +1. **F3.** `unique_constraint_name(cols) -> "unique_"` helper + (`db.rs`, `pub(crate)`). Extend `do_drop_constraint_by_name` with a + third step: match each composite UNIQUE's derived name; >1 match → + refuse (ambiguous); 1 match → `rebuild_table` with that entry removed + from `unique_constraints` (mirrors `do_alter_add_unique` in reverse) + + `do_describe_table`. Annotate the describe "Table constraints:" + section: `unique_b_c: UNIQUE (b, c)`. +2. **F1.** Up-front guard in `do_drop_column` after the index-covering + guard: column in any `schema.unique_constraints` entry → refuse with + the derived name + `alter table T drop constraint `. +3. **F2.** `error.generic.hint_no_table` catalog entry; in + `translate_generic`, pick it when `ctx.table` is `None`. + +Test-first for each (reproduce → fail → fix → pass), across the worker +API (Tier-1/3) and the friendly-layer unit tests + insta snapshots. + +## Phase 5 — verification + +Full `cargo test` + clippy; compare to baseline; every checklist item +addressed; engine-neutral vocab held (no SQLite/STRICT/PRAGMA in new +user-facing strings); ADR + README + this plan lockstep. diff --git a/src/db.rs b/src/db.rs index a08abf3..106d67a 100644 --- a/src/db.rs +++ b/src/db.rs @@ -4359,6 +4359,26 @@ fn do_drop_column( ))); } + // A composite UNIQUE covering this column (ADR-0035 Amendment 1): the + // engine refuses to drop a column a UNIQUE constraint spans, so refuse + // up-front with the constraint's derived name and the actionable drop + // command. Single-column UNIQUEs ride on the column `unique` flag (the + // engine drops their auto-index with the column), not + // `unique_constraints`, so they do not reach here. + if let Some(cols) = schema + .unique_constraints + .iter() + .find(|cols| cols.iter().any(|c| c == column)) + { + let cname = unique_constraint_name(cols); + return Err(DbError::Unsupported(format!( + "cannot drop `{table}.{column}` — it is part of the UNIQUE \ + constraint `{cname}` ({}); drop that constraint first \ + (`alter table {table} drop constraint {cname}`).", + cols.join(", "), + ))); + } + // A CHECK (table-level, or a *different* column's column-level CHECK) // that references this column (ADR-0035 §4e, the 4a.3 deferral): a // deliberate up-front refusal — dropping the column would break that @@ -6121,6 +6141,16 @@ fn read_unique_constraints( Ok((single, composite)) } +/// Engine-neutral display/address name for an anonymous composite UNIQUE +/// constraint (ADR-0035 Amendment 1): `unique__…`. A pure +/// function of the column list — recomputed wherever the name is shown +/// (`describe`) or matched (`ALTER TABLE … DROP CONSTRAINT `), so +/// nothing is persisted; the constraint stays a bare column-list in our +/// model and the §4g anonymity decision is intact. +pub(crate) fn unique_constraint_name(cols: &[String]) -> String { + format!("unique_{}", cols.join("_")) +} + /// Generate the CREATE TABLE DDL from a `ReadSchema`. Used during /// the rebuild dance. fn schema_to_ddl(table: &str, schema: &ReadSchema) -> String { @@ -7073,7 +7103,46 @@ fn do_drop_constraint_by_name( ); } - // 3. Not a known named constraint on this table. + // 3. A composite UNIQUE whose derived name (ADR-0035 Amendment 1, + // `unique_`) matches? The constraint is anonymous in our + // model, so we recompute each composite UNIQUE's name and match. + // Order matters: a named CHECK/FK above shadows a derived UNIQUE + // name (the distinctive `unique_` prefix makes a clash unlikely). + let schema = read_schema(conn, table)?; + let matched_cols: Vec> = schema + .unique_constraints + .iter() + .filter(|cols| unique_constraint_name(cols) == name) + .cloned() + .collect(); + if matched_cols.len() > 1 { + // Two distinct UNIQUEs can derive the same name (e.g. a column + // literally named `b_c` vs `UNIQUE (b, c)`). Refuse rather than + // guess which to drop. + return Err(DbError::Unsupported(format!( + "the constraint name `{name}` is ambiguous on `{table}` — it \ + matches more than one UNIQUE constraint; recreate the table \ + to change them." + ))); + } + if let Some(cols) = matched_cols.first() { + let old_schema = schema.clone(); + let mut new_schema = schema; + new_schema.unique_constraints.retain(|c| c != cols); + 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(()) + })?; + return Ok(Some(do_describe_table(conn, table)?)); + } + + // 4. Not a known named constraint on this table. Err(DbError::Sqlite { message: format!("no such constraint: {name} on {table}"), kind: SqliteErrorKind::Other, diff --git a/src/friendly/keys.rs b/src/friendly/keys.rs index a84cc7a..552cd1f 100644 --- a/src/friendly/keys.rs +++ b/src/friendly/keys.rs @@ -109,6 +109,7 @@ pub const KEYS_AND_PLACEHOLDERS: &[(&str, &[&str])] = &[ // ---- Generic engine refusal ---- ("error.generic.headline", &["operation"]), ("error.generic.hint", &["table"]), + ("error.generic.hint_no_table", &[]), // ---- Invalid-value errors (pre-engine, single-line) ---- ( "error.invalid_value.arity.headline", diff --git a/src/friendly/strings/en-US.yaml b/src/friendly/strings/en-US.yaml index 9341b6c..9077dd8 100644 --- a/src/friendly/strings/en-US.yaml +++ b/src/friendly/strings/en-US.yaml @@ -142,6 +142,10 @@ error: generic: headline: "the database refused this `{operation}`." hint: "The operation could not be completed against the current state of `{table}`." + # Used when no table is in context (e.g. contextless `friendly_message()` + # callsites: replay, undo, rebuild, export) so the hint never leaks a + # literal `{table}` placeholder. + hint_no_table: "The operation could not be completed against the current database state." # Errors that are specifically about value validation # (DbError::InvalidValue) — wrong arity, wrong literal diff --git a/src/friendly/translate.rs b/src/friendly/translate.rs index e89dd3b..3659980 100644 --- a/src/friendly/translate.rs +++ b/src/friendly/translate.rs @@ -659,10 +659,17 @@ fn translate_generic(message: &str, ctx: &TranslateContext) -> FriendlyError { let operation = ctx .operation .map_or("operation", Operation::keyword); - let table = ctx_table(ctx); + // F2 (ADR-0035 Amendment 1): when no table is in context, use the + // table-less hint so a contextless `friendly_message()` (replay, undo, + // rebuild, export) never renders a literal `{table}` placeholder. + let hint = if ctx.table.is_some() { + t!("error.generic.hint", table = ctx_table(ctx)) + } else { + t!("error.generic.hint_no_table") + }; fe( t!("error.generic.headline", operation = operation), - verbose_hint(ctx, t!("error.generic.hint", table = table)), + verbose_hint(ctx, hint), ) } @@ -1034,6 +1041,28 @@ mod tests { ); } + #[test] + fn generic_hint_has_no_unsubstituted_table_without_context() { + // F2 (ADR-0035 Amendment 1 plan): `friendly_message()` renders + // with a default, table-less context at the default Verbose + // verbosity, so a generic-bucket error must not leak a literal + // `{table}` in its hint. + let err = sqlite("some unclassified engine failure", SqliteErrorKind::Other); + let rendered = translate(&err, &TranslateContext::default()).render(); + assert!( + !rendered.contains("{table}"), + "no unsubstituted placeholder in the table-less generic hint; got:\n{rendered}" + ); + // The table-ful path is unchanged: a table in context still names it. + let mut ctx = TranslateContext::for_op(Operation::Delete); + ctx.table = Some("Orders".to_string()); + let with_table = translate(&err, &ctx).render(); + assert!( + with_table.contains("Orders"), + "the table-ful generic hint still names the table; got:\n{with_table}" + ); + } + // ---- passthrough variants ---- #[test] diff --git a/src/output_render.rs b/src/output_render.rs index 1c1ae2f..f318604 100644 --- a/src/output_render.rs +++ b/src/output_render.rs @@ -160,7 +160,13 @@ pub fn render_structure(desc: &TableDescription) -> Vec { if !desc.unique_constraints.is_empty() || !desc.check_constraints.is_empty() { out.push("Table constraints:".to_string()); for cols in &desc.unique_constraints { - out.push(format!(" unique ({})", cols.join(", "))); + // Annotate with the derived, addressable name (ADR-0035 + // Amendment 1) so the user can `drop constraint `. + out.push(format!( + " {}: unique ({})", + crate::db::unique_constraint_name(cols), + cols.join(", ") + )); } for chk in &desc.check_constraints { match &chk.name { @@ -880,6 +886,9 @@ mod tests { let out = render_structure(&desc).join("\n"); assert!(out.contains("Table constraints:"), "got:\n{out}"); assert!(out.contains("unique (a, b)"), "got:\n{out}"); + // The composite UNIQUE shows its derived, addressable name + // (ADR-0035 Amendment 1) so the user can `drop constraint `. + assert!(out.contains("unique_a_b: unique (a, b)"), "got:\n{out}"); assert!(out.contains("check (a < b)"), "unnamed check; got:\n{out}"); assert!(out.contains("check a_lt_b (a <> b)"), "named check shows its name; got:\n{out}"); } diff --git a/tests/column_op_guards.rs b/tests/column_op_guards.rs index a752569..8bec3b2 100644 --- a/tests/column_op_guards.rs +++ b/tests/column_op_guards.rs @@ -193,6 +193,56 @@ fn drop_column_referenced_by_a_table_check_is_refused() { .expect("dropping an unreferenced column succeeds"); } +/// `T (id int pk, a int, b int, c text)` with a composite UNIQUE (a, b) +/// (ADR-0035 Amendment 1). +fn make_t_with_composite_unique(db: &Database, r: &tokio::runtime::Runtime) { + r.block_on(db.sql_create_table( + "T".to_string(), + vec![ + ColumnSpec::new("id", Type::Int), + ColumnSpec::new("a", Type::Int), + ColumnSpec::new("b", Type::Int), + ColumnSpec::new("c", Type::Text), + ], + vec!["id".to_string()], + vec![], + vec![], + vec![], + false, + Some("create table T (id int primary key, a int, b int, c text)".to_string()), + )) + .expect("create T"); + r.block_on(db.alter_add_unique( + "T".to_string(), + vec!["a".to_string(), "b".to_string()], + Some("alter table T add unique (a, b)".to_string()), + )) + .expect("add composite UNIQUE (a, b)"); +} + +#[test] +fn drop_column_covered_by_a_composite_unique_is_refused_with_the_derived_name() { + let (_p, db, _d) = open(); + let r = rt(); + make_t_with_composite_unique(&db, &r); + // `a` participates in UNIQUE (a, b) → refused up-front, naming the + // derived constraint and the drop command (ADR-0035 Amendment 1, F1). + // Without this guard the drop reaches the engine and surfaces an + // unhelpful generic refusal. + let err = r + .block_on(db.drop_column("T".to_string(), "a".to_string(), false, None)) + .expect_err("dropping a composite-UNIQUE column is refused"); + let msg = err.friendly_message(); + assert!(msg.contains("unique_a_b"), "names the derived constraint; got: {msg}"); + assert!( + msg.contains("drop constraint unique_a_b"), + "points at the actionable drop command; got: {msg}" + ); + // `c` is in no UNIQUE → the drop succeeds. + r.block_on(db.drop_column("T".to_string(), "c".to_string(), false, None)) + .expect("dropping an uncovered column succeeds"); +} + #[test] fn rename_column_referenced_by_a_table_check_is_refused() { let (_p, db, _d) = open(); diff --git a/tests/sql_alter_table.rs b/tests/sql_alter_table.rs index 7136b22..0cf792f 100644 --- a/tests/sql_alter_table.rs +++ b/tests/sql_alter_table.rs @@ -481,6 +481,138 @@ fn e2e_add_unique_with_duplicate_data_is_refused() { ); } +#[test] +fn e2e_drop_composite_unique_by_derived_name() { + // ADR-0035 Amendment 1: a composite UNIQUE is anonymous, addressed by + // its derived name `unique_`. DROP CONSTRAINT + // removes it via the rebuild primitive and the UNIQUE stops enforcing. + 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\ + 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 == 4), + "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(1, 1, 2), "first (1, 2) accepted"); + assert!(!dup_ok(2, 1, 2), "duplicate (1, 2) rejected while the UNIQUE stands"); + + // Drop the UNIQUE by its derived name through the existing DROP + // CONSTRAINT grammar. + r.block_on(db.alter_drop_constraint( + "T".to_string(), + "unique_a_b".to_string(), + Some("alter table T drop constraint unique_a_b".to_string()), + )) + .expect("drop constraint unique_a_b resolves the composite UNIQUE"); + + // The UNIQUE no longer enforces: the previously-rejected duplicate is + // now accepted. + assert!(dup_ok(3, 1, 2), "duplicate (1, 2) accepted after the UNIQUE was dropped"); + + // And it stays gone across a rebuild from text. + r.block_on(db.rebuild_from_text(project.path().to_path_buf(), Some("rebuild".to_string()))) + .expect("rebuild"); + assert!(dup_ok(4, 1, 2), "still no UNIQUE after rebuild"); +} + +#[test] +fn e2e_drop_composite_unique_ambiguous_name_is_refused() { + // Two distinct composite UNIQUEs can derive the same name — + // `unique (a, b_c)` and `unique (a_b, c)` both → `unique_a_b_c`. The + // drop must refuse as ambiguous, never guess which to drop. + 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_c (int)\n\ + add column T: a_b (int)\n\ + add column T: c (int)\n\ + alter table T add unique (a, b_c)\n\ + alter table T add unique (a_b, c)\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 == 7), + "setup events: {events:?}" + ); + let err = r + .block_on(db.alter_drop_constraint( + "T".to_string(), + "unique_a_b_c".to_string(), + Some("alter table T drop constraint unique_a_b_c".to_string()), + )) + .expect_err("an ambiguous derived name is refused, not guessed"); + let msg = err.friendly_message(); + assert!( + msg.to_lowercase().contains("ambiguous") || msg.to_lowercase().contains("more than one"), + "refusal explains the ambiguity; got: {msg}" + ); +} + +#[test] +fn e2e_drop_composite_unique_is_one_undo_step() { + // Dropping a composite UNIQUE rebuilds the table = one undo step; undo + // restores the constraint (ADR-0035 Amendment 1). The drop is the last + // mutation, so a single undo targets it (checked via describe, so no + // extra mutation shifts the undo target). + let (project, db, _d) = open_with_undo(); + 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\ + alter table T add unique (a, b)\n", + ) + .expect("write"); + r.block_on(run_replay(&db, project.path(), "u.commands")); + let has_unique = || { + !r.block_on(db.describe_table("T".to_string(), None)) + .expect("describe") + .unique_constraints + .is_empty() + }; + assert!(has_unique(), "the composite UNIQUE exists before the drop"); + + r.block_on(db.alter_drop_constraint( + "T".to_string(), + "unique_a_b".to_string(), + Some("alter table T drop constraint unique_a_b".to_string()), + )) + .expect("drop the composite UNIQUE"); + assert!(!has_unique(), "the composite UNIQUE is gone after the drop"); + + assert!( + r.block_on(db.undo()).expect("undo").is_some(), + "the DROP CONSTRAINT was one undo step" + ); + assert!(has_unique(), "one undo restored the composite UNIQUE"); +} + #[test] fn e2e_add_foreign_key_creates_an_enforced_relationship() { let (project, db, _d) = open();