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..af10889540 --- /dev/null +++ b/.changeset/aggregate-driver-query-and-alias-retirement.md @@ -0,0 +1,53 @@ +--- +"@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/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/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); } 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.', + }, ], };