diff --git a/docs/adr/0035-advanced-mode-sql-ddl.md b/docs/adr/0035-advanced-mode-sql-ddl.md index 560dcdb..3099d0c 100644 --- a/docs/adr/0035-advanced-mode-sql-ddl.md +++ b/docs/adr/0035-advanced-mode-sql-ddl.md @@ -3,13 +3,13 @@ ## Status 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** +**validated end-to-end by sub-phases 4a / 4a.2 / 4a.3 / 4b / 4c** (`CREATE TABLE` with column- and table-level constraints and foreign -keys, implemented 2026-05-25 — plans +keys, and `DROP TABLE [IF EXISTS]`, 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`), so the decision is -accepted while the remaining sub-phases (**4c–4i**, §13) continue. This -is **Phase 4** of the ADR-0030 roadmap (the +`docs/plans/20260525-adr-0035-sql-ddl-4b.md`, `…-4c.md`), so the +decision is accepted while the remaining sub-phases (**4d–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. @@ -367,8 +367,19 @@ ADR-0033's structure: takes `CONSTRAINT `. PK-target only (UNIQUE-target deferred with `add relationship`); `Type::fk_target_type` (ADR-0011) governs type compatibility. -- **4c — `DROP TABLE [IF EXISTS]`** → `SqlDropTable` (cascade parity; - `IF EXISTS` no-op-with-note, §4). +- **4c — `DROP TABLE [IF EXISTS]`** → `SqlDropTable`. *(Implemented + 2026-05-25 — plan `docs/plans/20260525-adr-0035-sql-ddl-4c.md`.)* + Reuses `do_drop_table` (cascade parity + the inbound-relationship + refusal + metadata cleanup), so it matches the simple `drop table`; + `IF EXISTS` on an absent table is a no-op-with-note (a new + `DropOutcome::Skipped` mirroring `CreateOutcome::Skipped`; journalled, + no snapshot, §4). `drop` is a shared entry word: `drop table` parses + as `SqlDropTable` in advanced mode, `drop column`/`relationship`/ + `index`/`constraint` fall back to the simple `drop` node. Advanced- + mode `drop ` completion now surfaces the SQL `table` (the + shared-entry-word behaviour from `create`, ADR-0033 Amendment 3); the + DSL drops still parse via fallback — 4i grows the surface as `DROP + INDEX` lands in 4d. - **4d — `CREATE [UNIQUE] INDEX` / `DROP INDEX`** → `SqlCreateIndex` / `SqlDropIndex` (ADR-0025; the `UNIQUE` flag extension if needed). - **4e — `ALTER TABLE` add/drop/rename column.** Drop/rename column @@ -394,7 +405,21 @@ ADR-0033's structure: schema-existence diagnostic falsely flags the not-yet-created self table as unknown (the FK parent slot is `IdentSource::Tables`). Make the diagnostic treat a FK parent equal to the `CREATE TABLE` target as - valid, so the indicator stops lying for self-references. + valid, so the indicator stops lying for self-references. (d) **4c + shared-entry-word completion merge** — in advanced mode a shared entry + word surfaces only the SQL node's continuations, so `drop ` offers + only `table` (not the DSL `column`/`relationship`/`index`/`constraint`) + and a partial keyword like `drop rel` returns an *empty* list (a + mid-word dead end), even though the DSL drops still parse + execute via + fallback. Merge the expected sets of all candidate nodes for a shared + entry word so advanced completion offers every valid continuation + (`drop ` → table + column + relationship + index + constraint; `drop + rel` → relationship); verify `create`/`insert`/`update`/`delete` + completion stays sensible. (e) **Discussion flag (user, 2026-05-25):** + before/with (d), discuss **visually distinguishing simple- vs + advanced-mode completions in the hint UI (likely by colour)** so a + learner can see which continuations are DSL and which are SQL — a UX + design conversation, not just the mechanical merge. ## Consequences diff --git a/docs/adr/README.md b/docs/adr/README.md index 6644696..4655683 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), implemented 2026-05-25; 4c–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); 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]`, implemented 2026-05-25; 4d–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`); 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-4c.md b/docs/plans/20260525-adr-0035-sql-ddl-4c.md new file mode 100644 index 0000000..803e6b1 --- /dev/null +++ b/docs/plans/20260525-adr-0035-sql-ddl-4c.md @@ -0,0 +1,158 @@ +# Plan: ADR-0035 Phase 4, sub-phase 4c — `DROP TABLE [IF EXISTS]` + +Add advanced-mode SQL `DROP TABLE [IF EXISTS] ` → `SqlDropTable`. +Reuses the existing `do_drop_table` (cascade/inbound-refusal + +metadata cleanup), so it has full parity with the simple `drop table`; +the only new behaviour is `IF EXISTS` as a **no-op-that-succeeds-with-a- +note**, mirroring 4a's `CreateOutcome::Skipped` with a `DropOutcome` +(ADR-0035 §4/§13). Small slice. + +## 1. Baseline + +- Tests: **1795 passing, 0 failing, 1 ignored**; clippy clean. Branch + `main`, last commit `76d6059` (4b). 4c starts here. + +## 2. Decisions (from ADR §4/§13 + the 4a precedent — not re-litigated) + +1. **`IF EXISTS` → no-op-with-note** (universal cross-vendor idiom, + already in scope per the 4a `IF [NOT] EXISTS` decision): dropping an + absent table succeeds with a note instead of the plain "no such + table" error. Mirrors `CreateOutcome::Skipped` via a new + `DropOutcome { Dropped, Skipped }`. The skip is **journalled** (no + snapshot), exactly as the create-skip is. +2. **Table only.** SQL `DROP TABLE` drops a table; `DROP INDEX` is 4c's + sibling 4d (`SqlDropIndex`); column/constraint drops are `ALTER TABLE` + (4e+). `drop column`/`drop relationship`/`drop index` keep falling + back to the simple `drop` node. +3. **Dispatch (ADR-0033 Amendment 1).** `drop` is a shared entry word; + the SQL node is tried first in advanced mode. `drop table T` parses + as `SqlDropTable` in advanced mode (equivalent execution to the + simple `DropTable` — both call `do_drop_table`); the simple form is + the only one in simple mode. +4. **Cascade / inbound-relationship refusal parity** — reuse + `do_drop_table` unchanged (it already refuses dropping a table with + inbound relationships and cleans `__rdbms_*` metadata, incl. the 4a.3 + `table_checks` rows). One undo step (`snapshot_then`). + +## 3. Phase 1 — Requirements checklist (4c) + +- [ ] `DROP TABLE ` parses in advanced mode → `SqlDropTable`. +- [ ] `DROP TABLE IF EXISTS ` sets the `if_exists` flag. +- [ ] Dropping an existing table removes it + its metadata (parity with + simple `drop table`); one undo step; `undo` restores it. +- [ ] `IF EXISTS` on an **absent** table → success with a note, no + error, no snapshot; the line is journalled. +- [ ] Plain `DROP TABLE` on an absent table → the existing "no such + table" error (unchanged). +- [ ] Dropping a table with **inbound relationships** is refused + (existing `do_drop_table` semantics), in advanced mode too. +- [ ] `drop column` / `drop relationship` / `drop index` still parse as + the simple forms in advanced mode (fallback). +- [ ] Engine-neutral note wording. + +### Testing + +- [ ] **Tier 1** (builder): `drop table ` → `SqlDropTable{if_exists: + false}`; `drop table if exists ` → `if_exists: true`; the simple + `drop column`/`drop relationship` forms still parse (fallback). +- [ ] **Tier 3** (`tests/sql_drop_table.rs`): drop existing → gone + + one undo step + undo restores; `IF EXISTS` absent → skipped + note + + journalled; plain absent → error; inbound-relationship refusal. +- [ ] **Catalog** lockstep + vocab audit for the new note key. + +## 4. Architecture & design + +### 4.1 Grammar (`src/dsl/grammar/ddl.rs`) +- `IF_EXISTS_OPT` = `Optional(Seq[Word("if"), Word("exists")])`. +- `SQL_DROP_TABLE_SHAPE` = `Seq[Word("table"), IF_EXISTS_OPT, + TABLE_NAME_EXISTING, Optional(Punct(';'))]` (mirrors the simple + `DROP_TABLE` shape + the optional `IF EXISTS` + trailing `;`). +- `pub static SQL_DROP_TABLE: CommandNode { entry: "drop", shape, + ast_builder: build_sql_drop_table, help_id: "ddl.sql_drop_table", + usage_ids: ["parse.usage.sql_drop_table"] }`. +- `build_sql_drop_table`: `name = require_ident("table_name")`, + `if_exists = path.contains_word("if")` (the `if` only appears in the + `IF EXISTS` prefix; mirror the create builder's `if_not_exists`). + +### 4.2 Command (`src/dsl/command.rs`) +`SqlDropTable { name: String, if_exists: bool }`. Verb label +`"drop table"`; `target_table()` → `name` (add to the existing match +arms alongside `DropTable`). + +### 4.3 Worker (`src/db.rs`) +- `DropOutcome { Dropped, Skipped }` (the drop peer of `CreateOutcome`; + `Skipped` needs no payload — the runtime renders the note from the + command's name). +- `Request::SqlDropTable { name, if_exists, source, reply: + oneshot> }` + `db.sql_drop_table` method. +- Dispatch arm: if `if_exists && !user_table_exists(name)` → journal the + line (no snapshot) and reply `Skipped` (mirror the create-skip arm); + else `snapshot_then(… do_drop_table(name) … → Dropped)`. +- `do_drop_table` unchanged. + +### 4.4 Runtime + event + app (`src/runtime.rs`, `src/event.rs`, `src/app.rs`) +- `Command::SqlDropTable` → `db.sql_drop_table` → map `Dropped` → + `CommandOutcome::Schema(None)` (same as simple drop), `Skipped` → a new + `CommandOutcome::SchemaDropSkipped`. +- `CommandOutcome::SchemaDropSkipped` → `AppEvent::DslDropSkipped { + command }`; the app handler notes `ddl.drop_skipped_absent` with + `command.target_table()` (mirror `DslCreateSkipped`). No structure to + render (the table is gone / never existed). + +### 4.5 Friendly catalog +- New key `ddl.drop_skipped_absent: "table '{name}' doesn't exist — + skipped (no changes made)"` + a `keys.rs` `("ddl.drop_skipped_absent", + &["name"])` entry. Engine-neutral. Help/usage skeleton body for + `ddl.sql_drop_table` + `parse.usage.sql_drop_table` (a fresh node — its + keys must exist; unlike the deferred *refresh* of the create skeleton, + these are new keys the node references, so they land now). + +## 5. Out of 4c scope +- `DROP INDEX` (4d → `SqlDropIndex`); `ALTER TABLE` (4e–4h). +- **Shared-entry-word completion (deferred to 4i, user-confirmed + 2026-05-25).** Adding the SQL `drop` node makes advanced-mode `drop ` + completion surface only `table`; a partial DSL subcommand keyword + (`drop rel`) returns an empty list (mid-word dead end). The DSL drops + still parse + execute via fallback — only the completion hint is + affected. This is the pre-existing shared-entry-word model (same as + `create`/`insert`/`update`/`delete`), exposed here because `drop` has + five distinct DSL subcommands with no SQL equivalent. 4i merges the + candidate sets for shared entry words; **the user also wants to + discuss visually distinguishing simple- vs advanced-mode completions + in the hint UI (likely by colour)** before/with that work. Tracked in + ADR-0035 §13 4i (d)/(e) and the advanced-mode completion test in + `src/completion.rs`. + +## 6. Devil's Advocate review of this plan +- **Reuse vs fork?** `do_drop_table` is the single executor; the SQL + path differs only at the `IF EXISTS` branch (mirrors the create-skip). + ✓ +- **Dispatch fallback?** A builder test asserts `drop column`/`drop + relationship` still parse simple in advanced mode. ✓ +- **One undo step + restore?** `snapshot_then` wrap + a dedicated undo + test (drop → undo → table back, with its data/metadata). ✓ +- **No-op journalled, not snapshotted?** Mirrors the create-skip arm + exactly; a journalling test. ✓ +- **Engine-neutral?** The note is a catalog key under the vocab audit; + the plain-absent error is the existing `do_drop_table` path (unchanged, + already engine-neutral). ✓ + +## 7. Implementation sequence (test-first) +1. **Command + grammar + builder** — Tier-1 builder tests (parse + + `if_exists` + simple fallback) → red → add `SqlDropTable`, the grammar + node + shape + builder, REGISTRY entry, thread `Request`/method/ + dispatch/runtime (drop execution can land here too) → green. +2. **Worker no-op-with-note** — Tier-3 (`tests/sql_drop_table.rs`): drop + existing, `IF EXISTS` skip + journal, plain-absent error, inbound + refusal, one undo step + restore → red → add `DropOutcome` + the + dispatch arm + the `SchemaDropSkipped`/`DslDropSkipped`/app-note path + → green. +3. **Catalog** — add the note + help/usage keys + bodies; lockstep + + vocab audit → green. +4. **Full sweep** — `cargo test` (no regression from 1795) + clippy. +5. **Docs** — ADR-0035 Status/§13 4c; README; `requirements.md` Q1. + Propose commit; wait for approval. + +## 8. Exit gate +- All §3 items satisfied; four tiers green, zero skips; no regression + from the 1795 baseline; written DA pass; clippy clean. diff --git a/docs/requirements.md b/docs/requirements.md index 9e394b7..3d31e53 100644 --- a/docs/requirements.md +++ b/docs/requirements.md @@ -223,8 +223,9 @@ handoff-14 cleanup; 449 after B2/C2.) table, since the engine reports no CHECK constraints), then foreign keys (4b — inline `REFERENCES` + table-level `FOREIGN KEY` → ADR-0013 named relationships in the create transaction; self-references and - bare `REFERENCES ` supported)). Remaining DDL — `DROP TABLE` - (4c), indexes (4d), `ALTER TABLE` (4e–4h) — is phased per + bare `REFERENCES ` supported), then `DROP TABLE [IF EXISTS]` + (4c — reuses `do_drop_table`; `IF EXISTS` is a no-op-with-note)). + Remaining DDL — indexes (4d), `ALTER TABLE` (4e–4h) — is phased per ADR-0035 §13.)* - [ ] **Q2** Non-standard syntax rejected with a clear message pointing at the supported subset. diff --git a/src/app.rs b/src/app.rs index ca86387..a20adaf 100644 --- a/src/app.rs +++ b/src/app.rs @@ -459,6 +459,16 @@ impl App { self.current_table = Some(description); Vec::new() } + AppEvent::DslDropSkipped { command } => { + // No-op (DROP TABLE IF EXISTS on an absent table, + // ADR-0035 §4, 4c): just the skip note — no structure, + // no misleading "[ok] drop table" line. + self.note_system(crate::t!( + "ddl.drop_skipped_absent", + name = command.target_table() + )); + Vec::new() + } AppEvent::DslDataSucceeded { command, data } => { self.handle_dsl_query_success(&command, &data); Vec::new() @@ -1554,6 +1564,7 @@ impl App { (Operation::CreateTable, Some(name.as_str()), None) } C::DropTable { name } => (Operation::DropTable, Some(name.as_str()), None), + C::SqlDropTable { name, .. } => (Operation::DropTable, Some(name.as_str()), None), C::AddColumn { table, column, .. } => ( Operation::AddColumn, Some(table.as_str()), diff --git a/src/completion.rs b/src/completion.rs index 54c9345..9ee3664 100644 --- a/src/completion.rs +++ b/src/completion.rs @@ -1458,12 +1458,14 @@ mod tests { } #[test] - fn drop_offers_all_five_subcommands() { - // `drop` branches: column / relationship / table / index - // (ADR-0025) / constraint (ADR-0029 §2.2). Candidates - // follow grammar declaration order, so `constraint` — - // added last — appears last. - let cs = cands("drop ", 5); + fn drop_offers_all_five_subcommands_in_simple_mode() { + // The DSL `drop` branches: column / relationship / table / index + // (ADR-0025) / constraint (ADR-0029 §2.2). Candidates follow + // grammar declaration order, so `constraint` — added last — + // appears last. Simple mode, because `drop` is a shared entry + // word: advanced mode surfaces the SQL `DROP TABLE` completion + // instead (ADR-0033 Amendment 3 / ADR-0035 §4c — see below). + let cs = cands_simple("drop ", 5); assert_eq!( cs, vec![ @@ -1476,6 +1478,23 @@ mod tests { ); } + #[test] + fn drop_in_advanced_mode_surfaces_the_sql_drop_table_completion() { + // ADR-0035 §4c: `drop` gained an advanced SQL node + // (`DROP TABLE [IF EXISTS]`). As with the `create`/`insert`/ + // `update`/`delete` shared entry words (ADR-0033 Amendment 3), + // advanced mode surfaces the SQL grammar's completion — here + // just `table` — rather than the DSL subcommands. The DSL drops + // (`drop column` etc.) still parse via fallback; only the + // completion hint differs (and a partial DSL keyword like + // `drop rel` returns an empty list — a mid-word dead end). + // ADR-0035 §13 4i (d)/(e) tracks merging the candidate sets for + // shared entry words, and the user's request to visually + // distinguish simple- vs advanced-mode completions in the hint + // UI (likely by colour); this expectation grows when 4i lands. + assert_eq!(cands("drop ", 5), vec!["table".to_string()]); + } + #[test] fn complete_command_offers_no_candidates() { // `create table T with pk` is a complete command — @@ -2444,3 +2463,5 @@ mod tests { assert!(candidates_at_cursor_with("create ", 7, &cache, empty_ranker).is_none()); } } + + diff --git a/src/db.rs b/src/db.rs index 0609eb5..8dbc814 100644 --- a/src/db.rs +++ b/src/db.rs @@ -477,6 +477,15 @@ enum Request { source: Option, reply: oneshot::Sender>, }, + /// Advanced-mode SQL `DROP TABLE [IF EXISTS]` (ADR-0035 §4, 4c). + /// Executes through `do_drop_table`; `if_exists` turns an absent + /// table into a no-op (`DropOutcome::Skipped`, no snapshot). + SqlDropTable { + name: String, + if_exists: bool, + source: Option, + reply: oneshot::Sender>, + }, AddColumn { table: String, column: ColumnSpec, @@ -865,6 +874,26 @@ impl Database { recv.await.map_err(|_| DbError::WorkerGone)? } + /// Advanced-mode SQL `DROP TABLE [IF EXISTS]` (ADR-0035 §4, 4c). + /// Returns whether the table was dropped or skipped (the `IF EXISTS` + /// no-op on an absent table). + pub async fn sql_drop_table( + &self, + name: String, + if_exists: bool, + source: Option, + ) -> Result { + let (reply, recv) = oneshot::channel(); + self.send(Request::SqlDropTable { + name, + if_exists, + source, + reply, + }) + .await?; + recv.await.map_err(|_| DbError::WorkerGone)? + } + pub async fn add_column( &self, table: String, @@ -1765,6 +1794,31 @@ fn handle_request( do_drop_table(conn, persistence, source.as_deref(), &name) }); } + Request::SqlDropTable { + name, + if_exists, + source, + reply, + } => { + // `IF EXISTS` on an absent table is a no-op: reply `Skipped` + // and take **no** snapshot (nothing to undo). The submitted + // line is still journalled — like the `CREATE TABLE IF NOT + // EXISTS` skip and other no-ops (ADR-0034). ADR-0035 §4. + if if_exists && !user_table_exists(conn, &name).unwrap_or(false) { + let result = (|| { + if let (Some(p), Some(text)) = (persistence, source.as_deref()) { + p.append_history(text).map_err(DbError::from_persistence)?; + } + Ok(DropOutcome::Skipped) + })(); + let _ = reply.send(result); + } else { + snapshot_then(snap, batch, conn, source.as_deref(), reply, || { + do_drop_table(conn, persistence, source.as_deref(), &name) + .map(|()| DropOutcome::Dropped) + }); + } + } Request::AddColumn { table, column, @@ -2633,6 +2687,18 @@ pub enum CreateOutcome { Skipped(TableDescription), } +/// The result of an advanced-mode SQL `DROP TABLE` (ADR-0035 §4, 4c). +/// +/// Either the table was dropped, or `IF EXISTS` matched no table and +/// the statement was a no-op that drives the "doesn't exist — skipped" +/// note. Carries no payload — the runtime renders the note from the +/// command's table name. +#[derive(Debug)] +pub enum DropOutcome { + Dropped, + Skipped, +} + #[allow(clippy::too_many_arguments)] fn do_create_table( conn: &Connection, diff --git a/src/dsl/command.rs b/src/dsl/command.rs index ddd5dbc..b56df31 100644 --- a/src/dsl/command.rs +++ b/src/dsl/command.rs @@ -160,6 +160,15 @@ pub enum Command { DropTable { name: String, }, + /// Advanced-mode SQL `DROP TABLE [IF EXISTS] ` (ADR-0035 §4, + /// sub-phase 4c). Executes through the same `do_drop_table` + /// machinery as [`Self::DropTable`] (cascade / inbound-relationship + /// refusal / metadata cleanup); `if_exists` turns an absent table + /// into a no-op-with-note rather than an error. + SqlDropTable { + name: String, + if_exists: bool, + }, /// Advanced-mode SQL `CREATE TABLE` (ADR-0035 §1, sub-phase 4a). /// Its own command, but executed **structurally** through the /// same `do_create_table` machinery as [`Self::CreateTable`] — @@ -692,6 +701,7 @@ impl Command { Self::CreateTable { .. } => "create table", Self::SqlCreateTable { .. } => "create table", Self::DropTable { .. } => "drop table", + Self::SqlDropTable { .. } => "drop table", Self::AddColumn { .. } => "add column", Self::DropColumn { .. } => "drop column", Self::RenameColumn { .. } => "rename column", @@ -741,6 +751,7 @@ impl Command { Self::CreateTable { name, .. } | Self::SqlCreateTable { name, .. } | Self::DropTable { name } + | Self::SqlDropTable { name, .. } | Self::ShowTable { name } | Self::ShowData { name, .. } => name, Self::AddColumn { table, .. } diff --git a/src/dsl/grammar/ddl.rs b/src/dsl/grammar/ddl.rs index 4a8129e..4f84398 100644 --- a/src/dsl/grammar/ddl.rs +++ b/src/dsl/grammar/ddl.rs @@ -187,6 +187,22 @@ const DROP_TABLE_NODES: &[Node] = &[ ]; const DROP_TABLE: Node = Node::Seq(DROP_TABLE_NODES); +// Advanced-mode SQL `DROP TABLE [IF EXISTS] [;]` (ADR-0035 §4, +// sub-phase 4c). Same table-only target as the simple `drop table`, +// plus the optional `IF EXISTS` no-op-with-note. The leading concrete +// `table` keyword (not the Optional) keeps the element/dispatch +// matching honest. +static SQL_DROP_IF_EXISTS_NODES: &[Node] = + &[Node::Word(Word::keyword("if")), Node::Word(Word::keyword("exists"))]; +const SQL_DROP_IF_EXISTS_OPT: Node = Node::Optional(&Node::Seq(SQL_DROP_IF_EXISTS_NODES)); +static SQL_DROP_TABLE_SHAPE_NODES: &[Node] = &[ + Node::Word(Word::keyword("table")), + SQL_DROP_IF_EXISTS_OPT, + TABLE_NAME_EXISTING, + Node::Optional(&Node::Punct(';')), +]; +const SQL_DROP_TABLE_SHAPE: Node = Node::Seq(SQL_DROP_TABLE_SHAPE_NODES); + // ================================================================= // drop_column — `drop column [from] [table] : ` // ================================================================= @@ -1691,6 +1707,25 @@ pub static SQL_CREATE_TABLE: CommandNode = CommandNode { usage_ids: &["parse.usage.sql_create_table"], }; +/// Build a `Command::SqlDropTable` from the advanced-mode SQL +/// `DROP TABLE [IF EXISTS] ` shape (ADR-0035 §4, sub-phase 4c). +/// `if` appears only in the `IF EXISTS` prefix, so its presence is the +/// flag (mirroring `build_sql_create_table`'s `if_not_exists`). +fn build_sql_drop_table(path: &MatchedPath, _source: &str) -> Result { + Ok(Command::SqlDropTable { + name: require_ident(path, "table_name")?, + if_exists: path.contains_word("if"), + }) +} + +pub static SQL_DROP_TABLE: CommandNode = CommandNode { + entry: Word::keyword("drop"), + shape: SQL_DROP_TABLE_SHAPE, + ast_builder: build_sql_drop_table, + help_id: Some("ddl.sql_drop_table"), + usage_ids: &["parse.usage.sql_drop_table"], +}; + // ================================================================= // Tests — `create table` column constraints (ADR-0029 §2.1, §9) // ================================================================= @@ -1900,3 +1935,62 @@ mod constraint_tests { } } } + +// ================================================================= +// Tests — advanced-mode SQL `DROP TABLE [IF EXISTS]` (ADR-0035 §4, 4c) +// ================================================================= + +#[cfg(test)] +mod sql_drop_table_tests { + use crate::dsl::command::Command; + use crate::dsl::parser::parse_command_in_mode; + use crate::mode::Mode; + + fn drop_fields(input: &str) -> (String, bool) { + match parse_command_in_mode(input, Mode::Advanced).expect("should parse") { + Command::SqlDropTable { name, if_exists } => (name, if_exists), + other => panic!("expected SqlDropTable, got {other:?}"), + } + } + + #[test] + fn drop_table_parses_as_sql_drop_table_in_advanced_mode() { + let (name, if_exists) = drop_fields("drop table Orders"); + assert_eq!(name, "Orders"); + assert!(!if_exists); + } + + #[test] + fn if_exists_sets_the_flag() { + let (name, if_exists) = drop_fields("drop table if exists Orders"); + assert_eq!(name, "Orders"); + assert!(if_exists); + // trailing semicolon tolerated + assert!(drop_fields("drop table if exists Orders;").1); + } + + #[test] + fn simple_drop_table_in_simple_mode_is_the_dsl_command() { + // In simple mode the SQL node is gated; `drop table T` is the + // simple `DropTable` (which has no `if_exists`). + match parse_command_in_mode("drop table Orders", Mode::Simple).expect("parses") { + Command::DropTable { name } => assert_eq!(name, "Orders"), + other => panic!("expected DropTable, got {other:?}"), + } + } + + #[test] + fn other_drops_fall_back_to_the_simple_node_in_advanced_mode() { + // `drop column` / `drop relationship` are not SQL DROP TABLE — + // they fall through to the simple `drop` node even in advanced. + assert!(matches!( + parse_command_in_mode("drop column from Orders: note", Mode::Advanced).expect("parses"), + Command::DropColumn { .. } + )); + assert!(matches!( + parse_command_in_mode("drop relationship Customers_id_to_Orders_CustId", Mode::Advanced) + .expect("parses"), + Command::DropRelationship { .. } + )); + } +} diff --git a/src/dsl/grammar/mod.rs b/src/dsl/grammar/mod.rs index 12e4ae9..312ade4 100644 --- a/src/dsl/grammar/mod.rs +++ b/src/dsl/grammar/mod.rs @@ -590,6 +590,11 @@ pub static REGISTRY: &[(&CommandNode, CommandCategory)] = &[ // the `create table … with pk …` DSL node when the SQL shape // does not match — the `insert` precedent. (&ddl::SQL_CREATE_TABLE, CommandCategory::Advanced), + // Shared `drop` entry word: `ddl::DROP` (simple) and this advanced + // SQL node. SQL-first in advanced mode; `drop table [if exists] T` + // matches here while `drop column`/`drop relationship`/`drop index` + // fall back to the simple `drop` node. + (&ddl::SQL_DROP_TABLE, CommandCategory::Advanced), ]; /// Whether `entry` names an advanced-mode-only command (ADR-0030 diff --git a/src/event.rs b/src/event.rs index 727590f..9456783 100644 --- a/src/event.rs +++ b/src/event.rs @@ -35,6 +35,12 @@ pub enum AppEvent { command: Command, description: TableDescription, }, + /// A SQL `DROP TABLE IF EXISTS` matched no table — a no-op + /// (ADR-0035 §4, 4c). Renders a "doesn't exist — skipped" note; + /// there is no structure to show. + DslDropSkipped { + command: Command, + }, /// A `show data` query succeeded. DslDataSucceeded { command: Command, data: DataResult }, /// An `explain …` command succeeded (ADR-0028). `plan` diff --git a/src/friendly/keys.rs b/src/friendly/keys.rs index 272c81c..1d96dc1 100644 --- a/src/friendly/keys.rs +++ b/src/friendly/keys.rs @@ -172,8 +172,10 @@ pub const KEYS_AND_PLACEHOLDERS: &[(&str, &[&str])] = &[ ("help.app.redo", &[]), ("help.ddl.create", &[]), ("help.ddl.sql_create_table", &[]), - // Advanced-mode SQL CREATE TABLE no-op note (ADR-0035 §4). + ("help.ddl.sql_drop_table", &[]), + // Advanced-mode SQL CREATE TABLE / DROP TABLE no-op notes (ADR-0035 §4). ("ddl.create_skipped_exists", &["name"]), + ("ddl.drop_skipped_absent", &["name"]), ("help.ddl.drop", &[]), ("help.ddl.add", &[]), ("help.ddl.rename", &[]), @@ -245,6 +247,7 @@ pub const KEYS_AND_PLACEHOLDERS: &[(&str, &[&str])] = &[ ("parse.usage.change_column", &[]), ("parse.usage.create_table", &[]), ("parse.usage.sql_create_table", &[]), + ("parse.usage.sql_drop_table", &[]), ("parse.usage.delete", &[]), ("parse.usage.drop_column", &[]), ("parse.usage.drop_constraint", &[]), diff --git a/src/friendly/strings/en-US.yaml b/src/friendly/strings/en-US.yaml index a6255a3..ffeb989 100644 --- a/src/friendly/strings/en-US.yaml +++ b/src/friendly/strings/en-US.yaml @@ -263,6 +263,8 @@ help: sql_create_table: |- create table [if not exists] ( [not null] [unique] [primary key], ... [, primary key (, ...)]) — create a table (advanced SQL) + sql_drop_table: |- + drop table [if exists] — remove a table (advanced SQL) drop: |- drop table — remove a table drop column [from] [table] : [--cascade] — remove a column @@ -377,6 +379,9 @@ ddl: # present: a no-op that succeeds with this note instead of an # "already exists" error. create_skipped_exists: "table '{name}' already exists — skipped (no changes made)" + # `drop table if exists ` where the table is absent: a no-op that + # succeeds with this note instead of a "doesn't exist" error. + drop_skipped_absent: "table '{name}' doesn't exist — skipped (no changes made)" parse: # Wrapper around chumsky's structural error message. The @@ -446,6 +451,7 @@ parse: usage: create_table: "create table with pk [()[, ...]]" sql_create_table: "create table [if not exists] ( [not null] [unique] [primary key], ... [, primary key (, ...)])" + sql_drop_table: "drop table [if exists] " drop_table: "drop table " drop_column: "drop column [from] [table] : " drop_relationship: |- diff --git a/src/runtime.rs b/src/runtime.rs index 8faae5d..c7aab01 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -30,7 +30,8 @@ use crate::app::App; use crate::cli::Args; use crate::db::{ AddColumnResult, ChangeColumnTypeResult, CreateOutcome, DataResult, Database, DbError, - DeleteResult, DropColumnResult, InsertResult, QueryPlan, TableDescription, UpdateResult, + DeleteResult, DropColumnResult, DropOutcome, InsertResult, QueryPlan, TableDescription, + UpdateResult, }; use crate::dsl::{Command, ColumnSpec}; use crate::dsl::walker::Severity; @@ -1257,6 +1258,9 @@ fn spawn_dsl_dispatch( command: command.clone(), description, }, + Ok(CommandOutcome::SchemaDropSkipped) => AppEvent::DslDropSkipped { + command: command.clone(), + }, Ok(CommandOutcome::Query(data)) => AppEvent::DslDataSucceeded { command: command.clone(), data, @@ -1654,6 +1658,10 @@ enum CommandOutcome { /// so the App can render it alongside the "already exists — /// skipped" note. SchemaSkipped(TableDescription), + /// A SQL `DROP TABLE IF EXISTS` that matched no table — a no-op + /// (ADR-0035 §4, 4c). Carries no structure (there is none); the App + /// renders the "doesn't exist — skipped" note from the command. + SchemaDropSkipped, Query(DataResult), QueryPlan(QueryPlan), Insert(InsertResult), @@ -1948,6 +1956,13 @@ async fn execute_command_typed( .drop_table(name, src) .await .map(|()| CommandOutcome::Schema(None)), + Command::SqlDropTable { name, if_exists } => database + .sql_drop_table(name, if_exists, src) + .await + .map(|outcome| match outcome { + DropOutcome::Dropped => CommandOutcome::Schema(None), + DropOutcome::Skipped => CommandOutcome::SchemaDropSkipped, + }), Command::AddColumn { table, column, diff --git a/tests/sql_drop_table.rs b/tests/sql_drop_table.rs new file mode 100644 index 0000000..2c7f578 --- /dev/null +++ b/tests/sql_drop_table.rs @@ -0,0 +1,155 @@ +//! Sub-phase 4c integration tests for advanced-mode SQL +//! `DROP TABLE [IF EXISTS]` (ADR-0035 §4). +//! +//! `SqlDropTable` executes through the same `do_drop_table` machinery +//! as the simple `drop table` (cascade / inbound-relationship refusal / +//! metadata cleanup); the only new behaviour is `IF EXISTS` as a +//! no-op-with-note (`DropOutcome::Skipped`). These drive the worker +//! directly; parsing (text → `Command::SqlDropTable`) is covered by the +//! `sql_drop_table_tests` in `src/dsl/grammar/ddl.rs`. + +use rdbms_playground::db::{Database, DropOutcome}; +use rdbms_playground::dsl::{ColumnSpec, SqlForeignKey, Type, Value}; +use rdbms_playground::persistence::Persistence; +use rdbms_playground::project; + +fn rt() -> tokio::runtime::Runtime { + tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("tokio rt") +} + +fn open(undo: bool) -> (project::Project, Database, tempfile::TempDir) { + let dir = tempfile::tempdir().expect("create tempdir"); + let project = + project::open_or_create(None, Some(dir.path())).expect("open or create project"); + let persistence = Persistence::new(project.path().to_path_buf()); + let db = Database::open_with_persistence_and_undo(project.db_path(), persistence, undo) + .expect("open db with persistence"); + (project, db, dir) +} + +/// Create a simple `T (id int primary key, body text)`. +fn make_t(db: &Database, r: &tokio::runtime::Runtime) { + r.block_on(db.sql_create_table( + "T".to_string(), + vec![ColumnSpec::new("id", Type::Int), ColumnSpec::new("body", Type::Text)], + vec!["id".to_string()], + vec![], + vec![], + vec![], + false, + Some("create table T (id int primary key, body text)".to_string()), + )) + .expect("create T"); +} + +#[test] +fn drop_table_removes_an_existing_table() { + let (_p, db, _d) = open(false); + let r = rt(); + make_t(&db, &r); + let out = r + .block_on(db.sql_drop_table("T".to_string(), false, Some("drop table T".to_string()))) + .expect("drop"); + assert!(matches!(out, DropOutcome::Dropped)); + assert!(!r.block_on(db.list_tables()).unwrap().contains(&"T".to_string())); +} + +#[test] +fn if_exists_on_an_absent_table_is_a_noop_and_journalled() { + let (p, db, _d) = open(false); + let r = rt(); + let line = "drop table if exists Ghost"; + let out = r + .block_on(db.sql_drop_table("Ghost".to_string(), true, Some(line.to_string()))) + .expect("IF EXISTS on an absent table succeeds as a no-op"); + assert!(matches!(out, DropOutcome::Skipped)); + // The no-op is still journalled (ADR-0034), like the create-skip. + let log = std::fs::read_to_string(p.path().join("history.log")).expect("read history.log"); + assert!(log.contains(line), "the skipped drop should be journalled; log:\n{log}"); +} + +#[test] +fn plain_drop_of_an_absent_table_errors() { + let (_p, db, _d) = open(false); + let r = rt(); + let res = r.block_on(db.sql_drop_table("Ghost".to_string(), false, Some("drop table Ghost".to_string()))); + assert!(res.is_err(), "plain DROP TABLE on an absent table errors (no IF EXISTS)"); +} + +#[test] +fn dropping_a_referenced_parent_is_refused() { + // Parity with `do_drop_table`: a table with inbound relationships + // can't be dropped (ADR-0013), via the SQL path too. + let (_p, db, _d) = open(false); + let r = rt(); + r.block_on(db.sql_create_table( + "parent".to_string(), + vec![ColumnSpec::new("id", Type::Serial), ColumnSpec::new("label", Type::Text)], + vec!["id".to_string()], + vec![], + vec![], + vec![], + false, + Some("create table parent (id serial primary key, label text)".to_string()), + )) + .expect("create parent"); + r.block_on(db.sql_create_table( + "child".to_string(), + vec![ColumnSpec::new("id", Type::Serial), ColumnSpec::new("pid", Type::Int)], + vec!["id".to_string()], + vec![], + vec![], + vec![SqlForeignKey { + name: None, + child_column: "pid".to_string(), + parent_table: "parent".to_string(), + parent_column: Some("id".to_string()), + on_delete: rdbms_playground::dsl::ReferentialAction::NoAction, + on_update: rdbms_playground::dsl::ReferentialAction::NoAction, + }], + false, + Some("create table child (id serial primary key, pid int references parent(id))".to_string()), + )) + .expect("create child with FK"); + + // The parent is referenced — refused (even with IF EXISTS, since the + // table *does* exist; the refusal is about the relationship). + assert!( + r.block_on(db.sql_drop_table("parent".to_string(), false, Some("drop table parent".to_string()))) + .is_err(), + "a referenced parent can't be dropped" + ); + // Dropping the child first succeeds, then the parent. + r.block_on(db.sql_drop_table("child".to_string(), false, Some("drop table child".to_string()))) + .expect("drop child"); + r.block_on(db.sql_drop_table("parent".to_string(), false, Some("drop table parent".to_string()))) + .expect("now the parent drops"); +} + +#[test] +fn drop_table_is_one_undo_step_and_restores_data() { + let (_p, db, _d) = open(true); // undo enabled + let r = rt(); + make_t(&db, &r); + r.block_on(db.insert( + "T".to_string(), + Some(vec!["id".to_string(), "body".to_string()]), + vec![Value::Number("1".to_string()), Value::Text("hi".to_string())], + Some("insert".to_string()), + )) + .expect("row"); + r.block_on(db.sql_drop_table("T".to_string(), false, Some("drop table T".to_string()))) + .expect("drop"); + assert!(!r.block_on(db.list_tables()).unwrap().contains(&"T".to_string())); + + // One undo brings the table — and its row — back. + assert!(r.block_on(db.undo()).expect("undo").is_some(), "the drop was one undo step"); + assert!(r.block_on(db.list_tables()).unwrap().contains(&"T".to_string())); + let data = r + .block_on(db.query_data("T".to_string(), None, None, None)) + .expect("query"); + assert_eq!(data.rows.len(), 1, "the dropped row was restored by undo"); +} diff --git a/tests/typing_surface/mod.rs b/tests/typing_surface/mod.rs index 7722eac..a4d72cd 100644 --- a/tests/typing_surface/mod.rs +++ b/tests/typing_surface/mod.rs @@ -218,6 +218,7 @@ fn command_kind_label(cmd: &rdbms_playground::dsl::Command) -> String { CreateTable { .. } => "CreateTable".into(), SqlCreateTable { .. } => "SqlCreateTable".into(), DropTable { .. } => "DropTable".into(), + SqlDropTable { .. } => "SqlDropTable".into(), AddColumn { .. } => "AddColumn".into(), DropColumn { .. } => "DropColumn".into(), RenameColumn { .. } => "RenameColumn".into(),