From 6f6577b03ba8c64b8e538036103d193b71d8ca3f Mon Sep 17 00:00:00 2001 From: cevheri Date: Wed, 7 Oct 2026 11:02:30 +0300 Subject: [PATCH 01/46] fix(db): name an unnamed or repeated result column so every column keeps its value Add uniqueFieldNames beside unionFields: a column a driver declares with no name is named "(No column name)", and a repeated name is numbered "name (2)", never taking a name the result itself declares. QueryResult.fields now states the invariant the providers keep: non-empty, unique, and the keys rows use. --- src/lib/db/utils/result-fields.ts | 33 +++++++++++++++++++++ src/lib/types.ts | 7 +++++ tests/unit/db/result-fields.test.ts | 46 ++++++++++++++++++++++++++++- 3 files changed, 85 insertions(+), 1 deletion(-) diff --git a/src/lib/db/utils/result-fields.ts b/src/lib/db/utils/result-fields.ts index aed324c72..ac6296936 100644 --- a/src/lib/db/utils/result-fields.ts +++ b/src/lib/db/utils/result-fields.ts @@ -19,3 +19,36 @@ export function unionFields(rows: readonly object[]): string[] { } return [...fields]; } + +/** The name a column the driver declares with no name is shown and keyed under, as SQL Server's own tools word it. */ +export const UNNAMED_FIELD = "(No column name)"; + +/** + * The names a result's columns are keyed under, one per declared column, in order: each non-empty and + * different from every other, which is what `QueryResult.fields` promises. + * + * A driver may declare a column with no name (SQL Server leaves every unaliased expression unnamed) or two + * columns with one name (a join that projects `id` from both tables). A row keyed by name then loses a value, + * or carries an empty key no grid column can take. So a column with no name is named `UNNAMED_FIELD`, and a + * repeat is numbered `name (2)`, `name (3)`, as the Db2, Druid, Trino and search transports already number + * theirs. A number never produces a name the result itself declares, before or after the repeat, so no column + * the statement named is shown under another column's value. + * + * Read the rows positionally and key them by these names; reading them keyed by the declared names is what + * loses the value in the first place. Names that differ only in letter case stay apart, as row keys do. + * + * Server only, and it names no engine. + */ +export function uniqueFieldNames(declared: readonly string[]): string[] { + const declaredNames = new Set(declared); + const given = new Set(); + return declared.map((name) => { + const base = name === "" ? UNNAMED_FIELD : name; + // A column keeps its own declared name the first time; any other name must be one nothing declares. + const free = (candidate: string) => !given.has(candidate) && (candidate === name || !declaredNames.has(candidate)); + let unique = base; + for (let repeat = 2; !free(unique); repeat += 1) unique = `${base} (${repeat})`; + given.add(unique); + return unique; + }); +} diff --git a/src/lib/types.ts b/src/lib/types.ts index 06e3547b0..95cb1c35a 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -589,6 +589,12 @@ export interface AgentChartSpec { export interface QueryResult { rows: Record[]; + /** + * The result's columns, in order. Every name is non-empty and no two are the same, and rows key their + * values by these names, so no two columns read one value. + * A provider whose driver declares a column with no name or two columns with one name reads its rows + * positionally and keys them by `uniqueFieldNames` (`src/lib/db/utils/result-fields.ts`). + */ fields: string[]; rowCount: number; executionTime: number; @@ -638,6 +644,7 @@ export interface QueryResult { /** One result set of a text that produced several (`QueryResult.resultSets`). */ export interface QueryResultSet { rows: Record[]; + /** As `QueryResult.fields`: non-empty, unique, and the keys every row carries. */ fields: string[]; columnTypes?: Record; } diff --git a/tests/unit/db/result-fields.test.ts b/tests/unit/db/result-fields.test.ts index d4a11b4de..7a6d7f00b 100644 --- a/tests/unit/db/result-fields.test.ts +++ b/tests/unit/db/result-fields.test.ts @@ -1,9 +1,53 @@ /** * The columns of a result whose rows do not share one shape: a document store answers * documents, and two documents of one collection need not carry the same keys. + * And the names a result's columns are keyed under when a driver declares a column with no + * name, or two columns with one name. */ import { describe, expect, test } from "bun:test"; -import { unionFields } from "@/lib/db/utils/result-fields"; +import { UNNAMED_FIELD, unionFields, uniqueFieldNames } from "@/lib/db/utils/result-fields"; + +describe("uniqueFieldNames", () => { + test("names that are already unique and non-empty are kept as declared, in order", () => { + expect(uniqueFieldNames(["id", "name", "ID"])).toEqual(["id", "name", "ID"]); + expect(uniqueFieldNames([])).toEqual([]); + }); + + test("a column with no name is named the way SQL Server's own tools name it", () => { + // `SELECT @@VERSION` or `SELECT COUNT(*) FROM t` on SQL Server: the driver declares the + // column with an empty name, and a row keyed by "" crashed the results grid. + expect(UNNAMED_FIELD).toBe("(No column name)"); + expect(uniqueFieldNames([""])).toEqual(["(No column name)"]); + }); + + test("a repeated name is numbered from 2, so every value keeps a column of its own", () => { + // A join that projects `id` from both tables: keyed by name, the second value replaced the first. + expect(uniqueFieldNames(["id", "customer_id", "item", "id", "name"])).toEqual([ + "id", + "customer_id", + "item", + "id (2)", + "name", + ]); + expect(uniqueFieldNames(["?column?", "?column?", "?column?"])).toEqual([ + "?column?", + "?column? (2)", + "?column? (3)", + ]); + expect(uniqueFieldNames(["", ""])).toEqual(["(No column name)", "(No column name) (2)"]); + }); + + test("a generated name never takes a name the result itself declares", () => { + // The statement may already use the spelling a number would produce, before or after the repeat. + expect(uniqueFieldNames(["a", "a (2)", "a"])).toEqual(["a", "a (2)", "a (3)"]); + expect(uniqueFieldNames(["a", "a", "a (2)"])).toEqual(["a", "a (3)", "a (2)"]); + expect(uniqueFieldNames(["", "(No column name)"])).toEqual(["(No column name) (2)", "(No column name)"]); + }); + + test("names differing only in letter case are different columns, as row keys are", () => { + expect(uniqueFieldNames(["Id", "id", "id"])).toEqual(["Id", "id", "id (2)"]); + }); +}); describe("unionFields", () => { test("no rows, no columns", () => { From 3708a1465d30e0c7aa895a5d73ad4e6138858529 Mon Sep 17 00:00:00 2001 From: cevheri Date: Wed, 7 Oct 2026 11:18:29 +0300 Subject: [PATCH 02/46] fix(mssql): read user results positionally so unnamed and repeated columns keep their values query(), queryReadOnly() and queryInTransaction() now ask the driver for array row mode on the request that runs the user's statement, name the columns with uniqueFieldNames, and key rows, columnTypes and the zoneless value conversion by those names. SELECT @@VERSION answered fields [""], which crashed the results grid, and SELECT 1 AS a, 2 AS a merged both values into one column. FOR JSON and FOR XML stay one column holding the whole text, a row whose value count differs from the column count is refused, and the provider's own catalog and admission reads keep object rows. The transaction path now names a zero-row result's columns, which closes the mssql bullet of BACKLOG X9. --- docs/BACKLOG.md | 9 +- docs/providers/mssql.md | 37 ++- src/lib/db/providers/sql/mssql.ts | 171 +++++++---- tests/integration/db/mssql-provider.test.ts | 324 +++++++++++++++++++- 4 files changed, 469 insertions(+), 72 deletions(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index e75389560..601472665 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -2743,8 +2743,8 @@ Neither `StudioModals`, `StudioOverlays` nor the modals are memoized. ### X9. What `columnTypes` still cannot name, measured 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: +SQLite joined them on 2026-09-18 by reading its own declarations through the driver bridge. Three bounds +measured while doing it remain open, 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 compiled into the server and never reused), so an enum, a composite or an extension type falls @@ -2760,11 +2760,6 @@ measured while doing it, and each is a small residue rather than a defect: hands a bit string back as the string `"1010"` while `mysql2` hands back a Buffer, so the same declared name needs the text family on one engine and the binary family on the other. One name, two answers, which is why it was left alone. -- **The mssql transaction path declares types for columns `fields` does not list.** `queryInTransaction` - takes `fields` from `Object.keys(recordset[0])`, so a zero-row result has no fields while its - `recordset.columns` (which does carry the declaration, even for zero rows - measured) fills - `columnTypes`. Harmless today because all three consumers iterate `fields`; taking `fields` from - `columns` too would be the right fix and is a behaviour change of its own. **Done when:** each bound is closed or judged settled, with the enum case the only one a user is likely to meet. diff --git a/docs/providers/mssql.md b/docs/providers/mssql.md index bbbbda1a8..d9063b8bf 100644 --- a/docs/providers/mssql.md +++ b/docs/providers/mssql.md @@ -203,7 +203,7 @@ schema; it is deleted. Row counts still come from `SUM(sys.partitions.rows)`. ### 3.4 `rowsAffected` is surfaced Unlike the MySQL/Oracle providers (which report `rows.length`), `query()` sets -`rowCount = result.rowsAffected?.[0] ?? recordset.length` ([`mssql.ts`](../../src/lib/db/providers/sql/mssql.ts)), +`rowCount = result.rowsAffected?.[0] ?? rows.length` ([`mssql.ts`](../../src/lib/db/providers/sql/mssql.ts)), and `queryInTransaction()` repeats the same expression, so a non-`SELECT` statement returns its real affected-row count. @@ -346,9 +346,30 @@ to the newer name would change the wording on the form without changing a single `@p1`, `@p2`, … via `request.input()`, runs the query, and returns: ```ts -{ rows: recordset, fields, rowCount: rowsAffected[0] ?? recordset.length, executionTime, columnTypes? } +{ rows, fields, rowCount: rowsAffected[0] ?? rows.length, executionTime, columnTypes?, resultSets? } ``` +**Rows are read by position, so every column keeps its own name and value.** +`query()`, `queryReadOnly()` and `queryInTransaction()` set `request.arrayRowMode = true` on the request that runs the user's statement, and only there: the provider's own catalog, monitoring and admission reads keep the driver's object rows, because they read columns the provider itself named. +In array row mode the driver answers each row as its values in column order and the recordset's `columns` as a list, so `fields` is `uniqueFieldNames` ([`result-fields.ts`](../../src/lib/db/utils/result-fields.ts)) over the declared names and each row is keyed by those names, position by position. +SQL Server leaves every unaliased expression unnamed, and such a column is named `(No column name)`, as SQL Server's own tools word it; a repeated name is numbered `name (2)`, `name (3)`. +A row whose value count differs from the column count, or a result set without its column list, is refused with a `QueryError` rather than shifted or named by guess. +Measured 2026-10-07 on SQL Server 2022 RTM-CU27 (16.0.4295.3) through `mssql` 12.7.2, through `query()`: + +| statement | in object row mode, before | `fields` now | row now | +|---|---|---|---| +| `SELECT @@VERSION` | `fields: [""]`, row `{ "": "Microsoft SQL Server 2022 …" }` | `["(No column name)"]` | `{ "(No column name)": "Microsoft SQL Server 2022 …" }` | +| `SELECT 1, 2` | one column `""`, row `{ "": [1, 2] }` | `["(No column name)", "(No column name) (2)"]` | `{ "(No column name)": 1, "(No column name) (2)": 2 }` | +| `SELECT 1 AS a, 2 AS a` | one column `a`, row `{ a: [1, 2] }` | `["a", "a (2)"]` | `{ a: 1, "a (2)": 2 }` | +| `SELECT TOP 0 1 AS x, 2 AS x` | `fields: ["x"]` | `["x", "x (2)"]` | none | +| `… FOR JSON PATH` | one column, the whole text | `["JSON_F52E2B61-18A1-11d1-B105-00805F49916B"]` | that name, holding the whole JSON text | +| `… FOR XML PATH` | one column, the whole text | `["XML_F52E2B61-18A1-11d1-B105-00805F49916B"]` | that name, holding the whole XML text | + +The empty name crashed the results grid, which cannot build a column with an empty id, and its error boundary stayed failed until a page reload. +A repeated name lost every value but one: object row mode merges the repeats into one array under one key. +`FOR JSON` and `FOR XML` are the one shape the driver builds as an object even in array row mode: it joins the chunks of the text into a single row keyed by the column's position (`{ "0": text }`, `lib/tedious/request.js`), which the provider reads as the one value it is. +Each set of a multi-result batch is named on its own: `SELECT 1 AS a, 2 AS a; SELECT 3, 4 AS b` answers `fields` `["a", "a (2)"]` and a second set with `["(No column name)", "b"]`. + A text that returns several result sets still answers with its **first**, as `EXEC sp_help` always did, and carries every set in `resultSets` (#1312). `POST /api/db/query` and `POST /api/db/transaction` do not send that field; the multi-statement route reads it to show a batch's last result with rows. @@ -400,7 +421,7 @@ throw — it does **not** confirm the cancellation actually took effect. Exposed ISO instant, `CAST('10:30:00.1234567' AS time(7))` arrived as `1970-01-01T10:30:00.123Z`: a moment it does not hold, and four digits short. `query()`, `queryReadOnly()` and `queryInTransaction()` now rewrite the three declarations into the engine's text, keyed on - `recordset.columns` - the same map [§5.4](#54-declared-column-types) reads - and reconstruct + the result's column metadata by the names in `fields` - the same map [§5.4](#54-declared-column-types) reads - and reconstruct the fraction from the remainder the driver keeps on the value (`nanosecondsDelta`), so `time(7)` keeps all seven digits. `datetimeoffset` is deliberately untouched: it IS an instant. Measured 2026-09-28 on SQL Server 2022 CU27 (16.0.4295.3) through `mssql` 12.7.2 / @@ -430,11 +451,14 @@ throw — it does **not** confirm the cancellation actually took effect. Exposed ### 5.4 Declared column types -`mssql` attaches a `columns` map to the recordset, and each entry's `type` carries a `declaration` - +`mssql` attaches its column metadata to the recordset, and each entry's `type` carries a `declaration` - T-SQL's own lowercase spelling. That is passed through into `QueryResult.columnTypes` -([column-types.ts](../../src/lib/db/providers/sql/column-types.ts)) by both `query()` and -`queryInTransaction()`, keyed by the column name. `type` is a factory FUNCTION for some of the +([column-types.ts](../../src/lib/db/providers/sql/column-types.ts)) by `query()`, `queryReadOnly()` and +`queryInTransaction()`, keyed by the column's name in `fields`. `type` is a factory FUNCTION for some of the driver's types and a plain object for others; `declaration` is on both. +In the array row mode the user paths read with ([§5.1](#51-execution)) the metadata is a list in column order, so it is keyed by the unique names: `SELECT 1 AS a, N'x' AS a` declares `{ a: "int", "a (2)": "nvarchar" }`, and an unnamed column's type sits under `(No column name)`. +Object row mode kept one map entry per distinct name, so a repeated column's type was lost with its value. +A zero-row result names its columns and declares their types alike, on every path. Measured on SQL Server 2022 CU26 over the probe table: @@ -1673,6 +1697,7 @@ Integration tests live in The `mssql` module is replaced with an in-process mock via `mock.module('mssql', …)` **before** the provider is imported — there is no live SQL Server in the suite. The mock's pool/request returns canned `{ recordset, rowsAffected }` results, exercising the same code paths as the real driver. +A request the provider sets to array row mode gets the fixture converted to the driver's array shape (rows of values, a list of column metadata); a fixture that must say what only that shape can, an unnamed or repeated column, is written in it directly. > ⚠️ **Mock isolation:** `bun`'s `mock.module()` is process-wide; files mocking different drivers > would cross-contaminate if they shared one. They never do: `bun run test` gives every test file its diff --git a/src/lib/db/providers/sql/mssql.ts b/src/lib/db/providers/sql/mssql.ts index 37667c718..2837048ba 100644 --- a/src/lib/db/providers/sql/mssql.ts +++ b/src/lib/db/providers/sql/mssql.ts @@ -64,6 +64,7 @@ import { assertReadOnlyBudget, measureResultBytes } from "./read-only-budget"; import { formatBytes } from "../../utils/pool-manager"; import { analyzeQuery, DEFAULT_QUERY_LIMIT, MAX_UNLIMITED_ROWS } from "../../utils/query-limiter"; import { measuredNullableAggregate } from "../../utils/measured-aggregate"; +import { uniqueFieldNames } from "../../utils/result-fields"; import { readLeadingKeyword } from "@/lib/sql/leading-keyword"; import { resolveSqlGrammar, type SqlGrammar } from "@/lib/sql/grammar"; import { readStatementEnd } from "@/lib/sql/statement-end"; @@ -1507,27 +1508,18 @@ function timeText(value: Date, scale: number | undefined): string { /** * Rewrites, in place, every value whose declaration is one of those, into its text (#1132). * - * Keyed on `recordset.columns` - the map `mssqlColumnTypes` reads - because a value cannot - * be told from its own shape: `time` and `datetimeoffset` arrive as the same kind of - * `Date`, and only the column says which one holds an instant. A result without the map is - * left exactly as the driver built it. + * Keyed on the result's column metadata - the map `mssqlColumnTypes` reads - because a value + * cannot be told from its own shape: `time` and `datetimeoffset` arrive as the same kind of + * `Date`, and only the column says which one holds an instant. * - * All three query paths call it as the recordset is taken, before anything measures or - * shapes the result. + * All three query paths call it as the result is taken, before anything shapes it; the + * read-only path calls it after its cut check, which must see the values the server sent. */ -/** A result set's column names: its declared columns, or the first row's keys without them. */ -function mssqlFields(recordset: Record[] & { columns?: object }): string[] { - if (recordset.columns) return Object.keys(recordset.columns); - return recordset.length > 0 ? Object.keys(recordset[0]) : []; -} - -function convertZonelessValues(recordset: Record[]): void { - const columns = (recordset as { columns?: Record }).columns; - if (!columns) return; +function convertZonelessValues(rows: Record[], columns: Record): void { for (const [name, column] of Object.entries(columns)) { const declaration = (column.type as { declaration?: unknown } | undefined)?.declaration; if (typeof declaration !== "string" || !ZONELESS_VALUE_DECLARATIONS.has(declaration)) continue; - for (const row of recordset) { + for (const row of rows) { const value = row[name]; if (!(value instanceof Date)) continue; row[name] = @@ -1540,6 +1532,76 @@ function convertZonelessValues(recordset: Record[]): void { } } +/** One result set as every result path hands it on: rows keyed by `fields`, and the column metadata keyed the same. */ +interface MssqlResultSet { + rows: Record[]; + /** Non-empty and unique, which is what `QueryResult.fields` promises. */ + fields: string[]; + /** The driver's metadata for each column, keyed by its entry in `fields`. */ + columns: Record; +} + +/** A recordset as `mssql` answers it in array row mode: rows of values, and an array of column metadata hung off it. */ +type ArrayModeRecordset = unknown[] & { columns?: unknown }; + +/** + * A result set read by position, which is the only reading that keeps every column. + * + * SQL Server leaves every unaliased expression unnamed, and a join may project two columns + * with one name. In object row mode the driver keys both the column map and each row by + * that name, so `SELECT @@VERSION` came back under an empty key no grid column can take, + * and `SELECT 1 AS a, 2 AS a` as ONE column holding `[1, 2]`. In array row mode it answers + * the columns as a list and each row as its values in that order (measured on SQL Server + * 2022, docs/providers/mssql.md section 5.1), so the names come from `uniqueFieldNames` and + * each value lands under its own column. + * + * A statement that answers no result set (an UPDATE) has no recordset at all. A recordset + * without its column list, or a row whose values do not match the columns one for one, is + * refused: naming columns by guess or shifting a value into a neighbour is a wrong answer. + */ +function positionalResultSet(recordset: ArrayModeRecordset | undefined): MssqlResultSet { + if (recordset === undefined) return { rows: [], fields: [], columns: {} }; + const declared = recordset.columns; + if (!Array.isArray(declared)) { + throw new QueryError("SQL Server answered a result set without its column list", "mssql"); + } + const metadata = declared as Array; + const fields = uniqueFieldNames(metadata.map((column) => column.name)); + const rows = recordset.map((row) => { + const values = rowValues(row, fields.length); + return Object.fromEntries(fields.map((field, index) => [field, values[index]])); + }); + return { rows, fields, columns: Object.fromEntries(fields.map((field, index) => [field, metadata[index]])) }; +} + +/** + * One row's values in column order. + * + * A FOR JSON or FOR XML result is the exception the driver makes even in array row mode: it + * joins the chunks of the text into ONE object keyed by the column's position, `{ "0": text }` + * (`lib/tedious/request.js`, measured), so that single-column shape is read as the one value + * it is. + */ +function rowValues(row: unknown, width: number): unknown[] { + if (Array.isArray(row) && row.length === width) return row; + const keys = row !== null && typeof row === "object" && !Array.isArray(row) ? Object.keys(row) : []; + if (width === 1 && keys.length === 1 && keys[0] === "0") return [(row as Record)[0]]; + const count = Array.isArray(row) ? row.length : keys.length; + throw new QueryError(`SQL Server answered a row of ${count} value(s) for ${width} column(s)`, "mssql"); +} + +/** + * The query plan the read-only profile admitted a statement with, as a result set. + * + * Read in object mode, by name, because the admission reads `Parent` and `StmtId` from it. + * That loses nothing here: `SET SHOWPLAN_ALL` answers a fixed set of columns SQL Server + * itself names, each once. + */ +function planResultSet(plan: mssql.IResult): MssqlResultSet { + const recordset = plan.recordset as Record[] & { columns: Record }; + return { rows: recordset, fields: Object.keys(recordset.columns), columns: recordset.columns }; +} + // ============================================================================ // MSSQL Provider // ============================================================================ @@ -1870,6 +1932,8 @@ export class MSSQLProvider extends SQLBaseProvider { const { result, executionTime } = await this.measureExecution(async () => { try { const request = this.pool!.request(); + // Positional rows, so an unnamed or repeated column keeps its value (see positionalResultSet). + request.arrayRowMode = true; if (queryId) { this.runningRequests.set(queryId, request); @@ -1891,33 +1955,30 @@ export class MSSQLProvider extends SQLBaseProvider { } }); - const recordset = result.recordset || []; - convertZonelessValues(recordset); + const first = positionalResultSet(result.recordset); + convertZonelessValues(first.rows, first.columns); // A text with several result sets carries all of them (#1312). The editor sends a // T-SQL batch as one request, so `SELECT * FROM a; SELECT * FROM b` reaches here whole, // and the multi-statement route shows the last one with rows, as it does across a // script's statements. `rows` stays the FIRST set, which is what `EXEC sp_help` and // every caller before batches were shown. - const recordsets = (result.recordsets ?? []) as (typeof result.recordset)[]; + const recordsets = (result.recordsets ?? []) as ArrayModeRecordset[]; const resultSets = recordsets.length > 1 - ? recordsets.map((set) => { - if (set !== recordset) convertZonelessValues(set); - return { - rows: set as Record[], - fields: mssqlFields(set), - ...mssqlColumnTypes(set.columns), - }; + ? recordsets.map((recordset) => { + const set = recordset === result.recordset ? first : positionalResultSet(recordset); + if (set !== first) convertZonelessValues(set.rows, set.columns); + return { rows: set.rows, fields: set.fields, ...mssqlColumnTypes(set.columns) }; }) : undefined; return { - rows: recordset as Record[], - fields: mssqlFields(recordset), - rowCount: result.rowsAffected?.[0] ?? recordset.length, + rows: first.rows, + fields: first.fields, + rowCount: result.rowsAffected?.[0] ?? first.rows.length, executionTime, - ...mssqlColumnTypes(recordset.columns), + ...mssqlColumnTypes(first.columns), ...(resultSets && { resultSets }), }; }); @@ -2177,7 +2238,7 @@ export class MSSQLProvider extends SQLBaseProvider { // The admission compiled the statement, so the ESTIMATING plan is already in // hand: on this engine the plan a caller asks for IS the plan that admitted // the statement, which is a stronger pairing than composing a second one. - if (mode === "estimate-plan") return plan; + if (mode === "estimate-plan") return planResultSet(plan); // One more than the budget: SQL Server stops the result there, and the extra // row is what distinguishes "the statement returned exactly the budget" from @@ -2201,9 +2262,10 @@ export class MSSQLProvider extends SQLBaseProvider { // byte budget, so the refusal below fires for every truncation there can be. const valueCeiling = MSSQLProvider.sessionCeiling(budget.maxResultBytes); await new mssql.Request(transaction).batch(`SET TEXTSIZE ${valueCeiling}`); - const executed = await this.runWithDeadline(transaction, sql, budget.statementTimeoutMs); - MSSQLProvider.assertNoValueWasCut(executed, valueCeiling, sql); - return executed; + const executed = await this.runWithDeadline(transaction, sql, budget.statementTimeoutMs, true); + const read = positionalResultSet(executed.recordset); + MSSQLProvider.assertNoValueWasCut(read.rows, valueCeiling, sql); + return read; } catch (error) { throw error instanceof QueryError ? error : mapDatabaseError(error, "mssql", sql); } finally { @@ -2227,8 +2289,8 @@ export class MSSQLProvider extends SQLBaseProvider { } }); - const recordset = result.recordset || []; - convertZonelessValues(recordset); + const { rows, fields, columns } = result; + convertZonelessValues(rows, columns); // The ROW budget bounds DATA rows, and a plan's rows are not data: they are the // optimizer's nodes, one per operator. Measured on AdventureWorks2022, an ordinary // `SELECT TOP 10 *` over one shipped view compiles to 170 of them and the same view @@ -2242,14 +2304,14 @@ export class MSSQLProvider extends SQLBaseProvider { // output at all (340 rows with `SET ROWCOUNT 50`), so a plan is bounded by the // statement's own complexity and then by the byte cap below, which is exactly how // PostgreSQL's one-row plan is bounded. - if (mode !== "estimate-plan" && recordset.length > budget.maxResultRows) { + if (mode !== "estimate-plan" && rows.length > budget.maxResultRows) { throw new QueryError( - `Read-only execution exceeded the row budget: ${recordset.length} rows > ${budget.maxResultRows} allowed`, + `Read-only execution exceeded the row budget: ${rows.length} rows > ${budget.maxResultRows} allowed`, "mssql", sql, ); } - const resultBytes = measureResultBytes(recordset as unknown[]); + const resultBytes = measureResultBytes(rows); if (resultBytes > budget.maxResultBytes) { throw new QueryError( mode === "estimate-plan" @@ -2260,18 +2322,12 @@ export class MSSQLProvider extends SQLBaseProvider { ); } - const fields = recordset.columns - ? Object.keys(recordset.columns) - : recordset.length > 0 - ? Object.keys(recordset[0]) - : []; - return { - rows: recordset as Record[], + rows, fields, - rowCount: recordset.length, + rowCount: rows.length, executionTime, - ...mssqlColumnTypes(recordset.columns), + ...mssqlColumnTypes(columns), }; }); } @@ -2355,9 +2411,9 @@ export class MSSQLProvider extends SQLBaseProvider { * unlikely and would be refused fail-closed, which is the right way round for a check * whose alternative is a wrong answer nobody can detect. */ - private static assertNoValueWasCut(result: mssql.IResult, ceiling: number, sql: string): void { + private static assertNoValueWasCut(rows: Record[], ceiling: number, sql: string): void { const unicodeCut = Math.floor(ceiling / 2); - for (const row of (result.recordset ?? []) as Record[]) { + for (const row of rows) { for (const [column, value] of Object.entries(row)) { const size = typeof value === "string" ? value.length : Buffer.isBuffer(value) ? value.length : -1; if (size !== ceiling && size !== unicodeCut) continue; @@ -2414,8 +2470,11 @@ export class MSSQLProvider extends SQLBaseProvider { transaction: mssql.Transaction, sql: string, deadlineMs: number, + arrayRowMode = false, ): Promise { const request = new mssql.Request(transaction); + // The execution reads its rows positionally; the admission reads its plan by name. + request.arrayRowMode = arrayRowMode; // The timer records that IT fired, because tedious words every cancel the same way: // a statement this deadline ended arrives as `Canceled.`, which is also what a user // pressing stop produces, and a model told only that is given nothing to repair. The @@ -2672,6 +2731,7 @@ export class MSSQLProvider extends SQLBaseProvider { const { result, executionTime } = await this.measureExecution(async () => { try { const request = new mssql.Request(this.txTransaction!); + request.arrayRowMode = true; if (params && params.length > 0) { params.forEach((p, i) => { request.input(`p${i + 1}`, p); @@ -2683,16 +2743,15 @@ export class MSSQLProvider extends SQLBaseProvider { } }); - const recordset = result.recordset || []; - convertZonelessValues(recordset); - const fields = recordset.length > 0 ? Object.keys(recordset[0]) : []; + const { rows, fields, columns } = positionalResultSet(result.recordset); + convertZonelessValues(rows, columns); return { - rows: recordset as Record[], + rows, fields, - rowCount: result.rowsAffected?.[0] ?? recordset.length, + rowCount: result.rowsAffected?.[0] ?? rows.length, executionTime, - ...mssqlColumnTypes(recordset.columns), + ...mssqlColumnTypes(columns), }; }); } diff --git a/tests/integration/db/mssql-provider.test.ts b/tests/integration/db/mssql-provider.test.ts index c0977586a..eff05f77a 100644 --- a/tests/integration/db/mssql-provider.test.ts +++ b/tests/integration/db/mssql-provider.test.ts @@ -58,7 +58,63 @@ let mockTransactionHooks: { begin(): void; commit(): void; rollback(): void } = rollback() {}, }; +/** One column of a result the way `mssql` describes it in array row mode. */ +type ArrayModeColumn = { index: number; name: string; type?: unknown; scale?: number }; + +/** + * What `mssql` answers when the request asked for array row mode, built from a fixture + * written the object way. + * + * In array row mode the driver hands each row back as an array of values in column order + * and hangs an ARRAY of column metadata off the recordset, where object mode keys both by + * column name (measured on SQL Server 2022, the probe in docs/providers/mssql.md section 5.1). + * Most fixtures here describe a result by name because that is the readable way to write + * one, so a set whose rows are objects is converted: its columns from the fixture's + * `columns` map, or from the first row's keys when the fixture gives none, and each row's + * values in that order. A set whose rows are already arrays, or that carries a column ARRAY, + * is passed through untouched: those are the fixtures that model what only the array shape + * can say, an unnamed column or two columns with one name. + * + * `recordset` stays the same object as `recordsets[0]`, as it is in the driver. + */ +function asArrayRowModeAnswer(answer: unknown): unknown { + if (answer === null || typeof answer !== "object") return answer; + const result = answer as { recordset?: unknown; recordsets?: unknown[] }; + const converted = new Map(); + const convert = (set: unknown) => { + if (!Array.isArray(set)) return set; + const declared = (set as { columns?: unknown }).columns; + if (Array.isArray(declared) || set.some((row) => Array.isArray(row))) return set; + if (!converted.has(set)) { + const columns: ArrayModeColumn[] = declared + ? Object.entries(declared as Record).map(([name, column], index) => + Object.assign({}, column, { index, name }), + ) + : Object.keys((set[0] as object | undefined) ?? {}).map((name, index) => ({ index, name })); + const rows = (set as Record[]).map((row) => columns.map((column) => row[column.name])); + converted.set(set, Object.assign(rows, { columns })); + } + return converted.get(set); + }; + return { + ...result, + ...("recordset" in result && { recordset: convert(result.recordset) }), + ...(result.recordsets && { recordsets: result.recordsets.map(convert) }), + }; +} + +/** A recordset the way `mssql` builds one in array row mode, for the shapes a name cannot carry. */ +function arrayModeSet(columns: Array>, rows: unknown[][]) { + return Object.assign(rows, { + columns: columns.map((column, index) => + typeof column === "string" ? { index, name: column } : { ...column, index }, + ), + }); +} + class MockRequest { + /** Set by the provider per request; `null` is the driver's "take the pool's setting", which is object mode here. */ + arrayRowMode: boolean | null = null; /** The Transaction this request was made on, `undefined` for a pool request. */ readonly boundTo: unknown; /** What THIS request bound, as `mssql` hands it to the driver. */ @@ -77,11 +133,13 @@ class MockRequest { } async query(sql: string) { - return mockQueryFn(sql, this.inputs); + const answer = await mockQueryFn(sql, this.inputs); + return this.arrayRowMode ? asArrayRowModeAnswer(answer) : answer; } async batch(sql: string) { - return mockBatchFn(sql, this); + const answer = await mockBatchFn(sql, this); + return this.arrayRowMode ? asArrayRowModeAnswer(answer) : answer; } /** How an in-flight batch learns it was cancelled, which is what `cancel()` does here. */ @@ -2322,6 +2380,219 @@ describe("MSSQLProvider zoneless value types (#1132)", () => { }); }); +// --------------------------------------------------------------------------- +// Result column names: unnamed and repeated columns +// --------------------------------------------------------------------------- + +/** + * SQL Server leaves every unaliased expression unnamed, and a join may project two columns + * with one name. Keyed by name, as the driver's object row mode keys them, `SELECT @@VERSION` + * answered `fields: [""]` (a grid column no table can take), `SELECT 1, 2` one column + * holding `[1, 2]`, and `SELECT 1 AS a, 2 AS a` the same. Each fixture below is the shape the + * driver answers in array row mode, measured on SQL Server 2022 (docs/providers/mssql.md + * section 5.1): the column list carries the empty or repeated name, and the row carries + * every value in column order. + */ +describe("MSSQLProvider result column names", () => { + let provider: MSSQLProvider; + /** The requests the provider made for user statements, so a test can see the mode it asked for. */ + let requested: Array; + + const VERSION = "Microsoft SQL Server 2022 (RTM-CU27) (KB5104824) - 16.0.4295.3 (X64)"; + const JSON_COLUMN = "JSON_F52E2B61-18A1-11d1-B105-00805F49916B"; + + function answer(...sets: unknown[][]) { + return { recordset: sets[0], recordsets: sets, rowsAffected: sets.map((set) => set.length) }; + } + + /** Answers every user statement with `sets`, and records whether it asked for array row mode. */ + function answerWith(...sets: unknown[][]) { + mockQueryFn = async () => answer(...sets); + } + + beforeEach(() => { + capturedInputs = []; + cancelShouldThrow = false; + requested = []; + provider = new MSSQLProvider(baseConfig); + const query = MockRequest.prototype.query; + spyOn(MockRequest.prototype, "query").mockImplementation(function (this: MockRequest, sql: string) { + requested.push(this.arrayRowMode); + return query.call(this, sql); + }); + }); + + afterEach(async () => { + mock.restore(); + mockQueryFn = async (sql: string) => defaultQuery(sql); + try { + await provider.disconnect(); + } catch { + /* ignore */ + } + }); + + test("an unnamed column is named (No column name) and keeps its value", async () => { + answerWith(arrayModeSet([{ name: "", type: { declaration: "nvarchar" } }], [[VERSION]])); + await provider.connect(); + requested = []; + + const result = await provider.query("SELECT @@VERSION"); + + expect(requested).toEqual([true]); + expect(result.fields).toEqual(["(No column name)"]); + expect(result.rows).toEqual([{ "(No column name)": VERSION }]); + expect(result.columnTypes).toEqual({ "(No column name)": "nvarchar" }); + }); + + test("two unnamed columns are numbered and each keeps its own value", async () => { + answerWith(arrayModeSet(["", ""], [[1, 2]])); + await provider.connect(); + + const result = await provider.query("SELECT 1, 2"); + + expect(result.fields).toEqual(["(No column name)", "(No column name) (2)"]); + expect(result.rows).toEqual([{ "(No column name)": 1, "(No column name) (2)": 2 }]); + }); + + test("a repeated name is numbered, and the declared types are keyed by the names the rows use", async () => { + answerWith( + arrayModeSet( + [ + { name: "a", type: { declaration: "int" } }, + { name: "a", type: { declaration: "nvarchar" } }, + ], + [ + [1, "x"], + [2, "y"], + ], + ), + ); + await provider.connect(); + + const result = await provider.query("SELECT 1 AS a, N'x' AS a"); + + expect(result.fields).toEqual(["a", "a (2)"]); + expect(result.rows).toEqual([ + { a: 1, "a (2)": "x" }, + { a: 2, "a (2)": "y" }, + ]); + expect(result.columnTypes).toEqual({ a: "int", "a (2)": "nvarchar" }); + }); + + test("a zoneless value is converted under its unique name", async () => { + answerWith( + arrayModeSet( + [ + { name: "", type: { declaration: "datetime2" }, scale: 7 }, + { name: "", type: { declaration: "datetimeoffset" }, scale: 7 }, + ], + [[tediousDate(Date.UTC(2026, 9, 7, 10, 30, 0, 123), 0.0004567), new Date(Date.UTC(2026, 9, 7))]], + ), + ); + await provider.connect(); + + const result = await provider.query("SELECT SYSDATETIME(), SYSDATETIMEOFFSET()"); + + expect(result.rows[0]["(No column name)"]).toBe("2026-10-07 10:30:00.1234567"); + expect(result.rows[0]["(No column name) (2)"]).toBeInstanceOf(Date); + }); + + test("a FOR JSON result is one column holding the whole text, under the name the driver declares", async () => { + // Measured: in array row mode the driver still joins the chunks of a FOR JSON or FOR XML + // result into ONE object, keyed by the column's position rather than by its name. + const text = '[{"name":"sysrscols","object_id":3}]'; + answerWith(arrayModeSet([{ name: JSON_COLUMN, type: { declaration: "nvarchar" } }], [{ 0: text } as never])); + await provider.connect(); + + const result = await provider.query("SELECT name, object_id FROM sys.objects FOR JSON PATH"); + + expect(result.fields).toEqual([JSON_COLUMN]); + expect(result.rows).toEqual([{ [JSON_COLUMN]: text }]); + }); + + test("each result set of a batch is named and keyed on its own", async () => { + answerWith(arrayModeSet(["a", "a"], [[1, 2]]), arrayModeSet(["", "b"], [[3, 4]])); + await provider.connect(); + + const result = await provider.query("SELECT 1 AS a, 2 AS a; SELECT 3, 4 AS b"); + + expect(result.fields).toEqual(["a", "a (2)"]); + expect(result.rows).toEqual([{ a: 1, "a (2)": 2 }]); + expect(result.resultSets).toEqual([ + { rows: [{ a: 1, "a (2)": 2 }], fields: ["a", "a (2)"] }, + { rows: [{ "(No column name)": 3, b: 4 }], fields: ["(No column name)", "b"] }, + ]); + }); + + test("a statement that answers no result set answers no fields", async () => { + mockQueryFn = async () => ({ recordsets: [], rowsAffected: [3] }); + await provider.connect(); + + const result = await provider.query("UPDATE t SET a = 1"); + + expect(result).toMatchObject({ rows: [], fields: [], rowCount: 3 }); + }); + + test("a row whose value count is not the column count is refused, never shifted", async () => { + answerWith(arrayModeSet(["a", "b"], [[1]])); + await provider.connect(); + + await expect(provider.query("SELECT 1 AS a, 2 AS b")).rejects.toThrow( + "SQL Server answered a row of 1 value(s) for 2 column(s)", + ); + }); + + test("an object row that is not the driver's joined FOR JSON or FOR XML text is refused", async () => { + answerWith(arrayModeSet(["a"], [{ a: 1 } as never])); + await provider.connect(); + + await expect(provider.query("SELECT 1 AS a")).rejects.toThrow( + "SQL Server answered a row of 1 value(s) for 1 column(s)", + ); + }); + + test("a result set without the driver's column list is refused rather than named by guess", async () => { + answerWith([[1]]); + await provider.connect(); + + await expect(provider.query("SELECT 1")).rejects.toThrow( + "SQL Server answered a result set without its column list", + ); + }); + + test("queryInTransaction() names and keys its columns the same way, zero rows included", async () => { + answerWith(arrayModeSet(["", "id", "id"], [[1, 2, 3]])); + await provider.connect(); + await provider.beginTransaction(); + requested = []; + + const result = await provider.queryInTransaction("SELECT 1, o.id, c.id FROM o JOIN c ON c.id = o.c"); + + expect(requested).toEqual([true]); + expect(result.fields).toEqual(["(No column name)", "id", "id (2)"]); + expect(result.rows).toEqual([{ "(No column name)": 1, id: 2, "id (2)": 3 }]); + + // The X9 residue: a zero-row result named no fields while its types were declared. + answerWith(arrayModeSet([{ name: "u", type: { declaration: "uniqueidentifier" } }], [])); + const empty = await provider.queryInTransaction("SELECT u FROM t WHERE 1 = 0"); + + expect(empty.fields).toEqual(["u"]); + expect(empty.columnTypes).toEqual({ u: "uniqueidentifier" }); + await provider.rollbackTransaction(); + }); + + test("the provider's own catalog reads keep the driver's object rows", async () => { + await provider.connect(); + requested = []; + + await provider.getOverview(); + + expect(requested.length).toBeGreaterThan(0); + expect(requested.every((mode) => mode !== true)).toBe(true); + }); +}); + // --------------------------------------------------------------------------- // The object surface (#789) // --------------------------------------------------------------------------- @@ -4807,6 +5078,7 @@ const MEASURED_STATEMENTS: readonly MeasuredStatement[] = [ const PLANS_BY_STATEMENT = new Map(MEASURED_STATEMENTS.map((statement) => [statement.sql, statement.roots])); type ResultRow = Record; +const PLAN_COLUMNS = ["StmtText", "StmtId", "NodeId", "Parent", "Type"]; /** A `mssql` recordset: an array of rows with the driver's column metadata hung off it. */ type MockRecordset = ResultRow[] & { columns?: Record }; @@ -4956,7 +5228,8 @@ class ShowplanSession { }); } }); - return toResult(rows); + // SHOWPLAN_ALL's own columns, named by SQL Server, each once; the fixture models the five the profile reads. + return toResult(rows, Object.fromEntries(PLAN_COLUMNS.map((name) => [name, { name }]))); } async batch(sql: string, request: MockRequest) { @@ -5589,6 +5862,51 @@ describe("queryReadOnly() - the agent read-only execution profile (#328)", () => expect(result.rows[0].t).toBe("10:30:00.1234567"); }); + /** Answers the statement itself, once admitted, with `set`, and records the row mode each batch asked for. */ + function executeAnswers(set: unknown[]): Map> { + const modes = new Map>(); + mockBatchFn = async (sql: string, request: MockRequest) => { + modes.set(sql, [...(modes.get(sql) ?? []), request.arrayRowMode]); + if (sql === READ_SELECT.sql && !engine.showplanAll) { + engine.batches.push({ sql, pinned: request.boundTo === lastTransaction }); + return { recordset: set, recordsets: [set], rowsAffected: [set.length] }; + } + return engine.batch(sql, request); + }; + return modes; + } + + test("an unnamed or repeated column served under the profile is named and keeps its value", async () => { + const profiled = await openProfiled(); + const modes = executeAnswers( + arrayModeSet( + ["", { name: "id", type: { declaration: "int" } }, { name: "id", type: { declaration: "bigint" } }], + [[7, 1, 2]], + ), + ); + + const result = await profiled.queryReadOnly(READ_SELECT.sql, budget()); + + expect(result.fields).toEqual(["(No column name)", "id", "id (2)"]); + expect(result.rows).toEqual([{ "(No column name)": 7, id: 1, "id (2)": 2 }]); + expect(result.columnTypes).toEqual({ id: "int", "id (2)": "bigint" }); + // Only the execution reads positionally: the admission compiled the statement in object + // mode, because the plan's rows are read by the names SQL Server gives them. + expect(modes.get(READ_SELECT.sql)).toEqual([false, true]); + expect(modes.get(PLAN_MODE_PROBE.sql)).toEqual([null]); + }); + + test("a cut value is refused under the name the caller is shown", async () => { + const profiled = await openProfiled(); + executeAnswers(arrayModeSet(["v", "v"], [["short", "x".repeat(1025)]])); + + const rejection = profiled.queryReadOnly(READ_SELECT.sql, budget({ maxResultBytes: 1024 })); + + await expect(rejection).rejects.toThrow( + /^Read-only execution refused a value the server cut at the byte budget: v \(2\) came back/, + ); + }); + // ------------------------------------------------------------------------- // The deadline: cancelled, not abandoned // ------------------------------------------------------------------------- From e3e7ef3f3e8f0d690daa4db6e6459b624a00663b Mon Sep 17 00:00:00 2001 From: cevheri Date: Wed, 7 Oct 2026 11:25:29 +0300 Subject: [PATCH 03/46] fix(mssql): assert zoneless conversion in later result sets and retitle stale test names The batch test now gives its second set a datetime2 column and checks the converted text under its unique name, so dropping that conversion fails. Two tests are retitled to what they now exercise, a test comment no longer cites the deleted X9 bullet, and the columnTypes section names all three execution paths. --- docs/providers/mssql.md | 2 +- tests/integration/db/mssql-provider.test.ts | 22 +++++++++++++++------ 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/docs/providers/mssql.md b/docs/providers/mssql.md index d9063b8bf..fff8b8204 100644 --- a/docs/providers/mssql.md +++ b/docs/providers/mssql.md @@ -484,7 +484,7 @@ This is the only source of a type for a computed column or an ad-hoc projection. because `BIGINT` reaches the browser as a string and `DECIMAL` as a number (§5.3): measured before this existed, the probe table's `BIGINT` and `UNIQUEIDENTIFIER` columns both exported as `NVARCHAR(MAX)` and its `DECIMAL(10,2)` as `FLOAT` - which is the same evidence read the right way -round, a string exporting as text and a number as a float. Both execution paths fill it from the same column map - including +round, a string exporting as text and a number as a float. All three execution paths fill it from the same column map - including `queryInTransaction()`, which had the map available all along and simply never read it. ### 5.5 What the SQL INSERT export writes for a BIT diff --git a/tests/integration/db/mssql-provider.test.ts b/tests/integration/db/mssql-provider.test.ts index eff05f77a..5f27dda87 100644 --- a/tests/integration/db/mssql-provider.test.ts +++ b/tests/integration/db/mssql-provider.test.ts @@ -2146,7 +2146,7 @@ describe("MSSQLProvider declared column types", () => { expect(Object.hasOwn(result, "resultSets")).toBe(false); }); - test("the key is omitted entirely when the recordset carries no column map", async () => { + test("the key is omitted entirely when no column's metadata carries a declaration", async () => { mockQueryFn = async () => ({ recordset: [{ a: 1 }], rowsAffected: [1] }); await provider.connect(); @@ -2353,7 +2353,7 @@ describe("MSSQLProvider zoneless value types (#1132)", () => { expect(result.rows[0].t).toBeNull(); }); - test("a result without the driver's column map is left exactly as the driver built it", async () => { + test("a column whose metadata carries no declaration is left exactly as the driver built it", async () => { const untyped = tediousDate(Date.UTC(1970, 0, 1, 10, 30, 0, 123), 0.0004567); mockQueryFn = async () => ({ recordset: [{ t: untyped }], rowsAffected: [1] }); @@ -2512,16 +2512,26 @@ describe("MSSQLProvider result column names", () => { }); test("each result set of a batch is named and keyed on its own", async () => { - answerWith(arrayModeSet(["a", "a"], [[1, 2]]), arrayModeSet(["", "b"], [[3, 4]])); + answerWith( + arrayModeSet(["a", "a"], [[1, 2]]), + arrayModeSet( + ["", "b", { name: "", type: { declaration: "datetime2" }, scale: 7 }], + [[3, 4, tediousDate(Date.UTC(2026, 9, 7, 10, 30, 0, 123), 0.0004567)]], + ), + ); await provider.connect(); - const result = await provider.query("SELECT 1 AS a, 2 AS a; SELECT 3, 4 AS b"); + const result = await provider.query("SELECT 1 AS a, 2 AS a; SELECT 3, 4 AS b, SYSDATETIME()"); expect(result.fields).toEqual(["a", "a (2)"]); expect(result.rows).toEqual([{ a: 1, "a (2)": 2 }]); expect(result.resultSets).toEqual([ { rows: [{ a: 1, "a (2)": 2 }], fields: ["a", "a (2)"] }, - { rows: [{ "(No column name)": 3, b: 4 }], fields: ["(No column name)", "b"] }, + { + rows: [{ "(No column name)": 3, b: 4, "(No column name) (2)": "2026-10-07 10:30:00.1234567" }], + fields: ["(No column name)", "b", "(No column name) (2)"], + columnTypes: { "(No column name) (2)": "datetime2" }, + }, ]); }); @@ -2573,7 +2583,7 @@ describe("MSSQLProvider result column names", () => { expect(result.fields).toEqual(["(No column name)", "id", "id (2)"]); expect(result.rows).toEqual([{ "(No column name)": 1, id: 2, "id (2)": 3 }]); - // The X9 residue: a zero-row result named no fields while its types were declared. + // Formerly a zero-row result named no fields while its types were declared. answerWith(arrayModeSet([{ name: "u", type: { declaration: "uniqueidentifier" } }], [])); const empty = await provider.queryInTransaction("SELECT u FROM t WHERE 1 = 0"); From 70c412e7f11fffc1032d574effc292af7ee1f595 Mon Sep 17 00:00:00 2001 From: cevheri Date: Wed, 7 Oct 2026 11:34:02 +0300 Subject: [PATCH 04/46] fix(db): read PostgreSQL and MySQL results by position so a repeated column keeps its value pg and mysql2 object rows keep only the last of two columns that share a name, so a join projecting id from both tables showed one table's id under both headers. query(), queryInTransaction() and the PostgreSQL read-only profile now ask for array rows (rowMode "array", rowsAsArray), name the columns with uniqueFieldNames and key each row by position; columnTypes follows the same names, and a row whose value count differs from the column count raises. A multi-statement PostgreSQL text and a MySQL CALL answer a list of results: rows, fields and columnTypes are now the first result set's and resultSets lists every set. Before, the PostgreSQL text answered no rows and a CALL threw a TypeError after the procedure ran. Measured on PostgreSQL 16 and MySQL 8.4; provider docs updated. object-route.ts citations of postgres.ts line numbers moved with the inserted helpers. --- docs/providers/mysql.md | 28 +- docs/providers/postgres.md | 25 +- src/lib/api/object-route.ts | 4 +- src/lib/db/providers/sql/mysql.ts | 97 ++++- src/lib/db/providers/sql/postgres.ts | 110 ++++-- tests/integration/db/mysql-provider.test.ts | 271 +++++++++++++- .../db/mysql-wire-decoding.test.ts | 75 +++- .../integration/db/postgres-provider.test.ts | 340 +++++++++++++++--- .../isolated/agent-investigation-e2e.test.ts | 13 +- 9 files changed, 862 insertions(+), 101 deletions(-) diff --git a/docs/providers/mysql.md b/docs/providers/mysql.md index d0ff1df93..66b02b46a 100644 --- a/docs/providers/mysql.md +++ b/docs/providers/mysql.md @@ -270,7 +270,7 @@ against three live servers: One behaviour does differ, and only for a connection that opted into `multipleStatements=true` in its connection string: a `;`-separated statement is rejected by the prepared protocol and accepted by the text one, which then answers an array of result sets. That is the shape `CALL ()` already -answers today on both protocols, so nothing new reaches the envelope; the app splits multi-statement +answers on both protocols, and the envelope reads it the same way ([§5.1](#51-execution)); the app splits multi-statement input itself (`POST /api/db/multi-query`) and issues one statement per call. `rowCount` is `rows.length` **only when the driver returns a row array** (i.e. `SELECT`); for a @@ -656,9 +656,33 @@ standard envelope with the driver's own values ([§3.3](#33-blob--binary-values-reach-every-surface-as-bytes)): ```ts -{ rows, fields: string[], rowCount: rows.length, executionTime, columnTypes? } +{ rows, fields: string[], rowCount: rows.length, executionTime, columnTypes?, resultSets? } ``` +**Every column keeps its own value.** +`query()` and `queryInTransaction()` ask `mysql2` for array rows (`rowsAsArray: true`, on the text protocol, the prepared one, the client-side binding and the utf8mb3 relabelling path alike), name the columns with `uniqueFieldNames` ([result-fields.ts](../../src/lib/db/utils/result-fields.ts)) and key each row by those names by position. +`mysql2`'s object rows key a value by its column's name and keep the last of two columns that share one, so before this a repeated name lost a value with no error. +`rowsAsArray` changes the row's shape and nothing else: `dateStrings`, `supportBigNumbers` and the relabelled decoding read every value as before. +`columnTypes` is keyed by the same names. +A row whose value count is not the column count raises a `QueryError` rather than being read. +The provider's own reads (the object tree, monitoring, maintenance) keep object rows, because they read columns by names they wrote themselves. + +Measured on MySQL 8.4 through `mysql2` 3.24.5 on 2026-10-07, before and after: + +| Statement | Before | After | +|---|---|---| +| `SELECT 1 AS a, 2 AS a` | fields `["a","a"]`, row `{"a":2}` | fields `["a","a (2)"]`, row `{"a":1,"a (2)":2}` | +| `SELECT 1, 1` (MySQL names an unaliased expression by its text) | fields `["1","1"]`, row `{"1":1}` | fields `["1","1 (2)"]`, row `{"1":1,"1 (2)":1}` | +| `SELECT * FROM orders o JOIN customers c ON c.id = o.customer_id` | fields `["id","customer_id","item","id","name"]`, the customer's id under both `id` headers, the order's id gone | fields `["id","customer_id","item","id (2)","name"]`, the order's id under `id` and the customer's under `id (2)` | +| `CALL sys.ps_setup_show_enabled(FALSE, FALSE)` | `TypeError: undefined is not an object (evaluating 'f.name')`, after the procedure had run | the first of its four result sets, and all four in `resultSets` | + +MySQL never answers an empty column name, so an unnamed column is only ever a repeated one here. + +**A `CALL` answers a list of result sets.** +`mysql2` answers one result set per SELECT the procedure ran and the call's own OK packet last, with the field packets one list per set and `undefined` for each header. +`rows`, `fields` and `columnTypes` are the first set's, `rowCount` its row count, and `resultSets` lists every set when there are several. +A list with no result set in it, which only a connection that opted into `multipleStatements` can receive, answers the first header's `affectedRows`. + Native `mysql2` errors are normalised via `mapDatabaseError()` into the shared [`errors.ts`](../../src/lib/db/errors.ts) classes. diff --git a/docs/providers/postgres.md b/docs/providers/postgres.md index d9d176e17..f5b062101 100644 --- a/docs/providers/postgres.md +++ b/docs/providers/postgres.md @@ -1320,9 +1320,32 @@ acquires a pooled client, optionally records its backend PID for cancellation, r (optionally parameterized — `$1`, `$2`, …) statement, and returns the standard envelope: ```ts -{ rows, fields: string[], rowCount, executionTime, columnTypes?, warnings? } +{ rows, fields: string[], rowCount, executionTime, columnTypes?, warnings?, resultSets? } ``` +#### Result columns: every column keeps its own value + +`query()`, `queryInTransaction()` and `queryReadOnly()` ask `pg` for array rows (`rowMode: "array"`), name the columns with `uniqueFieldNames` ([result-fields.ts](../../src/lib/db/utils/result-fields.ts)) and key each row by those names by position. +`pg`'s object rows key a value by its column's name and keep the last of two columns that share one, so before this a repeated name lost a value with no error. +`rowMode` changes the row's shape and nothing else: the pool's type parsers (§5.5) read every value as before. +`columnTypes` is keyed by the same names. +A row whose value count is not the column count raises a `QueryError` rather than being read. + +Measured on PostgreSQL 16 through `pg` 8.23.1 on 2026-10-07, before and after: + +| Statement | Before | After | +|---|---|---| +| `SELECT 1 AS a, 2 AS a` | fields `["a","a"]`, row `{"a":2}` | fields `["a","a (2)"]`, row `{"a":1,"a (2)":2}` | +| `SELECT 1, 2` (unnamed columns, which PostgreSQL calls `?column?`) | fields `["?column?","?column?"]`, row `{"?column?":2}` | fields `["?column?","?column? (2)"]`, row `{"?column?":1,"?column? (2)":2}` | +| `SELECT * FROM orders o JOIN customers c ON c.id = o.customer_id` | fields `["id","customer_id","item","id","name"]`, the customer's id under both `id` headers, the order's id gone | fields `["id","customer_id","item","id (2)","name"]`, the order's id under `id` and the customer's under `id (2)` | + +PostgreSQL never answers an empty column name: an unaliased expression is named by the server (`?column?`, `count`, `now`), so an unnamed column is only ever a repeated one here. + +A text of several statements sent as one simple query (`SELECT 1 AS a; SELECT 2 AS b, 3 AS b`) answers one result per statement. +`rows`, `fields` and `columnTypes` are the first result set's, `rowCount` is the count of the statement that produced it (or of the first statement when none produced one), and `resultSets` lists every set when there are several; a statement with no row description (an INSERT, a COMMIT) is not a set. +Measured the same day, that text answers `rows` `[{"a":1}]` and two `resultSets`, the second with fields `["b","b (2)"]`. +Before, such a text answered no `rows` at all, because the list of answers was read as one answer. + #### Server notices (#1401) The NOTICE and WARNING messages the server sends while a statement that SUCCEEDS runs come back in diff --git a/src/lib/api/object-route.ts b/src/lib/api/object-route.ts index 179528f00..5d576b39a 100644 --- a/src/lib/api/object-route.ts +++ b/src/lib/api/object-route.ts @@ -711,7 +711,7 @@ function boundText(part: ObjectSourcePart, limit: number): ObjectSourcePart { * was the only engine that had landed; the day-one set is now three and the count was re-measured * rather than the digit bumped, because what it counts is what the paragraph is for. * - * There are THREE producers of `edit`: `providers/sql/postgres.ts:3709`, gated on + * There are THREE producers of `edit`: `providers/sql/postgres.ts:3773`, gated on * `kindAcceptsSourceEdits(capabilities, kind)`; `providers/sql/trino/index.ts:1284` and * `providers/keyvalue/redis.ts:2235`, both gated on `spec.acceptsSourceEdits === true`, which is the * same fact read through the same declaration. All three sit on the READABLE arm, verified rather @@ -745,7 +745,7 @@ function boundText(part: ObjectSourcePart, limit: number): ObjectSourcePart { * * THE BOUND, on both sides of the same constant. `edit-plan/route.ts:74` refuses a SUBMITTED text * longer than `EDIT_CHARACTER_LIMIT`, and all three day-one providers refuse a READ definition longer - * than it inside `buildObjectEdit`: `providers/sql/postgres.ts:3866`, `providers/keyvalue/redis.ts:2325` + * than it inside `buildObjectEdit`: `providers/sql/postgres.ts:3930`, `providers/keyvalue/redis.ts:2325` * and `providers/sql/trino/index.ts:1478`. The second is what closes the class rather than narrowing * it: a plan is minted only from the build's own read, so a definition the pane could only have shown * truncated never reaches a plan at all, whatever the client POSTs. diff --git a/src/lib/db/providers/sql/mysql.ts b/src/lib/db/providers/sql/mysql.ts index d2bc0b8bd..e32fd0491 100644 --- a/src/lib/db/providers/sql/mysql.ts +++ b/src/lib/db/providers/sql/mysql.ts @@ -9,10 +9,12 @@ import mysql, { type PoolConnection, type RowDataPacket, type FieldPacket, + type QueryOptions, type ResultSetHeader, } from "mysql2/promise"; import { SQLBaseProvider } from "./sql-base"; import { mysqlColumnTypes } from "./column-types"; +import { uniqueFieldNames } from "../../utils/result-fields"; import { type ColumnSchema, type Container, @@ -80,6 +82,7 @@ import { formatBytes } from "../../utils/pool-manager"; import { measuredNullableAggregate } from "../../utils/measured-aggregate"; import { CACHE_HIT_RATIO_UNAVAILABLE, formatCacheHitRatio, measuredNumber } from "@/lib/monitoring-cache-ratio"; import { unquoteLiteral } from "@/lib/sql/values"; +import type { QueryResultSet } from "@/lib/types"; import { portableDefaultSql, showCreateColumnDefaults } from "./mysql-show-create"; /** @@ -166,24 +169,38 @@ type MySQLQueryable = Pick; * The one exception is a server that refuses COM_STMT_PREPARE itself, which * `probeClientSideBinding()` measures at connect: there a parameterised statement is * written out by `bindClientSide()` and goes over the text protocol like any other. + * + * `arrayRows` asks for each row as its values in column order (`rowsAsArray`), on every one of + * those paths. A user's statement is read that way, see `buildQueryResult`; the provider's own + * reads keep object rows, because they read columns by the names they wrote themselves. */ const runStatement = ( queryable: MySQLQueryable, sql: string, params?: unknown[], + arrayRows = false, ): Promise<[T, FieldPacket[]]> => { const core = (queryable as { connection?: object }).connection; if (core !== undefined && BINDS_CLIENT_SIDE.has(core) && params !== undefined && params.length > 0) { - return runBoundClientSide(queryable, core as CoreConnection, sql, params); + return runBoundClientSide(queryable, core as CoreConnection, sql, params, arrayRows); } if (core !== undefined && UTF8_UNDER_UTF8MB3.has(core)) { - return runReadingUtf8mb3AsUtf8(core as CoreConnection, sql, params); + return runReadingUtf8mb3AsUtf8(core as CoreConnection, statementOf(sql, arrayRows), params); + } + // Two calls each rather than one over `statementOf()`: mysql2 types the text and the options + // object as two overloads, and the provider's own reads keep sending the bare text. + if (params === undefined || params.length === 0) { + return arrayRows ? queryable.query({ sql, rowsAsArray: true }) : queryable.query(sql); } - return params === undefined || params.length === 0 - ? queryable.query(sql) + return arrayRows + ? queryable.execute({ sql, rowsAsArray: true }, asExecuteParams(params)) : queryable.execute(sql, asExecuteParams(params)); }; +/** The statement as mysql2 takes it: the bare text, or with `rowsAsArray` when array rows are asked for. */ +const statementOf = (sql: string, arrayRows: boolean): string | QueryOptions => + arrayRows ? { sql, rowsAsArray: true } : sql; + /** * The core (callback) connections of a pool whose server refuses COM_STMT_PREPARE, see * `probeClientSideBinding()`. Filled from that pool's `acquire` event, the same way and for @@ -296,6 +313,7 @@ async function runBoundClientSide( core: CoreConnection, sql: string, params: unknown[], + arrayRows: boolean, ): Promise<[T, FieldPacket[]]> { const text = bindClientSide(core, sql, params); if (params.some((value) => typeof value === "string" && value.includes("\\"))) { @@ -307,7 +325,7 @@ async function runBoundClientSide( ); } } - return runStatement(queryable, text); + return runStatement(queryable, text, undefined, arrayRows); } /** @@ -378,6 +396,46 @@ const probeClientSideBinding = async (queryable: MySQLQueryable): Promise field.name)); + return { + rows: rows.map((values) => { + const row = values as unknown[]; + if (row.length !== fields.length) { + throw new QueryError(`MySQL answered a row of ${row.length} values for ${fields.length} columns`, "mysql"); + } + return Object.fromEntries(fields.map((name, index) => [name, row[index]])); + }), + fields, + ...mysqlColumnTypes(declared.map((field, index) => ({ ...field, name: fields[index] }))), + }; +} + +/** What a statement that answered a result set, or a `CALL`'s list of them, is read as (`buildQueryResult`). */ +function mysqlResults( + rows: readonly unknown[], + fields: FieldPacket[] | undefined, +): Pick { + const perAnswer = (fields ?? []) as unknown as (FieldPacket[] | undefined)[]; + if (!perAnswer.some((declared) => declared === undefined || Array.isArray(declared))) { + const set = mysqlResultSet(rows, fields ?? []); + return { ...set, rowCount: set.rows.length }; + } + const sets = perAnswer.flatMap((declared, index) => + Array.isArray(declared) ? [mysqlResultSet(rows[index] as unknown[], declared)] : [], + ); + const [first] = sets; + if (first === undefined) { + return { rows: [], fields: [], rowCount: (rows[0] as { affectedRows?: number }).affectedRows ?? 0 }; + } + return { ...first, rowCount: first.rows.length, ...(sets.length > 1 && { resultSets: sets }) }; +} + /** * The core (callback) connections of a pool whose server sends UTF-8 under a utf8mb3 * label, see `probeUtf8UnderUtf8mb3()`. Filled from that pool's `acquire` event, so @@ -392,8 +450,8 @@ interface CoreCommand { on(event: "fields", listener: (fields?: FieldPacket[]) => void): unknown; } interface CoreConnection { - query(sql: string, callback: CoreCallback): CoreCommand; - execute(sql: string, values: unknown[], callback: CoreCallback): CoreCommand; + query(sql: string | QueryOptions, callback: CoreCallback): CoreCommand; + execute(sql: string | QueryOptions, values: unknown[], callback: CoreCallback): CoreCommand; format(sql: string, values: unknown[]): string; } @@ -429,7 +487,7 @@ const readUtf8mb3AsUtf8 = (fields?: FieldPacket[]): void => { */ function runReadingUtf8mb3AsUtf8( core: CoreConnection, - sql: string, + sql: string | QueryOptions, params?: unknown[], ): Promise<[T, FieldPacket[]]> { return new Promise((resolve, reject) => { @@ -507,7 +565,9 @@ const SERVER_STATUS_AUTOCOMMIT = 2; * element is the header of the call itself, which is the state after everything the * procedure ran. The two arrays are told apart by their first element, a row set (an * array) for the `CALL` and a row (an object) for a read, so a column that happens to be - * named `serverStatus` is never read as the flags. + * named `serverStatus` is never read as the flags. A user's statement is read with array + * rows (`rowsAsArray`), where a read's first element is an array too; its last element is + * then a row, an array, which is no header either. */ function statusFlagsOf(result: unknown): number | undefined { const header = Array.isArray(result) ? (Array.isArray(result[0]) ? result[result.length - 1] : undefined) : result; @@ -3058,6 +3118,16 @@ export class MySQLProvider extends SQLBaseProvider { * footer renders. `affectedRows` is the matched count, which is why a no-op * UPDATE still reports 1 - matching mssql, whose `rowsAffected` counts the same * way. + * + * A result set arrives as array rows (`rowsAsArray`, see `runStatement`) and is keyed here by + * `uniqueFieldNames`, by position: mysql2's object rows keep the last of two columns that share + * a name, so a join projecting `id` from both tables showed one table's id under both headers. + * A `CALL` answers one result set per SELECT its procedure ran and its own OK packet last, with + * `fields` one list per set and `undefined` for each header; a connection string that opted into + * `multipleStatements` answers a `;`-separated text the same way. Then `rows`, `fields` and + * `columnTypes` are the first set's, `rowCount` its row count (or the first header's + * `affectedRows` when no set came back), and `resultSets` lists every set when there are several. + * Before, that list was read as one set's rows. */ private buildQueryResult(rows: unknown, fields: FieldPacket[] | undefined, executionTime: number): QueryResult { if (!Array.isArray(rows)) { @@ -3081,10 +3151,7 @@ export class MySQLProvider extends SQLBaseProvider { // `0x0102ab` where Postgres showed `\x0102ab`, and the export wrote the eight // characters `'0x0102ab'` into a BLOB column rather than the three bytes. // Measured against MySQL 26.7.0 on 2026-08-24; see docs/providers/mysql.md §3.3. - rows: rows as Record[], - fields: fields?.map((f: FieldPacket) => f.name) ?? [], - ...mysqlColumnTypes(fields), - rowCount: rows.length, + ...mysqlResults(rows, fields), executionTime, }; } @@ -3103,7 +3170,7 @@ export class MySQLProvider extends SQLBaseProvider { if (queryId) { this.runningQueryThreadIds.set(queryId, conn.threadId); } - const [rows, fields] = await runStatement(conn, sql, params); + const [rows, fields] = await runStatement(conn, sql, params, true); return { rows, fields }; } catch (error) { throw mapDatabaseError(error, "mysql", sql); @@ -3266,7 +3333,7 @@ export class MySQLProvider extends SQLBaseProvider { return this.trackQuery(async () => { const { result, executionTime } = await this.measureExecution(async () => { try { - const [rows, fields] = await runStatement(this.txConn!, sql, params); + const [rows, fields] = await runStatement(this.txConn!, sql, params, true); // The server's own word on whether the transaction survived the statement. A // statement that commits implicitly (DDL, `SET autocommit = 1`, a typed // `COMMIT`) ends it, and from then on the held connection autocommits: a diff --git a/src/lib/db/providers/sql/postgres.ts b/src/lib/db/providers/sql/postgres.ts index d43d09f6f..0cda34e74 100644 --- a/src/lib/db/providers/sql/postgres.ts +++ b/src/lib/db/providers/sql/postgres.ts @@ -86,9 +86,11 @@ import { NO_TRANSACTION_OPENED, } from "../../errors"; import { ApiErrorCode } from "@/lib/api/error-codes"; +import type { QueryResultSet } from "@/lib/types"; import { assertReadOnlyBudget, measureResultBytes } from "./read-only-budget"; import { sendPgCancelRequest, type PgCancelTarget } from "./pg-wire-cancel"; -import { postgresColumnTypes } from "./column-types"; +import { type PgFieldMetadata, postgresColumnTypes } from "./column-types"; +import { uniqueFieldNames } from "../../utils/result-fields"; import { formatBytes } from "../../utils/pool-manager"; import { measuredNullableAggregate } from "../../utils/measured-aggregate"; import { CACHE_HIT_RATIO_UNAVAILABLE, formatCacheHitRatio, measuredNumber } from "@/lib/monitoring-cache-ratio"; @@ -287,6 +289,75 @@ function noticesAsWarnings({ kept, dropped }: NoticeBatch): { warnings?: QueryWa return { warnings }; } +/** + * One `pg` answer read under `rowMode: "array"`: each row holds its values in the order `fields` + * declares the columns. + */ +interface PgArrayAnswer { + rows: unknown[][]; + fields?: PgFieldMetadata[]; + rowCount: number | null; +} + +/** + * A user's statement as `pg` is asked to answer it: array rows, so two columns of one name keep two + * values. `pg`'s object rows key a value by its column's name and keep the last of two that share + * one: measured 2026-10-07 on PostgreSQL 16, a join projecting `id` from both tables answered the + * customer's id under both headers. The values go beside the config, so the protocol `pg` picks is + * the one a bare string and values got. `rowMode` changes the row's shape, not its parsers. + */ +function arrayRowsOf(text: string): QueryConfig { + return { text, rowMode: "array" } as QueryConfig; +} + +/** + * One answer as a result set: its columns named by `uniqueFieldNames`, and each row keyed by those + * names by position. A row whose value count is not the column count is raised, not read. + */ +function pgResultSet(answer: PgArrayAnswer): QueryResultSet { + const declared = answer.fields ?? []; + const fields = uniqueFieldNames(declared.map((field) => field.name)); + const rows = answer.rows.map((values) => { + if (values.length !== fields.length) { + throw new QueryError( + `PostgreSQL answered a row of ${values.length} values for ${fields.length} columns`, + "postgres", + ); + } + // `Object.fromEntries` and not assignment, so a column named `__proto__` is a key like any other. + return Object.fromEntries(fields.map((name, index) => [name, values[index]])); + }); + return { + rows, + fields, + ...postgresColumnTypes(declared.map((field, index) => ({ name: fields[index], dataTypeID: field.dataTypeID }))), + }; +} + +/** + * The result of what `pg` answered a user's text with. + * + * One statement answers one result. A text of several statements on the simple protocol answers + * one per statement, and a statement with no row description (an INSERT, a COMMIT) carries no + * result set. Then, as `QueryResult.resultSets` says, `rows`, `fields` and `columnTypes` are the + * first set's and `rowCount` the count of the statement that produced it, or of the first + * statement when none did; every set is listed when there are several. Before this read such a + * text answered no `rows` at all, because the answer was an array and was read as one result. + */ +function pgQueryResult( + answer: PgArrayAnswer | PgArrayAnswer[], +): Pick { + if (!Array.isArray(answer)) return { ...pgResultSet(answer), rowCount: answer.rowCount ?? 0 }; + const producing = answer.filter((one) => (one.fields ?? []).length > 0); + const sets = producing.map(pgResultSet); + const [first] = sets; + return { + ...(first ?? { rows: [], fields: [] }), + rowCount: (producing[0] ?? answer[0])?.rowCount ?? 0, + ...(sets.length > 1 && { resultSets: sets }), + }; +} + /** * Whether a notice sent during the startup handshake is worth showing. SQLSTATE class `00` is * successful completion: Materialize greets every session with one (`00000`, "connected to @@ -2768,8 +2839,8 @@ export class PostgresProvider extends SQLBaseProvider { // Dropped first: a client fresh from the pool still holds its startup greeting, and // the PID read above is not the user's statement. takeNotices(client); - const res = await client.query(sql, params); - return { res, notices: takeNotices(client) }; + const res = await client.query(arrayRowsOf(sql), params); + return { res: res as unknown as PgArrayAnswer | PgArrayAnswer[], notices: takeNotices(client) }; } finally { if (queryId) this.runningQueries.delete(queryId); // Read while this call still HOLDS the client, and before the release that @@ -2787,10 +2858,7 @@ export class PostgresProvider extends SQLBaseProvider { }); return { - rows: result.res.rows, - fields: result.res.fields?.map((f) => f.name) ?? [], - ...postgresColumnTypes(result.res.fields), - rowCount: result.res.rowCount ?? 0, + ...pgQueryResult(result.res), executionTime, ...noticesAsWarnings(result.notices), }; @@ -2896,7 +2964,7 @@ export class PostgresProvider extends SQLBaseProvider { } return this.trackQuery(async () => { - const { result, executionTime } = await this.measureExecution(async () => { + const { result: answered, executionTime } = await this.measureExecution(async () => { const client = await this.pool!.connect(); try { await client.query("BEGIN READ ONLY"); @@ -2905,8 +2973,10 @@ export class PostgresProvider extends SQLBaseProvider { await client.query(`SET LOCAL statement_timeout = ${budget.statementTimeoutMs}`); // @types/pg does not model queryMode yet; the runtime supports it // since pg 8.11 (node_modules/pg/lib/query.js requiresPreparation). - const extendedQuery = { text: sql, queryMode: "extended" } as QueryConfig & { queryMode: "extended" }; - return await client.query(extendedQuery); + const extendedQuery = { ...arrayRowsOf(sql), queryMode: "extended" } as QueryConfig & { + queryMode: "extended"; + }; + return await client.query(extendedQuery); } catch (error) { throw mapDatabaseError(error, "postgres", sql); } finally { @@ -2927,6 +2997,9 @@ export class PostgresProvider extends SQLBaseProvider { } }); + // One statement on the extended protocol is one answer, never a list of them. The budgets are + // measured on the keyed rows, which are what the caller receives. + const result = pgQueryResult(answered as unknown as PgArrayAnswer); if (result.rows.length > budget.maxResultRows) { throw new QueryError( `Read-only execution exceeded the row budget: ${result.rows.length} rows > ${budget.maxResultRows} allowed`, @@ -2943,13 +3016,7 @@ export class PostgresProvider extends SQLBaseProvider { ); } - return { - rows: result.rows, - fields: result.fields?.map((f) => f.name) ?? [], - ...postgresColumnTypes(result.fields), - rowCount: result.rowCount ?? 0, - executionTime, - }; + return { ...result, executionTime }; }); } @@ -3227,8 +3294,8 @@ export class PostgresProvider extends SQLBaseProvider { // The held client is one session for the whole transaction, so what an earlier // statement left behind is dropped before this one runs. takeNotices(held); - const res = await held.query(sql, params); - return { res, notices: takeNotices(held) }; + const res = await held.query(arrayRowsOf(sql), params); + return { res: res as unknown as PgArrayAnswer | PgArrayAnswer[], notices: takeNotices(held) }; } catch (error) { throw mapDatabaseError(error, "postgres", sql); } finally { @@ -3243,10 +3310,7 @@ export class PostgresProvider extends SQLBaseProvider { }); return { - rows: result.res.rows, - fields: result.res.fields?.map((f) => f.name) ?? [], - ...postgresColumnTypes(result.res.fields), - rowCount: result.res.rowCount ?? 0, + ...pgQueryResult(result.res), executionTime, ...noticesAsWarnings(result.notices), }; diff --git a/tests/integration/db/mysql-provider.test.ts b/tests/integration/db/mysql-provider.test.ts index e3956598f..0e492537a 100644 --- a/tests/integration/db/mysql-provider.test.ts +++ b/tests/integration/db/mysql-provider.test.ts @@ -108,10 +108,57 @@ function methodFor(fragment: string): string | undefined { return protocolCalls.find((c) => c.sql.toLowerCase().includes(fragment.toLowerCase()))?.method; } +/** A mysql2 options object asking for array rows, which `query()` and `queryInTransaction()` send. */ +type MockStatement = string | { sql: string; rowsAsArray?: boolean }; + +type MockField = { name: string }; + +/** One result set's rows as mysql2 answers them under `rowsAsArray`: each row its values in column order. */ +function arrayRowsOf(rows: unknown[], declared: MockField[] | undefined): { rows: unknown[]; fields: MockField[] } { + const fields = + (declared ?? []).length > 0 || rows.length === 0 || Array.isArray(rows[0]) + ? (declared ?? []) + : Object.keys(rows[0] as object).map((name) => ({ name })); + return { + rows: rows.map((row) => (Array.isArray(row) ? row : fields.map((f) => (row as Record)[f.name]))), + fields, + }; +} + +/** + * What mysql2 answers under `rowsAsArray: true`. A fixture written with object rows is read in the + * order its fields declare, or its first row's key order when it declares none, as a real column + * definition would have said. A fixture that answers array rows is handed on unchanged, which is the + * only way to model two columns of one name. A header (no result set) and a CALL's list of result + * sets keep their shape, each set read the same way. + */ +function asRowsAsArray([rows, fields]: [unknown, unknown[] | undefined]): [unknown, unknown[] | undefined] { + if (!Array.isArray(rows)) return [rows, fields]; + if (fields !== undefined && fields.some((set) => set === undefined || Array.isArray(set))) { + const sets = rows.map((set, index) => + Array.isArray(set) ? arrayRowsOf(set, fields[index] as MockField[]).rows : set, + ); + return [sets, fields]; + } + const read = arrayRowsOf(rows, fields as MockField[] | undefined); + return [read.rows, fields === undefined ? fields : read.fields]; +} + +/** The statement's text for the fixtures, and its answer in the row shape the provider asked for. */ +function answerStatement( + method: "query" | "execute", + statement: MockStatement, + params?: unknown[], +): Promise<[unknown, unknown[] | undefined]> { + if (typeof statement === "string") return recordCall(method, statement, params); + const answered = recordCall(method, statement.sql, params); + return statement.rowsAsArray === true ? answered.then(asRowsAsArray) : answered; +} + const mockConnection = { threadId: 42, - query: (sql: string, params?: unknown[]) => recordCall("query", sql, params), - execute: (sql: string, params?: unknown[]) => recordCall("execute", sql, params), + query: (sql: MockStatement, params?: unknown[]) => answerStatement("query", sql, params), + execute: (sql: MockStatement, params?: unknown[]) => answerStatement("execute", sql, params), release: () => {}, beginTransaction: async () => {}, commit: async () => {}, @@ -121,8 +168,8 @@ const mockConnection = { const mockPool = { getConnection: async () => mockConnection, end: async () => {}, - query: (sql: string, params?: unknown[]) => recordCall("query", sql, params), - execute: (sql: string, params?: unknown[]) => recordCall("execute", sql, params), + query: (sql: MockStatement, params?: unknown[]) => answerStatement("query", sql, params), + execute: (sql: MockStatement, params?: unknown[]) => answerStatement("execute", sql, params), }; /** @@ -1895,7 +1942,13 @@ describe("MySQLProvider", () => { let callStatus = 3; mockExecuteFn = (sql: string) => { if (sql.startsWith("BEGIN")) return Promise.resolve([header(16387), undefined]); - if (sql.startsWith("CALL")) return Promise.resolve([[[{ id: 1 }], header(callStatus)], undefined]); + // `fields` holds one list per result set and `undefined` for the header, as mysql2 answers a CALL. + if (sql.startsWith("CALL")) { + return Promise.resolve([ + [[{ id: 1 }], header(callStatus)], + [[{ name: "id" }], undefined], + ]); + } return defaultMockExecute(sql) as Promise<[unknown, unknown[] | undefined]>; }; provider = new MySQLProvider(makeMySQLConfig()); @@ -3314,6 +3367,214 @@ describe("MySQLProvider declared column types", () => { }); }); +/** + * Every column keeps its own value (D3). mysql2's object rows key a value by its column's name, so of + * two columns one name declares, the row keeps only the last: measured 2026-10-07 on MySQL 8.4, the + * join below answered the customer's id under both `id` headers and the order's id nowhere. The + * provider asks for array rows and keys them by `uniqueFieldNames`, so a repeat is `id (2)`. The + * fixtures answer the array rows that run measured, so the mock cannot drop a value the driver kept. + */ +describe("MySQLProvider result columns", () => { + const int = { columnType: 3, characterSet: 63, columnLength: 11, decimals: 0, flags: 0 }; + const text = { columnType: 252, characterSet: 255, columnLength: 262140, decimals: 0, flags: 16 }; + const JOIN_SQL = "SELECT * FROM orders o JOIN customers c ON c.id = o.customer_id"; + const join: [unknown, unknown[]] = [ + [ + [1, 100, "widget", 100, "Acme"], + [2, 200, "gadget", 200, "Globex"], + ], + [ + { name: "id", ...int }, + { name: "customer_id", ...int }, + { name: "item", ...text }, + { name: "id", ...int }, + { name: "name", ...text }, + ], + ]; + const joinFields = ["id", "customer_id", "item", "id (2)", "name"]; + const joinRows = [ + { id: 1, customer_id: 100, item: "widget", "id (2)": 100, name: "Acme" }, + { id: 2, customer_id: 200, item: "gadget", "id (2)": 200, name: "Globex" }, + ]; + + async function connected() { + const provider = new MySQLProvider(makeMySQLConfig()); + await provider.connect(); + return provider; + } + + test("query() asks mysql2 for array rows and keeps both values of a repeated name", async () => { + const query = spyOn(mockConnection, "query"); + mockExecuteFn = () => + Promise.resolve([ + [[1, 2]], + [ + { name: "a", ...int }, + { name: "a", ...text }, + ], + ]); + try { + const provider = await connected(); + const result = await provider.query("SELECT 1 AS a, 2 AS a"); + expect(query.mock.calls.at(-1)).toEqual([{ sql: "SELECT 1 AS a, 2 AS a", rowsAsArray: true }]); + expect(result.fields).toEqual(["a", "a (2)"]); + expect(result.rows).toEqual([{ a: 1, "a (2)": 2 }]); + expect(result.columnTypes).toEqual({ a: "int", "a (2)": "text" }); + expect(result.rowCount).toBe(1); + await provider.disconnect(); + } finally { + query.mockRestore(); + } + }); + + test("query() keeps the order's id and the customer's id of a join apart, over the prepared protocol too", async () => { + const execute = spyOn(mockConnection, "execute"); + mockExecuteFn = () => Promise.resolve(join); + try { + const provider = await connected(); + for (const params of [undefined, [1]]) { + const result = await provider.query(JOIN_SQL, params); + expect(result.fields).toEqual(joinFields); + expect(result.rows).toEqual(joinRows); + expect(result.columnTypes).toEqual({ + id: "int", + customer_id: "int", + item: "text", + "id (2)": "int", + name: "text", + }); + } + expect(execute.mock.calls.at(-1)).toEqual([{ sql: JOIN_SQL, rowsAsArray: true }, [1]]); + await provider.disconnect(); + } finally { + execute.mockRestore(); + } + }); + + test("query() numbers the repeated name MySQL gives two unaliased literals", async () => { + mockExecuteFn = () => + Promise.resolve([ + [[1, 1]], + [ + { name: "1", ...int }, + { name: "1", ...int }, + ], + ]); + const provider = await connected(); + const result = await provider.query("SELECT 1, 1"); + expect(result.fields).toEqual(["1", "1 (2)"]); + expect(result.rows).toEqual([{ "1": 1, "1 (2)": 1 }]); + await provider.disconnect(); + }); + + test("queryInTransaction() reads the same way", async () => { + mockExecuteFn = (sql) => + Promise.resolve(sql === JOIN_SQL ? join : [{ affectedRows: 0, serverStatus: 3 }, undefined]); + const provider = await connected(); + await provider.beginTransaction(); + const result = await provider.queryInTransaction(JOIN_SQL); + expect(result.fields).toEqual(joinFields); + expect(result.rows).toEqual(joinRows); + expect(provider.isInTransaction()).toBe(true); + await provider.rollbackTransaction(); + await provider.disconnect(); + }); + + /** + * A CALL answers one result set per SELECT the procedure ran and the call's own OK packet last. + * Measured 2026-10-07 on MySQL 8.4 with `CALL sys.ps_setup_show_enabled(FALSE, FALSE)`: four sets + * and a header, with `fields` one list per set and `undefined` for the header. Before, the list of + * sets was read as one set's rows, so the grid had a row per set and a column with no name. + */ + test("a CALL answers its first result set, and every set when it has several", async () => { + mockExecuteFn = () => + Promise.resolve([ + [[[1]], [["EVENT", "%.%"]], { affectedRows: 0, serverStatus: 2 }], + [ + [{ name: "performance_schema_enabled", ...int }], + [ + { name: "object_type", ...text }, + { name: "objects", ...text }, + ], + undefined, + ], + ]); + const provider = await connected(); + const result = await provider.query("CALL sys.ps_setup_show_enabled(FALSE, FALSE)"); + expect(result.fields).toEqual(["performance_schema_enabled"]); + expect(result.rows).toEqual([{ performance_schema_enabled: 1 }]); + expect(result.rowCount).toBe(1); + expect(result.columnTypes).toEqual({ performance_schema_enabled: "int" }); + expect(result.resultSets).toEqual([ + { + rows: [{ performance_schema_enabled: 1 }], + fields: ["performance_schema_enabled"], + columnTypes: { performance_schema_enabled: "int" }, + }, + { + rows: [{ object_type: "EVENT", objects: "%.%" }], + fields: ["object_type", "objects"], + columnTypes: { object_type: "text", objects: "text" }, + }, + ]); + await provider.disconnect(); + }); + + test("a CALL with one result set answers it alone, its repeated names numbered", async () => { + mockExecuteFn = () => + Promise.resolve([ + [[[1, 2]], { affectedRows: 0, serverStatus: 2 }], + [ + [ + { name: "a", ...int }, + { name: "a", ...int }, + ], + undefined, + ], + ]); + const provider = await connected(); + const result = await provider.query("CALL two_a()"); + expect(result.fields).toEqual(["a", "a (2)"]); + expect(result.rows).toEqual([{ a: 1, "a (2)": 2 }]); + expect(Object.hasOwn(result, "resultSets")).toBe(false); + await provider.disconnect(); + }); + + test("several statements that return no result set answer the first one's count", async () => { + // Only a connection string that opted into `multipleStatements=true` sends such a text whole. + mockExecuteFn = () => + Promise.resolve([ + [ + { affectedRows: 3, serverStatus: 2 }, + { affectedRows: 5, serverStatus: 2 }, + ], + [undefined, undefined], + ]); + const provider = await connected(); + const result = await provider.query("INSERT INTO t VALUES (1), (2), (3); DELETE FROM u"); + expect(result.rows).toEqual([]); + expect(result.fields).toEqual([]); + expect(result.rowCount).toBe(3); + await provider.disconnect(); + }); + + test("a row whose value count differs from the column count is an error, not a guess", async () => { + mockExecuteFn = () => + Promise.resolve([ + [[1]], + [ + { name: "a", ...int }, + { name: "b", ...int }, + ], + ]); + const provider = await connected(); + await expect(provider.query("SELECT a, b FROM t")).rejects.toThrow( + "MySQL answered a row of 1 values for 2 columns", + ); + await provider.disconnect(); + }); +}); + // ============================================================================ // Non-SELECT statements (the ResultSetHeader shape) // ============================================================================ diff --git a/tests/integration/db/mysql-wire-decoding.test.ts b/tests/integration/db/mysql-wire-decoding.test.ts index 57b356e9d..0e1d557f7 100644 --- a/tests/integration/db/mysql-wire-decoding.test.ts +++ b/tests/integration/db/mysql-wire-decoding.test.ts @@ -53,6 +53,7 @@ interface ServerConnection { writeEof(warnings?: number, statusFlags?: number): void; writeColumns(columns: Record[]): void; writeTextRow(values: string[]): void; + writeBinaryRow(values: string[]): void; writePacket(packet: unknown): void; } @@ -176,6 +177,12 @@ const startServer = (textLabel: number, binding: Binding = "prepares"): Promise< ]; const rows = [{ v: TEXT, b: "AB" }]; const legacy = [definition({ name: "legacy", characterSet: UTF8MB3_GENERAL_CI, columnType: VAR_STRING, flags: 0 })]; + // Two columns of one name, as `SELECT 1 AS a, 2 AS a` or a join of two tables that both have an `id` + // declares them. `writeTextResult` keys a row by column name and could not answer two values. + const repeated = [ + definition({ name: "a", characterSet: textLabel, columnType: VAR_STRING, flags: 0 }), + definition({ name: "a", characterSet: textLabel, columnType: VAR_STRING, flags: 0 }), + ]; server.on("connection", (connection) => { connection.serverHandshake({ protocolVersion: 10, @@ -224,6 +231,10 @@ const startServer = (textLabel: number, binding: Binding = "prepares"): Promise< ); } else if (sql.startsWith("SELECT v, b")) { connection.writeTextResult(rows, result.map(definition)); + } else if (sql.startsWith("SELECT a, a")) { + connection.writeColumns(repeated); + connection.writeTextRow(["first", "second"]); + connection.writeEof(); } else if (sql.startsWith("SELECT legacy")) { connection.writeTextResult([{ legacy: TEXT }], legacy); } else if (/^(BEGIN|START TRANSACTION)/.test(sql)) { @@ -276,6 +287,10 @@ const startServer = (textLabel: number, binding: Binding = "prepares"): Promise< if (prepared.get(statementId)?.startsWith("INSERT")) { // A prepared statement answering zero columns. connection.writeOk({ affectedRows: 2, insertId: 7, serverStatus: status() }); + } else if (prepared.get(statementId)?.startsWith("SELECT a, a")) { + connection.writeColumns(repeated); + connection.writeBinaryRow(["first", "second"]); + connection.writeEof(); } else { connection.writeTextResult(rows, result.map(definition), true); } @@ -416,15 +431,12 @@ describe("a server that labels utf8mb4 text honestly (MySQL, MariaDB, TiDB)", () */ describe("statements that answer no result set, on both kinds of server", () => { /** The driver's own answer, as `buildQueryResult` received it. */ - const driverAnswer = async (run: (provider: MySQLProvider) => Promise, port: number, stub = false) => { + const driverAnswer = async (run: (provider: MySQLProvider) => Promise, port: number) => { const provider = await connected(port); const build = spyOn( MySQLProvider.prototype as unknown as { buildQueryResult: (rows: unknown) => unknown }, "buildQueryResult", ); - // `buildQueryResult` reads one result set; a multi-result answer is read here at the driver - // seam instead, which is the part this file is about. - if (stub) build.mockImplementation(() => ({ rows: [], fields: [], rowCount: 0, executionTime: 0 })); try { const result = await run(provider); return { result, header: build.mock.calls.at(-1)?.[0] as unknown as Record, provider }; @@ -479,16 +491,19 @@ describe("statements that answer no result set, on both kinds of server", () => * relabel the other, and the connection has to survive all three. */ test("a result set between two OK packets is still relabelled", async () => { - const relabelled = await driverAnswer((provider) => provider.query("CALL two_results()"), labelling33.port, true); - const plain = await driverAnswer((provider) => provider.query("CALL two_results()"), honest.port, true); + const relabelled = await driverAnswer((provider) => provider.query("CALL two_results()"), labelling33.port); + const plain = await driverAnswer((provider) => provider.query("CALL two_results()"), honest.port); - type Answer = [Record, Record[], Record]; + // The rows arrive as arrays, values in column order (`rowsAsArray`). + type Answer = [Record, unknown[][], Record]; const [okRelabelled, rowsRelabelled, endRelabelled] = relabelled.header as unknown as Answer; const [okPlain, rowsPlain, endPlain] = plain.header as unknown as Answer; expect(okRelabelled.affectedRows).toBe(okPlain.affectedRows); expect(endRelabelled.affectedRows).toBe(endPlain.affectedRows); - expect(rowsRelabelled[0]?.legacy).toBe(TEXT); - expect(rowsPlain[0]?.legacy).toBe(TEXT.replace(EMOJI, "\uFFFD".repeat(4))); + expect(rowsRelabelled[0]?.[0]).toBe(TEXT); + expect(rowsPlain[0]?.[0]).toBe(TEXT.replace(EMOJI, "\uFFFD".repeat(4))); + // The one result set between the two OK packets is what the call answers. + expect(relabelled.result).toMatchObject({ fields: ["legacy"], rows: [{ legacy: TEXT }], rowCount: 1 }); const after = await relabelled.provider.query("SELECT v, b FROM t"); expect((after.rows[0] as Record).v).toBe(TEXT); @@ -659,3 +674,45 @@ describe("a server that refuses COM_STMT_PREPARE (Databend)", () => { expect(reads(refusing)).toEqual(["SELECT v, b FROM t WHERE v = 'x'"]); }); }); + +/** + * Two columns of one name keep two values over the real driver (D3), on every path a user's statement + * takes: the text protocol and the prepared one, each through mysql2's promise wrapper (a server that + * labels honestly) and through the relabelling callback path (a server that labels utf8mb3), and the + * client-side binding a server that refuses to prepare gets. mysql2's object rows keep only the last. + */ +describe("a result with two columns of one name", () => { + const expected = { fields: ["a", "a (2)"], rows: [{ a: "first", "a (2)": "second" }], rowCount: 1 }; + + test.each([ + ["the text protocol, promise path", () => honest.port, undefined], + ["the text protocol, relabelling path", () => labelling33.port, undefined], + ["the prepared protocol, promise path", () => honest.port, ["x"]], + ["the prepared protocol, relabelling path", () => labelling33.port, ["x"]], + ])("keeps both values over %s", async (_label, port, params) => { + const provider = await connected(port()); + expect(await provider.query("SELECT a, a FROM t", params)).toMatchObject(expected); + }); + + test("and in a transaction", async () => { + const provider = await connected(honest.port); + await provider.beginTransaction(); + const result = await provider.queryInTransaction("SELECT a, a FROM t"); + await provider.rollbackTransaction(); + expect(result).toMatchObject(expected); + }); + + test.each([ + ["labels text utf8mb4", UTF8MB4_UNICODE_CI], + ["labels text utf8mb3", UTF8MB3_GENERAL_CI], + ])("keeps both values when the values are bound client-side (server %s)", async (_label, textLabel) => { + const server = await startServer(textLabel, "refuses-prepare"); + try { + const provider = await connected(server.port); + expect(await provider.query("SELECT a, a FROM t WHERE v = ?", ["x"])).toMatchObject(expected); + expect(server.received).toContain("SELECT a, a FROM t WHERE v = 'x'"); + } finally { + server.close(); + } + }); +}); diff --git a/tests/integration/db/postgres-provider.test.ts b/tests/integration/db/postgres-provider.test.ts index 761ef67bb..cf48415ae 100644 --- a/tests/integration/db/postgres-provider.test.ts +++ b/tests/integration/db/postgres-provider.test.ts @@ -130,48 +130,96 @@ function answerParseProbe(): Promise<{ rows: unknown[] }> { ); } +/** + * A `pg` config asking for array rows. `query()` and `queryInTransaction()` send the user's statement + * as `{ text, rowMode: "array" }` with the values beside it, and `queryReadOnly()` adds the + * same `rowMode` to its `queryMode: "extended"` config. + */ +interface MockRowModeConfig { + text: string; + rowMode: "array"; + queryMode?: "extended"; +} + +type MockPgAnswer = { rows: unknown[]; fields?: { name: string; dataTypeID?: number }[]; rowCount?: number | null }; + +/** + * What `pg` answers under `rowMode: "array"`: each row holds its values in column order, and one + * answer per statement for a multi-statement simple query. A fixture written with object rows is + * read in the order its `fields` declare, or in its first row's key order when it declares none, + * which is what a real server's row description would have said. A fixture that already answers + * array rows is handed on unchanged, which is the only way to model two columns of one name. + */ +function asRowModeArray(answer: MockPgAnswer | MockPgAnswer[]): MockPgAnswer | MockPgAnswer[] { + if (Array.isArray(answer)) return answer.map((one) => asRowModeArray(one) as MockPgAnswer); + const rows = answer.rows ?? []; + const declared = answer.fields ?? []; + const fields = + declared.length > 0 || rows.length === 0 || Array.isArray(rows[0]) + ? declared + : Object.keys(rows[0] as object).map((name) => ({ name })); + return { + ...answer, + fields, + rows: rows.map((row) => + Array.isArray(row) ? row : fields.map((field) => (row as Record)[field.name]), + ), + }; +} + +/** The fixture server behind `mockClient.query`, answering what the provider sent. */ +function mockClientQuery(sql: string | MockParseConfig, params?: unknown[]): Promise { + mockWire.push(sql); + // A config carrying a `name` is a NAMED prepared statement, which is the only way + // node-postgres reaches Parse, and it is answered the way the server answers a Parse (D76). + // A config with no name is `queryMode: "extended"`, which the read-only profile sends and + // which goes to `mockQueryFn` exactly as it always did. + if (typeof sql !== "string" && typeof sql.name === "string") return answerParseProbe(); + // The two statements that end a transaction on the wire also end it here, so a + // test can observe the provider's rollback rather than only the call to it. + const ended = /^\s*(COMMIT|ROLLBACK)\s*;?\s*$/i.test(sql as string); + if (ended) mockTxStatus = "I"; + // A bare BEGIN opens one, which PostgreSQL reports as "T" on the very next + // ReadyForQuery. `mockBeginOpens = false` is the server that accepts BEGIN and opens + // nothing, the way RisingWave does. + if (mockBeginOpens && /^\s*BEGIN\s*;?\s*$/i.test(sql as string)) mockTxStatus = "T"; + // The aborted block the maintenance probe asks inside (#1387), as PostgreSQL keeps it: the + // poison aborts an open block, and every later statement but the ROLLBACK is refused with + // `25P02` after its parse, or with the grammar error a statement in `mockAbortedBlockRefusals` + // gets from a server that does not have it. + if (sql === "SELECT 1/0" && mockTxStatus === "T") { + mockTxStatus = "E"; + return Promise.reject(Object.assign(new Error("division by zero"), { code: "22012" })); + } + if (typeof sql === "string" && mockTxStatus === "E" && !ended) { + const refusal = mockAbortedBlockRefusals[sql]; + if (refusal === "resolves") return Promise.resolve({ rows: [] }); + return Promise.reject( + refusal ?? + Object.assign(new Error("current transaction is aborted, commands ignored until end of transaction block"), { + code: "25P02", + }), + ); + } + const answer = mockQueryFn(sql as string, params); + if (typeof sql !== "string") return answer; + // One result per statement, which is what `pg` hands back for a multi-statement simple + // query and what the apply's post-condition counts. + const count = mockApplyResultCount; + return sql.startsWith(APPLY_UNIT_PREFIX) && count !== "not-an-array" + ? answer.then((result) => Array.from({ length: count }, () => result)) + : answer; +} + const mockClient = { - query: (sql: string | MockParseConfig, params?: unknown[]) => { - mockWire.push(sql); - // A config carrying a `name` is a NAMED prepared statement, which is the only way - // node-postgres reaches Parse, and it is answered the way the server answers a Parse (D76). - // A config with no name is `queryMode: "extended"`, which the read-only profile sends and - // which goes to `mockQueryFn` exactly as it always did. - if (typeof sql !== "string" && typeof sql.name === "string") return answerParseProbe(); - // The two statements that end a transaction on the wire also end it here, so a - // test can observe the provider's rollback rather than only the call to it. - const ended = /^\s*(COMMIT|ROLLBACK)\s*;?\s*$/i.test(sql as string); - if (ended) mockTxStatus = "I"; - // A bare BEGIN opens one, which PostgreSQL reports as "T" on the very next - // ReadyForQuery. `mockBeginOpens = false` is the server that accepts BEGIN and opens - // nothing, the way RisingWave does. - if (mockBeginOpens && /^\s*BEGIN\s*;?\s*$/i.test(sql as string)) mockTxStatus = "T"; - // The aborted block the maintenance probe asks inside (#1387), as PostgreSQL keeps it: the - // poison aborts an open block, and every later statement but the ROLLBACK is refused with - // `25P02` after its parse, or with the grammar error a statement in `mockAbortedBlockRefusals` - // gets from a server that does not have it. - if (sql === "SELECT 1/0" && mockTxStatus === "T") { - mockTxStatus = "E"; - return Promise.reject(Object.assign(new Error("division by zero"), { code: "22012" })); - } - if (typeof sql === "string" && mockTxStatus === "E" && !ended) { - const refusal = mockAbortedBlockRefusals[sql]; - if (refusal === "resolves") return Promise.resolve({ rows: [] }); - return Promise.reject( - refusal ?? - Object.assign(new Error("current transaction is aborted, commands ignored until end of transaction block"), { - code: "25P02", - }), - ); - } - const answer = mockQueryFn(sql as string, params); - if (typeof sql !== "string") return answer; - // One result per statement, which is what `pg` hands back for a multi-statement simple - // query and what the apply's post-condition counts. - const count = mockApplyResultCount; - return sql.startsWith(APPLY_UNIT_PREFIX) && count !== "not-an-array" - ? answer.then((result) => Array.from({ length: count }, () => result)) - : answer; + query: (sql: string | MockParseConfig | MockRowModeConfig, params?: unknown[]) => { + const arrayRows = typeof sql !== "string" && (sql as MockRowModeConfig).rowMode === "array"; + // The user's statement on the simple protocol is unwrapped to the text it carries, so every + // fixture below keeps matching on the statement; the read-only profile's extended config is + // handed on whole, as it always was. + if (arrayRows && (sql as MockRowModeConfig).queryMode === undefined) sql = (sql as MockRowModeConfig).text; + const answered = mockClientQuery(sql as string | MockParseConfig, params); + return arrayRows ? answered.then((answer) => asRowModeArray(answer as MockPgAnswer | MockPgAnswer[])) : answered; }, getTransactionStatus: () => mockTxStatus, // `pg`'s client is an EventEmitter; `runMaintenance` listens for the server's notices (#1387). @@ -3397,8 +3445,9 @@ describe("PostgresProvider", () => { const built = new SessionClient(lastPoolConfig); for (const sent of startup) built.emit("notice", sent); session = Object.assign(built, { - query: async (sql: string, params?: unknown[]) => { - for (const sent of raised[sql] ?? []) built.emit("notice", sent); + query: async (sql: string | { text: string }, params?: unknown[]) => { + // The user's statement arrives as a `{ text, rowMode }` config, the provider's own as text. + for (const sent of raised[typeof sql === "string" ? sql : sql.text] ?? []) built.emit("notice", sent); return mockClient.query(sql, params); }, getTransactionStatus: mockClient.getTransactionStatus, @@ -4576,6 +4625,211 @@ describe("PostgresProvider declared column types", () => { }); }); +/** + * Every column keeps its own value (D3). `pg`'s object rows key a value by its column's name, so of two + * columns one name declares, the row keeps only the last: measured 2026-10-07 on PostgreSQL 16, the join + * below answered the customer's id under both `id` headers and the order's id nowhere. The provider asks + * for array rows and keys them by `uniqueFieldNames`, so a repeat is `id (2)`. The fixtures answer the + * array rows that run measured, so the mock cannot drop a value the driver would have kept. + */ +describe("PostgresProvider result columns", () => { + const join = { + rows: [ + [1, 100, "widget", 100, "Acme"], + [2, 200, "gadget", 200, "Globex"], + ], + fields: [ + { name: "id", dataTypeID: 23 }, + { name: "customer_id", dataTypeID: 23 }, + { name: "item", dataTypeID: 25 }, + { name: "id", dataTypeID: 23 }, + { name: "name", dataTypeID: 25 }, + ], + rowCount: 2, + }; + const JOIN_SQL = "SELECT * FROM orders o JOIN customers c ON c.id = o.customer_id"; + const joinFields = ["id", "customer_id", "item", "id (2)", "name"]; + const joinRows = [ + { id: 1, customer_id: 100, item: "widget", "id (2)": 100, name: "Acme" }, + { id: 2, customer_id: 200, item: "gadget", "id (2)": 200, name: "Globex" }, + ]; + + async function connected() { + const provider = new PostgresProvider(makePgConfig()); + await provider.connect(); + return provider; + } + + test("query() asks pg for array rows and keeps both values of a repeated name", async () => { + const sent: unknown[] = []; + const query = spyOn(mockClient, "query"); + mockQueryFn = (sql) => + Promise.resolve( + sql === "SELECT 1 AS a, 2 AS a" + ? { + rows: [[1, 2]], + fields: [ + { name: "a", dataTypeID: 23 }, + { name: "a", dataTypeID: 25 }, + ], + rowCount: 1, + } + : { rows: [], fields: [], rowCount: 0 }, + ); + try { + const provider = await connected(); + const result = await provider.query("SELECT 1 AS a, 2 AS a", [7]); + sent.push(...query.mock.calls.at(-1)!); + expect(result.fields).toEqual(["a", "a (2)"]); + expect(result.rows).toEqual([{ a: 1, "a (2)": 2 }]); + expect(result.columnTypes).toEqual({ a: "integer", "a (2)": "text" }); + expect(result.rowCount).toBe(1); + await provider.disconnect(); + } finally { + query.mockRestore(); + } + // The values still travel beside the config, so the protocol pg picks is the one it picked before. + expect(sent).toEqual([{ text: "SELECT 1 AS a, 2 AS a", rowMode: "array" }, [7]]); + }); + + test("query() keeps the order's id and the customer's id of a join apart", async () => { + mockQueryFn = () => Promise.resolve(join); + const provider = await connected(); + const result = await provider.query(JOIN_SQL); + expect(result.fields).toEqual(joinFields); + expect(result.rows).toEqual(joinRows); + expect(result.columnTypes).toEqual({ + id: "integer", + customer_id: "integer", + item: "text", + "id (2)": "integer", + name: "text", + }); + await provider.disconnect(); + }); + + test("query() numbers the unnamed columns PostgreSQL calls ?column?", async () => { + mockQueryFn = () => + Promise.resolve({ + rows: [[1, 2]], + fields: [ + { name: "?column?", dataTypeID: 23 }, + { name: "?column?", dataTypeID: 23 }, + ], + rowCount: 1, + }); + const provider = await connected(); + const result = await provider.query("SELECT 1, 2"); + expect(result.fields).toEqual(["?column?", "?column? (2)"]); + expect(result.rows).toEqual([{ "?column?": 1, "?column? (2)": 2 }]); + await provider.disconnect(); + }); + + test("queryInTransaction() reads the same way", async () => { + mockQueryFn = (sql) => Promise.resolve(sql === JOIN_SQL ? join : { rows: [], fields: [], rowCount: 0 }); + const provider = await connected(); + await provider.beginTransaction(); + const result = await provider.queryInTransaction(JOIN_SQL); + expect(result.fields).toEqual(joinFields); + expect(result.rows).toEqual(joinRows); + await provider.rollbackTransaction(); + await provider.disconnect(); + }); + + test("queryReadOnly() reads the same way, and measures its budget on the rows it answers", async () => { + const sent: unknown[] = []; + mockQueryFn = (arg) => { + sent.push(arg); + const text = typeof arg === "string" ? arg : (arg as unknown as { text: string }).text; + if (text === JOIN_SQL) return Promise.resolve(join); + return Promise.resolve({ + rows: [ + { is_superuser: false, reads_server_files: false, writes_server_files: false, executes_programs: false }, + ], + fields: [], + rowCount: 1, + }); + }; + const provider = new PostgresProvider(makePgConfig(), {}, { readOnly: true }); + await provider.connect(); + const result = await provider.queryReadOnly(JOIN_SQL, { + statementTimeoutMs: 4500, + maxResultRows: 10, + maxResultBytes: 10_000, + } as ReadOnlyStatementBudget); + expect(result.fields).toEqual(joinFields); + expect(result.rows).toEqual(joinRows); + expect(sent).toContainEqual({ text: JOIN_SQL, queryMode: "extended", rowMode: "array" }); + + // The byte budget is measured on the keyed rows, which are what the agent receives. + const tight = { statementTimeoutMs: 4500, maxResultRows: 10, maxResultBytes: 60 } as ReadOnlyStatementBudget; + await expect(provider.queryReadOnly(JOIN_SQL, tight)).rejects.toThrow("exceeded the byte budget"); + await provider.disconnect(); + }); + + test("a multi-statement text answers its first result set, and every set when it has several", async () => { + mockQueryFn = () => + Promise.resolve([ + { rows: [], fields: [], rowCount: 3 }, + join, + { rows: [[7]], fields: [{ name: "n", dataTypeID: 20 }], rowCount: 1 }, + { rows: [], fields: [], rowCount: null }, + ] as never); + const provider = await connected(); + const result = await provider.query("INSERT INTO t SELECT 1; SELECT * FROM j; SELECT 7 AS n; COMMIT"); + expect(result.fields).toEqual(joinFields); + expect(result.rows).toEqual(joinRows); + expect(result.rowCount).toBe(2); + expect(result.columnTypes?.["id (2)"]).toBe("integer"); + expect(result.resultSets).toEqual([ + { + rows: joinRows, + fields: joinFields, + columnTypes: { id: "integer", customer_id: "integer", item: "text", "id (2)": "integer", name: "text" }, + }, + { rows: [{ n: 7 }], fields: ["n"], columnTypes: { n: "bigint" } }, + ]); + await provider.disconnect(); + }); + + test("a multi-statement text with one result set answers it alone", async () => { + mockQueryFn = () => + Promise.resolve([{ rows: [], fields: [], rowCount: 1 }, join, { rows: [], fields: [], rowCount: null }] as never); + const provider = await connected(); + await provider.beginTransaction(); + const result = await provider.queryInTransaction("INSERT INTO t VALUES (1); SELECT * FROM j; COMMIT"); + expect(result.fields).toEqual(joinFields); + expect(result.rows).toEqual(joinRows); + expect(result.rowCount).toBe(2); + expect(Object.hasOwn(result, "resultSets")).toBe(false); + await provider.disconnect(); + }); + + test("a multi-statement text with no result set answers its first statement's count", async () => { + mockQueryFn = () => + Promise.resolve([ + { rows: [], fields: [], rowCount: 2 }, + { rows: [], fields: [], rowCount: 5 }, + ] as never); + const provider = await connected(); + const result = await provider.query("INSERT INTO t VALUES (1), (2); DELETE FROM u"); + expect(result.rows).toEqual([]); + expect(result.fields).toEqual([]); + expect(result.rowCount).toBe(2); + expect(Object.hasOwn(result, "resultSets")).toBe(false); + await provider.disconnect(); + }); + + test("a row whose value count differs from the column count is an error, not a guess", async () => { + mockQueryFn = () => Promise.resolve({ rows: [[1]], fields: [{ name: "a" }, { name: "b" }], rowCount: 1 }); + const provider = await connected(); + await expect(provider.query("SELECT a, b FROM t")).rejects.toThrow( + "PostgreSQL answered a row of 1 values for 2 columns", + ); + await provider.disconnect(); + }); +}); + // ============================================================================ // The connect-time EXPLAIN grammar probe (#597) // ============================================================================ diff --git a/tests/isolated/agent-investigation-e2e.test.ts b/tests/isolated/agent-investigation-e2e.test.ts index e0c440cec..ea9e2b76a 100644 --- a/tests/isolated/agent-investigation-e2e.test.ts +++ b/tests/isolated/agent-investigation-e2e.test.ts @@ -92,7 +92,7 @@ function pgServerError(message: string, code: string): Error { } interface EngineAnswer { - readonly rows: Record[]; + readonly rows: Record[] | unknown[][]; readonly fields: { name: string }[]; readonly rowCount: number; } @@ -209,7 +209,18 @@ class PostgresEngineDouble { /** Statements that arrived OUTSIDE the read-only envelope. Must stay empty. */ readonly unenveloped: string[] = []; + /** + * `pg` under `rowMode: "array"`, which the provider asks for on the user's statement: each row + * its values in the order `fields` declares. + */ async query(arg: unknown): Promise { + const answer = await this.answer(arg); + if (typeof arg === "string" || (arg as { rowMode?: string }).rowMode !== "array") return answer; + const rows = answer.rows as Record[]; + return { ...answer, rows: rows.map((row) => answer.fields.map((field) => row[field.name])) }; + } + + private async answer(arg: unknown): Promise { const text = (typeof arg === "string" ? arg : (arg as { text: string }).text).trim(); // The role-privilege probe the profile runs at open, before any transaction. From 69d3b8521cc2b2149253da6c89e9ae5edda590bf Mon Sep 17 00:00:00 2001 From: cevheri Date: Wed, 7 Oct 2026 11:41:58 +0300 Subject: [PATCH 05/46] fix(db): pin the MySQL empty column name and correct the result-shape docs MySQL answers an empty column name for SELECT '' and SELECT 1 AS ''; the mysql doc claimed it never does. State the measured before and after, and pin SELECT '', '' in a mock and a wire test. Word the PostgreSQL multi-result rule by what the code tests (a statement that answers no column), and stop claiming mysql2 lets a __proto__ column through. --- docs/providers/mysql.md | 8 +++++--- docs/providers/postgres.md | 2 +- src/lib/db/providers/sql/mysql.ts | 3 ++- src/lib/db/providers/sql/postgres.ts | 4 ++-- tests/integration/db/mysql-provider.test.ts | 19 ++++++++++++++++++ .../db/mysql-wire-decoding.test.ts | 20 +++++++++++++++++++ 6 files changed, 49 insertions(+), 7 deletions(-) diff --git a/docs/providers/mysql.md b/docs/providers/mysql.md index 66b02b46a..8bd3b1300 100644 --- a/docs/providers/mysql.md +++ b/docs/providers/mysql.md @@ -270,8 +270,8 @@ against three live servers: One behaviour does differ, and only for a connection that opted into `multipleStatements=true` in its connection string: a `;`-separated statement is rejected by the prepared protocol and accepted by the text one, which then answers an array of result sets. That is the shape `CALL ()` already -answers on both protocols, and the envelope reads it the same way ([§5.1](#51-execution)); the app splits multi-statement -input itself (`POST /api/db/multi-query`) and issues one statement per call. +answers on both protocols, and the envelope reads it the same way ([§5.1](#51-execution)); the app +splits multi-statement input itself (`POST /api/db/multi-query`) and issues one statement per call. `rowCount` is `rows.length` **only when the driver returns a row array** (i.e. `SELECT`); for a non-`SELECT` statement mysql2 returns a `ResultSetHeader` rather than an array, and the provider @@ -674,9 +674,11 @@ Measured on MySQL 8.4 through `mysql2` 3.24.5 on 2026-10-07, before and after: | `SELECT 1 AS a, 2 AS a` | fields `["a","a"]`, row `{"a":2}` | fields `["a","a (2)"]`, row `{"a":1,"a (2)":2}` | | `SELECT 1, 1` (MySQL names an unaliased expression by its text) | fields `["1","1"]`, row `{"1":1}` | fields `["1","1 (2)"]`, row `{"1":1,"1 (2)":1}` | | `SELECT * FROM orders o JOIN customers c ON c.id = o.customer_id` | fields `["id","customer_id","item","id","name"]`, the customer's id under both `id` headers, the order's id gone | fields `["id","customer_id","item","id (2)","name"]`, the order's id under `id` and the customer's under `id (2)` | +| `SELECT ''` (MySQL names a column holding an empty string literal by it, and `SELECT 1 AS ''` names it empty too) | fields `[""]`, which the results grid cannot take as a column id | fields `["(No column name)"]`, row `{"(No column name)":""}` | +| `SELECT '', ''` | fields `["",""]`, one value under `""` | fields `["(No column name)","(No column name) (2)"]`, both values | | `CALL sys.ps_setup_show_enabled(FALSE, FALSE)` | `TypeError: undefined is not an object (evaluating 'f.name')`, after the procedure had run | the first of its four result sets, and all four in `resultSets` | -MySQL never answers an empty column name, so an unnamed column is only ever a repeated one here. +An empty column name is named `(No column name)`, as `uniqueFieldNames` names it for every engine. **A `CALL` answers a list of result sets.** `mysql2` answers one result set per SELECT the procedure ran and the call's own OK packet last, with the field packets one list per set and `undefined` for each header. diff --git a/docs/providers/postgres.md b/docs/providers/postgres.md index f5b062101..b7dcbcc55 100644 --- a/docs/providers/postgres.md +++ b/docs/providers/postgres.md @@ -1342,7 +1342,7 @@ Measured on PostgreSQL 16 through `pg` 8.23.1 on 2026-10-07, before and after: PostgreSQL never answers an empty column name: an unaliased expression is named by the server (`?column?`, `count`, `now`), so an unnamed column is only ever a repeated one here. A text of several statements sent as one simple query (`SELECT 1 AS a; SELECT 2 AS b, 3 AS b`) answers one result per statement. -`rows`, `fields` and `columnTypes` are the first result set's, `rowCount` is the count of the statement that produced it (or of the first statement when none produced one), and `resultSets` lists every set when there are several; a statement with no row description (an INSERT, a COMMIT) is not a set. +`rows`, `fields` and `columnTypes` are the first result set's, `rowCount` is the count of the statement that produced it (or of the first statement when none produced one), and `resultSets` lists every set when there are several; a statement that answers no column (an INSERT, a COMMIT, or a `SELECT` of no columns) is not a set. Measured the same day, that text answers `rows` `[{"a":1}]` and two `resultSets`, the second with fields `["b","b (2)"]`. Before, such a text answered no `rows` at all, because the list of answers was read as one answer. diff --git a/src/lib/db/providers/sql/mysql.ts b/src/lib/db/providers/sql/mysql.ts index e32fd0491..bb8a3ddf0 100644 --- a/src/lib/db/providers/sql/mysql.ts +++ b/src/lib/db/providers/sql/mysql.ts @@ -399,7 +399,8 @@ const probeClientSideBinding = async (queryable: MySQLQueryable): Promise field.name)); diff --git a/src/lib/db/providers/sql/postgres.ts b/src/lib/db/providers/sql/postgres.ts index 0cda34e74..13e5d9d27 100644 --- a/src/lib/db/providers/sql/postgres.ts +++ b/src/lib/db/providers/sql/postgres.ts @@ -338,8 +338,8 @@ function pgResultSet(answer: PgArrayAnswer): QueryResultSet { * The result of what `pg` answered a user's text with. * * One statement answers one result. A text of several statements on the simple protocol answers - * one per statement, and a statement with no row description (an INSERT, a COMMIT) carries no - * result set. Then, as `QueryResult.resultSets` says, `rows`, `fields` and `columnTypes` are the + * one per statement, and a statement that answers no column (an INSERT, a COMMIT, or a `SELECT` of + * no columns) carries no result set. Then, as `QueryResult.resultSets` says, `rows`, `fields` and `columnTypes` are the * first set's and `rowCount` the count of the statement that produced it, or of the first * statement when none did; every set is listed when there are several. Before this read such a * text answered no `rows` at all, because the answer was an array and was read as one result. diff --git a/tests/integration/db/mysql-provider.test.ts b/tests/integration/db/mysql-provider.test.ts index 0e492537a..d4c8707f1 100644 --- a/tests/integration/db/mysql-provider.test.ts +++ b/tests/integration/db/mysql-provider.test.ts @@ -3427,6 +3427,25 @@ describe("MySQLProvider result columns", () => { } }); + test("query() names an empty column name, which MySQL answers for SELECT '', and keeps both values", async () => { + // Measured on MySQL 8.4: `SELECT '', ''` declares two columns named "" and, read as object + // rows, kept one value under the key "". + mockExecuteFn = () => + Promise.resolve([ + [["", ""]], + [ + { name: "", ...text }, + { name: "", ...text }, + ], + ]); + const provider = await connected(); + const result = await provider.query("SELECT '', ''"); + expect(result.fields).toEqual(["(No column name)", "(No column name) (2)"]); + expect(result.rows).toEqual([{ "(No column name)": "", "(No column name) (2)": "" }]); + expect(result.columnTypes).toEqual({ "(No column name)": "text", "(No column name) (2)": "text" }); + await provider.disconnect(); + }); + test("query() keeps the order's id and the customer's id of a join apart, over the prepared protocol too", async () => { const execute = spyOn(mockConnection, "execute"); mockExecuteFn = () => Promise.resolve(join); diff --git a/tests/integration/db/mysql-wire-decoding.test.ts b/tests/integration/db/mysql-wire-decoding.test.ts index 0e1d557f7..c5ec6cc11 100644 --- a/tests/integration/db/mysql-wire-decoding.test.ts +++ b/tests/integration/db/mysql-wire-decoding.test.ts @@ -219,6 +219,14 @@ const startServer = (textLabel: number, binding: Binding = "prepares"): Promise< [definition({ name: "bound", characterSet: textLabel, columnType: VAR_STRING, flags: 0 })], ); } + } else if (sql === "SELECT '', ''") { + // MySQL names a column holding an empty string literal by that literal: an empty name. + connection.writeColumns([ + definition({ name: "", characterSet: textLabel, columnType: VAR_STRING, flags: 0 }), + definition({ name: "", characterSet: textLabel, columnType: VAR_STRING, flags: 0 }), + ]); + connection.writeTextRow(["", ""]); + connection.writeEof(); } else if (sql.startsWith("SELECT '")) { connection.writeTextResult( [{ probe: EMOJI }], @@ -694,6 +702,18 @@ describe("a result with two columns of one name", () => { expect(await provider.query("SELECT a, a FROM t", params)).toMatchObject(expected); }); + test.each([ + ["the promise path", () => honest.port], + ["the relabelling path", () => labelling33.port], + ])("names two empty column names apart and keeps both values over %s", async (_label, port) => { + const provider = await connected(port()); + expect(await provider.query("SELECT '', ''")).toMatchObject({ + fields: ["(No column name)", "(No column name) (2)"], + rows: [{ "(No column name)": "", "(No column name) (2)": "" }], + rowCount: 1, + }); + }); + test("and in a transaction", async () => { const provider = await connected(honest.port); await provider.beginTransaction(); From 363590955c29341dc7c193596d5249e9e96b22e9 Mon Sep 17 00:00:00 2001 From: cevheri Date: Wed, 7 Oct 2026 11:10:30 +0300 Subject: [PATCH 06/46] fix(sqlite): read result rows by position so a repeated or empty column name keeps its value The provider keyed each row by column name and took fields from the first row's keys, so SELECT 1 AS a, 2 AS a answered one column holding 2 and SELECT 1 AS "" answered a column named "". Both drivers now read user results as arrays (bun:sqlite values(), node:sqlite setReturnArrays) and key them by uniqueFieldNames, on query() and queryReadOnly(), with each declared type paired to its column by position. An empty result now names its columns. keyRowsByPosition is the shared row builder, and refuses a row whose value count differs from the column count. --- docs/providers/sqlite.md | 28 ++++- src/lib/db/providers/sql/sqlite-driver.ts | 51 +++++++- src/lib/db/providers/sql/sqlite.ts | 74 +++++++----- src/lib/db/utils/positional-rows.ts | 35 ++++++ tests/integration/db/sqlite-provider.test.ts | 115 +++++++++++++++++-- tests/unit/db/positional-rows.test.ts | 41 +++++++ tests/unit/db/sqlite-driver.test.ts | 45 +++++++- 7 files changed, 339 insertions(+), 50 deletions(-) create mode 100644 src/lib/db/utils/positional-rows.ts create mode 100644 tests/unit/db/positional-rows.test.ts diff --git a/docs/providers/sqlite.md b/docs/providers/sqlite.md index 97aa816c3..e471beed0 100644 --- a/docs/providers/sqlite.md +++ b/docs/providers/sqlite.md @@ -204,7 +204,7 @@ used to leave the handle held, so the user could not delete or move the file the ### 3.3 Read vs write dispatch `query()` ([`sqlite.ts`](../../src/lib/db/providers/sql/sqlite.ts)) prepares the statement and -branches on `stmt.returnsRows()`: a statement with result columns uses `stmt.all()` and returns rows; +branches on `stmt.returnsRows()`: a statement with result columns uses `stmt.values()` and returns rows ([Result column names](#result-column-names)); one without uses `stmt.run()` and returns `{ changes }`. `rowCount = rows.length || changes`. Both drivers are **synchronous**: the provider wraps them in the async signature but there is no real concurrency or cancellation. @@ -217,8 +217,8 @@ method. So the branch does not depend on how the statement is spelled: | Statement | Branch | Result | |---|---|---| -| `SELECT ...`, `WITH ... SELECT`, `WITH RECURSIVE ...`, `VALUES (1), (2)`, `EXPLAIN ...`, `PRAGMA journal_mode` | `all()` | the rows | -| `INSERT` / `UPDATE` / `DELETE ... RETURNING ...` | `all()` | the returned rows, and the write is applied; `rowCount` is the number of rows returned | +| `SELECT ...`, `WITH ... SELECT`, `WITH RECURSIVE ...`, `VALUES (1), (2)`, `EXPLAIN ...`, `PRAGMA journal_mode` | `values()` | the rows | +| `INSERT` / `UPDATE` / `DELETE ... RETURNING ...` | `values()` | the returned rows, and the write is applied; `rowCount` is the number of rows returned | | plain `INSERT` / `UPDATE` / `DELETE` (CTE-led ones included), DDL, `BEGIN`, `PRAGMA user_version = 3` | `run()` | no rows, `rowCount` from `changes` | The router used to be the inherited `isReadOnlyQuery(sql)`, a leading-keyword set of `SELECT`, @@ -305,7 +305,7 @@ spells the request its own way — bun `safeIntegers`, node `readBigInts` — bu 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 +nothing), wrapped by `withoutBigInts()` so `all()`, `values()`, `get()` and `run()` are covered alike. Measured through both adapters in the same pass, with identical answers: | Value read | Answered as | @@ -485,7 +485,7 @@ Homebrew). Each channel is browser-verified by ## 5. Query interface -`query(sql, params?)` — positional params via the driver's `all()`/`run()`. There is no +`query(sql, params?)` — positional params via the driver's `values()`/`run()`. There is no `prepareQuery()` override, so the inherited base injects a `LIMIT` into bare `SELECT`s (`DEFAULT_QUERY_LIMIT = 500`). No transactions, no cancellation ([§3.4](#34-no-transactions-api-no-cancellation-no-pool)). @@ -570,6 +570,24 @@ A 64-bit id is where the two features meet: it leaves as the decimal string declared `INTEGER`, so the export writes an `INTEGER` column rather than the `TEXT` a value-shaped guess would produce. +### Result column names + +Every result column comes back under a name that is non-empty and that no other column of the result has, with its own value. +The rows are read as arrays (bun:sqlite `values()`, node:sqlite `all()` after `setReturnArrays(true)`, bridged as one `values()` in [`sqlite-driver.ts`](../../src/lib/db/providers/sql/sqlite-driver.ts)) and keyed by position under `uniqueFieldNames` of the declared names ([`result-fields.ts`](../../src/lib/db/utils/result-fields.ts)). +Measured 2026-10-07 on both drivers in-process (Bun 1.4.2), pinned in `tests/integration/db/sqlite-provider.test.ts`: + +| Statement | `fields` | Row | +|---|---|---| +| `SELECT 1 AS a, 2 AS a` | `a`, `a (2)` | `{ a: 1, "a (2)": 2 }` | +| `SELECT * FROM orders o JOIN customers c ON c.id = o.customer_id` | `id`, `customer_id`, `item`, `id (2)`, `name` | the order's id under `id`, the customer's under `id (2)` | +| `SELECT 1 AS ""` | `(No column name)` | `{ "(No column name)": 1 }` | +| `SELECT 4 + 5` | `4 + 5` | SQLite names an unaliased expression by its text, so only an empty alias is ever unnamed | + +Before this, `fields` was the keys of the first row object, so a repeated name was one column holding the last value and an empty alias was a column named `""`. +A declared type is paired with its column by position, so `id (2)` above carries the customer table's `INTEGER`. +An empty result names its columns too, because the names come from the statement and not from a first row. +The agent's `queryReadOnly()` reads the same way. + ### BLOB values Both drivers read a `BLOB` as a plain `Uint8Array`, and the rows reach the browser through diff --git a/src/lib/db/providers/sql/sqlite-driver.ts b/src/lib/db/providers/sql/sqlite-driver.ts index 494e5f165..ab362197d 100644 --- a/src/lib/db/providers/sql/sqlite-driver.ts +++ b/src/lib/db/providers/sql/sqlite-driver.ts @@ -28,14 +28,23 @@ import { fitsJavaScriptNumber, isSQLiteInt64Digits } from "./sqlite-int64"; * 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. + * declare two columns called `c`. The provider names them apart with `uniqueFieldNames` + * and pairs each type with its column by position, so each keeps its own type. */ 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[]; + /** + * The rows as arrays of values, one per result column in column order: the one read that keeps + * every column. `all()` keys each row by column name, so `SELECT 1 AS a, 2 AS a` answers one key + * holding 2 on both drivers, and the provider reads every user result through this instead. + * Both drivers publish it, bun:sqlite as `values()` and node:sqlite as `all()` after + * `setReturnArrays(true)`, so the two are bridged below. The names come from `declaredColumns()`, + * read after this call for the reason that method states. + */ + values(...params: unknown[]): unknown[][]; get(...params: unknown[]): unknown; run(...params: unknown[]): { changes: number }; /** @@ -300,11 +309,11 @@ type RawSQLiteStatement = Omit { + const rows = stmt.values(...toSQLiteBindValues(params)); + // An array row is walked by index exactly as an object row is walked by key. + for (const row of rows) { + normalizeRecordInPlace(row); + } + return rows; + }, get: (...params: unknown[]): unknown => normalizeRecordInPlace(stmt.get(...toSQLiteBindValues(params))), run: (...params: unknown[]): { changes: number } => { // `changes` is a row count and always fits; `lastInsertRowid` (bun publishes it) @@ -360,7 +377,13 @@ export type BunSQLiteOpenOptions = SQLiteOpenOptions & { safeIntegers?: boolean * 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 & { +type BunStatementLike = Omit & { + /** + * bun answers `null`, not `[]`, for a statement with no result columns: measured 2026-10-07 on + * Bun 1.4.2, `values()` on `PRAGMA query_only = false` or a plain INSERT is `null` where `all()` + * is `[]`. The adapter below reads that as the empty row list it means. + */ + values(...params: unknown[]): unknown[][] | null; readonly columnNames: string[]; readonly declaredTypes: (string | null)[]; }; @@ -387,11 +410,17 @@ export function createBunSQLiteDriver(DatabaseCtor: BunSQLiteConstructor): SQLit prepare(sql: string): SQLiteStatement { const stmt = this.db.prepare(sql); + const raw: RawSQLiteStatement = { + all: (...params) => stmt.all(...params), + values: (...params) => stmt.values(...params) ?? [], + get: (...params) => stmt.get(...params), + run: (...params) => stmt.run(...params), + }; // 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, + raw, () => stmt.columnNames.map((name, index) => [name, stmt.declaredTypes[index] ?? undefined] as const), // `columnNames`, unlike `declaredTypes`, is filled at prepare time. () => stmt.columnNames.length > 0, @@ -414,6 +443,8 @@ export function createBunSQLiteDriver(DatabaseCtor: BunSQLiteConstructor): SQLit // tests never need the real module, which Bun does not implement). type NodeStatementLike = { all(...params: unknown[]): unknown; + /** node:sqlite's switch between object rows and array rows; `values()` below turns it on for one read. */ + setReturnArrays(enabled: boolean): void; get(...params: unknown[]): unknown; run(...params: unknown[]): { changes: number | bigint }; /** @@ -515,6 +546,14 @@ export function createNodeSQLiteDriver(DatabaseSyncCtor: NodeSQLiteModule["Datab return withoutBigInts( { all: (...params: unknown[]): unknown[] => stmt.all(...params) as unknown[], + values: (...params: unknown[]): unknown[][] => { + stmt.setReturnArrays(true); + try { + return stmt.all(...params) as unknown[][]; + } finally { + stmt.setReturnArrays(false); + } + }, get: (...params: unknown[]): unknown => stmt.get(...params) ?? null, run: (...params: unknown[]): { changes: number } => { const info = stmt.run(...params); diff --git a/src/lib/db/providers/sql/sqlite.ts b/src/lib/db/providers/sql/sqlite.ts index 83ac87455..722720305 100644 --- a/src/lib/db/providers/sql/sqlite.ts +++ b/src/lib/db/providers/sql/sqlite.ts @@ -47,7 +47,7 @@ import { } from "../../errors"; import { assertReadOnlyBudget, measureResultBytes } from "./read-only-budget"; import { formatBytes } from "../../utils/pool-manager"; -import { loadSQLiteDriver, type SQLiteDatabase } from "./sqlite-driver"; +import { loadSQLiteDriver, type SQLiteDatabase, type SQLiteStatement } from "./sqlite-driver"; import { declaredColumnTypes } from "./column-types"; import { applySourceBound, @@ -67,6 +67,8 @@ import { logger } from "@/lib/logger"; import * as fs from "fs"; import * as path from "path"; import { isUnwritableExistingFile } from "@/lib/db/utils/unwritable-file"; +import { uniqueFieldNames } from "@/lib/db/utils/result-fields"; +import { keyRowsByPosition } from "@/lib/db/utils/positional-rows"; import { isReservedStoragePath } from "@/lib/data-dir"; /** @@ -1005,6 +1007,44 @@ export function readDbstatSizes(db: SQLiteDatabase): Map[]; fields: string[]; declared: Pick } { + const values = stmt.values(...params); + const columns = stmt.declaredColumns(); + const fields = uniqueFieldNames(columns.map(([name]) => name)); + return { + rows: keyRowsByPosition(fields, values, "sqlite", sql), + fields, + declared: declaredColumnTypes(columns.map(([, type], index) => [fields[index], type] as const)), + }; +} + /** * Build one table's stats row. `size` is `null` when this driver publishes no page * bytes, and then the byte fields are OMITTED rather than zeroed: a 0 reads as an @@ -1358,23 +1398,10 @@ export class SQLiteProvider extends SQLBaseProvider { const stmt = this.db!.prepare(sql); // Routed on what SQLite compiled, not on the leading keyword: a statement with - // result columns is read with `all()`, whatever it starts with. A keyword set + // result columns is read for its rows, whatever it starts with. A keyword set // missed WITH, VALUES and every `... RETURNING`, and `run()` dropped their rows. if (stmt.returnsRows()) { - const rows = params ? stmt.all(...params) : stmt.all(); - const fields = rows.length > 0 ? Object.keys(rows[0] as object) : []; - return { - 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()), - }; + return { ...readResultRows(stmt, sql, params ?? []), changes: 0 }; } else { const info = params ? stmt.run(...params) : stmt.run(); return { @@ -1488,18 +1515,13 @@ export class SQLiteProvider extends SQLBaseProvider { return this.trackQuery(async () => { const { - result: { rows, declared }, + result: { rows, fields, declared }, executionTime, } = await this.measureExecution(async () => { try { - 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()) }; + // 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. + return readResultRows(this.db!.prepare(sql), sql, []); } catch (error) { throw mapDatabaseError(error, "sqlite", sql); } @@ -1535,7 +1557,7 @@ export class SQLiteProvider extends SQLBaseProvider { return { rows, - fields: rows.length > 0 ? Object.keys(rows[0]) : [], + fields, rowCount: rows.length, executionTime, ...declared, diff --git a/src/lib/db/utils/positional-rows.ts b/src/lib/db/utils/positional-rows.ts new file mode 100644 index 000000000..90bc3f741 --- /dev/null +++ b/src/lib/db/utils/positional-rows.ts @@ -0,0 +1,35 @@ +import { QueryError } from "../errors"; +import type { DatabaseType } from "@/lib/types"; + +/** + * Key each row a driver read positionally by the result's column names, one value per name, in order. + * + * `fields` are the names `uniqueFieldNames` gave the declared columns, so a repeated or unnamed column + * lands under a name of its own instead of overwriting another column's value, which is what reading the + * rows keyed by the declared names did. + * + * `Object.fromEntries` rather than assignment into a literal, because a column name is arbitrary SQL output + * and `row["__proto__"] = v` replaces the prototype instead of adding a key. + * + * A row whose value count is not the column count is refused: padding or cutting it would show a value + * under another column's name, and nothing downstream could tell. + * + * Server only, and it names no engine. + */ +export function keyRowsByPosition( + fields: readonly string[], + valueRows: readonly (readonly unknown[])[], + provider: DatabaseType, + sql: string, +): Record[] { + return valueRows.map((values, index) => { + if (values.length !== fields.length) { + throw new QueryError( + `Row ${index + 1} carries ${values.length} values for ${fields.length} result columns`, + provider, + sql, + ); + } + return Object.fromEntries(fields.map((field, position) => [field, values[position]])); + }); +} diff --git a/tests/integration/db/sqlite-provider.test.ts b/tests/integration/db/sqlite-provider.test.ts index 169b22862..f7498a8f1 100644 --- a/tests/integration/db/sqlite-provider.test.ts +++ b/tests/integration/db/sqlite-provider.test.ts @@ -302,14 +302,14 @@ describe("SQLiteProvider", () => { expect(select.rows).toEqual([{ id: 1, name: "Ann" }]); }); - test("SELECT with no matching rows returns empty rows and fields", async () => { + test("SELECT with no matching rows returns no rows and still names its columns", async () => { provider = new SQLiteProvider(makeSQLiteConfig()); await provider.connect(); await provider.query("CREATE TABLE empty_result (id INTEGER PRIMARY KEY)"); const result = await provider.query("SELECT * FROM empty_result WHERE id = 999"); expect(result.rows).toEqual([]); - expect(result.fields).toEqual([]); + expect(result.fields).toEqual(["id"]); expect(result.rowCount).toBe(0); }); @@ -818,6 +818,7 @@ describe("SQLiteProvider", () => { prepare: (sql: string) => ({ all: () => (sql.includes("dbstat") ? dbstat : owners), get: () => null, + values: () => [], run: () => ({ changes: 0 }), declaredColumns: () => [], returnsRows: () => true, @@ -1421,6 +1422,7 @@ function answerReadsMatching(provider: SQLiteProvider, match: string, rows: read interceptReads(provider, match, () => ({ all: () => [...rows], get: () => rows[0] ?? null, + values: () => [], run: () => ({ changes: 0 }), declaredColumns: () => [], returnsRows: () => true, @@ -1449,6 +1451,7 @@ function captureReadsMatching( return [...rows]; }, get: () => rows[0] ?? null, + values: () => [], run: () => ({ changes: 0 }), declaredColumns: () => [], returnsRows: () => true, @@ -3513,6 +3516,92 @@ describe.each(["bun", "node"] as const)("SQLiteProvider row-returning statements }); }); +// ============================================================================ +// Result column names: an unnamed or repeated column keeps its own value +// +// The rows were read keyed by name and `fields` was `Object.keys(rows[0])`, so +// `SELECT 1 AS a, 2 AS a` answered one column holding 2, and `SELECT 1 AS ""` answered +// `fields: [""]`, a column no grid can key. The rows are now read positionally on both +// drivers and keyed by `uniqueFieldNames`. + +describe.each(["bun", "node"] as const)("SQLiteProvider result column names on the %s driver", (driver) => { + const originalDriverEnv = process.env.LIBREDB_SQLITE_DRIVER; + let namesTmpDir: string; + let namesProvider: SQLiteProvider; + let namesAgent: SQLiteProvider | null = null; + + beforeEach(async () => { + process.env.LIBREDB_SQLITE_DRIVER = driver; + namesTmpDir = mkdtempSync(join(tmpdir(), "libredb-sqlite-names-")); + namesProvider = new SQLiteProvider( + makeSQLiteConfig({ id: `names-${driver}`, database: join(namesTmpDir, "n.db") }), + ); + await namesProvider.connect(); + await namesProvider.query("CREATE TABLE orders (id INTEGER PRIMARY KEY, customer_id INTEGER, item TEXT)"); + await namesProvider.query("CREATE TABLE customers (id INTEGER PRIMARY KEY, name TEXT)"); + await namesProvider.query("INSERT INTO orders VALUES (100, 1, 'book')"); + await namesProvider.query("INSERT INTO customers VALUES (1, 'Ann')"); + }); + + afterEach(async () => { + if (namesAgent?.isConnected()) await namesAgent.disconnect(); + namesAgent = null; + await namesProvider.disconnect(); + rmSync(namesTmpDir, { recursive: true }); + if (originalDriverEnv === undefined) delete process.env.LIBREDB_SQLITE_DRIVER; + else process.env.LIBREDB_SQLITE_DRIVER = originalDriverEnv; + }); + + test("a repeated alias keeps both values under two names", async () => { + const result = await namesProvider.query("SELECT 1 AS a, 2 AS a"); + + expect(result.fields).toEqual(["a", "a (2)"]); + expect(result.rows).toEqual([{ a: 1, "a (2)": 2 }]); + }); + + test("a join that projects id from both tables keeps the order's id", async () => { + const result = await namesProvider.query("SELECT * FROM orders o JOIN customers c ON c.id = o.customer_id"); + + expect(result.fields).toEqual(["id", "customer_id", "item", "id (2)", "name"]); + expect(result.rows).toEqual([{ id: 100, customer_id: 1, item: "book", "id (2)": 1, name: "Ann" }]); + expect(result.columnTypes).toEqual({ + id: "INTEGER", + customer_id: "INTEGER", + item: "TEXT", + "id (2)": "INTEGER", + name: "TEXT", + }); + }); + + test("an empty alias is named, and an unaliased expression keeps the name SQLite gives it", async () => { + const result = await namesProvider.query('SELECT 1 AS "", 4 + 5'); + + expect(result.fields).toEqual(["(No column name)", "4 + 5"]); + expect(result.rows).toEqual([{ "(No column name)": 1, "4 + 5": 9 }]); + }); + + test("an empty result still names its columns", async () => { + const result = await namesProvider.query("SELECT id, id FROM orders WHERE id = -1"); + + expect(result.rows).toEqual([]); + expect(result.fields).toEqual(["id", "id (2)"]); + }); + + test("the read-only profile keys a repeated column the same way", async () => { + namesAgent = new SQLiteProvider( + makeSQLiteConfig({ id: `names-agent-${driver}`, database: join(namesTmpDir, "n.db") }), + {}, + { readOnly: true }, + ); + await namesAgent.connect(); + + const result = await namesAgent.queryReadOnly('SELECT 1 AS a, 2 AS a, 3 AS ""', AGENT_BUDGET); + + expect(result.fields).toEqual(["a", "a (2)", "(No column name)"]); + expect(result.rows).toEqual([{ a: 1, "a (2)": 2, "(No column name)": 3 }]); + }); +}); + // ============================================================================ // Node driver (LIBREDB_SQLITE_DRIVER=node -> node:sqlite) // @@ -4292,11 +4381,11 @@ describe("declared column types (#273)", () => { 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. + // The names and the types both come from the statement, not from a row, so a result + // with no rows is named and described alike. A value-shaped guess could never answer + // this case at all. expect(result.rows).toEqual([]); - expect(result.fields).toEqual([]); + expect(result.fields).toEqual(["id", "price"]); expect(result.columnTypes).toEqual({ id: "INTEGER", price: "REAL" }); }); @@ -4307,13 +4396,15 @@ describe("declared column types (#273)", () => { 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"); + test("two result columns of one name each keep their own value and type", async () => { + const result = await declared.query("SELECT label AS c, 1 AS c, id 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" }); + // SQLite really does answer three columns called `c`. Each is named apart and keyed + // by position, so each keeps its value, and its type is paired with it by position: + // the undeclared literal declares none, and the third column is not shown the first's. + expect(result.fields).toEqual(["c", "c (2)", "c (3)"]); + expect(result.rows).toEqual([{ c: "first", "c (2)": 1, "c (3)": 1 }]); + expect(result.columnTypes).toEqual({ c: "TEXT", "c (3)": "INTEGER" }); }); /** diff --git a/tests/unit/db/positional-rows.test.ts b/tests/unit/db/positional-rows.test.ts new file mode 100644 index 000000000..6a468ccc9 --- /dev/null +++ b/tests/unit/db/positional-rows.test.ts @@ -0,0 +1,41 @@ +/** + * A result row read positionally and keyed by the result's unique column names, so a + * repeated or unnamed column keeps its own value. + */ +import { describe, expect, test } from "bun:test"; +import { QueryError } from "@/lib/db/errors"; +import { keyRowsByPosition } from "@/lib/db/utils/positional-rows"; + +describe("keyRowsByPosition", () => { + test("each value lands under the name at its own position", () => { + expect( + keyRowsByPosition( + ["a", "a (2)"], + [ + [1, 2], + [3, 4], + ], + "sqlite", + "SELECT 1 AS a, 2 AS a", + ), + ).toEqual([ + { a: 1, "a (2)": 2 }, + { a: 3, "a (2)": 4 }, + ]); + expect(keyRowsByPosition(["a"], [], "sqlite", "SELECT 1 AS a WHERE 0")).toEqual([]); + }); + + test("a column named __proto__ is an own key, not the row's prototype", () => { + const [row] = keyRowsByPosition(["__proto__"], [[{ polluted: true }]], "sqlite", "SELECT 1"); + + expect(Object.hasOwn(row, "__proto__")).toBe(true); + expect((row as { polluted?: boolean }).polluted).toBeUndefined(); + }); + + test("a row whose value count differs from the column count is refused, never padded or cut", () => { + const read = () => keyRowsByPosition(["a", "b"], [[1, 2], [3]], "duckdb", "SELECT a, b FROM t"); + + expect(read).toThrow(QueryError); + expect(read).toThrow("Row 2 carries 1 values for 2 result columns"); + }); +}); diff --git a/tests/unit/db/sqlite-driver.test.ts b/tests/unit/db/sqlite-driver.test.ts index ab651b59b..06a70153e 100644 --- a/tests/unit/db/sqlite-driver.test.ts +++ b/tests/unit/db/sqlite-driver.test.ts @@ -48,6 +48,7 @@ class StubDatabaseSync implements NodeDatabaseSyncLike { 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 }), + setReturnArrays: () => {}, // node:sqlite answers both halves in one call, and `null` is its word for a column // SQLite declared nothing for. columns: () => [ @@ -72,8 +73,16 @@ class StubDatabaseSync implements NodeDatabaseSyncLike { */ function bigIntStatement() { const row = () => ({ small: BigInt("1"), huge: BigInt("9007199254740993"), text: "x", nothing: null }); + const cells = () => [BigInt("1"), BigInt("9007199254740993"), "x", null]; + // node:sqlite's array mode is a switch on the statement, and bun's is its own method; the + // stand-in answers both, so the same row is read either way on either adapter. + let arrays = false; return { - all: () => [row()], + all: () => [arrays ? cells() : row()], + values: () => [cells()], + setReturnArrays: (enabled: boolean) => { + arrays = enabled; + }, get: () => row(), run: () => ({ changes: BigInt("2"), lastInsertRowid: BigInt("9007199254740993") }), // The declared columns in BOTH drivers' spellings, so the one stand-in can stand in @@ -319,6 +328,19 @@ describe("sqlite-driver", () => { expect(stmt.run()).toEqual(expectedRun); }); + test.each([ + ["bun", () => createBunSQLiteDriver(StubBunDatabase)], + ["node", () => createNodeSQLiteDriver(StubDatabaseSync)], + ] as const)("the %s adapter reads array rows with every BigInt converted", (_name, makeDriver) => { + const stmt = new (makeDriver())("/tmp/bigint.db").prepare("SELECT bigints"); + + // values(): the positional read every user result goes through. + expect(stmt.values()).toEqual([[1, "9007199254740993", "x", null]]); + // node's array mode is switched back off after the read, so a later all() on the same + // statement still answers objects. + expect(stmt.all()).toEqual([{ small: 1, huge: "9007199254740993", text: "x", nothing: null }]); + }); + test.each([ ["bun", () => createBunSQLiteDriver(StubBunDatabase)], ["node", () => createNodeSQLiteDriver(StubDatabaseSync)], @@ -627,6 +649,7 @@ describe("toSQLiteBindValue()", () => { function statementReturning(record: unknown): ReturnType { return { all: () => [record], + values: () => [record] as unknown[][], get: () => record, run: () => ({ changes: 1 }), columnNames: [], @@ -647,6 +670,26 @@ function driverReturning(record: unknown): SQLiteConstructor { return createBunSQLiteDriver(RecordDatabase as BunSQLiteConstructor); } +describe("the bun adapter's array rows", () => { + // Measured 2026-10-07 on Bun 1.4.2: bun:sqlite's `values()` answers `null`, not `[]`, for a + // statement with no result columns (`PRAGMA query_only = false`, a plain INSERT). + test("bun's null for a statement with no result columns is read as no rows", () => { + class NoColumnsDatabase implements BunDatabaseLike { + exec(): void {} + prepare(): ReturnType { + return { ...statementReturning({}), values: () => null }; + } + close(): void {} + readonly inTransaction = false; + } + const stmt = new (createBunSQLiteDriver(NoColumnsDatabase as BunSQLiteConstructor))(":memory:").prepare( + "PRAGMA x = 1", + ); + + expect(stmt.values()).toEqual([]); + }); +}); + 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. From 935c2ae5e94d8cc6d73e50a12fcc9f5d53f830f5 Mon Sep 17 00:00:00 2001 From: cevheri Date: Wed, 7 Oct 2026 11:12:37 +0300 Subject: [PATCH 07/46] fix(duckdb): read result rows by position so a repeated column keeps its value getRowObjectsJson() keys a repeated column a:1 while columnNames() lists the name twice, so both headers showed the first value, and a column the statement itself named a:1 was overwritten by the repeat. The client now reads getRowsJson() and keys each row by position under uniqueFieldNames, so query() and queryReadOnly() answer a, a (2) with both values and both types. DuckDB refuses an empty alias at parse time, pinned as such. --- docs/providers/duckdb.md | 37 ++++++++++---- src/lib/db/providers/sql/duckdb/client.ts | 33 ++++++++----- src/lib/db/providers/sql/duckdb/introspect.ts | 2 +- src/lib/db/providers/sql/duckdb/values.ts | 11 ++--- tests/integration/db/duckdb-provider.test.ts | 49 +++++++++++++++++-- tests/unit/db/duckdb/seam-guard.test.ts | 5 +- 6 files changed, 104 insertions(+), 33 deletions(-) diff --git a/docs/providers/duckdb.md b/docs/providers/duckdb.md index 4de0d5ec7..c693aa41f 100644 --- a/docs/providers/duckdb.md +++ b/docs/providers/duckdb.md @@ -49,8 +49,8 @@ is a native N-API addon carrying a 70 MB shared library rather than a runtime bu It is a separate type-id from `sqlite`, not a relative of it, for the same reason libSQL is: the execution layer has nothing in common. `bun:sqlite` is synchronous and returns JavaScript values; -`@duckdb/node-api` is asynchronous, returns a reader, and can only be serialised through -`getRowObjectsJson()` (§3.2). Every catalog statement differs. Every maintenance statement differs. +`@duckdb/node-api` is asynchronous, returns a reader, and can only be serialised through its +JSON readers (§3.2). Every catalog statement differs. Every maintenance statement differs. Sharing an id would mean one document describing two sets of measurements. ### 1.1 Deployment constraint @@ -103,7 +103,7 @@ src/lib/db/providers/sql/duckdb/ ``` The driver seam is narrow by construction: `DuckDBInstance` / `DuckDBConnection` and the reader API -(`runAndReadAll`, `getRowObjectsJson`, `columnNames`, `columnTypes`, `rowsChanged`, `interrupt`) are +(`runAndReadAll`, `getRowsJson`, `columnNames`, `columnTypes`, `rowsChanged`, `interrupt`) are vocabulary of `@duckdb/node-api` and stay inside **`client.ts`** — `duckdb-seam-guard.test.ts` fails the build when any of it appears in a sibling. The import of the driver is dynamic and lives inside the open function, never at module scope, so the ~70 MB `libduckdb.so` is not loaded into a process @@ -131,7 +131,7 @@ scoped with `database_name = current_database()` rather than by name, and object qualifies `schema.table`. A user who hits the binder error in the editor is being told about their own file name, and the doc says so here rather than leaving it to be rediscovered. -### 3.2 `getRowObjects()` is banned; rows come from `getRowObjectsJson()` +### 3.2 `getRowObjects()` is banned; rows come from `getRowsJson()` `getRowObjects()` throws the moment a result is serialised: @@ -141,8 +141,9 @@ JSON.stringify cannot serialize BigInt (Bun 1.3.14) ``` Wide integers arrive as JavaScript `BigInt` values in that shape, so a table with a `BIGINT` primary -key would break the whole API response rather than one cell. `getRowObjectsJson()` is the -only reader used on the API path, and it hands back values that are already JSON-safe. +key would break the whole API response rather than one cell. The JSON readers hand back values that +are already JSON-safe, and `getRowsJson()`, which answers each row as an array, is the only reader +used ([Result column names](#result-column-names) says why the array form and not `getRowObjectsJson()`). For @@ -173,7 +174,7 @@ Wide integers staying as strings is the engine protecting exactness, and the pro "repair" it: `Number("9223372036854775807")` is a different number, and a rounded key is a corruption nothing downstream can detect. The grid renders the string as measured. -**Columns come from `columnNames()`, never from the rows.** `getRowObjectsJson()` returns `[]` for an +**Columns come from `columnNames()`, never from the rows.** `getRowsJson()` returns `[]` for an empty result set and carries no column information, while `columnNames()` and `columnTypes()` answer regardless. `columnTypes().map(String)` gives DuckDB's own type text — `INTEGER`, `VARCHAR`, `INTEGER[]`, `STRUCT("x" INTEGER)`, `HUGEINT`, `INTERVAL`, `UUID` — which is what populates @@ -657,14 +658,30 @@ Explicitly **not** supported, so that a reader does not go looking: `query(sql, params)` runs one statement and returns `{ rows, fields, rowCount, executionTime, columnTypes? }`. -- **Rows** come from `getRowObjectsJson()`; `getRowObjects()` is never called (§3.2). -- **`fields`** come from `columnNames()`, so an empty result still names its columns. +- **Rows** come from `getRowsJson()`, keyed by position under `fields`; `getRowObjects()` is never called (§3.2). +- **`fields`** come from `columnNames()`, so an empty result still names its columns, and a repeated name is numbered ([Result column names](#result-column-names)). - **`columnTypes`** carries `columnTypes().map(String)` — DuckDB's own type text — and the key is **omitted entirely** when there is nothing to publish, never emitted as `{}`. - **`rowCount`** is the row count for a read and `result.rowsChanged` for a write; the synthetic `Count` column is not surfaced (§3.4). - **Cancellation** calls `interrupt()` on the connection (§3.9). +### Result column names + +Every result column comes back under a name that is non-empty and that no other column of the result has, with its own value. +`client.ts` reads the rows as arrays (`getRowsJson()`) and keys them by position under `uniqueFieldNames(columnNames())` ([`result-fields.ts`](../../src/lib/db/utils/result-fields.ts)), and `columnTypes` is keyed by the same names. +Measured 2026-10-07 in-process on @duckdb/node-api 1.5.5-r.4, pinned in `tests/integration/db/duckdb-provider.test.ts`: + +| Statement | `fields` | Row | +|---|---|---| +| `SELECT 1 AS a, 'x' AS a` | `a`, `a (2)` | `{ a: 1, "a (2)": "x" }`, typed `INTEGER` and `VARCHAR` | +| `SELECT 2 AS "a:1", 3 AS a, 4 AS a, 4 + 5` | `a:1`, `a`, `a (2)`, `(4 + 5)` | every value under its own name | +| `SELECT 1 AS ""` | none | the parser refuses it: *zero-length delimited identifier* | + +DuckDB names an unaliased expression by its text (`(4 + 5)`) and refuses an empty alias, so it never declares a column with no name. +A repeated name was the defect: the driver's `getRowObjectsJson()` keys a repeat `a:1` while `columnNames()` lists `a` twice, so both grid headers showed the first value, and a column the statement itself named `a:1` was overwritten by the repeat. +`queryReadOnly()` reads through the same client call. + ### A transaction a statement left open A `BEGIN` sent through `query()` opens a transaction on the ONE connection this provider holds @@ -1559,7 +1576,7 @@ The reach described in this section is an admin editor's on an inline connection | No `REINDEX`, no integrity check | Neither statement exists on 1.5.5 | The engine's (§8) | | No analyze/timing plan, ever | `EXPLAIN (ANALYZE, FORMAT JSON)` **executes the statement** — measured, an `INSERT` explained three times inserted three rows | **Ours, and permanent.** A later DuckDB emitting valid analyze JSON makes this worse, not better: the plan would look usable while still running what the user asked only to see (§3.6) | | Sizes arrive as formatted strings | `pragma_database_size()` publishes no raw bytes | The engine's (§3.5) | -| Wide integers render as strings | `getRowObjectsJson()` quotes them to stay exact | The engine's, and deliberate (§3.2) | +| Wide integers render as strings | `getRowsJson()` quotes them to stay exact | The engine's, and deliberate (§3.2) | | A multi-statement string runs only its first statement | The driver's `runAndReadAll` behaviour | The driver's (§3.11) | | No `SERIAL`, no `IDENTITY`, no `AUTOINCREMENT` | None of the three exists; `GENERATED … AS IDENTITY` parses and is refused at execution | The engine's — the create-table form defaults from a sequence instead (§3.13) | | ~140 MB of bindings on a Linux tree | glibc and musl packages both install | Ours to prune in the AppImage build (§11) | diff --git a/src/lib/db/providers/sql/duckdb/client.ts b/src/lib/db/providers/sql/duckdb/client.ts index 1003795c4..cd8a2320d 100644 --- a/src/lib/db/providers/sql/duckdb/client.ts +++ b/src/lib/db/providers/sql/duckdb/client.ts @@ -19,11 +19,16 @@ * 2026-08-27, and recorded in `.duckdb-measured.md`): * * - `getRowObjects()` throws on `JSON.stringify` ("Do not know how to serialize a - * BigInt"), so `getRowObjectsJson()` is the only row reader used here. It is not a + * BigInt"), so the JSON readers are the only row readers used here. It is not a * preference: the API route serializes every result. - * - `columnNames()` and `columnTypes()` answer even for an EMPTY row set, and - * `getRowObjectsJson()` carries no column information at all, so columns are read - * from the reader rather than from the first row. + * - `columnNames()` and `columnTypes()` answer even for an EMPTY row set, and the row + * readers carry no column information at all, so columns are read from the reader + * rather than from the first row. + * - `getRowObjectsJson()` keys a repeated column `a:1` (`deduplicatedColumnNames()`), + * while `columnNames()` lists `a` twice, and a column the statement itself named `a:1` + * collides with that key and loses its value (measured 2026-10-07, @duckdb/node-api + * 1.5.5-r.4). So the rows are read as arrays (`getRowsJson()`, the same per-value JSON + * conversion) and keyed by position under `uniqueFieldNames`. * - `access_mode: 'READ_ONLY'` refuses writes to the attached database AND refuses to * create a missing file, but on its own it is not a filesystem sandbox: `COPY ... TO`, * `read_text('/etc/hostname')` and `glob('/etc/*')` all succeeded on a handle whose @@ -43,6 +48,8 @@ import * as fs from "fs"; import * as os from "os"; import { join } from "path"; import { ConnectionError } from "../../../errors"; +import { keyRowsByPosition } from "../../../utils/positional-rows"; +import { uniqueFieldNames } from "../../../utils/result-fields"; // ============================================================================ // Neutral result shape @@ -51,13 +58,16 @@ import { ConnectionError } from "../../../errors"; /** * One statement's outcome, in the vocabulary the rest of the provider speaks. * - * `rows` are the JSON projections `getRowObjectsJson()` produces, which is the only - * reader that survives serialization: BIGINT, HUGEINT and DECIMAL arrive as decimal - * STRINGS, LIST as an array, STRUCT as an object, INTERVAL as - * `{months, days, micros}` and UUID as a string. + * `rows` are the JSON projections `getRowsJson()` produces, which survive serialization: + * BIGINT, HUGEINT and DECIMAL arrive as decimal STRINGS, LIST as an array, STRUCT as an + * object, INTERVAL as `{months, days, micros}` and UUID as a string. */ export interface DuckDBStatementResult { - /** Column order exactly as the engine declared it, present even for an empty row set. */ + /** + * The names `rows` are keyed under, one per column in the order the engine declared them, + * present even for an empty row set: the declared names through `uniqueFieldNames`, so a + * repeated name comes back as `a`, `a (2)` and each column keeps its own value. + */ columnNames: string[]; /** DuckDB's own type text per column, positionally aligned with `columnNames`. */ columnTypes: string[]; @@ -487,10 +497,11 @@ export async function openDuckDBClient(path: string, options: DuckDBOpenOptions) ? await connection.runAndReadAll(sql) : await connection.runAndReadAll(sql, params as Parameters[1]); + const columnNames = uniqueFieldNames(reader.columnNames()); return { - columnNames: reader.columnNames(), + columnNames, columnTypes: reader.columnTypes().map(String), - rows: reader.getRowObjectsJson() as Record[], + rows: keyRowsByPosition(columnNames, reader.getRowsJson(), "duckdb", sql), rowsChanged: reader.rowsChanged, }; }, diff --git a/src/lib/db/providers/sql/duckdb/introspect.ts b/src/lib/db/providers/sql/duckdb/introspect.ts index 8a3e66d9b..79c5aa734 100644 --- a/src/lib/db/providers/sql/duckdb/introspect.ts +++ b/src/lib/db/providers/sql/duckdb/introspect.ts @@ -120,7 +120,7 @@ const DATABASE_PATH_SQL = `SELECT path FROM duckdb_databases() // ============================================================================ /** - * Every 64-bit column arrives as a decimal STRING through `getRowObjectsJson()`, which + * Every 64-bit column arrives as a decimal STRING through `getRowsJson()`, which * is why the numeric fields below are typed `unknown` and read through `readCount` * rather than typed `number` and trusted. */ diff --git a/src/lib/db/providers/sql/duckdb/values.ts b/src/lib/db/providers/sql/duckdb/values.ts index 1fa5c65a5..8e97a0682 100644 --- a/src/lib/db/providers/sql/duckdb/values.ts +++ b/src/lib/db/providers/sql/duckdb/values.ts @@ -84,11 +84,10 @@ export function isWriteAcknowledgement(result: DuckDBStatementResult, leadingKey // ============================================================================ /** - * The engine's declared type per column, keyed by name. + * The engine's declared type per column, keyed by the name the rows use. * - * Duplicate column names collapse to the last one, which is what the row objects - * themselves do (`getRowObjectsJson()` builds plain objects), so the two agree. The - * map is built even when it would be empty; the caller decides whether to emit it, + * The client has already made those names unique (`a`, `a (2)`), so a repeated column + * keeps its own type. The map is built even when it would be empty; the caller decides whether to emit it, * because `QueryResult.columnTypes` must be ABSENT rather than `{}` when there is * nothing to say. */ @@ -108,7 +107,7 @@ export function columnTypeMap(result: DuckDBStatementResult): Record { expect(result.columnTypes).toEqual({ a: "INTEGER", b: "VARCHAR" }); }); - test("wide types arrive in the shapes getRowObjectsJson produces", async () => { + test("wide types arrive in the shapes the JSON row reader produces", async () => { // Pinned because they are the reason `getRowObjects()` is banned: it throws on // JSON.stringify ("Do not know how to serialize a BigInt"). provider = new DuckDBProvider(makeConfig()); @@ -856,6 +856,37 @@ describe("query()", () => { expect(result.columnTypes).toEqual({ id: "INTEGER", name: "VARCHAR" }); }); + // DuckDB's own row objects key a repeat `a:1`, while `columnNames()` lists `a` twice, so + // both headers showed the first value; and a column the statement itself named `a:1` + // collided with that key and lost its value. The rows are read positionally now. + test("a repeated alias keeps both values under two names, with both types", async () => { + provider = new DuckDBProvider(makeConfig()); + await provider.connect(); + + const result = await provider.query("SELECT 1 AS a, 'x' AS a"); + + expect(result.fields).toEqual(["a", "a (2)"]); + expect(result.rows).toEqual([{ a: 1, "a (2)": "x" }]); + expect(result.columnTypes).toEqual({ a: "INTEGER", "a (2)": "VARCHAR" }); + }); + + test("a column named like DuckDB's own repeat key keeps its value", async () => { + provider = new DuckDBProvider(makeConfig()); + await provider.connect(); + + const result = await provider.query('SELECT 2 AS "a:1", 3 AS a, 4 AS a, 4 + 5'); + + expect(result.fields).toEqual(["a:1", "a", "a (2)", "(4 + 5)"]); + expect(result.rows).toEqual([{ "a:1": 2, a: 3, "a (2)": 4, "(4 + 5)": 9 }]); + }); + + test("an empty alias is refused by the engine's parser, so no column comes back unnamed", async () => { + provider = new DuckDBProvider(makeConfig()); + await provider.connect(); + + await expect(provider.query('SELECT 1 AS ""')).rejects.toThrow(/zero-length delimited identifier/); + }); + test("an INSERT reports the rows it changed and shows no Count grid", async () => { provider = new DuckDBProvider(makeConfig()); await provider.connect(); @@ -1349,6 +1380,18 @@ describe("queryReadOnly()", () => { await expect(provider.queryReadOnly("SELECT 1", GENEROUS_BUDGET)).rejects.toThrow(/agent read-only profile/); }); + test("a repeated alias keeps both values through the profiled handle", async () => { + provider = await readOnlyProvider(); + + const result = await provider.queryReadOnly("SELECT 1 AS id, id FROM users ORDER BY id", GENEROUS_BUDGET); + + expect(result.fields).toEqual(["id", "id (2)"]); + expect(result.rows).toEqual([ + { id: 1, "id (2)": 1 }, + { id: 1, "id (2)": 2 }, + ]); + }); + test("reads answer normally through the profiled handle", async () => { provider = await readOnlyProvider(); @@ -1960,7 +2003,7 @@ describe("a non-admin editor handle has no statement-level file or network reach await admin.connect(); try { expect((await admin.query("SELECT current_setting('enable_external_access') AS v")).rows).toEqual([{ v: true }]); - // DuckDB's replacement scan returns untyped CSV columns as VARCHAR, so getRowObjectsJson + // DuckDB's replacement scan returns untyped CSV columns as VARCHAR, so getRowsJson // quotes them, matching the BARE_SCAN assertions elsewhere in this file. expect((await admin.query(`SELECT * FROM read_csv_auto('${input}')`)).rows).toEqual([{ a: "1", b: "2" }]); expect(String((await admin.query(`SELECT content FROM read_text('${secret}')`)).rows[0].content)).toContain( @@ -4216,7 +4259,7 @@ describe("DuckDB object statements: what the fixture cannot show", () => { test("a count the driver cannot read leaves its seeded value rather than writing NaN", () => { // The `toNumber` seam is injected precisely so this can be driven: a DuckDB COUNT(*) - // is BIGINT and arrives as a decimal STRING through `getRowObjectsJson()`, so no live + // is BIGINT and arrives as a decimal STRING through `getRowsJson()`, so no live // read can produce an unreadable one. A driver change that did would otherwise write // `Number(undefined)`, and NaN renders as a blank badge that looks like a measurement. const counts = { table: { count: 0 }, macro: { count: 0 } }; diff --git a/tests/unit/db/duckdb/seam-guard.test.ts b/tests/unit/db/duckdb/seam-guard.test.ts index 595f595a7..c8abdcf23 100644 --- a/tests/unit/db/duckdb/seam-guard.test.ts +++ b/tests/unit/db/duckdb/seam-guard.test.ts @@ -57,6 +57,7 @@ const DRIVER_TOKENS = [ "runAndReadAll", "getRowObjectsJson", "getRowObjects", + "getRowsJson", "disconnectSync", "closeSync", "access_mode", @@ -66,7 +67,7 @@ const SEAM_RULE = [ `The DuckDB driver leaked out of ${CLIENT_FILE}.`, "", "`@duckdb/node-api` opens a DuckDBInstance, connects a DuckDBConnection, and answers a statement with a", - "DuckDBResultReader whose rows only survive serialization through getRowObjectsJson(). Issue #424 keeps all", + "DuckDBResultReader whose rows only survive serialization through its JSON readers. Issue #424 keeps all", "of that inside client.ts: provider logic reads the neutral DuckDBStatementResult (columnNames, columnTypes,", "rows, rowsChanged) through the DuckDBClient seam. That is what makes a second implementation - the Wasm", "build, a subprocess against the CLI - one new file rather than a rewrite of the provider and its", @@ -169,7 +170,7 @@ describe("DuckDB driver seam", () => { // A detector that finds nothing anywhere is indistinguishable from a broken one, so // the file that is SUPPOSED to speak to the driver must light it up. - test.each(["@duckdb/node-api", "DuckDBInstance", "DuckDBConnection", "runAndReadAll", "getRowObjectsJson"])( + test.each(["@duckdb/node-api", "DuckDBInstance", "DuckDBConnection", "runAndReadAll", "getRowsJson"])( "the client itself uses %s, proving the detector reads real code", (token) => { const tokens = findLeaks(CLIENT_FILE, readProviderSource(CLIENT_FILE)).map((leak) => leak.token); From 3b2ebf4c89dffd7eec191949bae5d3ff5354f8cb Mon Sep 17 00:00:00 2001 From: cevheri Date: Wed, 7 Oct 2026 11:14:12 +0300 Subject: [PATCH 08/46] fix(libsql): name result columns uniquely so a repeated or unnamed column keeps its value The Hrana transport keyed each row by the declared name, so a repeated name kept only the last value while fields listed it twice, and an unnamed column became column_N, which a column the statement named column_1 overwrote. Columns are now named with uniqueFieldNames and rows keyed by position, with declared types under the same names. A row whose value count is not the column count, or that is not a list, is refused instead of padded. --- docs/providers/libsql.md | 6 ++ .../providers/sql/libsql/hrana-transport.ts | 54 ++++++++------- src/lib/db/providers/sql/libsql/transport.ts | 11 +-- tests/integration/db/libsql-provider.test.ts | 22 ++++++ tests/unit/db/libsql/hrana-transport.test.ts | 67 ++++++++++++++++--- 5 files changed, 119 insertions(+), 41 deletions(-) diff --git a/docs/providers/libsql.md b/docs/providers/libsql.md index 9f04d037d..860987334 100644 --- a/docs/providers/libsql.md +++ b/docs/providers/libsql.md @@ -425,6 +425,12 @@ columnTypes? }`. - **Parameters are positional** (`?`), encoded per type: an integer as a decimal string, a non-integral number as a float, a boolean as SQLite's own 1/0, `Uint8Array` as base64, a `Date` as an ISO string. Passing none sends no `args` member at all. +- **`fields`** are the declared column names, each non-empty and unique, and the rows are keyed by position under them in [`hrana-transport.ts`](../../src/lib/db/providers/sql/libsql/hrana-transport.ts). + A repeated name comes back numbered: two columns declared `id` are `id` and `id (2)`, each with its own value and declared type. + A column declared with no name, or with an empty one (`1 AS ""`), comes back as `(No column name)`. + Before this, a repeated name kept only the last value while `fields` listed it twice, and an unnamed column was `column_N`, which a column the statement itself named `column_1` overwrote. + Pinned against the Hrana answer shape in `tests/unit/db/libsql/hrana-transport.test.ts` and `tests/integration/db/libsql-provider.test.ts`; not re-measured against a live sqld in this change. + A row whose value count is not the column count is refused with an error rather than padded. - **`rowCount`** is the row count for a read and the engine's `affected_row_count` for a write. - **`columnTypes`** carries SQLite's declared types verbatim (`INTEGER`, `TEXT`) and is OMITTED when the engine declared none — which it does for every computed column and every PRAGMA, so an absent diff --git a/src/lib/db/providers/sql/libsql/hrana-transport.ts b/src/lib/db/providers/sql/libsql/hrana-transport.ts index 23d00a7a5..61fe98d94 100644 --- a/src/lib/db/providers/sql/libsql/hrana-transport.ts +++ b/src/lib/db/providers/sql/libsql/hrana-transport.ts @@ -35,6 +35,8 @@ import { endpointUrl, type HttpOrigin, httpOrigin, rejectRedirect } from "@/lib/ import { DatabaseConfigError } from "@/lib/db/errors"; import { httpTransportFetch } from "@/lib/db/http/egress-policy"; import type { DatabaseConnection } from "@/lib/db/types"; +import { keyRowsByPosition } from "@/lib/db/utils/positional-rows"; +import { uniqueFieldNames } from "@/lib/db/utils/result-fields"; import { isSQLiteInt64Digits } from "../sqlite-int64"; import { type LibSQLBatchOutcome, @@ -289,36 +291,36 @@ function encodeValue(param: unknown): HranaValue { return { type: "text", value: String(param) }; } -/** The column names and the types the engine declared for them. */ +/** + * The column names and the types the engine declared for them. + * + * The names are the declared ones through `uniqueFieldNames`: a column the engine did not + * name is `(No column name)` and a repeated name is numbered `id (2)`, so every column keeps a + * key of its own. A positional `column_N` used to stand in for a missing name, and it could + * collide with a column the statement itself named `column_1`. + */ function readColumns(cols: unknown): { fieldNames: string[]; columnTypes: Record } { - const fieldNames: string[] = []; const columnTypes: Record = {}; - if (!Array.isArray(cols)) return { fieldNames, columnTypes }; - - for (const [index, raw] of cols.entries()) { - const col = (asRecord(raw) ?? {}) as HranaColumn; - // A column the engine did not name still occupies a position, so it gets a - // positional name rather than being dropped - dropping one would shift every - // later value into the wrong key. - const name = typeof col.name === "string" && col.name !== "" ? col.name : `column_${index + 1}`; - fieldNames.push(name); - if (typeof col.decltype === "string" && col.decltype !== "") columnTypes[name] = col.decltype; + if (!Array.isArray(cols)) return { fieldNames: [], columnTypes }; + + const declared = cols.map((raw) => (asRecord(raw) ?? {}) as HranaColumn); + const fieldNames = uniqueFieldNames(declared.map((col) => (typeof col.name === "string" ? col.name : ""))); + for (const [index, col] of declared.entries()) { + if (typeof col.decltype === "string" && col.decltype !== "") columnTypes[fieldNames[index]] = col.decltype; } return { fieldNames, columnTypes }; } -function readRows(rows: unknown, fieldNames: string[]): LibSQLRow[] { +/** + * The rows, keyed by position under `fieldNames`. A row that is not a list, or carries a value + * count other than the column count, is refused by `keyRowsByPosition` rather than padded. + */ +function readRows(rows: unknown, fieldNames: string[], sql: string): LibSQLRow[] { if (!Array.isArray(rows)) return []; - return rows.map((raw) => { - const values = Array.isArray(raw) ? raw : []; - const row: LibSQLRow = {}; - for (const [index, name] of fieldNames.entries()) { - row[name] = decodeValue(values[index]); - } - return row; - }); + const values = rows.map((raw) => (Array.isArray(raw) ? raw.map(decodeValue) : [])); + return keyRowsByPosition(fieldNames, values, "libsql", sql); } function toNumber(raw: unknown, fallback: number): number { @@ -333,7 +335,7 @@ function toNumber(raw: unknown, fallback: number): number { * than shifting every later result onto the wrong statement - the shape a short * answer would otherwise silently produce. */ -function readOutcome(raw: unknown): LibSQLBatchOutcome { +function readOutcome(raw: unknown, sql: string): LibSQLBatchOutcome { const step = asRecord(raw); if (!step) return { ok: false, error: new LibSQLTransportError(NO_RESULT, 200) }; @@ -347,15 +349,15 @@ function readOutcome(raw: unknown): LibSQLBatchOutcome { return { ok: false, error: new LibSQLTransportError(message, 200, code) }; } - return { ok: true, result: toStatementResult(asRecord(step.response)?.result) }; + return { ok: true, result: toStatementResult(asRecord(step.response)?.result, sql) }; } -function toStatementResult(raw: unknown): LibSQLStatementResult { +function toStatementResult(raw: unknown, sql: string): LibSQLStatementResult { const result = (asRecord(raw) ?? {}) as HranaStatementResult; const { fieldNames, columnTypes } = readColumns(result.cols); return { - rows: readRows(result.rows, fieldNames), + rows: readRows(result.rows, fieldNames, sql), fieldNames, columnTypes, affectedRowCount: toNumber(result.affected_row_count, 0), @@ -473,7 +475,7 @@ export class LibSQLHranaTransport implements LibSQLTransport { // fails the whole call: there is no per-statement answer to hand back. if (!Array.isArray(results)) throw new LibSQLTransportError(NOT_AN_ENVELOPE, 200); - return statements.map((_statement, index) => readOutcome(results[index])); + return statements.map((statement, index) => readOutcome(results[index], statement.sql)); } public async serverVersion(): Promise { diff --git a/src/lib/db/providers/sql/libsql/transport.ts b/src/lib/db/providers/sql/libsql/transport.ts index 6e6b01a66..efa15edc6 100644 --- a/src/lib/db/providers/sql/libsql/transport.ts +++ b/src/lib/db/providers/sql/libsql/transport.ts @@ -19,13 +19,12 @@ */ /** - * One result row, keyed by the column name the engine declared. + * One result row, keyed by the names in `fieldNames`. * * libSQL answers a statement with declared columns and positional rows, so an * object row is a mapping this layer performs rather than a shape the wire - * carries. Duplicate column names collapse - `SELECT 1 AS a, 2 AS a` keeps the - * last - which is SQLite's own behaviour through every driver here and is why - * `fieldNames` is carried separately. + * carries. The keys are the declared names made unique (`uniqueFieldNames`), so + * `SELECT 1 AS a, 2 AS a` answers `a` and `a (2)` and keeps both values. */ export type LibSQLRow = Record; @@ -34,7 +33,9 @@ export interface LibSQLStatementResult { rows: LibSQLRow[]; /** - * Column order exactly as the engine declared it. Never null: libSQL declares + * The names `rows` are keyed under, one per column in the order the engine declared + * them: the declared names, with an unnamed column named `(No column name)` and a + * repeat numbered `a (2)`. Never null: libSQL declares * columns for every statement, answering an empty list for one that projects * nothing (a `CREATE TABLE`), which is a declaration rather than an absence. */ diff --git a/tests/integration/db/libsql-provider.test.ts b/tests/integration/db/libsql-provider.test.ts index 873181303..d0eb75cf5 100644 --- a/tests/integration/db/libsql-provider.test.ts +++ b/tests/integration/db/libsql-provider.test.ts @@ -519,6 +519,28 @@ describe("LibSQLProvider query", () => { await provider.disconnect(); }); + test("a repeated or empty column name keeps its own value and type", async () => { + // The shape sqld answers for `SELECT o.id, c.id, 1 AS "" ...`: two columns declared `id` + // and one declared with an empty name. Keyed by name, the second id replaced the first. + server = () => + result( + [ + ["id", "INTEGER"], + ["id", "INTEGER"], + ["", null], + ], + [[int(100), int(1), int(7)]], + ); + const provider = await connected(); + + const answer = await provider.query('SELECT o.id, c.id, 1 AS "" FROM probe_orders o JOIN probe_customers c'); + + expect(answer.fields).toEqual(["id", "id (2)", "(No column name)"]); + expect(answer.rows).toEqual([{ id: 100, "id (2)": 1, "(No column name)": 7 }]); + expect(answer.columnTypes).toEqual({ id: "INTEGER", "id (2)": "INTEGER" }); + await provider.disconnect(); + }); + test("omits columnTypes entirely when the engine declared none", async () => { const provider = await connected(); diff --git a/tests/unit/db/libsql/hrana-transport.test.ts b/tests/unit/db/libsql/hrana-transport.test.ts index 618bbac5b..2406929a3 100644 --- a/tests/unit/db/libsql/hrana-transport.test.ts +++ b/tests/unit/db/libsql/hrana-transport.test.ts @@ -748,13 +748,14 @@ describe("LibSQLHranaTransport tolerances", () => { await transport().execute("SELECT 1", { timeoutMs: 250 }); }); - test("names a column the engine left unnamed by its position rather than dropping it", async () => { + test("names a column the engine left unnamed rather than dropping it", async () => { withResult({ - cols: [{ decltype: null }, { name: "", decltype: "TEXT" }], + cols: [{ decltype: null }, { name: "", decltype: "TEXT" }, { name: "column_1", decltype: "INTEGER" }], rows: [ [ { type: "integer", value: "1" }, { type: "text", value: "x" }, + { type: "integer", value: "3" }, ], ], affected_row_count: 0, @@ -762,13 +763,38 @@ describe("LibSQLHranaTransport tolerances", () => { query_duration_ms: 0.01, }); - const result = await transport().execute("SELECT 1, 'x'"); + const result = await transport().execute("SELECT 1, 'x' AS \"\", 3 AS column_1"); - expect(result.fieldNames).toEqual(["column_1", "column_2"]); - expect(result.rows).toEqual([{ column_1: 1, column_2: "x" }]); - // The empty name is not a declared type either: the second column's TEXT - // lands under the positional name, never under "". - expect(result.columnTypes).toEqual({ column_2: "TEXT" }); + // A positional `column_1` collided with a column the statement itself named so, and the + // later value replaced the earlier one. The unnamed columns are named apart instead. + expect(result.fieldNames).toEqual(["(No column name)", "(No column name) (2)", "column_1"]); + expect(result.rows).toEqual([{ "(No column name)": 1, "(No column name) (2)": "x", column_1: 3 }]); + // The empty name is not a declared type either: the second column's TEXT lands under + // its own name, never under "". + expect(result.columnTypes).toEqual({ "(No column name) (2)": "TEXT", column_1: "INTEGER" }); + }); + + test("a repeated column name keeps both values and both types", async () => { + withResult({ + cols: [ + { name: "id", decltype: "INTEGER" }, + { name: "id", decltype: "TEXT" }, + ], + rows: [ + [ + { type: "integer", value: "100" }, + { type: "text", value: "c1" }, + ], + ], + affected_row_count: 0, + query_duration_ms: 0, + }); + + const result = await transport().execute("SELECT o.id, c.id FROM o JOIN c"); + + expect(result.fieldNames).toEqual(["id", "id (2)"]); + expect(result.rows).toEqual([{ id: 100, "id (2)": "c1" }]); + expect(result.columnTypes).toEqual({ id: "INTEGER", "id (2)": "TEXT" }); }); test("reads a column declaration that is not a list as no columns at all", async () => { @@ -779,7 +805,26 @@ describe("LibSQLHranaTransport tolerances", () => { expect(result).toMatchObject({ fieldNames: [], rows: [], affectedRowCount: 0, executionTimeMs: 0 }); }); - test("reads a row that is not a list as a row of nulls, keeping the column count", async () => { + test("refuses a row whose value count is not the column count, rather than padding it", async () => { + withResult({ + cols: [{ name: "a" }, { name: "b" }], + rows: [ + [ + { type: "integer", value: "1" }, + { type: "integer", value: "2" }, + ], + [{ type: "integer", value: "3" }], + ], + affected_row_count: 0, + query_duration_ms: 0, + }); + + await expect(transport().execute("SELECT a, b FROM t")).rejects.toThrow( + "Row 2 carries 1 values for 2 result columns", + ); + }); + + test("refuses a row that is not a list at all", async () => { withResult({ cols: [{ name: "a" }, { name: "b" }], rows: [null], @@ -787,7 +832,9 @@ describe("LibSQLHranaTransport tolerances", () => { query_duration_ms: 0, }); - expect((await transport().execute("SELECT a, b FROM t")).rows).toEqual([{ a: null, b: null }]); + await expect(transport().execute("SELECT a, b FROM t")).rejects.toThrow( + "Row 1 carries 0 values for 2 result columns", + ); }); test("decodes a float the protocol quoted, and a text value it did not", async () => { From 9730e6cebe211a3448676e4a609af968101eec7a Mon Sep 17 00:00:00 2001 From: cevheri Date: Wed, 7 Oct 2026 11:15:45 +0300 Subject: [PATCH 09/46] fix(cassandra): name an empty result column and refuse two columns of one name cassandra-driver builds each row as row[column.name] = value, so two columns of one name reach the provider holding only the last value, and both grid headers showed it. That result is now refused with a query error naming the column and asking for an alias, since the earlier value cannot be recovered. A column declared with an empty name is keyed "(No column name)", with its declared type under the same name. --- docs/providers/cassandra.md | 5 +++ .../sql/cassandra/driver-transport.ts | 27 +++++++++++++-- .../integration/db/cassandra-provider.test.ts | 11 +++++++ tests/unit/db/cassandra/wire.test.ts | 33 +++++++++++++++++++ 4 files changed, 73 insertions(+), 3 deletions(-) diff --git a/docs/providers/cassandra.md b/docs/providers/cassandra.md index 977cb6867..df61f59c9 100644 --- a/docs/providers/cassandra.md +++ b/docs/providers/cassandra.md @@ -580,6 +580,11 @@ The **declaration** drives the row shape, not the row's own keys: it is the only the statement projected. `fieldNames: null` (a write) and `fieldNames: []` are kept apart at the seam and both collapse to no columns for the grid. +A column declared with an empty name comes back as `(No column name)` (`uniqueFieldNames` in [`result-fields.ts`](../../src/lib/db/utils/result-fields.ts)), so the grid always has a key for it. +Two columns of one name (`SELECT id AS x, name AS x`) are refused with a query error naming the column: cassandra-driver 4.10.0 builds each row as `row[column.name] = value` (`lib/streams.js`, `parseRows`), so the earlier column's value is already gone when the row reaches this provider, and a second header would only repeat the last value. +Give each column its own alias. +This is read from the driver source and pinned with the driver's row shape in `tests/unit/db/cassandra/wire.test.ts` and `tests/integration/db/cassandra-provider.test.ts`; it was not measured against a live cluster in this change, so whether a given server version accepts a repeated name at all is not stated here. + `columnTypes` carries the wire's declared type per column. Note that `text` reads back as `varchar` and a column declared `varchar` reads back as `text` — they are one type on the wire — so this is whatever the protocol declared rather than the DDL word. The schema tree reads diff --git a/src/lib/db/providers/sql/cassandra/driver-transport.ts b/src/lib/db/providers/sql/cassandra/driver-transport.ts index 76fc55e9c..02992cf39 100644 --- a/src/lib/db/providers/sql/cassandra/driver-transport.ts +++ b/src/lib/db/providers/sql/cassandra/driver-transport.ts @@ -36,6 +36,7 @@ import { Client, types, type ClientOptions, type QueryOptions } from "cassandra-driver"; import type { DatabaseConnection } from "@/lib/db/types"; +import { uniqueFieldNames } from "@/lib/db/utils/result-fields"; import { type CassandraExecuteOptions, type CassandraQueryResult, @@ -314,20 +315,40 @@ export function normalizeCassandraValue(value: unknown): unknown { * ResultSet with no declaration and no rows - and it is carried through as `null` * rather than flattened to an empty list, so the provider can tell "no columns" * from "no rows". + * + * The names are the declared ones through `uniqueFieldNames`, so a column declared with + * an empty name is keyed `(No column name)` rather than `""`. Two columns of ONE name are + * refused: the driver builds each row as `row[column.name] = value` (cassandra-driver + * 4.10.0, `lib/streams.js` `parseRows`), so the earlier column's value is gone before the + * row reaches this seam, and numbering the second header would only show the last value + * twice. */ export function toCassandraResult(result: CassandraResultSetLike): CassandraQueryResult { const pageState = result.pageState ?? null; const columns = result.columns ?? null; if (columns === null) return { rows: [], fieldNames: null, columnTypes: null, pageState }; + const declared = columns.map((column) => column.name); + const repeated = declared.find((name, index) => declared.indexOf(name) !== index); + if (repeated !== undefined) { + throw new CassandraTransportError( + `The result has two columns named "${repeated}", and the Cassandra driver keeps one value per column name, so one of them cannot be shown. Give each column its own alias.`, + "invalid", + null, + ); + } + const fieldNames = uniqueFieldNames(declared); + const rows: CassandraRow[] = (result.rows ?? []).map((row) => - Object.fromEntries(columns.map((column) => [column.name, normalizeCassandraValue(row[column.name])])), + Object.fromEntries(columns.map((column, index) => [fieldNames[index], normalizeCassandraValue(row[column.name])])), ); return { rows, - fieldNames: columns.map((column) => column.name), - columnTypes: Object.fromEntries(columns.map((column) => [column.name, describeColumnType(column.type)])), + fieldNames, + columnTypes: Object.fromEntries( + columns.map((column, index) => [fieldNames[index], describeColumnType(column.type)]), + ), pageState, }; } diff --git a/tests/integration/db/cassandra-provider.test.ts b/tests/integration/db/cassandra-provider.test.ts index 3f32b489e..ce3de545f 100644 --- a/tests/integration/db/cassandra-provider.test.ts +++ b/tests/integration/db/cassandra-provider.test.ts @@ -770,6 +770,17 @@ describe("query", () => { }); }); + test("two result columns of one name surface as a query error naming the column", async () => { + const twice = "SELECT id AS x, name AS x FROM probe.customers"; + // What cassandra-driver hands over for that statement: two declared columns and a row + // object holding only the last one's value. + const answer = result(declare(["x", INT], ["x", TEXT]), [{ x: "a" }]); + const { provider } = await connectedProvider(healthyReplies({ [twice]: answer })); + + await expect(provider.query(twice)).rejects.toThrow(QueryError); + await expect(provider.query(twice)).rejects.toThrow('The result has two columns named "x"'); + }); + test("a write reports no columns and no invented row count", async () => { const insert = "INSERT INTO probe.customers (id, name) VALUES (1, 'a')"; const { provider } = await connectedProvider(healthyReplies({ [insert]: VOID_RESULT })); diff --git a/tests/unit/db/cassandra/wire.test.ts b/tests/unit/db/cassandra/wire.test.ts index ead44e777..31efb1242 100644 --- a/tests/unit/db/cassandra/wire.test.ts +++ b/tests/unit/db/cassandra/wire.test.ts @@ -249,6 +249,39 @@ describe("toCassandraResult", () => { expect(result.pageState).toBeNull(); }); + test("a column declared with an empty name is named, so the grid can key it", () => { + const result = toCassandraResult({ + columns: [ + { name: "", type: { code: 9, info: null } }, + { name: "id", type: { code: 9, info: null } }, + ], + rows: [{ "": 7, id: 1 }], + pageState: null, + }); + + expect(result.fieldNames).toEqual(["(No column name)", "id"]); + expect(result.rows).toEqual([{ "(No column name)": 7, id: 1 }]); + expect(result.columnTypes).toEqual({ "(No column name)": "int", id: "int" }); + }); + + test("two columns of one name are refused, because the driver kept only the last value", () => { + // cassandra-driver 4.10.0 builds each row as `row[column.name] = value` + // (lib/streams.js parseRows), so `SELECT a AS x, b AS x` reaches this seam as one key + // holding b. Showing both headers would show b twice; the first value is gone. + const read = () => + toCassandraResult({ + columns: [ + { name: "x", type: { code: 9, info: null } }, + { name: "x", type: { code: 13, info: null } }, + ], + rows: [{ x: "b" }], + pageState: null, + }); + + expect(read).toThrow(CassandraTransportError); + expect(read).toThrow('The result has two columns named "x"'); + }); + test("a void result declares no columns at all", () => { // Measured: an INSERT, an ALTER and a `USE` all answer a ResultSet whose // `columns` is null - not an empty array - and whose rows are empty. From f5a8d2d4e8644f78df5d21a9280191f07962021f Mon Sep 17 00:00:00 2001 From: cevheri Date: Wed, 7 Oct 2026 11:11:08 +0300 Subject: [PATCH 10/46] fix(db): number repeated columns with the shared helper in Druid, search, Db2 and Trino The four private copies of the repeat-numbering rule never reserved a name the result declares later, so `SELECT 1 AS a, 2 AS a, 3 AS "a (2)"` showed the user's own `a (2)` column under a generated name. Each transport now calls uniqueFieldNames, which numbers that repeat `a (3)` and names an empty Trino declaration `(No column name)`. Docs state the new numbering. --- docs/providers/db2.md | 6 ++- docs/providers/druid.md | 15 +++++--- docs/providers/elasticsearch.md | 14 ++++--- docs/providers/opensearch.md | 6 +-- docs/providers/trino.md | 4 ++ src/lib/db/providers/sql/db2/values.ts | 23 ++--------- .../db/providers/sql/druid/http-transport.ts | 28 +++----------- .../db/providers/sql/search/http-transport.ts | 38 ++++++------------- .../db/providers/sql/trino/http-transport.ts | 26 +++++-------- tests/integration/db/db2-provider.test.ts | 14 +++++++ tests/integration/db/druid-provider.test.ts | 12 ++++++ .../db/elasticsearch-provider.test.ts | 18 +++++++++ .../db/opensearch-provider.test.ts | 2 +- tests/integration/db/trino-provider.test.ts | 19 ++++++++++ tests/unit/db/db2/values.test.ts | 13 +++++++ tests/unit/db/druid/http-transport.test.ts | 12 ++++++ tests/unit/db/trino/http-transport.test.ts | 20 ++++++++-- 17 files changed, 164 insertions(+), 106 deletions(-) diff --git a/docs/providers/db2.md b/docs/providers/db2.md index 5145faa23..2625bd2c2 100644 --- a/docs/providers/db2.md +++ b/docs/providers/db2.md @@ -166,7 +166,7 @@ Reading them is exact since 1.0.25: `SELECT *` over `APP.ALLTYPES` answers all 2 These were measured on 1.0.24, reported upstream one issue each, and fixed in [1.0.25](https://github.com/gurungabit/db2-node/releases/tag/v1.0.25); each now probes `GONE` on 12.1.0.0 and 11.5.9.0, and the provider workaround each one needed is gone with it unless the line says otherwise. - K4: a LOB or XML column read beside other columns came back wrong, lost rows or failed: a BLOB beside a CLOB answered the CLOB's bytes, a CLOB(1M) beside a GRAPHIC dropped rows when a later LOB was NULL, a CLOB beside a DOUBLE failed with "Protocol error: Invalid SQLDTAGRP indicator 0xEF", and `SELECT *` over `APP.ALLTYPES` answered 1 row of 3 and 23 of 25 columns ([gurungabit/db2-node#19](https://github.com/gurungabit/db2-node/issues/19), [#20](https://github.com/gurungabit/db2-node/issues/20), fixed by [#27](https://github.com/gurungabit/db2-node/pull/27)). Every such read, NULL LOB rows and a 50000-byte CLOB included, now answers what each column read alone answers. The integrity warning is gone; the preview still leaves CLOB, DBCLOB and BLOB out, for K24, and reads XML. -- K15: two columns of one name collapsed into one value in a row ([gurungabit/db2-node#21](https://github.com/gurungabit/db2-node/issues/21), fixed by [#28](https://github.com/gurungabit/db2-node/pull/28)). Object rows still keep only the last; the provider reads `rowMode: "array"`, which keeps both, and names the repeat `A (2)`, as the Druid and Trino providers do. +- K15: two columns of one name collapsed into one value in a row ([gurungabit/db2-node#21](https://github.com/gurungabit/db2-node/issues/21), fixed by [#28](https://github.com/gurungabit/db2-node/pull/28)). Object rows still keep only the last; the provider reads `rowMode: "array"`, which keeps both, and names the repeat `A (2)`, never a name the statement itself declares. - K16: a BOOLEAN parameter had to be a JS boolean, so the text `true` the grid's inline editor binds was refused ([gurungabit/db2-node#22](https://github.com/gurungabit/db2-node/issues/22), fixed by [#28](https://github.com/gurungabit/db2-node/pull/28)). The texts `true` and `false` are now stored as those booleans, so a BOOLEAN cell takes an inline edit. - K17: a failure the driver raised itself carried no SQLSTATE and nothing else to classify it by ([gurungabit/db2-node#23](https://github.com/gurungabit/db2-node/issues/23), fixed by [#28](https://github.com/gurungabit/db2-node/pull/28)). It still carries no SQLSTATE, which only a server gives, and now carries a `driverCode`, which the provider maps (section 7.1). - K23: a password holding `!`, `^`, `[`, `]` or `|` was sent wrongly and refused as "user id or password invalid" ([gurungabit/db2-node#25](https://github.com/gurungabit/db2-node/issues/25), fixed by [#29](https://github.com/gurungabit/db2-node/pull/29)). Measured with one user per character and one holding all five, over TLS under the default mechanism and under `userPassword` and without TLS under `userPassword`, through Bun and Node: every one signs in, the IBM CLP signs in with the same passwords over TCP, and a wrong password is still refused. The provider's refusal of those characters is gone. @@ -321,7 +321,9 @@ A statement may start with a comment, which 1.0.22 refused (K18). `fields` and `columnTypes` come from the driver's column metadata, so a zero-row result still has its header. A SELECT reports the number of rows returned; any other statement reports the driver's affected-row count, which is 0 for an UPDATE or DELETE that matched no row (on 1.0.22 it was -2147221503, K19). Each statement commits on its own. -The statement is read with `rowMode: "array"`, so two columns of one name keep both values: the repeat is named `A (2)`, then `A (3)`, skipping a name the statement already used, as the Druid and Trino providers name theirs (K15, fixed in 1.0.25). +The statement is read with `rowMode: "array"`, so two columns of one name keep both values: the repeat is named `A (2)`, then `A (3)`, never a name the statement itself declares, before or after the repeat (K15, fixed in 1.0.25). +So `SELECT 1 AS A, 2 AS A, 3 AS "A (2)"` comes back as `A`, `A (3)`, `A (2)`, and the user's own `A (2)` keeps its value. +Db2 names an unaliased expression itself (`1`, `2`, see below), so an empty name is not something it declares. The driver's diagnostics are passed through as result warnings, after the LOB warning of K24. A failure the driver raises itself carries a `driverCode` and no SQLSTATE: `DB2_PARAMETER_COUNT` and `DB2_PARAMETER_TYPE`, a wrong number of parameters or a value that does not fit its target, are a query error, and `DB2_PROTOCOL` and `DB2_INVALID_OPTION` are the driver's own and stay a plain database error whatever their words say, each with the driver's message (K17, fixed in 1.0.25). Every other error goes through the shared `mapDatabaseError()`, which reads the SQLSTATE and SQLCODE when the driver's message carries them. diff --git a/docs/providers/druid.md b/docs/providers/druid.md index 8971412df..1f907688f 100644 --- a/docs/providers/druid.md +++ b/docs/providers/druid.md @@ -326,9 +326,12 @@ convincing possible lie: a successful query, over the right datasource, that hap nothing. Data loss has to surface as an error, which is the same reason the mid-response truncation case above raises instead of returning what it managed to read. -Rows are rebuilt from the declared names, and a repeat is **disambiguated rather than overwritten**: -`SELECT 1 AS c, 2 AS c` reaches the grid as columns `c` and `c (2)`, both with their values. The -suffix keeps climbing (`c (3)`, …) because `SELECT 1 AS c, 2 AS "c (2)", 3 AS c` is legal too. Without +Rows are rebuilt from the declared names, and a repeat is **numbered rather than overwritten**: +`SELECT 1 AS c, 2 AS c` reaches the grid as columns `c` and `c (2)`, both with their values. A +number never takes a name the statement itself declares, before or after the repeat: +`SELECT 1 AS a, 2 AS a, 3 AS "a (2)"` reaches the grid as `a`, `a (3)`, `a (2)`, so the user's own +`a (2)` keeps its value. A header naming a column with an empty string would be shown as +`(No column name)`. Without this the array format would have been chosen and then thrown away one step later, since a row is a `Record`. @@ -862,11 +865,11 @@ both the reason and the alternative, which is more useful than anything this pro | Source | `QueryResult` field | Notes | |---|---|---| -| the data rows | `rows` | Rebuilt from the positional arrays, keyed by the disambiguated column names | -| header row 0 | `fields` | Declared column order, made unique (`c`, `c (2)`); `[]` when the payload carried no header | +| the data rows | `rows` | Rebuilt from the positional arrays, keyed by the unique column names | +| header row 0 | `fields` | Declared column order, made unique (`c`, `c (2)`, never a name the payload itself declares); `[]` when the payload carried no header | | — | `rowCount` | `rows.length`. There is no second number: no Druid statement mutates, so a mutation count could only ever be zero | | the measured exchange | `executionTime` | Rounded milliseconds, **measured by the transport**. The endpoint reports no timing whatsoever, so there is no server-side number this could be preferred over ([§3.2](#32-the-transport-seam-one-interface-one-implementation)) | -| header row 2 (SQL types) | `columnTypes` | Keyed by the same disambiguated names; **absent** when the payload declared no types. The native types (header row 1) deliberately do not travel — they lie for an expression ([§3.5](#35-the-sql-type-labels-the-column-because-the-native-type-lies)) | +| header row 2 (SQL types) | `columnTypes` | Keyed by the same unique names; **absent** when the payload declared no types. The native types (header row 1) deliberately do not travel — they lie for an expression ([§3.5](#35-the-sql-type-labels-the-column-because-the-native-type-lies)) | | `X-Druid-Response-Context.missingSegments` | `warnings` | One warning naming how many segments were unavailable, and **absent** for a whole answer or an answer that said nothing about availability ([§13](#13-known-limitations--future-work)) | **`COUNT(DISTINCT ...)` answers an estimate.** Druid documents it as an alias for diff --git a/docs/providers/elasticsearch.md b/docs/providers/elasticsearch.md index 6443365f0..876bf52f9 100644 --- a/docs/providers/elasticsearch.md +++ b/docs/providers/elasticsearch.md @@ -256,10 +256,12 @@ Three properties the code depends on: as an object. The declared **order** is therefore authoritative in a way object keys never are. - **Duplicate output names are legal here.** Measured, `SELECT 1 AS c, 2 AS c, 3 AS c` answers HTTP 200 with `[{"name":"c",…},{"name":"c",…},{"name":"c",…}]` and the row `[1,2,3]`. A `SearchRow` is a - record, so without `disambiguate()` - ([http-transport.ts:695](../../src/lib/db/providers/sql/search/http-transport.ts)) the second and - third values would vanish **before** the seam. They reach the grid as `c`, `c (2)`, `c (3)`, and the - suffix keeps climbing because `SELECT 1 AS c, 2 AS "c (2)", 3 AS c` is legal too. **The same + record, so without `uniqueFieldNames()` + ([result-fields.ts](../../src/lib/db/utils/result-fields.ts)) the second and + third values would vanish **before** the seam. They reach the grid as `c`, `c (2)`, `c (3)`. A + number never takes a name the statement itself declares, before or after the repeat: + `SELECT 1 AS a, 2 AS a, 3 AS "a (2)"` reaches the grid as `a`, `a (3)`, `a (2)`, so the user's own + `a (2)` keeps its value. A column declared with no name would be shown as `(No column name)`. **The same statement is refused outright by OpenSearch**, so this invariant is load-bearing on exactly one of the two products — which is a fact about that engine, not dead code ([opensearch.md §3.4](./opensearch.md#34-the-success-envelope-schemadatarows-a-separate-alias-and-a-count)). @@ -595,8 +597,8 @@ everything it *would* have accepted, which is more useful than anything substitu | Source | `QueryResult` field | Notes | |---|---|---| -| `rows` | `rows` | Rebuilt from the positional arrays, keyed by the disambiguated column names; an integer past 2^53 arrives as its exact digits ([§5.3](#53-row-values-arrive-as-the-mappings-json)) | -| `columns[].name` | `fields` | Declared order, made unique (`c`, `c (2)`); `[]` when the answer described no columns | +| `rows` | `rows` | Rebuilt from the positional arrays, keyed by the unique column names; an integer past 2^53 arrives as its exact digits ([§5.3](#53-row-values-arrive-as-the-mappings-json)) | +| `columns[].name` | `fields` | Declared order, made unique (`c`, `c (2)`, never a name the answer itself declares); `[]` when the answer described no columns | | — | `rowCount` | `rows.length`. There is no second number: no statement here mutates, so a mutation count could only ever be zero | | the measured exchange | `executionTime` | Rounded milliseconds, **measured by this process**. Neither the body nor the headers carry any timing, so there is no server number to prefer | | `columns[].type` | `columnTypes` | The engine's **mapping** types (`keyword`, `double`, `datetime`); **absent** when the answer declared none ([§3.9](#39-columns-are-labelled-with-mapping-types-not-sql-types)) | diff --git a/docs/providers/opensearch.md b/docs/providers/opensearch.md index cd7659c37..c5bab97d2 100644 --- a/docs/providers/opensearch.md +++ b/docs/providers/opensearch.md @@ -259,11 +259,11 @@ Four properties the code depends on: must show. - **Rows are positional**, so each row is rebuilt against the declared column list rather than read as an object; the declared **order** is authoritative in a way object keys never are. -- **A duplicate output name is REFUSED, not disambiguated.** Measured, `SELECT 1 AS c, 2 AS c` answers +- **A duplicate output name is REFUSED, not numbered.** Measured, `SELECT 1 AS c, 2 AS c` answers HTTP 400, `IllegalArgumentException`, "Multiple entries with same key: c=2 and c=1" — where upstream answers 200 with two columns named `c`. So the seam's uniqueness invariant is load-bearing on exactly - one of the two products, and `disambiguate()` - ([http-transport.ts:695](../../src/lib/db/providers/sql/search/http-transport.ts)) can never fire + one of the two products, and the numbering in `uniqueFieldNames()` + ([result-fields.ts](../../src/lib/db/utils/result-fields.ts)) can never fire here. That is a fact about this engine, not dead code. - **`total` and `size` accompany every answer**, so `SearchQueryResult.totalHits` is a real number here and `null` upstream. It is deliberately **not used** by the provider diff --git a/docs/providers/trino.md b/docs/providers/trino.md index a9b619f96..eb77eebbd 100644 --- a/docs/providers/trino.md +++ b/docs/providers/trino.md @@ -417,6 +417,10 @@ counter above 2^53 would arrive as a string and `numberField` would read it as a Column labels are taken from the page's declaration and **de-duplicated** — a second column called `x` becomes `x (2)` — because the seam promises `fieldNames` is exactly the key set of every row. +A number never takes a name the statement itself declares, before or after the repeat: +`SELECT 1 AS a, 2 AS a, 3 AS "a (2)"` reaches the grid as `a`, `a (3)`, `a (2)`, so the user's own +`a (2)` keeps its value. Trino names an unaliased expression itself (`_col0`), so a declaration with +no name is not something the engine sends; if one ever arrives it is shown as `(No column name)`. ### 3.12 Statelessness is a warning, not a silent surprise diff --git a/src/lib/db/providers/sql/db2/values.ts b/src/lib/db/providers/sql/db2/values.ts index aa17cc014..96bbd8cf5 100644 --- a/src/lib/db/providers/sql/db2/values.ts +++ b/src/lib/db/providers/sql/db2/values.ts @@ -12,6 +12,7 @@ */ import type { PreviewProjection, QueryResult, QueryWarning } from "@/lib/db/types"; +import { uniqueFieldNames } from "@/lib/db/utils/result-fields"; import { declaredColumnTypes } from "../column-types"; import type { Db2ArrayQueryResult, Db2ColumnMeta } from "./driver"; @@ -98,30 +99,14 @@ function largeObjectEditWarning(types: readonly (readonly [string, string])[]): ]; } -/** - * The column names made unique, as the Druid and Trino transports make theirs. - * - * `SELECT 1 AS A, 2 AS A` really declares two columns named `A`, and a row the grid reads is a - * record, so the repeat is numbered while the record is built or its value is lost (K15). The - * number keeps climbing past a name the statement already used, so every field stays unique. - */ -function uniqueNames(declared: readonly string[]): string[] { - const taken = new Set(); - return declared.map((name) => { - let unique = name; - for (let repeat = 2; taken.has(unique); repeat += 1) unique = `${name} (${repeat})`; - taken.add(unique); - return unique; - }); -} - /** * One driver result, read with `rowMode: "array"`, as the product's `QueryResult` without its * execution time. Array rows are what keep a duplicated column's every value (K15, fixed in - * 1.0.25): an object row keys by name and keeps the last. + * 1.0.25): an object row keys by name and keeps the last. `SELECT 1 AS A, 2 AS A` really declares + * two columns named `A`, so the repeat is numbered while the record is built. */ export function readResult(result: Db2ArrayQueryResult): Omit { - const fields = uniqueNames(result.columns.map((column) => column.name)); + const fields = uniqueFieldNames(result.columns.map((column) => column.name)); const types = result.columns.map((column, index) => [fields[index], db2TypeName(column)] as const); const isResultSet = result.columns.length > 0; const rows = result.rows.map((row) => Object.fromEntries(fields.map((field, index) => [field, row[index]]))); diff --git a/src/lib/db/providers/sql/druid/http-transport.ts b/src/lib/db/providers/sql/druid/http-transport.ts index 86c2fad1b..b4c9622c3 100644 --- a/src/lib/db/providers/sql/druid/http-transport.ts +++ b/src/lib/db/providers/sql/druid/http-transport.ts @@ -39,6 +39,7 @@ import type { DatabaseConnection } from "@/lib/db/types"; // An explain strategy may not import from a provider directory, which is why this // lives in db/utils rather than here. import { quoteUnsafeIntegers } from "@/lib/db/utils/json-integers"; +import { uniqueFieldNames } from "@/lib/db/utils/result-fields"; import { DRUID_TRANSPORT_FAILURE, type DruidQueryOptions, @@ -261,27 +262,7 @@ function unavailableSegmentCount(headers: Headers): number | null { // ============================================================================ /** - * The declared names, made unique. - * - * `SELECT 1 AS c, 2 AS c` really declares `["c","c"]` (live-verified), and a row - * is a record, so the repeat has to be disambiguated as the row is built or the - * second column disappears BEFORE the seam rather than after it. The suffix keeps - * climbing because `SELECT 1 AS c, 2 AS "c (2)", 3 AS c` is legal too, and - * uniqueness is the invariant the seam states. - */ -function disambiguate(declared: readonly string[]): string[] { - const taken = new Set(); - - return declared.map((name) => { - let unique = name; - for (let repeat = 2; taken.has(unique); repeat += 1) unique = `${name} (${repeat})`; - taken.add(unique); - return unique; - }); -} - -/** - * One type per column, keyed by the disambiguated name. + * One type per column, keyed by the unique name. * * A column the header row did not reach is left OUT rather than given a * placeholder: an invented type name would be indistinguishable from one the @@ -326,7 +307,10 @@ function toQueryResult( throw new DruidTransportError(UNREADABLE_PAYLOAD); } - const fieldNames = disambiguate((names as unknown[]).map(String)); + // `SELECT 1 AS c, 2 AS c` really declares `["c","c"]` (live-verified), and a row is a + // record, so the repeat is numbered as the row is built or the second column + // disappears BEFORE the seam rather than after it. + const fieldNames = uniqueFieldNames((names as unknown[]).map(String)); return { rows: payload.slice(HEADER_ROW_COUNT).map((row) => toRow(fieldNames, row)), fieldNames, diff --git a/src/lib/db/providers/sql/search/http-transport.ts b/src/lib/db/providers/sql/search/http-transport.ts index 797e3c4c1..b8421795c 100644 --- a/src/lib/db/providers/sql/search/http-transport.ts +++ b/src/lib/db/providers/sql/search/http-transport.ts @@ -27,7 +27,7 @@ * and is REFUSED outright by OpenSearch - HTTP 400, * `IllegalArgumentException`, "Multiple entries with same key: c=3 and c=2". * So the seam's uniqueness invariant is load-bearing on exactly one of the two - * products, and `disambiguate` below is what upholds it; on OpenSearch it can + * products, and `uniqueFieldNames` is what upholds it; on OpenSearch it can * never fire, which is a fact about that engine and not dead code. * 2. **Rows are positional on both** (`rows` / `datarows` are arrays of arrays), so * a row is rebuilt against the declared column list rather than read as an @@ -76,6 +76,7 @@ import { endpointUrl, type HttpOrigin, httpOrigin, rejectRedirect } from "@/lib/ import { httpTransportFetch } from "@/lib/db/http/egress-policy"; import type { DatabaseConnection } from "@/lib/db/types"; import { quoteUnsafeIntegers } from "@/lib/db/utils/json-integers"; +import { uniqueFieldNames } from "@/lib/db/utils/result-fields"; import { type SearchClusterHealth, type SearchDialectId, @@ -681,31 +682,6 @@ interface DeclaredColumn { type?: unknown; } -/** - * The declared names, made unique. - * - * Measured on Elasticsearch: `SELECT 1 AS c, 2 AS c, 3 AS c` answers HTTP 200 with - * three columns all named `c` and the row `[1,2,3]`. A `SearchRow` is a record, so - * without this the second and third values would vanish BEFORE the seam rather - * than after it, and `columnTypes` would silently describe only the last of them. - * The suffix keeps climbing because `SELECT 1 AS c, 2 AS "c (2)", 3 AS c` is legal - * too, and uniqueness is the invariant the seam states. - * - * On OpenSearch this can never fire - the same statement is refused with - * `IllegalArgumentException`, "Multiple entries with same key: c=3 and c=2" - which - * is a difference between the engines, not a reason to make the transport branch. - */ -function disambiguate(declared: readonly string[]): string[] { - const taken = new Set(); - - return declared.map((name) => { - let unique = name; - for (let repeat = 2; taken.has(unique); repeat += 1) unique = `${name} (${repeat})`; - taken.add(unique); - return unique; - }); -} - /** * Declared order and types, or nulls when the envelope described neither. * @@ -725,7 +701,15 @@ function describeColumns( const columns = declared as DeclaredColumn[]; // The alias is what the user typed and therefore what the grid must show. Only // OpenSearch keeps it separate from `name`; see `aliasKey` for the measurement. - const fieldNames = disambiguate( + // + // Measured on Elasticsearch: `SELECT 1 AS c, 2 AS c, 3 AS c` answers HTTP 200 with + // three columns all named `c` and the row `[1,2,3]`. A `SearchRow` is a record, so + // without unique names the second and third values would vanish BEFORE the seam, + // and `columnTypes` would silently describe only the last of them. On OpenSearch + // the same statement is refused with `IllegalArgumentException`, "Multiple entries + // with same key: c=3 and c=2", which is a difference between the engines, not a + // reason to make the transport branch. + const fieldNames = uniqueFieldNames( columns.map((column) => { const alias = spec.aliasKey === null ? undefined : (column as Record)[spec.aliasKey]; return String(typeof alias === "string" && alias.length > 0 ? alias : column.name); diff --git a/src/lib/db/providers/sql/trino/http-transport.ts b/src/lib/db/providers/sql/trino/http-transport.ts index 29900f0cc..43f5a0309 100644 --- a/src/lib/db/providers/sql/trino/http-transport.ts +++ b/src/lib/db/providers/sql/trino/http-transport.ts @@ -51,6 +51,7 @@ import type { DatabaseConnection } from "@/lib/db/types"; // Shared with the Druid transport, which is in the same position; ClickHouse escapes // it server-side instead (`output_format_json_quote_64bit_integers`, #264). import { quoteUnsafeIntegers } from "@/lib/db/utils/json-integers"; +import { uniqueFieldNames } from "@/lib/db/utils/result-fields"; import { resolveSqlGrammar, type SqlGrammar } from "@/lib/sql/grammar"; import { readStatementEnd } from "@/lib/sql/statement-end"; import { @@ -449,28 +450,19 @@ function delay(ms: number, signal?: AbortSignal): Promise { * * Measured on 476: `SELECT 1 AS c, 2 AS c` really declares * `[{name:"c",...},{name:"c",...}]`, and a row is a record, so the repeat has to - * be disambiguated while the row is built or the second column disappears BEFORE - * the seam rather than after it. The suffix keeps climbing because - * `SELECT 1 AS c, 2 AS "c (2)", 3 AS c` is legal too, and uniqueness is the - * invariant the seam states. + * be numbered while the row is built or the second column disappears BEFORE the + * seam rather than after it. A column declared with no name is shown as + * `(No column name)`, the shared helper's `UNNAMED_FIELD`. */ function readColumns(page: Record): { names: string[]; types: Record } | null { const declared = page[RESULT_FIELDS.COLUMNS]; if (!Array.isArray(declared)) return null; - const names: string[] = []; - const types: Record = {}; - const taken = new Set(); - - for (const entry of declared) { - const column = asRecord(entry) ?? {}; - const declaredName = textField(column, COLUMN_FIELDS.NAME) ?? ""; - let unique = declaredName; - for (let repeat = 2; taken.has(unique); repeat += 1) unique = `${declaredName} (${repeat})`; - taken.add(unique); - names.push(unique); - types[unique] = textField(column, COLUMN_FIELDS.TYPE) ?? ""; - } + const columns = declared.map((entry) => asRecord(entry) ?? {}); + const names = uniqueFieldNames(columns.map((column) => textField(column, COLUMN_FIELDS.NAME) ?? "")); + const types = Object.fromEntries( + names.map((name, index) => [name, textField(columns[index], COLUMN_FIELDS.TYPE) ?? ""]), + ); return { names, types }; } diff --git a/tests/integration/db/db2-provider.test.ts b/tests/integration/db/db2-provider.test.ts index 9ec5d644c..b95ad88e7 100644 --- a/tests/integration/db/db2-provider.test.ts +++ b/tests/integration/db/db2-provider.test.ts @@ -810,6 +810,20 @@ describe("Db2Provider: query", () => { expect(result).not.toHaveProperty("warnings"); }); + test("a repeat is numbered past a name the statement declares later", async () => { + userQuery = async () => ({ + rows: [[1, 2, 3]], + rowCount: 1, + columns: [column("A", "Integer"), column("A", "Integer"), column("A (2)", "Integer")], + diagnostics: [], + }); + const provider = await connected(); + const result = await provider.query('SELECT 1 AS A, 2 AS A, 3 AS "A (2)" FROM SYSIBM.SYSDUMMY1'); + + expect(result.fields).toEqual(["A", "A (3)", "A (2)"]); + expect(result.rows).toEqual([{ A: 1, "A (3)": 2, "A (2)": 3 }]); + }); + // K24, measured on 12.1.0.0 and 11.5.9.0 through 1.0.24 and 1.0.25: a value bound to a CLOB(1M) // answers 0 changed rows and is not written, so a result holding one says the grid does not edit it. test("a result holding a CLOB says the grid does not edit it (K24)", async () => { diff --git a/tests/integration/db/druid-provider.test.ts b/tests/integration/db/druid-provider.test.ts index 58fa6665f..ddb9b74ea 100644 --- a/tests/integration/db/druid-provider.test.ts +++ b/tests/integration/db/druid-provider.test.ts @@ -827,6 +827,18 @@ describe("DruidProvider query", () => { expect(result.rowCount).toBe(1); }); + test("numbers a repeat past a name the statement declares later", async () => { + // `SELECT 1 AS a, 2 AS a, 3 AS "a (2)"`: the third column is the user's own `a (2)`, + // so the repeat of `a` is numbered `a (3)` and every value stays under its own name. + const provider = await connectProvider(); + replyFor = () => ok('[["a","a","a (2)"],["LONG","LONG","LONG"],["INTEGER","INTEGER","INTEGER"],[1,2,3]]'); + + const result = await provider.query('SELECT 1 AS a, 2 AS a, 3 AS "a (2)"'); + + expect(result.fields).toEqual(["a", "a (3)", "a (2)"]); + expect(result.rows).toEqual([{ a: 1, "a (3)": 2, "a (2)": 3 }]); + }); + test("delivers a 64-bit id exactly, as the string it has to become", async () => { // The value on the wire is the unquoted number 9007199254740993, which // JSON.parse turns into ...992 with no error at all. Druid has no diff --git a/tests/integration/db/elasticsearch-provider.test.ts b/tests/integration/db/elasticsearch-provider.test.ts index a984b2509..5acdba281 100644 --- a/tests/integration/db/elasticsearch-provider.test.ts +++ b/tests/integration/db/elasticsearch-provider.test.ts @@ -1583,6 +1583,24 @@ describe("ElasticsearchProvider query", () => { expect(result.rowCount).toBe(1); }); + test("numbers a repeat past a name the statement declares later", async () => { + // `SELECT 1 AS a, 2 AS a, 3 AS "a (2)"`: the third column is the user's own `a (2)`, + // so the repeat of `a` is numbered `a (3)` and every value stays under its own name. + const provider = await connectProvider(); + overrideSql( + ok( + '{"columns":[{"name":"a","type":"integer"},{"name":"a","type":"long"},{"name":"a (2)","type":"integer"}],' + + '"rows":[[1,2,3]]}', + ), + ); + + const result = await provider.query('SELECT 1 AS a, 2 AS a, 3 AS "a (2)"'); + + expect(result.fields).toEqual(["a", "a (3)", "a (2)"]); + expect(result.rows).toEqual([{ a: 1, "a (3)": 2, "a (2)": 3 }]); + expect(result.columnTypes).toEqual({ a: "integer", "a (3)": "long", "a (2)": "integer" }); + }); + test("shows the alias the user typed, which this product folds into the column name", async () => { // `SELECT customer AS who` declares `{"name":"who"}` here and // `{"name":"customer","alias":"who"}` on OpenSearch, so reading `name` alone diff --git a/tests/integration/db/opensearch-provider.test.ts b/tests/integration/db/opensearch-provider.test.ts index 75e413e22..976b6e3dc 100644 --- a/tests/integration/db/opensearch-provider.test.ts +++ b/tests/integration/db/opensearch-provider.test.ts @@ -924,7 +924,7 @@ describe("OpenSearch faults", () => { test("refuses duplicate output names instead of needing them disambiguated", async () => { // `SELECT 1 AS c, 2 AS c` answers HTTP 200 on Elasticsearch with TWO columns - // named `c`, which is what `disambiguate` upholds the seam's uniqueness + // named `c`, which is what `uniqueFieldNames` upholds the seam's uniqueness // invariant against. Here the engine refuses the statement outright, so that // code can never fire on this product - a fact about the engine, not dead code. overridePath( diff --git a/tests/integration/db/trino-provider.test.ts b/tests/integration/db/trino-provider.test.ts index 2b39e40b8..68d9d1cad 100644 --- a/tests/integration/db/trino-provider.test.ts +++ b/tests/integration/db/trino-provider.test.ts @@ -805,6 +805,25 @@ describe("TrinoProvider query", () => { expect(result.rows).toEqual([{ c: 1, "c (2)": 2 }]); }); + test("numbers a repeat past a name the statement declares later", async () => { + const provider = await connectProvider(); + overrideSurface( + "SELECT 1 AS a", + rows( + [ + { name: "a", type: "integer" }, + { name: "a", type: "integer" }, + { name: "a (2)", type: "integer" }, + ], + [[1, 2, 3]], + ), + ); + const result = await provider.query('SELECT 1 AS a, 2 AS a, 3 AS "a (2)"'); + + expect(result.fields).toEqual(["a", "a (3)", "a (2)"]); + expect(result.rows).toEqual([{ a: 1, "a (3)": 2, "a (2)": 3 }]); + }); + /** * The one value the transport rewrites, proved through the PROVIDER rather than * through the seam alone, because this is the layer a caller actually reads. diff --git a/tests/unit/db/db2/values.test.ts b/tests/unit/db/db2/values.test.ts index 05e4a71c5..95dc8eec3 100644 --- a/tests/unit/db/db2/values.test.ts +++ b/tests/unit/db/db2/values.test.ts @@ -127,6 +127,19 @@ describe("readResult", () => { expect(read.rows).toEqual([{ A: 1, "A (2)": 2, "A (3)": 3 }]); }); + test("the numbering skips a name the statement declares later", () => { + const read = readResult( + result({ + columns: [column("A", "Integer"), column("A", "BigInt"), column("A (2)", "Integer")], + rows: [[1, 2, 3]], + }), + ); + + expect(read.fields).toEqual(["A", "A (3)", "A (2)"]); + expect(read.rows).toEqual([{ A: 1, "A (3)": 2, "A (2)": 3 }]); + expect(read.columnTypes).toEqual({ A: "INTEGER", "A (3)": "BIGINT", "A (2)": "INTEGER" }); + }); + test("the driver's own diagnostics are passed through as warnings", () => { const read = readResult(result({ rowCount: 1, diagnostics: ["SQLSTATE 01003: null values were eliminated"] })); diff --git a/tests/unit/db/druid/http-transport.test.ts b/tests/unit/db/druid/http-transport.test.ts index 6e63bcc3f..cc01ea34c 100644 --- a/tests/unit/db/druid/http-transport.test.ts +++ b/tests/unit/db/druid/http-transport.test.ts @@ -419,6 +419,18 @@ describe("DruidHttpTransport results", () => { expect(result.rows).toEqual([{ c: 1, "c (2)": 2, "c (3)": 3 }]); }); + // A name the statement declares AFTER the repeat is the user's own column too, so + // the number skips it rather than showing that column under a generated name. + test("never numbers a repeat into a name the result declares later", async () => { + handler = () => respond('[["c","c","c (2)"],["LONG","LONG","LONG"],["INTEGER","BIGINT","INTEGER"],[1,2,3]]'); + + const result = await makeTransport().query('SELECT 1 AS c, 2 AS c, 3 AS "c (2)"'); + + expect(result.fieldNames).toEqual(["c", "c (3)", "c (2)"]); + expect(result.rows).toEqual([{ c: 1, "c (3)": 2, "c (2)": 3 }]); + expect(result.sqlTypes).toEqual({ c: "INTEGER", "c (3)": "BIGINT", "c (2)": "INTEGER" }); + }); + // Live-verified: `SELECT id FROM libredb_demo WHERE id = -1` answers // `[["id"],["LONG"],["BIGINT"]]` - all three header rows, no data. test("describes the columns of a result set with no rows", async () => { diff --git a/tests/unit/db/trino/http-transport.test.ts b/tests/unit/db/trino/http-transport.test.ts index 3f260fdf4..6a36804a5 100644 --- a/tests/unit/db/trino/http-transport.test.ts +++ b/tests/unit/db/trino/http-transport.test.ts @@ -867,6 +867,19 @@ describe("TrinoHttpTransport result", () => { expect(result.fieldNames).toEqual(["c", "c (2)", "c (3)"]); }); + test("never numbers a repeat into a name the result declares later", async () => { + sequence( + `{"id":"${QUERY_ID}","columns":[{"name":"c","type":"integer"},{"name":"c","type":"bigint"},` + + `{"name":"c (2)","type":"integer"}],"data":[[1,2,3]],"stats":${FINISHED_STATS},"warnings":[]}`, + ); + + const result = await makeTransport().query('SELECT 1 AS c, 2 AS c, 3 AS "c (2)"'); + + expect(result.fieldNames).toEqual(["c", "c (3)", "c (2)"]); + expect(result.rows).toEqual([{ c: 1, "c (3)": 2, "c (2)": 3 }]); + expect(result.columnTypes).toEqual({ c: "integer", "c (3)": "bigint", "c (2)": "integer" }); + }); + test("passes every value encoding through untouched", async () => { sequence(TYPED_PAGE); @@ -1031,15 +1044,16 @@ describe("TrinoHttpTransport result", () => { ]); }); - test("names a column the declaration left unnamed rather than inventing one", async () => { + test("names a column the declaration left unnamed as an unnamed column", async () => { sequence( `{"id":"${QUERY_ID}","columns":[{"type":"integer"},"not-a-column"],"data":[[1,2]],"stats":${FINISHED_STATS},"warnings":[]}`, ); const result = await makeTransport().query("SELECT 1, 2"); - expect(result.fieldNames).toEqual(["", " (2)"]); - expect(result.columnTypes).toEqual({ "": "integer", " (2)": "" }); + expect(result.fieldNames).toEqual(["(No column name)", "(No column name) (2)"]); + expect(result.rows).toEqual([{ "(No column name)": 1, "(No column name) (2)": 2 }]); + expect(result.columnTypes).toEqual({ "(No column name)": "integer", "(No column name) (2)": "" }); }); test("says the rows were never described when no page declared a column", async () => { From 2d9f170f0ab9a0cae0de7dbb6269d01fb9813bd2 Mon Sep 17 00:00:00 2001 From: cevheri Date: Wed, 7 Oct 2026 11:18:50 +0300 Subject: [PATCH 11/46] fix(db): refuse a row whose value count differs from its columns in Druid, search, Db2 and Trino Each transport read a row by position and padded a short one with null or dropped the tail of a long one, so a malformed answer reached the grid with invented or missing values. A count mismatch, or a row that is not an array, now raises the provider's own error. --- docs/providers/db2.md | 1 + docs/providers/druid.md | 3 +- docs/providers/elasticsearch.md | 3 +- docs/providers/opensearch.md | 3 +- docs/providers/trino.md | 1 + src/lib/db/providers/sql/db2/values.ts | 13 +++++++- .../db/providers/sql/druid/http-transport.ts | 18 +++++++++-- .../db/providers/sql/search/http-transport.ts | 30 ++++++++++++------- .../db/providers/sql/trino/http-transport.ts | 23 ++++++++++---- .../db/elasticsearch-provider.test.ts | 13 ++++++++ tests/unit/db/db2/values.test.ts | 11 +++++++ tests/unit/db/druid/http-transport.test.ts | 24 +++++++-------- tests/unit/db/trino/http-transport.test.ts | 18 ++++++----- 13 files changed, 118 insertions(+), 43 deletions(-) diff --git a/docs/providers/db2.md b/docs/providers/db2.md index 2625bd2c2..5632d15d6 100644 --- a/docs/providers/db2.md +++ b/docs/providers/db2.md @@ -324,6 +324,7 @@ Each statement commits on its own. The statement is read with `rowMode: "array"`, so two columns of one name keep both values: the repeat is named `A (2)`, then `A (3)`, never a name the statement itself declares, before or after the repeat (K15, fixed in 1.0.25). So `SELECT 1 AS A, 2 AS A, 3 AS "A (2)"` comes back as `A`, `A (3)`, `A (2)`, and the user's own `A (2)` keeps its value. Db2 names an unaliased expression itself (`1`, `2`, see below), so an empty name is not something it declares. +A row whose value count differs from the column count is refused with a query error rather than read by position, which would leave a value `undefined` or drop one. The driver's diagnostics are passed through as result warnings, after the LOB warning of K24. A failure the driver raises itself carries a `driverCode` and no SQLSTATE: `DB2_PARAMETER_COUNT` and `DB2_PARAMETER_TYPE`, a wrong number of parameters or a value that does not fit its target, are a query error, and `DB2_PROTOCOL` and `DB2_INVALID_OPTION` are the driver's own and stay a plain database error whatever their words say, each with the driver's message (K17, fixed in 1.0.25). Every other error goes through the shared `mapDatabaseError()`, which reads the SQLSTATE and SQLCODE when the driver's message carries them. diff --git a/docs/providers/druid.md b/docs/providers/druid.md index 1f907688f..7d4acd866 100644 --- a/docs/providers/druid.md +++ b/docs/providers/druid.md @@ -331,7 +331,8 @@ Rows are rebuilt from the declared names, and a repeat is **numbered rather than number never takes a name the statement itself declares, before or after the repeat: `SELECT 1 AS a, 2 AS a, 3 AS "a (2)"` reaches the grid as `a`, `a (3)`, `a (2)`, so the user's own `a (2)` keeps its value. A header naming a column with an empty string would be shown as -`(No column name)`. Without +`(No column name)`. A data row whose value count differs from the header is refused as +unreadable rather than padded with nulls or cut, the same reasoning as the short header above. Without this the array format would have been chosen and then thrown away one step later, since a row is a `Record`. diff --git a/docs/providers/elasticsearch.md b/docs/providers/elasticsearch.md index 876bf52f9..abc9f0d1d 100644 --- a/docs/providers/elasticsearch.md +++ b/docs/providers/elasticsearch.md @@ -253,7 +253,8 @@ Three properties the code depends on: - **Rows are positional**, so each row is rebuilt against the declared column list ([http-transport.ts:747](../../src/lib/db/providers/sql/search/http-transport.ts)) rather than read - as an object. The declared **order** is therefore authoritative in a way object keys never are. + as an object. The declared **order** is therefore authoritative in a way object keys never are, and a + row whose value count differs from the declaration is refused rather than padded with nulls or cut. - **Duplicate output names are legal here.** Measured, `SELECT 1 AS c, 2 AS c, 3 AS c` answers HTTP 200 with `[{"name":"c",…},{"name":"c",…},{"name":"c",…}]` and the row `[1,2,3]`. A `SearchRow` is a record, so without `uniqueFieldNames()` diff --git a/docs/providers/opensearch.md b/docs/providers/opensearch.md index c5bab97d2..9773c9e1b 100644 --- a/docs/providers/opensearch.md +++ b/docs/providers/opensearch.md @@ -258,7 +258,8 @@ Four properties the code depends on: when the dialect declares an `aliasKey`, and the alias is what the user typed, so it is what the grid must show. - **Rows are positional**, so each row is rebuilt against the declared column list rather than read as - an object; the declared **order** is authoritative in a way object keys never are. + an object; the declared **order** is authoritative in a way object keys never are, and a row whose + value count differs from the declaration is refused rather than padded with nulls or cut. - **A duplicate output name is REFUSED, not numbered.** Measured, `SELECT 1 AS c, 2 AS c` answers HTTP 400, `IllegalArgumentException`, "Multiple entries with same key: c=2 and c=1" — where upstream answers 200 with two columns named `c`. So the seam's uniqueness invariant is load-bearing on exactly diff --git a/docs/providers/trino.md b/docs/providers/trino.md index eb77eebbd..15f7969df 100644 --- a/docs/providers/trino.md +++ b/docs/providers/trino.md @@ -421,6 +421,7 @@ A number never takes a name the statement itself declares, before or after the r `SELECT 1 AS a, 2 AS a, 3 AS "a (2)"` reaches the grid as `a`, `a (3)`, `a (2)`, so the user's own `a (2)` keeps its value. Trino names an unaliased expression itself (`_col0`), so a declaration with no name is not something the engine sends; if one ever arrives it is shown as `(No column name)`. +A row whose value count differs from the declaration is refused rather than padded with nulls or cut. ### 3.12 Statelessness is a warning, not a silent surprise diff --git a/src/lib/db/providers/sql/db2/values.ts b/src/lib/db/providers/sql/db2/values.ts index 96bbd8cf5..20b45d9ae 100644 --- a/src/lib/db/providers/sql/db2/values.ts +++ b/src/lib/db/providers/sql/db2/values.ts @@ -14,6 +14,7 @@ import type { PreviewProjection, QueryResult, QueryWarning } from "@/lib/db/types"; import { uniqueFieldNames } from "@/lib/db/utils/result-fields"; import { declaredColumnTypes } from "../column-types"; +import { QueryError } from "../../../errors"; import type { Db2ArrayQueryResult, Db2ColumnMeta } from "./driver"; /** The driver's spellings that map one to one onto Db2's own type name. */ @@ -109,7 +110,17 @@ export function readResult(result: Db2ArrayQueryResult): Omit column.name)); const types = result.columns.map((column, index) => [fields[index], db2TypeName(column)] as const); const isResultSet = result.columns.length > 0; - const rows = result.rows.map((row) => Object.fromEntries(fields.map((field, index) => [field, row[index]]))); + const rows = result.rows.map((row) => { + // A row whose value count differs from the declaration cannot be read by position: a short + // one would carry `undefined` values and a long one would lose its tail, both silently. + if (row.length !== fields.length) { + throw new QueryError( + `Db2 answered a row with ${row.length} values for ${fields.length} columns, so the result cannot be read`, + "db2", + ); + } + return Object.fromEntries(fields.map((field, index) => [field, row[index]])); + }); const warnings: QueryWarning[] = [...largeObjectEditWarning(types), ...result.diagnostics].map((message) => ({ message, })); diff --git a/src/lib/db/providers/sql/druid/http-transport.ts b/src/lib/db/providers/sql/druid/http-transport.ts index b4c9622c3..2dd1d8169 100644 --- a/src/lib/db/providers/sql/druid/http-transport.ts +++ b/src/lib/db/providers/sql/druid/http-transport.ts @@ -276,11 +276,23 @@ function typesByName(fieldNames: readonly string[], row: unknown): Record [name, values[column] ?? null])); + return Object.fromEntries(fieldNames.map((name, column) => [name, values[column]])); } function toQueryResult( diff --git a/src/lib/db/providers/sql/search/http-transport.ts b/src/lib/db/providers/sql/search/http-transport.ts index b8421795c..150c0f265 100644 --- a/src/lib/db/providers/sql/search/http-transport.ts +++ b/src/lib/db/providers/sql/search/http-transport.ts @@ -730,14 +730,24 @@ function describeColumns( }; } -/** One positional row, rebuilt as the record the seam promises. */ -function toRow(fieldNames: readonly string[], row: unknown): SearchRow { - const values = Array.isArray(row) ? (row as unknown[]) : []; - - // `?? null` normalizes a row shorter than its declaration: measured never to - // happen, but the alternative is a key whose value is `undefined`, which the - // seam's "exactly the key set of every row" invariant does not allow. - return Object.fromEntries(fieldNames.map((name, column) => [name, values[column] ?? null])); +/** + * One positional row, rebuilt as the record the seam promises. + * + * A row whose value count differs from its declaration is measured never to + * happen, and it cannot be read by position: padding it would invent nulls and + * cutting it would drop values, both silently. So it is refused. + */ +function toRow(spec: SearchDialectSpec, fieldNames: readonly string[], row: unknown): SearchRow { + if (!Array.isArray(row) || row.length !== fieldNames.length) { + const count = Array.isArray(row) ? row.length : 0; + throw new SearchTransportError( + "engine", + `${spec.label} answered a row with ${count} values for ${fieldNames.length} columns, so the result cannot be read`, + ); + } + const values = row as unknown[]; + + return Object.fromEntries(fieldNames.map((name, column) => [name, values[column]])); } /** @@ -766,7 +776,7 @@ function rebuildRows( const rows = envelope[spec.rowsKey]; if (fieldNames === null || !Array.isArray(rows)) return []; - return (rows as unknown[]).map((row) => toRow(fieldNames, row)); + return (rows as unknown[]).map((row) => toRow(spec, fieldNames, row)); } function toQueryResult(spec: SearchDialectSpec, envelope: Record): SearchQueryResult { @@ -777,7 +787,7 @@ function toQueryResult(spec: SearchDialectSpec, envelope: Record toRow(described.fieldNames as string[], row)), + : (rows as unknown[]).map((row) => toRow(spec, described.fieldNames as string[], row)), ...described, // Null on Elasticsearch by construction (`totalKey` is null): the product // sends no count, and the seam requires "unknown" rather than zero. diff --git a/src/lib/db/providers/sql/trino/http-transport.ts b/src/lib/db/providers/sql/trino/http-transport.ts index 43f5a0309..37fc4fe96 100644 --- a/src/lib/db/providers/sql/trino/http-transport.ts +++ b/src/lib/db/providers/sql/trino/http-transport.ts @@ -490,10 +490,23 @@ function readData(dialect: TrinoDialect, page: Record): unknown return data; } -/** One positional row, rebuilt as the record the seam promises. */ -function toRow(fieldNames: readonly string[], row: unknown): TrinoRow { - const values = Array.isArray(row) ? (row as unknown[]) : []; - return Object.fromEntries(fieldNames.map((name, column) => [name, values[column] ?? null])); +/** + * One positional row, rebuilt as the record the seam promises. + * + * A row whose value count differs from its declaration cannot be read by + * position: padding it would invent nulls and cutting it would drop values, + * both silently. So it is refused. + */ +function toRow(dialect: TrinoDialect, fieldNames: readonly string[], row: unknown): TrinoRow { + if (!Array.isArray(row) || row.length !== fieldNames.length) { + const count = Array.isArray(row) ? row.length : 0; + throw new TrinoTransportError( + "engine", + `${dialect.displayName} answered a row with ${count} values for ${fieldNames.length} columns, so the result cannot be read`, + ); + } + const values = row as unknown[]; + return Object.fromEntries(fieldNames.map((name, column) => [name, values[column]])); } function readStats(page: Record): TrinoExecutionStats | null { @@ -828,7 +841,7 @@ export class TrinoHttpTransport implements TrinoTransport { } return { - rows: fieldNames === null ? [] : positional.map((row) => toRow(fieldNames, row)), + rows: fieldNames === null ? [] : positional.map((row) => toRow(this.dialect, fieldNames, row)), fieldNames, columnTypes, queryId, diff --git a/tests/integration/db/elasticsearch-provider.test.ts b/tests/integration/db/elasticsearch-provider.test.ts index 5acdba281..ce79d341c 100644 --- a/tests/integration/db/elasticsearch-provider.test.ts +++ b/tests/integration/db/elasticsearch-provider.test.ts @@ -1601,6 +1601,19 @@ describe("ElasticsearchProvider query", () => { expect(result.columnTypes).toEqual({ a: "integer", "a (3)": "long", "a (2)": "integer" }); }); + // A row whose value count differs from the declaration cannot be read by position: + // padding a short one invents nulls and cutting a long one drops values, both silently. + test.each<[string, string]>([ + ["short", "[[1]]"], + ["long", "[[1,2,3]]"], + ["not an array", '["not-a-row"]'], + ])("refuses a row the engine sent %s", async (_label, rows) => { + const provider = await connectProvider(); + overrideSql(ok(`{"columns":[{"name":"a","type":"integer"},{"name":"b","type":"integer"}],"rows":${rows}}`)); + + await expect(provider.query("SELECT a, b FROM probe_orders")).rejects.toThrow(/row with \d+ values? for 2 columns/); + }); + test("shows the alias the user typed, which this product folds into the column name", async () => { // `SELECT customer AS who` declares `{"name":"who"}` here and // `{"name":"customer","alias":"who"}` on OpenSearch, so reading `name` alone diff --git a/tests/unit/db/db2/values.test.ts b/tests/unit/db/db2/values.test.ts index 95dc8eec3..48b8d8b3e 100644 --- a/tests/unit/db/db2/values.test.ts +++ b/tests/unit/db/db2/values.test.ts @@ -140,6 +140,17 @@ describe("readResult", () => { expect(read.columnTypes).toEqual({ A: "INTEGER", "A (3)": "BIGINT", "A (2)": "INTEGER" }); }); + // A row whose value count differs from the declaration cannot be read by position: a short + // one would carry `undefined` values and a long one would lose its tail, both silently. + test.each<[string, unknown[]]>([ + ["short", [1]], + ["long", [1, 2, 3]], + ])("a %s row is refused rather than read by position", (_label, row) => { + expect(() => + readResult(result({ columns: [column("A", "Integer"), column("B", "Integer")], rows: [row] })), + ).toThrow(/row with \d+ values for 2 columns/); + }); + test("the driver's own diagnostics are passed through as warnings", () => { const read = readResult(result({ rowCount: 1, diagnostics: ["SQLSTATE 01003: null values were eliminated"] })); diff --git a/tests/unit/db/druid/http-transport.test.ts b/tests/unit/db/druid/http-transport.test.ts index cc01ea34c..3d292190d 100644 --- a/tests/unit/db/druid/http-transport.test.ts +++ b/tests/unit/db/druid/http-transport.test.ts @@ -468,20 +468,18 @@ describe("DruidHttpTransport results", () => { await expect(makeTransport().query("SELECT id FROM libredb_demo")).rejects.toThrow(/incomplete/i); }); - test("fills a short data row with nulls rather than dropping the row", async () => { - handler = () => respond('[["a","b"],["LONG","LONG"],["BIGINT","BIGINT"],[1]]'); - - const result = await makeTransport().query("SELECT a, b FROM t"); - - expect(result.rows).toEqual([{ a: 1, b: null }]); - }); - - test("reads a row that is not an array as one with no values", async () => { - handler = () => respond('[["a"],["LONG"],["BIGINT"],7]'); - - const result = await makeTransport().query("SELECT a FROM t"); + // A row whose value count differs from its header cannot be read by position: + // padding a short one invents nulls and cutting a long one drops values, both + // silently. A truncated body or a proxy rewrite is the only way to get one, so + // it fails like the short header above. + test.each<[string, string]>([ + ["a short data row", '[["a","b"],["LONG","LONG"],["BIGINT","BIGINT"],[1]]'], + ["a long data row", '[["a","b"],["LONG","LONG"],["BIGINT","BIGINT"],[1,2,3]]'], + ["a row that is not an array", '[["a"],["LONG"],["BIGINT"],7]'], + ])("raises on %s rather than inventing or dropping values", async (_label, body) => { + handler = () => respond(body); - expect(result.rows).toEqual([{ a: null }]); + await expect(makeTransport().query("SELECT a, b FROM t")).rejects.toThrow(/row with \d+ values? for \d+ columns?/); }); // Never fabricate a type: a types row something rewrote describes fewer columns diff --git a/tests/unit/db/trino/http-transport.test.ts b/tests/unit/db/trino/http-transport.test.ts index 6a36804a5..e372f7d36 100644 --- a/tests/unit/db/trino/http-transport.test.ts +++ b/tests/unit/db/trino/http-transport.test.ts @@ -1030,18 +1030,20 @@ describe("TrinoHttpTransport result", () => { expect(result.affectedRows).toBeNull(); }); - test("pads a row the server sent short", async () => { + // A row whose value count differs from its declaration cannot be read by + // position: padding a short one invents nulls and cutting a long one drops + // values, both silently. + test.each<[string, string]>([ + ["short", "[[1]]"], + ["long", "[[1,2,3]]"], + ["not an array", '["not-a-row"]'], + ])("raises on a row the server sent %s", async (_label, data) => { sequence( `{"id":"${QUERY_ID}","columns":[{"name":"a","type":"integer"},{"name":"b","type":"integer"}],` + - `"data":[[1],"not-a-row"],"stats":${FINISHED_STATS},"warnings":[]}`, + `"data":${data},"stats":${FINISHED_STATS},"warnings":[]}`, ); - const result = await makeTransport().query("SELECT a, b"); - - expect(result.rows).toEqual([ - { a: 1, b: null }, - { a: null, b: null }, - ]); + await expect(makeTransport().query("SELECT a, b")).rejects.toThrow(/row with \d+ values? for 2 columns/); }); test("names a column the declaration left unnamed as an unnamed column", async () => { From 3ab5ca3f773bc44b12b7c5918c4d4973e7b57fe1 Mon Sep 17 00:00:00 2001 From: cevheri Date: Wed, 7 Oct 2026 11:18:50 +0300 Subject: [PATCH 12/46] fix(db): name a search column declared without a name as an unnamed column A declaration with no name member was keyed "undefined"; it now reaches the shared helper as an empty name and shows as (No column name), as Trino already does. --- src/lib/db/providers/sql/search/http-transport.ts | 2 +- tests/integration/db/elasticsearch-provider.test.ts | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/lib/db/providers/sql/search/http-transport.ts b/src/lib/db/providers/sql/search/http-transport.ts index 150c0f265..f4eaec03d 100644 --- a/src/lib/db/providers/sql/search/http-transport.ts +++ b/src/lib/db/providers/sql/search/http-transport.ts @@ -712,7 +712,7 @@ function describeColumns( const fieldNames = uniqueFieldNames( columns.map((column) => { const alias = spec.aliasKey === null ? undefined : (column as Record)[spec.aliasKey]; - return String(typeof alias === "string" && alias.length > 0 ? alias : column.name); + return String(typeof alias === "string" && alias.length > 0 ? alias : (column.name ?? "")); }), ); diff --git a/tests/integration/db/elasticsearch-provider.test.ts b/tests/integration/db/elasticsearch-provider.test.ts index ce79d341c..9a86d2dfa 100644 --- a/tests/integration/db/elasticsearch-provider.test.ts +++ b/tests/integration/db/elasticsearch-provider.test.ts @@ -1601,6 +1601,18 @@ describe("ElasticsearchProvider query", () => { expect(result.columnTypes).toEqual({ a: "integer", "a (3)": "long", "a (2)": "integer" }); }); + test("shows a column declared with no name, or an empty one, as an unnamed column", async () => { + // Measured never to happen: the engine names an unaliased expression with its own text. + // A declaration without one still gets a name the grid can key, never "undefined". + const provider = await connectProvider(); + overrideSql(ok('{"columns":[{"type":"integer"},{"name":"","type":"long"}],"rows":[[1,2]]}')); + + const result = await provider.query("SELECT 1, 2"); + + expect(result.fields).toEqual(["(No column name)", "(No column name) (2)"]); + expect(result.rows).toEqual([{ "(No column name)": 1, "(No column name) (2)": 2 }]); + }); + // A row whose value count differs from the declaration cannot be read by position: // padding a short one invents nulls and cutting a long one drops values, both silently. test.each<[string, string]>([ From 15690e2b2f204360ceb4d2321cd4862f9b957f14 Mon Sep 17 00:00:00 2001 From: cevheri Date: Wed, 7 Oct 2026 11:16:46 +0300 Subject: [PATCH 13/46] fix(panel): recover the results panel from a render error and name it as one The panel's one ChunkBoundary caught every render error, called each a chunk that did not load, and never reset, so one result the grid could not draw (a SQL Server column with no name) left the Reload notice over every later result, mode and tab. lazyRetry and GraphView now throw a ChunkLoadError for a load that failed, and only that gets the Reload copy; any other error says the view could not be displayed, with its message when it has one, and offers Try again. The boundary resets when its resetKeys change, and BottomPanel keys it by mode, result, agent artifact and explain plan. --- src/components/LazyView.tsx | 101 +++++++++--- src/components/results-graph/GraphView.tsx | 7 +- src/components/studio/BottomPanel.tsx | 6 +- src/lib/lazy.ts | 30 +++- tests/components/LazyView.test.tsx | 146 +++++++++++++++++- tests/components/Studio.test.tsx | 8 +- .../results-graph/GraphView.test.tsx | 4 +- tests/components/studio/BottomPanel.test.tsx | 53 ++++++- tests/unit/lib/lazy.test.ts | 26 +++- 9 files changed, 347 insertions(+), 34 deletions(-) diff --git a/src/components/LazyView.tsx b/src/components/LazyView.tsx index aa971ce8e..f3b2354ba 100644 --- a/src/components/LazyView.tsx +++ b/src/components/LazyView.tsx @@ -2,6 +2,7 @@ import React from "react"; import { LoaderCircle } from "lucide-react"; +import { ChunkLoadError } from "@/lib/lazy"; import { logger } from "@/lib/logger"; import { cn } from "@/lib/utils"; @@ -42,53 +43,113 @@ interface ChunkBoundaryProps { className?: string; /** Takes the failed view away, for a caller that can: the notice then offers Close. */ onDismiss?: () => void; + /** + * What the boundary is showing, for a caller whose one boundary holds many things (the + * results panel: every result, every mode, every tab). When any entry changes, by + * `Object.is`, a failure is cleared and the new children are rendered; a view that + * still fails throws again and is shown failing again. + */ + resetKeys?: readonly unknown[]; +} + +interface ChunkBoundaryState { + /** What the view threw, or null while it renders. */ + error: { thrown: unknown } | null; + /** The keys the current state belongs to. */ + resetKeys: readonly unknown[] | undefined; +} + +function sameKeys(a: readonly unknown[] | undefined, b: readonly unknown[] | undefined): boolean { + if (a === b) return true; + if (a === undefined || b === undefined || a.length !== b.length) return false; + return a.every((key, index) => Object.is(key, b[index])); +} + +function messageOf(thrown: unknown): string { + return thrown instanceof Error ? thrown.message : String(thrown); } /** - * The boundary a failed chunk lands in. + * The boundary a failed view lands in. * * A class, because catching a render-time error is still the one thing hooks cannot - * do. Reloading is the honest remedy rather than a local retry: the common cause is a - * page whose build no longer exists on the server, and no number of retries against - * the old chunk names will produce it — only re-fetching the document will. + * do. It tells two failures apart, by class and never by message (a production render + * error can have none): + * + * - a `ChunkLoadError`, a view whose code never arrived. Reloading is the honest remedy + * rather than a local retry: the common cause is a page whose build no longer exists + * on the server, and no number of retries against the old chunk names will produce + * it; only re-fetching the document will. + * - anything else, a view that threw while drawing what it was given. That is said as + * such, with the error's own message, and offers Try again; Reload would be a false + * diagnosis, and the next thing the boundary is given (`resetKeys`) clears it. */ -export class ChunkBoundary extends React.Component { - state = { failed: false }; +export class ChunkBoundary extends React.Component { + state: ChunkBoundaryState = { error: null, resetKeys: this.props.resetKeys }; + + static getDerivedStateFromError(thrown: unknown): Partial { + return { error: { thrown } }; + } - static getDerivedStateFromError() { - return { failed: true }; + static getDerivedStateFromProps( + props: ChunkBoundaryProps, + state: ChunkBoundaryState, + ): Partial | null { + if (sameKeys(props.resetKeys, state.resetKeys)) return null; + return { error: null, resetKeys: props.resetKeys }; } - componentDidCatch(error: Error) { - logger.warn("A split view could not be loaded", { + componentDidCatch(thrown: unknown) { + logger.warn(thrown instanceof ChunkLoadError ? "A split view could not be loaded" : "A view failed to render", { route: "ChunkBoundary", view: this.props.label, - error: error.message, + error: messageOf(thrown), }); } + private readonly retry = () => this.setState({ error: null }); + render() { - if (!this.state.failed) return this.props.children; + const { error } = this.state; + if (error === null) return this.props.children; + const isChunk = error.thrown instanceof ChunkLoadError; + const message = messageOf(error.thrown); return (
-

{this.props.label} could not be loaded.

-

- This view is fetched when it is first opened, and the request did not complete. If the server was upgraded - while this page was open, reloading picks up the new one. -

+ {isChunk ? ( + <> +

{this.props.label} could not be loaded.

+

+ This view is fetched when it is first opened, and the request did not complete. If the server was upgraded + while this page was open, reloading picks up the new one. +

+ + ) : ( + <> +

{this.props.label} could not be displayed.

+ {message !== "" && ( +

+ {message} +

+ )} + + )}
{this.props.onDismiss && (