From 3459e8915f3553ed72cb205735829a9a9a6aed30 Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Fri, 7 Aug 2026 16:55:02 +0000 Subject: [PATCH 1/2] refactor(drivers)!: aggregate takes DriverQuery, and two undeclared aliases retire (#6212 batch B, #6321) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three doors moved off `query: any` onto `DriverQuery`: `SqlDriver.aggregate`, `TursoDriver.aggregate` (the forwarding layer) and `RemoteTransport.aggregate`. `any` on a query parameter is not "the object name goes unchecked", it is every check off on exactly the members these bodies read. Three steps, in the order that makes the reds explicable: 1. Fixtures re-spelled to the declared keys (`aggregate:` -> `aggregations:`, `func:` -> `function:`) in driver-sql's and driver-sqlite-wasm's advanced + queryast suites. Non-test writers were zero, so this took the writer count of both undeclared keys to zero. 2. `query.aggregations || query.aggregate` and `agg.function || agg.func` deleted on both faces (ADR-0049 enforce-or-remove, #6321). The trailing `|| ''` on the remote face went with them: it only fired when neither key was written, and it made the two faces quote different spellings back. 3. Signatures narrowed. One real behaviour change, not a type-only one. `groupBy` is a `GroupByNode` union and the remote transport read it as `string[]`. The plain structured form (`{ field: 'region' }`) is spec-valid AND pushed down by objectql today, and it died here as `"[object Object]"` while the local face of the same driver compiled it — a live remote/local fork. It now reads `.field`. A date-bucketed entry genuinely cannot be compiled remotely and is refused with the ADR-0112 envelope (NOT_IMPLEMENTED/501). `SqlDriver`'s bare `throw` for the identical condition took the same envelope in the same commit, so the two faces keep one wire identity (#5240/#5907); a cross-package parity case compares their runtime messages. `alias` is deliberately not read, in step with `SqlDriver.aggregate`. Closes #6321 --- ...egate-driver-query-and-alias-retirement.md | 51 +++ .../src/sql-driver-advanced.test.ts | 22 +- ...l-driver-aggregate-undeclared-keys.test.ts | 220 +++++++++++ .../src/sql-driver-date-bucket.test.ts | 33 +- .../src/sql-driver-queryast.test.ts | 19 +- packages/drivers/driver-sql/src/sql-driver.ts | 84 ++++- .../src/sqlite-wasm-driver-advanced.test.ts | 22 +- .../sqlite-wasm-driver-date-bucket.test.ts | 24 +- .../src/sqlite-wasm-driver-queryast.test.ts | 13 +- .../src/remote-transport-groupby-node.test.ts | 347 ++++++++++++++++++ ...ransport-undeclared-aggregate-keys.test.ts | 216 +++++++++++ .../driver-turso/src/remote-transport.ts | 87 ++++- .../drivers/driver-turso/src/turso-driver.ts | 12 +- 13 files changed, 1101 insertions(+), 49 deletions(-) create mode 100644 .changeset/aggregate-driver-query-and-alias-retirement.md create mode 100644 packages/drivers/driver-sql/src/sql-driver-aggregate-undeclared-keys.test.ts create mode 100644 packages/drivers/driver-turso/src/remote-transport-groupby-node.test.ts create mode 100644 packages/drivers/driver-turso/src/remote-transport-undeclared-aggregate-keys.test.ts diff --git a/.changeset/aggregate-driver-query-and-alias-retirement.md b/.changeset/aggregate-driver-query-and-alias-retirement.md new file mode 100644 index 0000000000..1984529249 --- /dev/null +++ b/.changeset/aggregate-driver-query-and-alias-retirement.md @@ -0,0 +1,51 @@ +--- +"@objectstack/driver-sql": major +"@objectstack/driver-turso": major +--- + +refactor(drivers)!: `aggregate` 的 query 参数收窄到 `DriverQuery`，并退役 `aggregate` / `func` 两个未声明别名 (#6212 批 B、#6321) + +#5181（PR #6076）收窄了 `IDataDriver` 声明的六个方法，#6075（PR #6210）让五个驱动的实现跟上，#6212 批 A+E 处理了 SQL 驱动自有的另两道门。本次是同一条线上的 `aggregate`：`driver-sql`、`driver-turso` 的转发层与 `RemoteTransport` 三处，全部从 `query: any` 收到 `DriverQuery`（`@objectstack/spec/contracts`）。 + +`any` 在 query 参数上不是「对象名没检查」，而是**检查全关**：`where` 的 filter 方言、`groupBy` 的节点联合、`aggregations` 的节点形状——而这三样恰恰是这几个方法体读的全部内容。 + +## 一、退役两个协议从未声明的别名（#6321，ADR-0049） + +```ts +const aggregates = query.aggregations || query.aggregate; // driver-sql +const funcName = agg.function || agg.func; +const aggregations = query?.aggregations || query?.aggregate || []; // RemoteTransport +const func = String(agg.function || agg.func || ''); +``` + +`QueryASTSchema` 声明的是 `aggregations`，`AggregationNodeSchema` 声明的是 `function`；`aggregate` / `func` 在 `packages/spec` 里**一个字都没有**。实测全仓唯一书写者是这两个驱动包自己的 fixture（`sql-driver-advanced` 7 处、`sql-driver-queryast` 1 处、`sqlite-wasm-driver-advanced` 7 处、`sqlite-wasm-driver-queryast` 1 处），非测试面零书写者——#4984 那一家：**fixture 拼着别名，宽容分支就永远绿着活下去，没有任何测试能在删掉它时转红**。fixture 已按已声明拼写重拼，写者归零，PD#12 与 ADR-0049 enforce-or-remove 于是把这两条 `||` 一并删掉。 + +顺带删掉的还有 `|| ''`：它只在**两个键都没写**时才生效，而那时这一面把名字回引成 `""`、本地面回引成 `"undefined"`，同一份越界输入两种措辞（#5240）。别名在时这条岔路够不着，删别名恰恰让它够得着，所以同一次关掉。 + +**迁移**：`aggregate:` → `aggregations:`，`func:` → `function:`。写旧拼写的内联字面量现在是编译错误（TS2353）；越过 `tsc` 的 JS 调用方，`aggregate:` 会静默拿不到聚合列，`func:` 则拿到已有的具名 400（`INVALID_QUERY`，#5907）。本仓实测需要改动的非测试调用点为零。 + +## 二、一处真实行为改动：`RemoteTransport` 现在会编 `GroupByNode` 联合 + +`GroupByNodeSchema` 是 `z.union([z.string(), z.object({ field, dateGranularity?, alias? })])`，而这一层把它当 `string[]` 读。收窄后 `tsc` 直接把这条假设摆上台面（TS2322）。联合的两半状况完全不同，所以这不是一个 cast 能了事的： + +- **无 granularity 的结构化条目**（`{ field: 'region' }`）是 spec 合法、且**今天就会下推到驱动**的形状：objectql 的 aggregate 派发对它一律判为「受支持」（`engine.ts` 里逐字写着 `plain {field} object is fine`），`objectql/src/secret-fields.test.ts:341` 就是这个形状的活体。本驱动的**本地面**把它编成普通的 `GROUP BY "region"`，远端面却把它插值成 `"[object Object]"`、死在标识符安全检查里——一条查询两种答案、由连接串决定，正是 #6203 那个形状，而且**是活体不是休眠**：能力位 `queryDateGranularity` 只管带 granularity 的那一半，管不到这一半。现在读 `.field`，两面收敛。 +- **带 dateGranularity 的条目**远端确实编不出来，而这一点是**已声明**的：remote 模式发布 `queryDateGranularity: {}`，引擎据此全部落到内存分桶，因此不会下推。缺的是「绕过能力位、直连驱动」的那个调用方该得到什么答案——现在得到 ADR-0112 信封（`NOT_IMPLEMENTED` / 501），与聚合函数「协议已声明、本后端编不出」用的是同一类，而不是一句 SQL 注入告警。 + +`alias` **不读**，与本地面一致：`SqlDriver.aggregate` 也不读它，只在这一面读会是新的分叉而不是修复。 + +## 三、`SqlDriver` 那一面的同一条件也换上了信封 + +`SqlDriver.aggregate` 对「本方言编不出这个 granularity」原本抛裸 `Error`（`code`/`status` 皆 `undefined` ⇒ `mapDataError` 落默认分支，一个具名能力缺口以不透明 500 到达调用方）。只给远端面加信封就会造出 #5907 花一整个 issue 才关掉的那种分叉——`TursoDriver` 由 `url` 选面，同一条件不能有两种线上身份。两面首句逐字一致（`Date bucketing by '' is not supported by this backend.`），尾句各报**本面**编得出的 granularity，由一条跨包 parity 用例比对两个**运行时**消息钉住。 + +**消息文本变更**（可能影响按文本匹配的下游断言）： + +``` +- SqlDriver: dateGranularity 'week' not supported on dialect 'better-sqlite3'. Engine must fall back to in-memory bucketing. ++ Date bucketing by 'week' is not supported by this backend. Bucketed here: day, month, quarter, year (dialect 'better-sqlite3'). … (code=NOT_IMPLEMENTED, status=501) +``` + +## 定级依据 + +标 major 与 #5181 / #6075 / #6210 一致：**源码级破坏性**（调用点内联字面量、以及被删的两个别名键），加上第二、三节两处真实的运行期改动。`check:api-surface` 只记录导出的存在与否、不记录签名，所以这条说明是该变更唯一的下游载体。 + +`driver-sqlite-wasm` 未列入：它整个继承 `SqlDriver.aggregate`，自身源码零改动（改的只有它的 fixture 与一条断言）——与批 A+E 的处理一致。它读的是 driver-sql 的 `dist/*.d.ts`，因此验证时**必须先重建 driver-sql** 再 typecheck/test，否则是假绿。 diff --git a/packages/drivers/driver-sql/src/sql-driver-advanced.test.ts b/packages/drivers/driver-sql/src/sql-driver-advanced.test.ts index 1dbe61e86d..49cb53ec0c 100644 --- a/packages/drivers/driver-sql/src/sql-driver-advanced.test.ts +++ b/packages/drivers/driver-sql/src/sql-driver-advanced.test.ts @@ -42,7 +42,7 @@ describe('SqlDriver Advanced Operations (SQLite)', () => { it('should sum values', async () => { const result = await driver.aggregate('orders', { where: { status: 'completed' }, - aggregate: [{ func: 'sum', field: 'amount', alias: 'total_amount' }], + aggregations: [{ function: 'sum', field: 'amount', alias: 'total_amount' }], }); expect(result).toHaveLength(1); @@ -51,7 +51,7 @@ describe('SqlDriver Advanced Operations (SQLite)', () => { it('should count records', async () => { const result = await driver.aggregate('orders', { - aggregate: [{ func: 'count', field: '*', alias: 'total_orders' }], + aggregations: [{ function: 'count', field: '*', alias: 'total_orders' }], }); expect(result).toHaveLength(1); @@ -61,7 +61,7 @@ describe('SqlDriver Advanced Operations (SQLite)', () => { it('should calculate average', async () => { const result = await driver.aggregate('orders', { where: { status: 'completed' }, - aggregate: [{ func: 'avg', field: 'amount', alias: 'avg_amount' }], + aggregations: [{ function: 'avg', field: 'amount', alias: 'avg_amount' }], }); expect(result).toHaveLength(1); @@ -70,9 +70,9 @@ describe('SqlDriver Advanced Operations (SQLite)', () => { it('should find min and max values', async () => { const result = await driver.aggregate('orders', { - aggregate: [ - { func: 'min', field: 'amount', alias: 'min_amount' }, - { func: 'max', field: 'amount', alias: 'max_amount' }, + aggregations: [ + { function: 'min', field: 'amount', alias: 'min_amount' }, + { function: 'max', field: 'amount', alias: 'max_amount' }, ], }); @@ -84,9 +84,9 @@ describe('SqlDriver Advanced Operations (SQLite)', () => { it('should group by with aggregates', async () => { const result = await driver.aggregate('orders', { groupBy: ['customer'], - aggregate: [ - { func: 'sum', field: 'amount', alias: 'total_spent' }, - { func: 'count', field: '*', alias: 'order_count' }, + aggregations: [ + { function: 'sum', field: 'amount', alias: 'total_spent' }, + { function: 'count', field: '*', alias: 'order_count' }, ], }); @@ -104,7 +104,7 @@ describe('SqlDriver Advanced Operations (SQLite)', () => { it('should handle multiple group by fields', async () => { const result = await driver.aggregate('orders', { groupBy: ['customer', 'status'], - aggregate: [{ func: 'sum', field: 'quantity', alias: 'total_qty' }], + aggregations: [{ function: 'sum', field: 'quantity', alias: 'total_qty' }], }); expect(result.length).toBeGreaterThan(0); @@ -118,7 +118,7 @@ describe('SqlDriver Advanced Operations (SQLite)', () => { const result = await driver.aggregate('orders', { where: { status: { $ne: 'cancelled' } }, groupBy: ['product'], - aggregate: [{ func: 'sum', field: 'quantity', alias: 'total_quantity' }], + aggregations: [{ function: 'sum', field: 'quantity', alias: 'total_quantity' }], }); const laptop = result.find((r: any) => r.product === 'Laptop'); diff --git a/packages/drivers/driver-sql/src/sql-driver-aggregate-undeclared-keys.test.ts b/packages/drivers/driver-sql/src/sql-driver-aggregate-undeclared-keys.test.ts new file mode 100644 index 0000000000..2ef63cc328 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-aggregate-undeclared-keys.test.ts @@ -0,0 +1,220 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#6321] `aggregate` and `func` are not keys of the Query Protocol, and + * `SqlDriver.aggregate` no longer reads them. + * + * # What was there + * + * ```ts + * const aggregates = query.aggregations || query.aggregate; // sql-driver.ts + * const funcName = agg.function || agg.func; + * ``` + * + * `QueryASTSchema` declares `aggregations`; `AggregationNodeSchema` declares + * `function`. Neither `aggregate` nor `func` appears in `packages/spec` at all — + * they are a dialect this consumer tolerated, which PD#12 rejects: a lenient + * `||` in a consumer fossilises the wrong spelling into a second de-facto + * contract and hides the producer's bug. + * + * # Why nobody noticed + * + * This is #4984's family. The measurement that closed it: the ONLY writers of + * either key in the whole repository were `sql-driver-advanced.test.ts` (7), + * `sql-driver-queryast.test.ts` (1) and driver-sqlite-wasm's two mirrors (8) — + * the drivers' own fixtures. Non-test writers: zero. So the limbs were exercised + * on every run, always passed, and no test in existence could have gone red on + * their deletion. Re-spelling the fixtures to the declared keys took the writer + * count to zero, and ADR-0049 enforce-or-remove then says the tolerance goes. + * + * # ⚠️ This file pins a KEY, so its primary channel is `tsc`, not vitest + * + * What "`aggregations`/`function` are the only authoring surface" MEANS is that + * the other spelling does not compile. That is a compile-time fact, so the + * `@ts-expect-error` cases below are the load-bearing ones — and they are real + * checks rather than phantoms because `driver-sql/tsconfig.json` includes + * `src/**` and this file lives there (`PINS_CHECKED`, AGENTS.md). `vitest` never + * type-checks, so those cases can only fail under `pnpm typecheck`. + * + * They are written against `Parameters[1]` rather than + * against `DriverQuery` directly, because what this PR changed is that METHOD's + * parameter: read off the signature, the pin fails if the signature slips back + * to `any` even though `DriverQuery` itself is unharmed (spec's own + * `data-driver.test.ts` guards the type; nothing guarded the door). + * + * The runtime cases beside them record what an off-contract JS caller — one with + * no `tsc` in the loop at all — now gets, and they are what makes a + * REINTRODUCED limb go red in the test run too. + * + * # Reverse verification — direction predicted BEFORE it was run, per channel + * + * Restore the two `||` limbs and nothing else: + * + * - `pnpm test`: the two runtime cases go RED, in two different ways. The + * `aggregate:` case fails on `toBeUndefined()` — the limb computes the sum and + * the column appears. The `func:` case fails through `refusalOf`'s "expected a + * refusal, but it resolved" branch, because with `agg.func` read again there is + * a compilable function name and nothing refuses. + * - `pnpm typecheck`: UNCHANGED, green. Excess-property checking does not care + * what the body reads. + * + * Restore the `query: any` signature as well: + * + * - `pnpm typecheck` goes RED on the `@ts-expect-error` lines with TS2578 + * ("Unused '@ts-expect-error' directive"), which is the pin firing. + * - `pnpm test` is unaffected by that second revert on its own. + * + * Recording it per channel rather than as one number is the point: a reader who + * ran only vitest would conclude the key pins are dead, and a reader who ran only + * tsc would conclude the limb deletion is untested. Both halves are needed. + * + * Measured after writing the above, exactly as predicted: + * + * ``` + * limbs restored, signature kept vitest: 2 failed / 57 passed of 59 + * drops an `aggregate:` list -> AssertionError: expected 30 to be undefined + * refuses a `func:`-spelled -> Error: expected the driver to refuse the + * query, but it resolved + * tsc: clean + * signature back to `any` tsc: sql-driver-aggregate-undeclared-keys + * .test.ts(116,7) + (124,7) error TS2578: + * Unused '@ts-expect-error' directive. + * ``` + * + * Note which cases did NOT move under the first revert: the re-spelled fixtures + * in `sql-driver-advanced.test.ts` / `sql-driver-queryast.test.ts` stayed green, + * because a restored `a || b` still reads `a` first. That is the whole reason the + * fixture re-spelling had to come FIRST and cannot double as the pin — those + * files can no longer tell whether the alias limb exists. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { SqlDriver } from './index.js'; +import type { DriverQuery } from '@objectstack/spec/contracts'; + +interface WireBearingError extends Error { + code?: string; + status?: number; +} + +/** The narrowed door, read off the signature rather than restated. */ +type AggregateQuery = Parameters[1]; + +describe('[#6321] SqlDriver.aggregate reads only the declared aggregation keys', () => { + let driver: SqlDriver; + + beforeEach(async () => { + driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + await driver.initObjects([ + { + name: 'deal', + fields: { + id: { type: 'text', name: 'id' }, + stage: { type: 'text', name: 'stage' }, + amount: { type: 'number', name: 'amount' }, + }, + } as any, + ]); + await driver.create('deal', { id: '1', stage: 'won', amount: 10 }); + await driver.create('deal', { id: '2', stage: 'won', amount: 20 }); + await driver.create('deal', { id: '3', stage: 'lost', amount: 30 }); + }); + + // ── The compile-time half: neither key is an authoring surface ───────────── + // + // Each literal is kept on ONE line on purpose: `@ts-expect-error` covers the + // next line only, and the excess-property error is reported at the offending + // property, not at the `const`. + + describe('the undeclared spellings do not compile', () => { + it('rejects `aggregate:` where the protocol declares `aggregations:`', () => { + // @ts-expect-error [#6321] `aggregate` is not a key of QueryAST. + const undeclaredList: AggregateQuery = { aggregate: [{ function: 'sum', field: 'amount', alias: 'total' }] }; + // The directive above IS the assertion; the value is only touched so the + // binding is not dead code. + expect(Object.keys(undeclaredList)).toEqual(['aggregate']); + }); + + it('rejects `func:` where the protocol declares `function:`', () => { + // @ts-expect-error [#6321] `func` is not a key of AggregationNode — and + // the required `function` is then missing, so the line is wrong twice. + const undeclaredFunc: AggregateQuery = { aggregations: [{ func: 'sum', field: 'amount', alias: 'total' }] }; + expect(Object.keys(undeclaredFunc)).toEqual(['aggregations']); + }); + + it('admits the declared spelling — the pin is not green because nothing fits', () => { + // No directive here, deliberately. Without this case the two above would + // stay green if `AggregateQuery` degenerated into a type that admits + // nothing at all, which would be a broken door rather than a strict one. + const declared: AggregateQuery = { + groupBy: ['stage'], + aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }], + }; + expect(declared.aggregations).toHaveLength(1); + }); + }); + + // ── The runtime half: what an off-contract JS caller now gets ────────────── + + describe('an off-contract caller that writes them anyway', () => { + /** + * Spelled `as unknown as DriverQuery`, never `as any`: it names the contract + * being bypassed, keeps every other key checked, and greps as a deliberate + * act — the distinction `query-options/no-any-erasure` exists to enforce. + */ + const offContract = (q: Record) => q as unknown as DriverQuery; + + const refusalOf = async (query: DriverQuery): Promise => { + try { + await driver.aggregate('deal', query); + } catch (e) { + return e as WireBearingError; + } + throw new Error('expected the driver to refuse the query, but it resolved'); + }; + + it('drops an `aggregate:` list — the column it asked for is simply not there', async () => { + const rows = await driver.aggregate( + 'deal', + offContract({ + groupBy: ['stage'], + aggregate: [{ function: 'sum', field: 'amount', alias: 'total' }], + }), + ); + + // The grouping still happens — only the undeclared key goes unread — so + // this is the honest description of the loss rather than "it throws". + expect(rows.map((r: any) => r.stage).sort()).toEqual(['lost', 'won']); + for (const row of rows) expect(row.total).toBeUndefined(); + }); + + it('refuses a `func:`-spelled aggregation with INVALID_QUERY / 400', async () => { + // With the alias limb gone `agg.function` is `undefined`, so the name + // reaches `refuseAggregateFunction` and is classified there: the caller + // gets the named 400 this door already gives every undeclared function + // name (#5907), not a silently different answer. + const err = await refusalOf( + offContract({ aggregations: [{ func: 'sum', field: 'amount', alias: 'total' }] }), + ); + expect(err.code).toBe('INVALID_QUERY'); + expect(err.status).toBe(400); + expect(err.message).toContain('is not a declared aggregate function'); + }); + }); + + // ── Control: the declared spelling computes what it always computed ──────── + + it('computes the same aggregate through the declared keys', async () => { + const rows = await driver.aggregate('deal', { + groupBy: ['stage'], + aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }], + }); + + const byStage = Object.fromEntries(rows.map((r: any) => [r.stage, Number(r.total)])); + expect(byStage).toEqual({ won: 30, lost: 30 }); + }); +}); diff --git a/packages/drivers/driver-sql/src/sql-driver-date-bucket.test.ts b/packages/drivers/driver-sql/src/sql-driver-date-bucket.test.ts index 39470cb2b6..fbf079191b 100644 --- a/packages/drivers/driver-sql/src/sql-driver-date-bucket.test.ts +++ b/packages/drivers/driver-sql/src/sql-driver-date-bucket.test.ts @@ -140,13 +140,36 @@ describe('SqlDriver date bucket (dateGranularity)', () => { ); describe('unsupported granularity', () => { - it('throws a loud error for week on SQLite (so engine routes to in-memory)', async () => { - await expect( - driver.aggregate('events', { + /** + * [#6212] The subject is unchanged — week is not bucketed in SQL on SQLite, + * so the engine must be pushed back to in-memory bucketing — but the refusal + * now carries a wire identity, so the assertion moved with it. It used to be + * `rejects.toThrow(/dateGranularity 'week' not supported/)`: a bare `Error` + * with `code`/`status` both `undefined`, which `mapDataError` served as an + * opaque 500 for a named capability gap. `code` and `status` are asserted + * here for the #6144 reason — the un-fixed driver threw for this input too, + * so a `toThrow()` alone was green before and after and could never see the + * defect. The remote face's twin, and the parity between them, live in + * driver-turso's `remote-transport-groupby-node.test.ts`. + */ + it('refuses week on SQLite with NOT_IMPLEMENTED / 501 (so engine routes to in-memory)', async () => { + const err = await driver + .aggregate('events', { groupBy: [{ field: 'ts', dateGranularity: 'week' }], aggregations: [{ function: 'count', alias: 'n' }], - }), - ).rejects.toThrow(/dateGranularity 'week' not supported/); + }) + .then( + () => { throw new Error('expected the driver to refuse week on SQLite'); }, + (e) => e as Error & { code?: string; status?: number }, + ); + + expect(err.code).toBe('NOT_IMPLEMENTED'); + expect(err.status).toBe(501); + expect(err.message.startsWith("Date bucketing by 'week' is not supported by this backend.")).toBe(true); + // The message names what this dialect DOES bucket, so a reader is told + // where the boundary is rather than only that they crossed it. + expect(err.message).toContain('Bucketed here: day, month, quarter, year'); + expect(err.message).toContain('supports.queryDateGranularity'); }); }); diff --git a/packages/drivers/driver-sql/src/sql-driver-queryast.test.ts b/packages/drivers/driver-sql/src/sql-driver-queryast.test.ts index 59df16138c..63f29aec49 100644 --- a/packages/drivers/driver-sql/src/sql-driver-queryast.test.ts +++ b/packages/drivers/driver-sql/src/sql-driver-queryast.test.ts @@ -154,9 +154,24 @@ describe('SqlDriver (QueryAST Format)', () => { expect(results[0].name).toBe('Mouse'); }); - it('should still support legacy aggregate format', async () => { + /** + * [#6321] Was `should still support legacy aggregate format`, spelling + * `aggregate:` / `func:` — two keys the Query Protocol has never declared, + * kept alive by two `||` limbs in `SqlDriver.aggregate` whose only writers + * were this fixture and its three siblings. That is #4984's family: the + * fixture spells the alias, the lenient limb stays green forever, and nothing + * ever measures that deleting it costs nobody anything. + * + * The limbs are gone, so this case is REPLACED, not re-spelled: what it + * pinned no longer exists, and a case whose title advertises a "legacy + * format" the driver no longer has is worse than no case. What survives is + * the same query in the DECLARED spelling — which is what this file, about + * the standard QueryAST keys, is for. The refusal of the old spelling is + * pinned in `sql-driver-aggregate-undeclared-keys.test.ts`. + */ + it('aggregates through the declared `aggregations` / `function` keys', async () => { const results = await driver.aggregate('products', { - aggregate: [{ func: 'avg', field: 'price', alias: 'avg_price' }], + aggregations: [{ function: 'avg', field: 'price', alias: 'avg_price' }], groupBy: ['category'], }); diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index 21e6043636..a02f35a9ab 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -624,6 +624,46 @@ function refuseAggregateFunction(func: string): never { : undeclaredAggregateFunctionError(func); } +/** + * [#6212] A `groupBy` entry asks for a date BUCKET this face cannot emit. + * + * `GroupByNodeSchema` declares `{ field, dateGranularity }` and `DateGranularity` + * declares all five names, so a granularity this dialect has no expression for is + * a CAPABILITY GAP in the backend, not a mistake in the query — the same 501 + * class {@link uncompilableAggregateFunctionError} answers for a + * declared-but-uncompiled aggregate function, and for the same reason (#5907, + * ADR-0112). It used to be a bare `throw new Error(...)`: `code`/`status` both + * `undefined`, so `mapDataError` served an opaque 500 for a named condition. + * + * WHICH granularities a backend buckets natively is PUBLISHED, per driver, as + * `supports.queryDateGranularity`, and the engine reads that record and falls + * back to in-memory bucketing for anything absent from it (objectql `engine.ts`, + * aggregate dispatch). Reaching this throw therefore means a caller went around + * that bit, so the message names the bit rather than the SQL. + * + * Written once per FACE — `RemoteTransport` carries the twin, first sentence for + * first sentence — because `TursoDriver` picks its face from `url` and one + * condition may not have two wire identities (#5240 / #5907). The two faces + * refuse different populations (this one refuses what its DIALECT cannot bucket, + * the remote transport refuses every granularity because it buckets none), but + * both refuse exactly `supports.queryDateGranularity[g] !== true`, which is one + * condition stated per face. + */ +function refuseDateBucketedGroupBy(granularity: string, bucketedHere: string[], face: string): never { + const err = new Error( + `Date bucketing by '${granularity}' is not supported by this backend. ` + + `Bucketed here: ${bucketedHere.length > 0 ? bucketedHere.join(', ') : 'none'} (${face}). ` + + `The query is spelled correctly and @objectstack/spec DateGranularity declares it — this is ` + + `a capability gap in the backend, not a mistake in the query, which is why it answers ` + + `NOT_IMPLEMENTED/501 rather than a 400. A driver publishes the granularities it buckets ` + + `natively as \`supports.queryDateGranularity\`; the engine reads that record and buckets ` + + `in memory for every granularity absent from it, which is always correct (#6212).`, + ) as Error & { code?: string; status?: number }; + err.code = StandardErrorCode.enum.NOT_IMPLEMENTED; + err.status = 501; + throw err; +} + /** * [#5158] A `FilterArray` reached the driver unlowered. * @@ -3588,7 +3628,16 @@ export class SqlDriver implements IDataDriver { // Aggregation // =================================== - async aggregate(object: string, query: any, options?: DriverOptions): Promise { + /** + * [#6212] `query` is a {@link DriverQuery}, not `any`. + * + * `any` here was not "the object name goes unchecked", it was every check off + * on the members this body READS: `where`'s filter dialect, `groupBy`'s node + * union, `aggregations`' node shape. #5181 narrowed the six methods + * `IDataDriver` declares and #6075 followed through on five drivers; + * `aggregate` is not on that contract, so neither reached it. + */ + async aggregate(object: string, query: DriverQuery, options?: DriverOptions): Promise { const builder = this.getBuilder(object, options); this.applyTenantScope(builder, object, options); @@ -3616,7 +3665,12 @@ export class SqlDriver implements IDataDriver { // ({ field: 'closed_at', dateGranularity: 'quarter' }). For structured // items we emit a dialect-specific bucket expression aliased as the // field name so the resulting row keys match in-memory bucketDateValue. - for (const g of query.groupBy as Array) { + // [#6212] The element type is `GroupByNode` — the spec's own union — so + // the local `Array` restatement is + // gone. It had drifted from the declaration it was restating: `alias` was + // missing from it and `dateGranularity` was widened to `string`, which is + // what forced the `as any` on the `buildDateBucketExpr` call below. + for (const g of query.groupBy) { if (typeof g === 'string') { builder.groupBy(g); builder.select(g); @@ -3624,11 +3678,15 @@ export class SqlDriver implements IDataDriver { if (kind) presentedOutput.set(g, kind); } else if (g && typeof g === 'object' && g.field) { if (g.dateGranularity) { - const bucket = this.buildDateBucketExpr(g.field, g.dateGranularity as any, table); + const bucket = this.buildDateBucketExpr(g.field, g.dateGranularity, table); if (!bucket) { - throw new Error( - `SqlDriver: dateGranularity '${g.dateGranularity}' not supported on dialect ` + - `'${(this.config as any).client}'. Engine must fall back to in-memory bucketing.`, + // [#6212] Was a bare `throw new Error(...)`; see + // {@link refuseDateBucketedGroupBy} for why it now carries the + // ADR-0112 envelope and what the remote face answers. + refuseDateBucketedGroupBy( + g.dateGranularity, + Object.entries(this.dateGranularityCapabilities).filter(([, on]) => on).map(([k]) => k), + `dialect '${(this.config as any).client}'`, ); } builder.groupByRaw(bucket.sql, bucket.bindings); @@ -3643,10 +3701,20 @@ export class SqlDriver implements IDataDriver { } } - const aggregates = query.aggregations || query.aggregate; + // [#6321] Was `query.aggregations || query.aggregate` / `agg.function || + // agg.func`. Neither `aggregate` nor `func` is declared anywhere in the + // Query Protocol — `QueryASTSchema` declares `aggregations` and + // `AggregationNodeSchema` declares `function` — so those two limbs were a + // private dialect this consumer tolerated, which is what PD#12 rejects. The + // only writers were this package's own fixtures and driver-sqlite-wasm's + // (#4984's family: a fixture spelling the alias keeps the lenient limb green + // forever, and nothing ever measures that deleting it costs nothing). Both + // fixture sets now spell the declared keys, the non-test writer count was + // zero when measured, and ADR-0049 says an unenforced tolerance goes. + const aggregates = query.aggregations; if (aggregates) { for (const agg of aggregates) { - const funcName = agg.function || agg.func; + const funcName = agg.function; const rawFunc = this.mapAggregateFunc(funcName); // Spec: `field` is optional for COUNT (means COUNT(*)). const fieldExpr = agg.field ?? '*'; diff --git a/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-driver-advanced.test.ts b/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-driver-advanced.test.ts index 402855c24b..1f3d047de9 100644 --- a/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-driver-advanced.test.ts +++ b/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-driver-advanced.test.ts @@ -38,7 +38,7 @@ describe('SqliteWasmDriver Advanced Operations (SQLite)', () => { it('should sum values', async () => { const result = await driver.aggregate('orders', { where: { status: 'completed' }, - aggregate: [{ func: 'sum', field: 'amount', alias: 'total_amount' }], + aggregations: [{ function: 'sum', field: 'amount', alias: 'total_amount' }], }); expect(result).toHaveLength(1); @@ -47,7 +47,7 @@ describe('SqliteWasmDriver Advanced Operations (SQLite)', () => { it('should count records', async () => { const result = await driver.aggregate('orders', { - aggregate: [{ func: 'count', field: '*', alias: 'total_orders' }], + aggregations: [{ function: 'count', field: '*', alias: 'total_orders' }], }); expect(result).toHaveLength(1); @@ -57,7 +57,7 @@ describe('SqliteWasmDriver Advanced Operations (SQLite)', () => { it('should calculate average', async () => { const result = await driver.aggregate('orders', { where: { status: 'completed' }, - aggregate: [{ func: 'avg', field: 'amount', alias: 'avg_amount' }], + aggregations: [{ function: 'avg', field: 'amount', alias: 'avg_amount' }], }); expect(result).toHaveLength(1); @@ -66,9 +66,9 @@ describe('SqliteWasmDriver Advanced Operations (SQLite)', () => { it('should find min and max values', async () => { const result = await driver.aggregate('orders', { - aggregate: [ - { func: 'min', field: 'amount', alias: 'min_amount' }, - { func: 'max', field: 'amount', alias: 'max_amount' }, + aggregations: [ + { function: 'min', field: 'amount', alias: 'min_amount' }, + { function: 'max', field: 'amount', alias: 'max_amount' }, ], }); @@ -80,9 +80,9 @@ describe('SqliteWasmDriver Advanced Operations (SQLite)', () => { it('should group by with aggregates', async () => { const result = await driver.aggregate('orders', { groupBy: ['customer'], - aggregate: [ - { func: 'sum', field: 'amount', alias: 'total_spent' }, - { func: 'count', field: '*', alias: 'order_count' }, + aggregations: [ + { function: 'sum', field: 'amount', alias: 'total_spent' }, + { function: 'count', field: '*', alias: 'order_count' }, ], }); @@ -100,7 +100,7 @@ describe('SqliteWasmDriver Advanced Operations (SQLite)', () => { it('should handle multiple group by fields', async () => { const result = await driver.aggregate('orders', { groupBy: ['customer', 'status'], - aggregate: [{ func: 'sum', field: 'quantity', alias: 'total_qty' }], + aggregations: [{ function: 'sum', field: 'quantity', alias: 'total_qty' }], }); expect(result.length).toBeGreaterThan(0); @@ -114,7 +114,7 @@ describe('SqliteWasmDriver Advanced Operations (SQLite)', () => { const result = await driver.aggregate('orders', { where: { status: { $ne: 'cancelled' } }, groupBy: ['product'], - aggregate: [{ func: 'sum', field: 'quantity', alias: 'total_quantity' }], + aggregations: [{ function: 'sum', field: 'quantity', alias: 'total_quantity' }], }); const laptop = result.find((r: any) => r.product === 'Laptop'); diff --git a/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-driver-date-bucket.test.ts b/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-driver-date-bucket.test.ts index b238846f17..48f73e2f65 100644 --- a/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-driver-date-bucket.test.ts +++ b/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-driver-date-bucket.test.ts @@ -172,13 +172,27 @@ describe('SqliteWasmDriver date bucket (dateGranularity)', () => { }); describe('unsupported granularity', () => { - it('throws a loud error for week on SQLite (so engine routes to in-memory)', async () => { - await expect( - driver.aggregate('events', { + /** + * [#6212] The twin of `driver-sql`'s case, moved for the same reason and + * kept here rather than dropped: this driver inherits `SqlDriver.aggregate` + * but reports a DIFFERENT dialect name in the refusal's tail, which is the + * one part of the message a shared implementation cannot prove. + */ + it('refuses week on SQLite with NOT_IMPLEMENTED / 501 (so engine routes to in-memory)', async () => { + const err = await driver + .aggregate('events', { groupBy: [{ field: 'ts', dateGranularity: 'week' }], aggregations: [{ function: 'count', alias: 'n' }], - }), - ).rejects.toThrow(/dateGranularity 'week' not supported/); + }) + .then( + () => { throw new Error('expected the driver to refuse week on SQLite'); }, + (e) => e as Error & { code?: string; status?: number }, + ); + + expect(err.code).toBe('NOT_IMPLEMENTED'); + expect(err.status).toBe(501); + expect(err.message.startsWith("Date bucketing by 'week' is not supported by this backend.")).toBe(true); + expect(err.message).toContain('Bucketed here: day, month, quarter, year'); }); }); diff --git a/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-driver-queryast.test.ts b/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-driver-queryast.test.ts index 7941914ac7..7f23032329 100644 --- a/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-driver-queryast.test.ts +++ b/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-driver-queryast.test.ts @@ -144,9 +144,18 @@ describe('SqliteWasmDriver (QueryAST Format)', () => { expect(results[0].name).toBe('Mouse'); }); - it('should still support legacy aggregate format', async () => { + /** + * [#6321] The twin of `sql-driver-queryast.test.ts`'s case, replaced for the + * same reason: it was titled `should still support legacy aggregate format` + * and spelled `aggregate:` / `func:`, two keys the Query Protocol never + * declared. This driver inherits `SqlDriver.aggregate` verbatim, so the two + * `||` limbs that used to read them were deleted once and stop existing here + * too — a case pinning a "legacy format" that no longer exists cannot be + * re-spelled into truth, only replaced with the declared spelling. + */ + it('aggregates through the declared `aggregations` / `function` keys', async () => { const results = await driver.aggregate('products', { - aggregate: [{ func: 'avg', field: 'price', alias: 'avg_price' }], + aggregations: [{ function: 'avg', field: 'price', alias: 'avg_price' }], groupBy: ['category'], }); diff --git a/packages/drivers/driver-turso/src/remote-transport-groupby-node.test.ts b/packages/drivers/driver-turso/src/remote-transport-groupby-node.test.ts new file mode 100644 index 0000000000..59cc758cd9 --- /dev/null +++ b/packages/drivers/driver-turso/src/remote-transport-groupby-node.test.ts @@ -0,0 +1,347 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#6212] `groupBy` is a UNION, and this transport used to read it as `string[]`. + * + * ```ts + * const groupBy: string[] = Array.isArray(query?.groupBy) ? query.groupBy : []; + * ``` + * + * `GroupByNodeSchema` is `z.union([z.string(), z.object({ field, + * dateGranularity?, alias? })])`. Under `query: any` that mismatch was invisible; + * narrowing the signature to `DriverQuery` made `tsc` state it outright: + * + * ``` + * remote-transport.ts(826,11): error TS2322: Type '(string | { field: string; + * dateGranularity?: … ; alias?: string })[]' is not assignable to type 'string[]' + * ``` + * + * The two halves of the union were in very different shape, which is why this is + * a behaviour change and not a cast: + * + * # A PLAIN structured item was a live remote/local fork + * + * `{ field: 'stage' }` with no granularity is spec-valid and reaches drivers + * today: objectql's aggregate dispatch treats it as supported by everyone + * ("plain {field} object is fine" — `engine.ts`) and pushes it down, and + * `secret-fields.test.ts` puts exactly that shape through `engine.aggregate`. + * The LOCAL face of this driver compiles it as a plain `GROUP BY "stage"` + * (`SqlDriver.aggregate` reads `g.field`). This face interpolated the object, + * got `"[object Object]"`, and died in `assertSafeIdentifier` — a SQL-injection + * message for a query the sibling face answers. One query, two answers, decided + * by a connection string: the #6203 shape. + * + * # A DATE-BUCKETED item genuinely cannot be compiled here + * + * That half is guarded, and honestly so: remote mode publishes + * `queryDateGranularity: {}` (see `TursoDriver.supports`), the engine reads that + * record and buckets in memory for every granularity absent from it, so nothing + * bucketed is pushed down. What was missing was an answer for the caller that + * goes around the bit — it also got `"[object Object]"`. It now gets the + * ADR-0112 envelope the aggregate door already uses for a declared-but- + * uncompilable name (#5907 / PR #6204): NOT_IMPLEMENTED / 501, because + * `DateGranularity` declares the name and this backend simply cannot emit it. + * + * The local face carried the same condition with a BARE `Error` (`code`/`status` + * both `undefined` → an opaque 500 from `mapDataError`). Giving only this face an + * envelope would have been a new fork, so both faces were moved together and the + * parity block below compares their runtime messages. + * + * # `alias` is deliberately NOT read + * + * `GroupByNodeSchema.alias` is honoured by the in-memory path + * (`in-memory-aggregation.ts` projects `g.alias ?? g.field`) and ignored by + * `SqlDriver.aggregate`. Reading it here would make this transport the only SQL + * face that honours it — a new divergence dressed as a fix. It is ignored, in + * step with the local face, and the pushdown/in-memory disagreement is filed + * separately; it is not created here. + * + * # Reverse verification — direction predicted BEFORE it was run, per case + * + * Restore `const groupBy: string[] = Array.isArray(query?.groupBy) ? … : []` + * (with a cast, since the narrowed signature no longer permits it): + * + * - the two PLAIN-structured cases go RED, and NOT on a comparison: the + * transport throws `unsafe identifier rejected: "[object Object]"`, so they + * fail inside the call. + * - the date-bucket refusal case goes RED on `err.code` → `undefined`, having + * thrown the identifier error instead of the refusal — it fails on the first + * assertion, not through the "it compiled" branch, because the old code threw + * for this input too, just anonymously. + * - the PARITY case goes RED for the same reason — `remote.code` is `undefined` + * where `local.code` is `NOT_IMPLEMENTED`. + * - the `'month'` asymmetry case goes RED on its remote half only: the local + * half still resolves (it is not touched by this revert), the remote half + * throws the identifier error instead of the refusal sentence. + * - the unsafe-identifier case stays GREEN on its first assertion and goes RED + * on the second — see the note on the case itself: the old code threw the + * same SENTENCE for the wrong REASON, which is why the offending text is + * asserted rather than the sentence alone. + * - the string-form control, and the case that pins what the LOCAL face lists, + * stay GREEN: the string half of the union never moved and the local face is + * not part of this revert. + * + * Measured after writing the above — **11 failed / 2 passed of 13**, case for + * case as predicted: + * + * ``` + * structured, no granularity (2) Error: RemoteTransport: unsafe identifier + * rejected: "[object Object]" (thrown IN the call) + * unsafe identifier expected 'RemoteTransport: unsafe identifier re…' + * to contain 'stage"; DROP TABLE deal; --' + * (first assertion green, second red — the point) + * date-bucket refusals (5) expected undefined to be 'NOT_IMPLEMENTED' + * "Bucketed here: none" expected '…unsafe identifier re…' to contain it + * parity 'week' expected undefined to be 'NOT_IMPLEMENTED' + * 'month' asymmetry remote half only: expected […] to throw + * "Date bucketing by 'month' …" but got + * "RemoteTransport: unsafe identifier re…" + * ── green ── + * string entry compiles to GROUP BY + * the local face lists day, month, quarter, year + * ``` + * + * Not one failure came through an "it compiled" branch: the un-narrowed + * transport threw for every one of these inputs too, just with the wrong + * message and no wire identity. That is the #6144 shape restated as evidence, + * and it is why every case here asserts `code`/`status` or the offending TEXT + * rather than merely that something was thrown. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { RemoteTransport } from './remote-transport.js'; + +interface WireBearingError extends Error { + code?: string; + status?: number; +} + +/** The first sentence, spelled out rather than imported — #5240's contract. */ +const REFUSAL_SENTENCE = (g: string) => + `Date bucketing by '${g}' is not supported by this backend.`; + +function transportWithCapturingClient() { + const calls: Array<{ sql: string; args: any[] }> = []; + const client = { + execute: vi.fn(async (stmt: any) => { + calls.push({ sql: stmt.sql ?? String(stmt), args: stmt.args ?? [] }); + return { rows: [], columns: [] }; + }), + close: vi.fn(), + }; + const t = new RemoteTransport(); + t.setClient(client as any); + return { t, calls }; +} + +async function localDriver(): Promise { + const d = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + await d.initObjects([ + { + name: 'deal', + fields: { + id: { type: 'text', name: 'id' }, + stage: { type: 'text', name: 'stage' }, + closed_at: { type: 'datetime', name: 'closed_at' }, + }, + } as any, + ]); + return d; +} + +describe('[#6212] RemoteTransport compiles the GroupByNode union', () => { + describe('a plain field name — the half that never moved', () => { + it('compiles a string entry to GROUP BY, as it always did', async () => { + const { t, calls } = transportWithCapturingClient(); + await t.aggregate('deal', { + groupBy: ['stage'], + aggregations: [{ function: 'count', field: 'stage', alias: 'n' }], + }); + expect(calls[0].sql).toBe('SELECT "stage", count("stage") AS "n" FROM "deal" GROUP BY "stage"'); + }); + }); + + describe('a structured entry with no granularity', () => { + it('compiles to the SAME statement as the string spelling', async () => { + const { t, calls } = transportWithCapturingClient(); + await t.aggregate('deal', { + groupBy: [{ field: 'stage' }], + aggregations: [{ function: 'count', field: 'stage', alias: 'n' }], + }); + expect(calls[0].sql).toBe('SELECT "stage", count("stage") AS "n" FROM "deal" GROUP BY "stage"'); + }); + + it('ignores `alias`, in step with the local face', async () => { + // Pinned as a DELIBERATE choice, not an oversight: `SqlDriver.aggregate` + // does not read `alias` either, so honouring it only here would make this + // transport the one SQL face that does. See the file header. + const { t, calls } = transportWithCapturingClient(); + await t.aggregate('deal', { + groupBy: [{ field: 'stage', alias: 'bucket' }], + aggregations: [{ function: 'count', field: 'stage', alias: 'n' }], + }); + expect(calls[0].sql).toBe('SELECT "stage", count("stage") AS "n" FROM "deal" GROUP BY "stage"'); + expect(calls[0].sql).not.toContain('bucket'); + }); + + it('still refuses an unsafe identifier inside a structured entry', async () => { + // Unwrapping the union must not unwrap the injection guard with it. + // + // The assertion names the OFFENDING TEXT rather than only the sentence, + // and that is load-bearing: the un-narrowed transport also threw + // `unsafe identifier rejected` here — for `"[object Object]"`, because it + // garbled the whole entry — so a case matching only the sentence would be + // green before and after, for opposite reasons (the #6144 shape). + const { t, calls } = transportWithCapturingClient(); + const err = await t + .aggregate('deal', { + groupBy: [{ field: 'stage"; DROP TABLE deal; --' }], + aggregations: [{ function: 'count', alias: 'n' }], + }) + .then( + () => { throw new Error('expected the transport to refuse an unsafe identifier'); }, + (e) => e as Error, + ); + expect(err.message).toContain('unsafe identifier rejected'); + expect(err.message).toContain('stage"; DROP TABLE deal; --'); + expect(err.message).not.toContain('[object Object]'); + expect(calls).toEqual([]); + }); + }); + + describe('a date-bucketed entry — the half this backend cannot compile', () => { + const refusalOf = async (granularity: 'day' | 'week' | 'month' | 'quarter' | 'year') => { + const { t, calls } = transportWithCapturingClient(); + try { + await t.aggregate('deal', { + groupBy: [{ field: 'closed_at', dateGranularity: granularity }], + aggregations: [{ function: 'count', alias: 'n' }], + }); + } catch (e) { + expect(calls).toEqual([]); + return e as WireBearingError; + } + throw new Error(`expected a refusal for '${granularity}', but it compiled to ${JSON.stringify(calls)}`); + }; + + for (const granularity of ['day', 'week', 'month', 'quarter', 'year'] as const) { + it(`refuses '${granularity}' with NOT_IMPLEMENTED / 501`, async () => { + // ⚠️ Asserts `code` AND `status`, never merely "it threw" (#6144): the + // un-narrowed transport threw for this input too — with an unsafe- + // identifier message — so `rejects.toThrow()` would have been green + // before and after, blind to the whole change. + const err = await refusalOf(granularity); + expect(err.code).toBe('NOT_IMPLEMENTED'); + expect(err.status).toBe(501); + expect(err.message.startsWith(REFUSAL_SENTENCE(granularity))).toBe(true); + expect(err.message).toContain('capability gap'); + expect(err.message).toContain('supports.queryDateGranularity'); + expect(err.message).not.toContain('[object Object]'); + expect(err.message).not.toContain('unsafe identifier'); + // #1116's note: driver-internal wording is not the caller's business. + expect(err.message).not.toContain('[RemoteTransport]'); + }); + } + + it('names every granularity it declines, so the message cannot claim a false capability', async () => { + const err = await refusalOf('month'); + expect(err.message).toContain('Bucketed here: none'); + }); + }); + + // ── The cross-package half: one condition, one wording ───────────────────── + + /** + * ⚠️ What this block pins is not "the wording is right today" — it is that a + * FUTURE PR cannot move one face and leave the other. + * + * Both sides are RUNTIME messages, produced by two different packages and + * compared to each other. Neither side reads a shared constant: a pin written + * that way agrees with itself however far the two implementations drift, which + * is the one failure mode a parity test exists to rule out. `REFUSAL_SENTENCE` + * above is a third, independent copy of the bytes — so the block fails if + * either face moves, and also if both move together. + * + * The condition is literally one condition stated twice + * (`supports.queryDateGranularity[g] !== true`), which is why the envelope had + * to land on both faces in one PR rather than on the remote one alone: giving + * only the new refusal a `code`/`status` would have shipped the exact + * "one condition, two wire identities" shape #5907 spent an issue closing. + */ + describe('local/remote parity (#5240 — one condition, one wording)', () => { + it("answers 'week' identically on both faces", async () => { + // `week` is the granularity BOTH faces decline: SQLite buckets + // day/month/quarter/year and leaves week to the in-memory path (strftime + // %V landed only in SQLite 3.46), and this transport buckets nothing. So + // it is the one input on which the two messages are comparable at all. + const query = { + groupBy: [{ field: 'closed_at', dateGranularity: 'week' as const }], + aggregations: [{ function: 'count' as const, alias: 'n' }], + }; + + const { t } = transportWithCapturingClient(); + const remote = await t.aggregate('deal', query).then( + () => { throw new Error('expected the transport to refuse'); }, + (e) => e as WireBearingError, + ); + + const d = await localDriver(); + const local = await d.aggregate('deal', query).then( + () => { throw new Error('expected the local driver to refuse'); }, + (e) => e as WireBearingError, + ); + + expect(remote.code).toBe(local.code); + expect(remote.status).toBe(local.status); + // First sentence is the contract; the tails differ on purpose — each face + // reports what IT buckets, which is the whole content of the asymmetry + // below. + expect(remote.message.split('. ')[0]).toBe(local.message.split('. ')[0]); + expect(remote.message.split('. ')[0]).toBe(REFUSAL_SENTENCE('week').replace(/\.$/, '')); + }); + + it("'month' is declined here and compiled there — a DECLARED asymmetry, not a fork", async () => { + // The two faces publish different `supports.queryDateGranularity`, and the + // engine reads it. `month` on the local face must therefore compile, not + // refuse: a parity test that demanded identical behaviour here would be + // demanding the capability bit mean nothing. + const d = await localDriver(); + await expect( + d.aggregate('deal', { + groupBy: [{ field: 'closed_at', dateGranularity: 'month' }], + aggregations: [{ function: 'count', alias: 'n' }], + }), + ).resolves.toBeDefined(); + + const { t } = transportWithCapturingClient(); + await expect( + t.aggregate('deal', { + groupBy: [{ field: 'closed_at', dateGranularity: 'month' }], + aggregations: [{ function: 'count', alias: 'n' }], + }), + ).rejects.toThrow(REFUSAL_SENTENCE('month')); + }); + + it('the local face lists the granularities it DOES bucket', async () => { + const d = await localDriver(); + const err = await d + .aggregate('deal', { + groupBy: [{ field: 'closed_at', dateGranularity: 'week' }], + aggregations: [{ function: 'count', alias: 'n' }], + }) + .then( + () => { throw new Error('expected the local driver to refuse'); }, + (e) => e as WireBearingError, + ); + expect(err.code).toBe('NOT_IMPLEMENTED'); + expect(err.status).toBe(501); + expect(err.message).toContain('Bucketed here: day, month, quarter, year'); + expect(err.message).toContain("dialect 'better-sqlite3'"); + }); + }); +}); diff --git a/packages/drivers/driver-turso/src/remote-transport-undeclared-aggregate-keys.test.ts b/packages/drivers/driver-turso/src/remote-transport-undeclared-aggregate-keys.test.ts new file mode 100644 index 0000000000..178e584111 --- /dev/null +++ b/packages/drivers/driver-turso/src/remote-transport-undeclared-aggregate-keys.test.ts @@ -0,0 +1,216 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#6321] The remote half of the same deletion: `RemoteTransport.aggregate` no + * longer reads `query.aggregate` or `agg.func`. + * + * ```ts + * const aggregations = query?.aggregations || query?.aggregate || []; + * const func = String(agg.function || agg.func || ''); + * ``` + * + * Neither key is declared anywhere in `packages/spec`. The twin note in + * `driver-sql`'s `sql-driver-aggregate-undeclared-keys.test.ts` carries the + * measurement and the ADR-0049 reasoning; this file exists because `TursoDriver` + * picks its face from `url`, so a deletion done on one face and not the other is + * the #5907 / #6203 fork all over again — one query, two answers, decided by a + * connection string. + * + * # The `|| ''` went with it, and that is a wording change worth naming + * + * The third limb only ever fired when NEITHER key was written. It coalesced the + * missing name to `''`, so this face quoted `""` back at the caller while the + * local driver — reading `agg.function || agg.func`, then interpolating — quoted + * `"undefined"`. Same off-contract input, two messages. It was unreachable in + * practice while `agg.func` was read (a `func:`-spelling caller landed on the + * alias limb, not here); deleting the alias is exactly what makes it reachable, + * so it is closed in the same breath rather than left as a new fork (#5240). + * + * # Reverse verification — direction predicted BEFORE it was run, per channel + * + * Restore both limbs on this face only: + * + * - `pnpm test`: the two runtime cases go RED — the `aggregate:` case because + * the transport emits a `count(...)` column it should not, the `func:` case + * through `refusalOf`'s "it compiled" branch. The PARITY case goes red as + * well, failing on its own `expected the transport to refuse`. + * - `pnpm typecheck`: unchanged, green. Excess-property checking does not care + * what the body reads. + * + * Restore both limbs on BOTH faces and the parity case stays RED — which is + * worth stating explicitly, because the sibling parity block in + * `remote-transport-aggregate-function-refusal.test.ts` behaves the OPPOSITE + * way and its PR recorded the surprise. That one compares two error identities + * and nothing else, so reverting both faces leaves them agreeing on + * `undefined`/`undefined` and it goes green: a parity test measures AGREEMENT, + * not correctness. This one asserts each face REFUSES before it compares + * anything, so the correctness half fires first on whichever face drifted — and + * the failure names it (`expected the transport to refuse` vs `expected the + * local driver to refuse`). It therefore cannot be satisfied by breaking both + * faces equally, and the `toContain('"undefined"')` at the end guards the + * remaining hole: two faces agreeing on the WRONG spelling. + * + * Measured after writing the above, with both limbs restored on both faces — + * 3 failed / 4 passed of 7, and the parity case among the three, as predicted: + * + * ``` + * drops an `aggregate:` list -> expected 'SELECT "stage" FROM "deal" GROUP BY "stage"' + * received 'SELECT "stage", count("stage") AS "n" …' + * refuses a `func:`-spelled -> expected the transport to refuse, but it compiled to […] + * local/remote parity -> expected the transport to refuse + * ``` + * + * `pnpm typecheck` was clean under that revert, and went red only when the + * signature itself was put back to `any`: + * `remote-transport-undeclared-aggregate-keys.test.ts(106,7)` and `(112,7)`, + * `error TS2578: Unused '@ts-expect-error' directive.` + */ + +import { describe, it, expect, vi } from 'vitest'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { RemoteTransport } from './remote-transport.js'; +import type { DriverQuery } from '@objectstack/spec/contracts'; + +interface WireBearingError extends Error { + code?: string; + status?: number; +} + +/** The narrowed door, read off the signature rather than restated. */ +type AggregateQuery = Parameters[1]; + +/** See the twin's note: `as unknown as DriverQuery`, never `as any`. */ +const offContract = (q: Record) => q as unknown as DriverQuery; + +function transportWithCapturingClient() { + const calls: Array<{ sql: string; args: any[] }> = []; + const client = { + execute: vi.fn(async (stmt: any) => { + calls.push({ sql: stmt.sql ?? String(stmt), args: stmt.args ?? [] }); + return { rows: [], columns: [] }; + }), + close: vi.fn(), + }; + const t = new RemoteTransport(); + t.setClient(client as any); + return { t, calls }; +} + +/** The other face of `TursoDriver`: local/replica, which inherits `SqlDriver`. */ +async function localDriver(): Promise { + const d = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + await d.initObjects([ + { + name: 'deal', + fields: { + id: { type: 'text', name: 'id' }, + stage: { type: 'text', name: 'stage' }, + }, + } as any, + ]); + return d; +} + +describe('[#6321] RemoteTransport.aggregate reads only the declared aggregation keys', () => { + describe('the undeclared spellings do not compile', () => { + it('rejects `aggregate:` where the protocol declares `aggregations:`', () => { + // @ts-expect-error [#6321] `aggregate` is not a key of QueryAST. + const undeclaredList: AggregateQuery = { aggregate: [{ function: 'count', field: 'stage', alias: 'n' }] }; + expect(Object.keys(undeclaredList)).toEqual(['aggregate']); + }); + + it('rejects `func:` where the protocol declares `function:`', () => { + // @ts-expect-error [#6321] `func` is not a key of AggregationNode. + const undeclaredFunc: AggregateQuery = { aggregations: [{ func: 'count', field: 'stage', alias: 'n' }] }; + expect(Object.keys(undeclaredFunc)).toEqual(['aggregations']); + }); + + it('admits the declared spelling — the pin is not green because nothing fits', () => { + const declared: AggregateQuery = { + groupBy: ['stage'], + aggregations: [{ function: 'count', field: 'stage', alias: 'n' }], + }; + expect(declared.aggregations).toHaveLength(1); + }); + }); + + describe('an off-contract caller that writes them anyway', () => { + const refusalOf = async (query: DriverQuery): Promise => { + const { t, calls } = transportWithCapturingClient(); + try { + await t.aggregate('deal', query); + } catch (e) { + // A refused aggregation must not have reached the database on its way + // to throwing — the same guard the function-refusal twin makes. + expect(calls).toEqual([]); + return e as WireBearingError; + } + throw new Error(`expected the transport to refuse, but it compiled to ${JSON.stringify(calls)}`); + }; + + it('drops an `aggregate:` list — no aggregate column reaches the SQL', async () => { + const { t, calls } = transportWithCapturingClient(); + await t.aggregate( + 'deal', + offContract({ groupBy: ['stage'], aggregate: [{ function: 'count', field: 'stage', alias: 'n' }] }), + ); + + // The grouping still compiles; the undeclared key contributes nothing. + expect(calls[0].sql).toBe('SELECT "stage" FROM "deal" GROUP BY "stage"'); + expect(calls[0].sql).not.toContain('count('); + }); + + it('refuses a `func:`-spelled aggregation with INVALID_QUERY / 400', async () => { + const err = await refusalOf( + offContract({ aggregations: [{ func: 'count', field: 'stage', alias: 'n' }] }), + ); + expect(err.code).toBe('INVALID_QUERY'); + expect(err.status).toBe(400); + expect(err.message).toContain('is not a declared aggregate function'); + }); + }); + + // ── The cross-package half: one condition, one wording ───────────────────── + + describe('local/remote parity (#5240)', () => { + it('answers a `func:`-spelled aggregation identically on both faces', async () => { + // Compared as RUNTIME messages from the two packages, never as two copies + // of a literal: a shared constant agrees with itself however far the two + // faces drift. This is also the case that pins the deleted `|| ''` — + // before it went, this face said `""` and the local one said `"undefined"`. + const query = offContract({ aggregations: [{ func: 'count', field: 'stage', alias: 'n' }] }); + + const { t } = transportWithCapturingClient(); + const remote = await t.aggregate('deal', query).then( + () => { throw new Error('expected the transport to refuse'); }, + (e) => e as WireBearingError, + ); + + const d = await localDriver(); + const local = await d.aggregate('deal', query).then( + () => { throw new Error('expected the local driver to refuse'); }, + (e) => e as WireBearingError, + ); + + expect(remote.code).toBe(local.code); + expect(remote.status).toBe(local.status); + expect(remote.message).toBe(local.message); + expect(remote.message).toContain('"undefined"'); + }); + }); + + // ── Control: the compiled vocabulary is untouched ────────────────────────── + + it('emits the same SQL as ever for the declared spelling', async () => { + const { t, calls } = transportWithCapturingClient(); + await t.aggregate('deal', { + groupBy: ['stage'], + aggregations: [{ function: 'count', field: 'stage', alias: 'n' }], + }); + expect(calls[0].sql).toBe('SELECT "stage", count("stage") AS "n" FROM "deal" GROUP BY "stage"'); + }); +}); diff --git a/packages/drivers/driver-turso/src/remote-transport.ts b/packages/drivers/driver-turso/src/remote-transport.ts index 374144df0e..25eb2f33c1 100644 --- a/packages/drivers/driver-turso/src/remote-transport.ts +++ b/packages/drivers/driver-turso/src/remote-transport.ts @@ -19,6 +19,7 @@ import { FILTER_OPERATORS, LOGICAL_OPERATORS } from '@objectstack/spec/data'; // transport's "the protocol has no such function" refusal cannot drift from what // `AggregationNodeSchema.function` admits, nor from the local driver's twin. import { AggregationFunction } from '@objectstack/spec/data'; +import type { DriverQuery } from '@objectstack/spec/contracts'; import { nanoid } from 'nanoid'; /** @@ -620,6 +621,40 @@ function refuseAggregateFunction(func: string): never { : undeclaredAggregateFunctionError(func); } +/** + * [#6212] A `groupBy` entry asks for a date BUCKET — the twin of `driver-sql`'s + * `refuseDateBucketedGroupBy`, first sentence for first sentence, and the same + * NOT_IMPLEMENTED/501 class for the same reason: `DateGranularity` declares the + * name, this backend cannot emit it, so it is a capability gap rather than a + * mistake in the query (#5907, ADR-0112). + * + * This transport buckets NOTHING natively, which is exactly what `TursoDriver` + * publishes for it — `supports.queryDateGranularity` is `{}` in remote mode (see + * the comment there), so the engine buckets every granularity in memory and + * never pushes a bucketed item down here. The refusal therefore only fires for a + * caller that went around the capability bit and reached this transport + * directly, which is the caller this message is written for. + * + * Before #6212 there was no refusal at all: `groupBy` was read as `string[]`, + * a structured item was interpolated as `"[object Object]"` and died in + * {@link RemoteTransport.assertSafeIdentifier} — a SQL-injection message for a + * capability question. + */ +function refuseDateBucketedGroupBy(granularity: string): never { + const err = new Error( + `Date bucketing by '${granularity}' is not supported by this backend. ` + + `Bucketed here: none (Turso remote transport). ` + + `The query is spelled correctly and @objectstack/spec DateGranularity declares it — this is ` + + `a capability gap in the backend, not a mistake in the query, which is why it answers ` + + `NOT_IMPLEMENTED/501 rather than a 400. A driver publishes the granularities it buckets ` + + `natively as \`supports.queryDateGranularity\`; the engine reads that record and buckets ` + + `in memory for every granularity absent from it, which is always correct (#6212).`, + ) as Error & { code?: string; status?: number }; + err.code = StandardErrorCode.enum.NOT_IMPLEMENTED; + err.status = 501; + throw err; +} + /** * How a filtered column must be READ so it is in the same storage form the * comparand was coerced into — the column half of the driver's temporal seam @@ -824,19 +859,55 @@ export class RemoteTransport { // yielding, so it never streamed anything either; its only caller was // `TursoDriver.findStream`, which went at the same time. - async aggregate(object: string, query: any): Promise[]> { + /** + * [#6212] `query` is a {@link DriverQuery}, not `any` — the same narrowing + * `SqlDriver.aggregate` and `TursoDriver.aggregate` took, because all three are + * one door and a caller may not be told three different things about it. + */ + async aggregate(object: string, query: DriverQuery): Promise[]> { await this.ensureConnected(); this.assertSafeIdentifier(object); const selectParts: string[] = []; - const groupBy: string[] = Array.isArray(query?.groupBy) ? query.groupBy : []; + + // [#6212] `groupBy` is `GroupByNode[]` — a UNION of a bare field name and a + // structured `{ field, dateGranularity?, alias? }` entry — so reading it as + // `string[]` was a type assumption held up by a CAPABILITY BIT rather than by + // the type, and only for half of the union: + // + // - a DATE-BUCKETED item genuinely cannot reach here through the engine, + // because remote mode publishes `queryDateGranularity: {}` and the engine + // falls back to in-memory bucketing (`TursoDriver.supports`). A caller that + // goes around that bit now gets {@link refuseDateBucketedGroupBy} instead + // of `"[object Object]"` rejected as an unsafe identifier. + // - a PLAIN structured item (`{ field: 'region' }`, no granularity) is NOT + // guarded by that bit at all: objectql's aggregate dispatch treats it as + // supported by every driver ("plain {field} object is fine") and pushes it + // down. It arrived here, stringified, and died — while the LOCAL face of + // this same driver compiles it as a plain `GROUP BY "region"`. That is the + // #6203 shape again: one query, two answers, decided by a connection + // string. Reading `.field` converges them. + // + // `alias` is deliberately not read: `SqlDriver.aggregate` does not read it + // either, so honouring it here would be the divergence rather than the fix. + // That the SQL faces ignore a key the in-memory path honours + // (`in-memory-aggregation.ts` projects `g.alias ?? g.field`) is filed + // separately — it is not created here. + const groupBy: string[] = (Array.isArray(query?.groupBy) ? query.groupBy : []).map((g) => { + if (typeof g === 'string') return g; + if (g?.dateGranularity) refuseDateBucketedGroupBy(g.dateGranularity); + return g?.field; + }); for (const field of groupBy) { this.assertSafeIdentifier(field); selectParts.push(`"${field}"`); } - const aggregations = query?.aggregations || query?.aggregate || []; + // [#6321] Was `query?.aggregations || query?.aggregate` — see the twin note + // on `SqlDriver.aggregate`. `aggregate` is not a key the Query Protocol ever + // declared; its only writers were the two driver packages' own fixtures. + const aggregations = query?.aggregations || []; for (const agg of aggregations) { // [#5907] The caller's spelling is what the refusal quotes back and what // the declared-vocabulary check is judged against. @@ -849,7 +920,15 @@ export class RemoteTransport { // spelling the Query Protocol never declared; deleting it converges both // faces on the declared spelling rather than fossilising the dialect into // a second contract (PD#12). See {@link refuseAggregateFunction}. - const func = String(agg.function || agg.func || ''); + // + // [#6321] The `|| agg.func` limb is gone — an undeclared spelling this + // consumer tolerated — and so is the `|| ''` behind it, which only ever + // fired when NEITHER key was written. Coalescing to `''` there made this + // face quote `""` back at a caller while the local face quoted + // `"undefined"` for the same off-contract input: one condition, two + // wordings (#5240). `String()` stays so the quoted spelling is a string + // whatever a JS caller put there. + const func = String(agg.function); const sqlFunc = REMOTE_AGGREGATE_FUNCTIONS.get(func); if (sqlFunc === undefined) refuseAggregateFunction(func); const field = agg.field || '*'; diff --git a/packages/drivers/driver-turso/src/turso-driver.ts b/packages/drivers/driver-turso/src/turso-driver.ts index 67a6766d68..aa889a05e1 100644 --- a/packages/drivers/driver-turso/src/turso-driver.ts +++ b/packages/drivers/driver-turso/src/turso-driver.ts @@ -545,7 +545,17 @@ export class TursoDriver extends SqlDriver { return super.count(object, query, options); } - override async aggregate(object: string, query: any, options?: any): Promise { + /** + * [#6212] `query` is a {@link DriverQuery}, matching the narrowed + * `SqlDriver.aggregate` this forwards to — the two faces of one driver may not + * declare one argument two ways. + * + * `options` is deliberately left `any`: it is a SECOND axis, shared verbatim + * with the four overrides above it, and narrowing one of five mid-file would + * read as a decision about the others. #6210 left the same `options?: any` on + * `count` for the same reason. + */ + override async aggregate(object: string, query: DriverQuery, options?: any): Promise { if (this.isRemote) return this.remoteTransport!.aggregate(object, this.toRemoteQuery(object, query)); return super.aggregate(object, query, options); } From 673951891537df90841b24d790a15c2255f7c5a1 Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Fri, 7 Aug 2026 17:15:20 +0000 Subject: [PATCH 2/2] spec(migrations): register the aggregate alias retirement in the ADR-0087 ledger (#6321) The #6148 gate asks a declared-breaking changeset what the ledger disposition is. Measured, rather than assumed: a candidate entry renders into the upgrade guide's "Semantic (delegated to you, with acceptance criteria)" section as a true, executable human instruction, and NOT into the "Mechanical (applied for you)" table -- so it carries no `migrate meta` implication and names no stored field that does not exist. That is the same disposition three existing entries already carry for code surfaces: data-driver-find-stream-retired (#4484), storage-service-list-retired (#5540), actor-user-roles-to-positions (#6011). An untyped JS caller has no enforced channel at all once the alias is gone -- tsc reaches typed call sites only -- which is precisely the case the ledger exists to reach. --- ...egate-driver-query-and-alias-retirement.md | 2 + docs/protocol-upgrade-guide.md | 3 ++ packages/spec/spec-changes.json | 14 ++++++ packages/spec/src/migrations/registry.ts | 45 +++++++++++++++++++ 4 files changed, 64 insertions(+) diff --git a/.changeset/aggregate-driver-query-and-alias-retirement.md b/.changeset/aggregate-driver-query-and-alias-retirement.md index 1984529249..af10889540 100644 --- a/.changeset/aggregate-driver-query-and-alias-retirement.md +++ b/.changeset/aggregate-driver-query-and-alias-retirement.md @@ -49,3 +49,5 @@ const func = String(agg.function || agg.func || ''); 标 major 与 #5181 / #6075 / #6210 一致：**源码级破坏性**（调用点内联字面量、以及被删的两个别名键），加上第二、三节两处真实的运行期改动。`check:api-surface` 只记录导出的存在与否、不记录签名，所以这条说明是该变更唯一的下游载体。 `driver-sqlite-wasm` 未列入：它整个继承 `SqlDriver.aggregate`，自身源码零改动（改的只有它的 fixture 与一条断言）——与批 A+E 的处理一致。它读的是 driver-sql 的 `dist/*.d.ts`，因此验证时**必须先重建 driver-sql** 再 typecheck/test，否则是假绿。 + + diff --git a/docs/protocol-upgrade-guide.md b/docs/protocol-upgrade-guide.md index 0644b8d5fe..e4d8fb4be0 100644 --- a/docs/protocol-upgrade-guide.md +++ b/docs/protocol-upgrade-guide.md @@ -358,6 +358,9 @@ Last, it reconciles the SDUI component-props surface with the renderers that ser - **`storage-service-list-retired`** — `contracts.IStorageService.list` → no replacement — track the keys you wrote (sys_file / file-reference records, queryable through ObjectQL with real pagination) instead of enumerating the bucket - Why not automatic: `list(prefix)` was an OPTIONAL contract method documented as "List files in a directory/prefix", and the two shipped adapters answered the same call with two different semantics — both of them silently incomplete. `LocalStorageAdapter.list` was a single-level `readdir`, so a nested key `a/b/c` was invisible under `list('a')` (only `a/b` came back), and a subdirectory that `stat` succeeded on was pushed into the result as a file, yielding a `StorageFileInfo` whose `size` is a directory inode and which cannot be downloaded at all. `S3StorageAdapter.list` was RECURSIVE (`ListObjectsV2` matches the whole key) and read neither `IsTruncated` nor `ContinuationToken`, so past 1000 objects the "all files" a caller received was the first page, with no signal. One contract method, two dialects, both quietly incomplete — and the first feature that genuinely needed to enumerate a prefix (backup, orphan sweep, migration audit) would have got two different answers on two deployments without an error on either. #5172 was nearly that feature: it planned to drive attachment reclamation off `list(EMAIL_ATTACHMENT_KEY_PREFIX)`, found the local adapter could not see one level down, and switched to queue-driven deferred work instead. Nothing consumed it afterwards: the only in-repo call site was the `SwappableStorageService` pass-through (which itself rejects when the active adapter has no `list`), and REST, CLI and the storage routes never called it. Remove was chosen over align-and-tighten (maintainer ruling, 2026-08-05, #5266): aligning would grow a conformance surface nobody walks, while a prefix listing that cannot paginate is the wrong signature to inherit — when a real caller needs enumeration it returns cursor-shaped, `list(prefix, { cursor, limit })`, with adapter-conformance cases (nested keys, directory entries, >1000 objects) proving both backends agree. This is a TS/API contract surface — a storage adapter is CODE, never stack metadata — so there is no source for the chain to rewrite, and deliberately no schema tombstone: nothing ever ran an adapter through a `.parse()`, so a prescription there would reach no one. The enforced channel is tsc, and it reports at the call site. Same disposition, and the same reason, as `data-driver-find-stream-retired` (#4484). ADR-0049 / ADR-0087, #5540 (analysis #5266). - Done when: No code calls `storage.list(...)` on the `file-storage` service or on any `IStorageService` value. Code that needed "which files are under this prefix" reads the records it wrote — `sys_file` / file-reference rows carry the storage key and page deterministically through ObjectQL — rather than asking the bucket, which is also the only form that stays correct past 1000 objects and across both adapters. An adapter that still IMPLEMENTS `list` keeps compiling (an extra method is not an error on a class) and is simply unreachable through the contract, so deleting it is cleanup that can follow. The break is on the CALLER side: `storage.list(...)` no longer type-checks, and a PROXY typed against `IStorageService` that forwards to `inner.list` is exactly such a caller — the one in `@objectstack/service-storage` goes with the adapters (#5541). +- **`driver-aggregate-undeclared-key-aliases-removed`** — `driver aggregate() call argument — query.aggregate and aggregations[].func` → query.aggregations and aggregations[].function — the spellings QueryASTSchema and AggregationNodeSchema have always declared + - Why not automatic: `SqlDriver.aggregate` and `RemoteTransport.aggregate` each read two aliases the Query Protocol has never declared: `query.aggregations || query.aggregate` and `agg.function || agg.func`. "Never declared" is measured, not assumed — `git log -S` over `data/query.zod.ts` finds no commit that ever introduced either name, there is no `retiredKey()` tombstone and no alias-table entry for them (the file's only alias table is `SortNode`'s `direction` → `order`), and neither appears in any upgrade guide or release note. So this entry does not record a declared surface being withdrawn; it records a LENIENCY being withdrawn, which is why it is here rather than behind a tombstone. The only writers in this repository were the two driver packages' own fixtures — #4984's family, where a fixture spelling the alias keeps the tolerant limb green forever and no test in existence can go red on its deletion — so ADR-0049 enforce-or-remove applies once those are re-spelt. ⚠️ Do NOT read this across to `dashboard`/`page` measures: `aggregate` IS the canonical key there and `func` IS a declared, loudly-suggesting alias (`DatasetMeasureSchema`, ui/dataset.zod.ts). That neighbouring vocabulary is untouched, and it is the most likely reason an off-repo caller ever wrote these keys on a QUERY — one habit, two surfaces, only one of which declared it. This is a driver CALL ARGUMENT — code, never stack metadata — so there is no source for the D2 chain to rewrite and deliberately no schema tombstone: nothing ever ran a query through `QueryASTSchema.parse()` on this path. The enforced channel is tsc at the call site, once the parameter is `DriverQuery` — and for an untyped JS caller there is no enforced channel at all, which is exactly why this ledger entry has to exist: the generated upgrade guide is the only way such a reader learns of the rename. Same disposition, and the same reason, as `data-driver-find-stream-retired` (#4484), `storage-service-list-retired` (#5540) and `actor-user-roles-to-positions` (#6011). ADR-0049 / ADR-0087, #6321 (PR #6404). + - Done when: No caller passes `aggregate:` to a driver's `aggregate()`, and no aggregation entry spells its function `func:`; both are written `aggregations:` / `function:`. An inline literal still using either old spelling no longer type-checks (TS2353 at the call site). An untyped JS caller that keeps writing `aggregate:` silently receives no aggregate column — the grouping still happens, the measure is simply absent — and one that keeps writing `func:` receives INVALID_QUERY / 400 naming the undeclared function, identically on the local driver and the Turso remote transport. --- diff --git a/packages/spec/spec-changes.json b/packages/spec/spec-changes.json index 3bb2ce508f..4d55606a0b 100644 --- a/packages/spec/spec-changes.json +++ b/packages/spec/spec-changes.json @@ -630,6 +630,13 @@ "migrationId": "storage-service-list-retired", "toMajor": 17, "rationale": "`list(prefix)` was an OPTIONAL contract method documented as \"List files in a directory/prefix\", and the two shipped adapters answered the same call with two different semantics — both of them silently incomplete. `LocalStorageAdapter.list` was a single-level `readdir`, so a nested key `a/b/c` was invisible under `list('a')` (only `a/b` came back), and a subdirectory that `stat` succeeded on was pushed into the result as a file, yielding a `StorageFileInfo` whose `size` is a directory inode and which cannot be downloaded at all. `S3StorageAdapter.list` was RECURSIVE (`ListObjectsV2` matches the whole key) and read neither `IsTruncated` nor `ContinuationToken`, so past 1000 objects the \"all files\" a caller received was the first page, with no signal. One contract method, two dialects, both quietly incomplete — and the first feature that genuinely needed to enumerate a prefix (backup, orphan sweep, migration audit) would have got two different answers on two deployments without an error on either. #5172 was nearly that feature: it planned to drive attachment reclamation off `list(EMAIL_ATTACHMENT_KEY_PREFIX)`, found the local adapter could not see one level down, and switched to queue-driven deferred work instead. Nothing consumed it afterwards: the only in-repo call site was the `SwappableStorageService` pass-through (which itself rejects when the active adapter has no `list`), and REST, CLI and the storage routes never called it. Remove was chosen over align-and-tighten (maintainer ruling, 2026-08-05, #5266): aligning would grow a conformance surface nobody walks, while a prefix listing that cannot paginate is the wrong signature to inherit — when a real caller needs enumeration it returns cursor-shaped, `list(prefix, { cursor, limit })`, with adapter-conformance cases (nested keys, directory entries, >1000 objects) proving both backends agree. This is a TS/API contract surface — a storage adapter is CODE, never stack metadata — so there is no source for the chain to rewrite, and deliberately no schema tombstone: nothing ever ran an adapter through a `.parse()`, so a prescription there would reach no one. The enforced channel is tsc, and it reports at the call site. Same disposition, and the same reason, as `data-driver-find-stream-retired` (#4484). ADR-0049 / ADR-0087, #5540 (analysis #5266)." + }, + { + "surface": "driver aggregate() call argument — query.aggregate and aggregations[].func", + "replacement": "query.aggregations and aggregations[].function — the spellings QueryASTSchema and AggregationNodeSchema have always declared", + "migrationId": "driver-aggregate-undeclared-key-aliases-removed", + "toMajor": 17, + "rationale": "`SqlDriver.aggregate` and `RemoteTransport.aggregate` each read two aliases the Query Protocol has never declared: `query.aggregations || query.aggregate` and `agg.function || agg.func`. \"Never declared\" is measured, not assumed — `git log -S` over `data/query.zod.ts` finds no commit that ever introduced either name, there is no `retiredKey()` tombstone and no alias-table entry for them (the file's only alias table is `SortNode`'s `direction` → `order`), and neither appears in any upgrade guide or release note. So this entry does not record a declared surface being withdrawn; it records a LENIENCY being withdrawn, which is why it is here rather than behind a tombstone. The only writers in this repository were the two driver packages' own fixtures — #4984's family, where a fixture spelling the alias keeps the tolerant limb green forever and no test in existence can go red on its deletion — so ADR-0049 enforce-or-remove applies once those are re-spelt. ⚠️ Do NOT read this across to `dashboard`/`page` measures: `aggregate` IS the canonical key there and `func` IS a declared, loudly-suggesting alias (`DatasetMeasureSchema`, ui/dataset.zod.ts). That neighbouring vocabulary is untouched, and it is the most likely reason an off-repo caller ever wrote these keys on a QUERY — one habit, two surfaces, only one of which declared it. This is a driver CALL ARGUMENT — code, never stack metadata — so there is no source for the D2 chain to rewrite and deliberately no schema tombstone: nothing ever ran a query through `QueryASTSchema.parse()` on this path. The enforced channel is tsc at the call site, once the parameter is `DriverQuery` — and for an untyped JS caller there is no enforced channel at all, which is exactly why this ledger entry has to exist: the generated upgrade guide is the only way such a reader learns of the rename. Same disposition, and the same reason, as `data-driver-find-stream-retired` (#4484), `storage-service-list-retired` (#5540) and `actor-user-roles-to-positions` (#6011). ADR-0049 / ADR-0087, #6321 (PR #6404)." } ], "removed": [] @@ -1319,6 +1326,13 @@ "migrationId": "storage-service-list-retired", "toMajor": 17, "rationale": "`list(prefix)` was an OPTIONAL contract method documented as \"List files in a directory/prefix\", and the two shipped adapters answered the same call with two different semantics — both of them silently incomplete. `LocalStorageAdapter.list` was a single-level `readdir`, so a nested key `a/b/c` was invisible under `list('a')` (only `a/b` came back), and a subdirectory that `stat` succeeded on was pushed into the result as a file, yielding a `StorageFileInfo` whose `size` is a directory inode and which cannot be downloaded at all. `S3StorageAdapter.list` was RECURSIVE (`ListObjectsV2` matches the whole key) and read neither `IsTruncated` nor `ContinuationToken`, so past 1000 objects the \"all files\" a caller received was the first page, with no signal. One contract method, two dialects, both quietly incomplete — and the first feature that genuinely needed to enumerate a prefix (backup, orphan sweep, migration audit) would have got two different answers on two deployments without an error on either. #5172 was nearly that feature: it planned to drive attachment reclamation off `list(EMAIL_ATTACHMENT_KEY_PREFIX)`, found the local adapter could not see one level down, and switched to queue-driven deferred work instead. Nothing consumed it afterwards: the only in-repo call site was the `SwappableStorageService` pass-through (which itself rejects when the active adapter has no `list`), and REST, CLI and the storage routes never called it. Remove was chosen over align-and-tighten (maintainer ruling, 2026-08-05, #5266): aligning would grow a conformance surface nobody walks, while a prefix listing that cannot paginate is the wrong signature to inherit — when a real caller needs enumeration it returns cursor-shaped, `list(prefix, { cursor, limit })`, with adapter-conformance cases (nested keys, directory entries, >1000 objects) proving both backends agree. This is a TS/API contract surface — a storage adapter is CODE, never stack metadata — so there is no source for the chain to rewrite, and deliberately no schema tombstone: nothing ever ran an adapter through a `.parse()`, so a prescription there would reach no one. The enforced channel is tsc, and it reports at the call site. Same disposition, and the same reason, as `data-driver-find-stream-retired` (#4484). ADR-0049 / ADR-0087, #5540 (analysis #5266)." + }, + { + "surface": "driver aggregate() call argument — query.aggregate and aggregations[].func", + "replacement": "query.aggregations and aggregations[].function — the spellings QueryASTSchema and AggregationNodeSchema have always declared", + "migrationId": "driver-aggregate-undeclared-key-aliases-removed", + "toMajor": 17, + "rationale": "`SqlDriver.aggregate` and `RemoteTransport.aggregate` each read two aliases the Query Protocol has never declared: `query.aggregations || query.aggregate` and `agg.function || agg.func`. \"Never declared\" is measured, not assumed — `git log -S` over `data/query.zod.ts` finds no commit that ever introduced either name, there is no `retiredKey()` tombstone and no alias-table entry for them (the file's only alias table is `SortNode`'s `direction` → `order`), and neither appears in any upgrade guide or release note. So this entry does not record a declared surface being withdrawn; it records a LENIENCY being withdrawn, which is why it is here rather than behind a tombstone. The only writers in this repository were the two driver packages' own fixtures — #4984's family, where a fixture spelling the alias keeps the tolerant limb green forever and no test in existence can go red on its deletion — so ADR-0049 enforce-or-remove applies once those are re-spelt. ⚠️ Do NOT read this across to `dashboard`/`page` measures: `aggregate` IS the canonical key there and `func` IS a declared, loudly-suggesting alias (`DatasetMeasureSchema`, ui/dataset.zod.ts). That neighbouring vocabulary is untouched, and it is the most likely reason an off-repo caller ever wrote these keys on a QUERY — one habit, two surfaces, only one of which declared it. This is a driver CALL ARGUMENT — code, never stack metadata — so there is no source for the D2 chain to rewrite and deliberately no schema tombstone: nothing ever ran a query through `QueryASTSchema.parse()` on this path. The enforced channel is tsc at the call site, once the parameter is `DriverQuery` — and for an untyped JS caller there is no enforced channel at all, which is exactly why this ledger entry has to exist: the generated upgrade guide is the only way such a reader learns of the rename. Same disposition, and the same reason, as `data-driver-find-stream-retired` (#4484), `storage-service-list-retired` (#5540) and `actor-user-roles-to-positions` (#6011). ADR-0049 / ADR-0087, #6321 (PR #6404)." } ], "removed": [] diff --git a/packages/spec/src/migrations/registry.ts b/packages/spec/src/migrations/registry.ts index 16a19807ef..c20d4f5e31 100644 --- a/packages/spec/src/migrations/registry.ts +++ b/packages/spec/src/migrations/registry.ts @@ -2148,6 +2148,51 @@ const step17: MigrationStep = { + '`IStorageService` that forwards to `inner.list` is exactly such a caller — the ' + 'one in `@objectstack/service-storage` goes with the adapters (#5541).', }, + { + id: 'driver-aggregate-undeclared-key-aliases-removed', + // No backticks in `surface`: the upgrade-guide renderer wraps this string + // in a code span of its own, and a nested pair renders as literal ticks. + surface: "driver aggregate() call argument — query.aggregate and aggregations[].func", + replacement: + 'query.aggregations and aggregations[].function — the spellings QueryASTSchema and ' + + 'AggregationNodeSchema have always declared', + reason: + '`SqlDriver.aggregate` and `RemoteTransport.aggregate` each read two aliases the ' + + 'Query Protocol has never declared: `query.aggregations || query.aggregate` and ' + + '`agg.function || agg.func`. "Never declared" is measured, not assumed — `git log ' + + '-S` over `data/query.zod.ts` finds no commit that ever introduced either name, ' + + 'there is no `retiredKey()` tombstone and no alias-table entry for them (the file\'s ' + + 'only alias table is `SortNode`\'s `direction` → `order`), and neither appears in any ' + + 'upgrade guide or release note. So this entry does not record a declared surface ' + + 'being withdrawn; it records a LENIENCY being withdrawn, which is why it is here ' + + 'rather than behind a tombstone. The only writers in this repository were the two ' + + 'driver packages\' own fixtures — #4984\'s family, where a fixture spelling the alias ' + + 'keeps the tolerant limb green forever and no test in existence can go red on its ' + + 'deletion — so ADR-0049 enforce-or-remove applies once those are re-spelt. ⚠️ Do ' + + 'NOT read this across to `dashboard`/`page` measures: `aggregate` IS the canonical ' + + 'key there and `func` IS a declared, loudly-suggesting alias (`DatasetMeasureSchema`, ' + + 'ui/dataset.zod.ts). That neighbouring vocabulary is untouched, and it is the most ' + + 'likely reason an off-repo caller ever wrote these keys on a QUERY — one habit, two ' + + 'surfaces, only one of which declared it. This is a driver CALL ARGUMENT — code, ' + + 'never stack metadata — so there is no source for the D2 chain to rewrite and ' + + 'deliberately no schema tombstone: nothing ever ran a query through ' + + '`QueryASTSchema.parse()` on this path. The enforced channel is tsc at the call ' + + 'site, once the parameter is `DriverQuery` — and for an untyped JS caller there is ' + + 'no enforced channel at all, which is exactly why this ledger entry has to exist: ' + + 'the generated upgrade guide is the only way such a reader learns of the rename. ' + + 'Same disposition, and the same reason, as `data-driver-find-stream-retired` ' + + '(#4484), `storage-service-list-retired` (#5540) and `actor-user-roles-to-positions` ' + + '(#6011). ADR-0049 / ADR-0087, #6321 (PR #6404).', + acceptanceCriteria: + 'No caller passes `aggregate:` to a driver\'s `aggregate()`, and no aggregation entry ' + + 'spells its function `func:`; both are written `aggregations:` / `function:`. An ' + + 'inline literal still using either old spelling no longer type-checks (TS2353 at the ' + + 'call site). An untyped JS caller that keeps writing `aggregate:` silently receives ' + + 'no aggregate column — the grouping still happens, the measure is simply absent — ' + + 'and one that keeps writing `func:` receives INVALID_QUERY / 400 naming the ' + + 'undeclared function, identically on the local driver and the Turso remote ' + + 'transport.', + }, ], };