diff --git a/PARITY.md b/PARITY.md index 45189b85b..ee33ba3a1 100644 --- a/PARITY.md +++ b/PARITY.md @@ -140,6 +140,15 @@ This lives entirely in the shared Rust core (`create_table` runs DDL verbatim; `query` returns vanilla rows), so `PRAGMA table_info` and `SELECT *` report the same user-only columns across all three SDKs. +**Multi-statement `ddl` batch (#956) — parity by construction, no drift.** A +`[[table]]`'s `ddl` runs as a whole SQL batch (`execute_batch`, one +transaction), so indexes, FTS5/vec0 virtual tables and triggers are declarable +in config; what the batch produced is settled against SQLite's catalog +(`pragma_table_list`), not by reading DDL text. The `ddl` value is an opaque +string every SDK already forwards unchanged, so no binding signature moves and +all three gain the batch, the catalog validation and the new load errors +together. + **Internal tables unreachable through `query()` (#378, epic #358) — parity by construction, no drift.** The internal bookkeeping tables (`_dirsql_internal_rows`, `_dirsql_files`, `_dirsql_meta`) are denied on the `query()` path by a SQLite @@ -411,6 +420,9 @@ incl. #313). | `bytes`/`Vec`/`Buffer` → BLOB (list/array of ints does NOT) | Y (#465) | Y | Y (#343: `Buffer`/`Uint8Array` in, `Buffer` out) | | Invalid SQL raises | Y | Y | Y | | Invalid DDL raises | Y | Y | Y | +| Multi-statement `ddl` batch (index / FTS5 vtab / triggers) runs whole (#956) | core | Y (`ddl_batch.rs`, `ddl_batch_e2e.rs`) | core | +| `ddl` batch SQLite rejects: rolled back, error prefixed with the entry (#956) | core | Y (`ddl_batch.rs`, `sdk.rs`) | core | +| Declared `name` resolving to a virtual table errors (#956) | core | Y (`ddl_batch.rs`) | core | | Query rejects writes | Y | Y | Y | | Write-rejection edge matrix (leading comments, mixed case, CTE/whitespace allowed) | core | Y (`readonly_query.rs`) | core | | Internal `_dirsql_*` columns hidden from `SELECT *` | Y | Y | Y | @@ -473,6 +485,7 @@ incl. #313). | `[[table]]` missing `ddl` errors | Y | core | Y | | `[[table]]` missing `name` errors | Y | Y | Y | | `[[table]]` `name` absent from the catalog after `ddl` errors | Y | Y | Y | +| `[[table]]` `ddl` batch: whole batch runs, catalog settles the result (#956) | core | Y | core | | Config `persist` / `persist_path` resolution | core | Y (`from_config.rs`) | core | ### Persistence diff --git a/docs/howto/define-tables.md b/docs/howto/define-tables.md index 603d038ee..ae0294e9c 100644 --- a/docs/howto/define-tables.md +++ b/docs/howto/define-tables.md @@ -65,6 +65,50 @@ Add one `[[table]]` entry per table — each with its own `glob`, `ddl`, and table — each table is an independent view. See [`[[table]]`](../reference/config.md#table) for the remaining key, `strict`. +## Add indexes and full-text search + +`ddl` is a SQL batch, not a single statement — SQLite runs the whole thing. So +a table can arrive with its own index, and with an FTS5 index kept current by +two triggers: + +```toml +[[table]] +name = "posts" +glob = "posts/**/*.md" +on-file = "python3 extract.py {path}" +ddl = ''' +CREATE TABLE posts (title TEXT, slug TEXT, body TEXT); +CREATE INDEX posts_slug ON posts(slug); + +CREATE VIRTUAL TABLE posts_fts + USING fts5(body, content='posts', content_rowid='rowid'); +CREATE TRIGGER posts_ai AFTER INSERT ON posts BEGIN + INSERT INTO posts_fts(rowid, body) VALUES (new.rowid, new.body); +END; +CREATE TRIGGER posts_ad AFTER DELETE ON posts BEGIN + INSERT INTO posts_fts(posts_fts, rowid, body) + VALUES ('delete', old.rowid, old.body); +END; +''' +``` + +```bash +dirsql query "SELECT slug FROM posts JOIN posts_fts ON posts.rowid = posts_fts.rowid + WHERE posts_fts MATCH 'recursion'" -c ./.dirsql.toml +``` + +```json +[{"slug":"again"}] +``` + +dirsql writes rows with plain `INSERT` and `DELETE`, so those triggers hold +through the initial load and every [watcher](./react-to-changes.md) event — you +maintain nothing. `name` still has to be the row table (`posts`); the virtual +table lives beside it under its own name. The batch runs once, when the table +is created, and editing any part of it rebuilds a +[persistent cache](./persist.md) from scratch. Full rules: +[Batch `ddl`](../reference/config.md#batch-ddl). + ## Going further - The parser mechanics — placeholders, stdout protocol, per-file failure diff --git a/docs/howto/load-extension.md b/docs/howto/load-extension.md index 5a9ea7296..f569553a7 100644 --- a/docs/howto/load-extension.md +++ b/docs/howto/load-extension.md @@ -76,9 +76,11 @@ interpreter to resolve package names with ## Notes -- Extensions add **functions**. An extension-backed virtual table cannot be - declared as a `[[table]]` — `dirsql` tables are per-file row tables - ([reference](../reference/config.md#dirsql-extension)). +- Extensions add **functions**, and virtual tables a table's `ddl` batch can + create. A `[[table]]`'s own `name` may not be a virtual table — that one is + the per-file row table `dirsql` inserts into — so declare the virtual table + alongside it in the same batch + ([reference](../reference/config.md#batch-ddl)). - Loading happens before any table DDL runs, and the SQL `load_extension()` function is never exposed to queries ([reference](../reference/config.md#dirsql-extension)). diff --git a/docs/howto/search-by-meaning.md b/docs/howto/search-by-meaning.md index f692cff3e..1d5e15885 100644 --- a/docs/howto/search-by-meaning.md +++ b/docs/howto/search-by-meaning.md @@ -95,10 +95,12 @@ LIMIT 10 ::: tip Top-k is `LIMIT k` If you know `sqlite-vec` you may reach for its `MATCH … AND k = 10` idiom. -That syntax belongs to `sqlite-vec`'s `vec0` virtual table, which `dirsql` -does not use — `dirsql` tables are per-file row tables. For plain -expressions, `sqlite-vec`'s own documented pattern is exactly what this -guide uses: `ORDER BY vec_distance_cosine(...) LIMIT k`. +That syntax belongs to `sqlite-vec`'s `vec0` virtual table, which the table +above does not use: a `[[table]]`'s own `name` is always a per-file row table. +For plain expressions, `sqlite-vec`'s own documented pattern is exactly what +this guide uses: `ORDER BY vec_distance_cosine(...) LIMIT k`. To get the `vec0` +idiom instead, declare the `vec0` table alongside the row table in the same +[`ddl` batch](../reference/config.md#batch-ddl) and fill it from a trigger. ::: ## Repeat runs are cheap diff --git a/docs/reference/config.md b/docs/reference/config.md index 407be9efa..0f1f58c1c 100644 --- a/docs/reference/config.md +++ b/docs/reference/config.md @@ -87,10 +87,11 @@ name you installed: `path = "sqlite-vec-linux-x64"` (matching your platform), not `path = "sqlite-vec"`, whose meta-package ships no loadable. -Extensions add **functions** callable in queries and in a table's DDL. An -extension-backed **virtual table** cannot be declared as a `[[table]]` — -`dirsql` tables are per-file row tables, so a `CREATE VIRTUAL TABLE` DDL is -rejected; call the extension's functions in queries instead. +Extensions add **functions** callable in queries and in a table's DDL, and +**virtual tables** a table's [`ddl` batch](#batch-ddl) can create. What a +`[[table]]`'s own `name` may not be is a virtual table: that one is the +per-file row table `dirsql` inserts into. Create the virtual table alongside +it, under its own name. ## `[[dirsql.function]]` @@ -167,7 +168,7 @@ what its required `on-file` command emits — dirsql injects nothing (see | Key | Required | Description | |---|---|---| | `name` | yes | The table's SQL name — the name you query it by. Declared, never derived from `ddl`: dirsql does not read the DDL text. The `ddl` must create a table by this name; if it doesn't, loading fails. | -| `ddl` | yes | A SQLite `CREATE TABLE` statement, run verbatim. Only the columns declared here are kept; keys the `on-file` command emits that are not declared are dropped. | +| `ddl` | yes | A SQL batch, run verbatim — any number of statements. It must create a table called `name`; that table holds the file rows, and only the columns it declares are kept (keys the `on-file` command emits that are not declared are dropped). The rest of the batch is yours: indexes, virtual tables, triggers. See [Batch `ddl`](#batch-ddl). | | `glob` | yes | Glob pattern matched against root-relative paths. Every table whose glob matches a file receives that file's rows — a file can populate multiple tables. A `{name}` segment is rewritten to `*` (it matches one path segment but captures nothing). | | `on-file` | **yes** | A command run once per matched file; its stdout (a JSON array of row objects) becomes the file's rows. Must be non-empty. A `[[table]]` with no `on-file` is a load error (see [parse errors](#parse-errors)). See [Command hooks](./hooks.md#on-file). | | `strict` | no (default `false`) | When `true`, rows whose keys do not exactly match the declared columns are rejected with an error: extra keys error, and every declared column must be supplied by the `on-file` output. When `false`, extra keys are dropped and missing columns become `NULL`. | @@ -194,6 +195,72 @@ on-file = "uv run python extract_papers.py {path}" strict = true ``` +### Batch `ddl` + +`ddl` is handed to SQLite whole, so a table declaration is not limited to one +statement: + +```toml +[[table]] +name = "messages" +glob = "sessions/*/messages/*.json" +on-file = "jq -c '.' {path}" +ddl = ''' +CREATE TABLE messages (session TEXT, idx INT, role TEXT, text TEXT); +CREATE INDEX messages_session ON messages(session); + +CREATE VIRTUAL TABLE messages_fts + USING fts5(text, content='messages', content_rowid='rowid'); +CREATE TRIGGER messages_ai AFTER INSERT ON messages BEGIN + INSERT INTO messages_fts(rowid, text) VALUES (new.rowid, new.text); +END; +CREATE TRIGGER messages_ad AFTER DELETE ON messages BEGIN + INSERT INTO messages_fts(messages_fts, rowid, text) + VALUES ('delete', old.rowid, old.text); +END; +''' +``` + +```bash +dirsql query "SELECT text FROM messages_fts WHERE messages_fts MATCH 'deploy'" \ + -c ./.dirsql.toml +``` + +Those two triggers are all a keyword index needs. dirsql writes file rows with +plain `INSERT` and `DELETE` — an update is a delete and an insert in one +transaction, and there is no `UPDATE` path on user rows — so triggers you +declare here stay current through the initial load and every +[watcher](../howto/react-to-changes.md) event. The same shape with a `vec0` +virtual table and an [`embed()`](#dirsql-function) call in the trigger gives +you stored vectors. + +**dirsql never reads the batch.** After it runs, SQLite's own catalog +(`pragma_table_list`) settles what it produced: + +- No table called `name` → a load error that lists what the batch *did* + create, so a typo is obvious. +- `name` is a **virtual** table → a load error. The declared table is the one + dirsql inserts file rows into, so it has to be a real row table. Create the + virtual table alongside it, under its own name. +- `name` is **`WITHOUT ROWID`** → a warning. Internal row bookkeeping is keyed + on rowid, so these will be rejected in a future release. + +The whole batch runs in one transaction: if any statement fails, none of them +took effect, and the error is SQLite's own, prefixed with the config entry — +`table 'messages': SQLite error: near "(": syntax error`. Context, never +interpretation. + +Two consequences of `ddl` running **once, when the table is created**: + +- **Rows the batch inserts itself are not file-tracked.** No file owns them, so + they survive file deletions — and they vanish on any rebuild. +- **Editing `ddl` at all rebuilds a + [persistent cache](../howto/persist.md).** The config hash covers the entire + batch, so a new index, a different FTS5 + tokenizer or a changed embedding model id forces a full sweep and re-ingest. + That is the only invalidation lane: dirsql tracks no ownership of what the + batch made. + ### `on-file` row mapping The command prints a JSON array of objects; each object becomes one row. @@ -247,13 +314,24 @@ SDKs raise/reject) when: - A `[[table]]` entry omits `name`, `ddl`, or `glob` (or `name` is empty/whitespace). - A `[[table]]` entry's `ddl` runs but creates no table by its `name`. The - error carries the entry's name and points at the fix: + error carries the entry's name, lists what the batch did create, and points + at the fix: - > `table 'messages': its `ddl` ran but created no table called 'messages'. Set `name` to the table the `ddl` creates.` + > `table 'messages': its `ddl` ran but created no table called 'messages' (it created: mesages, mesages_fts). Set `name` to the table the `ddl` creates.` dirsql asks SQLite's catalog rather than interpreting the DDL, so quoted (`CREATE TABLE "messages"`), schema-qualified (`main.messages`) and `IF NOT EXISTS` forms all match a plain `name = "messages"`. +- A `[[table]]` entry's `name` names a **virtual** table. The declared table + holds the file rows, so it must be a real row table: + + > `table 'messages': its `ddl` created a virtual table called 'messages'. The declared table holds the file rows, so it must be a real row table; create the virtual table alongside it, under its own name.` + +- A `[[table]]` entry's `ddl` is rejected by SQLite. Nothing the batch did + takes effect, and SQLite's own message is passed through under the entry's + name: + + > `table 'messages': SQLite error: near "(": syntax error` - A `[[table]]` entry omits `on-file` (or it is empty/whitespace). The error names the offending glob and points at the fix: diff --git a/packages/python/e2e-attestations/claude-tackle-956-xzen74.json b/packages/python/e2e-attestations/claude-tackle-956-xzen74.json new file mode 100644 index 000000000..a31398207 --- /dev/null +++ b/packages/python/e2e-attestations/claude-tackle-956-xzen74.json @@ -0,0 +1,7 @@ +{ + "command": "uv run python -m pytest tests/e2e/ -x -q", + "ran_at": 1787258633, + "exit_code": 0, + "commit": "9191ced6e960be0f079fd21957473441f5cd2455", + "branch": "claude/tackle-956-xzen74" +} diff --git a/packages/rust/changelog.d/2026-08-20-ddl-batch.md b/packages/rust/changelog.d/2026-08-20-ddl-batch.md new file mode 100644 index 000000000..02ccad0d9 --- /dev/null +++ b/packages/rust/changelog.d/2026-08-20-ddl-batch.md @@ -0,0 +1,9 @@ +**Changed** + +- **A `[[table]]`'s `ddl` is now a wide-open multi-statement SQL batch.** It runs through `execute_batch` inside one transaction, so a table can arrive with its own indexes, virtual tables and triggers — `CREATE INDEX` for B-trees, an external-content FTS5 vtab plus two triggers for keyword search with `bm25()`/`snippet()`, a `vec0` vtab plus an `embed()` trigger for stored vectors. dirsql writes user rows with plain `INSERT`/`DELETE` (an upsert is a delete and an insert in one transaction, and there is no `UPDATE` path on user rows), so those triggers stay current through the initial load and every watcher event, with no bookkeeping on dirsql's side. The two string sniffs the single-statement rule needed — `is_virtual_table_ddl` and `is_without_rowid_ddl` — are gone: what the batch produced is settled against SQLite's catalog (`pragma_table_list`) instead. A declared `name` that ends up a virtual table is a load error; one that is `WITHOUT ROWID` keeps its existing warning. (#956) + +**Fixed** + +- **A `ddl` holding more than one statement no longer runs only its first.** `Connection::execute` prepares one statement and discards the rest of the string, so a second statement was silently dropped — a `CREATE INDEX` that never ran, and a batch whose tail SQLite could not even parse still loading successfully. The whole batch now runs, and a statement SQLite rejects fails the load with SQLite's own message under the config entry that caused it (`table 'messages': SQLite error: near "(": syntax error`), leaving nothing behind. (#956) + +- **The persistent-cache sweep now drops virtual tables before plain ones.** Drop order came from `sqlite_master`'s creation order, which happened to be safe; it is now derived from `pragma_table_list`'s type. Dropping a shadow table before the virtual table that owns it poisons the vtab's own drop even with `IF EXISTS` (probed against sqlite-vec 0.1.9), which a `ddl` batch creating a `vec0` table could now hit. (#956) diff --git a/packages/rust/migrations.d/2026-08-20-ddl-batch.md b/packages/rust/migrations.d/2026-08-20-ddl-batch.md new file mode 100644 index 000000000..193602d19 --- /dev/null +++ b/packages/rust/migrations.d/2026-08-20-ddl-batch.md @@ -0,0 +1,88 @@ +### `ddl` becomes a multi-statement SQL batch, validated against the catalog (#956) + +#### Summary + +A `[[table]]`'s `ddl` had to be exactly one statement, and dirsql string-sniffed +it for `CREATE VIRTUAL TABLE` and `WITHOUT ROWID`. It is now a whole SQL batch, +run through `execute_batch` inside one transaction, and what it produced is +settled afterwards against SQLite's catalog (`pragma_table_list`) rather than by +reading the DDL text. `Db::create_table` keeps its `(name, ddl)` signature; +`db::is_virtual_table_ddl`, `db::is_without_rowid_ddl` and `DbError::DdlParse` +are deleted. `DirSqlError` gains `TableIsVirtual` and `TableDdl`, and +`TableNotCreated` gains a `created` field carrying the before/after catalog diff. + +#### Required changes + +| Before | After | Fix | +| ------ | ----- | --- | +| `Err(DirSqlError::TableNotCreated { name })` as an exhaustive pattern | Does not compile — the variant gained `created: Vec` | Match `TableNotCreated { name, .. }`, or bind `created` to read what the batch did create. | +| `Err(DirSqlError::Core(_))` for a `ddl` SQLite rejects | No longer produced for that case | Match `DirSqlError::TableDdl { name, source }`; `source` is the same `DbError` as before, and `Display` prefixes it with `table '': `. | +| `DbError::DdlParse` constructed or matched | Does not compile | Nothing produces it any more. A rejected batch is `DbError::Sqlite`, reached through `DirSqlError::TableDdl`. | +| `ddl = "CREATE VIRTUAL TABLE vss USING vec0(...)"` rejected before running | The batch runs; the declared table being virtual is caught after, as `DirSqlError::TableIsVirtual` | Keep `name` on a real row table and declare the virtual table alongside it in the same batch. | + +#### Deprecations removed + +- `DbError::DdlParse` — removed outright, with `db::is_virtual_table_ddl` and + `db::is_without_rowid_ddl`, the two whitespace-normalizing string sniffs + behind it. + +#### Behavior changes without code changes + +- **Every statement in `ddl` now runs.** `Connection::execute` prepared only the + first and discarded the tail, so a config with `CREATE TABLE t (...); CREATE + INDEX ...` silently created the table without the index, and one whose second + statement SQLite could not parse still loaded. Both now behave as written: the + index is created, and the unparseable batch is a load error. +- **A batch SQLite rejects leaves nothing behind**, and its error carries the + config entry: `table 'messages': SQLite error: near "(": syntax error`. + Previously the first statement of a partly-invalid `ddl` took effect. +- **A `[[table]]` whose `name` resolves to a virtual table is a load error** + raised after the batch runs, not a string match before it. A batch is now free + to create as many virtual tables as it likes under other names. +- **`WITHOUT ROWID` is detected from the catalog's `wr` flag** rather than from + the DDL text. Same warning, same "will be rejected in a future release" + wording; a `WITHOUT ROWID` *shadow* table belonging to some other virtual + table no longer triggers it. +- **The persistent-cache sweep drops virtual tables first**, derived from + `pragma_table_list`'s type instead of `sqlite_master`'s creation order, and + leaves shadow tables to the virtual table that owns them. +- **Editing any part of `ddl` still invalidates a persistent cache** — the + config hash already covered the whole string, and now that the whole string + runs, that is the single invalidation lane for indexes, tokenizers and + embedding model ids alike. Rows a batch inserts itself are not file-tracked: + they survive file deletions and vanish on a rebuild. + +#### Verification + +```bash +# .dirsql.toml +# [[table]] +# name = "records" +# glob = "data/*.json" +# on-file = "cat {path}" +# ddl = ''' +# CREATE TABLE records (id TEXT, body TEXT); +# CREATE INDEX records_id ON records(id); +# CREATE VIRTUAL TABLE records_fts +# USING fts5(body, content='records', content_rowid='rowid'); +# CREATE TRIGGER records_ai AFTER INSERT ON records BEGIN +# INSERT INTO records_fts(rowid, body) VALUES (new.rowid, new.body); +# END; +# ''' + +dirsql query "SELECT name FROM pragma_index_list('records')" -c ./.dirsql.toml +# -> [{"name":"records_id"}] (before: [], the CREATE INDEX was dropped) + +dirsql query "SELECT body FROM records_fts WHERE records_fts MATCH 'hello'" \ + -c ./.dirsql.toml +# -> [{"body":"hello world"}] + +# Truncate the batch's last statement: +dirsql query "SELECT id FROM records" -c ./.dirsql.toml +# -> error: table 'records': SQLite error: incomplete input +# (before: exit 0 — the tail was silently ignored) + +# Point `name` at a virtual table: +dirsql query "SELECT id FROM records" -c ./.dirsql.toml +# -> error: table 'records': its `ddl` created a virtual table called 'records'. +``` diff --git a/packages/rust/src/db.rs b/packages/rust/src/db.rs index 3d459cfd0..addb5cb60 100644 --- a/packages/rust/src/db.rs +++ b/packages/rust/src/db.rs @@ -24,9 +24,6 @@ pub enum DbError { #[error("Schema mismatch: {0}")] SchemaMismatch(String), - #[error("DDL parse error: {0}")] - DdlParse(String), - #[error("invalid identifier: {0:?} (must match [A-Za-z_][A-Za-z0-9_]*)")] InvalidIdentifier(String), @@ -400,41 +397,26 @@ impl Db { Ok(()) } - /// Create a table from a user-provided DDL statement, executed - /// **verbatim**: dirsql injects no tracking columns — row ownership lives - /// entirely in `_dirsql_internal_rows`, so a table's schema is exactly - /// the DDL the user wrote. + /// Run a table's user-provided DDL **batch**, executed **verbatim**: + /// dirsql injects no tracking columns — row ownership lives entirely in + /// `_dirsql_internal_rows`, so a table's schema is exactly the DDL the + /// user wrote. + /// + /// The batch may hold any number of statements, of any kind: the row table + /// plus whatever indexes, virtual tables and triggers go with it. It runs + /// inside one transaction, so a statement SQLite rejects leaves nothing + /// behind, and SQLite's own error is returned untouched. /// /// `name` is the table's declared name, validated as a safe unquoted SQL - /// identifier before the DDL reaches SQLite: it is spliced into + /// identifier before the batch reaches SQLite: it is spliced into /// `format!()`-built INSERT/DELETE SQL downstream, so a poisoned name must - /// never get that far. Whether the DDL actually creates a table by that - /// name is settled afterwards against SQLite's catalog, not by reading the - /// DDL text. + /// never get that far. What the batch actually created is settled + /// afterwards against SQLite's catalog, not by reading the DDL text. pub fn create_table(&self, name: &str, ddl: &str) -> Result<()> { - // A dirsql table is a per-file row table; an extension-backed virtual - // table is not one, so reject `CREATE VIRTUAL TABLE` with a clear - // message. Load the extension and use its functions in queries instead. - if is_virtual_table_ddl(ddl) { - return Err(DbError::DdlParse( - "CREATE VIRTUAL TABLE is not supported as a dirsql table \ - (dirsql tables are per-file row tables); load the extension \ - and call its functions in queries instead" - .to_string(), - )); - } validate_identifier(name)?; - // A `WITHOUT ROWID` table has no rowid, so `last_insert_rowid()` cannot - // identify the inserted row and the `_dirsql_internal_rows.rowid_ref` - // mapping would be meaningless. Warn; the table is still created. - if is_without_rowid_ddl(ddl) { - eprintln!( - "dirsql: table `{name}` is declared WITHOUT ROWID; internal row \ - bookkeeping relies on rowid and WITHOUT ROWID tables will be \ - rejected in a future release" - ); - } - self.conn.execute(ddl, [])?; + let tx = self.conn.unchecked_transaction()?; + self.conn.execute_batch(ddl)?; + tx.commit()?; Ok(()) } @@ -900,21 +882,58 @@ impl rusqlite::types::ToSql for Value { } } -/// True if `ddl` is a `CREATE VIRTUAL TABLE` statement. dirsql tables are -/// per-file row tables, structurally incompatible with an extension-backed -/// virtual table, so those are rejected with a clear error. -fn is_virtual_table_ddl(ddl: &str) -> bool { - let normalized = ddl.split_whitespace().collect::>().join(" "); - normalized.to_uppercase().contains("CREATE VIRTUAL TABLE") +/// What SQLite's catalog says about one table in `main`. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct CatalogEntry { + /// `pragma_table_list`'s `type`: `table`, `virtual`, `view` or `shadow`. + pub kind: String, + /// Whether the table is declared `WITHOUT ROWID`. + pub without_rowid: bool, +} + +/// The catalog rows a table's `ddl` batch could be responsible for: real and +/// virtual tables in `main`, never the shadow tables a virtual table keeps +/// behind it, SQLite's own `sqlite_*` tables, or dirsql's `_dirsql_*` +/// bookkeeping. +/// +/// Virtual tables come first, and that order is load-bearing for the cache +/// sweep: dropping a shadow table before its virtual table poisons the virtual +/// table's own drop even with `IF EXISTS` (probed against sqlite-vec 0.1.9). +/// `sqlite_master`'s creation order happens to get this right today; asking +/// the catalog for the type makes it guaranteed. +const USER_TABLES_SQL: &str = "SELECT name FROM pragma_table_list \ + WHERE schema = 'main' AND type IN ('table', 'virtual') \ + AND name NOT LIKE '_dirsql_%' AND name NOT LIKE 'sqlite_%' \ + ORDER BY type = 'virtual' DESC, name"; + +/// Every user table on `conn`, in an order that is safe to drop in. +pub(crate) fn user_table_names(conn: &Connection) -> rusqlite::Result> { + let mut stmt = conn.prepare(USER_TABLES_SQL)?; + let names = stmt + .query_map([], |row| row.get::<_, String>(0))? + .collect::>>()?; + Ok(names) } -/// True if `ddl` declares a `WITHOUT ROWID` table. Such tables have no rowid, -/// so `last_insert_rowid()` cannot identify an inserted row and the -/// `_dirsql_internal_rows.rowid_ref` mapping is meaningless. Whitespace is -/// normalized so `WITHOUT ROWID` and newline-separated forms are detected. -fn is_without_rowid_ddl(ddl: &str) -> bool { - let normalized = ddl.split_whitespace().collect::>().join(" "); - normalized.to_uppercase().contains("WITHOUT ROWID") +/// What `conn`'s catalog holds under `name`, or `None` if it holds nothing. +/// `pragma_table_list` is the catalog API for exactly this question, so no +/// part of dirsql has to interpret DDL text to answer it. +pub(crate) fn table_catalog_entry( + conn: &Connection, + name: &str, +) -> rusqlite::Result> { + use rusqlite::OptionalExtension; + conn.query_row( + "SELECT type, wr FROM pragma_table_list WHERE schema = 'main' AND name = ?1", + rusqlite::params![name], + |row| { + Ok(CatalogEntry { + kind: row.get(0)?, + without_rowid: row.get::<_, i64>(1)? != 0, + }) + }, + ) + .optional() } impl From for Value { @@ -1351,8 +1370,8 @@ mod tests { let err = DbError::SchemaMismatch("test error".to_string()); assert!(err.to_string().contains("Schema mismatch")); - let err = DbError::DdlParse("bad ddl".to_string()); - assert!(err.to_string().contains("DDL parse error")); + let err = DbError::InvalidIdentifier("a b".to_string()); + assert!(err.to_string().contains("invalid identifier")); } #[test] @@ -1551,25 +1570,6 @@ mod tests { assert!(matches!(err, DbError::Sqlite(_)), "got: {err}"); } - #[test] - fn create_table_rejects_virtual_table_with_clear_error() { - let db = Db::new().unwrap(); - let err = db - .create_table( - "vss", - "CREATE VIRTUAL TABLE vss USING vec0(embedding float[4])", - ) - .unwrap_err(); - // Must be a clear "not supported" message, NOT the generic `DdlParse` - // echo (which trivially contains "virtual table" because it echoes - // the DDL back). - let msg = err.to_string().to_lowercase(); - assert!( - msg.contains("virtual table") && msg.contains("not supported"), - "expected a clear 'virtual table not supported' error, not a generic DDL-parse echo, got: {err}" - ); - } - #[test] fn open_sets_wal_journal_mode_and_normal_synchronous() { let dir = tempfile::TempDir::new().unwrap(); @@ -1588,18 +1588,6 @@ mod tests { assert_eq!(synchronous, 1, "Db::open must set synchronous=NORMAL (1)"); } - #[test] - fn is_virtual_table_ddl_detects_variants() { - assert!(is_virtual_table_ddl("CREATE VIRTUAL TABLE x USING vec0(a)")); - assert!(is_virtual_table_ddl( - "create virtual table x using fts5(a)" - )); - assert!(!is_virtual_table_ddl("CREATE TABLE x (a TEXT)")); - assert!(!is_virtual_table_ddl( - "CREATE TABLE IF NOT EXISTS x (a TEXT)" - )); - } - /// Read the raw mapping rows for a table, ordered for stable assertions. fn mapping_rows(db: &Db, table: &str) -> Vec<(String, i64, i64)> { let mut stmt = db @@ -1821,7 +1809,65 @@ mod tests { } #[test] - fn create_table_allows_without_rowid_and_warns() { + fn create_table_runs_every_statement_in_the_batch() { + let db = Db::new().unwrap(); + db.create_table( + "notes", + "CREATE TABLE notes (path TEXT);\n\ + CREATE INDEX notes_path ON notes(path);\n\ + CREATE VIRTUAL TABLE notes_fts USING fts5(path);", + ) + .unwrap(); + + assert_eq!(user_table_names(&db.conn).unwrap(), ["notes_fts", "notes"]); + let index: i64 = db + .conn + .query_row( + "SELECT COUNT(*) FROM pragma_index_list('notes') WHERE name = 'notes_path'", + [], + |row| row.get(0), + ) + .unwrap(); + assert_eq!(index, 1, "the batch's CREATE INDEX must have run"); + } + + #[test] + fn create_table_rolls_back_a_batch_sqlite_rejects() { + let db = Db::new().unwrap(); + let err = db + .create_table( + "notes", + "CREATE TABLE notes (path TEXT); CREATE TABLE oops (", + ) + .unwrap_err(); + + assert!(matches!(err, DbError::Sqlite(_)), "got: {err}"); + assert!( + err.to_string().contains("incomplete input"), + "SQLite's own error must come through untouched, got: {err}" + ); + assert!( + user_table_names(&db.conn).unwrap().is_empty(), + "a rejected batch must leave none of its earlier statements behind" + ); + } + + #[test] + fn create_table_validates_the_name_before_running_the_batch() { + let db = Db::new().unwrap(); + let err = db + .create_table("a b", "CREATE TABLE \"a b\" (path TEXT)") + .unwrap_err(); + + assert!(matches!(err, DbError::InvalidIdentifier(_)), "got: {err}"); + assert!( + user_table_names(&db.conn).unwrap().is_empty(), + "a poisoned name must be rejected before SQLite sees the batch" + ); + } + + #[test] + fn create_table_still_runs_a_without_rowid_ddl() { let db = Db::new().unwrap(); db.create_table("t", "CREATE TABLE t (id TEXT PRIMARY KEY) WITHOUT ROWID") .unwrap(); @@ -1830,14 +1876,62 @@ mod tests { } #[test] - fn is_without_rowid_ddl_detects_variants() { - assert!(is_without_rowid_ddl( - "CREATE TABLE t (id TEXT PRIMARY KEY) WITHOUT ROWID" - )); - assert!(is_without_rowid_ddl( - "create table t (id text primary key)\n without rowid" - )); - assert!(!is_without_rowid_ddl("CREATE TABLE t (id TEXT)")); + fn user_table_names_lists_virtual_tables_before_real_ones() { + let db = Db::new().unwrap(); + // Created real-table-first, so `sqlite_master`'s creation order would + // put `notes` ahead of the virtual table it belongs to. + db.conn + .execute_batch( + "CREATE TABLE notes (body TEXT);\n\ + CREATE VIRTUAL TABLE notes_fts USING fts5(body);\n\ + CREATE TABLE archive (body TEXT);", + ) + .unwrap(); + + assert_eq!( + user_table_names(&db.conn).unwrap(), + ["notes_fts", "archive", "notes"] + ); + } + + #[test] + fn user_table_names_omits_shadow_and_bookkeeping_tables() { + let db = Db::new().unwrap(); + db.conn + .execute_batch("CREATE VIRTUAL TABLE notes_fts USING fts5(body);") + .unwrap(); + + let names = user_table_names(&db.conn).unwrap(); + assert_eq!( + names, + ["notes_fts"], + "FTS5's shadow tables and `_dirsql_internal_rows` must not be listed" + ); + } + + #[test] + fn table_catalog_entry_reports_kind_and_without_rowid() { + let db = Db::new().unwrap(); + db.conn + .execute_batch( + "CREATE TABLE plain (id TEXT);\n\ + CREATE TABLE keyed (id TEXT PRIMARY KEY) WITHOUT ROWID;\n\ + CREATE VIRTUAL TABLE virt USING fts5(body);", + ) + .unwrap(); + + let plain = table_catalog_entry(&db.conn, "plain").unwrap().unwrap(); + assert_eq!(plain.kind, "table"); + assert!(!plain.without_rowid); + + let keyed = table_catalog_entry(&db.conn, "keyed").unwrap().unwrap(); + assert_eq!(keyed.kind, "table"); + assert!(keyed.without_rowid); + + let virt = table_catalog_entry(&db.conn, "virt").unwrap().unwrap(); + assert_eq!(virt.kind, "virtual"); + + assert_eq!(table_catalog_entry(&db.conn, "absent").unwrap(), None); } #[test] diff --git a/packages/rust/src/lib.rs b/packages/rust/src/lib.rs index 97de62302..4ee6a8840 100644 --- a/packages/rust/src/lib.rs +++ b/packages/rust/src/lib.rs @@ -128,10 +128,25 @@ pub enum DirSqlError { Ddl(String), #[error( - "table '{name}': its `ddl` ran but created no table called '{name}'. \ - Set `name` to the table the `ddl` creates." + "table '{name}': its `ddl` ran but created no table called '{name}' \ + (it created: {}). Set `name` to the table the `ddl` creates.", + if .created.is_empty() { "nothing".to_string() } else { .created.join(", ") } )] - TableNotCreated { name: String }, + TableNotCreated { name: String, created: Vec }, + + #[error( + "table '{name}': its `ddl` created a virtual table called '{name}'. \ + The declared table holds the file rows, so it must be a real row \ + table; create the virtual table alongside it, under its own name." + )] + TableIsVirtual { name: String }, + + #[error("table '{name}': {source}")] + TableDdl { + name: String, + #[source] + source: DbError, + }, #[error("failed to load extension '{}': {source}", .path.display())] Extension { @@ -848,17 +863,28 @@ impl DirSQL { for table in tables { let table_name = table.name.clone(); - // When the cache already holds this table from a prior run, - // skip CREATE TABLE: the schema is preserved verbatim across - // runs (the glob_config_hash captures the DDL). - if !table_exists(&db, &table_name)? { - db.create_table(&table_name, &table.ddl)?; - // Ask SQLite whether the DDL actually produced this name - // rather than reading the DDL text. No row means the declared - // `name` names nothing -- a load-time failure, before any file - // is ingested. - if !table_exists(&db, &table_name)? { - return Err(DirSqlError::TableNotCreated { name: table_name }); + // A real table under this name is the cache already holding it + // from a prior run: skip the batch, since the schema is preserved + // verbatim across runs (the glob_config_hash captures the whole + // `ddl`). Anything else under the name is not a warm cache — run + // the batch and let SQLite object to the collision itself. + let warm = catalog_entry(&db, &table_name)?.is_some_and(|entry| entry.kind == "table"); + if !warm { + let before = db::user_table_names(db.conn()).map_err(DirSqlError::sqlite)?; + db.create_table(&table_name, &table.ddl).map_err(|source| { + DirSqlError::TableDdl { + name: table_name.clone(), + source, + } + })?; + let created = db::user_table_names(db.conn()) + .map_err(DirSqlError::sqlite)? + .into_iter() + .filter(|name| !before.contains(name)) + .collect(); + let entry = catalog_entry(&db, &table_name)?; + if let Some(warning) = classify_declared_table(&table_name, entry, created)? { + eprintln!("{warning}"); } } // Reject a `{name}` glob placeholder whose name is also a declared @@ -1564,20 +1590,52 @@ fn reconcile_scan( Ok((to_parse, trusted, deleted)) } -/// Whether `main` holds a table called `name`, asked of SQLite's own catalog. -/// `pragma_table_list` is the catalog API for exactly this question, so no -/// part of dirsql has to interpret DDL text to answer it. -fn table_exists(db: &Db, name: &str) -> Result { - let count: i64 = db - .conn() - .query_row( - "SELECT COUNT(*) FROM pragma_table_list \ - WHERE schema = 'main' AND type = 'table' AND name = ?1", - rusqlite::params![name], - |row| row.get(0), - ) - .map_err(DirSqlError::sqlite)?; - Ok(count > 0) +/// What SQLite's catalog holds under `name`, if anything. +fn catalog_entry(db: &Db, name: &str) -> Result> { + db::table_catalog_entry(db.conn(), name).map_err(DirSqlError::sqlite) +} + +/// Settle what a table's `ddl` batch produced for its declared `name`, given +/// SQLite's catalog and the tables the batch added. `created` is a before/after +/// catalog diff, so a `name` that never appeared can say what did. +/// +/// `Ok(None)` accepts the table, `Ok(Some(warning))` accepts it with a line for +/// stderr, and `Err` fails the load before any file is ingested. Pure: the +/// caller reads the catalog and prints the warning. +fn classify_declared_table( + name: &str, + entry: Option, + mut created: Vec, +) -> Result> { + let Some(entry) = entry else { + // The catalog hands these back in drop-safe order (virtuals first); + // sort so the message reads as a list rather than as an artifact of + // how the sweep needs them. + created.sort(); + return Err(DirSqlError::TableNotCreated { + name: name.to_string(), + created, + }); + }; + // A dirsql table is the per-file row table rows are inserted into; an + // extension-backed virtual table is not one. The batch may still create as + // many as it likes alongside it. + if entry.kind == "virtual" { + return Err(DirSqlError::TableIsVirtual { + name: name.to_string(), + }); + } + // A `WITHOUT ROWID` table has no rowid, so `last_insert_rowid()` cannot + // identify the inserted row and the `_dirsql_internal_rows.rowid_ref` + // mapping would be meaningless. Warn; the table is still usable. + if entry.without_rowid { + return Ok(Some(format!( + "dirsql: table `{name}` is declared WITHOUT ROWID; internal row \ + bookkeeping relies on rowid and WITHOUT ROWID tables will be \ + rejected in a future release" + ))); + } + Ok(None) } fn map_db_error(e: DbError) -> DirSqlError { @@ -3328,6 +3386,98 @@ mod internal_tests { assert!(msg.contains("on-file")); } + /// A `name` the batch never created: the error has to say what it *did* + /// create, or a typo gives the reader nothing to compare against. + #[test] + fn classify_declared_table_rejects_a_name_the_batch_never_created() { + let err = classify_declared_table( + "messages", + None, + vec!["mesages_fts".to_string(), "mesages".to_string()], + ) + .expect_err("a missing catalog entry must fail the load"); + + let msg = err.to_string(); + assert!( + matches!(err, DirSqlError::TableNotCreated { .. }), + "got: {msg}" + ); + assert!(msg.contains("table 'messages'"), "got: {msg}"); + assert!( + msg.contains("it created: mesages, mesages_fts"), + "the list must read sorted, not in the catalog's drop order, got: {msg}" + ); + } + + #[test] + fn table_not_created_says_nothing_when_the_batch_created_nothing() { + let err = DirSqlError::TableNotCreated { + name: "messages".to_string(), + created: Vec::new(), + }; + assert!( + err.to_string().contains("it created: nothing"), + "got: {err}" + ); + } + + #[test] + fn classify_declared_table_rejects_a_virtual_declared_table() { + let entry = db::CatalogEntry { + kind: "virtual".to_string(), + without_rowid: false, + }; + let err = classify_declared_table("messages", Some(entry), Vec::new()) + .expect_err("a virtual declared table must fail the load"); + + let msg = err.to_string(); + assert!( + matches!(err, DirSqlError::TableIsVirtual { .. }), + "got: {msg}" + ); + assert!( + msg.contains("table 'messages'") && msg.contains("virtual"), + "got: {msg}" + ); + } + + #[test] + fn classify_declared_table_warns_about_without_rowid() { + let entry = db::CatalogEntry { + kind: "table".to_string(), + without_rowid: true, + }; + let warning = classify_declared_table("messages", Some(entry), Vec::new()) + .expect("a WITHOUT ROWID table is a warning, not a failure") + .expect("it must produce a warning"); + + assert!(warning.contains("messages"), "got: {warning}"); + assert!(warning.contains("WITHOUT ROWID"), "got: {warning}"); + } + + #[test] + fn classify_declared_table_accepts_a_plain_row_table() { + let entry = db::CatalogEntry { + kind: "table".to_string(), + without_rowid: false, + }; + assert_eq!( + classify_declared_table("messages", Some(entry), Vec::new()).unwrap(), + None + ); + } + + #[test] + fn table_ddl_error_prefixes_sqlites_own_message_with_the_entry() { + let err = DirSqlError::TableDdl { + name: "messages".to_string(), + source: DbError::SchemaMismatch("near \"(\": syntax error".to_string()), + }; + let msg = err.to_string(); + assert!(msg.starts_with("table 'messages': "), "got: {msg}"); + assert!(msg.contains("near \"(\": syntax error"), "got: {msg}"); + } + #[test] fn build_tables_from_config_creates_on_file_tables() { let cfg = config::load_config_str(concat!( diff --git a/packages/rust/src/persist.rs b/packages/rust/src/persist.rs index 737a99fd1..ac1c408bc 100644 --- a/packages/rust/src/persist.rs +++ b/packages/rust/src/persist.rs @@ -414,15 +414,14 @@ pub fn delete_file(conn: &Connection, rel_path: &str, table_name: &str) -> rusql /// Drop every user-defined table from a cache database. Used when the /// reconcile detects an incompatible meta state and we need to wipe the /// cache before re-ingesting from scratch. +/// +/// A table's `ddl` is a whole SQL batch, so the cache may hold virtual tables +/// and the shadow tables behind them. `db::user_table_names` orders the drops +/// virtuals-first (and leaves shadows to the virtual table that owns them), +/// which is what keeps a vec0 vtab droppable; indexes and triggers go with +/// their table. pub fn drop_user_tables(conn: &Connection) -> rusqlite::Result<()> { - let mut stmt = - conn.prepare("SELECT name FROM sqlite_master WHERE type = 'table' AND name NOT LIKE '_dirsql_%' AND name NOT LIKE 'sqlite_%'")?; - let names: Vec = stmt - .query_map([], |row| row.get::<_, String>(0))? - .filter_map(|r| r.ok()) - .collect(); - drop(stmt); - for name in names { + for name in crate::db::user_table_names(conn)? { conn.execute(&format!("DROP TABLE IF EXISTS \"{name}\""), [])?; } conn.execute("DELETE FROM _dirsql_files", [])?; @@ -607,6 +606,45 @@ mod tests { assert!(read_cached_files(&conn).unwrap().is_empty()); } + /// A `ddl` batch may leave a virtual table and its shadow tables in the + /// cache. The sweep must clear all of it: the shadows go with the virtual + /// table, and dropping one out from under the other is what poisons the + /// vtab's own drop. + #[test] + fn drop_user_tables_clears_virtual_tables_and_their_shadows() { + let conn = Connection::open_in_memory().unwrap(); + create_sidecar_tables(&conn).unwrap(); + conn.execute( + "CREATE TABLE _dirsql_internal_rows ( + table_name TEXT NOT NULL, file_path TEXT NOT NULL, + row_index INTEGER NOT NULL, rowid_ref INTEGER NOT NULL)", + [], + ) + .unwrap(); + conn.execute_batch( + "CREATE TABLE notes (body TEXT);\n\ + CREATE VIRTUAL TABLE notes_fts USING fts5(body, content='notes');", + ) + .unwrap(); + + drop_user_tables(&conn).unwrap(); + + let left: Vec = conn + .prepare( + "SELECT name FROM sqlite_master WHERE type = 'table' \ + AND name NOT LIKE '_dirsql_%' AND name NOT LIKE 'sqlite_%' ORDER BY name", + ) + .unwrap() + .query_map([], |row| row.get::<_, String>(0)) + .unwrap() + .collect::>>() + .unwrap(); + assert!( + left.is_empty(), + "the sweep must leave no user table or shadow table behind, got {left:?}" + ); + } + #[test] fn drop_user_tables_clears_user_data_and_files_index() { let conn = Connection::open_in_memory().unwrap(); diff --git a/packages/rust/tests/ddl_batch.rs b/packages/rust/tests/ddl_batch.rs new file mode 100644 index 000000000..b1890080f --- /dev/null +++ b/packages/rust/tests/ddl_batch.rs @@ -0,0 +1,255 @@ +//! Integration tests for `ddl` as a multi-statement SQL batch. +//! +//! A `[[table]]`'s `ddl` is handed to SQLite whole: any number of statements, +//! of any kind. dirsql never reads the text — what the batch produced is +//! settled afterwards against SQLite's own catalog. That is what makes +//! indexes, FTS5 virtual tables and triggers expressible in config. + +use dirsql::{DirSQL, Row, Table, Value}; +use std::collections::HashMap; +use std::fs; +use std::path::Path; +use tempfile::TempDir; + +const BASE: &str = "CREATE TABLE notes (path TEXT, body TEXT)"; + +/// A `notes` table plus an external-content FTS5 index kept current by an +/// insert and a delete trigger — the shape the config unlocks. +const FTS_DDL: &str = "\ +CREATE TABLE notes (path TEXT, body TEXT); +CREATE VIRTUAL TABLE notes_fts USING fts5(body, content='notes', content_rowid='rowid'); +CREATE TRIGGER notes_ai AFTER INSERT ON notes BEGIN + INSERT INTO notes_fts(rowid, body) VALUES (new.rowid, new.body); +END; +CREATE TRIGGER notes_ad AFTER DELETE ON notes BEGIN + INSERT INTO notes_fts(notes_fts, rowid, body) VALUES ('delete', old.rowid, old.body); +END;"; + +/// A `notes` table whose rows are each markdown file's name and contents. +fn notes_table(ddl: &str) -> Table { + Table::new("notes", ddl, "*.md", |path| { + let body = fs::read_to_string(path).unwrap(); + vec![HashMap::from([ + ( + "path".into(), + Value::Text( + Path::new(path) + .file_name() + .unwrap() + .to_string_lossy() + .into_owned(), + ), + ), + ("body".into(), Value::Text(body.trim().to_string())), + ])] as Vec + }) +} + +fn seed(root: &Path) { + fs::write(root.join("a.md"), "hello world").unwrap(); +} + +fn build(root: &Path, ddl: &str) -> dirsql::Result { + DirSQL::builder().root(root).table(notes_table(ddl)).build() +} + +fn build_persist(root: &Path, ddl: &str) -> dirsql::Result { + DirSQL::builder() + .root(root) + .table(notes_table(ddl)) + .persist(None::<&Path>) + .build() +} + +/// The error message from a build that must fail. `DirSQL` is not `Debug`, +/// so `expect_err` is unavailable. +fn build_error(result: dirsql::Result, what: &str) -> String { + match result { + Ok(_) => panic!("{what}"), + Err(err) => err.to_string(), + } +} + +fn index_names(db: &DirSQL) -> Vec { + db.query("SELECT name FROM pragma_index_list('notes')") + .unwrap() + .into_iter() + .map(|row| row["name"].clone()) + .collect() +} + +#[test] +fn a_multi_statement_batch_runs_every_statement() { + let root = TempDir::new().unwrap(); + seed(root.path()); + + let db = build( + root.path(), + &format!("{BASE};\nCREATE INDEX notes_path ON notes(path);"), + ) + .expect("a multi-statement `ddl` must load"); + + let rows = db.query("SELECT body FROM notes").unwrap(); + assert_eq!(rows.len(), 1); + assert_eq!(rows[0]["body"], Value::Text("hello world".into())); + + let indexes = index_names(&db); + assert!( + indexes.contains(&Value::Text("notes_path".into())), + "the batch's CREATE INDEX must have run, got {indexes:?}" + ); +} + +/// The unlock the issue is really about: dirsql writes user rows with plain +/// `INSERT`/`DELETE`, so triggers declared in `ddl` keep an FTS5 index current +/// through the initial load *and* through a later file deletion. +#[test] +fn an_fts5_index_declared_in_ddl_tracks_inserts_and_deletes() { + let root = TempDir::new().unwrap(); + seed(root.path()); + + let db = build_persist(root.path(), FTS_DDL).expect("an FTS5 batch must load"); + let hits = db + .query("SELECT body FROM notes_fts WHERE notes_fts MATCH 'hello'") + .unwrap(); + assert_eq!( + hits.len(), + 1, + "the insert trigger must have indexed the row" + ); + assert_eq!(hits[0]["body"], Value::Text("hello world".into())); + drop(db); + + fs::remove_file(root.path().join("a.md")).unwrap(); + let db = build_persist(root.path(), FTS_DDL).expect("a warm rebuild must load"); + let hits = db + .query("SELECT body FROM notes_fts WHERE notes_fts MATCH 'hello'") + .unwrap(); + assert!( + hits.is_empty(), + "the delete trigger must have dropped the vanished file's row from the index, got {hits:?}" + ); +} + +#[test] +fn a_batch_sqlite_rejects_fails_the_build_and_rolls_back() { + let root = TempDir::new().unwrap(); + seed(root.path()); + + let message = build_error( + build_persist(root.path(), &format!("{BASE};\nCREATE TABLE oops (")), + "a batch SQLite rejects must fail the build", + ); + assert!( + message.contains("table 'notes'"), + "the error must carry the config-entry prefix `table 'notes'`, got {message:?}" + ); + assert!( + message.contains("incomplete input"), + "SQLite's own error text must come through raw, got {message:?}" + ); + + let cache = rusqlite::Connection::open(root.path().join(".dirsql").join("cache.db")).unwrap(); + let created: i64 = cache + .query_row( + "SELECT COUNT(*) FROM pragma_table_list WHERE schema = 'main' AND name = 'notes'", + [], + |row| row.get(0), + ) + .unwrap(); + assert_eq!( + created, 0, + "the failed batch's earlier statements must roll back, leaving no `notes` table" + ); +} + +/// A batch may create virtual tables freely — but the *declared* table is the +/// per-file row table dirsql inserts into, so it may not itself be one. +#[test] +fn a_declared_name_that_is_a_virtual_table_is_a_load_error() { + let root = TempDir::new().unwrap(); + seed(root.path()); + + let message = build_error( + build( + root.path(), + "CREATE VIRTUAL TABLE notes USING fts5(path, body)", + ), + "a dirsql table must be a real per-file row table", + ); + assert!( + message.contains("table 'notes'") && message.contains("virtual"), + "the error must name the entry and say the table is virtual, got {message:?}" + ); +} + +#[test] +fn the_not_created_error_names_what_the_batch_did_create() { + let root = TempDir::new().unwrap(); + seed(root.path()); + + let table = Table::new( + "messages", + "CREATE TABLE notes (path TEXT);\n\ + CREATE VIRTUAL TABLE notes_fts USING fts5(path);", + "*.md", + |_| Vec::::new(), + ); + let message = build_error( + DirSQL::builder().root(root.path()).table(table).build(), + "a `name` the batch never creates must fail to load", + ); + assert!( + message.contains("table 'messages'"), + "the error must carry the config-entry prefix, got {message:?}" + ); + assert!( + message.contains("notes") && message.contains("notes_fts"), + "the error must list what the batch did create, got {message:?}" + ); +} + +/// One invalidation lane: the persist hash covers the whole batch, so adding a +/// statement forces a full sweep and rebuild rather than a warm skip. +#[test] +fn editing_the_ddl_batch_rebuilds_the_cache() { + let root = TempDir::new().unwrap(); + seed(root.path()); + drop(build_persist(root.path(), BASE).expect("first build")); + + let db = build_persist( + root.path(), + &format!("{BASE};\nCREATE INDEX notes_path ON notes(path);"), + ) + .expect("an edited batch must rebuild"); + + let indexes = index_names(&db); + assert!( + indexes.contains(&Value::Text("notes_path".into())), + "a ddl edit must invalidate the cache and re-run the whole batch, got {indexes:?}" + ); +} + +/// The sweep that precedes a rebuild has to cope with whatever the previous +/// batch created — virtual tables and the shadow tables behind them included. +#[test] +fn a_rebuild_sweeps_a_cache_holding_virtual_tables() { + let root = TempDir::new().unwrap(); + seed(root.path()); + drop(build_persist(root.path(), FTS_DDL).expect("first build")); + + let db = build_persist( + root.path(), + &format!("{FTS_DDL}\nCREATE INDEX notes_path ON notes(path);"), + ) + .expect("sweeping a cache holding a virtual table and its shadows must succeed"); + + let hits = db + .query("SELECT body FROM notes_fts WHERE notes_fts MATCH 'hello'") + .unwrap(); + assert_eq!( + hits.len(), + 1, + "the rebuilt index must hold the re-ingested row, got {hits:?}" + ); +} diff --git a/packages/rust/tests/ddl_batch_e2e.rs b/packages/rust/tests/ddl_batch_e2e.rs new file mode 100644 index 000000000..44d89eacb --- /dev/null +++ b/packages/rust/tests/ddl_batch_e2e.rs @@ -0,0 +1,157 @@ +//! End-to-end tests for `ddl` as a multi-statement SQL batch. +//! +//! Spawns the real compiled `dirsql` binary against a real temp directory and +//! a real `.dirsql.toml`. Nothing is mocked (real process, real filesystem, +//! real SQLite, real `on-file` command spawn). +//! +//! Gated behind `--features cli` (the `dirsql` bin needs it) and Unix (the +//! fixture shells out to `cat`); the Rust CI test job runs on Linux. + +#![cfg(all(feature = "cli", unix))] + +use std::fs; +use std::process::Output; + +use assert_cmd::prelude::*; +use serde_json::Value; +use tempfile::TempDir; + +/// A tempdir holding one JSON file of rows and a `.dirsql.toml` with `config`. +fn fixture(config: &str) -> TempDir { + let root = TempDir::new().unwrap(); + fs::create_dir_all(root.path().join("data")).unwrap(); + fs::write( + root.path().join("data/a.json"), + r#"[{"id": "one", "body": "hello world"}, {"id": "two", "body": "goodbye moon"}]"#, + ) + .unwrap(); + fs::write(root.path().join(".dirsql.toml"), config).unwrap(); + root +} + +fn query(root: &TempDir, sql: &str) -> Output { + std::process::Command::cargo_bin("dirsql") + .expect("`dirsql` binary must be built with --features cli") + .arg("query") + .arg(sql) + .arg("--config") + .arg(root.path().join(".dirsql.toml")) + .current_dir(root.path()) + .output() + .expect("spawning `dirsql query` failed") +} + +fn rows(out: &Output) -> Vec { + assert!( + out.status.success(), + "query must succeed, stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + serde_json::from_slice(&out.stdout).expect("stdout must be JSON") +} + +/// A batch declaring the row table, a B-tree index over it, an FTS5 index and +/// the trigger that fills it — every statement in one `ddl` key. +const BATCH_CONFIG: &str = r#" +[[table]] +name = "records" +glob = "data/*.json" +on-file = "cat {path}" +ddl = ''' +CREATE TABLE records (id TEXT, body TEXT); +CREATE INDEX records_id ON records(id); +CREATE VIRTUAL TABLE records_fts USING fts5(body, content='records', content_rowid='rowid'); +CREATE TRIGGER records_ai AFTER INSERT ON records BEGIN + INSERT INTO records_fts(rowid, body) VALUES (new.rowid, new.body); +END; +''' +"#; + +#[test] +fn a_multi_statement_batch_indexes_and_queries_through_the_cli() { + let root = fixture(BATCH_CONFIG); + + let out = query(&root, "SELECT id FROM records ORDER BY id"); + let parsed = rows(&out); + let ids: Vec<&str> = parsed.iter().map(|r| r["id"].as_str().unwrap()).collect(); + assert_eq!(ids, ["one", "two"]); + + let out = query(&root, "SELECT name FROM pragma_index_list('records')"); + let parsed = rows(&out); + let names: Vec = parsed + .iter() + .map(|r| r["name"].as_str().unwrap().to_string()) + .collect(); + assert!( + names.iter().any(|n| n == "records_id"), + "the batch's CREATE INDEX must have run, got {names:?}" + ); +} + +#[test] +fn keyword_search_over_an_fts5_index_declared_in_ddl() { + let root = fixture(BATCH_CONFIG); + + let out = query( + &root, + "SELECT body FROM records_fts WHERE records_fts MATCH 'hello'", + ); + let parsed = rows(&out); + let bodies: Vec<&str> = parsed.iter().map(|r| r["body"].as_str().unwrap()).collect(); + assert_eq!( + bodies, + ["hello world"], + "the insert trigger must have filled the FTS5 index" + ); +} + +#[test] +fn a_batch_sqlite_rejects_exits_nonzero_naming_the_table() { + let root = fixture( + r#" +[[table]] +name = "records" +glob = "data/*.json" +on-file = "cat {path}" +ddl = ''' +CREATE TABLE records (id TEXT, body TEXT); +CREATE TABLE oops ( +''' +"#, + ); + + let out = query(&root, "SELECT id FROM records"); + assert!( + !out.status.success(), + "a batch SQLite rejects must exit non-zero" + ); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!( + stderr.contains("table 'records'") && stderr.contains("incomplete input"), + "stderr must prefix SQLite's raw error with the config entry, got {stderr:?}" + ); +} + +#[test] +fn a_declared_name_that_is_a_virtual_table_exits_nonzero() { + let root = fixture( + r#" +[[table]] +name = "records" +glob = "data/*.json" +on-file = "cat {path}" +ddl = "CREATE VIRTUAL TABLE records USING fts5(id, body)" +"#, + ); + + let out = query(&root, "SELECT id FROM records"); + assert!( + !out.status.success(), + "a declared table that is virtual must exit non-zero" + ); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!( + stderr.contains("table 'records'") && stderr.contains("virtual"), + "stderr must name the entry and say the table is virtual, got {stderr:?}" + ); +} diff --git a/packages/rust/tests/sdk.rs b/packages/rust/tests/sdk.rs index daea35784..0e9908e22 100644 --- a/packages/rust/tests/sdk.rs +++ b/packages/rust/tests/sdk.rs @@ -786,8 +786,20 @@ fn wait_file_events_after_watch_errors() { fn ddl_sqlite_rejects_errors() { let root = TempDir::new().unwrap(); let table = Table::new("t", "THIS IS NOT A CREATE TABLE", "*.txt", |_| vec![]); - let result = DirSQL::new(root.path(), vec![table]); - assert!(matches!(result, Err(dirsql::DirSqlError::Core(_)))); + let message = match DirSQL::new(root.path(), vec![table]) { + Ok(_) => panic!("a `ddl` SQLite rejects must fail the build"), + Err(err) => { + assert!( + matches!(err, dirsql::DirSqlError::TableDdl { .. }), + "got: {err}" + ); + err.to_string() + } + }; + assert!( + message.starts_with("table 't': "), + "SQLite's error must carry the config entry that caused it, got {message:?}" + ); } /// A `ddl` that runs but creates something else is caught against SQLite's @@ -804,7 +816,7 @@ fn declared_name_absent_from_the_catalog_errors() { let result = DirSQL::new(root.path(), vec![table]); assert!(matches!( result, - Err(dirsql::DirSqlError::TableNotCreated { ref name }) if name == "messages" + Err(dirsql::DirSqlError::TableNotCreated { ref name, .. }) if name == "messages" )); } diff --git a/packages/ts/e2e-attestations/claude-tackle-956-xzen74.json b/packages/ts/e2e-attestations/claude-tackle-956-xzen74.json new file mode 100644 index 000000000..5f5c314db --- /dev/null +++ b/packages/ts/e2e-attestations/claude-tackle-956-xzen74.json @@ -0,0 +1,7 @@ +{ + "command": "pnpm test:e2e", + "ran_at": 1787258854, + "exit_code": 0, + "commit": "4a0f0aa55a6d9fcb3be9ee4955839d9e2d66eb48", + "branch": "claude/tackle-956-xzen74" +}