From 7231920c49e14c170678b79fc59d1264cd6fb5d4 Mon Sep 17 00:00:00 2001 From: Yusuf GUNDOGDU Date: Fri, 18 Sep 2026 17:59:46 +0300 Subject: [PATCH 01/19] fix(editor): refuse an inline edit whose key would write to more than one row The key an inline edit builds its WHERE on is a guess: the first field called id or ending in _id. On a result that carries a foreign key rather than the table's own key - SELECT category_id, product_name FROM products - the guess lands on category_id, and the UPDATE rewrites every product in that category. Measured on PostgreSQL 16: one cell edited, fifteen rows changed, one statement reported as accepted. Two questions now stand between an edit and a write, and both had to be asked. IS IT THE TABLE'S COLUMN. A name is not a provenance. SELECT ROW_NUMBER() OVER (ORDER BY product_name) AS product_id, product_name FROM products puts 1, 2, 3 in a field called product_id; products really has a product_id; and the UPDATEs land on whichever products those are, not on the rows on screen. Measured, two cells edited, two rows written, neither of them visible, both reported accepted. SELECT sku AS product_id is the same thing spelled shorter. selectsPlainColumn reads the select list and requires the field to be the column itself - a star, or a reference whose alias, if any, names what it already names. DOES IT ADDRESS ONE ROW PER VALUE. A grouped count over the distinct keys, bound rather than interpolated: as many distinct keys as rows on screen, as many groups back as distinct keys, every group holding one row. Each clause replaced a version measured letting a write through. Counting per row sent IN (87, 87, 87) for three rows sharing order_id 87 and wrote all three to all three. An ungrouped total passed abc and ABC on MySQL's case-insensitive collation, where WHERE k = 'abc' writes to both. String() alone collapsed SQLite's text '1' and integer 1, and two MySQL BIGINTs past 2^53 that mysql2 rounds to the same number. The check goes to /api/db/transaction when a transaction is open, because that is where the UPDATEs go: asked on a pooled connection it cannot see a row the transaction has not committed, and the apply refuses for ever with a sentence that is false about a row the user is looking at. It asks for one more row than there are keys, because the default page is 500 and groups that fell off would read as rows that are gone. A key that cannot be read as text at all - a value with a null prototype - is refused before anything is built, where it used to throw as the statement was assembled and take the apply down with no write and no message. A null key is refused too. Fewer rows than keys gets its own sentence, because it says nothing about whether the column tells them apart. A check that could not be run is not a check that passed, and the pending edits are kept in every case. --- SECURITY.md | 6 +- docs/FEATURES.md | 6 +- src/components/Studio.tsx | 1 + src/hooks/use-inline-editing.ts | 211 +++++++- src/lib/sql/update-target.ts | 115 ++++ tests/hooks/use-inline-editing.test.ts | 693 +++++++++++++++++++++++++ tests/unit/sql/update-target.test.ts | 91 +++- 7 files changed, 1115 insertions(+), 8 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index 15beddbea..b7b999d9d 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -120,7 +120,11 @@ When using LibreDB Studio, please follow these security best practices: that fetched the rows on screen, read under the connection's own dialect, and is refused whenever that statement's rows have no single table or the reader cannot settle the name. An unquoted name is validated as a bare identifier rather than quoted, because quoting changes its - case semantics; a quoted one is copied exactly as the query spells it + case semantics; a quoted one is copied exactly as the query spells it. The key column the + `WHERE` is built on is inferred as well, from the result's own field names, so the apply asks + the engine whether that column addresses one row per value and refuses the whole apply when it + does not: a result carrying a foreign key instead of the table's own key made one cell edit + rewrite every row sharing that value - Login attempts, the AI endpoints and every database-reaching route (query execution, schema browsing, maintenance operations, and the admin fleet-health check) are rate limited in the application. The counters live in the application process, so with more than one replica the diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 4dfa1c1e4..90bd8a0d3 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -21,7 +21,7 @@ ### 3. Pro Data Grid (Excel-Style) * **High Performance:** Virtualized rendering using TanStack Virtual for smooth scrolling through millions of rows. -* **Inline Editing:** Double-click any cell to edit data directly; apply pending cell changes as one `UPDATE` per edited row or discard them. The table written to is the one the *statement that fetched the rows* names, not the tab's title. A query whose rows have no single table - a join, a comma-separated `FROM`, a subquery, a CTE, a set operation - is refused with a reason rather than guessed at. Offered only where the provider declares `supportsInlineRowEdit`. ClickHouse, Druid, Elasticsearch, OpenSearch, Trino, Cassandra, MongoDB, Redis and LibreDB show no editing control at all because they have no single-table row update - on Cassandra because CQL requires the WHOLE primary key restricted by equality while the editor names one column it guessed from the result fields, so a clustered table answers "Some partition key parts are missing" (measured) — on Trino because it declares no primary key for any table in any catalog, so the generated `WHERE` could not identify one row — on the two search engines `UPDATE` is absent from the SQL grammar itself, measured on both; Couchbase shows none because the document key reaches the grid as a projection alias the generated `WHERE` cannot address. +* **Inline Editing:** Double-click any cell to edit data directly; apply pending cell changes as one `UPDATE` per edited row or discard them. The table written to is the one the *statement that fetched the rows* names, not the tab's title. A query whose rows have no single table - a join, a comma-separated `FROM`, a subquery in `FROM` or in the select list, a CTE, a set operation - is refused with a reason rather than guessed at. The key the `WHERE` is built on is a guess off the result's own fields, so before anything is written the apply asks the engine whether that column addresses one row per value and refuses the whole apply when it does not: on a result carrying a foreign key rather than the table's own key, one cell edit used to rewrite every row sharing that value. Offered only where the provider declares `supportsInlineRowEdit`. ClickHouse, Druid, Elasticsearch, OpenSearch, Trino, Cassandra, MongoDB, Redis and LibreDB show no editing control at all because they have no single-table row update - on Cassandra because CQL requires the WHOLE primary key restricted by equality while the editor names one column it guessed from the result fields, so a clustered table answers "Some partition key parts are missing" (measured) — on Trino because it declares no primary key for any table in any catalog, so the generated `WHERE` could not identify one row — on the two search engines `UPDATE` is absent from the SQL grammar itself, measured on both; Couchbase shows none because the document key reaches the grid as a projection alias the generated `WHERE` cannot address. * **Data-Type Formatting:** Specialized rendering for Numbers, Booleans, and Nulls. * **Column Management:** Resizable columns and advanced sorting. * **Row Detail:** A control at the left edge of every row opens that row field by field, values beside field names, with per-field copy and the same masking the grid applies. @@ -140,8 +140,8 @@ ### 18. The Database Agent (read-only investigation runs) * **A run, not a chat:** you state an objective and press Start; the run drafts SQL against the connected database, reads the results, and composes a report whose every claim cites the result it came from. An uncited claim is refused, so it cannot be composed at all. -* **Read-only, enforced by the database:** every statement the agent runs goes through the agent's own audited pipeline — a policy decision, an audit event and budget accounting before the driver is touched, through `executeAuditedOperation()` ([`execution.ts`](../src/lib/db/operations/execution.ts)) — under a read-only execution profile: a read-only transaction on PostgreSQL, `PRAGMA query_only` re-asserted per statement on SQLite, a `READ_ONLY` engine handle plus an SQL-level guard on DuckDB — the flag alone is not a filesystem sandbox, since `COPY … TO`, `EXPORT DATABASE`, `INSTALL`/`LOAD` and the local-file table functions all succeed under it. On SQL Server the profile is four layers instead of one, because the engine has no read-only transaction and no session-level read-only switch: a session principal verified at open to be unable to write or to reach the server's dangerous surfaces, an admission step that asks the optimizer to compile each statement without running it, a server-side row bound (`SET ROWCOUNT`) that stops an unbounded read before the result is materialised at all, and a pinned transaction that is always rolled back. Writes and DDL are refused before the database is reached, and `EXPLAIN ANALYZE` is default-denied because it would execute the statement. The pipeline is the agent's alone and is not shared with the editor: a statement you run yourself calls the provider directly in `POST()` ([`query/route.ts`](../src/app/api/db/query/route.ts)), receiving neither the policy decision nor the audit event. -* **Agent mode is PostgreSQL, SQLite, DuckDB and SQL Server only — except Operate:** the read-only profile is database-native, so it exists only where a provider implements `queryReadOnly` — [`postgres.ts`](../src/lib/db/providers/sql/postgres.ts), [`sqlite.ts`](../src/lib/db/providers/sql/sqlite.ts), [`duckdb/index.ts`](../src/lib/db/providers/sql/duckdb/index.ts) and [`mssql.ts`](../src/lib/db/providers/sql/mssql.ts), and no other provider does. On MySQL, Oracle, libSQL, MongoDB, Redis, ClickHouse, Druid, Couchbase, Elasticsearch, OpenSearch, Trino, Cassandra or the embedded LibreDB store an Agent-mode run whose workflow sends statements is refused by `POST /api/agent/runs` before a run id exists, and one that reaches the provider factory ends `engine-unsupported` in `driveAgentRun()` ([`runtime.ts`](../src/lib/agent/runtime.ts)). The search providers implement no `queryReadOnly` and could not: their SQL grammars have no transaction and no session-scoped setting to make read-only, and the surface is already read-only in the grammar itself, which is a different guarantee from one the database enforces per statement. The **Operate** workflow is the exception and runs on every engine, because it sends no SQL at all: it reads the engine's own reporting interface, which every provider implements. Plan mode opens on every connection: its model is handed no tools, so no read-only profile has to be acquired for it. It is not blind, though — since 2026-08-15 the server reads the connection's schema and the engine's own estimated statistics before the model's first turn. That **grounding** reaches every engine: on PostgreSQL and SQLite the server composes catalog statements and reads them through that same read-only path, and on every other connection it asks the provider to describe its own schema — the reading the sidebar already performs when it lists your tables, which needs no read-only statement path. So the two limits are separate ones: agent mode is those four engines, grounding is all of them, and a run whose reading fails — refused, overran its time, or rejected by the engine — says so rather than inventing tables. +* **Read-only, enforced by the database:** every statement the agent runs goes through the agent's own audited pipeline — a policy decision, an audit event and budget accounting before the driver is touched, through `executeAuditedOperation()` ([`execution.ts`](../src/lib/db/operations/execution.ts)) — under a read-only execution profile: a read-only transaction on PostgreSQL, `PRAGMA query_only` re-asserted per statement on SQLite, and a `READ_ONLY` engine handle plus an SQL-level guard on DuckDB — the flag alone is not a filesystem sandbox, since `COPY … TO`, `EXPORT DATABASE`, `INSTALL`/`LOAD` and the local-file table functions all succeed under it. Writes and DDL are refused before the database is reached, and `EXPLAIN ANALYZE` is default-denied because it would execute the statement. The pipeline is the agent's alone and is not shared with the editor: a statement you run yourself calls the provider directly in `POST()` ([`query/route.ts`](../src/app/api/db/query/route.ts)), receiving neither the policy decision nor the audit event. +* **Agent mode is PostgreSQL, SQLite and DuckDB only — except Operate:** the read-only profile is database-native, so it exists only where a provider implements `queryReadOnly` — [`postgres.ts`](../src/lib/db/providers/sql/postgres.ts), [`sqlite.ts`](../src/lib/db/providers/sql/sqlite.ts) and [`duckdb/index.ts`](../src/lib/db/providers/sql/duckdb/index.ts), and no other provider does. On MySQL, Oracle, SQL Server, libSQL, MongoDB, Redis, ClickHouse, Druid, Couchbase, Elasticsearch, OpenSearch, Trino or Cassandra an Agent-mode run ends `engine-unsupported` in `driveAgentRun()` ([`runtime.ts`](../src/lib/agent/runtime.ts)). The search providers implement no `queryReadOnly` and could not: their SQL grammars have no transaction and no session-scoped setting to make read-only, and the surface is already read-only in the grammar itself, which is a different guarantee from one the database enforces per statement. The **Operate** workflow is the exception and runs on every engine, because it sends no SQL at all: it reads the engine's own reporting interface, which every provider implements. Plan mode opens on every connection: its model is handed no tools, so no read-only profile has to be acquired for it. It is not blind, though — since 2026-08-15 the server reads the connection's schema and the engine's own estimated statistics before the model's first turn. That **grounding** reaches every engine: on PostgreSQL and SQLite the server composes catalog statements and reads them through that same read-only path, and on every other connection it asks the provider to describe its own schema — the reading the sidebar already performs when it lists your tables, which needs no read-only statement path. So the two limits are separate ones: agent mode is those two engines, grounding is all of them, and a run whose reading fails — refused, overran its time, or rejected by the engine — says so rather than inventing tables. * **Two independent axes:** the **mode** (Plan, whose model is toolless and whose deliverable is one statement for you to run yourself — the run executes no statement of yours and writes nothing — or Agent) and the **workflow** (Investigate, Optimize, Assess, Operate, Analyze). Both are fixed when the run opens and read from the run's own record thereafter. * **Operate reads the live server, not its tables:** the slowest queries, who is connected and what is blocked, table and index statistics, storage and health — each a curated reading the server takes through the provider's own reporting interface, stored as an ordinary citable artifact. Every reading is a point in time, and both the prompt and the timeline say so rather than letting a report imply a trend was measured. * **Counts, never values:** the Assess workflow's table profiling composes aggregates only — row counts, present counts, distinct counts, and shape matches computed inside the database. There is deliberately no `min`/`max`, because on a text column those return real values. diff --git a/src/components/Studio.tsx b/src/components/Studio.tsx index aaa7ef008..eb04b8309 100644 --- a/src/components/Studio.tsx +++ b/src/components/Studio.tsx @@ -409,6 +409,7 @@ export default function Studio() { activeConnection: conn.activeConnection, currentTab: tabMgr.currentTab, executeQuery: queryExec.executeQuery, + transactionActive: txn.transactionActive, }); // Inline row editing is offered only where the provider declares the row-update diff --git a/src/hooks/use-inline-editing.ts b/src/hooks/use-inline-editing.ts index 9fa5efdc6..39186741e 100644 --- a/src/hooks/use-inline-editing.ts +++ b/src/hooks/use-inline-editing.ts @@ -5,12 +5,25 @@ import type { DatabaseConnection, QueryTab } from "@/lib/types"; import type { CellChange } from "@/components/ResultsGrid"; import { useToast } from "@/hooks/use-toast"; import { quoteIdentifier } from "@/lib/sql/identifier"; -import { resolveUpdateTarget } from "@/lib/sql/update-target"; +import { resolveUpdateTarget, selectsPlainColumn } from "@/lib/sql/update-target"; import { positionalPlaceholder, quoteLiteral } from "@/lib/sql/values"; +import { appFetch } from "@/lib/config/base-path"; +import { buildConnectionPayload } from "@/hooks/use-connection-payload"; interface UseInlineEditingParams { activeConnection: DatabaseConnection | null; currentTab: QueryTab; + /** + * Whether a transaction is open on this connection. + * + * The UPDATEs already follow it: `executeQuery` sends them to `/api/db/transaction`, + * which holds the one reserved connection the transaction lives on. The key check has to + * follow it too, or it asks a different pooled connection and cannot see anything the + * transaction has not committed. Measured: a row INSERTed inside an open transaction is + * on screen, invisible to the check, and the apply refuses for ever with "no longer in + * the table" - a sentence that is false, about rows the user is looking at. + */ + transactionActive?: boolean; /** * `useQueryExecution`'s `executeQuery`. `handleApplyChanges` awaits it between * rows and passes its execution options, so the signature carries both. @@ -34,7 +47,143 @@ const rows = (count: number) => `${count} row${count === 1 ? "" : "s"}`; /** The same, for the statements a run is made of. */ const updates = (count: number) => `${count} UPDATE statement${count === 1 ? "" : "s"}`; -export function useInlineEditing({ activeConnection, currentTab, executeQuery }: UseInlineEditingParams) { +/** + * Whether the column this editor found actually addresses ONE row per value. + * + * The key is a GUESS: the first field called `id` or ending in `_id`. On a result that + * carries a foreign key and not the table's own key — `SELECT category_id, product_name + * FROM products` — the guess lands on `category_id`, and the `UPDATE ... WHERE + * category_id = 5` that follows rewrites every product in that category. Measured on + * PostgreSQL 16 against the sample data: editing one cell changed FIFTEEN rows, and the + * apply reported one statement accepted, so nothing on screen said otherwise. Resolving + * the right TABLE (#881) does not help here; this is the right table and the wrong rows. + * + * One grouped count over the DISTINCT keys about to be written answers it for the whole + * apply: every group has to come back holding exactly one row, and there have to be as + * many groups as there are distinct keys. + * + * Distinct is the first word that matters. Counting the keys per ROW lets the defect + * straight back through: editing three rows that share `order_id` 87 sends `IN (87, 87, + * 87)`, the engine counts the three rows behind that one value, three equals three, and + * all three UPDATEs write to all three rows. Measured on the sample data — `order_items` + * has a composite key and the guess takes `order_id` — and editing a whole order's lines + * is the ordinary thing to do, so this needed no coincidence at all. + * + * GROUPED is the second, and a plain total would not have caught it: the engine decides + * what counts as the same key, not JavaScript. MySQL's default collation is + * case-insensitive, so two rows keyed `abc` and `ABC` are two distinct keys here and one + * key there. Measured on MySQL 8.4: `IN ('abc', 'ABC')` counts two rows, two equals two, + * and `WHERE k = 'abc'` then writes to BOTH. Grouped, the engine answers one group of two + * and the apply refuses. The same argument covers trailing spaces on CHAR columns and + * every other collation the engine applies and this side cannot see. + * + * A row that has gone missing is a different fact and gets a different sentence: fewer + * rows than keys says nothing about whether the column tells them apart. + * + * And the rows ON SCREEN have to answer as many keys as there are of them. Two grid rows + * that collapse to one key are not told apart by that column either, and this side cannot + * always see it: `bun:sqlite` hands back the text `'1'` and the integer `1` from the same + * dynamically typed column, and `mysql2` rounds a BIGINT past 2^53, so `9007199254740993` + * arrives as `...992` — the same number as its neighbour. Measured on both. In each case + * the engine was asked about ONE key, answered one group holding one row, and two UPDATEs + * then went out carrying the raw values the grid still held: a row the user never edited + * was overwritten and the apply reported success. So the dedup key carries the type as + * well as the text, and the number of edited rows has to equal the number of distinct keys. + * + * Refusing is what a failed check does: this exists to stop a write nobody asked for. + */ +async function keyAddressesOneRow( + connection: DatabaseConnection, + table: string, + keyColumn: string, + keys: readonly unknown[], + inTransaction: boolean, +): Promise<{ ok: true } | { ok: false; reason: string }> { + // A key with no value cannot be addressed by `=` at all, and `String(null)` would send + // the text "null" — which an integer column rejects, so the whole apply would fail on a + // driver error rather than on the reason. + if (keys.some((key) => key === null || key === undefined)) { + return { ok: false, reason: `a row you edited has no ${keyColumn}, so it cannot be addressed` }; + } + // Safe to read as text: the caller has already refused any key this would throw on. + const distinct = [...new Map(keys.map((key) => [`${typeof key}:${String(key)}`, key])).values()]; + if (distinct.length !== keys.length) { + return { + ok: false, + reason: + `This editor cannot tell these rows apart by ${keyColumn}: ${rows(keys.length)} on screen carry ` + + `${distinct.length === 1 ? "one value" : `only ${distinct.length} values`} between them. ` + + `Put a key that identifies a row in the query and run it again`, + }; + } + + const dialect = connection.type; + const params: unknown[] = []; + const placeholders = distinct.map((key) => { + const placeholder = positionalPlaceholder(dialect, params.length + 1); + if (placeholder !== null) { + params.push(typeof key === "number" ? key : String(key)); + return placeholder; + } + return typeof key === "number" ? String(key) : quoteLiteral(String(key), dialect); + }); + const key = quoteIdentifier(keyColumn, dialect); + const sql = `SELECT ${key}, COUNT(*) FROM ${table} WHERE ${key} IN (${placeholders.join(", ")}) GROUP BY ${key}`; + + let data: { rows?: Record[]; error?: string }; + try { + const res = await appFetch(inTransaction ? "/api/db/transaction" : "/api/db/query", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + ...buildConnectionPayload(connection), + ...(inTransaction && { action: "query" }), + sql, + // A limit the answer cannot reach: one group per distinct key, and the keys are the + // rows a person edited by hand. Left to the default the answer would be cut at 500 + // and the missing groups would read as missing ROWS, which is a refusal with a false + // reason attached. + options: { limit: distinct.length + 1 }, + ...(params.length > 0 && { params }), + }), + }); + // A proxy answering HTML rather than JSON would throw here, and the catch below is + // what turns that into a refusal instead of an unhandled rejection. + data = await res.json(); + if (!res.ok) return { ok: false, reason: data.error ?? "the check could not be run" }; + } catch (err) { + return { ok: false, reason: err instanceof Error ? err.message : String(err) }; + } + + // `/api/db/query` answers rows as objects, always, and the name a bare `COUNT(*)` comes + // back under is the engine's business: `count` on PostgreSQL, `COUNT(*)` on MySQL and + // SQLite. So the count is read by POSITION — second value of each row, after the key — + // rather than by a name no dialect agrees on. PostgreSQL returns it as a STRING, which + // is why it goes through `Number`. + const groups = data.rows ?? []; + const counts = groups.map((row) => Number(Object.values(row)[1])); + if (counts.some((count) => !Number.isFinite(count))) { + return { ok: false, reason: "the check returned no count" }; + } + const matched = counts.reduce((total, count) => total + count, 0); + if (groups.length === distinct.length && counts.every((count) => count === 1)) return { ok: true }; + if (matched < distinct.length) { + return { ok: false, reason: "some of the rows you edited are no longer in the table. Run the query again" }; + } + return { + ok: false, + reason: + `${keyColumn} does not tell these rows apart in this table: the ${rows(distinct.length)} you edited ` + + `would write to ${rows(matched)}. Put the table's own key in the query and run it again`, + }; +} + +export function useInlineEditing({ + activeConnection, + currentTab, + executeQuery, + transactionActive = false, +}: UseInlineEditingParams) { const [editingEnabled, setEditingEnabled] = useState(false); const [pendingChanges, setPendingChanges] = useState([]); const { toast } = useToast(); @@ -145,6 +294,26 @@ export function useInlineEditing({ activeConnection, currentTab, executeQuery }: const dialect = activeConnection.type; const quote = (identifier: string) => quoteIdentifier(identifier, dialect); + // Every key has to survive being read as text before anything is built from it. A value + // with a null prototype has no `toString`, and `String()` throws on it - which happened + // where the statement is assembled, so the apply died as an unhandled rejection with no + // write and no toast either. Asked here, it is a refusal like any other. + const keysByRow = new Map(); + for (const rowIndex of changesByRow.keys()) { + const value = currentTab.result.rows[rowIndex]?.[pkColumn]; + try { + void String(value); + } catch { + toast({ + title: "Cannot Apply Changes", + description: `This editor cannot read the ${pkColumn} of every row it would write to. Edit the SQL manually.`, + variant: "destructive", + }); + return; + } + keysByRow.set(rowIndex, value); + } + // Generate UPDATE statements const statements: Array<{ sql: string; params: unknown[]; rowIndex: number }> = []; for (const [rowIndex, changes] of changesByRow) { @@ -188,6 +357,42 @@ export function useInlineEditing({ activeConnection, currentTab, executeQuery }: }); } + // Before anything is written: is that key column the TABLE's, and does it address one + // row per value? The first question comes first because it decides whether the second + // one is even being asked about the right thing: a key that is an expression or a + // rename sends the check to a real column the grid never showed, and every answer it + // gives is about rows nobody is looking at. + if (!selectsPlainColumn(currentTab.resultQuery ?? currentTab.query, pkColumn, activeConnection.type)) { + toast({ + title: "Cannot Apply Changes", + description: `${pkColumn} is not read straight from the table here, so it cannot identify a row to write to. Edit the SQL manually.`, + variant: "destructive", + }); + return; + } + + // And then: does that key column address one row per value? + // `pkColumn` is a guess off the field list, and on a result carrying a foreign key + // rather than the table's own key it aims at the foreign key — one cell edit then + // rewrote fifteen rows, reported as one statement accepted. + const uniqueness = await keyAddressesOneRow( + activeConnection, + tableName, + pkColumn, + // The RAW cell values. Converting here would turn a missing key into the text + // "null" before the check could see it was missing. + statements.map((statement) => keysByRow.get(statement.rowIndex)), + transactionActive, + ); + if (!uniqueness.ok) { + toast({ + title: "Cannot Apply Changes", + description: `${uniqueness.reason}.`, + variant: "destructive", + }); + return; + } + // One request per row (issue #269), sequentially and with the safety dialog // skipped. Each part matters: // - per row, because a joined payload reaches the engine as ONE string whenever @@ -273,7 +478,7 @@ export function useInlineEditing({ activeConnection, currentTab, executeQuery }: ? `${updates(statements.length)} accepted. Run the query again to see the saved rows.` : `${updates(statements.length)} accepted. The results are up to date.`, }); - }, [activeConnection, currentTab, pendingChanges, executeQuery, toast]); + }, [activeConnection, currentTab, pendingChanges, executeQuery, toast, transactionActive]); const handleDiscardChanges = useCallback(() => { setPendingChanges([]); diff --git a/src/lib/sql/update-target.ts b/src/lib/sql/update-target.ts index 922aa518d..b6156cfac 100644 --- a/src/lib/sql/update-target.ts +++ b/src/lib/sql/update-target.ts @@ -368,6 +368,121 @@ function opensASubquery(pieces: Piece[], fromOffset: number, type?: DatabaseType * Only the shape is read. The statement is not executed and no part of it other than the * table reference is copied anywhere. */ +/** + * Whether `column` reaches the grid straight off the base table, rather than through an + * expression or a rename. + * + * The key an inline edit writes against is picked by NAME off the result's field list, and + * a name is not a provenance. `SELECT ROW_NUMBER() OVER (ORDER BY product_name) AS + * product_id, product_name FROM products` puts 1, 2, 3 in a column called `product_id`; + * `products` really has a `product_id`; and the `UPDATE ... WHERE product_id = 1` that + * follows lands on whichever product that is, not on the row anybody was looking at. + * Measured against PostgreSQL 16: two cells edited, two rows written, neither of them the + * ones on screen, and the apply reported both as accepted. `SELECT sku AS product_id` + * is the same defect spelled shorter. + * + * `*` is the safe case and the common one: every field is the table's own. Otherwise the + * item that produces this name has to BE the column - one identifier, or a qualified one, + * and any `AS` on it has to name the column it already names. + * + * Refusing when the shape cannot be read, like everything else here: a key this reader + * cannot vouch for is one it should not let a write aim with. + */ +export function selectsPlainColumn(sql: string, column: string, type?: DatabaseType): boolean { + const pieces = readPieces(sql, type); + if (typeof pieces === "string") return false; + const top = pieces.filter((piece) => piece.depth === 0); + + const select = top.findIndex((piece) => isWord(piece, "SELECT")); + const from = top.findIndex((piece) => isWord(piece, "FROM")); + if (select === -1 || from === -1 || from < select) return false; + + // `DISTINCT`, `ALL`, and DuckDB's and PostgreSQL's `DISTINCT ON (...)` sit between the + // keyword and the list. They say how many rows come back, not where a field comes from. + let start = select + 1; + if (isWord(top[start], "ALL") || isWord(top[start], "DISTINCT")) { + const distinct = isWord(top[start], "DISTINCT"); + start++; + if (distinct && isWord(top[start], "ON")) { + start++; + // The parenthesised list is one `(` and one `)` at this level; its contents are deeper. + if (top[start]?.kind === "other" && top[start].text === "(") { + start++; + while (start < from && !(top[start].kind === "other" && top[start].text === ")")) start++; + start++; + } + } + } + + // The select list, split on its own commas. A comma inside parens belongs to a function's + // arguments, and those pieces are not at this depth to begin with. + const items: Piece[][] = [[]]; + for (const piece of top.slice(start, from)) { + if (piece.kind === "other" && piece.text === ",") items.push([]); + else items[items.length - 1].push(piece); + } + + const named = (piece: Piece) => (piece.kind === "quoted" ? sql.slice(piece.start + 1, piece.end - 1) : piece.text); + const sameName = (a: string, b: string) => a === b || a.toLowerCase() === b.toLowerCase(); + const isName = (piece: Piece | undefined) => + piece !== undefined && (piece.kind === "name" || piece.kind === "quoted"); + + /** + * `*` on its own, or `t.*`. Not any `*` anywhere in the item: `ROW_NUMBER() OVER (...) * 1` + * multiplies, and reading that as a star let a computed field pass as the table's own - + * measured, and it is the whole defect this function exists to stop. + */ + const isStar = (item: Piece[]) => { + const star = (piece: Piece | undefined) => piece?.kind === "other" && piece.text === "*"; + if (item.length === 1) return star(item[0]); + return item.length === 3 && isName(item[0]) && item[1].kind === "other" && item[1].text === "." && star(item[2]); + }; + + // The LAST item that produces this name is the one the grid reads. Drivers build a row + // object keyed by field name, so `SELECT product_id, sku AS product_id` hands over sku's + // value under that name - measured on node-postgres - and deciding on the first match + // would vouch for a column the user never sees. + let answer = false; + for (const item of items) { + if (item.length === 0) continue; + + if (isStar(item)) { + // Every one of the table's columns, this one included, unless a later item renames + // over it. + answer = true; + continue; + } + + // Strip a trailing alias, with or without the keyword. + let body = item; + let alias: string | null = null; + const last = item[item.length - 1]; + if (item.length >= 2 && isName(last)) { + if (isWord(item[item.length - 2], "AS")) { + alias = named(last); + body = item.slice(0, item.length - 2); + } else if (isName(item[item.length - 2])) { + // Two identifiers side by side is an alias with the keyword left out. + alias = named(last); + body = item.slice(0, item.length - 1); + } + } + + // A plain reference is a chain of identifiers joined by dots: `c`, `t.c`, `s.t.c`. + const isReference = + body.length > 0 && + body.length % 2 === 1 && + body.every((piece, index) => (index % 2 === 0 ? isName(piece) : piece.kind === "other" && piece.text === ".")); + + const source = isReference ? named(body[body.length - 1]) : null; + const output = alias ?? source; + if (output === null || !sameName(output, column)) continue; + // An alias that renames is a different column wearing this name. + answer = source !== null && sameName(source, column); + } + return answer; +} + export function resolveUpdateTarget(sql: string, type?: DatabaseType): UpdateTarget { const pieces = readPieces(sql, type); if (pieces === "unterminated") { diff --git a/tests/hooks/use-inline-editing.test.ts b/tests/hooks/use-inline-editing.test.ts index e94c102f8..54ff0bbcb 100644 --- a/tests/hooks/use-inline-editing.test.ts +++ b/tests/hooks/use-inline-editing.test.ts @@ -60,10 +60,47 @@ const makeChange = (overrides: Partial = {}): CellChange => ({ describe("useInlineEditing", () => { let mockExecuteQuery: ReturnType; + /** + * The key check an apply now makes before it writes anything: one `COUNT(*)` over the + * keys about to be updated, which has to come back equal to how many there are. By + * default it does, so every test below is about what it was about before. The tests that + * are about the check answer it themselves. + */ + function answerKeyCheck(matched?: number) { + globalThis.fetch = mock((_url: string, init?: RequestInit) => { + const body = JSON.parse(String(init?.body ?? "{}")); + void body; + // The shape the product actually answers with: `/api/db/query` returns rows as + // OBJECTS, and PostgreSQL reports a bare `COUNT(*)` as the string "1" under a column + // it names `count`. A mock returning `[[1]]` would exercise a branch the product + // never takes, and line coverage would not notice. + // + // One group per key, each holding one row, which is the shape that passes. Pass + // `matched` to answer a single group of that many rows instead — the key that does + // not tell its rows apart. + const body2 = JSON.parse(String(init?.body ?? "{}")); + // Where the dialect has no positional bind form the values are written into the + // statement instead, so there is no `params` to count: one group is the right answer + // for the single key those tests edit. + const bound = (body2.params ?? [1]) as unknown[]; + const answer = + matched === undefined + ? bound.map((key) => ({ id: key, count: "1" })) + : matched === 0 + ? [] + : [{ id: bound[0], count: String(matched) }]; + return Promise.resolve({ + ok: true, + json: () => Promise.resolve({ rows: answer, fields: ["id", "count"], rowCount: answer.length }), + }); + }) as unknown as typeof fetch; + } + beforeEach(() => { mockExecuteQuery = mock(() => {}); mockToastSuccess.mockClear(); mockToastError.mockClear(); + answerKeyCheck(); }); afterEach(() => { @@ -1294,4 +1331,660 @@ describe("useInlineEditing", () => { description: expect.stringContaining("no longer on screen"), }); }); + + // ── The key has to address one row ──────────────────────────────────────── + + test("refuses the whole apply when the key it found is not unique", async () => { + // The measured defect: a result carrying `category_id` and not `product_id` makes the + // guess land on the foreign key, and `UPDATE ... WHERE category_id = 5` rewrites every + // product in that category. Fifteen rows on the sample data, reported as one statement + // accepted. Nothing is written, and the reason names the column and both counts. + answerKeyCheck(15); + const { result } = renderHook(() => + useInlineEditing({ + activeConnection: makeConnection(), + currentTab: makeTab({ + result: makeResult({ + rows: [{ category_id: 5, product_name: "Chai" }], + fields: ["category_id", "product_name"], + rowCount: 1, + }), + query: "SELECT category_id, product_name FROM products", + resultQuery: "SELECT category_id, product_name FROM products", + }), + executeQuery: mockExecuteQuery, + }), + ); + + act(() => { + result.current.handleCellChange({ + rowIndex: 0, + columnId: "product_name", + originalValue: "Chai", + newValue: "Chai Reserve", + }); + }); + await act(async () => { + await result.current.handleApplyChanges(); + }); + + expect(updateCalls()).toHaveLength(0); + expect(mockToastError).toHaveBeenCalledWith("Cannot Apply Changes", { + description: expect.stringContaining("category_id does not tell these rows apart"), + }); + expect(mockToastError).toHaveBeenCalledWith("Cannot Apply Changes", { + description: expect.stringContaining("15 rows"), + }); + // The work stays on screen: nothing was written, so nothing is discarded. + expect(result.current.pendingChanges).toHaveLength(1); + }); + + test("refuses when that check cannot be run at all", async () => { + globalThis.fetch = mock(() => + Promise.resolve({ ok: false, json: () => Promise.resolve({ error: "connection refused" }) }), + ) as unknown as typeof fetch; + const { result } = renderHook(() => + useInlineEditing({ + activeConnection: makeConnection(), + currentTab: makeTab(), + executeQuery: mockExecuteQuery, + }), + ); + + act(() => { + result.current.handleCellChange(makeChange()); + }); + await act(async () => { + await result.current.handleApplyChanges(); + }); + + // A check that did not run is not a check that passed. + expect(updateCalls()).toHaveLength(0); + expect(mockToastError).toHaveBeenCalledWith("Cannot Apply Changes", { + description: expect.stringContaining("connection refused"), + }); + expect(result.current.pendingChanges).toHaveLength(1); + }); + + test("refuses N rows that share one foreign key, without needing to ask the engine", async () => { + // The hole the first version of this check left open, and the one that matters most: + // three rows sharing `order_id` 87 sent `IN (87, 87, 87)`, the engine counted the three + // rows behind that one value, three equalled three, and all three UPDATEs wrote to all + // three rows. Measured on the sample data. Three rows on screen carrying one key + // between them is already the answer, so this refuses before any request goes out. + const seen: Array<{ params: unknown[] }> = []; + globalThis.fetch = mock((_url: string, init?: RequestInit) => { + seen.push({ params: JSON.parse(String(init?.body ?? "{}")).params ?? [] }); + return Promise.resolve({ + ok: true, + json: () => + Promise.resolve({ rows: [{ order_id: 87, count: "3" }], fields: ["order_id", "count"], rowCount: 1 }), + }); + }) as unknown as typeof fetch; + + const { result } = renderHook(() => + useInlineEditing({ + activeConnection: makeConnection(), + currentTab: makeTab({ + result: makeResult({ + rows: [ + { order_id: 87, quantity: 1 }, + { order_id: 87, quantity: 2 }, + { order_id: 87, quantity: 3 }, + ], + fields: ["order_id", "quantity"], + rowCount: 3, + }), + resultQuery: "SELECT order_id, quantity FROM order_items", + }), + executeQuery: mockExecuteQuery, + }), + ); + + act(() => { + for (let i = 0; i < 3; i++) { + result.current.handleCellChange({ rowIndex: i, columnId: "quantity", originalValue: i + 1, newValue: "9" }); + } + }); + await act(async () => { + await result.current.handleApplyChanges(); + }); + + // Nothing was asked of the engine at all. + expect(seen).toHaveLength(0); + expect(updateCalls()).toHaveLength(0); + expect(mockToastError).toHaveBeenCalledWith("Cannot Apply Changes", { + description: expect.stringContaining("cannot tell these rows apart by order_id"), + }); + expect(mockToastError).toHaveBeenCalledWith("Cannot Apply Changes", { + description: expect.stringContaining("3 rows on screen carry one value between them"), + }); + expect(result.current.pendingChanges).toHaveLength(3); + }); + + test("refuses two rows whose keys arrived identical, whatever they are in the table", async () => { + // Measured on MySQL 8.4 through the product's own query route: `mysql2` rounds a BIGINT + // past 2^53, so a table holding 9007199254740992 and ...993 sends BOTH to the browser as + // ...992. One key would reach the engine, it would answer one group of one row, and two + // UPDATEs would then go out with the same WHERE - one row taking the other's value and + // the other never written, reported as two statements accepted. Two rows on screen + // carrying one key between them is the answer on its own, before anything is asked. + const seen: unknown[] = []; + globalThis.fetch = mock((_url: string, init?: RequestInit) => { + seen.push(init); + return Promise.resolve({ + ok: true, + json: () => Promise.resolve({ rows: [{ id: 1, count: "1" }], fields: ["id", "count"], rowCount: 1 }), + }); + }) as unknown as typeof fetch; + + const { result } = renderHook(() => + useInlineEditing({ + activeConnection: makeConnection({ type: "mysql" }), + currentTab: makeTab({ + result: makeResult({ + rows: [ + { id: 9007199254740992, note: "first" }, + { id: 9007199254740992, note: "second" }, + ], + fields: ["id", "note"], + rowCount: 2, + }), + resultQuery: "SELECT id, note FROM big", + }), + executeQuery: mockExecuteQuery, + }), + ); + + act(() => { + result.current.handleCellChange({ rowIndex: 0, columnId: "note", originalValue: "first", newValue: "x" }); + result.current.handleCellChange({ rowIndex: 1, columnId: "note", originalValue: "second", newValue: "y" }); + }); + await act(async () => { + await result.current.handleApplyChanges(); + }); + + expect(seen).toHaveLength(0); + expect(updateCalls()).toHaveLength(0); + expect(mockToastError).toHaveBeenCalledWith("Cannot Apply Changes", { + description: expect.stringContaining("2 rows on screen carry one value between them"), + }); + expect(result.current.pendingChanges).toHaveLength(2); + }); + + test("keeps a text key and a numeric key apart, and lets the engine settle them", async () => { + // `bun:sqlite` hands back the text `1` and the integer 1 from the same dynamically typed + // column. Collapsing them by their text would ask about one key and write two; keeping + // the type asks about both, and SQLite answers two groups - one of them holding the two + // text rows, which is the refusal. + const seen: Array<{ params: unknown[] }> = []; + globalThis.fetch = mock((_url: string, init?: RequestInit) => { + seen.push({ params: JSON.parse(String(init?.body ?? "{}")).params ?? [] }); + return Promise.resolve({ + ok: true, + json: () => + Promise.resolve({ + rows: [ + { id: "1", count: "2" }, + { id: 1, count: "1" }, + ], + fields: ["id", "count"], + rowCount: 2, + }), + }); + }) as unknown as typeof fetch; + + const { result } = renderHook(() => + useInlineEditing({ + activeConnection: makeConnection({ type: "sqlite" }), + currentTab: makeTab({ + result: makeResult({ + rows: [ + { id: "1", note: "text one" }, + { id: 1, note: "number one" }, + ], + fields: ["id", "note"], + rowCount: 2, + }), + resultQuery: "SELECT id, note FROM t", + }), + executeQuery: mockExecuteQuery, + }), + ); + + act(() => { + result.current.handleCellChange({ rowIndex: 0, columnId: "note", originalValue: "text one", newValue: "x" }); + result.current.handleCellChange({ rowIndex: 1, columnId: "note", originalValue: "number one", newValue: "y" }); + }); + await act(async () => { + await result.current.handleApplyChanges(); + }); + + // Both keys were asked about, not one. + expect(seen[0].params).toEqual(["1", 1]); + expect(updateCalls()).toHaveLength(0); + expect(mockToastError).toHaveBeenCalledWith("Cannot Apply Changes", { + description: expect.stringContaining("would write to 3 rows"), + }); + }); + + test("asks inside the transaction when one is open, not beside it", async () => { + // The UPDATEs go to /api/db/transaction, which holds the one connection the transaction + // lives on. A check sent to /api/db/query takes a different pooled connection and cannot + // see anything the transaction has not committed: measured, a row INSERTed inside the + // open transaction is on screen, invisible to the check, and the apply refuses for ever + // with "no longer in the table" - false, about a row the user is looking at. + const seen: Array<{ url: string; body: Record }> = []; + globalThis.fetch = mock((url: string, init?: RequestInit) => { + seen.push({ url: String(url), body: JSON.parse(String(init?.body ?? "{}")) }); + return Promise.resolve({ + ok: true, + json: () => Promise.resolve({ rows: [{ id: 1, count: "1" }], fields: ["id", "count"], rowCount: 1 }), + }); + }) as unknown as typeof fetch; + + const { result } = renderHook(() => + useInlineEditing({ + activeConnection: makeConnection(), + currentTab: makeTab(), + executeQuery: mockExecuteQuery, + transactionActive: true, + }), + ); + + act(() => { + result.current.handleCellChange(makeChange()); + }); + await act(async () => { + await result.current.handleApplyChanges(); + }); + + expect(seen).toHaveLength(1); + expect(seen[0].url).toBe("/api/db/transaction"); + expect(seen[0].body.action).toBe("query"); + expect(updateCalls()).toHaveLength(1); + }); + + test("asks for enough rows that a default page cannot cut the answer", async () => { + // Left to the default the answer is cut at 500 rows, and the groups that fell off would + // read as rows that are no longer in the table - a refusal with a false reason. The + // limit is the number of distinct keys plus one, which the answer can never reach. + const seen: Array> = []; + globalThis.fetch = mock((_url: string, init?: RequestInit) => { + seen.push(JSON.parse(String(init?.body ?? "{}"))); + return Promise.resolve({ + ok: true, + json: () => + Promise.resolve({ + rows: [ + { id: 1, count: "1" }, + { id: 2, count: "1" }, + ], + fields: ["id", "count"], + rowCount: 2, + }), + }); + }) as unknown as typeof fetch; + + const { result } = renderHook(() => + useInlineEditing({ + activeConnection: makeConnection(), + currentTab: makeTab(), + executeQuery: mockExecuteQuery, + }), + ); + + act(() => { + result.current.handleCellChange(makeChange()); + result.current.handleCellChange({ rowIndex: 1, columnId: "name", originalValue: "Bob", newValue: "Bobby" }); + }); + await act(async () => { + await result.current.handleApplyChanges(); + }); + + expect((seen[0].options as { limit: number }).limit).toBe(3); + }); + + test("reads the count by POSITION, because no two engines name it the same", async () => { + // PostgreSQL calls it `count`, MySQL and SQLite both call it `COUNT(*)`. Reading it by + // name would work on whichever one the test happened to imitate and refuse every apply + // on the others, so the mock here answers with MySQL's name. + globalThis.fetch = mock(() => + Promise.resolve({ + ok: true, + json: () => Promise.resolve({ rows: [{ id: 1, "COUNT(*)": 1 }], fields: ["id", "COUNT(*)"], rowCount: 1 }), + }), + ) as unknown as typeof fetch; + + const { result } = renderHook(() => + useInlineEditing({ + activeConnection: makeConnection({ type: "mysql" }), + currentTab: makeTab(), + executeQuery: mockExecuteQuery, + }), + ); + + act(() => { + result.current.handleCellChange(makeChange()); + }); + await act(async () => { + await result.current.handleApplyChanges(); + }); + + // It passed, which it could only do by reading the second value rather than a name. + expect(updateCalls()).toHaveLength(1); + expect(mockToastError).not.toHaveBeenCalled(); + }); + + test("refuses two keys the ENGINE treats as one, which this side cannot see", async () => { + // The engine decides what counts as the same key, not JavaScript. MySQL's default + // collation is case-insensitive: `abc` and `ABC` are two distinct keys here and one + // key there. Measured on MySQL 8.4 - `IN ("abc", "ABC")` counts two rows, a plain + // total would read that as two keys matching two rows, and `WHERE k = "abc"` then + // writes to BOTH. Asked grouped, the engine answers ONE group holding two rows. + globalThis.fetch = mock(() => + Promise.resolve({ + ok: true, + json: () => + Promise.resolve({ rows: [{ user_id: "abc", count: "2" }], fields: ["user_id", "count"], rowCount: 1 }), + }), + ) as unknown as typeof fetch; + + const { result } = renderHook(() => + useInlineEditing({ + activeConnection: makeConnection({ type: "mysql" }), + currentTab: makeTab({ + result: makeResult({ + rows: [ + { user_id: "abc", note: "one" }, + { user_id: "ABC", note: "two" }, + ], + fields: ["user_id", "note"], + rowCount: 2, + }), + resultQuery: "SELECT user_id, note FROM accounts", + }), + executeQuery: mockExecuteQuery, + }), + ); + + act(() => { + result.current.handleCellChange({ rowIndex: 0, columnId: "note", originalValue: "one", newValue: "x" }); + result.current.handleCellChange({ rowIndex: 1, columnId: "note", originalValue: "two", newValue: "y" }); + }); + await act(async () => { + await result.current.handleApplyChanges(); + }); + + expect(updateCalls()).toHaveLength(0); + expect(mockToastError).toHaveBeenCalledWith("Cannot Apply Changes", { + description: expect.stringContaining("user_id does not tell these rows apart"), + }); + expect(result.current.pendingChanges).toHaveLength(2); + }); + + test("does not call the key unique when there are FEWER rows than keys", async () => { + // A row deleted under the user. The column may be perfectly unique, so saying it is not + // would be false, and telling them to add a key already in their query is no help. + answerKeyCheck(0); + const { result } = renderHook(() => + useInlineEditing({ + activeConnection: makeConnection(), + currentTab: makeTab(), + executeQuery: mockExecuteQuery, + }), + ); + + act(() => { + result.current.handleCellChange(makeChange()); + }); + await act(async () => { + await result.current.handleApplyChanges(); + }); + + expect(updateCalls()).toHaveLength(0); + expect(mockToastError).toHaveBeenCalledWith("Cannot Apply Changes", { + description: expect.stringContaining("no longer in the table"), + }); + expect(mockToastError).not.toHaveBeenCalledWith("Cannot Apply Changes", { + description: expect.stringContaining("does not tell these rows apart"), + }); + }); + + test("refuses a count it cannot read, rather than treating it as a pass", async () => { + // A group came back with nothing where the count should be. `Number(null)` is zero and + // would read as a real answer, so the row carries no second value at all. + globalThis.fetch = mock(() => + Promise.resolve({ + ok: true, + json: () => Promise.resolve({ rows: [{ id: 1 }], fields: ["id"], rowCount: 1 }), + }), + ) as unknown as typeof fetch; + const { result } = renderHook(() => + useInlineEditing({ + activeConnection: makeConnection(), + currentTab: makeTab(), + executeQuery: mockExecuteQuery, + }), + ); + + act(() => { + result.current.handleCellChange(makeChange()); + }); + await act(async () => { + await result.current.handleApplyChanges(); + }); + + expect(updateCalls()).toHaveLength(0); + expect(mockToastError).toHaveBeenCalledWith("Cannot Apply Changes", { + description: expect.stringContaining("returned no count"), + }); + }); + + test("refuses a key that is an expression wearing a column's name", async () => { + // Measured against PostgreSQL 16. `SELECT ROW_NUMBER() OVER (ORDER BY product_name) AS + // product_id, product_name FROM products` puts 1, 2, 3 in a field called product_id; + // the table really has a product_id; the uniqueness check asks the table about 1 and 2 + // and is told one row each, so all three of its conditions hold; and the UPDATEs then + // land on whichever products those are, not on the rows anyone was looking at. Two + // cells edited, two rows written, neither on screen, both reported as accepted. + const seen: unknown[] = []; + globalThis.fetch = mock((_url: string, init?: RequestInit) => { + seen.push(init); + return Promise.resolve({ + ok: true, + json: () => + Promise.resolve({ + rows: [ + { product_id: 1, count: "1" }, + { product_id: 2, count: "1" }, + ], + fields: ["product_id", "count"], + rowCount: 2, + }), + }); + }) as unknown as typeof fetch; + + const { result } = renderHook(() => + useInlineEditing({ + activeConnection: makeConnection(), + currentTab: makeTab({ + result: makeResult({ + rows: [ + { product_id: 1, product_name: "Alice Mutton 1" }, + { product_id: 2, product_name: "Alice Mutton 2" }, + ], + fields: ["product_id", "product_name"], + rowCount: 2, + }), + resultQuery: "SELECT ROW_NUMBER() OVER (ORDER BY product_name) AS product_id, product_name FROM products", + }), + executeQuery: mockExecuteQuery, + }), + ); + + act(() => { + result.current.handleCellChange({ + rowIndex: 0, + columnId: "product_name", + originalValue: "Alice Mutton 1", + newValue: "x", + }); + }); + await act(async () => { + await result.current.handleApplyChanges(); + }); + + // Refused before the engine is asked anything at all. + expect(seen).toHaveLength(0); + expect(updateCalls()).toHaveLength(0); + expect(mockToastError).toHaveBeenCalledWith("Cannot Apply Changes", { + description: expect.stringContaining("not read straight from the table"), + }); + expect(result.current.pendingChanges).toHaveLength(1); + }); + + test("refuses a key that is another column renamed", async () => { + // The same defect spelled shorter: the WHERE would carry sku's value. + const { result } = renderHook(() => + useInlineEditing({ + activeConnection: makeConnection(), + currentTab: makeTab({ + result: makeResult({ + rows: [{ product_id: "SKU-0001", product_name: "Chai" }], + fields: ["product_id", "product_name"], + rowCount: 1, + }), + resultQuery: "SELECT sku AS product_id, product_name FROM products", + }), + executeQuery: mockExecuteQuery, + }), + ); + + act(() => { + result.current.handleCellChange({ rowIndex: 0, columnId: "product_name", originalValue: "Chai", newValue: "x" }); + }); + await act(async () => { + await result.current.handleApplyChanges(); + }); + + expect(updateCalls()).toHaveLength(0); + expect(mockToastError).toHaveBeenCalledWith("Cannot Apply Changes", { + description: expect.stringContaining("not read straight from the table"), + }); + }); + + test("refuses a key value this editor cannot even read", async () => { + // A value with a null prototype has no `toString`, so turning it into text throws - and + // that happens before the request, outside the try that guards the request itself. + // Unguarded it left the apply as an unhandled rejection: no write, but no toast either. + const unreadable = Object.create(null) as Record; + const { result } = renderHook(() => + useInlineEditing({ + activeConnection: makeConnection(), + currentTab: makeTab({ + result: makeResult({ rows: [{ id: unreadable, name: "Alice" }], fields: ["id", "name"], rowCount: 1 }), + }), + executeQuery: mockExecuteQuery, + }), + ); + + act(() => { + result.current.handleCellChange(makeChange()); + }); + await act(async () => { + await result.current.handleApplyChanges(); + }); + + expect(updateCalls()).toHaveLength(0); + expect(mockToastError).toHaveBeenCalledWith("Cannot Apply Changes", { + description: expect.stringContaining("cannot read the id of every row it would write to"), + }); + }); + + test("refuses a row whose key is null before it sends anything", async () => { + answerKeyCheck(); + const { result } = renderHook(() => + useInlineEditing({ + activeConnection: makeConnection(), + currentTab: makeTab({ + result: makeResult({ rows: [{ id: null, name: "Alice" }], fields: ["id", "name"], rowCount: 1 }), + }), + executeQuery: mockExecuteQuery, + }), + ); + + act(() => { + result.current.handleCellChange(makeChange()); + }); + await act(async () => { + await result.current.handleApplyChanges(); + }); + + expect(updateCalls()).toHaveLength(0); + expect(mockToastError).toHaveBeenCalledWith("Cannot Apply Changes", { + description: expect.stringContaining("has no id"), + }); + }); + + test("refuses when the check never reaches the server", async () => { + // A rejected request, not a refused one: the browser went offline mid-apply. Same + // answer as any other unanswered check, because an unanswered check is not a pass. + globalThis.fetch = mock(() => Promise.reject(new Error("Failed to fetch"))) as unknown as typeof fetch; + const { result } = renderHook(() => + useInlineEditing({ + activeConnection: makeConnection(), + currentTab: makeTab(), + executeQuery: mockExecuteQuery, + }), + ); + + act(() => { + result.current.handleCellChange(makeChange()); + }); + await act(async () => { + await result.current.handleApplyChanges(); + }); + + expect(updateCalls()).toHaveLength(0); + expect(mockToastError).toHaveBeenCalledWith("Cannot Apply Changes", { + description: expect.stringContaining("Failed to fetch"), + }); + expect(result.current.pendingChanges).toHaveLength(1); + }); + + test("the check is bound, not interpolated, and names the resolved table", async () => { + const seen: Array<{ sql: string; params: unknown[] }> = []; + globalThis.fetch = mock((_url: string, init?: RequestInit) => { + const body = JSON.parse(String(init?.body ?? "{}")); + seen.push({ sql: body.sql, params: body.params ?? [] }); + return Promise.resolve({ + ok: true, + json: () => Promise.resolve({ rows: [{ id: 1, count: "1" }], fields: ["id", "count"], rowCount: 1 }), + }); + }) as unknown as typeof fetch; + + const { result } = renderHook(() => + useInlineEditing({ + activeConnection: makeConnection(), + currentTab: makeTab({ resultQuery: "SELECT * FROM public.users" }), + executeQuery: mockExecuteQuery, + }), + ); + + act(() => { + result.current.handleCellChange(makeChange()); + }); + await act(async () => { + await result.current.handleApplyChanges(); + }); + + expect(seen).toHaveLength(1); + // The table the STATEMENT names, the same one the UPDATE will use. + expect(seen[0].sql).toContain("FROM public.users"); + expect(seen[0].sql).toContain('"id", COUNT(*) FROM public.users WHERE "id" IN ($1) GROUP BY "id"'); + // The value travels beside the statement, not inside it. + expect(seen[0].sql).not.toContain("IN (1)"); + expect(seen[0].params).toEqual([1]); + }); }); diff --git a/tests/unit/sql/update-target.test.ts b/tests/unit/sql/update-target.test.ts index fbc3e8b2d..1312c017c 100644 --- a/tests/unit/sql/update-target.test.ts +++ b/tests/unit/sql/update-target.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test"; -import { resolveUpdateTarget } from "@/lib/sql/update-target"; +import { resolveUpdateTarget, selectsPlainColumn } from "@/lib/sql/update-target"; // Inline grid editing writes `UPDATE SET ...`, and this decides what `
` // may be. It used to be the tab's title, which is free text and outlives the query it was @@ -559,3 +559,92 @@ describe("resolveUpdateTarget", () => { } }); }); + +describe("selectsPlainColumn", () => { + // The key an inline edit writes against is picked by NAME off the result's field list, and + // a name is not a provenance. Measured against PostgreSQL 16: `SELECT ROW_NUMBER() OVER + // (ORDER BY product_name) AS product_id, product_name FROM products` puts 1, 2, 3 in a + // column called `product_id`, `products` really has a `product_id`, and the two UPDATEs + // that followed wrote to two rows that were never on screen - reported as accepted. + + test("a star means every field is the table's own", () => { + expect(selectsPlainColumn("SELECT * FROM products", "product_id")).toBe(true); + expect(selectsPlainColumn("SELECT p.* FROM products p", "product_id")).toBe(true); + }); + + test("a plain reference is the column, qualified or not", () => { + expect(selectsPlainColumn("SELECT product_id, sku FROM products", "product_id")).toBe(true); + expect(selectsPlainColumn("SELECT p.product_id, p.sku FROM products p", "product_id")).toBe(true); + expect(selectsPlainColumn('SELECT "Id", name FROM users', "Id")).toBe(true); + // An alias that names the column it already names changes nothing. + expect(selectsPlainColumn("SELECT product_id AS product_id FROM products", "product_id")).toBe(true); + }); + + test("an expression wearing the column's name is not the column", () => { + expect( + selectsPlainColumn( + "SELECT ROW_NUMBER() OVER (ORDER BY product_name) AS product_id, product_name FROM products", + "product_id", + ), + ).toBe(false); + expect(selectsPlainColumn("SELECT 1 AS id, name FROM users", "id")).toBe(false); + expect(selectsPlainColumn("SELECT count(*) AS id FROM users", "id")).toBe(false); + }); + + test("an alias that renames another column is not the column either", () => { + // Shorter to write and the same defect: the WHERE would carry sku's value. + expect(selectsPlainColumn("SELECT sku AS product_id, product_name FROM products", "product_id")).toBe(false); + expect(selectsPlainColumn("SELECT p.sku AS product_id FROM products p", "product_id")).toBe(false); + }); + + test("an alias without the word AS is still an alias", () => { + // `SELECT sku product_id FROM products` is the same rename with the keyword left out, + // and every engine that offers inline editing accepts it. + expect(selectsPlainColumn("SELECT sku product_id, product_name FROM products", "product_id")).toBe(false); + expect(selectsPlainColumn("SELECT product_id product_id FROM products", "product_id")).toBe(true); + }); + + test("a multiplication is not a star", () => { + // The hole an adversarial review found: reading any `*` in the item as a star let the + // whole check be skipped by writing `* 1`. Measured live - the UPDATEs then wrote two + // products that were never on screen. + expect( + selectsPlainColumn( + "SELECT ROW_NUMBER() OVER (ORDER BY product_name) * 1 AS product_id, product_name FROM products", + "product_id", + ), + ).toBe(false); + expect(selectsPlainColumn("SELECT product_name, 2 * 1 AS product_id FROM products", "product_id")).toBe(false); + }); + + test("a later item renaming over a star wins, because that is what the driver hands over", () => { + // Two fields of the same name reach the row object as one, and the LAST one written is + // the value the grid reads - measured on node-postgres. + expect( + selectsPlainColumn("SELECT p.*, ROW_NUMBER() OVER (ORDER BY x) AS product_id FROM products p", "product_id"), + ).toBe(false); + expect(selectsPlainColumn("SELECT product_id, sku AS product_id FROM products", "product_id")).toBe(false); + // And the other way round: the rename comes first, the real column last. + expect(selectsPlainColumn("SELECT sku AS product_id, product_id FROM products", "product_id")).toBe(true); + }); + + test("row-count keywords are not part of the list", () => { + expect(selectsPlainColumn("SELECT DISTINCT product_id, sku FROM products", "product_id")).toBe(true); + expect(selectsPlainColumn("SELECT ALL product_id, sku FROM products", "product_id")).toBe(true); + expect(selectsPlainColumn("SELECT DISTINCT ON (sku) product_id, sku FROM products", "product_id")).toBe(true); + }); + + test("a reference can carry its schema as well as its table", () => { + expect(selectsPlainColumn("SELECT public.products.product_id FROM public.products", "product_id")).toBe(true); + expect(selectsPlainColumn("SELECT p.product_id product_id FROM products p", "product_id")).toBe(true); + }); + + test("a column the select list does not produce at all is refused", () => { + expect(selectsPlainColumn("SELECT sku, product_name FROM products", "product_id")).toBe(false); + }); + + test("a statement this reader cannot take apart is refused rather than guessed at", () => { + expect(selectsPlainColumn("SELECT id FROM users /* unclosed", "id")).toBe(false); + expect(selectsPlainColumn("UPDATE products SET sku = 'x'", "product_id")).toBe(false); + }); +}); From 108e518bac74ecb316e45d15e3182d941f632073 Mon Sep 17 00:00:00 2001 From: Yusuf GUNDOGDU Date: Fri, 18 Sep 2026 17:59:46 +0300 Subject: [PATCH 02/19] fix(schema-diff): read the database when a snapshot is taken and when a diff is asked for #884 moved Current Schema off the explorer's cached copy and onto a read of the connection, but that read sits in an effect keyed on [connection] alone, so it happens once and not again while the panel stays open. The sequence the Diff tab exists for - snapshot, change the database, compare - still answered "No differences found". Measured against PostgreSQL 16 with the panel left open. Two moments read the connection now, and between them they are the sequence: a snapshot reads it, and choosing a target reads it again, because that is when a person asks to be told the difference. Freshening only the snapshot was tried first and is not enough - the other side stays at the moment of the snapshot. Two snapshots compared against each other read nothing: neither side is the database. Three reads can be in flight at once, so they are sequenced through useReadGeneration, which the repository already states once for this problem. It replaces comparing the connection OBJECT, which is not safe: activeConnection is a useMemo over a prop the embedded host supplies, so a host handing over a fresh array per render produces a fresh object per render and a read would be discarded on a connection that never changed. The rest is what an awaited read in a click handler needs and did not have: a re-entrancy guard in a ref, because two Enter presses land in the same tick; setSnapshotting(false) in a finally, or a superseded read leaves the button reading Reading... for the life of the panel; the label input and Cancel disabled while the read is out; the storage write inside the try, because a localStorage quota refusal is an ordinary outcome for a whole schema; and a snapshot that was overtaken saying so rather than returning quietly, which saved nothing and said nothing while the button went back to Save. The failure banner carries the connection it was about, so a failure on a database the user has left does not sit over the one they are looking at, and a later read of that database that works clears it. --- src/components/SchemaDiff.tsx | 258 ++++++++++--- src/hooks/use-read-generation.ts | 9 +- tests/components/SchemaDiff.test.tsx | 546 ++++++++++++++++++++++++++- 3 files changed, 734 insertions(+), 79 deletions(-) diff --git a/src/components/SchemaDiff.tsx b/src/components/SchemaDiff.tsx index 5e820ebeb..2447bad06 100644 --- a/src/components/SchemaDiff.tsx +++ b/src/components/SchemaDiff.tsx @@ -1,7 +1,8 @@ "use client"; import { appFetch } from "@/lib/config/base-path"; -import React, { useState, useMemo, useCallback, useEffect } from "react"; +import React, { useState, useMemo, useCallback, useEffect, useRef } from "react"; +import { useReadGeneration } from "@/hooks/use-read-generation"; import { GitCompare, Plus, @@ -85,32 +86,49 @@ export function SchemaDiff({ schema, connection }: SchemaDiffProps) { const [showMigration, setShowMigration] = useState(false); const [snapshotLabel, setSnapshotLabel] = useState(""); const [showLabelInput, setShowLabelInput] = useState(false); - + /** True while a snapshot's own read is in flight. State, because the button reads it. */ + const [snapshotting, setSnapshotting] = useState(false); /** - * The objects the database holds right now, read when this panel opens. + * The same fact as a ref, because the GUARD cannot read the state. * - * `schema` is the prop the explorer already had, and it is what "Current Schema" used to - * mean — so a diff taken right after a DDL change compared a copy of the schema from - * before the change and answered "No differences found" (#884). The panel is mounted when - * the user opens it, so reading here is the moment that matters for that sequence. + * Two Enter presses land in the same tick, before React has re-rendered, so both see the + * `snapshotting` the callback closed over — `false` — and both save. A ref is written and + * read synchronously, which is what a re-entrancy guard needs. + */ + const snapshotInFlight = useRef(false); + /** + * The reason the last snapshot was not saved, and the connection it was about. * - * `null` until the read lands, and the prop stands in meanwhile: an empty side would - * report every object as removed, which is worse than being briefly out of date. + * Carried together for the reason `liveRead` is: a failure on the connection the user has + * left is not a failure of the one they are looking at, and a banner about the other + * database over a working panel is its own small lie. Derived rather than cleared by an + * effect, so there is no render where the wrong one is on screen. */ + const [snapshotFailure, setSnapshotFailure] = useState<{ connectionId: string; reason: string } | null>(null); + /** - * The last read, and the connection it was a read OF. + * Which read is the current one. * - * The connection is stored with the result rather than the result being cleared when the - * connection changes, because the panel outlives a switch: `BottomPanel` keeps it mounted, - * so holding the previous database's objects made "Current Schema" mean the OTHER - * connection until the new read landed, and for good if that read failed. `takeSnapshot` - * then wrote the new connection's id and type onto the old one's objects, which is the - * stale-copy defect #884 is about, kept for as long as the snapshot is. + * Three things read this connection now - the panel opening, a snapshot, and a target + * being chosen to compare against - and any of them can settle after another has already + * started. A counter is what settles that, and the repository already states the rule + * once in `useReadGeneration`: begin a read, and every write it performs asks first + * whether it is still the one that matters. * - * Carrying the connection makes a stale read unusable rather than something a second - * effect has to remember to clear, and it is keyed on the connection OBJECT — the same - * thing the effect depends on — so a read can never outlive the exact render that asked - * for it. + * Comparing the connection OBJECT instead was the earlier attempt and it is not safe: + * `use-connection-adapter.ts` builds `activeConnection` with a `useMemo` over a prop the + * embedded host supplies, so a host that hands over a fresh array per render produces a + * fresh object per render, and a read would then be discarded on a connection that never + * changed - the snapshot silently not saved, with nothing on screen. + */ + const reads = useReadGeneration(); + + /** + * The objects the database holds, and the connection they were read FROM. + * + * Carried together rather than cleared on a switch, because the panel outlives one: + * holding the previous database's objects made "Current Schema" mean the OTHER connection + * until the new read landed, and for good if that read failed. * * `error` is the other half. The panel falls back to the explorer's copy, and that copy is * precisely what #884 is about, so the reason is state that reaches the screen rather than @@ -122,64 +140,162 @@ export function SchemaDiff({ schema, connection }: SchemaDiffProps) { error: string | null; } | null>(null); - const readForThisConnection = liveRead?.connection === connection ? liveRead : null; + const readForThisConnection = liveRead?.connection.id === connection?.id ? liveRead : null; + const liveSchema = readForThisConnection?.objects ?? null; + const liveSchemaError = readForThisConnection?.error ?? null; + const snapshotError = + snapshotFailure !== null && snapshotFailure.connectionId === connection?.id ? snapshotFailure.reason : null; /** - * The objects the database holds right now, read when this panel opens. - * - * `schema` is the prop the explorer already had, and it is what "Current Schema" used to - * mean — so a diff taken right after a DDL change compared a copy of the schema from - * before the change and answered "No differences found" (#884). The panel is mounted when - * the user opens it, so reading here is the moment that matters for that sequence. + * Begin a read of this connection, and hand back both the promise and the question every + * write it performs has to ask first. * - * `null` until the read lands, and the prop stands in meanwhile: an empty side would - * report every object as removed, which is worse than being briefly out of date. + * The write is left to the caller rather than done here, and deliberately: a `setState` + * reached through a helper called straight from an effect is what the React lint rules + * forbid, and the shape they accept - settle first, then write - is also the honest one, + * because the two callers want different things from a failure. The panel opening falls + * back to the explorer's copy and says so; a snapshot saves nothing at all. */ - const liveSchema = readForThisConnection?.objects ?? null; - const liveSchemaError = readForThisConnection?.error ?? null; + const beginRead = useCallback( + (conn: DatabaseConnection) => ({ read: readLiveSchema(conn), isCurrent: reads.begin() }), + [reads], + ); useEffect(() => { if (!connection) return; - let cancelled = false; - readLiveSchema(connection) + const { read, isCurrent } = beginRead(connection); + read .then((objects) => { - if (!cancelled) setLiveRead({ connection, objects, error: null }); + if (!isCurrent()) return; + setLiveRead({ connection, objects, error: null }); + // A reading of this database that worked settles the last one that did not: leaving + // it up meant a banner about a failure the user had already walked away from. + setSnapshotFailure((previous) => (previous?.connectionId === connection.id ? null : previous)); }) .catch((err) => { const reason = err instanceof Error ? err.message : String(err); - if (!cancelled) setLiveRead({ connection, objects: null, error: reason }); + if (isCurrent()) setLiveRead({ connection, objects: null, error: reason }); logger.warn("Failed to read the current schema for a diff; falling back to the explorer's copy", { route: "SchemaDiff", error: reason, }); }); - return () => { - cancelled = true; - }; - }, [connection]); + }, [connection, beginRead]); + + /** + * A comparison reads the database again. + * + * Without this the panel answers the question it was opened with rather than the one being + * asked: take a snapshot, change the database, pick that snapshot as the target, and both + * sides are the moment of the snapshot - "No differences found" again, which is the whole + * defect wearing different clothes. The read happens when a target is CHOSEN, because that + * is the moment a person asks to be told the difference. + */ + useEffect(() => { + // Only when one side of the comparison IS the database. Two snapshots against each + // other are two files; reading the connection for them is a round trip that changes + // nothing either side shows. + if (!connection || !targetId) return; + if (sourceId !== "current" && targetId !== "current") return; + const { read, isCurrent } = beginRead(connection); + read + .then((objects) => { + if (!isCurrent()) return; + setLiveRead({ connection, objects, error: null }); + // Same rule as the read when the panel opens: a reading of this database that worked + // settles the last one that did not. + setSnapshotFailure((previous) => (previous?.connectionId === connection.id ? null : previous)); + }) + .catch((err) => { + const reason = err instanceof Error ? err.message : String(err); + // Falling back to the last copy rather than emptying the side, which would report + // every object as removed; the banner says why it may be out of date. + if (isCurrent()) setLiveRead({ connection, objects: null, error: reason }); + }); + }, [targetId, sourceId, connection, beginRead]); /** What "Current Schema" means on both sides of the diff, and in a new snapshot. */ const currentSchema = liveSchema ?? schema; - // Take snapshot of current schema - const takeSnapshot = useCallback(() => { - if (!connection) return; - const snapshot: SchemaSnapshot = { - id: Date.now().toString(), - connectionId: connection.id, - connectionName: connection.name, - databaseType: connection.type, - // The live read, not the prop: a snapshot taken from a stale copy is stale for as - // long as it is kept, and it is kept to be compared against later (#884). - schema: JSON.parse(JSON.stringify(currentSchema)), - createdAt: new Date(), - label: snapshotLabel.trim() || undefined, - }; - storage.saveSchemaSnapshot(snapshot); - setSnapshots(storage.getSchemaSnapshots()); - setSnapshotLabel(""); - setShowLabelInput(false); - }, [currentSchema, connection, snapshotLabel]); + /** + * Freeze the schema the database holds AT THIS MOMENT, not the one the panel read when + * it opened. + * + * #884 moved "Current Schema" off the explorer's copy and onto a read of the connection, + * but that read sits in an effect keyed on `[connection]` alone, so it happens once and + * not again for as long as the panel stays open. The sequence the Diff tab exists for — + * snapshot, change the database, compare — still answered "No differences found": the + * snapshot froze that first copy, and so did the other side of the comparison. Measured + * against PostgreSQL 16 with the panel left open. Leaving the tab and coming back was + * the only thing that helped, and it helped because `BottomPanel` mounts one view at a + * time, so returning is a remount and the effect runs again — not a step anyone would + * guess, and not one the panel tells you about. + * + * Reading here fixes both halves at once, because the same read becomes the new + * `liveRead`: the snapshot records the database, and the "Current Schema" it will be + * compared against is refreshed to the same instant. + * + * A read that fails saves NOTHING. A snapshot is kept to be compared against later, so a + * silently stale one is the defect again with a longer fuse; the banner says why and the + * label stays typed so the button can be pressed again. + */ + const takeSnapshot = useCallback(async () => { + if (!connection || snapshotInFlight.current) return; + snapshotInFlight.current = true; + setSnapshotting(true); + setSnapshotFailure(null); + try { + // The same read that becomes "Current Schema", so the snapshot and the side it will + // be compared against are the same instant. + const { read, isCurrent } = beginRead(connection); + const objects = await read; + if (!isCurrent()) { + // Something asked for a newer read while this one was in flight - choosing a target + // does, on this same connection. Returning quietly here saved nothing and said + // nothing, so the button came back to "Save" and the user believed it had. The + // banner stays until the next attempt: a later read landing is not a snapshot, and + // clearing it on one put the silence straight back. + setSnapshotFailure({ + connectionId: connection.id, + reason: "the schema was read again before this finished. Press Save again", + }); + return; + } + setLiveRead({ connection, objects, error: null }); + const snapshot: SchemaSnapshot = { + id: Date.now().toString(), + connectionId: connection.id, + connectionName: connection.name, + databaseType: connection.type, + schema: JSON.parse(JSON.stringify(objects)), + createdAt: new Date(), + label: snapshotLabel.trim() || undefined, + }; + // Inside the try as well: snapshots live in localStorage and a snapshot is a whole + // schema, so a quota refusal is an ordinary outcome rather than an exotic one. + // Inside the try, so a write that throws reaches the same banner the read failure + // does rather than escaping as an unhandled rejection. + // + // It does NOT catch a full disk. `storage.saveSchemaSnapshot` returns nothing and + // `local-storage.ts` swallows the quota error, so a refused write is reported here as + // a snapshot taken. That is the store's to fix - every caller of it has the same + // problem and none of them can see the failure - and it predates this change. + storage.saveSchemaSnapshot(snapshot); + setSnapshots(storage.getSchemaSnapshots()); + setSnapshotLabel(""); + setShowLabelInput(false); + } catch (err) { + const reason = err instanceof Error ? err.message : String(err); + setSnapshotFailure({ connectionId: connection.id, reason }); + logger.warn("Nothing was saved for this snapshot", { route: "SchemaDiff", error: reason }); + } finally { + // In `finally`, not at the end of each branch: a throw between them would otherwise + // leave the button reading "Reading..." for the life of the panel, with nothing on + // screen saying why, and `snapshotInFlight` stuck true so no later press does anything. + snapshotInFlight.current = false; + setSnapshotting(false); + } + }, [connection, snapshotLabel, beginRead]); // Delete snapshot const deleteSnapshot = useCallback( @@ -385,16 +501,26 @@ export function SchemaDiff({ schema, connection }: SchemaDiffProps) { value={snapshotLabel} onChange={(e) => setSnapshotLabel(e.target.value)} onKeyDown={(e) => e.key === "Enter" && takeSnapshot()} - className="h-7 px-2 text-xs bg-fill border border-hairline-strong rounded text-fg-secondary focus:outline-none focus:border-brand-tint w-32" + disabled={snapshotting} + className="h-7 px-2 text-xs bg-fill border border-hairline-strong rounded text-fg-secondary focus:outline-none focus:border-brand-tint w-32 disabled:opacity-60" autoFocus /> - + {/* Snapshot controls */} {showLabelInput ? (
@@ -566,6 +842,21 @@ export function SchemaDiff({ schema, connection }: SchemaDiffProps) {
{`No snapshot was saved: ${snapshotError}`} + {/* The way out, and it is a button rather than a rule about which reads clear the + report - a rule is what wiped the message in the first place. Nothing else here + is an exit a user could rely on: pressing Save again is a retry that can fail + again, leaving the connection only hides the report until they come back, and + "it goes away when you reopen the panel" is something they would have to guess. + This clears the report and nothing else; the panel's own warning above is + derived from the last read and has its own way out, the next read that works. */} +
)} @@ -584,13 +875,26 @@ export function SchemaDiff({ schema, connection }: SchemaDiffProps) { snapshots={snapshots} onCompare={(sourceId, targetId) => { setSourceId(sourceId); - setTargetId(targetId); + chooseTarget(targetId); }} onDelete={deleteSnapshot} />
)} + ) : missingSnapshotId ? ( + /* The snapshot a side names is not in the store any more. What stood here was a + full diff computed against an empty array - the whole database reported as + removed - which is the loudest thing this panel can say and was not true. It + says what is actually the matter instead, and offers the only move there is: + pick something else. */ +
+ + {"This snapshot is no longer stored, so there is nothing to compare"} + + {"Only the last 50 snapshots are kept, and another tab may have deleted it. Choose a different one."} + +
) : showMigration && migrationSQL ? ( 
diff --git a/tests/components/SchemaDiff.test.tsx b/tests/components/SchemaDiff.test.tsx
index d6315c6e3..94bf0cf1a 100644
--- a/tests/components/SchemaDiff.test.tsx
+++ b/tests/components/SchemaDiff.test.tsx
@@ -4,6 +4,7 @@ import "../helpers/mock-navigation";
 
 import { mock } from "bun:test";
 import React from "react";
+import * as ReactNS from "react";
 
 // ── Mock data ────────────────────────────────────────────────────────────────
 
@@ -198,6 +199,36 @@ mock.module("@/lib/storage", () => ({
   },
 }));
 
+// ── Watch the panel's own state setters ──────────────────────────────────────
+
+/**
+ * A `setState` on an unmounted component is a SILENT no-op in React 19. It does not warn,
+ * it does not throw, and nothing outside the component can tell that it happened - measured
+ * here, on this React, before these tests were written. So "the panel writes nothing after
+ * it is gone" cannot be held by watching the screen, the store or the console: there is
+ * nothing to watch. It is held by watching the setters.
+ *
+ * `useState` is wrapped once, for the whole file, and records only while `stateWrites` is an
+ * array - which `recordStateWrites()` switches on for the span of one assertion, so the rest
+ * of the suite pays nothing and sees nothing. The real hook does the work; this only counts.
+ */
+let stateWrites: string[] | null = null;
+const realUseState = ReactNS.useState;
+const watchedReact = {
+  ...ReactNS,
+  useState: (initial: unknown) => {
+    const [value, set] = (realUseState as (i: unknown) => [unknown, (v: unknown) => void])(initial);
+    return [
+      value,
+      (next: unknown) => {
+        if (stateWrites) stateWrites.push(typeof next === "function" ? "fn" : String(JSON.stringify(next)));
+        return set(next);
+      },
+    ];
+  },
+};
+mock.module("react", () => ({ ...watchedReact, default: watchedReact }));
+
 mock.module("@/hooks/use-all-connections", () => ({
   useAllConnections: () => ({
     connections: mockGetConnections(),
@@ -237,6 +268,23 @@ function getTargetCallback() {
   return selectCallbacks.get("__empty__") || selectCallbacks.get("");
 }
 
+/**
+ * Record every state write the panel performs, until `stop()` is called.
+ *
+ * Switched on AFTER the panel has been unmounted, so what it returns is exactly the set of
+ * writes a dead component performed - which must be empty.
+ */
+function recordStateWrites() {
+  stateWrites = [];
+  return {
+    stop() {
+      const seen = stateWrites ?? [];
+      stateWrites = null;
+      return seen;
+    },
+  };
+}
+
 /** Helper to set native input value and trigger React change handler */
 function changeInput(input: HTMLInputElement, value: string) {
   // React controlled inputs need nativeInputValueSetter
@@ -263,6 +311,15 @@ describe("SchemaDiff", () => {
     selectCallbacks.clear();
     capturedTimelineProps = {};
 
+    // The default behaviour of the two write mocks, restored here rather than only at their
+    // declaration: `mockClear` forgets the CALLS and keeps the IMPLEMENTATION, so a test that
+    // swaps one for a store with the real filter-by-id or the real 50-row cap would otherwise
+    // hand that store to every test after it.
+    mockSaveSchemaSnapshot.mockImplementation((snapshot?: unknown) => {
+      if (snapshot !== undefined) savedSnapshots.push(snapshot);
+    });
+    mockDeleteSchemaSnapshot.mockImplementation(() => {});
+
     mockDiffSchemas.mockImplementation(() => structuredClone(mockDiffWithChanges));
     mockGenerateMigrationSQL.mockImplementation(
       () => "CREATE TABLE new_table (\n  id integer\n);\nDROP TABLE old_table;",
@@ -586,6 +643,67 @@ describe("SchemaDiff", () => {
       expect(mockSaveSchemaSnapshot).toHaveBeenCalledTimes(1);
     });
 
+    /**
+     * The double-Enter guard, measured. `snapshotInFlight` is a ref and not state because
+     * both presses land in the SAME tick, before React has re-rendered, so both see the
+     * `snapshotting` their callback closed over - `false` - and both begin a read.
+     *
+     * "One snapshot was saved" is not the measurement, and the test above is green with the
+     * guard deleted: the second read supersedes the first, the first asks `isCurrent()` and
+     * is told no, so exactly one snapshot is written either way. What the missing guard
+     * really costs is the two things below, and they are the two things the user pays for -
+     * the database read twice for one press of Save, and a banner blaming them for a race
+     * they did not cause.
+     */
+    test("Enter twice reads the database ONCE, not twice", async () => {
+      const mount = answerSchemaReads();
+      const { getByText, getByPlaceholderText } = renderDiff();
+      await act(async () => {});
+      mount.restore();
+
+      // A fresh answer, so the count below is the snapshot's reads and not the mount's.
+      const { fetchMock, restore } = answerSchemaReads();
+      fireEvent.click(getByText("Snapshot"));
+      const input = getByPlaceholderText("Label (optional)...") as HTMLInputElement;
+      await act(async () => {
+        fireEvent.keyDown(input, { key: "Enter" });
+        fireEvent.keyDown(input, { key: "Enter" });
+      });
+      restore();
+
+      // `inventory` is the read that matters - the whole object surface of the database.
+      // Without the guard this is 2: one press of Save, two round trips to the server.
+      const inventoryReads = (fetchMock.mock.calls as unknown[][]).filter((c) =>
+        String(c[0]).includes("inventory"),
+      ).length;
+      expect(inventoryReads).toBe(1);
+      expect(mockSaveSchemaSnapshot).toHaveBeenCalledTimes(1);
+    });
+
+    test("Enter twice leaves no banner the user did nothing to earn", async () => {
+      // The half a person actually sees. Without the guard the first read is superseded by
+      // the second, takes that for someone else asking for a newer read, and raises
+      // "the schema was read again before this finished. Press Save again" - over a snapshot
+      // that WAS saved. The user pressed Save, it worked, and the panel tells them it did not.
+      const mount = answerSchemaReads();
+      const { getByText, getByPlaceholderText, queryByText } = renderDiff();
+      await act(async () => {});
+      mount.restore();
+
+      const { restore } = answerSchemaReads();
+      fireEvent.click(getByText("Snapshot"));
+      const input = getByPlaceholderText("Label (optional)...") as HTMLInputElement;
+      await act(async () => {
+        fireEvent.keyDown(input, { key: "Enter" });
+        fireEvent.keyDown(input, { key: "Enter" });
+      });
+      restore();
+
+      expect(mockSaveSchemaSnapshot).toHaveBeenCalledTimes(1);
+      expect(queryByText(/No snapshot was saved/)).toBeNull();
+      expect(queryByText(/Press Save again/)).toBeNull();
+    });
+
     test("a storage refusal unlocks the button and says so", async () => {
       // Snapshots live in localStorage and a snapshot is a whole schema, so a quota refusal
       // is ordinary. Before the `finally`, this left the button reading "Reading..." for the
@@ -698,6 +816,85 @@ describe("SchemaDiff", () => {
       globalThis.fetch = orig;
       restore();
     });
+    test("leaving the Diff tab while a snapshot read is in flight saves nothing", async () => {
+      // The lock on the label input and Cancel stops the panel being closed out from under a
+      // save that is already running - and it only covers the panel's own buttons. Leaving the
+      // tab walks straight past it: `BottomPanel` mounts one view at a time, so changing tabs
+      // UNMOUNTS this, the read lands afterwards and the snapshot is written for a panel that
+      // is gone, with no banner, no refreshed list, and nothing on screen saying it happened.
+      // Measured before the fix: one snapshot saved after the panel had left the screen.
+      const origFetch = globalThis.fetch;
+      try {
+        const a = answerSchemaReads([{ name: "a_table" }]);
+        let view!: ReturnType;
+        await act(async () => {
+          view = renderDiff();
+        });
+        a.restore();
+
+        // Save is pressed, and THAT read is held open.
+        const held = holdSchemaRead([{ name: "a_table" }]);
+        fireEvent.click(view.getByText("Snapshot"));
+        await act(async () => {
+          fireEvent.click(view.getByText("Save"));
+        });
+
+        // The user changes tabs while it is still out. One view at a time, so this is an
+        // unmount and not a hidden panel.
+        mockSaveSchemaSnapshot.mockClear();
+        await act(async () => {
+          view.unmount();
+        });
+
+        await act(async () => {
+          held.release();
+          await new Promise((r) => setTimeout(r, 0));
+        });
+        held.restore();
+
+        // Nothing is written for a panel that is no longer on screen.
+        expect(mockSaveSchemaSnapshot).not.toHaveBeenCalled();
+      } finally {
+        globalThis.fetch = origFetch;
+      }
+    });
+
+    test("leaving the Diff tab while a REMOTE fetch is in flight saves nothing either", async () => {
+      // The other read that writes a snapshot. Choosing a connection to compare against
+      // auto-saves what it reads as a "Live:" snapshot, so the same tab change leaves the
+      // same litter behind - a snapshot of a database nobody is looking at, written by a
+      // panel that no longer exists. It runs on its own counter, so it needs its own answer.
+      const origFetch = globalThis.fetch;
+      try {
+        const a = answerSchemaReads([{ name: "a_table" }]);
+        let view!: ReturnType;
+        await act(async () => {
+          view = renderDiff();
+        });
+        a.restore();
+
+        const held = holdSchemaRead([{ name: "remote_table" }]);
+        const target = getTargetCallback();
+        await act(async () => {
+          target?.("conn:remote-1");
+        });
+
+        mockSaveSchemaSnapshot.mockClear();
+        await act(async () => {
+          view.unmount();
+        });
+
+        await act(async () => {
+          held.release();
+          await new Promise((r) => setTimeout(r, 0));
+        });
+        held.restore();
+
+        expect(mockSaveSchemaSnapshot).not.toHaveBeenCalled();
+      } finally {
+        globalThis.fetch = origFetch;
+      }
+    });
 
     test("a snapshot read that lands after the connection changed saves nothing and is not kept", async () => {
       // A read is not instant - it opens a connection and asks a catalog - and the panel
@@ -871,10 +1068,161 @@ describe("SchemaDiff", () => {
       }
     });
 
-    test("a later successful read clears the banner a failed snapshot left", async () => {
+    /**
+     * Two reads of the SAME connection, settled in an order the test chooses.
+     *
+     * `holdSchemaRead` gates one read by swapping `globalThis.fetch` wholesale, so it cannot
+     * express the ordinary order: the snapshot's own read settling FIRST and the read that
+     * overtook it settling after. That order is the common one - the overtaken read was
+     * started earlier, so it is answering the older question and usually answers first - and
+     * it is the order the defect lives in, so it has to be expressible. `provider-meta` still
+     * answers at once; every inventory read parks here until the test settles it by index.
+     */
+    function queuedSchemaReads() {
+      const orig = globalThis.fetch;
+      type ReadOutcome = { ok: true; objects: Array<{ name: string }> } | { ok: false; error: string };
+      type Answer = { ok: boolean; json: () => Promise };
+      const pending: Array<(outcome: ReadOutcome) => void> = [];
+      globalThis.fetch = mock((url: string) =>
+        String(url).includes("provider-meta")
+          ? Promise.resolve({
+              ok: true,
+              json: () =>
+                Promise.resolve({
+                  capabilities: {
+                    queryLanguage: "sql",
+                    objectKinds: [{ id: "table", role: "relation", label: "Table", labelPlural: "Tables" }],
+                  },
+                }),
+            })
+          : new Promise((resolve) => {
+              pending.push((outcome) =>
+                resolve({
+                  ok: outcome.ok,
+                  json: () =>
+                    Promise.resolve(
+                      outcome.ok
+                        ? {
+                            objects: outcome.objects.map((o) => ({
+                              name: o.name,
+                              kind: "table",
+                              path: ["public", o.name],
+                            })),
+                            details: outcome.objects.map((o) => ({
+                              path: ["public", o.name],
+                              columns: [],
+                              indexes: [],
+                              foreignKeys: [],
+                            })),
+                          }
+                        : { error: outcome.error },
+                    ),
+                }),
+              );
+            }),
+      ) as unknown as typeof fetch;
+      /** Let every read that has been ISSUED get as far as this queue. */
+      const flush = () =>
+        act(async () => {
+          await new Promise((r) => setTimeout(r, 0));
+        });
+      const settle = async (index: number, outcome: ReadOutcome) => {
+        pending[index](outcome);
+        await flush();
+      };
+      return { pending, flush, settle, restore: () => void (globalThis.fetch = orig) };
+    }
+
+    test("an overtaken snapshot still says so when the newer read settles AFTER it", async () => {
+      // The ordinary order, and the one the earlier attempt never ran. The overtaken read
+      // was started first, so it is the first to answer: it raises the banner, and the read
+      // that overtook it lands a tick later. Clearing the report on any successful read of
+      // this connection wiped the banner in that tick, and what the user was left with was a
+      // button back at "Save", nothing saved, and nothing on screen - the exact silence the
+      // banner exists to end.
+      const q = queuedSchemaReads();
+      try {
+        let view!: ReturnType;
+        await act(async () => {
+          view = renderDiff();
+        });
+        await q.flush();
+        await q.settle(0, { ok: true, objects: [{ name: "users" }] });
+
+        fireEvent.click(view.getByText("Snapshot"));
+        await act(async () => {
+          fireEvent.click(view.getByText("Save"));
+        });
+        await q.flush();
+
+        // Choosing a target begins a newer read of the same connection.
+        await act(async () => {
+          changeTarget("snap-1");
+        });
+        await q.flush();
+
+        mockSaveSchemaSnapshot.mockClear();
+        // The overtaken snapshot answers FIRST.
+        await q.settle(1, { ok: true, objects: [{ name: "users" }] });
+        expect(view.queryByText(/read again before this finished/)).not.toBeNull();
+
+        // The read that overtook it answers second, and it is a read of this connection
+        // that worked - which is precisely what used to wipe the banner.
+        await q.settle(2, { ok: true, objects: [{ name: "users" }] });
+
+        expect(mockSaveSchemaSnapshot).not.toHaveBeenCalled();
+        expect(view.queryByText(/read again before this finished/)).not.toBeNull();
+        expect(view.queryByText("Reading...")).toBeNull();
+      } finally {
+        q.restore();
+      }
+    });
+
+    test("an overtaken snapshot survives the panel's OWN re-read of the same connection", async () => {
+      // The other way a newer read starts, and nothing the user did. The embedded host hands
+      // over a fresh connection OBJECT with the same id - `use-connection-adapter.ts` builds
+      // it with a `useMemo` over a prop - so the mount effect runs again and reads the same
+      // database. The snapshot in flight is overtaken all the same, and the banner has to
+      // outlive that read too, not just a target selection.
+      const q = queuedSchemaReads();
+      try {
+        let view!: ReturnType;
+        await act(async () => {
+          view = renderDiff();
+        });
+        await q.flush();
+        await q.settle(0, { ok: true, objects: [{ name: "users" }] });
+
+        fireEvent.click(view.getByText("Snapshot"));
+        await act(async () => {
+          fireEvent.click(view.getByText("Save"));
+        });
+        await q.flush();
+
+        // Same id, different object.
+        await act(async () => {
+          view.rerender();
+        });
+        await q.flush();
+
+        mockSaveSchemaSnapshot.mockClear();
+        await q.settle(1, { ok: true, objects: [{ name: "users" }] });
+        expect(view.queryByText(/read again before this finished/)).not.toBeNull();
+        await q.settle(2, { ok: true, objects: [{ name: "users" }] });
+
+        expect(mockSaveSchemaSnapshot).not.toHaveBeenCalled();
+        expect(view.queryByText(/read again before this finished/)).not.toBeNull();
+      } finally {
+        q.restore();
+      }
+    });
+
+    test("a snapshot failure on one connection does not sit over another that is working", async () => {
+      // The reason the report carries the connection it is about. A banner over a database
+      // the user is not looking at is its own small lie, and the panel outlives a switch.
       const origFetch = globalThis.fetch;
       try {
-        const a = answerSchemaReads([{ name: "users" }]);
+        const a = answerSchemaReads([{ name: "a_table" }]);
         let view!: ReturnType;
         await act(async () => {
           view = renderDiff();
@@ -890,15 +1238,155 @@ describe("SchemaDiff", () => {
         });
         expect(await view.findByText(/No snapshot was saved: the database blinked/)).toBeTruthy();
 
-        // The panel reads this same database again - choosing a target does - and it works.
-        // The old banner is about a moment the user has already walked away from.
-        const c = answerSchemaReads([{ name: "users" }]);
+        // The user moves to another database, which reads cleanly.
+        const b = answerSchemaReads([{ name: "b_table" }]);
         await act(async () => {
-          changeTarget("snap-1");
+          view.rerender();
+          await new Promise((r) => setTimeout(r, 0));
+        });
+        b.restore();
+        expect(view.queryByText(/No snapshot was saved/)).toBeNull();
+
+        // Back on the connection it was about, it is still true: nothing was written for it,
+        // and a later read of it that works does not write it.
+        const c = answerSchemaReads([{ name: "a_table" }]);
+        await act(async () => {
+          view.rerender();
           await new Promise((r) => setTimeout(r, 0));
         });
         c.restore();
+        expect(view.queryByText(/No snapshot was saved: the database blinked/)).not.toBeNull();
+      } finally {
+        globalThis.fetch = origFetch;
+      }
+    });
+
+    test("a panel-read failure and a snapshot failure are two facts, and both stay on screen", async () => {
+      // One says what "Current Schema" currently means; the other says a snapshot the user
+      // asked for was not written. Neither answers the other, so neither may erase it.
+      const origFetch = globalThis.fetch;
+      try {
+        const a = answerSchemaReads([{ name: "users" }]);
+        let view!: ReturnType;
+        await act(async () => {
+          view = renderDiff();
+        });
+        a.restore();
+
+        // Nothing is written for the snapshot.
+        globalThis.fetch = mock(() =>
+          Promise.resolve({ ok: false, json: () => Promise.resolve({ error: "the database blinked" }) }),
+        ) as unknown as typeof fetch;
+        fireEvent.click(view.getByText("Snapshot"));
+        await act(async () => {
+          fireEvent.click(view.getByText("Save"));
+        });
+        expect(await view.findByText(/No snapshot was saved: the database blinked/)).toBeTruthy();
+
+        // The panel reads this same connection again and it WORKS. That refreshes Current
+        // Schema. It does not write the snapshot, so it does not answer the report.
+        const b = answerSchemaReads([{ name: "users" }]);
+        await act(async () => {
+          view.rerender();
+          await new Promise((r) => setTimeout(r, 0));
+        });
+        b.restore();
+
+        // The read after that fails, so Current Schema falls back to the explorer's copy.
+        globalThis.fetch = mock(() =>
+          Promise.resolve({ ok: false, json: () => Promise.resolve({ error: "the catalog is gone" }) }),
+        ) as unknown as typeof fetch;
+        await act(async () => {
+          view.rerender();
+          await new Promise((r) => setTimeout(r, 0));
+        });
+
+        expect(view.queryByText(/which may be out of date: the catalog is gone/)).not.toBeNull();
+        expect(view.queryByText(/No snapshot was saved: the database blinked/)).not.toBeNull();
+      } finally {
+        globalThis.fetch = origFetch;
+      }
+    });
+
+    test("Dismiss is the way out, and it clears only the snapshot report", async () => {
+      // No read clears the report any more, so there has to be something on the screen that
+      // does. Pressing Save again is a retry that can fail again; leaving the connection only
+      // hides it. A labelled button is the only exit a user does not have to guess at.
+      const origFetch = globalThis.fetch;
+      try {
+        const a = answerSchemaReads([{ name: "users" }]);
+        let view!: ReturnType;
+        await act(async () => {
+          view = renderDiff();
+        });
+        a.restore();
+
+        globalThis.fetch = mock(() =>
+          Promise.resolve({ ok: false, json: () => Promise.resolve({ error: "the database blinked" }) }),
+        ) as unknown as typeof fetch;
+        fireEvent.click(view.getByText("Snapshot"));
+        await act(async () => {
+          fireEvent.click(view.getByText("Save"));
+        });
+        expect(await view.findByText(/No snapshot was saved: the database blinked/)).toBeTruthy();
+
+        // The panel's own read of this connection is failing at the same time.
+        await act(async () => {
+          view.rerender();
+          await new Promise((r) => setTimeout(r, 0));
+        });
+        expect(view.queryByText(/which may be out of date: the database blinked/)).not.toBeNull();
+
+        await act(async () => {
+          fireEvent.click(view.getByText("Dismiss"));
+        });
+        expect(view.queryByText(/No snapshot was saved/)).toBeNull();
+        // The panel's own warning is not the snapshot report and is left where it was.
+        expect(view.queryByText(/which may be out of date: the database blinked/)).not.toBeNull();
 
+        // Dismissing one report does not silence the next. The label panel is still open -
+        // a save that failed leaves what was typed where it was - so Save is still there to
+        // press, and failing again says so again.
+        await act(async () => {
+          fireEvent.click(view.getByText("Save"));
+        });
+        expect(await view.findByText(/No snapshot was saved: the database blinked/)).toBeTruthy();
+      } finally {
+        globalThis.fetch = origFetch;
+      }
+    });
+
+    test("a snapshot that works clears the report the last failed one left", async () => {
+      // The report is spent when the thing it reports on is done. A new attempt clears it at
+      // the start, so a snapshot that is written leaves nothing behind.
+      const origFetch = globalThis.fetch;
+      try {
+        const a = answerSchemaReads([{ name: "users" }]);
+        let view!: ReturnType;
+        await act(async () => {
+          view = renderDiff();
+        });
+        a.restore();
+
+        globalThis.fetch = mock(() =>
+          Promise.resolve({ ok: false, json: () => Promise.resolve({ error: "the database blinked" }) }),
+        ) as unknown as typeof fetch;
+        fireEvent.click(view.getByText("Snapshot"));
+        await act(async () => {
+          fireEvent.click(view.getByText("Save"));
+        });
+        expect(await view.findByText(/No snapshot was saved: the database blinked/)).toBeTruthy();
+
+        // The label panel is still open after a failure, so the same Save is pressed again.
+        const b = answerSchemaReads([{ name: "users" }]);
+        mockSaveSchemaSnapshot.mockClear();
+        await act(async () => {
+          fireEvent.click(view.getByText("Save"));
+          await new Promise((r) => setTimeout(r, 0));
+        });
+        b.restore();
+
+        expect(mockSaveSchemaSnapshot).toHaveBeenCalled();
         expect(view.queryByText(/No snapshot was saved/)).toBeNull();
       } finally {
         globalThis.fetch = origFetch;
@@ -959,25 +1447,551 @@ describe("SchemaDiff", () => {
       expect(queryByText(/1 added, 1 removed, 1 modified/)).toBeTruthy();
     });
 
-    test("selecting same source and target shows same-schema message", () => {
-      const { getByText } = renderDiff();
-      changeTarget("current");
-      // source=current, target=current → same → null diff
-      expect(getByText("Cannot compare same schema with itself")).toBeTruthy();
+    test("selecting same source and target shows same-schema message", () => {
+      const { getByText } = renderDiff();
+      changeTarget("current");
+      // source=current, target=current → same → null diff
+      expect(getByText("Cannot compare same schema with itself")).toBeTruthy();
+    });
+
+    test("changing source updates diff", () => {
+      renderDiff();
+      changeSource("snap-1");
+      changeTarget("current");
+      expect(mockDiffSchemas).toHaveBeenCalled();
+    });
+  });
+
+  // ═══════════════════════════════════════════════════════════════════════════
+  // Reading the database again without leaving the tab (#35)
+  // ═══════════════════════════════════════════════════════════════════════════
+
+  describe("refreshing the current schema", () => {
+    /**
+     * Every inventory read parked until this test settles it, by index.
+     *
+     * `pending.length` is therefore the number of reads the panel has ISSUED, which is the
+     * measurement these tests are about: the defect is a gesture that issues none.
+     *
+     * Local rather than borrowed from the snapshot block, which gates reads the same way:
+     * the helpers there are scoped to that block, and lifting them out would have rewritten
+     * the tests that hold the snapshot rules to prove something about a button.
+     */
+    function schemaReads() {
+      const orig = globalThis.fetch;
+      type Outcome = { ok: true; objects: string[] } | { ok: false; error: string };
+      type Answer = { ok: boolean; json: () => Promise };
+      const pending: Array<(outcome: Outcome) => void> = [];
+      globalThis.fetch = mock((url: string) => {
+        if (String(url).includes("provider-meta")) {
+          return Promise.resolve({
+            ok: true,
+            json: () =>
+              Promise.resolve({
+                capabilities: {
+                  queryLanguage: "sql",
+                  objectKinds: [{ id: "table", role: "relation", label: "Table", labelPlural: "Tables" }],
+                },
+              }),
+          });
+        }
+        return new Promise((resolve) => {
+          pending.push((outcome) =>
+            resolve({
+              ok: outcome.ok,
+              json: () =>
+                Promise.resolve(
+                  outcome.ok
+                    ? {
+                        objects: outcome.objects.map((name) => ({ name, kind: "table", path: ["public", name] })),
+                        details: outcome.objects.map((name) => ({
+                          path: ["public", name],
+                          columns: [],
+                          indexes: [],
+                          foreignKeys: [],
+                        })),
+                      }
+                    : { error: outcome.error },
+                ),
+            }),
+          );
+        });
+      }) as unknown as typeof fetch;
+      /** Let every read that has been ISSUED get as far as this queue. */
+      const flush = () =>
+        act(async () => {
+          await new Promise((r) => setTimeout(r, 0));
+        });
+      const settle = async (index: number, outcome: Outcome) => {
+        pending[index](outcome);
+        await flush();
+      };
+      return { pending, flush, settle, restore: () => void (globalThis.fetch = orig) };
+    }
+
+    /** The control by its label, which is also its accessible name. */
+    function refreshButton(view: ReturnType) {
+      const buttons = Array.from(view.container.querySelectorAll("button"));
+      return buttons.find((b) => /refresh/i.test(b.textContent ?? ""));
+    }
+
+    /** Mount, answer the panel's own read, choose a target, answer that read too. */
+    async function openOnADiff(q: ReturnType) {
+      let view!: ReturnType;
+      await act(async () => {
+        view = renderDiff();
+      });
+      await q.flush();
+      await q.settle(0, { ok: true, objects: ["users"] });
+      await act(async () => {
+        changeTarget("snap-1");
+      });
+      await q.flush();
+      await q.settle(1, { ok: true, objects: ["users"] });
+      return view;
+    }
+
+    /** What "Current Schema" was worth the last time the diff was computed. */
+    function currentSideNames() {
+      const latest = (mockDiffSchemas.mock.calls as unknown[][]).at(-1)!;
+      return (latest[0] as Array<{ name: string }>).map((o) => o.name);
+    }
+
+    test("choosing the target that is ALREADY chosen reads nothing", async () => {
+      // The defect itself. The panel re-reads when the comparison target CHANGES, and a
+      // Select reports a selection only when the value lands on something else - so picking
+      // the same target again is the most the panel can even be told: `setTargetId` with the
+      // id it already holds. React bails out, the effect keyed on that id does not run, and
+      // the database is not read. The gesture a person makes for "look again" does nothing,
+      // which is why it cannot be the answer to #35.
+      const q = schemaReads();
+      try {
+        const view = await openOnADiff(q);
+        expect(q.pending.length).toBe(2);
+
+        const pickTheSameTargetAgain = selectCallbacks.get("snap-1")!;
+        await act(async () => {
+          pickTheSameTargetAgain("snap-1");
+        });
+        await q.flush();
+
+        expect(q.pending.length).toBe(2);
+        expect(view.container.textContent).toContain("Schema Diff");
+      } finally {
+        q.restore();
+      }
+    });
+
+    test("a fresh read can be asked for without leaving the tab", async () => {
+      // The other half of #35, and the half that matters: the panel shipped with exactly one
+      // way to see a change - leave the Diff tab and come back, because `BottomPanel` mounts
+      // one view at a time and returning is a remount. This is that step removed. The user
+      // changes the database, presses the control, and the side that says "current" is the
+      // database as it is now.
+      const q = schemaReads();
+      try {
+        const view = await openOnADiff(q);
+        expect(currentSideNames()).toEqual(["users"]);
+
+        const refresh = refreshButton(view);
+        expect(refresh).toBeTruthy();
+        await act(async () => {
+          fireEvent.click(refresh!);
+        });
+        await q.flush();
+
+        // A read was issued, and no remount happened to issue it.
+        expect(q.pending.length).toBe(3);
+        await q.settle(2, { ok: true, objects: ["added_after_the_snapshot"] });
+        expect(currentSideNames()).toEqual(["added_after_the_snapshot"]);
+      } finally {
+        q.restore();
+      }
+    });
+
+    test("a slow refresh overtaken by a newer read does not win", async () => {
+      // The refresh goes through the panel's read counter rather than around it. Press
+      // Refresh, then Save: the snapshot's read is the newer question, and the refresh is
+      // the slow one that answers LAST with what the database said BEFORE. Writing on the
+      // way out would put a stale "Current Schema" on screen under a snapshot that was just
+      // taken - the stale-copy defect this panel exists to have stopped.
+      const q = schemaReads();
+      try {
+        const view = await openOnADiff(q);
+
+        await act(async () => {
+          fireEvent.click(refreshButton(view)!);
+        });
+        await q.flush();
+        expect(q.pending.length).toBe(3);
+
+        fireEvent.click(view.getByText("Snapshot"));
+        await act(async () => {
+          fireEvent.click(view.getByText("Save"));
+        });
+        await q.flush();
+        expect(q.pending.length).toBe(4);
+
+        // The newer read answers first...
+        await q.settle(3, { ok: true, objects: ["what_the_database_holds_now"] });
+        // ...and the refresh, which was started earlier, answers after it with older objects.
+        await q.settle(2, { ok: true, objects: ["stale_from_the_refresh"] });
+
+        expect(currentSideNames()).toEqual(["what_the_database_holds_now"]);
+        expect(currentSideNames()).not.toContain("stale_from_the_refresh");
+        // The snapshot was the current read, so it is kept - superseding runs one way only.
+        expect(mockSaveSchemaSnapshot).toHaveBeenCalledTimes(1);
+      } finally {
+        q.restore();
+      }
+    });
+
+    test("two clicks in one tick read the database ONCE, not twice", async () => {
+      // The same guard the Save button needs, for the same reason: both clicks land in one
+      // tick, before React has re-rendered, so both see the `refreshing` the handler closed
+      // over - false - and the `disabled` that would have stopped the second is not on the
+      // button yet. The counter keeps the older read from WRITING, so the data stays right;
+      // what breaks is the screen, because the first read to settle runs the `finally` and
+      // hands the button back while the read the user is waiting for is still out.
+      const q = schemaReads();
+      try {
+        const view = await openOnADiff(q);
+        const refresh = refreshButton(view)!;
+
+        await act(async () => {
+          fireEvent.click(refresh);
+          fireEvent.click(refresh);
+        });
+        await q.flush();
+
+        expect(q.pending.length).toBe(3);
+      } finally {
+        q.restore();
+      }
+    });
+
+    test("the button is locked while its own read is in flight, and comes back after", async () => {
+      const q = schemaReads();
+      try {
+        const view = await openOnADiff(q);
+        await act(async () => {
+          fireEvent.click(refreshButton(view)!);
+        });
+        await q.flush();
+
+        expect(refreshButton(view)!.disabled).toBe(true);
+        expect(refreshButton(view)!.textContent).toContain("Refreshing");
+
+        await q.settle(2, { ok: true, objects: ["users"] });
+
+        expect(refreshButton(view)!.disabled).toBe(false);
+        expect(refreshButton(view)!.textContent?.trim()).toBe("Refresh");
+      } finally {
+        q.restore();
+      }
+    });
+
+    test("the button is disabled when there is no connection to read", () => {
+      const view = renderDiff({ connection: null });
+      expect(refreshButton(view)!.disabled).toBe(true);
+    });
+
+    test("it is a keyboard-reachable control with an accessible name", async () => {
+      // A native
)} + {/* A third line, beside the two above, because it is a third fact: not what Current + Schema means, and not a snapshot that went unwritten, but a database the user asked + to compare AGAINST that never arrived - so the comparison on screen is still the + old one. It said nothing at all before this, which is the worst of the three: the + spinner stopped and the panel looked finished. Same shape and same Dismiss as the + snapshot report rather than a second style of error surface, because it is the same + kind of statement - something you asked for was not done, spent only by you. */} + {remoteFailure !== null && ( +
+ + + {`Nothing was fetched from ${remoteFailure.connectionName}, so the comparison on screen is not that database: ${remoteFailure.reason}`} + + +
+ )} + {/* Content */}
{!targetId ? ( diff --git a/tests/components/SchemaDiff.test.tsx b/tests/components/SchemaDiff.test.tsx index 94bf0cf1a..7977457d5 100644 --- a/tests/components/SchemaDiff.test.tsx +++ b/tests/components/SchemaDiff.test.tsx @@ -3132,6 +3132,201 @@ describe("SchemaDiff", () => { } }); }); + + // ───────────────────────────────────────────────────────────────────────── + // A fetch that FAILS (#46) + // ───────────────────────────────────────────────────────────────────────── + + /** + * Nested here rather than in a block of its own, because it is the same path and these + * are the same reads: the queue, `twoFetchesOut`, `targetValue`, `savedFrom` and `busy` + * above are exactly what a failure has to be measured against. + */ + describe("a failure the user can see", () => { + /** Mount, answer the panel's own read, then ask ONE remote connection for its schema. */ + async function oneFetchOut(q: ReturnType) { + let view!: ReturnType; + await act(async () => { + view = renderDiff(); + }); + await q.flush(); + await q.settle(0, { ok: true, objects: ["users"] }); + await act(async () => { + getTargetCallback()?.("conn:remote-1"); + }); + await q.flush(); + expect(q.pending.length).toBe(2); + return view; + } + + /** What "Current Schema" was worth the last time the diff was computed. */ + function currentSideNames() { + const latest = (mockDiffSchemas.mock.calls as unknown[][]).at(-1)!; + return (latest[0] as Array<{ name: string }>).map((o) => o.name); + } + + test("a fetch that fails says so, and says the comparison is not the one asked for", async () => { + // The defect. The fetch failed, the spinner went down, the target stayed where it + // was - and the only trace was a log line nobody standing in front of the panel can + // read. The user is looking at the comparison they had BEFORE and believes it is the + // database they just picked. + const warn = spyOn(logger, "warn").mockImplementation(() => {}); + const q = queuedReads(); + try { + const view = await oneFetchOut(q); + await q.settle(1, { ok: false, error: "password authentication failed" }); + + const text = view.container.textContent ?? ""; + // What the database said... + expect(text).toContain("password authentication failed"); + // ...which database did not answer... + expect(text).toContain("Remote PG"); + // ...and what is on screen instead of it. Phrased so it stays true for as long as + // the banner is up: choosing a stored target afterwards changes the comparison, + // and a message that said "unchanged" would quietly become a lie. + expect(text).toMatch(/comparison on screen is not that database/i); + + // Nothing was written, which is why the message has to exist at all. + expect(targetValue(view.container)).toBe(""); + expect(savedFrom()).toEqual([]); + expect(busy(view)).toBe(false); + // Still logged: the log is the record of what the database said, and it stays. + expect(warn).toHaveBeenCalled(); + } finally { + q.restore(); + warn.mockRestore(); + } + }); + + test("Dismiss is the way out, exactly as it is for a snapshot that was not saved", async () => { + const warn = spyOn(logger, "warn").mockImplementation(() => {}); + const q = queuedReads(); + try { + const view = await oneFetchOut(q); + await q.settle(1, { ok: false, error: "password authentication failed" }); + expect(view.container.textContent).toContain("password authentication failed"); + + const dismiss = Array.from(view.container.querySelectorAll("button")).find( + (b) => b.textContent?.trim() === "Dismiss", + ); + expect(dismiss).toBeTruthy(); + await act(async () => { + fireEvent.click(dismiss!); + }); + + expect(view.container.textContent).not.toContain("password authentication failed"); + } finally { + q.restore(); + warn.mockRestore(); + } + }); + + test("an older fetch failing while the newer one is still out says nothing", async () => { + // The message belongs to the read the user is WAITING on. A connection they turned + // away from failing afterwards is not their question being answered, and a banner + // about it would be a report on a database nobody asked about any more. + const warn = spyOn(logger, "warn").mockImplementation(() => {}); + const q = queuedReads(); + try { + const view = await twoFetchesOut(q); + + await q.settle(1, { ok: false, error: "the connection you left is gone" }); + + expect(view.container.textContent).not.toContain("the connection you left is gone"); + expect(busy(view)).toBe(true); + + await q.settle(2, { ok: true, objects: ["prod_table"] }); + + expect(view.container.textContent).not.toContain("the connection you left is gone"); + expect(savedFrom()).toEqual(["remote-2"]); + // Logged all the same, whichever read it was. + expect(warn).toHaveBeenCalled(); + } finally { + q.restore(); + warn.mockRestore(); + } + }); + + test("an older fetch failing AFTER the newer one landed says nothing either", async () => { + const warn = spyOn(logger, "warn").mockImplementation(() => {}); + const q = queuedReads(); + try { + const view = await twoFetchesOut(q); + + await q.settle(2, { ok: true, objects: ["prod_table"] }); + const chosen = lastSavedId(); + + await q.settle(1, { ok: false, error: "the connection you left is gone" }); + + expect(view.container.textContent).not.toContain("the connection you left is gone"); + // The comparison the user DID ask for is untouched by the other one's failure. + expect(targetValue(view.container)).toBe(chosen!); + expect(busy(view)).toBe(false); + } finally { + q.restore(); + warn.mockRestore(); + } + }); + + test("a fetch that works clears the message the failed one left", async () => { + // Spent by the next attempt, like the snapshot report: a fetch that arrives is the + // answer to the one that did not. + const warn = spyOn(logger, "warn").mockImplementation(() => {}); + const q = queuedReads(); + try { + const view = await oneFetchOut(q); + await q.settle(1, { ok: false, error: "password authentication failed" }); + expect(view.container.textContent).toContain("password authentication failed"); + + await act(async () => { + getTargetCallback()?.("conn:remote-2"); + }); + await q.flush(); + await q.settle(2, { ok: true, objects: ["prod_table"] }); + + expect(view.container.textContent).not.toContain("password authentication failed"); + expect(targetValue(view.container)).toBe(lastSavedId()!); + } finally { + q.restore(); + warn.mockRestore(); + } + }); + + test("the panel's own read, overtaken while it was still out, does not become Current Schema", async () => { + // Not the remote path: the read the panel makes on the way IN. It is guarded like + // every other read here, and nothing measured that guard - it was removed while this + // defect was being reported and the whole suite stayed green. A read from the moment + // the panel opened winning over a newer one puts a stale "Current Schema" on screen, + // which is the defect this panel exists to have stopped. + const q = queuedReads(); + try { + let view!: ReturnType; + await act(async () => { + view = renderDiff(); + }); + await q.flush(); + expect(q.pending.length).toBe(1); + + // A target is chosen while that read is still out: a newer read of the SAME + // connection, on the same counter, which supersedes it. + await act(async () => { + changeTarget("snap-1"); + }); + await q.flush(); + expect(q.pending.length).toBe(2); + + await q.settle(1, { ok: true, objects: ["what_the_database_holds_now"] }); + // The read from the panel opening answers last, with what the database held before. + await q.settle(0, { ok: true, objects: ["stale_from_the_panel_opening"] }); + + expect(currentSideNames()).toEqual(["what_the_database_holds_now"]); + expect(currentSideNames()).not.toContain("stale_from_the_panel_opening"); + expect(view.container.textContent).toContain("Schema Diff"); + } finally { + q.restore(); + } + }); + }); }); // ═══════════════════════════════════════════════════════════════════════════ From 648b8a31889b69c81021e2b13ae17144c0673ace Mon Sep 17 00:00:00 2001 From: Yusuf GUNDOGDU Date: Fri, 18 Sep 2026 21:15:38 +0300 Subject: [PATCH 11/19] docs: put SQL Server back in the agent-mode engine list An earlier commit on this branch removed SQL Server from the list of engines agent mode reads. That was wrong and it was unrelated to what the commit was for. mssql.ts implements queryReadOnly, and engine-support.ts lists four engines, not three, which is what the engine-support test reads from the real providers. It was also applied unevenly, so the documents contradicted each other: README, DOCKERHUB and FEATURES said three engines while AGENT_GUIDE and the Chinese, Japanese and Hindi READMEs still said four. FEATURES went further and listed three engines in one sentence and then called them "those two engines". The English text is restored and the sentence that counted them is right again. The four-layer read-only profile SQL Server needs, which has no read-only transaction of its own, is described where the other three engines' profiles are. README also carried four source references with line numbers that do not hold those symbols - postgres.ts:915, sqlite.ts:537, duckdb/index.ts:525 and runtime.ts:199. The line numbers are gone rather than corrected, because they go stale on the next edit and the file name alone is enough to find the symbol. The Spanish and Urdu READMEs never carried this claim, so nothing changed there. --- DOCKERHUB.md | 2 +- README.md | 25 +++++++++++++++---------- docs/FEATURES.md | 4 ++-- 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/DOCKERHUB.md b/DOCKERHUB.md index 755b6b727..4a445e046 100644 --- a/DOCKERHUB.md +++ b/DOCKERHUB.md @@ -161,7 +161,7 @@ Details, probed versions and each caveat: [`docs/providers/README.md`](https://g - **Professional SQL IDE** — Monaco editor (VS Code engine), schema-aware autocomplete, multi-tab workspace, Visual EXPLAIN. - **Interactive ER diagrams** — real FK edges, cardinality, auto-layout (ELK.js), PNG/SVG export. - **Schema diff & migration** — compare snapshots/connections and auto-generate migration SQL. -- **Read-only database agent** — state an objective, and the run drafts SQL, reads the results and composes a report whose claims cite them. Three workflows (investigate / optimize / assess), a visible statement-and-time budget, and writes refused before the database is reached. **Agent mode reads PostgreSQL, SQLite and DuckDB only** — they are the only engines with a database-native read-only execution profile, and on any other engine an Agent-mode run ends `engine-unsupported`; Plan mode is toolless, runs no statement of yours, and is **grounded in your own schema on every engine** — it reads the inventory before the model's first turn and asks for one statement in that engine's own language, or refuses with `NO STATEMENT:` and the question that would unblock it. Standalone image only. [Guide](https://github.com/libredb/libredb-studio/blob/main/docs/AGENT_GUIDE.md) · [What leaves the machine](https://github.com/libredb/libredb-studio/blob/main/docs/AGENT_DATA_FLOW.md). +- **Read-only database agent** — state an objective, and the run drafts SQL, reads the results and composes a report whose claims cite them. Three workflows (investigate / optimize / assess), a visible statement-and-time budget, and writes refused before the database is reached. **Agent mode reads PostgreSQL, SQLite, DuckDB and SQL Server only** — they are the only engines with a database-native read-only execution profile, and on any other engine an Agent-mode run whose workflow sends statements is refused when it is started, with `engine-unsupported`; Plan mode is toolless, runs no statement of yours, and is **grounded in your own schema on every engine** — it reads the inventory before the model's first turn and asks for one statement in that engine's own language, or refuses with `NO STATEMENT:` and the question that would unblock it. Standalone image only. [Guide](https://github.com/libredb/libredb-studio/blob/main/docs/AGENT_GUIDE.md) · [What leaves the machine](https://github.com/libredb/libredb-studio/blob/main/docs/AGENT_DATA_FLOW.md). - **Model-backed helpers** — query safety analysis, EXPLAIN-in-plain-English, AI-generated schema docs, data-profile summaries. Gemini / OpenAI / Ollama / custom; with no model configured — no `LLM_*` variables at all — no AI call is made. A key is required for Gemini and OpenAI only: Ollama and a custom endpoint count as a configured model without one, which enables the AI features — and the agent too, once its ledger path is writable. - **Pro data grid** — virtualized millions of rows, inline editing, per-column filters, pivot table, CSV/JSON export. - **Data visualization** — 8 chart types with aggregation and saved-chart dashboards. diff --git a/README.md b/README.md index f1cecce28..841351870 100644 --- a/README.md +++ b/README.md @@ -161,21 +161,26 @@ what comes back, and finishes by composing a report whose every claim cites the goes through the agent's own audited pipeline — a policy decision, an audit event and budget accounting before the driver is touched (`executeAuditedOperation`, `src/lib/db/operations/execution.ts:129`) — under a read-only execution profile: a read-only transaction on PostgreSQL, `PRAGMA query_only` - re-asserted per statement on SQLite, and a `READ_ONLY` engine handle on DuckDB paired with an + re-asserted per statement on SQLite, a `READ_ONLY` engine handle on DuckDB paired with an SQL-level guard, because that flag alone still lets `COPY … TO`, `EXPORT DATABASE` and the - local-file table functions through. Writes and DDL are refused before the database is reached, + local-file table functions through, and, on SQL Server, which has no read-only transaction of any + kind, a session principal verified at open to be unable to write, an optimizer admission that + compiles each statement without running it, a server-side row bound, and a transaction that is + always rolled back. Writes and DDL are refused before the database is reached, and `EXPLAIN ANALYZE` is default-denied because it would run the statement. This pipeline is the agent's alone: statements you run yourself in the editor call the provider directly (`src/app/api/db/query/route.ts:44`) and are neither policy-checked nor audited this way. -- **Agent mode reads PostgreSQL, SQLite and DuckDB only.** The read-only profile is database-native, - so it exists only where a provider implements it — `queryReadOnly` on `postgres.ts:915`, - `sqlite.ts:537` and `duckdb/index.ts:525`, and nowhere else. On any other engine, an Agent-mode run ends `engine-unsupported` - (`src/lib/agent/runtime.ts:199`). **Plan** mode opens on every connection — the model there is - toolless, runs no statement of yours, writes nothing, and drafts a statement for you to run - yourself. Its GROUNDING reaches every engine: on PostgreSQL and SQLite the server composes catalog - statements itself, and on every other connection it asks that connection's own provider to describe its +- **Agent mode reads PostgreSQL, SQLite, DuckDB and SQL Server only.** The read-only profile is + database-native, so it exists only where a provider implements it — `queryReadOnly` on + `postgres.ts`, `sqlite.ts`, `duckdb/index.ts` and `mssql.ts`, and nowhere else. On any other engine + an Agent-mode run whose workflow sends statements is refused when it is started, before a run is + opened, and any that reaches the provider factory ends `engine-unsupported`. **Plan** mode opens on + every connection — the model there is toolless, runs no statement of yours, writes nothing, and + drafts a statement for you to run yourself. Its GROUNDING reaches every engine: on PostgreSQL and + SQLite the server composes catalog statements itself, and on every other connection it asks that + connection's own provider to describe its schema — the reading the sidebar already performs — which needs no read-only statement path. So the - two limits are separate: agent mode is those three engines, grounding is all of them, and a run whose + two limits are separate: agent mode is those four engines, grounding is all of them, and a run whose reading fails says so plainly rather than inventing tables. - **Three workflows**: **Investigate** (answer a question), **Optimize** (compare estimated plans, propose an index or a rewrite), **Assess** (profile tables — counts only, never values). diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 90bd8a0d3..b20994cc3 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -140,8 +140,8 @@ ### 18. The Database Agent (read-only investigation runs) * **A run, not a chat:** you state an objective and press Start; the run drafts SQL against the connected database, reads the results, and composes a report whose every claim cites the result it came from. An uncited claim is refused, so it cannot be composed at all. -* **Read-only, enforced by the database:** every statement the agent runs goes through the agent's own audited pipeline — a policy decision, an audit event and budget accounting before the driver is touched, through `executeAuditedOperation()` ([`execution.ts`](../src/lib/db/operations/execution.ts)) — under a read-only execution profile: a read-only transaction on PostgreSQL, `PRAGMA query_only` re-asserted per statement on SQLite, and a `READ_ONLY` engine handle plus an SQL-level guard on DuckDB — the flag alone is not a filesystem sandbox, since `COPY … TO`, `EXPORT DATABASE`, `INSTALL`/`LOAD` and the local-file table functions all succeed under it. Writes and DDL are refused before the database is reached, and `EXPLAIN ANALYZE` is default-denied because it would execute the statement. The pipeline is the agent's alone and is not shared with the editor: a statement you run yourself calls the provider directly in `POST()` ([`query/route.ts`](../src/app/api/db/query/route.ts)), receiving neither the policy decision nor the audit event. -* **Agent mode is PostgreSQL, SQLite and DuckDB only — except Operate:** the read-only profile is database-native, so it exists only where a provider implements `queryReadOnly` — [`postgres.ts`](../src/lib/db/providers/sql/postgres.ts), [`sqlite.ts`](../src/lib/db/providers/sql/sqlite.ts) and [`duckdb/index.ts`](../src/lib/db/providers/sql/duckdb/index.ts), and no other provider does. On MySQL, Oracle, SQL Server, libSQL, MongoDB, Redis, ClickHouse, Druid, Couchbase, Elasticsearch, OpenSearch, Trino or Cassandra an Agent-mode run ends `engine-unsupported` in `driveAgentRun()` ([`runtime.ts`](../src/lib/agent/runtime.ts)). The search providers implement no `queryReadOnly` and could not: their SQL grammars have no transaction and no session-scoped setting to make read-only, and the surface is already read-only in the grammar itself, which is a different guarantee from one the database enforces per statement. The **Operate** workflow is the exception and runs on every engine, because it sends no SQL at all: it reads the engine's own reporting interface, which every provider implements. Plan mode opens on every connection: its model is handed no tools, so no read-only profile has to be acquired for it. It is not blind, though — since 2026-08-15 the server reads the connection's schema and the engine's own estimated statistics before the model's first turn. That **grounding** reaches every engine: on PostgreSQL and SQLite the server composes catalog statements and reads them through that same read-only path, and on every other connection it asks the provider to describe its own schema — the reading the sidebar already performs when it lists your tables, which needs no read-only statement path. So the two limits are separate ones: agent mode is those two engines, grounding is all of them, and a run whose reading fails — refused, overran its time, or rejected by the engine — says so rather than inventing tables. +* **Read-only, enforced by the database:** every statement the agent runs goes through the agent's own audited pipeline — a policy decision, an audit event and budget accounting before the driver is touched, through `executeAuditedOperation()` ([`execution.ts`](../src/lib/db/operations/execution.ts)) — under a read-only execution profile: a read-only transaction on PostgreSQL, `PRAGMA query_only` re-asserted per statement on SQLite, a `READ_ONLY` engine handle plus an SQL-level guard on DuckDB — the flag alone is not a filesystem sandbox, since `COPY … TO`, `EXPORT DATABASE`, `INSTALL`/`LOAD` and the local-file table functions all succeed under it. On SQL Server the profile is four layers instead of one, because the engine has no read-only transaction and no session-level read-only switch: a session principal verified at open to be unable to write or to reach the server's dangerous surfaces, an admission step that asks the optimizer to compile each statement without running it, a server-side row bound (`SET ROWCOUNT`) that stops an unbounded read before the result is materialised at all, and a pinned transaction that is always rolled back. Writes and DDL are refused before the database is reached, and `EXPLAIN ANALYZE` is default-denied because it would execute the statement. The pipeline is the agent's alone and is not shared with the editor: a statement you run yourself calls the provider directly in `POST()` ([`query/route.ts`](../src/app/api/db/query/route.ts)), receiving neither the policy decision nor the audit event. +* **Agent mode is PostgreSQL, SQLite, DuckDB and SQL Server only — except Operate:** the read-only profile is database-native, so it exists only where a provider implements `queryReadOnly` — [`postgres.ts`](../src/lib/db/providers/sql/postgres.ts), [`sqlite.ts`](../src/lib/db/providers/sql/sqlite.ts), [`duckdb/index.ts`](../src/lib/db/providers/sql/duckdb/index.ts) and [`mssql.ts`](../src/lib/db/providers/sql/mssql.ts), and no other provider does. On MySQL, Oracle, libSQL, MongoDB, Redis, ClickHouse, Druid, Couchbase, Elasticsearch, OpenSearch, Trino, Cassandra or the embedded LibreDB store an Agent-mode run whose workflow sends statements is refused by `POST /api/agent/runs` before a run id exists, and one that reaches the provider factory ends `engine-unsupported` in `driveAgentRun()` ([`runtime.ts`](../src/lib/agent/runtime.ts)). The search providers implement no `queryReadOnly` and could not: their SQL grammars have no transaction and no session-scoped setting to make read-only, and the surface is already read-only in the grammar itself, which is a different guarantee from one the database enforces per statement. The **Operate** workflow is the exception and runs on every engine, because it sends no SQL at all: it reads the engine's own reporting interface, which every provider implements. Plan mode opens on every connection: its model is handed no tools, so no read-only profile has to be acquired for it. It is not blind, though — since 2026-08-15 the server reads the connection's schema and the engine's own estimated statistics before the model's first turn. That **grounding** reaches every engine: on PostgreSQL and SQLite the server composes catalog statements and reads them through that same read-only path, and on every other connection it asks the provider to describe its own schema — the reading the sidebar already performs when it lists your tables, which needs no read-only statement path. So the two limits are separate ones: agent mode is those four engines, grounding is all of them, and a run whose reading fails — refused, overran its time, or rejected by the engine — says so rather than inventing tables. * **Two independent axes:** the **mode** (Plan, whose model is toolless and whose deliverable is one statement for you to run yourself — the run executes no statement of yours and writes nothing — or Agent) and the **workflow** (Investigate, Optimize, Assess, Operate, Analyze). Both are fixed when the run opens and read from the run's own record thereafter. * **Operate reads the live server, not its tables:** the slowest queries, who is connected and what is blocked, table and index statistics, storage and health — each a curated reading the server takes through the provider's own reporting interface, stored as an ordinary citable artifact. Every reading is a point in time, and both the prompt and the timeline say so rather than letting a report imply a trend was measured. * **Counts, never values:** the Assess workflow's table profiling composes aggregates only — row counts, present counts, distinct counts, and shape matches computed inside the database. There is deliberately no `min`/`max`, because on a text column those return real values. From f431c559b4a9334e6fae4f291df10170e2b20d15 Mon Sep 17 00:00:00 2001 From: Yusuf GUNDOGDU Date: Fri, 18 Sep 2026 21:15:48 +0300 Subject: [PATCH 12/19] test(sqlite): cover the two record guards at the row seam The coverage gate was red at 59092 of 59094 lines. The two uncovered lines were the scalar-bigint and typed-array branches of the driver's record normalizer, both added by the 64-bit integer commit on this branch, and the repository requires every line. The seam they sit on is typed unknown deliberately: the statement interface declares all() as unknown[] and get() as unknown, so what arrives is whatever the injected driver returns. Measured against both shipped drivers, for all(), get(), run(), a miss and a PRAGMA read, every answer is a row object, a null miss or run()'s info object, and a BLOB is a cell inside a row rather than the record itself. So neither branch is on a path those two take today. They are what stops a driver that answers with a bare cell - a values mode, or the future row-returning method the module's own comment warns about - from handing a BigInt out of the provider or walking a typed array cell by cell. They are driven through the injectable constructor the module already exposes for exactly this, so no driver is mocked and no source changed. Each test states what it kills: removing the scalar branch sends a BigInt through JSON.stringify, which refuses it outright, and removing the typed-array branch writes converted cells back into a typed array that will not hold them. --- tests/unit/db/sqlite-driver.test.ts | 87 +++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/tests/unit/db/sqlite-driver.test.ts b/tests/unit/db/sqlite-driver.test.ts index a9e93a328..5f60d4cc3 100644 --- a/tests/unit/db/sqlite-driver.test.ts +++ b/tests/unit/db/sqlite-driver.test.ts @@ -8,9 +8,11 @@ import { normalizeSQLiteBigInt, resolveSQLiteDriverName, toSQLiteBindValue, + type BunSQLiteConstructor, type BunSQLiteOpenOptions, type NodeDatabaseSyncLike, type NodeSQLiteModule, + type SQLiteConstructor, type SQLiteDatabase, type SQLiteStatement, } from "@/lib/db/providers/sql/sqlite-driver"; @@ -581,3 +583,88 @@ describe("toSQLiteBindValue()", () => { } }); }); + +// ============================================================================ +// The two record guards at the row seam +// ============================================================================ +// Every row, every single-row read and every write result crosses the same private +// normalizer inside the driver, and the seam it crosses is typed `unknown` on purpose: +// `SQLiteStatement` declares `all(): unknown[]` and `get(): unknown`, so what arrives is +// whatever the injected driver hands back. +// +// Measured 2026-09-18 against both shipped drivers (bun:sqlite on Bun 1.4.0, node:sqlite +// on Node 24), for all(), get(), run(), a miss and a PRAGMA read: every one of them +// answers with a row OBJECT, a null/undefined miss, or run()'s info object. A BLOB is a +// Uint8Array CELL inside a row, never the record itself. So neither guard below is on a +// path those two drivers take today - they are what keeps a driver that answers with a +// bare cell (a raw/values mode, or the "future row-returning driver method" the module +// warns about) from being handed on as a BigInt or walked index by index. They are +// pinned through the injectable constructor, which is the seam this module already +// exposes so its semantics can be driven without the real driver. + +/** A statement stand-in whose every read answers with one fixed record. */ +function statementReturning(record: unknown): SQLiteStatement { + return { + all: () => [record], + get: () => record, + run: () => ({ changes: 1 }), + }; +} + +/** The bun adapter, wired to a driver whose every read answers with `record`. */ +function driverReturning(record: unknown): SQLiteConstructor { + class RecordDatabase implements SQLiteDatabase { + exec(): void {} + prepare(): SQLiteStatement { + return statementReturning(record); + } + close(): void {} + readonly inTransaction = false; + } + return createBunSQLiteDriver(RecordDatabase as BunSQLiteConstructor); +} + +describe("the record seam's guards", () => { + // Kills "hand a bare integer straight on": without this branch the record falls + // through to the object guard, which sends a BigInt out of the provider whole. + // Reached through get(), which is the entry point that USES the returned value; + // all() normalizes its rows in place and discards what the normalizer returns, so a + // bare cell can only be corrected on the single-row path. + test("a bare 64-bit integer is converted at the seam, not handed on as a BigInt", () => { + // Outside the safe range: every digit kept, as the decimal string the rest of this + // provider already answers with. + const huge = new (driverReturning(BigInt("9007199254740993")))(":memory:").prepare("SELECT id FROM t"); + expect(huge.get()).toBe("9007199254740993"); + + // Inside it: the same value as a number, so a COUNT(*) or a `1` is unchanged. + const small = new (driverReturning(BigInt("1")))(":memory:").prepare("SELECT 1"); + expect(small.get()).toBe(1); + + // Why the conversion has to happen HERE: rows are sent to the browser with + // JSON.stringify, which refuses a BigInt outright. A record that skipped this + // branch would throw on the way out instead of reaching the grid. + expect(() => JSON.stringify(huge.get())).not.toThrow(); + expect(() => JSON.stringify(small.get())).not.toThrow(); + }); + + // Kills "walk it like a row": the loop the guard skips writes converted cells BACK + // into the record, which a typed array of 64-bit integers refuses. + test("a typed array is handed back untouched, not walked cell by cell", () => { + // The shape the driver names: a BLOB. The same object comes back, bytes intact. + const blob = new Uint8Array([0, 1, 254, 255]); + const blobStmt = new (driverReturning(blob))(":memory:").prepare("SELECT data FROM t"); + expect(blobStmt.get()).toBe(blob); + expect(Array.from(blob)).toEqual([0, 1, 254, 255]); + + // And the shape that proves it is the GUARD doing the work rather than the loop + // simply finding no BigInt cell: a typed array whose cells ARE 64-bit integers. + // Walking it converts cell 0 to the number 1 and writes it back, and a + // BigInt64Array cell cannot take a number - so an unguarded walk throws here. + const cells = new BigInt64Array([BigInt(1), BigInt("9007199254740993")]); + const cellStmt = new (driverReturning(cells))(":memory:").prepare("SELECT ids FROM t"); + expect(cellStmt.get()).toBe(cells); + expect(Array.from(cells).map(String)).toEqual(["1", "9007199254740993"]); + // The multi-row path walks each row through the same normalizer, so it is guarded too. + expect(cellStmt.all()).toEqual([cells]); + }); +}); From d4ecfa50abe270542988917a19fdd315c18c5bd9 Mon Sep 17 00:00:00 2001 From: Yusuf GUNDOGDU Date: Fri, 18 Sep 2026 21:22:20 +0300 Subject: [PATCH 13/19] fix(docs): stop handing a working login to the API examples, and close four holes in the guard A reviewer put working passwords into this repository four ways and the guard that is supposed to stop that stayed green each time. Every one is now caught, with the nearest innocent shape written beside it so the next person it stops does not delete it. A Markdown table row was the worst of them, because a variable table is how these files list their settings: the name is one cell and the value is the next, and nothing between them is an assignment. Only a value cell written as a code span with no space in it counts, which is what tells the value column from the description column beside it. The other three were one rule being too strict. `NAME=value` had to be followed by a comment, a pipe, another flag or the end of the line, so `docker run -e ADMIN_PASSWORD=Secret123 imagename` and `export ADMIN_PASSWORD=Secret123 && echo` both read as prose. A sentence does not write an equals sign, so an `=` no longer asks what follows it; `NAME: value` still does, which is what keeps `ADMIN_PASSWORD: generated on first run` a sentence. And a quoted value was read to the first space, so a password with a space in it looked like one word followed by prose - the same hole hid a secret at full length, and a secret is judged by its length. One value had to be excused: deploy/azure/src/install.sh writes `printf 'ADMIN_PASSWORD=%s\n'` and pours the password in from a variable. A run made entirely of format specifiers is the hole, not the value. docs/API_DOCS.md was still handing readers admin123 in two login examples, a cURL block and a fetch block, which is the same string removed from CONTRIBUTING.md by hand earlier. They now carry a placeholder and a line saying the real password is generated on first run and printed to the log. The guard reads login bodies now, found by the `email` key beside the password. Connection bodies are deliberately left alone: the password in one of those is the reader's own database, sampled as postgres or password123, and nothing here is reachable with it. --- docs/API_DOCS.md | 8 +- tests/unit/published-credentials.test.ts | 168 ++++++++++++++++++++++- 2 files changed, 170 insertions(+), 6 deletions(-) diff --git a/docs/API_DOCS.md b/docs/API_DOCS.md index 08d744386..b62fdf637 100644 --- a/docs/API_DOCS.md +++ b/docs/API_DOCS.md @@ -1679,10 +1679,14 @@ including login - is refused this way. ### cURL Examples #### Login + +The admin password is generated on first run and printed to the server log, or set +through `ADMIN_PASSWORD`. Put yours in place of the placeholder below. + ```bash curl -X POST http://localhost:3000/api/auth/login \ -H "Content-Type: application/json" \ - -d '{"email": "admin@libredb.org", "password": "admin123"}' \ + -d '{"email": "admin@libredb.org", "password": ""}' \ -c cookies.txt ``` @@ -1766,7 +1770,7 @@ async function executeQuery(sql: string) { await fetch('/api/auth/login', { method: 'POST', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ email: 'admin@libredb.org', password: 'admin123' }), + body: JSON.stringify({ email: 'admin@libredb.org', password: process.env.ADMIN_PASSWORD }), credentials: 'include' }); diff --git a/tests/unit/published-credentials.test.ts b/tests/unit/published-credentials.test.ts index c6a0996cf..077a9ef0b 100644 --- a/tests/unit/published-credentials.test.ts +++ b/tests/unit/published-credentials.test.ts @@ -102,6 +102,12 @@ function documentationFiles(): string[] { * Markdown table cell. So the line no longer has to end - the VALUE has to end, at one of * the four things that end one. Prose carries on in words, and a word matches none of these, * so `ADMIN_PASSWORD: generated on first run` is still a sentence and still unflagged. + * + * This applies to `NAME: value` only. `NAME=value` needs none of it: a sentence does not + * write an equals sign, so whatever follows the value is the rest of a command line - the + * image name of a `docker run`, an `&& echo`, a second statement. Requiring one of these + * four after an `=` is what let `docker run -e ADMIN_PASSWORD=example-fake-password img` publish a + * working login, measured. */ const VALUE_ENDS = /^\s*(?:#|$)|^\s*\||^\s+-{1,2}[A-Za-z]|^\s+[A-Za-z_][A-Za-z_0-9.]*\s*=/; @@ -139,6 +145,10 @@ function usableValue(raw: string): string | null { if (value === "" || value === "..." || /^\$/.test(value) || value.startsWith("{{") || value.startsWith("<")) { return null; } + // A printf format is a hole the value is poured into, not the value: + // `printf 'ADMIN_PASSWORD=%s\n' "$APP_ADMIN_PASSWORD"` writes the password from a variable. + // Only a run made ENTIRELY of specifiers and escapes counts, so `%s-2026` is still a value. + if (/^(?:%[-#0 +'0-9.]*[a-zA-Z]|\\[nrt0])+$/.test(value)) return null; return value; } @@ -169,6 +179,34 @@ function pairedAssignments(lines: string[], name: string): string[] { return found; } +/** + * A Markdown table row, which is how a README lists its variables: the name in one cell and + * the value in the next. It assigns nothing in the `NAME=value` sense and a reader still + * reads a working login out of it, which is how `| `ADMIN_PASSWORD` | `example-not-a-real-password` |` + * passed this guard - measured, on the most common table shape in these files. + * + * Only a value cell written as a code span with no space inside it counts. That is what + * tells the value column from the description column beside it: `| `ADMIN_PASSWORD` | Admin + * password |` is a table OF variables, not a table of credentials, and flagging those would + * light up every README here and get this guard deleted by the next person it stopped. + */ +function tableAssignments(lines: string[], name: string): string[] { + const found: string[] = []; + for (const line of lines) { + if (!/^\s*\|/.test(line)) continue; + const cells = line.split("|").map((cell) => cell.trim()); + for (const [index, cell] of cells.entries()) { + if (cell.replace(/`/g, "").trim() !== name) continue; + const span = /^`([^`\s]+)`$/.exec(cells[index + 1] ?? ""); + if (span === null) continue; + const value = usableValue(span[1]); + // A cell of punctuation - an em dash for "none", a lone hyphen - is not a password. + if (value !== null && /[A-Za-z0-9]/.test(value)) found.push(value); + } + } + return found; +} + /** * `NAME=value` in a shell example, or `NAME: value` in a compose one - plus the two-line * form above, because a credential does not stop working for being written across two lines. @@ -187,13 +225,53 @@ function assignments(text: string, name: string): string[] { if (at > 0 && /[A-Za-z_0-9.]/.test(line[at - 1])) continue; if (parents[index] === "existingSecretKeys") continue; const rest = line.slice(at + name.length).replace(/\\\s*$/, ""); - const assigned = /^\s*[=:]\s*(\S+)([\s\S]*)$/.exec(rest); + // A quoted value is taken whole. Reading up to the first space instead stopped at the + // first word, and a value with a space in it then looked like a value followed by prose, + // which the check below reads as a sentence: `ADMIN_PASSWORD="example fake admin password"` was + // published that way, measured, and so was a 42-character JWT_SECRET. + const assigned = /^\s*([=:])\s*("[^"]*"|'[^']*'|\S+)([\s\S]*)$/.exec(rest); if (assigned === null) continue; - if (!VALUE_ENDS.test(assigned[2])) continue; - const value = usableValue(assigned[1]); + if (assigned[1] === ":" && !VALUE_ENDS.test(assigned[3])) continue; + const value = usableValue(assigned[2]); if (value !== null) found.push(value); } - return [...found, ...pairedAssignments(lines, name)]; + return [...found, ...pairedAssignments(lines, name), ...tableAssignments(lines, name)]; +} + +/** + * Words that stand in for a password in the slot a login example puts one. A schema or an + * API table writes the TYPE there, and a sentence writes an instruction - which is why a + * value with a space in it is never read as one. + */ +const JSON_PASSWORD_STANDINS = new Set(["string", "password", "secret", "null", "undefined", "admin", "user"]); + +/** + * A login example's JSON body, which names no environment variable at all and so was read + * by none of the rules above: `-d '{"email": "...", "password": "example-fake-login"}'` published a + * working login in docs/API_DOCS.md twice, in a cURL block and a fetch() block, and the + * same string had already been removed from CONTRIBUTING.md by hand. + * + * What makes it a LOGIN body is the `email` beside it, and that is the whole test. The same + * documentation is full of CONNECTION bodies - `host`, `port`, `database`, `user`, + * `password` - and the password in one of those is the READER'S own database, sampled as + * `postgres` or `example-fake-connection-pw`. Nothing this project ships is reachable with those, and + * flagging them would put this guard in the way of writing a connection example at all. + * + * Of what is left, only a double-quoted literal that reads as a value counts: a space in it + * makes it an instruction, and a `your-` prefix makes it a placeholder. + */ +function jsonPasswordValues(text: string): string[] { + const found: string[] = []; + // `email` on either side of `password`, within one small object - not across a document. + const inLoginBody = + /"email"\s*:[\s\S]{0,120}?"(?:password|newPassword|currentPassword)"\s*:\s*"([^"]*)"|"(?:password|newPassword|currentPassword)"\s*:\s*"([^"]*)"[\s\S]{0,120}?"email"\s*:/g; + for (const match of text.matchAll(inLoginBody)) { + const value = usableValue(match[1] ?? match[2] ?? ""); + if (value === null || /\s/.test(value) || /^your[-_]/i.test(value)) continue; + if (JSON_PASSWORD_STANDINS.has(value.toLowerCase())) continue; + found.push(value); + } + return found; } function publishedValues(file: string, name: string): string[] { @@ -229,6 +307,36 @@ describe("the documentation publishes no credential that works", () => { expect(offenders).toEqual([]); }); + test("hands no working password to a login example", () => { + const offenders: string[] = []; + for (const file of files) { + for (const value of jsonPasswordValues(readFileSync(file, "utf8"))) offenders.push(`${file}: ${value}`); + } + expect(offenders).toEqual([]); + }); + + test("reads a password out of a login body, and leaves a schema alone", () => { + const caught = (text: string) => jsonPasswordValues(text); + // The two shapes that were in docs/API_DOCS.md, one cURL and one fetch(). + expect(caught(`-d '{"email": "admin@libredb.org", "password": "example-fake-login"}'`)).toEqual(["example-fake-login"]); + expect(caught(`body: JSON.stringify({ "email": "a@b.c", "password": "example-not-a-real-password" })`)).toEqual(["example-not-a-real-password"]); + // The password before the email reads the same way. + expect(caught(`{\n "password": "example-fake-login",\n "email": "admin@libredb.org"\n}`)).toEqual(["example-fake-login"]); + + // A CONNECTION body is the reader's own database, not an account this project ships. + expect(caught(`{"host": "127.0.0.1", "user": "postgres", "password": "postgres"}`)).toEqual([]); + expect(caught(`{"host": "h", "port": 8091, "user": "Administrator", "password": "example-fake-connection-pw"}`)).toEqual([]); + + // And the stand-ins, or every API table in docs/ fails this guard. + expect(caught(`{"email": "a@b.c", "password": "string"}`)).toEqual([]); + expect(caught(`{"email": "a@b.c", "password": "your-password"}`)).toEqual([]); + expect(caught(`{"email": "a@b.c", "password": "your admin password"}`)).toEqual([]); + expect(caught(`{"email": "a@b.c", "password": ""}`)).toEqual([]); + expect(caught(`{"email": "a@b.c", "password": "$ADMIN_PASSWORD"}`)).toEqual([]); + expect(caught(`{"email": "a@b.c", "password": ""}`)).toEqual([]); + expect(caught(`| \`password\` | string | required |`)).toEqual([]); + }); + test("assigns no secret the server would accept", () => { const offenders: string[] = []; for (const file of files) { @@ -273,4 +381,56 @@ describe("the documentation publishes no credential that works", () => { ), ).toEqual([]); }); + + test("reads the four shapes a reviewer published a working password through", () => { + const caught = (text: string, name: string) => assignments(text, name); + + // 1. The variable table, which is how every README here lists its settings. The name and + // the value are two cells of one row and nothing between them is an assignment. + expect(caught("| `ADMIN_PASSWORD` | `example-not-a-real-password` | the admin login |", "ADMIN_PASSWORD")).toEqual([ + "example-not-a-real-password", + ]); + expect(caught("| ADMIN_PASSWORD | `example-not-a-real-password` |", "ADMIN_PASSWORD")).toEqual(["example-not-a-real-password"]); + + // 2. A shell line that carries on after the value. + expect(caught("export ADMIN_PASSWORD=example-not-a-real-password && echo ok", "ADMIN_PASSWORD")).toEqual(["example-not-a-real-password"]); + + // 3. A value with a space in it, which used to be read as one word plus prose. + expect(caught('ADMIN_PASSWORD="example fake admin password"', "ADMIN_PASSWORD")).toEqual(["example fake admin password"]); + expect(caught("USER_PASSWORD='example fake user password'", "USER_PASSWORD")).toEqual(["example fake user password"]); + // The same hole let a secret through, and a secret is judged by its LENGTH, so reading + // one word of it hid a value the server would have accepted. + const secret = caught('JWT_SECRET="an example fake secret of forty chars xx"', "JWT_SECRET"); + expect(secret).toEqual(["an example fake secret of forty chars xx"]); + expect(secret[0].length).toBeGreaterThanOrEqual(JWT_SECRET_MIN_LENGTH); + + // 4. `docker run` with the image name after the value, rather than another flag. + expect(caught("docker run -e ADMIN_PASSWORD=example-not-a-real-password libredb/libredb-studio", "ADMIN_PASSWORD")).toEqual([ + "example-not-a-real-password", + ]); + }); + + test("stays quiet on the innocent shapes nearest to those four", () => { + const caught = (text: string, name: string) => assignments(text, name); + + // A table OF variables. The cell beside the name is a description, and if these fired + // the guard would be deleted by the next person it stopped. + expect(caught("| `ADMIN_PASSWORD` | Admin password | no |", "ADMIN_PASSWORD")).toEqual([]); + expect(caught("| `ADMIN_PASSWORD` | `generated on first run` |", "ADMIN_PASSWORD")).toEqual([]); + expect(caught("| `ADMIN_PASSWORD` | - | printed to the log |", "ADMIN_PASSWORD")).toEqual([]); + expect(caught("| `ADMIN_PASSWORD` | `` |", "ADMIN_PASSWORD")).toEqual([]); + expect(caught("| Variable | Value |\n| --- | --- |", "ADMIN_PASSWORD")).toEqual([]); + + // A printf format is the hole a value is poured into, not the value. This one is in + // deploy/azure/src/install.sh and the password it writes comes from a variable. + expect(caught(`printf 'ADMIN_PASSWORD=%s\\n' "$APP_ADMIN_PASSWORD"`, "ADMIN_PASSWORD")).toEqual([]); + + // A quoted stand-in is still a stand-in. + expect(caught('ADMIN_PASSWORD=""', "ADMIN_PASSWORD")).toEqual([]); + expect(caught('ADMIN_PASSWORD=""', "ADMIN_PASSWORD")).toEqual([]); + + // And prose still reads as prose, with a colon and without one. + expect(caught("ADMIN_PASSWORD: generated on first run and printed", "ADMIN_PASSWORD")).toEqual([]); + expect(caught("Set ADMIN_PASSWORD to a value of your own", "ADMIN_PASSWORD")).toEqual([]); + }); }); From d6867b2fa6d8da1043d7d6af2fb328a13119510d Mon Sep 17 00:00:00 2001 From: Yusuf GUNDOGDU Date: Fri, 18 Sep 2026 22:16:55 +0300 Subject: [PATCH 14/19] fix(sqlite): report declared column types, so an exported table keeps its integer column Every other provider sends the declared column types alongside the rows. The local SQLite provider never did, and until this branch nothing noticed: the exporter falls back to the JavaScript type of the value, and a SQLite integer used to arrive as a number. The 64-bit fix on this branch changed that. An id past 2^53 now arrives as a string, so a table carrying one exported as `"id" TEXT` with the value in quotes, and a user moving that table to another engine got a text column where they had an integer one. Measured: exported, restored, and `typeof(id)` came back `text`. libsql was unaffected the whole time, because it reports its declarations. This follows the libsql path exactly, down to omitting the key when the map is empty rather than sending `{}`. The two drivers spell the declarations differently, so one read-only accessor is bridged in sqlite-driver.ts the way `safeIntegers` and `inTransaction` already are, and nothing new reaches a shared surface. It is read after the rows, not before, because bun's `declaredTypes` raises until the statement has run. bun also publishes `columnTypes`, which is not this: it is the runtime class of the value, so a REAL column reads FLOAT and an undeclared one reads INTEGER, and it raises on `PRAGMA journal_mode`. Measured on Bun 1.4.0 and not used. An absent declaration stays absent. An expression, a literal, an aggregate, a function call, a PRAGMA column and a column declared with no type all get no entry, because two decisions on this branch now read this map and a guess in it would be worse than nothing. Not fixed here: a SQLite REAL key is still refused by the row editor, which asks whether the column is a 64-bit float and does not count `real` because on PostgreSQL and Trino it is 32 bits. On SQLite it is 64. That answer belongs in the editor, with the engine in hand. --- src/lib/db/providers/sql/sqlite-driver.ts | 116 +++++- src/lib/db/providers/sql/sqlite.ts | 40 +- tests/integration/db/sqlite-provider.test.ts | 361 +++++++++++++++++++ tests/unit/db/sqlite-driver.test.ts | 333 ++++++++++++++++- 4 files changed, 821 insertions(+), 29 deletions(-) diff --git a/src/lib/db/providers/sql/sqlite-driver.ts b/src/lib/db/providers/sql/sqlite-driver.ts index 9fa09e369..65022ae65 100644 --- a/src/lib/db/providers/sql/sqlite-driver.ts +++ b/src/lib/db/providers/sql/sqlite-driver.ts @@ -20,11 +20,48 @@ import { DatabaseConfigError } from "../../errors"; +/** + * One result column's name and the type it was DECLARED with — `undefined` where SQLite + * declared none. + * + * A PAIR rather than a map, because that is exactly what `declaredColumnTypes()` in + * `column-types.ts` already consumes from the four other drivers that answer this + * question, duplicate column names and all: `SELECT 1 AS c, name AS c` really does + * declare two columns called `c`, the row object keeps the last one, and the shared + * helper is where last-wins is decided. + */ +export type SQLiteDeclaredColumn = readonly [name: string, declaredType: string | undefined]; + // The exact driver surface the SQLite provider uses (bun:sqlite-shaped). export type SQLiteStatement = { all(...params: unknown[]): unknown[]; get(...params: unknown[]): unknown; run(...params: unknown[]): { changes: number }; + /** + * What the schema DECLARED each result column to be (`sqlite3_column_decltype`), in + * column order. Both drivers publish it and spell it differently — bun:sqlite + * `columnNames` beside `declaredTypes`, node:sqlite one `columns()` answering both — so + * the two are bridged here, exactly as `inTransaction` and the big-integer flag are. + * + * CALL IT AFTER THE ROWS. Measured 2026-09-18 on bun:sqlite (Bun 1.4.0) and node:sqlite + * (Node 24.14.0): bun THROWS "Statement must be executed before accessing declaredTypes" + * until the statement has run, while node answers either way — so after the rows is the + * one order both drivers accept. A statement that matched NO rows still answers + * (`SELECT id, r FROM t WHERE id = -1` → `INTEGER`, `REAL`), so an empty result is + * described rather than guessed at, and a write answers an EMPTY list on both. + * + * `undefined` is an ordinary answer and not a failure: an expression, a literal, an + * aggregate, a function call, every PRAGMA column and a column declared with no type at + * all have no declaration for SQLite to report, and both drivers say so with `null`. + * + * NOT bun:sqlite's `columnTypes`, which is a different question wearing a similar name. + * Measured the same day: it reports the RUNTIME storage class of the row just read — a + * `REAL` column answers `FLOAT`, an undeclared column answers whatever that row happens + * to hold — and it throws outright on anything that is not a read-only statement, + * `PRAGMA journal_mode` included. Reading it here would have typed every float column + * wrong and broken every PRAGMA the provider runs. + */ + declaredColumns(): readonly SQLiteDeclaredColumn[]; }; export type SQLiteDatabase = { @@ -226,6 +263,13 @@ function normalizeRecordInPlace(record: unknown): unknown { return record; } +/** + * A driver's OWN statement: the three row methods, before the bridge below adds the + * declarations. Neither driver publishes `declaredColumns` — it is this module's name for + * a question each of them answers its own way. + */ +type RawSQLiteStatement = Omit; + /** * Wrap one prepared statement so every row it returns, and every parameter it is given, * crosses the conversions above. @@ -239,9 +283,20 @@ function normalizeRecordInPlace(record: unknown): unknown { * PRAGMA read go through it alike, and a future row-returning driver method cannot * quietly bypass it. The parameters travel the same three methods, so the two * directions are inverses at ONE seam rather than at two that can drift apart. + * + * `declaredColumns` is handed in rather than read off `stmt`, because it is the one part + * of the surface the two drivers do not already spell the same way; each adapter below + * passes its own spelling and both come out of here as one method on one object. Keeping + * it on the SAME object as the rows is what makes "after the rows" checkable: a caller + * holds the statement that produced them and asks it, rather than holding a second handle + * whose order nothing constrains. */ -function withoutBigInts(stmt: SQLiteStatement): SQLiteStatement { +function withoutBigInts( + stmt: RawSQLiteStatement, + declaredColumns: SQLiteStatement["declaredColumns"], +): SQLiteStatement { return { + declaredColumns, all: (...params: unknown[]): unknown[] => { const rows = stmt.all(...toSQLiteBindValues(params)); for (const row of rows) { @@ -270,7 +325,22 @@ function withoutBigInts(stmt: SQLiteStatement): SQLiteStatement { * and no new option reaches any shared surface. */ export type BunSQLiteOpenOptions = SQLiteOpenOptions & { safeIntegers?: boolean }; -export type BunSQLiteConstructor = new (path: string, options?: BunSQLiteOpenOptions) => SQLiteDatabase; + +/** + * Minimal structural view of bun:sqlite's own Statement and Database, kept local for the + * reason `NodeDatabaseSyncLike` below is: the adapter then says exactly which of the + * driver's members it uses, and a stand-in can satisfy that and nothing more. + * + * `columnNames` and `declaredTypes` are bun's two halves of the answer node:sqlite gives + * in one `columns()` call, and the only reason this type exists at all - the rest of the + * surface was already bun-shaped. + */ +type BunStatementLike = RawSQLiteStatement & { + readonly columnNames: string[]; + readonly declaredTypes: (string | null)[]; +}; +export type BunDatabaseLike = Omit & { prepare(sql: string): BunStatementLike }; +export type BunSQLiteConstructor = new (path: string, options?: BunSQLiteOpenOptions) => BunDatabaseLike; /** * Adapt bun:sqlite's Database: open it with `safeIntegers`, and convert what the @@ -280,7 +350,7 @@ export type BunSQLiteConstructor = new (path: string, options?: BunSQLiteOpenOpt */ export function createBunSQLiteDriver(DatabaseCtor: BunSQLiteConstructor): SQLiteConstructor { class BunSQLiteDatabase implements SQLiteDatabase { - private readonly db: SQLiteDatabase; + private readonly db: BunDatabaseLike; constructor(dbPath: string, options?: SQLiteOpenOptions) { this.db = new DatabaseCtor(dbPath, { ...options, safeIntegers: true }); @@ -291,7 +361,13 @@ export function createBunSQLiteDriver(DatabaseCtor: BunSQLiteConstructor): SQLit } prepare(sql: string): SQLiteStatement { - return withoutBigInts(this.db.prepare(sql)); + const stmt = this.db.prepare(sql); + // Read lazily, never here: bun refuses `declaredTypes` until the statement has run + // (measured - see `SQLiteStatement.declaredColumns`), so reading it at `prepare()` + // would throw on every query the provider makes. + return withoutBigInts(stmt, () => + stmt.columnNames.map((name, index) => [name, stmt.declaredTypes[index] ?? undefined] as const), + ); } close(throwOnError?: boolean): void { @@ -312,6 +388,13 @@ type NodeStatementLike = { all(...params: unknown[]): unknown; get(...params: unknown[]): unknown; run(...params: unknown[]): { changes: number | bigint }; + /** + * node:sqlite's spelling of bun:sqlite's `columnNames` + `declaredTypes`: one call + * answering both, with `type` null where the column was declared with none. It carries + * `column`, `database` and `table` as well; the bridge reads neither, because the name + * the ROW object uses is `name` (the alias, where there is one). + */ + columns(): { name: string; type: string | null }[]; }; export type NodeDatabaseSyncLike = { exec(sql: string): void; @@ -371,6 +454,12 @@ async function loadBunDriver(): Promise { * - the big-integer flag is `readBigInts` here and `safeIntegers` on bun:sqlite; both * adapters set their own spelling and both send `prepare()` through the same * conversion, so the two drivers answer a 64-bit id identically (#39). + * - the DECLARED column types are `columns()[].name`/`.type` here and `columnNames` + + * `declaredTypes` on bun:sqlite; both adapters hand their own spelling to + * `withoutBigInts`, which republishes one `declaredColumns()` (#273). A handle that + * dropped it would leave the result carrying no types at all, which is the state this + * provider was in: the SQL export then names a column by the JavaScript type of its + * value, and the inline editor has nothing to read a key's width from. * - `get()` returns `undefined` on a miss where bun:sqlite returns `null`. * - `run()` reports `changes` as `number | bigint`; normalize to `number`. * @@ -392,14 +481,19 @@ export function createNodeSQLiteDriver(DatabaseSyncCtor: NodeSQLiteModule["Datab prepare(sql: string): SQLiteStatement { const stmt = this.db.prepare(sql); - return withoutBigInts({ - all: (...params: unknown[]): unknown[] => stmt.all(...params) as unknown[], - get: (...params: unknown[]): unknown => stmt.get(...params) ?? null, - run: (...params: unknown[]): { changes: number } => { - const info = stmt.run(...params); - return { changes: Number(info.changes) }; + return withoutBigInts( + { + all: (...params: unknown[]): unknown[] => stmt.all(...params) as unknown[], + get: (...params: unknown[]): unknown => stmt.get(...params) ?? null, + run: (...params: unknown[]): { changes: number } => { + const info = stmt.run(...params); + return { changes: Number(info.changes) }; + }, }, - }); + // node answers this before the statement has run as readily as after it, so the + // "after the rows" rule the bun half needs costs this half nothing. + () => stmt.columns().map((column) => [column.name, column.type ?? undefined] as const), + ); } // Takes no `throwOnError`, and needs none: node:sqlite's own close already finalizes diff --git a/src/lib/db/providers/sql/sqlite.ts b/src/lib/db/providers/sql/sqlite.ts index cf081d299..8e1c787bb 100644 --- a/src/lib/db/providers/sql/sqlite.ts +++ b/src/lib/db/providers/sql/sqlite.ts @@ -49,6 +49,7 @@ import { import { assertReadOnlyBudget, measureResultBytes } from "./read-only-budget"; import { formatBytes } from "../../utils/pool-manager"; import { loadSQLiteDriver, type SQLiteDatabase } from "./sqlite-driver"; +import { declaredColumnTypes } from "./column-types"; import { applySourceBound, callerBoundTruncationReason, @@ -1254,6 +1255,13 @@ export class SQLiteProvider extends SQLBaseProvider { rows: (rows as unknown[]).map((row) => row as Record) as Record[], fields, changes: 0, + // Declared types travel with the result (#273), and are read AFTER the rows + // because bun:sqlite refuses the question until the statement has run - see + // `SQLiteStatement.declaredColumns`, where both drivers were measured. + // `declaredColumnTypes` omits the key entirely when nothing was declared, + // which is the common case here rather than a failure: SQLite declares + // nothing for a computed column, a literal, an aggregate or any PRAGMA. + declared: declaredColumnTypes(stmt.declaredColumns()), }; } else { const stmt = this.db!.prepare(sql); @@ -1262,6 +1270,10 @@ export class SQLiteProvider extends SQLBaseProvider { rows: [], fields: [], changes: info.changes, + // A write has no result columns at all - measured, both drivers answer an + // EMPTY column list after `run()` - so there is nothing to declare, and the + // key is left off exactly as `fields: []` leaves the names off. + declared: {}, }; } } catch (error) { @@ -1274,6 +1286,7 @@ export class SQLiteProvider extends SQLBaseProvider { fields: result.fields, rowCount: result.rows.length || result.changes, executionTime, + ...result.declared, }; }); } @@ -1356,22 +1369,32 @@ export class SQLiteProvider extends SQLBaseProvider { this.enforceQueryOnly(); return this.trackQuery(async () => { - const { result, executionTime } = await this.measureExecution(async () => { + const { + result: { rows, declared }, + executionTime, + } = await this.measureExecution(async () => { try { - return this.db!.prepare(sql).all() as Record[]; + const stmt = this.db!.prepare(sql); + // The rows first and the declarations second, for the reason `query()` above + // states: bun:sqlite answers the second question only once the first has been + // asked. The budgets below are checked on the rows either way, so a result + // refused for being too large carries its types no further than it carries its + // rows. + const all = stmt.all() as Record[]; + return { rows: all, declared: declaredColumnTypes(stmt.declaredColumns()) }; } catch (error) { throw mapDatabaseError(error, "sqlite", sql); } }); - if (result.length > budget.maxResultRows) { + if (rows.length > budget.maxResultRows) { throw new QueryError( - `Read-only execution exceeded the row budget: ${result.length} rows > ${budget.maxResultRows} allowed`, + `Read-only execution exceeded the row budget: ${rows.length} rows > ${budget.maxResultRows} allowed`, "sqlite", sql, ); } - const resultBytes = measureResultBytes(result); + const resultBytes = measureResultBytes(rows); if (resultBytes > budget.maxResultBytes) { throw new QueryError( `Read-only execution exceeded the byte budget: ${resultBytes} bytes > ${budget.maxResultBytes} allowed`, @@ -1393,10 +1416,11 @@ export class SQLiteProvider extends SQLBaseProvider { } return { - rows: result, - fields: result.length > 0 ? Object.keys(result[0]) : [], - rowCount: result.length, + rows, + fields: rows.length > 0 ? Object.keys(rows[0]) : [], + rowCount: rows.length, executionTime, + ...declared, }; }); } diff --git a/tests/integration/db/sqlite-provider.test.ts b/tests/integration/db/sqlite-provider.test.ts index 949bd3feb..a59ccca23 100644 --- a/tests/integration/db/sqlite-provider.test.ts +++ b/tests/integration/db/sqlite-provider.test.ts @@ -54,6 +54,7 @@ import { QueryError, } from "@/lib/db/errors"; import { CACHE_HIT_RATIO_UNAVAILABLE } from "@/lib/monitoring-cache-ratio"; +import { buildResultExport } from "@/lib/export/result-export"; import { comparePaths } from "@/lib/db/object-path"; import { readFixtureStatements } from "../../../docker/sqlite-init/build-fixture"; @@ -747,6 +748,7 @@ describe("SQLiteProvider", () => { all: () => (sql.includes("dbstat") ? dbstat : owners), get: () => null, run: () => ({ changes: 0 }), + declaredColumns: () => [], }), }; @@ -1297,6 +1299,7 @@ function answerReadsMatching(provider: SQLiteProvider, match: string, rows: read all: () => [...rows], get: () => rows[0] ?? null, run: () => ({ changes: 0 }), + declaredColumns: () => [], })); } @@ -1323,6 +1326,7 @@ function captureReadsMatching( }, get: () => rows[0] ?? null, run: () => ({ changes: 0 }), + declaredColumns: () => [], }; }); return captured; @@ -3882,3 +3886,360 @@ describe("64-bit integers past 2^53, independently verified", () => { } }); }); + +// ============================================================================ +// Declared column types (#273) +// ============================================================================ +// +// Every other provider family fills `QueryResult.columnTypes`; the local SQLite one +// did not, and two things downstream read it and decide with it: +// +// - `src/lib/export/result-export.ts` falls back to the JAVASCRIPT TYPE of a value +// when no declaration came with the result. Since the 64-bit seam (#39) hands a +// key past 2^53 over as its digits, a SQLite `INTEGER PRIMARY KEY` holding +// 9007199254740993 was exported as `"id" TEXT` - measured below, by replaying the +// generated file into SQLite, where the column came back with the `text` storage +// class and the table was no longer keyed by an integer. +// - `src/hooks/use-inline-editing.ts` reads the declared type to decide whether a +// key can be sent back as the row that holds it, and with nothing declared it has +// to fail closed. +// +// The declarations themselves are SQLite's own `sqlite3_column_decltype`, bridged in +// `sqlite-driver.ts` because the two drivers spell the question differently. What is +// asserted HERE is the provider's half, on the bun driver these tests run under - the +// split this file has kept since it was written. The OTHER driver's half is asserted +// against the real node:sqlite module at the seam itself, in +// tests/unit/db/sqlite-driver.test.ts, over the same nine shapes; it is not repeated +// in-process here, because `loadSQLiteDriver` caches per driver NAME and the unit file +// deliberately caches a FAILURE under "node", which a second file sharing one bun +// process would then inherit. + +const DECLARATION_FIXTURE = [ + "CREATE TABLE decl (id INTEGER PRIMARY KEY, price REAL, label TEXT, payload BLOB, flag BOOLEAN, bare)", + "INSERT INTO decl VALUES (1, 1.5, 'first', x'0011', 1, 'anything')", + "CREATE VIEW decl_view AS SELECT id, price, label FROM decl", +]; + +describe("declared column types (#273)", () => { + let declared: SQLiteProvider; + + beforeAll(async () => { + declared = new SQLiteProvider(makeSQLiteConfig()); + await declared.connect(); + for (const statement of DECLARATION_FIXTURE) await declared.query(statement); + }); + + afterAll(async () => { + await declared.disconnect(); + }); + + test("the driver under test is the bun one, as this file's design assumes", () => { + expect(resolveSQLiteDriverName()).toBe("bun"); + }); + + /** + * What SQLite declares for each shape a result column can have. + * + * The `undefined` rows are the point of the table rather than gaps in it: SQLite + * declares a type for a column that came out of a TABLE and for nothing else, so a + * computed column, a literal, an aggregate, a function call, every PRAGMA column and + * a column created with no type at all have no declaration to carry, and the key is + * left OFF the result rather than filled with a guess. + */ + test.each([ + ["a plain column", "SELECT id, price, label FROM decl", { id: "INTEGER", price: "REAL", label: "TEXT" }], + ["an alias", "SELECT id AS ident, price AS ratio FROM decl", { ident: "INTEGER", ratio: "REAL" }], + ["a view column", "SELECT id, label FROM decl_view", { id: "INTEGER", label: "TEXT" }], + ["an expression", "SELECT id + 1 AS e, price * 2 AS e2 FROM decl", undefined], + ["a literal", "SELECT 1 AS one, 'x' AS ex", undefined], + ["an aggregate", "SELECT COUNT(*) AS c, SUM(id) AS s FROM decl", undefined], + ["a function call", "SELECT upper(label) AS u, sqlite_version() AS v FROM decl", undefined], + ["a PRAGMA column", "PRAGMA table_info(decl)", undefined], + ["an undeclared column", "SELECT bare FROM decl", undefined], + ])("%s", async (_shape, sql, expected) => { + const result = await declared.query(sql); + + expect(result.columnTypes).toEqual(expected); + // ABSENT rather than `{}` when nothing was declared, which is the contract every + // other provider already answers to: a consumer decides from the field's presence. + expect(Object.hasOwn(result, "columnTypes")).toBe(expected !== undefined); + // and the names are the result's own, so every declared key is a field on the row + for (const name of Object.keys(result.columnTypes ?? {})) expect(result.fields).toContain(name); + }); + + test("a result mixing a table column with a computed one declares only the table column", async () => { + const result = await declared.query("SELECT id, COUNT(*) AS n FROM decl GROUP BY id"); + + // Not all-or-nothing: `n` has no declaration and `id` does, and dropping the map + // because one column was undeclared would lose the one type that exists. + expect(result.fields).toEqual(["id", "n"]); + expect(result.columnTypes).toEqual({ id: "INTEGER" }); + }); + + test("a result that matched no rows is still described", async () => { + const result = await declared.query("SELECT id, price FROM decl WHERE id = -1"); + + // `fields` comes from row 0 and there is none, so this is the one case where the + // declaration says more than the rows do. It is also the case a value-shaped guess + // could never answer at all. + expect(result.rows).toEqual([]); + expect(result.fields).toEqual([]); + expect(result.columnTypes).toEqual({ id: "INTEGER", price: "REAL" }); + }); + + test("a write declares nothing, because it has no result columns", async () => { + const result = await declared.query("UPDATE decl SET label = 'first' WHERE id = 1"); + + expect(result.rowCount).toBe(1); + expect(Object.hasOwn(result, "columnTypes")).toBe(false); + }); + + test("two result columns of one name keep the type of the one the row kept", async () => { + const result = await declared.query("SELECT 1 AS c, label AS c FROM decl"); + + // SQLite really does answer two columns called `c`; the row object keeps the LAST, + // so the type that describes what the grid shows is the last one's too. + expect(result.rows).toEqual([{ c: "first" }]); + expect(result.columnTypes).toEqual({ c: "TEXT" }); + }); + + /** + * The spelling is the SCHEMA's, not the storage class of the row that was read. + * + * This is the whole reason bun:sqlite's `columnTypes` is not what the bridge reads. + * Measured 2026-09-18 on Bun 1.4.0, `columnTypes` for this very statement answers + * `["INTEGER", "FLOAT", "TEXT", "BLOB", "INTEGER", "TEXT"]`: the `REAL` column comes + * back as `FLOAT`, the `BOOLEAN` column as `INTEGER`, and the undeclared column as + * whatever that row happened to hold. Reading it would have renamed half the schema. + */ + test("the types are the ones the schema declares, not the storage classes of the values", async () => { + const result = await declared.query("SELECT id, price, label, payload, flag, bare FROM decl"); + + expect(result.columnTypes).toEqual({ + id: "INTEGER", + price: "REAL", + label: "TEXT", + payload: "BLOB", + flag: "BOOLEAN", + }); + // `bare` was created with no type, so it is the one column of the six with no entry. + expect(Object.hasOwn(result.columnTypes!, "bare")).toBe(false); + }); + + test("a 64-bit id that left the provider as digits is still declared INTEGER", async () => { + await declared.query("CREATE TABLE IF NOT EXISTS big_decl (id INTEGER PRIMARY KEY, label TEXT)"); + await declared.query("DELETE FROM big_decl"); + await declared.query("INSERT INTO big_decl VALUES (9007199254740993, 'target')"); + + const result = await declared.query("SELECT id, label FROM big_decl"); + + // The value is a string by the time it leaves the 64-bit seam (#39) and the column + // is an INTEGER all the same. Anything inferring the type from the value answers + // TEXT here, which is exactly the export defect below. + expect(result.rows).toEqual([{ id: "9007199254740993", label: "target" }]); + expect(result.columnTypes).toEqual({ id: "INTEGER", label: "TEXT" }); + }); +}); + +describe("declared column types on the read-only profile (#273 + #328)", () => { + let profileDir: string; + + beforeAll(() => { + profileDir = mkdtempSync(join(tmpdir(), "libredb-sqlite-decl-")); + }); + + afterAll(() => { + rmSync(profileDir, { recursive: true, force: true }); + }); + + test("the agent's read-only path carries them exactly as the ordinary path does", async () => { + const dbPath = join(profileDir, "profile.db"); + const seed = new SQLiteProvider(makeSQLiteConfig({ database: dbPath })); + await seed.connect(); + await seed.query("CREATE TABLE t (id INTEGER PRIMARY KEY, price REAL, v TEXT)"); + await seed.query("INSERT INTO t VALUES (1, 1.5, 'seeded')"); + await seed.disconnect(); + + const profile = new SQLiteProvider(makeSQLiteConfig({ database: dbPath }), {}, { readOnly: true }); + await profile.connect(); + try { + // The agent reads the same result shape the user does; a snapshot that described + // its columns on one path and not the other would be two answers to one question. + const result = await profile.queryReadOnly("SELECT id, price, v FROM t", AGENT_BUDGET); + expect(result.rows).toEqual([{ id: 1, price: 1.5, v: "seeded" }]); + expect(result.columnTypes).toEqual({ id: "INTEGER", price: "REAL", v: "TEXT" }); + + // and the same absence, on the same path + const computed = await profile.queryReadOnly("SELECT COUNT(*) AS n FROM t", AGENT_BUDGET); + expect(Object.hasOwn(computed, "columnTypes")).toBe(false); + } finally { + await profile.disconnect(); + } + }); +}); + +/** + * The consequence the missing declarations had on the file a user keeps (#273). + * + * `result-export.ts` writes the declared type when the result carries one and falls + * back to the JavaScript type of a value when it does not. Nothing here edits that + * file - it was already right; it was being handed nothing to read. + */ +describe("SQL export of a SQLite result (#273)", () => { + let exportDir: string; + + beforeAll(() => { + exportDir = mkdtempSync(join(tmpdir(), "libredb-sqlite-export-")); + }); + + afterAll(() => { + rmSync(exportDir, { recursive: true, force: true }); + }); + + test("a 64-bit key is exported as the INTEGER column it is, and replays as one", async () => { + const db = new SQLiteProvider(makeSQLiteConfig()); + await db.connect(); + let file: string; + try { + await db.query("CREATE TABLE zz (id INTEGER PRIMARY KEY, price REAL, label TEXT)"); + await db.query("INSERT INTO zz VALUES (9007199254740993, 1.5, 'target')"); + const result = await db.query("SELECT id, price, label FROM zz"); + + // The value really is a string here - that is the 64-bit seam doing its job - so + // the declaration is the only thing standing between it and a TEXT column. + expect(typeof result.rows[0].id).toBe("string"); + + const source = { + rows: result.rows, + fields: result.fields, + tabName: "zz", + dialect: "sqlite" as const, + ...(result.columnTypes === undefined ? {} : { columnTypes: result.columnTypes }), + }; + const ddl = buildResultExport("sql-ddl", source).content; + // MEASURED before this: `"id" TEXT` and `"price" DOUBLE PRECISION`, because + // `inferKind` saw a string and a fractional number. Both are now the schema's. + expect(ddl).toContain('"id" INTEGER'); + expect(ddl).toContain('"price" REAL'); + expect(ddl).not.toContain('"id" TEXT'); + + file = `${ddl}\n${buildResultExport("sql-insert", source).content}`; + } finally { + await db.disconnect(); + } + + // The file is meant to be RUN somewhere else, so it is run: replayed into a fresh + // database, the key has to come back as an integer holding every digit. Before + // this it came back with the `text` storage class, and the table a user moved to + // another engine was no longer keyed by a number. + const replayPath = join(exportDir, "replayed.db"); + const replay = new SQLiteProvider(makeSQLiteConfig({ database: replayPath })); + await replay.connect(); + try { + for (const statement of file.split(";\n").filter((part) => part.trim().length > 0)) { + await replay.query(statement); + } + expect(await replay.query("SELECT typeof(id) AS kind FROM zz")).toMatchObject({ rows: [{ kind: "integer" }] }); + expect(await replay.query("SELECT id FROM zz")).toMatchObject({ rows: [{ id: "9007199254740993" }] }); + // and the column the replayed schema declares is the one the source declared + expect((await replay.query("SELECT id, price FROM zz")).columnTypes).toEqual({ id: "INTEGER", price: "REAL" }); + } finally { + await replay.disconnect(); + } + }); + + test("a computed column still falls back to the value's shape, because nothing declared it", async () => { + const db = new SQLiteProvider(makeSQLiteConfig()); + await db.connect(); + try { + await db.query("CREATE TABLE zc (id INTEGER PRIMARY KEY)"); + await db.query("INSERT INTO zc VALUES (1)"); + const result = await db.query("SELECT COUNT(*) AS n FROM zc"); + + expect(Object.hasOwn(result, "columnTypes")).toBe(false); + // The fallback is the RIGHT answer here rather than a leftover: SQLite declares + // nothing for `COUNT(*)`, so the value is the only witness there has ever been. + const ddl = buildResultExport("sql-ddl", { + rows: result.rows, + fields: result.fields, + tabName: "zc", + dialect: "sqlite", + }).content; + expect(ddl).toContain('"n" BIGINT'); + } finally { + await db.disconnect(); + } + }); +}); + +/** + * The consequence the missing declarations had on the row editor (#273). + * + * `src/hooks/use-inline-editing.ts` decides whether a key can be sent back as the row + * that holds it by reading `QueryResult.columnTypes`, and with nothing declared it has + * to fail closed - so on this provider it refused keys the engine answers about + * perfectly. What the provider owes that decision is the declaration itself, spelled + * the way SQLite spells it, which is what these pin. The rule that reads them lives in + * the hook and is tested there; neither file is touched here. + * + * MEASURED end to end on 2026-09-18, rendering that hook over rows read through this + * provider, before and after the declarations existed: + * + * key column, holding 1.5 | before | after + * DOUBLE | refused | 1 UPDATE accepted + * REAL | refused | refused (see below) + * TEXT holding an ISO-shaped string | refused | 1 UPDATE accepted + * + * The REAL row is the one case that does NOT close, and the reason is entirely inside + * the hook: its `FLOAT64_TYPE_NAMES` deliberately omits `real`, because PostgreSQL's + * and Trino's `real` is 32 bits wide even though SQLite's is 64. Closing it means + * teaching that set which engine it is reading, in a file this change does not own. + */ +describe("what the row editor reads off a SQLite result (#273)", () => { + let editing: SQLiteProvider; + + beforeAll(async () => { + editing = new SQLiteProvider(makeSQLiteConfig()); + await editing.connect(); + await editing.query("CREATE TABLE dbl (id DOUBLE, note TEXT)"); + await editing.query("INSERT INTO dbl VALUES (1.5, 'first')"); + await editing.query("CREATE TABLE rl (id REAL, note TEXT)"); + await editing.query("INSERT INTO rl VALUES (1.5, 'first'), (2.5, 'second')"); + await editing.query("CREATE TABLE txt (id TEXT, note TEXT)"); + await editing.query("INSERT INTO txt VALUES ('2026-01-01T10:00:00.123Z', 'first')"); + }); + + afterAll(async () => { + await editing.disconnect(); + }); + + test("a 64-bit float key arrives declared, so the editor's float64 rule can see it", async () => { + const result = await editing.query("SELECT id, note FROM dbl"); + + // `double` is the first word of the type, which is what the hook matches on. With no + // declaration the same 1.5 was refused as "a fractional number", and the engine was + // never asked about a key it answers for exactly. + expect(result.rows).toEqual([{ id: 1.5, note: "first" }]); + expect(result.columnTypes).toEqual({ id: "DOUBLE", note: "TEXT" }); + }); + + test("a text key that merely LOOKS like an instant arrives declared TEXT", async () => { + const result = await editing.query("SELECT id, note FROM txt"); + + // The hook reads a serialized-date SHAPE as a date only where nothing was declared, + // because a `Date` through JSON is exactly that string. SQLite has no date type, so + // this key is text and the declaration is what says so. + expect(result.rows).toEqual([{ id: "2026-01-01T10:00:00.123Z", note: "first" }]); + expect(result.columnTypes).toEqual({ id: "TEXT", note: "TEXT" }); + }); + + test("a REAL key is declared REAL, and the engine really does answer for it", async () => { + const result = await editing.query("SELECT id, note FROM rl"); + expect(result.columnTypes).toEqual({ id: "REAL", note: "TEXT" }); + + // The other half of the measurement, so the remaining refusal is recorded against a + // fact rather than an assumption: SQLite matches this key exactly, once. + const matched = await editing.query("SELECT note FROM rl WHERE id = ?", [1.5]); + expect(matched.rows).toEqual([{ note: "first" }]); + }); +}); diff --git a/tests/unit/db/sqlite-driver.test.ts b/tests/unit/db/sqlite-driver.test.ts index 5f60d4cc3..4da4e2c6b 100644 --- a/tests/unit/db/sqlite-driver.test.ts +++ b/tests/unit/db/sqlite-driver.test.ts @@ -1,4 +1,7 @@ import { describe, test, expect, beforeEach, afterEach } from "bun:test"; +// The raw driver, for the one test that measures bun:sqlite's own two fields rather +// than the bridge over them. +import { Database as BunDatabase } from "bun:sqlite"; import { DatabaseConfigError } from "@/lib/db/errors"; import { createBunSQLiteDriver, @@ -8,13 +11,13 @@ import { normalizeSQLiteBigInt, resolveSQLiteDriverName, toSQLiteBindValue, + type BunDatabaseLike, type BunSQLiteConstructor, type BunSQLiteOpenOptions, type NodeDatabaseSyncLike, type NodeSQLiteModule, type SQLiteConstructor, - type SQLiteDatabase, - type SQLiteStatement, + type SQLiteDeclaredColumn, } from "@/lib/db/providers/sql/sqlite-driver"; /** In-memory stand-in for node:sqlite's DatabaseSync (Bun cannot import the real one). */ @@ -36,15 +39,21 @@ class StubDatabaseSync implements NodeDatabaseSyncLike { this.calls.push(`exec:${sql}`); } - prepare(sql: string) { + prepare(sql: string): ReturnType { this.calls.push(`prepare:${sql}`); if (sql === "SELECT bigints") { - return bigIntStatement(); + return bigIntStatement() as unknown as ReturnType; } return { all: (...params: unknown[]) => [{ sql, params }], get: (...params: unknown[]) => (params[0] === "miss" ? undefined : { sql, first: params[0] }), run: (...params: unknown[]) => ({ changes: params[0] === "bigint" ? BigInt(3) : 1 }), + // node:sqlite answers both halves in one call, and `null` is its word for a column + // SQLite declared nothing for. + columns: () => [ + { name: "sql", type: "TEXT" }, + { name: "params", type: null }, + ], }; } @@ -67,11 +76,23 @@ function bigIntStatement() { all: () => [row()], get: () => row(), run: () => ({ changes: BigInt("2"), lastInsertRowid: BigInt("9007199254740993") }), + // The declared columns in BOTH drivers' spellings, so the one stand-in can stand in + // for either: bun publishes two parallel arrays, node one `columns()` call. `null` is + // each driver's word for a column SQLite declared nothing for - here `nothing`, which + // is why the same stand-in proves the undeclared case on both adapters. + columnNames: ["small", "huge", "text", "nothing"], + declaredTypes: ["INTEGER", "INTEGER", "TEXT", null], + columns: () => [ + { name: "small", type: "INTEGER" }, + { name: "huge", type: "INTEGER" }, + { name: "text", type: "TEXT" }, + { name: "nothing", type: null }, + ], }; } /** In-memory stand-in for bun:sqlite's Database. */ -class StubBunDatabase implements SQLiteDatabase { +class StubBunDatabase implements BunDatabaseLike { static lastInstance: StubBunDatabase | undefined; readonly path: string; readonly options: BunSQLiteOpenOptions | undefined; @@ -88,9 +109,9 @@ class StubBunDatabase implements SQLiteDatabase { this.calls.push(`exec:${sql}`); } - prepare(sql: string): SQLiteStatement { + prepare(sql: string): ReturnType { this.calls.push(`prepare:${sql}`); - return bigIntStatement() as unknown as SQLiteStatement; + return bigIntStatement() as unknown as ReturnType; } close(throwOnError?: boolean): void { @@ -603,19 +624,21 @@ describe("toSQLiteBindValue()", () => { // exposes so its semantics can be driven without the real driver. /** A statement stand-in whose every read answers with one fixed record. */ -function statementReturning(record: unknown): SQLiteStatement { +function statementReturning(record: unknown): ReturnType { return { all: () => [record], get: () => record, run: () => ({ changes: 1 }), + columnNames: [], + declaredTypes: [], }; } /** The bun adapter, wired to a driver whose every read answers with `record`. */ function driverReturning(record: unknown): SQLiteConstructor { - class RecordDatabase implements SQLiteDatabase { + class RecordDatabase implements BunDatabaseLike { exec(): void {} - prepare(): SQLiteStatement { + prepare(): ReturnType { return statementReturning(record); } close(): void {} @@ -668,3 +691,293 @@ describe("the record seam's guards", () => { expect(cellStmt.all()).toEqual([cells]); }); }); + +// ============================================================================ +// The declared-type bridge (#273) +// ============================================================================ +// A result carries the type each of its columns was DECLARED with. Both drivers +// publish it and neither publishes it the same way, so it is bridged at the same seam +// `inTransaction` and the big-integer flag already are: +// +// bun:sqlite `stmt.columnNames` + `stmt.declaredTypes`, two parallel arrays +// node:sqlite `stmt.columns()`, one array of `{ name, type }` +// +// Both spell "nothing was declared" as `null`; the bridge answers `undefined`, which is +// what `declaredColumnTypes()` in `column-types.ts` drops a column for. + +/** The pairs one adapter answers, read back as a plain array so the two can be compared. */ +function declaredPairs(stmt: { declaredColumns(): readonly SQLiteDeclaredColumn[] }): [string, string | undefined][] { + return stmt.declaredColumns().map(([name, type]) => [name, type]); +} + +describe("declaredColumns() bridges the two spellings (#273)", () => { + test("the bun adapter reads columnNames beside declaredTypes", () => { + const stmt = new (createBunSQLiteDriver(StubBunDatabase))(":memory:").prepare("SELECT bigints"); + + expect(declaredPairs(stmt)).toEqual([ + ["small", "INTEGER"], + ["huge", "INTEGER"], + ["text", "TEXT"], + ["nothing", undefined], + ]); + }); + + test("the node adapter reads columns()", () => { + const stmt = new (createNodeSQLiteDriver(StubDatabaseSync))(":memory:").prepare("SELECT bigints"); + + expect(declaredPairs(stmt)).toEqual([ + ["small", "INTEGER"], + ["huge", "INTEGER"], + ["text", "TEXT"], + ["nothing", undefined], + ]); + }); + + // The whole point of a bridge: one stand-in, two adapters, ONE answer. A mapping that + // drifted on either side would show up here rather than on whichever runtime shipped. + test("both adapters answer the same pairs for the same statement", () => { + const fromBun = declaredPairs(new (createBunSQLiteDriver(StubBunDatabase))(":memory:").prepare("SELECT bigints")); + const fromNode = declaredPairs( + new (createNodeSQLiteDriver(StubDatabaseSync))(":memory:").prepare("SELECT bigints"), + ); + + expect(fromBun).toEqual(fromNode); + }); + + // `null` and `undefined` are not the same answer downstream: `declaredColumnTypes()` + // keeps a column whose type is `null` and drops one whose type is `undefined`, so a + // bridge that forwarded the driver's null would publish `{ nothing: null }` and every + // consumer reading the map with `Object.hasOwn` would believe a type was declared. + test.each([ + ["bun", () => createBunSQLiteDriver(StubBunDatabase)], + ["node", () => createNodeSQLiteDriver(StubDatabaseSync)], + ] as const)("the %s adapter answers undefined, not null, for an undeclared column", (_name, makeDriver) => { + const pairs = declaredPairs(new (makeDriver())(":memory:").prepare("SELECT bigints")); + + expect(pairs[3][1]).toBeUndefined(); + expect(Object.is(pairs[3][1], null)).toBe(false); + }); +}); + +// ============================================================================ +// What the two drivers actually do (measured, not assumed) +// ============================================================================ + +describe("the real drivers' declared types (#273)", () => { + let savedDriver: string | undefined; + + // The same save/restore the suite above keeps: these tests force the driver NAME, and + // a leaked override would decide which driver a later file's provider opened. + beforeEach(() => { + savedDriver = process.env.LIBREDB_SQLITE_DRIVER; + }); + + afterEach(() => { + if (savedDriver === undefined) delete process.env.LIBREDB_SQLITE_DRIVER; + else process.env.LIBREDB_SQLITE_DRIVER = savedDriver; + }); + + /** The declarations of one statement, read through whichever REAL driver is handed in. */ + function declarationsOf(Driver: SQLiteConstructor, sql: string): [string, string | undefined][] { + const db = new Driver(":memory:", { create: true, readwrite: true }); + try { + db.exec("CREATE TABLE d (id INTEGER PRIMARY KEY, price REAL, label TEXT, flag BOOLEAN, bare)"); + db.exec("INSERT INTO d VALUES (1, 1.5, 'first', 1, 'anything')"); + db.exec("CREATE VIEW dv AS SELECT id, price FROM d"); + const stmt = db.prepare(sql); + stmt.all(); + return declaredPairs(stmt); + } finally { + db.close(true); + } + } + + /** + * The same nine shapes the provider's integration tests assert, one level lower: this + * is the driver's answer, before anything turns it into a map. + */ + const SHAPES: [string, string, [string, string | undefined][]][] = [ + [ + "a plain column", + "SELECT id, price, label, flag, bare FROM d", + [ + ["id", "INTEGER"], + ["price", "REAL"], + ["label", "TEXT"], + ["flag", "BOOLEAN"], + ["bare", undefined], + ], + ], + [ + "an alias", + "SELECT id AS ident, price AS ratio FROM d", + [ + ["ident", "INTEGER"], + ["ratio", "REAL"], + ], + ], + [ + "a view column", + "SELECT id, price FROM dv", + [ + ["id", "INTEGER"], + ["price", "REAL"], + ], + ], + [ + "an expression", + "SELECT id + 1 AS e, price * 2 AS e2 FROM d", + [ + ["e", undefined], + ["e2", undefined], + ], + ], + [ + "a literal", + "SELECT 1 AS one, 'x' AS ex", + [ + ["one", undefined], + ["ex", undefined], + ], + ], + [ + "an aggregate", + "SELECT COUNT(*) AS c, SUM(id) AS s FROM d", + [ + ["c", undefined], + ["s", undefined], + ], + ], + ["a function call", "SELECT upper(label) AS u FROM d", [["u", undefined]]], + [ + "a PRAGMA column", + "PRAGMA journal_mode", + // One column, declared nothing - and the statement bun:sqlite's `columnTypes` + // refuses outright, which is why the bridge does not read that field. + [["journal_mode", undefined]], + ], + [ + "a statement that matched no rows", + "SELECT id, price FROM d WHERE id = -1", + [ + ["id", "INTEGER"], + ["price", "REAL"], + ], + ], + ]; + + test.each(SHAPES)("bun:sqlite declares %s", async (_shape, sql, expected) => { + process.env.LIBREDB_SQLITE_DRIVER = "bun"; + expect(declarationsOf(await loadSQLiteDriver(), sql)).toEqual(expected); + }); + + test.each(SHAPES)("node:sqlite declares %s", async (_shape, sql, expected) => { + expect(declarationsOf(await loadNodeSQLiteDriver(), sql)).toEqual(expected); + }); + + test("a write declares nothing on either driver", async () => { + process.env.LIBREDB_SQLITE_DRIVER = "bun"; + for (const Driver of [await loadSQLiteDriver(), await loadNodeSQLiteDriver()]) { + const db = new Driver(":memory:", { create: true, readwrite: true }); + try { + db.exec("CREATE TABLE w (id INTEGER)"); + const stmt = db.prepare("INSERT INTO w VALUES (?)"); + stmt.run(1); + expect(declaredPairs(stmt)).toEqual([]); + } finally { + db.close(true); + } + } + }); + + /** + * The ORDER the bridge is documented to need, asserted rather than trusted. + * + * bun:sqlite refuses `declaredTypes` until the statement has run, so a bridge that + * read it at `prepare()` would throw on every query. The wrapper reads it lazily, so + * preparing is safe and asking before the rows raises the driver's own message. + */ + test("bun:sqlite answers only after the rows, and the bridge is lazy enough for that", async () => { + process.env.LIBREDB_SQLITE_DRIVER = "bun"; + const db = new (await loadSQLiteDriver())(":memory:", { create: true, readwrite: true }); + try { + db.exec("CREATE TABLE d (id INTEGER PRIMARY KEY, price REAL)"); + // Preparing must not read it: this is the line that would throw on every SELECT. + const stmt = db.prepare("SELECT id, price FROM d"); + expect(() => stmt.declaredColumns()).toThrow(/executed/); + + stmt.all(); + expect(declaredPairs(stmt)).toEqual([ + ["id", "INTEGER"], + ["price", "REAL"], + ]); + } finally { + db.close(true); + } + }); + + /** + * node:sqlite has no such restriction, so the rule costs it nothing. Asserted because + * it is the other half of "after the rows is the one order BOTH drivers accept". + */ + test("node:sqlite answers before the rows as readily as after them", async () => { + const db = new (await loadNodeSQLiteDriver())(":memory:", { create: true, readwrite: true }); + try { + db.exec("CREATE TABLE d (id INTEGER PRIMARY KEY, price REAL)"); + const stmt = db.prepare("SELECT id, price FROM d"); + const before = declaredPairs(stmt); + stmt.all(); + + expect(before).toEqual([ + ["id", "INTEGER"], + ["price", "REAL"], + ]); + expect(declaredPairs(stmt)).toEqual(before); + } finally { + db.close(true); + } + }); + + /** + * The trap this bridge exists NOT to fall into. + * + * bun:sqlite publishes a second field called `columnTypes`, and it answers a different + * question: the storage class of the row just read. Measured here rather than asserted + * from memory, because the two names are one word apart and the wrong one is wrong + * quietly - a `REAL` column would be renamed `FLOAT`, a `BOOLEAN` column `INTEGER`, and + * every PRAGMA the provider runs would throw. + */ + test("bun:sqlite's columnTypes is the runtime storage class and refuses a non-read-only statement", () => { + const raw = new BunDatabase(":memory:"); + try { + raw.exec("CREATE TABLE d (id INTEGER PRIMARY KEY, price REAL, flag BOOLEAN, bare)"); + raw.exec("INSERT INTO d VALUES (1, 1.5, 1, 'anything')"); + const read = raw.prepare("SELECT id, price, flag, bare FROM d") as unknown as { + all(): unknown[]; + columnTypes: string[]; + declaredTypes: (string | null)[]; + }; + read.all(); + + // What the SCHEMA says, which is what the bridge publishes... + expect(read.declaredTypes).toEqual(["INTEGER", "REAL", "BOOLEAN", null]); + // ...and what the ROW happened to hold, which is not the same answer for three of + // the four columns. + expect(read.columnTypes).toEqual(["INTEGER", "FLOAT", "INTEGER", "TEXT"]); + + // And it is not merely different - it is unavailable exactly where the provider + // reads plenty of results: `PRAGMA journal_mode` is not a read-only statement to + // bun, while `declaredTypes` answers for it. + const pragma = raw.prepare("PRAGMA journal_mode") as unknown as { + all(): unknown[]; + columnTypes: string[]; + declaredTypes: (string | null)[]; + }; + pragma.all(); + expect(pragma.declaredTypes).toEqual([null]); + expect(() => pragma.columnTypes).toThrow(/non-read-only/); + } finally { + raw.close(true); + } + }); +}); From b1df6bc1f33a94158167f134c43ed338bdb9e885 Mon Sep 17 00:00:00 2001 From: Yusuf GUNDOGDU Date: Fri, 18 Sep 2026 23:03:37 +0300 Subject: [PATCH 15/19] fix(editor): let a SQLite float key through, because REAL is 64 bits there The editor asks whether a fractional key's column is a 64-bit float, because a value read at a different width than the row holds can miss the row or write to the wrong one. `real` and `float` were left out of the safe list, and that was right for the engines it was written against. SQLite has no 32-bit float. REAL is the only float width it has, and FLOAT, DOUBLE and DOUBLE PRECISION are accepted spellings of the same storage class. Measured live: all four report the same decltype, `typeof()` answers `real` for each, 0.30000000000000004 reads back exactly, and `WHERE k_id = 1.5` matched one row while the editor was refusing that same edit. The sentence it gave - that nothing said the column was a 64-bit float - was false, because the declaration said so. This only became visible now that the SQLite provider reports its declarations. So the decision reads the engine as well as the name, from the connection the caller already holds. Nothing new is threaded through the module. Each engine was measured rather than assumed. PostgreSQL `real` and `float4` are four bytes by `pg_column_size` and `0.1::real::float8` is 0.10000000149011612, so they stay refused. MySQL FLOAT compared false against 0.1 in the same row where DOUBLE compared true, so FLOAT stays refused. DuckDB hands 32-bit values over as 0.10000000149011612, so REAL and FLOAT stay refused. libsql is SQLite and behaves as SQLite, checked against a running server. Trino REAL and SQL Server `float` could not be reached from here, so both keep the refusal: an unmeasured engine gets the closed side, which is what this rule chose in the first place. --- src/hooks/use-inline-editing.ts | 50 ++++++++++++++- tests/hooks/use-inline-editing.test.ts | 89 ++++++++++++++++++++++++++ 2 files changed, 136 insertions(+), 3 deletions(-) diff --git a/src/hooks/use-inline-editing.ts b/src/hooks/use-inline-editing.ts index a36486650..80446297f 100644 --- a/src/hooks/use-inline-editing.ts +++ b/src/hooks/use-inline-editing.ts @@ -119,9 +119,46 @@ const INSTANT_TYPE_NAMES: ReadonlySet = new Set([ * - `real`. PostgreSQL's and Trino's are 32 bits; SQLite's is 64. (PostgreSQL happens to * match a `real` key anyway — it infers the parameter's type from the column and re-reads * the decimal at 32-bit precision — but that is `pg`'s doing, not the value's.) + * + * Which is why the two words are absent from THIS set and not from the rule: on an engine + * that has no 32-bit float at all they mean 64 bits and nothing else, and that is what + * `FLOAT64_ONLY_NAMES` below adds back, for those engines only. */ const FLOAT64_TYPE_NAMES: ReadonlySet = new Set(["double", "float8", "float64", "binary_double"]); +/** + * The engines with only ONE float width, and the words that therefore state it there. + * + * SQLite has no 32-bit float: `REAL`, `FLOAT`, `DOUBLE` and `DOUBLE PRECISION` are four + * spellings of one storage class, 8 bytes of IEEE double, which is exactly as wide as the + * JavaScript number in front of us. So on these dialects the declaration DOES say the + * width, and the refusal's sentence — "nothing here says the column holds it as a 64-bit + * float" — is false about them. + * + * MEASURED 2026-09-18 through the real provider path, on bun:sqlite (Bun 1.4.0) and on + * libSQL server v0.24.33 over its HTTP pipeline: `zz_real(r REAL, f FLOAT, d DOUBLE, dp + * DOUBLE PRECISION)` reported those four decltypes and answered `real` to `typeof()` for + * every one of them; 0.30000000000000004 was written and read back identical, which no + * 32-bit column can do; and `WHERE r = 1.5`, `WHERE f = 0.1`, `WHERE d = + * 0.30000000000000004` and `WHERE dp = 0.1` each matched exactly one row on both. + * + * `libsql` is here for the reason `sqlite` is: it embeds the same engine and reports the + * same `sqlite3_column_decltype` declarations. + * + * NO OTHER DIALECT IS, and each was measured rather than assumed. PostgreSQL 16.15: + * `real` and `float4` are both spelled `real` by `pg`, both `pg_column_size` 4, and + * `0.1::real::float8` is 0.10000000149011612 — a different number to the double 0.1. + * MySQL 8.4.11: one row holding 0.1 in a `FLOAT` and a `DOUBLE` answered `f = 0.1` FALSE + * and `d = 0.1` TRUE. DuckDB: `REAL` and `FLOAT` are one 32-bit type, handed over as + * 0.10000000149011612, and `r::DOUBLE = 0.1` is false. Trino was not reachable to measure, + * so it keeps the refusal — the closed side is the safe side, which is what this rule + * already chose. + */ +const FLOAT64_ONLY_DIALECTS: ReadonlySet = new Set(["sqlite", "libsql"]); + +/** The words that mean 64 bits ONLY on the dialects above, and 32 elsewhere. */ +const FLOAT64_ONLY_NAMES: ReadonlySet = new Set(["real", "float"]); + /** * Reads one declared type down to its first word. * @@ -130,8 +167,12 @@ const FLOAT64_TYPE_NAMES: ReadonlySet = new Set(["double", "float8", "fl * come off first, and only then the parameters: `DateTime64(6, 'UTC')` is `datetime64`, * Trino's `timestamp(3) with time zone` is `timestamp`, and `character varying` is * `character`, which is in no set here. + * + * The DIALECT is read alongside the word, because one word is two widths across engines: + * `REAL` is 64 bits on SQLite and 32 on PostgreSQL, and the same declaration therefore + * settles the question on one and settles nothing on the other. */ -function keyColumnKind(declaredType: string | undefined): KeyColumnKind { +function keyColumnKind(declaredType: string | undefined, dialect: DatabaseConnection["type"]): KeyColumnKind { if (declaredType === undefined) return "undeclared"; let name = declaredType.trim().toLowerCase(); for (;;) { @@ -143,7 +184,8 @@ function keyColumnKind(declaredType: string | undefined): KeyColumnKind { } const first = name.split("(")[0].trim().split(/\s+/)[0]; if (INSTANT_TYPE_NAMES.has(first)) return "date-time"; - return FLOAT64_TYPE_NAMES.has(first) ? "float64" : "declared"; + if (FLOAT64_TYPE_NAMES.has(first)) return "float64"; + return FLOAT64_ONLY_DIALECTS.has(dialect) && FLOAT64_ONLY_NAMES.has(first) ? "float64" : "declared"; } /** @@ -395,7 +437,9 @@ async function keyAddressesOneRow( /** What the result said this key column is — `undefined` where it said nothing. */ declaredKeyType: string | undefined, ): Promise<{ ok: true } | { ok: false; reason: string }> { - const column = keyColumnKind(declaredKeyType); + // The connection is already in hand — the same object line 433 reads its dialect from — + // so the width a declared float means is read from the engine that declared it. + const column = keyColumnKind(declaredKeyType, connection.type); // A key with no value cannot be addressed by `=` at all, and `String(null)` would send // the text "null" — which an integer column rejects, so the whole apply would fail on a // driver error rather than on the reason. diff --git a/tests/hooks/use-inline-editing.test.ts b/tests/hooks/use-inline-editing.test.ts index f4d06ab89..313ef1b15 100644 --- a/tests/hooks/use-inline-editing.test.ts +++ b/tests/hooks/use-inline-editing.test.ts @@ -2483,6 +2483,95 @@ describe("useInlineEditing", () => { expect(mockToastError).not.toHaveBeenCalled(); }); + // ── SQLite's REAL is the engine's only float, and it is 64 bits ─────────── + // + // MEASURED 2026-09-18 through the real `createDatabaseProvider` path, on bun:sqlite + // (Bun 1.4.0) and on libSQL server v0.24.33 over its HTTP pipeline: + // + // sqlite zz_real(r REAL, f FLOAT, d DOUBLE, dp DOUBLE PRECISION) + // decltypes REAL / FLOAT / DOUBLE / DOUBLE PRECISION, and + // typeof() answered `real` for every one of them + // libsql the same table, the same four decltypes, the same four `real`s + // + // One storage class, and it is 8 bytes: SQLite has NO 32-bit float to hold these in. + // 0.30000000000000004 was written and read back `=== 0.30000000000000004`, which no + // 32-bit column can do, and `WHERE r = 1.5`, `WHERE f = 0.1`, `WHERE d = + // 0.30000000000000004` and `WHERE dp = 0.1` each answered exactly ONE row on both. + // + // So on these two dialects the declaration DOES state the width, and the refusal's + // sentence - "nothing here says the column holds it as a 64-bit float" - was false + // about them. Measured before this changed, on a live `zz_live(k_id REAL PRIMARY KEY)` + // holding 1.5 and 2.5: the engine answered one row for `WHERE "k_id" IN (1.5)` and the + // editor refused the edit anyway, leaving the table untouched. + test.each([ + ["sqlite", "REAL"], + ["sqlite", "FLOAT"], + ["sqlite", "DOUBLE"], + ["sqlite", "DOUBLE PRECISION"], + ["libsql", "REAL"], + ["libsql", "FLOAT"], + ] as const)("a %s %s key is carried back, because that engine has no other float width", async (type, declared) => { + const seen = countAsks(); + await applyKeyedBy(1.5, type, declared); + + expect(seen).toHaveLength(1); + expect(updateCalls()).toHaveLength(1); + expect(mockToastError).not.toHaveBeenCalled(); + }); + + test.each([ + ["postgres", "real"], + ["postgres", "float4"], + ["mysql", "float"], + ["duckdb", "REAL"], + ["duckdb", "FLOAT"], + ["trino", "real"], + ] as const)("a %s %s key is still refused, because that word is 32 bits there", async (type, declared) => { + // The other side, and why the dialect has to be read rather than the word. MEASURED + // 2026-09-18 on the live engines: + // + // PostgreSQL 16.15 `real` and `float4` are both spelled `real` by `pg`, both + // `pg_column_size` 4, and `0.1::real::float8` is + // 0.10000000149011612 - NOT the double 0.1. + // MySQL 8.4.11 `zz_w(f FLOAT, d DOUBLE)` holding 0.1: `f = 0.1` answered + // FALSE and `d = 0.1` answered TRUE, in the same row. + // DuckDB `zz_flt(r REAL, f FLOAT, d DOUBLE)` holding 0.1: the driver + // hands the two 32-bit columns over as 0.10000000149011612 and + // the DOUBLE as 0.1, and `r::DOUBLE = 0.1` answered false. + // Trino not measured here - no engine was reachable - so it keeps the + // refusal, which is the closed side of the same rule. + // + // 1.5 is exact at both widths, which is the point: what is refused is the WIDTH the + // decimal will be read back at, and on these dialects nothing in front of us states it. + const seen = countAsks(); + await applyKeyedBy(1.5, type, declared); + + expect(seen).toHaveLength(0); + expect(updateCalls()).toHaveLength(0); + expect(mockToastError).toHaveBeenCalledWith("Cannot Apply Changes", { + description: expect.stringContaining("nothing here says the column holds it as a 64-bit float"), + }); + }); + + test.each([ + ["sqlite", "TEXT"], + ["sqlite", undefined], + ["libsql", "NUMERIC"], + ] as const)("a %s key declared %s is refused like any other, dialect or no dialect", async (type, declared) => { + // The dialect widens WHICH WORDS state 64 bits; it does not wave a column through that + // declared something else, or nothing at all. A SQLite column is dynamically typed, so + // a `TEXT` or `NUMERIC` one really can be holding 1.5, and neither word says at what + // width the engine will read the decimal back. + const seen = countAsks(); + await applyKeyedBy(1.5, type, declared); + + expect(seen).toHaveLength(0); + expect(updateCalls()).toHaveLength(0); + expect(mockToastError).toHaveBeenCalledWith("Cannot Apply Changes", { + description: expect.stringContaining("it is a fractional number"), + }); + }); + test.each([ ["postgres", "double precision", 1e21], ["postgres", "double precision", 1e300], From 86d975e0d56731fe036d3a1709b9ae797a92ec33 Mon Sep 17 00:00:00 2001 From: Yusuf GUNDOGDU Date: Fri, 18 Sep 2026 23:38:21 +0300 Subject: [PATCH 16/19] fix(test): decide a table cell by its column header, and read a login body as it is written An adversarial reviewer broke this guard three ways and proved each by reproduction. It read only the cell immediately after the name, and only a backticked span. The real tables here are shaped Variable, Required, Description, so the value lives in the third cell, written inside the description as "(default: LibreDB.2026)". Seven shapes of that got past, including the plain and bolded spellings. Reading more cells is what made the second finding possible: docs/HELM_CHART.md has a Source column whose cells are chart value paths, so a row there reported secrets.adminPassword as a published password, and secrets.jwtSecret.fromExistingSecret is thirty-seven characters, which would have failed the secret-length test outright. A guard that fires on innocent text is the one that gets deleted by the next person it stops. So position is not what decides a cell any more: the table's own header does. A column headed Value, Default or Example holds values; a "default: x" written in words is an assignment wherever it appears; a two-column table's single marked cell is a value; with no header at all the cell after the name is read, as before. A value that spells the variable's own name back - secrets.adminPassword, admin-password - is a reference, not a value. The third was the JSON rule requiring double quotes around the email key. The file it was written for does not use them: docs/API_DOCS.md wrote `{ email: 'admin@libredb.org', password: 'admin123' }`, a bare key and single quotes, and that one escaped while its sibling was caught. The test that claimed to cover "the two shapes in the file" had hand-converted this one to double quotes - a line that never existed there. It now uses the real shape. Measured rather than argued: a credential line injected into README.md, DOCKERHUB.md, docs/API_DOCS.md, deploy/railway, deploy/koyeb and docs/HELM_CHART is caught in all six, and six innocent rows added to the real HELM_CHART table stay silent in all six. Eight tests became ten and none of the eight was weakened. --- tests/unit/published-credentials.test.ts | 291 +++++++++++++++++++++-- 1 file changed, 268 insertions(+), 23 deletions(-) diff --git a/tests/unit/published-credentials.test.ts b/tests/unit/published-credentials.test.ts index 077a9ef0b..8a7196c6e 100644 --- a/tests/unit/published-credentials.test.ts +++ b/tests/unit/published-credentials.test.ts @@ -181,27 +181,145 @@ function pairedAssignments(lines: string[], name: string): string[] { /** * A Markdown table row, which is how a README lists its variables: the name in one cell and - * the value in the next. It assigns nothing in the `NAME=value` sense and a reader still - * reads a working login out of it, which is how `| `ADMIN_PASSWORD` | `example-not-a-real-password` |` - * passed this guard - measured, on the most common table shape in these files. + * the value somewhere else in the row. It assigns nothing in the `NAME=value` sense and a + * reader still reads a working login out of it, which is how + * `| `ADMIN_PASSWORD` | `example-not-a-real-password` |` passed this guard - measured, on the most common + * table shape in these files. * - * Only a value cell written as a code span with no space inside it counts. That is what - * tells the value column from the description column beside it: `| `ADMIN_PASSWORD` | Admin - * password |` is a table OF variables, not a table of credentials, and flagging those would - * light up every README here and get this guard deleted by the next person it stopped. + * WHICH cell holds the value is not answerable by position. The tables actually written here + * are `| Variable | Required | Description |` (README.md:640, DOCKERHUB.md:184), where a + * default is written inside the description as ``(default: `x`)`` - the `ADMIN_EMAIL` row + * does exactly that - and `| Variable | Source | Conditional |` (docs/HELM_CHART.md:166), + * where the second cell is a chart value PATH. Reading the cell after the name misses the + * first and reports `secrets.adminPassword` as a published password in the second, which is + * the failure that gets a guard deleted by the next person it stops. + * + * So a row is read by what the table says about itself, not by where a cell sits: + * + * 1. A column whose HEADER names values - `Value`, `Default`, `Example`, `Sample` - holds + * values, and is read as one. `Required`, `Source`, `Conditional`, `Notes`, `Description` + * and every header not recognised describe something ABOUT the variable, never its value, + * so nothing is read from them positionally. + * 2. A cell in ANY column that writes a default in words - ``default: `x` ``, `defaults to x`, + * ``default `x` `` - is read, because that phrasing is itself the assignment. A bare word + * counts only when the phrase separates it (`default: x`, not `by default the account ...`) + * AND the value ends the cell: prose carries on in words, which is the same thing + * VALUE_ENDS says one line up. + * 3. With no header row at all - a row quoted on its own - only the cell after the name is + * read. That is the single positional convention left when nothing has been declared. + * 4. A two-column table offers no column to choose between: whatever its header calls that + * one cell, it is everything the table says about the variable, so a value written there + * AS a value - `x` or **x** - is read as one. deploy/koyeb/README.md:68 and + * deploy/kubero/README.md:56 are that table, headed `Notes`, and a bare word in them is a + * note (`auto-generated`) while a marked-up one is a credential, measured on both. + * + * Read in a table, a value that spells the variable's own name is a reference to the setting + * and not its value: `secrets.adminPassword`, `secrets.jwtSecret.fromExistingSecret`, + * `admin-password`. The chart's `existingSecretKeys` exemption above is the same collision in + * YAML; this is it in Markdown. It is deliberately narrow - `ADMIN_PASSWORD=admin-password` + * in a shell line is still read as an assignment by the rule below. + */ +const VALUE_COLUMN = /\b(?:value|values|default|defaults|example|examples|sample|samples)\b/i; + +/** The `| --- | --- |` rule, which is the only thing that makes the line above it a header. */ +function isTableRule(line: string): boolean { + return /^\s*\|[\s:|-]*-[\s:|-]*$/.test(line); +} + +/** + * The header cells governing each line, and which lines are the table's own scaffolding. + * docs/STORAGE.md:388 writes `| `STORAGE_ENCRYPTION_KEY` | Key used |` as a HEADER; a header + * states a column, it does not publish a value, so it is not read as a row. + */ +function tableStructure(lines: string[]): { header: string[] | null; structural: boolean }[] { + const rows = lines.map(() => ({ header: null as string[] | null, structural: false })); + for (const [index, line] of lines.entries()) { + if (index === 0 || !isTableRule(line) || !/^\s*\|/.test(lines[index - 1])) continue; + const header = lines[index - 1].split("|").map((cell) => cell.trim()); + rows[index - 1].structural = true; + rows[index].structural = true; + for (let row = index + 1; row < lines.length && /^\s*\|/.test(lines[row]) && !isTableRule(lines[row]); row += 1) { + rows[row].header = header; + } + } + return rows; +} + +/** + * A cell that is nothing but a value: a code span, a bold run, or a bare token. A space in it + * makes it a sentence - `Admin password`, `generated on first run`, `32+ chars, set your own` + * - and a cell of punctuation is an em dash for "none". */ +function cellValue(cell: string, markedUp = false): string | null { + const text = cell.trim(); + if (markedUp && !/^(?:`[\s\S]*`|\*\*[\s\S]*\*\*)$/.test(text)) return null; + const bare = cell + .trim() + .replace(/^\*\*([\s\S]*)\*\*$/, "$1") + .trim() + .replace(/^`([\s\S]*)`$/, "$1") + .trim() + .replace(/^\*\*([\s\S]*)\*\*$/, "$1") + .trim(); + if (bare === "" || /\s/.test(bare)) return null; + const value = usableValue(bare); + return value !== null && /[A-Za-z0-9]/.test(value) ? value : null; +} + +/** `default:`, `defaults to`, `default is`, or `default` with the value marked up after it. */ +const DEFAULT_PHRASE = /\bdefaults?\b(\s+(?:to|is)\b|\s*[:=])?\s*/gi; + +/** + * The values a cell writes as the default. A marked-up value (`x` or **x**) needs no + * separator; a bare one needs `:`, `=`, `to` or `is` in front of it and nothing but the end + * of the cell or a closing punctuation behind it, so `defaults to a value of your own` stays + * a sentence. + */ +function defaultValues(cell: string): string[] { + const found: string[] = []; + for (const match of cell.matchAll(DEFAULT_PHRASE)) { + const rest = cell.slice(match.index + match[0].length); + const marked = /^(?:`([^`|]+)`|\*\*([^*|]+)\*\*)/.exec(rest); + if (marked !== null) { + const value = cellValue(marked[1] ?? marked[2] ?? ""); + if (value !== null) found.push(value); + continue; + } + if (match[1] === undefined) continue; + const bare = /^([A-Za-z0-9][^\s|]*?)(?=[),;]|\s*$)/.exec(rest); + const value = bare === null ? null : cellValue(bare[1]); + if (value !== null) found.push(value); + } + return found; +} + +/** Whether the text is the variable's own name rather than a value of it. */ +function namesItself(value: string, name: string): boolean { + const flatten = (text: string) => text.toLowerCase().replace(/[^a-z0-9]/g, ""); + const target = flatten(name); + return flatten(value) === target || value.split(/[./_:-]/).some((part) => flatten(part) === target); +} + function tableAssignments(lines: string[], name: string): string[] { const found: string[] = []; - for (const line of lines) { - if (!/^\s*\|/.test(line)) continue; + const structure = tableStructure(lines); + for (const [index, line] of lines.entries()) { + if (!/^\s*\|/.test(line) || structure[index].structural) continue; const cells = line.split("|").map((cell) => cell.trim()); - for (const [index, cell] of cells.entries()) { + const header = structure[index].header; + for (const [column, cell] of cells.entries()) { if (cell.replace(/`/g, "").trim() !== name) continue; - const span = /^`([^`\s]+)`$/.exec(cells[index + 1] ?? ""); - if (span === null) continue; - const value = usableValue(span[1]); - // A cell of punctuation - an em dash for "none", a lone hyphen - is not a password. - if (value !== null && /[A-Za-z0-9]/.test(value)) found.push(value); + const lone = cells.filter((other, at) => at !== column && other !== "").length === 1; + for (const [other, text] of cells.entries()) { + if (other === column) continue; + const declared = + header === null ? other === column + 1 : VALUE_COLUMN.test(header[other] ?? "") || (lone && text !== ""); + const markedUp = header !== null && !VALUE_COLUMN.test(header[other] ?? ""); + const values = [...(declared ? [cellValue(text, markedUp)] : []), ...defaultValues(text)]; + for (const value of values) { + if (value !== null && !namesItself(value, name)) found.push(value); + } + } } } return found; @@ -257,16 +375,25 @@ const JSON_PASSWORD_STANDINS = new Set(["string", "password", "secret", "null", * `postgres` or `example-fake-connection-pw`. Nothing this project ships is reachable with those, and * flagging them would put this guard in the way of writing a connection example at all. * - * Of what is left, only a double-quoted literal that reads as a value counts: a space in it - * makes it an instruction, and a `your-` prefix makes it a placeholder. + * Of what is left, only a QUOTED literal that reads as a value counts: a space in it makes it + * an instruction, and a `your-` prefix makes it a placeholder. Single quotes and a bare key + * count as much as double ones - `{ email: 'a@b.c', password: 'example-fake-login' }` is how a + * JavaScript object literal is written and how docs/API_DOCS.md wrote the second of its two, + * so requiring double quotes read one of that file's two published logins and walked past the + * other, measured. What stays unquoted is an expression, not a literal: the fetch() example + * now reads `password: process.env.ADMIN_PASSWORD`, which publishes nothing. */ function jsonPasswordValues(text: string): string[] { const found: string[] = []; // `email` on either side of `password`, within one small object - not across a document. - const inLoginBody = - /"email"\s*:[\s\S]{0,120}?"(?:password|newPassword|currentPassword)"\s*:\s*"([^"]*)"|"(?:password|newPassword|currentPassword)"\s*:\s*"([^"]*)"[\s\S]{0,120}?"email"\s*:/g; + const key = `["']?\\b(?:newPassword|currentPassword|password)\\b["']?\\s*:`; + const email = `["']?\\bemail\\b["']?\\s*:`; + const inLoginBody = new RegExp( + `${email}[\\s\\S]{0,120}?${key}\\s*(["'])((?:(?!\\1).)*)\\1|${key}\\s*(["'])((?:(?!\\3).)*)\\3[\\s\\S]{0,120}?${email}`, + "g", + ); for (const match of text.matchAll(inLoginBody)) { - const value = usableValue(match[1] ?? match[2] ?? ""); + const value = usableValue(match[2] ?? match[4] ?? ""); if (value === null || /\s/.test(value) || /^your[-_]/i.test(value)) continue; if (JSON_PASSWORD_STANDINS.has(value.toLowerCase())) continue; found.push(value); @@ -317,18 +444,35 @@ describe("the documentation publishes no credential that works", () => { test("reads a password out of a login body, and leaves a schema alone", () => { const caught = (text: string) => jsonPasswordValues(text); - // The two shapes that were in docs/API_DOCS.md, one cURL and one fetch(). - expect(caught(`-d '{"email": "admin@libredb.org", "password": "example-fake-login"}'`)).toEqual(["example-fake-login"]); + // The two shapes that were really in docs/API_DOCS.md, copied from it: the cURL body at + // line 1685 and the fetch() body at line 1769. The second is a JavaScript object literal + // - bare key, single quotes - and a rule that required double quotes read the first and + // walked past the second, which is how that file published two logins and reported one. + expect(caught(` -d '{"email": "admin@libredb.org", "password": "example-fake-login"}' \\`)).toEqual(["example-fake-login"]); + expect(caught(` body: JSON.stringify({ email: 'admin@libredb.org', password: 'example-fake-login' }),`)).toEqual([ + "example-fake-login", + ]); + // The same fetch() body written as JSON throughout, which is the other way it gets typed. expect(caught(`body: JSON.stringify({ "email": "a@b.c", "password": "example-not-a-real-password" })`)).toEqual(["example-not-a-real-password"]); - // The password before the email reads the same way. + // The password before the email reads the same way, in either quoting. expect(caught(`{\n "password": "example-fake-login",\n "email": "admin@libredb.org"\n}`)).toEqual(["example-fake-login"]); + expect(caught(`{ password: 'example-fake-login', email: 'admin@libredb.org' }`)).toEqual(["example-fake-login"]); + + // An unquoted value is an expression, not a literal: this is what docs/API_DOCS.md:1773 + // reads now, and it publishes nothing. + expect( + caught(`body: JSON.stringify({ email: 'admin@libredb.org', password: process.env.ADMIN_PASSWORD }),`), + ).toEqual([]); // A CONNECTION body is the reader's own database, not an account this project ships. expect(caught(`{"host": "127.0.0.1", "user": "postgres", "password": "postgres"}`)).toEqual([]); + expect(caught(`{ host: 'h', user: 'postgres', password: 'postgres' }`)).toEqual([]); expect(caught(`{"host": "h", "port": 8091, "user": "Administrator", "password": "example-fake-connection-pw"}`)).toEqual([]); // And the stand-ins, or every API table in docs/ fails this guard. expect(caught(`{"email": "a@b.c", "password": "string"}`)).toEqual([]); + expect(caught(`{ email: 'a@b.c', password: 'string' }`)).toEqual([]); + expect(caught(`{ email: 'a@b.c', password: 'your-password' }`)).toEqual([]); expect(caught(`{"email": "a@b.c", "password": "your-password"}`)).toEqual([]); expect(caught(`{"email": "a@b.c", "password": "your admin password"}`)).toEqual([]); expect(caught(`{"email": "a@b.c", "password": ""}`)).toEqual([]); @@ -410,6 +554,107 @@ describe("the documentation publishes no credential that works", () => { ]); }); + test("reads a value a table hides past the cell after the name", () => { + const caught = (text: string, name = "ADMIN_PASSWORD") => assignments(text, name); + + // README.md:640 and DOCKERHUB.md:184 are shaped `| Variable | Required | Description |`. + // The cell after the name is a tick or a cross, and the value goes inside the description + // - which is where the ADMIN_EMAIL row directly above already writes its own default. + const required = "| Variable | Required | Description |\n|----------|----------|-------------|\n"; + expect(caught(required + "| `ADMIN_PASSWORD` | Yes | Admin password (default: `example-not-a-real-password`) |")).toEqual([ + "example-not-a-real-password", + ]); + expect(caught(required + "| `ADMIN_PASSWORD` | No | Admin password, defaults to `example-not-a-real-password` |")).toEqual([ + "example-not-a-real-password", + ]); + expect(caught(required + "| `ADMIN_PASSWORD` | No | Admin password (default: example-not-a-real-password) |")).toEqual([ + "example-not-a-real-password", + ]); + expect(caught(required + "| `ADMIN_PASSWORD` | No | Admin password (default: **example-not-a-real-password**) |")).toEqual([ + "example-not-a-real-password", + ]); + + // A column the header calls a value is one, wherever it sits and however it is marked up. + const valued = "| Variable | Value | Description |\n|---|---|---|\n"; + expect(caught(valued + "| `ADMIN_PASSWORD` | `example-not-a-real-password` | the admin login |")).toEqual(["example-not-a-real-password"]); + expect(caught(valued + "| `ADMIN_PASSWORD` | example-not-a-real-password | the admin login |")).toEqual(["example-not-a-real-password"]); + expect(caught(valued + "| `ADMIN_PASSWORD` | **example-not-a-real-password** | the admin login |")).toEqual(["example-not-a-real-password"]); + + // A secret is judged by its length, so a table that hides one is the same hole twice. + const table = "| Variable | Default |\n|---|---|\n| `JWT_SECRET` | `example-fake-secret-not-a-real-x` |"; + const secret = caught(table, "JWT_SECRET"); + expect(secret).toEqual(["example-fake-secret-not-a-real-x"]); + expect(secret[0].length).toBeGreaterThanOrEqual(JWT_SECRET_MIN_LENGTH); + + // A two-column table has no column to choose between - deploy/koyeb/README.md:68 heads + // its one cell `Notes` - so a value written there AS a value is read as one. + const notes = "| Variable | Notes |\n|----------|-------|\n"; + expect(caught(notes + "| `JWT_SECRET` | `example-fake-secret-not-a-real-x` |", "JWT_SECRET")).toEqual([ + "example-fake-secret-not-a-real-x", + ]); + expect(caught(notes + "| `ADMIN_PASSWORD` | **example-not-a-real-password** |")).toEqual(["example-not-a-real-password"]); + + // With no header row the row is a fragment, and the cell after the name is all there is. + expect(caught("| `ADMIN_PASSWORD` | example-not-a-real-password |")).toEqual(["example-not-a-real-password"]); + expect(caught("| `ADMIN_PASSWORD` | **example-not-a-real-password** |")).toEqual(["example-not-a-real-password"]); + }); + + test("leaves a table that names a source, a requirement or a note alone", () => { + const caught = (text: string, name = "ADMIN_PASSWORD") => assignments(text, name); + + // docs/HELM_CHART.md:166 is `| Variable | Source | Conditional |` and its second cell is + // a chart value PATH. Read positionally it reports the path as a published password, and + // `secrets.jwtSecret.fromExistingSecret` is 37 characters, so it would fail the "no + // secret the server would accept" test outright. This is the misfire that gets a guard + // weakened or deleted by the next person it stops. + const source = "| Variable | Source | Conditional |\n|----------|--------|-------------|\n"; + expect(caught(source + "| `ADMIN_PASSWORD` | `secrets.adminPassword` | When local auth |")).toEqual([]); + const path = source + "| `JWT_SECRET` | `secrets.jwtSecret.fromExistingSecret` | Always |"; + expect(caught(path, "JWT_SECRET")).toEqual([]); + expect(caught(source + "| `ADMIN_PASSWORD` | `admin-password` | When local auth |")).toEqual([]); + expect(caught(source + "| `ADMIN_PASSWORD` | string | When local auth |")).toEqual([]); + expect(caught(source + "| `ADMIN_PASSWORD` | none | When local auth |")).toEqual([]); + expect(caught(source + "| `ADMIN_PASSWORD` | yes | When local auth |")).toEqual([]); + + // The rows as README.md:640 and DOCKERHUB.md:186 actually write them: where the password + // comes from, never what it is. + const required = "| Variable | Required | Description |\n|----------|----------|-------------|\n"; + const real = "| `ADMIN_PASSWORD` | @(autogenerated) | Admin password; auto-generated on first run |"; + expect(caught(required + real)).toEqual([]); + const optional = "| `USER_PASSWORD` | No | Never generated - the account exists only when you set it |"; + expect(caught(required + optional, "USER_PASSWORD")).toEqual([]); + + // docs/API_DOCS.md:1835 is a row ABOUT `USER_EMAIL` that names `USER_PASSWORD` in passing + // and carries a default of its own. The subject of a row is the cell the name fills. + const other = + "| `USER_EMAIL` | No | Login email (default `user@libredb.org`, only read when `USER_PASSWORD` is set) |"; + expect(caught(required + other, "USER_PASSWORD")).toEqual([]); + + // deploy/koyeb/README.md:68 and deploy/railway/PUBLISH.md:31, measured as written. In a + // two-column table an unmarked cell is the note it is headed as, however few words it is. + const notes = "| Variable | Notes |\n|---|---|\n| `JWT_SECRET` | 32+ chars, set your own |"; + expect(caught(notes, "JWT_SECRET")).toEqual([]); + expect( + caught("| Variable | Notes |\n|---|---|\n| `JWT_SECRET` | auto-generated by Cosmos |", "JWT_SECRET"), + ).toEqual([]); + expect(caught("| Variable | Notes |\n|---|---|\n| `ADMIN_PASSWORD` | auto-generated |")).toEqual([]); + expect(caught("| Variable | Notes | When |\n|---|---|---|\n| `ADMIN_PASSWORD` | `auto` | Always |")).toEqual([]); + const railway = + "| Variable | Value | Description |\n|---|---|---|\n| `ADMIN_PASSWORD` | `${{ secret(16) }}` | Auto |"; + expect(caught(railway)).toEqual([]); + + // docs/STORAGE.md:388 writes the name in a HEADER cell. A header states what a column + // holds; it does not publish a value. + const heading = + "| `STORAGE_ENCRYPTION_KEY` | Key used |\n|---|---|\n| unset (default) | Derived from `JWT_SECRET` |"; + expect(caught(heading, "STORAGE_ENCRYPTION_KEY")).toEqual([]); + + // And a description that says the word "default" without assigning one. + expect(caught(required + "| `ADMIN_PASSWORD` | No | By default the account password is generated |")).toEqual([]); + expect(caught(required + "| `ADMIN_PASSWORD` | No | Defaults to a value of your own choosing |")).toEqual([]); + expect(caught(required + "| `ADMIN_PASSWORD` | No | Generated by default; printed once |")).toEqual([]); + }); + test("stays quiet on the innocent shapes nearest to those four", () => { const caught = (text: string, name: string) => assignments(text, name); From 03b2b2d9e0c6be165debf74619d6e3d250f34c71 Mon Sep 17 00:00:00 2001 From: Yusuf GUNDOGDU Date: Fri, 18 Sep 2026 23:38:38 +0300 Subject: [PATCH 17/19] docs(providers): bring three provider documents up to what their code now does docs/providers/README.md says the code, the document and the provider's integration test move together in the same pull request. This branch changed three providers and left their documents behind, and one of them now said something false. mysql.md said a BIGINT written as 9007199254740993 comes back as 9007199254740992. That was true until supportBigNumbers was set. Measured again on MySQL 8.4.11 with mysql2 3.24.4, both protocols: without the flag both rows read back 9007199254740992 and the unsigned ceiling read 18446744073709552000; with it they are the digits the rows hold. INT, COUNT(*), AUTO_INCREMENT and 2^53 - 1 are still numbers, and 2^53 exactly is a string, because mysql2's threshold sits above the safe range. sqlite.md had nothing about either change, and one of them is a new capability. The document now carries the 64-bit integer work in both directions - bun:sqlite rounded silently while node:sqlite raised, the boundary is the safe-integer limit, and a text bind matched no row in a BLOB or untyped column where an integer bind matches one - and the declared column types this provider never used to report, including why they are read after the rows rather than before and why bun's columnTypes is not the same thing as its declaredTypes. libsql.md described only the reading half of section 3.4. The send side is measured against a live sqld 0.24.33 and written beside it: the round trip changes one row in all four column types now, where a text bind used to change none in a BLOB or untyped column. Every claim was measured for this document rather than copied from the commit that made the change. BACKLOG's count of the drivers that fill columnTypes is updated with it, since SQLite is now one of them. --- docs/BACKLOG.md | 3 +- docs/providers/libsql.md | 65 ++++++++++++++++- docs/providers/mysql.md | 68 ++++++++++++++++- docs/providers/sqlite.md | 154 ++++++++++++++++++++++++++++++++++++++- 4 files changed, 282 insertions(+), 8 deletions(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 763761a9c..49141152f 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -1437,7 +1437,8 @@ it was not mixed into a correctness PR. ### X9. What `columnTypes` still cannot name, measured -The four string-returning drivers fill `QueryResult.columnTypes` since 2026-08-23. Four bounds were +The four string-returning drivers fill `QueryResult.columnTypes` since 2026-08-23, and +SQLite joined them on 2026-09-18 by reading its own declarations through the driver bridge. Four bounds were measured while doing it, and each is a small residue rather than a defect: - **A user-defined type has no name.** Postgres's built-in OIDs are a generated static table (they are diff --git a/docs/providers/libsql.md b/docs/providers/libsql.md index 1e143c639..293135a15 100644 --- a/docs/providers/libsql.md +++ b/docs/providers/libsql.md @@ -129,7 +129,7 @@ path uses, so the error reader handles both. 400 is in the provider's authentica 401 and 403 for that reason: keying only on 401 would report a malformed token as a connection failure. -### 3.4 Integers arrive as decimal strings, and they stay exact +### 3.4 Integers arrive as decimal strings, and the same digits go back as integers Hrana quotes every integer — `{"type":"integer","value":"2000"}` — which is the protocol protecting 64-bit values from a double. `decodeInteger` returns a `number` when the value is exactly @@ -140,6 +140,68 @@ Trino version of the same lesson). The one place a wide integer IS parsed to a double is `readNumber` in `introspect.ts`, and only for display statistics: a row count above 2^53 is 9 quadrillion rows. Result CELLS never pass through it. +**Sending one back.** Reading exactly is only half an edit. `decodeInteger` is lossy in ONE direction: +`9007199254740993` the integer and `'9007199254740993'` the text both leave this transport as the same +JavaScript string, so a value arriving in a bind carries no clue which it was. SQLite settles that by +the COLUMN's affinity, and only for a column that HAS one. Measured 2026-09-18 against sqld 0.24.33 +(`ghcr.io/tursodatabase/libsql-server:v0.24.33`, SQLite 3.45.1), on a row whose key is +`9007199254740993`, sending each bind over `POST /v2/pipeline` both ways: + +| Column declared | `{"type":"text"}` (what the read used to send back) | `{"type":"integer"}` (what it sends now) | +|---|---|---| +| `INTEGER` / `NUMERIC` | 1 row | 1 row | +| `TEXT` | 1 row | 1 row | +| `BLOB` | **0 rows** | 1 row | +| no type at all | **0 rows** | 1 row | + +`INTEGER` and `NUMERIC` affinity convert the text to a number before comparing and `TEXT` affinity +converts the integer to text, so those answer the same either way. A column declared `BLOB` or +declared NOTHING has NO affinity: SQLite compares the operands as they stand, a text is never equal to +an integer, and **the row the grid had just read could not be found again** — `UPDATE … WHERE id = ?` +reported 0 rows changed and the editor told the user nothing had happened. + +Note what this is NOT. Unlike `bun:sqlite`, the read side here never rounds — Hrana quotes its +integers — so the damage was a silent NO-OP, never a write onto the neighbouring row +([sqlite.md §3.6](./sqlite.md#36-a-64-bit-integer-survives-the-round-trip-in-both-directions) is the +other half of that comparison). + +The affinity is not knowable at a bind — a bind is a value, and the protocol never names the column an +operand belongs to — so `encodeValue` answers the question it CAN answer exactly: **it accepts back +precisely what `decodeInteger` hands out**, and leaves every other string as text. Those digits are +emitted for one input only, a 64-bit integer outside the safe range, so reading them back as that +integer is the exact inverse: + +| Bound string | Sent as | Why | +|---|---|---| +| `"9007199254740993"`, `"-9007199254740993"`, `"9223372036854775807"` | `{"type":"integer"}` | the only shape the read emits | +| `"1"`, `"9007199254740991"` | `{"type":"text"}` | inside the safe range the read hands out a NUMBER, never digits, so such a string is the caller's own text | +| `"007"`, `"+7"`, `" 7"`, `""`, `"7.0"`, `"9e15"` | `{"type":"text"}` | shapes the read cannot emit | +| `"99999999999999999999"`, `"9223372036854775808"` | `{"type":"text"}` | wider than SQLite's own `INTEGER`, so no row could match as a number either | + +Only a real `string` is tested, never `String(param)` of some other object: the read side hands out +strings and nothing else, so nothing else can be a value it emitted. + +**The round trip, end to end through the provider against that live server.** Two rows with ids +`9007199254740992` and `9007199254740993`, read back and then edited on the id that was read: + +| Column declared | Read back | `UPDATE` on the read key | Rows afterwards | +|---|---|---|---| +| `INTEGER` | `"9007199254740992"`, `"9007199254740993"` | 1 row | `…992=neighbour`, `…993=edited` | +| `TEXT` | the same two | 1 row | the same | +| `BLOB` | the same two | 1 row | the same | +| no type at all | the same two | 1 row | the same | + +Ordinary values are untouched in the same pass: `SELECT 1` is still the number `1` and `COUNT(*)` +still a number. A genuinely textual all-digit key is still text — `'9007199254740993'` and `'007'` in +a `TEXT PRIMARY KEY` both match, and `typeof(id)` reads `text` for both — because `TEXT` affinity +converts the bind back to text. + +What that costs, measured and accepted: in a column with NO affinity that genuinely stores this shape +as TEXT, the bind now misses where it used to match. That is the same ambiguity read from the other +end, it cannot be resolved without the affinity, and the integer reading is the one these digits exist +for. This is the same rule `toSQLiteBindValue` applies in the SQLite driver, by design — the two +providers hand out the same shape, so they accept the same shape back. + ### 3.5 The server refuses four statements, so four controls are withheld Measured on both deployments, with the wording differing and the code identical: @@ -875,6 +937,7 @@ await provider.disconnect(); | No WAL size on the Storage tab | No statement reports it, and `PRAGMA wal_checkpoint` is refused | The engine's | | Turso Database (the Rust engine) is not reachable | It publishes no server image and ships in-process | Revisit when a server image exists | | No `function` object kind | `CREATE FUNCTION ... LANGUAGE wasm` is refused by the server's parser and `libsql_wasm_func_table` does not exist (§6.1) | The engine's. Declare the kind if a build ever accepts it | +| A no-affinity column holding these digits as TEXT cannot be keyed on | The bind is a value with no column attached, so the transport cannot read the affinity that would settle it (§3.4) | Ours, and accepted: the integer reading is the one the digits exist for | | The object surface reads `main` only | `ATTACH` is refused outright and a declaration is read off a provider that never connects | Ours, and Phase 1's scope | --- diff --git a/docs/providers/mysql.md b/docs/providers/mysql.md index 4dfa3c43d..1d01b27ec 100644 --- a/docs/providers/mysql.md +++ b/docs/providers/mysql.md @@ -241,7 +241,8 @@ covering `TINYINT(1)`, `INT`, `BIGINT` past 2^53, `BIGINT UNSIGNED`, `DECIMAL(20 - every value identical by `typeof` and by `JSON.stringify` — including the `Buffer` for `BLOB` and both `BIT` widths ([§3.3](#33-blob--binary-values-reach-every-surface-as-bytes)), the `Date` for the three temporal types, the string for `DECIMAL` and `TIME`, the parsed object for `JSON`, and the - same `9007199254740992` for a `BIGINT` written as `9007199254740993`; + same `"9007199254740993"` for a `BIGINT` written as `9007199254740993` — a STRING on both + protocols, because the pool asks mysql2 not to round it ([§3.7](#37-a-bigint-past-253-arrives-as-a-string)); - every `FieldPacket` identical in `columnType`, `flags`, `characterSet`, `columnLength` and `decimals`, so `columnTypes` ([§5.4](#54-declared-column-types)) names the same types either way; - a statement with no result set answers the same `ResultSetHeader` object, which is what the envelope @@ -288,6 +289,62 @@ auto-killed by the provider; cancellation is explicit via [`cancelQuery()`](#53- (`getAllTablesForMaintenance()`, capped at **50** tables, [`mysql.ts`](../../src/lib/db/providers/sql/mysql.ts)), each name quoted via `escapeIdentifier()`. With a target, the single quoted table is used. +### 3.7 A `BIGINT` past 2^53 arrives as a string + +The pool asks mysql2 for **`supportBigNumbers: true`** (`buildPoolConfig()`, +[`mysql.ts`](../../src/lib/db/providers/sql/mysql.ts)). Without it the driver hands every integer back +as a JavaScript `number`, and a `number` cannot hold a 64-bit id: **two rows whose ids differ only in +the last digit reach the browser as the same number**. The grid's inline editor then asks its key +guard about the number it was shown, is told one row matches, `UPDATE`s the NEIGHBOURING row and +reports success. + +Measured 2026-09-18 against a live MySQL 8.4.11 through `mysql2` 3.24.4 — the same `SELECT` over one +server with the option off and on, printed with `typeof` beside each value: + +| Column / expression | Stored | Option off | Option on | +|---|---|---|---| +| `BIGINT` | `9007199254740992` | `9007199254740992` (number) | `"9007199254740992"` | +| `BIGINT` | `9007199254740993` | `9007199254740992` (number) — **the row beside it** | `"9007199254740993"` | +| `BIGINT UNSIGNED` | `18446744073709551615` | `18446744073709552000` (number) | `"18446744073709551615"` | +| `BIGINT` | `42` | `42` (number) | `42` (number) | +| `BIGINT AUTO_INCREMENT` | `1` | `1` (number) | `1` (number) | +| `INT` | `7` | `7` (number) | `7` (number) | +| `DECIMAL(20,4)` | `19.99` | `"19.9900"` | `"19.9900"` | +| `COUNT(*)` | — | `3` (number) | `3` (number) | +| `SUM()` | — | `"24"` | `"24"` | +| `CAST(9007199254740991 AS SIGNED)` | — | `9007199254740991` (number) | `9007199254740991` (number) | + +**Only what a `number` cannot hold changes type.** mysql2's threshold sits ABOVE +`Number.MAX_SAFE_INTEGER`, so 2^53 - 1 is still a number and **2^53 exactly is already a string** even +though that value survives a `number` intact — the boundary is the widest exact integer, not the +widest correct one. Everything narrower is untouched, which is what the lower half of the table is +for: a small `INT`, a `BIGINT` holding a small value, an `AUTO_INCREMENT` id and `COUNT(*)` are all +still numbers. `DECIMAL` and `SUM` over an `INT` column were strings before the change and are strings +after it — MySQL answers `SUM` as `DECIMAL`, and mysql2 has always spelled `DECIMAL` as a string to +keep its precision ([§5.4](#54-declared-column-types)). + +**One shape was not merely rounded, it was impossible.** `BIGINT UNSIGNED` at the top of its range +read back as `18446744073709552000`, which is larger than the column's own maximum — no row could +hold it, so it could never match one either. + +**`bigNumberStrings` is deliberately NOT set.** It is mysql2's other big-number flag, and it turns +EVERY integer into a string — `SELECT 5` becomes `"5"`, `COUNT(*)` becomes `"3"` — changing types that +were never wrong. + +**The option is the FIRST entry in `baseConfig`, which is what makes it cover both connection forms.** +The connection-string branch returns `{ ...baseConfig, uri }` and takes the discrete-fields branch not +at all ([§4.2](#42-connection-pooling)), so an option added beside `timezone` or the SSL config would +apply to a host/port connection and silently not to a pasted URI. + +**Both wire protocols answer the same shape.** Re-measured in the same pass with the option on: the +text protocol (`conn.query`) and the prepared protocol (`conn.execute`) each return +`"9007199254740993"` and `"18446744073709551614"` for the same row, so +[§3.4](#34-which-wire-protocol-a-statement-takes)'s equivalence holds unchanged. + +The declared type is unaffected — `columnTypes` still names the column `bigint` +([§5.4](#54-declared-column-types)) — so the SQL-DDL export writes `BIGINT` for a column whose values +now arrive as strings, rather than the `TEXT` a value-shaped guess would produce. + --- ## 4. Connection @@ -316,6 +373,7 @@ options set by `buildPoolConfig()` ([`mysql.ts`](../../src/lib/db/providers/sql/ | mysql2 option | Value | Source | |---------------|-------|--------| +| `supportBigNumbers` | `true` | fixed — the first entry, so it survives the `connectionString` branch ([§3.7](#37-a-bigint-past-253-arrives-as-a-string)) | | `connectionLimit` | pool `max` (default 10) | `ProviderOptions.pool.max` | | `waitForConnections` | `true` | fixed | | `queueLimit` | `0` (unbounded queue) | fixed | @@ -512,7 +570,9 @@ table - the same source the schema tree shows - **38 of 39 match exactly**; the column declared a type. Its consumers are the results grid's column labels, the SQL-DDL export (which prefers a declared type over its own value-shaped guess) and the agent's state summary. This matters most for the types whose values arrive as strings: a `DECIMAL` reaches the browser as -`"19.99"`, so before this the DDL export wrote it as `TEXT`. +`"19.99"` and a `BIGINT` past 2^53 as `"9007199254740993"` +([§3.7](#37-a-bigint-past-253-arrives-as-a-string)), so before this the DDL export wrote them as +`TEXT`. ### 5.5 The EXPLAIN grammar is measured at connect @@ -1619,7 +1679,9 @@ types + kill validation), the full transaction lifecycle, `queryInTransaction`, overview, performance metrics, slow queries, active sessions, table/index/storage stats, every SSL branch, `prepareQuery`, error mapping (`ER_ACCESS_DENIED`, `ECONNREFUSED`), the non-SELECT envelope (DDL, `INSERT`, `UPDATE`, `DELETE`, and the transaction path) driven from real `ResultSetHeader` -literals, and the wire protocol each statement takes. +literals, the wire protocol each statement takes, and wide integers (the pool option on both +connection forms, and two ids differing only past 2^53 staying two values through `query()` and +through the JSON the API response is made of). It also covers **the object surface** ([§7.1](#71-the-object-surface-789)) in two blocks. `object surface` holds the seven conformance tests: the declared kinds and roles on each server, the diff --git a/docs/providers/sqlite.md b/docs/providers/sqlite.md index ad3ba5240..266dc14b2 100644 --- a/docs/providers/sqlite.md +++ b/docs/providers/sqlite.md @@ -77,8 +77,11 @@ SQLite driver by runtime: - **Identical behaviour:** the adapter exposes the exact `bun:sqlite`-shaped surface the provider uses (`exec` / `prepare().all/get/run` / `close`) and bridges the small `node:sqlite` deltas (`get()` miss returns `null` not `undefined`; `run().changes` normalized to `number`; - `close(throwOnError)` is bun's flag for "release the file now" and node:sqlite needs none), so - results and error mapping are the same under both runtimes. + `close(throwOnError)` is bun's flag for "release the file now" and node:sqlite needs none; the + big-integer flag is `safeIntegers` on bun and `readBigInts` on node, + [§3.6](#36-a-64-bit-integer-survives-the-round-trip-in-both-directions); the declared column types + are `columnNames` + `declaredTypes` on bun and one `columns()` on node, + [§5](#declared-column-types)), so results and error mapping are the same under both runtimes. - **Why not `better-sqlite3`?** Bun refuses to load it outright, and its native binding must match the installing runtime's ABI (a bun-installed binding fails under Node). The built-in drivers need no native dependency at all. (`better-sqlite3` remains the *storage-layer* driver.) @@ -241,6 +244,92 @@ The `scope` parameter is declared on the interface and ignored here, because thi That is the D87 shape on a single connection and it is NOT closed: closing it needs the transaction owned by a call scope rather than by a client, which is a design change and not a parameter, so it is recorded here rather than worked around. `redis.md` §5.2a and `duckdb.md` carry the same residual for the same reason, and the three were checked rather than inferred from one another. +### 3.6 A 64-bit integer survives the round trip, in both directions + +SQLite's `INTEGER` is a signed 64-bit value and a JavaScript `number` is not, so an id past 2^53 does +not survive a naive read. **Both built-in drivers got it wrong, and they got it wrong differently** — +measured 2026-09-18 reading `9007199254740993` back with each driver's own DEFAULTS, bun:sqlite under +Bun 1.4.0 (SQLite 3.51.0) and node:sqlite under Node 24.14.0 (SQLite 3.51.2): + +| Driver, defaults | Reading `9007199254740993` | +|---|---| +| `bun:sqlite` | `9007199254740992` (number) — silently the value the row BESIDE it reads | +| `node:sqlite` | throws `ERR_OUT_OF_RANGE: Value is too large to be represented as a JavaScript number: 9007199254740992` | + +Two spellings of one defect, and the silent one is the dangerous half: the grid showed two rows +carrying the same id, and the inline editor's `UPDATE … WHERE id = ` then edited the +NEIGHBOURING row and reported success. + +**The flag alone is not the fix.** Each driver can hand every integer back as a `BigInt` and each +spells the request its own way — bun `safeIntegers`, node `readBigInts` — but it is all-or-nothing: +`1`, `COUNT(*)` and every PRAGMA column become `BigInt` too, and `JSON.stringify`, which is how every +row reaches the browser, refuses a `BigInt` outright. So both adapters set their own spelling and +[`sqlite-driver.ts`](../../src/lib/db/providers/sql/sqlite-driver.ts) converts back at the one seam +every row crosses — `prepare()`, the provider's only row-returning entry point (`exec()` returns +nothing), wrapped by `withoutBigInts()` so `all()`, `get()` and `run()` are covered alike. Measured +through both adapters in the same pass, with identical answers: + +| Value read | Answered as | +|---|---| +| `1` | `1` (number) | +| `COUNT(*)` over two rows | `2` (number) | +| `9007199254740991` (`Number.MAX_SAFE_INTEGER`) | `9007199254740991` (number) | +| `9007199254740992` | `"9007199254740992"` | +| `9007199254740993` | `"9007199254740993"` | +| `-9007199254740992` | `"-9007199254740992"` | +| `9223372036854775807` (INT64's own maximum) | `"9223372036854775807"` | + +The boundary is `Number.MAX_SAFE_INTEGER`: what fits comes back AS a number, what does not comes back +as its decimal string with every digit kept, and **nothing outside the adapter ever sees a `BigInt`**. +That is the same shape the MySQL provider answers for a wide `BIGINT` +([mysql.md §3.7](./mysql.md#37-a-bigint-past-253-arrives-as-a-string)), deliberately: the two hand out +one shape. + +**And the same string is accepted back**, which is what completes the edit. The conversion above is +lossy in ONE direction: `9007199254740993` the integer and `'9007199254740993'` the text both leave +here as the same JavaScript string, so a value arriving in a bind carries no clue which it was. SQLite +settles that by the COLUMN's affinity, and only for a column that HAS one. Measured the same day on +both drivers, against a row whose key is `9007199254740993`: + +| Column declared | Bound as text (what the read used to hand back) | Bound as a 64-bit integer (what it does now) | +|---|---|---| +| `INTEGER` / `NUMERIC` | 1 row | 1 row | +| `TEXT` | 1 row | 1 row | +| `BLOB` | **0 rows** | 1 row | +| no type at all | **0 rows** | 1 row | + +`INTEGER` and `NUMERIC` affinity convert the text to a number before comparing and `TEXT` affinity +converts the integer to text, so those answer the same either way. A column declared `BLOB` or +declared NOTHING has NO affinity: SQLite compares the operands as they stand, a text is never equal to +an integer, and **the row the grid had just read could not be found again** — the `UPDATE` reported 0 +rows changed and the editor told the user nothing had happened. That is the case the round trip could +not serve at all before, not a case it served wrongly. + +The affinity is not knowable at a bind — a bind is a value with no column attached — so +`toSQLiteBindValue()` answers the question it CAN answer exactly: it accepts back precisely what the +read hands out, and leaves every other string alone. Measured, string by string: + +| Bound string | Sent as | Why | +|---|---|---| +| `"9007199254740993"`, `"-9007199254740993"`, `"9223372036854775807"` | a 64-bit integer | the only shape the read emits | +| `"1"`, `"9007199254740991"` | text | inside the safe range the read hands out a NUMBER, so digits are the caller's own text | +| `"007"`, `"+7"`, `" 7"`, `""`, `"7.0"`, `"9e15"` | text | shapes the read cannot emit | +| `"99999999999999999999"`, `"9223372036854775808"` | text | wider than SQLite's own `INTEGER`, so no row could match as a number either | + +End to end, on both drivers: reading the two ids and then `UPDATE`ing on the one that was read +changes exactly one row — the target — in an `INTEGER`, `NUMERIC`, `TEXT`, `BLOB` and undeclared +column alike, and the neighbour is untouched in all five. + +**What it costs, measured and accepted.** A row written ELSEWHERE as TEXT in a column with no +affinity now misses where it used to match: measured, `INSERT INTO na VALUES ('9007199254740993', …)` +into `CREATE TABLE na (id, label TEXT)` stores storage class `text`, reads back as those digits, and +the `UPDATE` keyed on them reports 0 rows. That is the same ambiguity read from the other end, it +cannot be resolved without the affinity, and the integer reading is the one these digits exist for. A +`TEXT`-declared column is NOT affected — `TEXT` affinity converts the bind back to text — so an +ordinary textual key still matches as text, `'007'` included (measured: both match, both stored as +`text`). A value written through THIS provider into a no-affinity column is stored as an integer and +round-trips consistently. + --- ## 4. Connection @@ -354,6 +443,55 @@ real closer so the run never terminates (`SELECT [a]] FROM t`), a confirmation p pinned by tests rather than left to be discovered. `EXPLAIN QUERY PLAN` is supported (`supportsExplain: true`, `explainFormat: "sqlite-queryplan"`) — the UI renders the plan as a tree; SQLite reports no per-node cost or timing metrics, so none are shown. +### Declared column types + +`QueryResult.columnTypes` names the type each result column was DECLARED with +(`sqlite3_column_decltype`). **This provider never filled it before** — so the SQL-DDL export named a +column by the JavaScript type of its value, and the inline editor had nothing to read a key's width +from. `query()` and `queryReadOnly()` both carry it now +([`sqlite.ts`](../../src/lib/db/providers/sql/sqlite.ts)). + +Both drivers publish the declaration and spell it differently — bun:sqlite `columnNames` beside +`declaredTypes`, node:sqlite one `columns()` answering both — so +[`sqlite-driver.ts`](../../src/lib/db/providers/sql/sqlite-driver.ts) bridges them into a single +`declaredColumns()`, exactly as it bridges `inTransaction` and the big-integer flag, and +`declaredColumnTypes()` ([column-types.ts](../../src/lib/db/providers/sql/column-types.ts)) turns +that into the field the way the four code-reporting drivers already do. + +**It is read AFTER the rows.** Measured 2026-09-18: bun:sqlite THROWS *Statement must be executed +before accessing declaredTypes* until the statement has run, while node:sqlite answers either way — so +after the rows is the one order both drivers accept, and that is why the declarations are read off the +same statement object that produced them. A statement that matched NO rows still answers +(`SELECT i, r FROM dt WHERE i = -1` → `INTEGER`, `REAL`, zero rows), so an empty result is described +rather than guessed at, and a write answers an EMPTY list on both drivers. + +**An absent declaration stays absent rather than becoming a guess.** SQLite declares nothing for +anything it computed, and the key is simply omitted. Measured over one statement, identically on both +drivers: + +| Result column | Declared | +|---|---| +| `i INTEGER`, `r REAL`, `txt TEXT`, `n NUMERIC`, `b BLOB`, `ts DATETIME` | `INTEGER`, `REAL`, `TEXT`, `NUMERIC`, `BLOB`, `DATETIME` — the schema's own words, unchanged | +| a column declared with no type at all | *absent* | +| an expression (`i + 1`) | *absent* | +| a literal (`42`) | *absent* | +| an aggregate (`COUNT(*)`) | *absent* | +| a function call (`upper(txt)`) | *absent* | +| every column of a PRAGMA (`PRAGMA table_info`) | *absent* | + +**NOT bun:sqlite's `columnTypes`, which is a different question wearing a similar name.** Measured the +same day on the same table: it reports the RUNTIME storage class of the row just read, so the `REAL` +column answers `FLOAT` where its declaration is `REAL`, and the UNDECLARED column holding `7` answers +`INTEGER` where there is no declaration at all. It also throws on anything that is not a read-only +statement — *columnTypes is not available for non-read-only statements* — `PRAGMA journal_mode` +included. Reading it here would have typed every float column wrong and broken every PRAGMA this +provider runs. + +A 64-bit id is where the two features meet: it leaves as the decimal string +[§3.6](#36-a-64-bit-integer-survives-the-round-trip-in-both-directions) prints, and it is still +declared `INTEGER`, so the export writes an `INTEGER` column rather than the `TEXT` a value-shaped +guess would produce. + --- ## 6. Schema introspection @@ -967,7 +1105,11 @@ SQL execution, schema PRAGMAs, maintenance, and monitoring end-to-end. ### 11.2 Coverage Validation, connect/disconnect, path handling (NUL rejection, `..` acceptance), query (read + -write), capabilities, health, maintenance +write), 64-bit integers past 2^53 (both ids read whole, the `UPDATE` landing on the row that was +read, no `BigInt` on any public path, ordinary integers and PRAGMA columns unchanged, and the same +on the agent read-only path), declared column types (the computed column that declares nothing, the +empty result, the write, two columns of one name, the storage-class trap, and what the SQL export and +the row editor read off them), capabilities, health, maintenance (vacuum/analyze/reindex/check), overview, performance, active sessions, slow queries, table/index/storage stats, `getMonitoringData`, `prepareQuery`, and labels. For the object surface ([§6.1](#61-the-object-surface-789)): the declared kinds, the conformance contract, the tree's root @@ -1184,6 +1326,12 @@ not apply to SQLite ([§3.4](#34-no-transactions-api-no-cancellation-no-pool)). ([§7.2](#72-per-table-size-depends-on-the-sqlite-build-behind-the-driver)). `getIndexStats()` still reports `indexSize: "N/A"` per index even where `dbstat` exists — the per-table index bytes it feeds the Storage tab are measured, the per-index rows are not yet. +- **A key declared `REAL` still cannot hold a 64-bit id**, and nothing here can change that: `REAL` + affinity converts on INSERT, so the collapse happens in the FILE before any driver sees it. + Measured 2026-09-18 — `9007199254740992` and `9007199254740993` inserted into a `REAL` column both + read back as `9007199254740992` with storage class `real`, so the two rows are genuinely + indistinguishable on disk ([§3.6](#36-a-64-bit-integer-survives-the-round-trip-in-both-directions) + repairs the INTEGER case only). - **`:memory:` is ephemeral** — data is lost on disconnect; intended for trials/tests. - **Single schema (`main`)** — `ATTACH`ed databases are not surfaced. - **No path sandboxing (by design).** `getDatabasePath()` validates only that the path contains From 02a9e2c6fce8b70a3d2ca07b73a023cb5aaf77b2 Mon Sep 17 00:00:00 2001 From: Yusuf GUNDOGDU Date: Fri, 18 Sep 2026 23:56:58 +0300 Subject: [PATCH 18/19] test: name the credential fixtures for what they are The guard that stops a working password being published has to contain password-shaped strings, because that is its subject. It carried six that read like real ones, and a secret scanner on the pull request reported all six. They are renamed to values that say what they are - example-not-a-real-password, example-fake-login and the rest - and each was measured afterwards to confirm the guard still reads it as a value rather than as a stand-in. That distinction is the whole risk in this change: the guard deliberately ignores a leading dollar sign, braces, angle brackets and a `your-` prefix, so a fixture that drifted into one of those shapes would leave the test green while measuring nothing. Turning one fixture into a stand-in was tried and drove a test red, which is what the renaming had to preserve. The two length-dependent assertions are untouched and still exact at forty and thirty-two characters, because a secret is judged by its length. Two sentences elsewhere spelled a removed password out while explaining that a value reading as a placeholder still works as one - one in the Koyeb deploy notes and one in its test. The sentences are gone; the paragraph around them and the assertion they described are unchanged. Ten tests before, ten after, ninety-nine assertions in both. --- deploy/koyeb/README.md | 5 +- tests/unit/koyeb-deploy-button.test.ts | 5 +- tests/unit/published-credentials.test.ts | 100 +++++++++++++++-------- 3 files changed, 69 insertions(+), 41 deletions(-) diff --git a/deploy/koyeb/README.md b/deploy/koyeb/README.md index 4499adcbf..d5eca4add 100644 --- a/deploy/koyeb/README.md +++ b/deploy/koyeb/README.md @@ -38,10 +38,7 @@ by hand. URL-encode every special character (`@` → `%40`, `:` → `%3A`, `JWT_SECRET is too short` instead of coming up on a secret that is printed in a public README. Keep it that way — a placeholder that clears the minimum is a published working secret. -- **No prefilled passwords.** The button carries none. A placeholder password is - still a password: `a-placeholder-shaped-value` was the previous spelling and it signed - in, so a deploy where only the administrator field was replaced left the - standard user account open on a credential anyone could read here. The two +- **No prefilled passwords.** The button carries none. The two fields behave differently when unset, and both answers are safe ones: `ADMIN_PASSWORD` is generated on first run and printed to the Koyeb runtime log, the same as a bare `docker run`; `USER_PASSWORD` is never generated, and diff --git a/tests/unit/koyeb-deploy-button.test.ts b/tests/unit/koyeb-deploy-button.test.ts index 9619a3a52..cdc65dd19 100644 --- a/tests/unit/koyeb-deploy-button.test.ts +++ b/tests/unit/koyeb-deploy-button.test.ts @@ -43,10 +43,7 @@ describe("the Koyeb deploy button", () => { test("prefills no password at all", () => { const env = koyebEnv(); - // A placeholder password is still a password. `a-placeholder-shaped-value` was the previous - // spelling and it SIGNED IN: measured against a running container, the standard user - // account accepted it, and a deploy where only the administrator field was replaced - // left an account open on a credential published in this file. There is no placeholder + // A placeholder password is still a password. There is no placeholder // that fixes that, so the button carries neither. Unset, the two behave differently and // both answers are safe: `ADMIN_PASSWORD` is generated on first run and printed to the // log, while `USER_PASSWORD` is never generated — `getAuthUsers` adds that account only diff --git a/tests/unit/published-credentials.test.ts b/tests/unit/published-credentials.test.ts index 8a7196c6e..12cdb979d 100644 --- a/tests/unit/published-credentials.test.ts +++ b/tests/unit/published-credentials.test.ts @@ -4,10 +4,14 @@ import { join } from "node:path"; import { JWT_SECRET_MIN_LENGTH } from "@/lib/config/auth-env"; // A copy-and-run example that carries a password IS a published credential, whatever the -// value is called. `a-placeholder-shaped-value` read as a placeholder and signed in; example-not-a-real-password -// was repeated underneath as the login to use; change-me-to-a-random-32-char-string is 36 -// characters, so it clears the minimum and the server accepts it - and with -// STORAGE_ENCRYPTION_KEY unset it is also what saved connection passwords are sealed with. +// value is called. Of the three removed by hand, the first told the reader in its own name +// to set a real password and signed in as it stood; the second was the admin login, written +// underneath as the one to use; the third was a change-me placeholder 36 characters long, +// so it cleared the minimum and the server accepted it - and with STORAGE_ENCRYPTION_KEY +// unset that one is also what saved connection passwords are sealed with. +// +// None of the three is spelled out here, and no fixture below is a value anyone could sign +// in with: every fixture in this file names itself as an example on sight. // // The rule these files follow instead: set no password, and say where the generated one is // printed. This guard exists because each of those three was removed by hand and nothing @@ -448,14 +452,20 @@ describe("the documentation publishes no credential that works", () => { // line 1685 and the fetch() body at line 1769. The second is a JavaScript object literal // - bare key, single quotes - and a rule that required double quotes read the first and // walked past the second, which is how that file published two logins and reported one. - expect(caught(` -d '{"email": "admin@libredb.org", "password": "example-fake-login"}' \\`)).toEqual(["example-fake-login"]); - expect(caught(` body: JSON.stringify({ email: 'admin@libredb.org', password: 'example-fake-login' }),`)).toEqual([ + expect(caught(` -d '{"email": "admin@libredb.org", "password": "example-fake-login"}' \\`)).toEqual([ "example-fake-login", ]); + expect(caught(` body: JSON.stringify({ email: 'admin@libredb.org', password: 'example-fake-login' }),`)).toEqual( + ["example-fake-login"], + ); // The same fetch() body written as JSON throughout, which is the other way it gets typed. - expect(caught(`body: JSON.stringify({ "email": "a@b.c", "password": "example-not-a-real-password" })`)).toEqual(["example-not-a-real-password"]); + expect(caught(`body: JSON.stringify({ "email": "a@b.c", "password": "example-not-a-real-password" })`)).toEqual([ + "example-not-a-real-password", + ]); // The password before the email reads the same way, in either quoting. - expect(caught(`{\n "password": "example-fake-login",\n "email": "admin@libredb.org"\n}`)).toEqual(["example-fake-login"]); + expect(caught(`{\n "password": "example-fake-login",\n "email": "admin@libredb.org"\n}`)).toEqual([ + "example-fake-login", + ]); expect(caught(`{ password: 'example-fake-login', email: 'admin@libredb.org' }`)).toEqual(["example-fake-login"]); // An unquoted value is an expression, not a literal: this is what docs/API_DOCS.md:1773 @@ -498,14 +508,22 @@ describe("the documentation publishes no credential that works", () => { test("reads a value that is not the last thing on its line, and still not prose", () => { const caught = (text: string, name: string) => assignments(text, name); // The shapes that got past the old "value must end the line" rule. - expect(caught("docker run -e ADMIN_PASSWORD=example-fake-password -e HOSTNAME=db \\", "ADMIN_PASSWORD")).toEqual(["example-fake-password"]); - expect(caught(" ADMIN_PASSWORD: example-fake-password # the login", "ADMIN_PASSWORD")).toEqual(["example-fake-password"]); - expect(caught("| `ADMIN_PASSWORD=example-fake-password` | the admin login |", "ADMIN_PASSWORD")).toEqual(["example-fake-password"]); + expect(caught("docker run -e ADMIN_PASSWORD=example-fake-password -e HOSTNAME=db \\", "ADMIN_PASSWORD")).toEqual([ + "example-fake-password", + ]); + expect(caught(" ADMIN_PASSWORD: example-fake-password # the login", "ADMIN_PASSWORD")).toEqual([ + "example-fake-password", + ]); + expect(caught("| `ADMIN_PASSWORD=example-fake-password` | the admin login |", "ADMIN_PASSWORD")).toEqual([ + "example-fake-password", + ]); expect(caught(" adminPassword: example-fake-password", "adminPassword")).toEqual(["example-fake-password"]); - expect(caught(" - key: ADMIN_PASSWORD\n value: example-fake-password", "ADMIN_PASSWORD")).toEqual(["example-fake-password"]); - expect(caught(" - name: ADMIN_PASSWORD\n value: example-fake-password", "ADMIN_PASSWORD")).toEqual([ + expect(caught(" - key: ADMIN_PASSWORD\n value: example-fake-password", "ADMIN_PASSWORD")).toEqual([ "example-fake-password", ]); + expect( + caught(" - name: ADMIN_PASSWORD\n value: example-fake-password", "ADMIN_PASSWORD"), + ).toEqual(["example-fake-password"]); // ...and the shapes that must stay quiet, or a maintainer deletes this guard. expect(caught("ADMIN_PASSWORD: generated on first run", "ADMIN_PASSWORD")).toEqual([]); expect(caught(' adminPassword: "{{ .Values.secrets.adminPassword }}"', "adminPassword")).toEqual([]); @@ -534,14 +552,22 @@ describe("the documentation publishes no credential that works", () => { expect(caught("| `ADMIN_PASSWORD` | `example-not-a-real-password` | the admin login |", "ADMIN_PASSWORD")).toEqual([ "example-not-a-real-password", ]); - expect(caught("| ADMIN_PASSWORD | `example-not-a-real-password` |", "ADMIN_PASSWORD")).toEqual(["example-not-a-real-password"]); + expect(caught("| ADMIN_PASSWORD | `example-not-a-real-password` |", "ADMIN_PASSWORD")).toEqual([ + "example-not-a-real-password", + ]); // 2. A shell line that carries on after the value. - expect(caught("export ADMIN_PASSWORD=example-not-a-real-password && echo ok", "ADMIN_PASSWORD")).toEqual(["example-not-a-real-password"]); + expect(caught("export ADMIN_PASSWORD=example-not-a-real-password && echo ok", "ADMIN_PASSWORD")).toEqual([ + "example-not-a-real-password", + ]); // 3. A value with a space in it, which used to be read as one word plus prose. - expect(caught('ADMIN_PASSWORD="example fake admin password"', "ADMIN_PASSWORD")).toEqual(["example fake admin password"]); - expect(caught("USER_PASSWORD='example fake user password'", "USER_PASSWORD")).toEqual(["example fake user password"]); + expect(caught('ADMIN_PASSWORD="example fake admin password"', "ADMIN_PASSWORD")).toEqual([ + "example fake admin password", + ]); + expect(caught("USER_PASSWORD='example fake user password'", "USER_PASSWORD")).toEqual([ + "example fake user password", + ]); // The same hole let a secret through, and a secret is judged by its LENGTH, so reading // one word of it hid a value the server would have accepted. const secret = caught('JWT_SECRET="an example fake secret of forty chars xx"', "JWT_SECRET"); @@ -549,9 +575,9 @@ describe("the documentation publishes no credential that works", () => { expect(secret[0].length).toBeGreaterThanOrEqual(JWT_SECRET_MIN_LENGTH); // 4. `docker run` with the image name after the value, rather than another flag. - expect(caught("docker run -e ADMIN_PASSWORD=example-not-a-real-password libredb/libredb-studio", "ADMIN_PASSWORD")).toEqual([ - "example-not-a-real-password", - ]); + expect( + caught("docker run -e ADMIN_PASSWORD=example-not-a-real-password libredb/libredb-studio", "ADMIN_PASSWORD"), + ).toEqual(["example-not-a-real-password"]); }); test("reads a value a table hides past the cell after the name", () => { @@ -561,25 +587,31 @@ describe("the documentation publishes no credential that works", () => { // The cell after the name is a tick or a cross, and the value goes inside the description // - which is where the ADMIN_EMAIL row directly above already writes its own default. const required = "| Variable | Required | Description |\n|----------|----------|-------------|\n"; - expect(caught(required + "| `ADMIN_PASSWORD` | Yes | Admin password (default: `example-not-a-real-password`) |")).toEqual([ - "example-not-a-real-password", - ]); - expect(caught(required + "| `ADMIN_PASSWORD` | No | Admin password, defaults to `example-not-a-real-password` |")).toEqual([ + expect( + caught(required + "| `ADMIN_PASSWORD` | Yes | Admin password (default: `example-not-a-real-password`) |"), + ).toEqual(["example-not-a-real-password"]); + expect( + caught(required + "| `ADMIN_PASSWORD` | No | Admin password, defaults to `example-not-a-real-password` |"), + ).toEqual(["example-not-a-real-password"]); + expect( + caught(required + "| `ADMIN_PASSWORD` | No | Admin password (default: example-not-a-real-password) |"), + ).toEqual(["example-not-a-real-password"]); + expect( + caught(required + "| `ADMIN_PASSWORD` | No | Admin password (default: **example-not-a-real-password**) |"), + ).toEqual(["example-not-a-real-password"]); + + // A column the header calls a value is one, wherever it sits and however it is marked up. + const valued = "| Variable | Value | Description |\n|---|---|---|\n"; + expect(caught(valued + "| `ADMIN_PASSWORD` | `example-not-a-real-password` | the admin login |")).toEqual([ "example-not-a-real-password", ]); - expect(caught(required + "| `ADMIN_PASSWORD` | No | Admin password (default: example-not-a-real-password) |")).toEqual([ + expect(caught(valued + "| `ADMIN_PASSWORD` | example-not-a-real-password | the admin login |")).toEqual([ "example-not-a-real-password", ]); - expect(caught(required + "| `ADMIN_PASSWORD` | No | Admin password (default: **example-not-a-real-password**) |")).toEqual([ + expect(caught(valued + "| `ADMIN_PASSWORD` | **example-not-a-real-password** | the admin login |")).toEqual([ "example-not-a-real-password", ]); - // A column the header calls a value is one, wherever it sits and however it is marked up. - const valued = "| Variable | Value | Description |\n|---|---|---|\n"; - expect(caught(valued + "| `ADMIN_PASSWORD` | `example-not-a-real-password` | the admin login |")).toEqual(["example-not-a-real-password"]); - expect(caught(valued + "| `ADMIN_PASSWORD` | example-not-a-real-password | the admin login |")).toEqual(["example-not-a-real-password"]); - expect(caught(valued + "| `ADMIN_PASSWORD` | **example-not-a-real-password** | the admin login |")).toEqual(["example-not-a-real-password"]); - // A secret is judged by its length, so a table that hides one is the same hole twice. const table = "| Variable | Default |\n|---|---|\n| `JWT_SECRET` | `example-fake-secret-not-a-real-x` |"; const secret = caught(table, "JWT_SECRET"); @@ -592,7 +624,9 @@ describe("the documentation publishes no credential that works", () => { expect(caught(notes + "| `JWT_SECRET` | `example-fake-secret-not-a-real-x` |", "JWT_SECRET")).toEqual([ "example-fake-secret-not-a-real-x", ]); - expect(caught(notes + "| `ADMIN_PASSWORD` | **example-not-a-real-password** |")).toEqual(["example-not-a-real-password"]); + expect(caught(notes + "| `ADMIN_PASSWORD` | **example-not-a-real-password** |")).toEqual([ + "example-not-a-real-password", + ]); // With no header row the row is a fragment, and the cell after the name is all there is. expect(caught("| `ADMIN_PASSWORD` | example-not-a-real-password |")).toEqual(["example-not-a-real-password"]); From f989a8517031a11aeba38f7d153de88ba9c8c5cd Mon Sep 17 00:00:00 2001 From: Yusuf GUNDOGDU Date: Sat, 19 Sep 2026 00:07:32 +0300 Subject: [PATCH 19/19] style: rewrap one assertion after the fixture rename The renamed fixture is a different width, so the line it sits on no longer fits the formatter's wrap. Content unchanged. --- tests/unit/published-credentials.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/unit/published-credentials.test.ts b/tests/unit/published-credentials.test.ts index 12cdb979d..f24a05f8a 100644 --- a/tests/unit/published-credentials.test.ts +++ b/tests/unit/published-credentials.test.ts @@ -477,7 +477,9 @@ describe("the documentation publishes no credential that works", () => { // A CONNECTION body is the reader's own database, not an account this project ships. expect(caught(`{"host": "127.0.0.1", "user": "postgres", "password": "postgres"}`)).toEqual([]); expect(caught(`{ host: 'h', user: 'postgres', password: 'postgres' }`)).toEqual([]); - expect(caught(`{"host": "h", "port": 8091, "user": "Administrator", "password": "example-fake-connection-pw"}`)).toEqual([]); + expect( + caught(`{"host": "h", "port": 8091, "user": "Administrator", "password": "example-fake-connection-pw"}`), + ).toEqual([]); // And the stand-ins, or every API table in docs/ fails this guard. expect(caught(`{"email": "a@b.c", "password": "string"}`)).toEqual([]);