From 7e58cda3286bc216fe79cf2851014505e4ef1cda Mon Sep 17 00:00:00 2001 From: claude Date: Fri, 7 Aug 2026 10:45:46 +0000 Subject: [PATCH 1/2] =?UTF-8?q?fix(drivers):=20=E8=81=9A=E5=90=88=E5=87=BD?= =?UTF-8?q?=E6=95=B0=E6=8B=92=E6=94=B6=E5=B8=A6=E4=B8=8A=20ADR-0112=20?= =?UTF-8?q?=E4=BF=A1=E5=B0=81,=E5=B9=B6=E6=8A=8A=E4=B8=A4=E7=B1=BB?= =?UTF-8?q?=E6=9D=A1=E4=BB=B6=E5=88=86=E5=BC=80=E6=8E=AA=E8=BE=9E=20(#5907?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SqlDriver.mapAggregateFunc()` 与 `RemoteTransport.aggregate()` 对同一条件各抛 一个裸 `Error`(`code`/`status` 皆 `undefined`),`mapDataError` 因此落默认分支, 一条本该 4xx 的调用方错误以不透明 500 到达客户端。两处同改: - 协议未声明的函数名(`median`)→ `INVALID_QUERY` / 400,与协议门 (`metadata-protocol` 的 `invalidQueryError`,#4254)对同一条件已有的码一致。 - 协议已声明、本后端编不出(`count_distinct` / `array_agg` / `string_agg`)→ `NOT_IMPLEMENTED` / 501,标准目录的能力缺口码;措辞明确说查询拼写无误, 不把作者说成打错字(#5345 在 driver-memory 上画过的同一条线)。 两面首句逐字一致(#5240),由一条跨包 parity 用例比对两个**运行时**消息钉住。 编得出的五个函数生成的 SQL 逐字节不变;分类按调用方写的原始拼写判定,免得两面 归一化差异顺着信封扩散(#6203)。 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01WyvqvKMG6asi9aXjKE6xtx --- ...tidy-drivers-aggregate-refusal-envelope.md | 19 ++ content/docs/data-modeling/queries.mdx | 8 +- .../docs/protocol/objectql/query-syntax.mdx | 12 +- ...out-of-contract-aggregate-function.test.ts | 222 ++++++++++++++++ packages/drivers/driver-sql/src/sql-driver.ts | 163 +++++++++++- ...ansport-aggregate-function-refusal.test.ts | 244 ++++++++++++++++++ .../driver-turso/src/remote-transport.ts | 127 ++++++++- 7 files changed, 769 insertions(+), 26 deletions(-) create mode 100644 .changeset/tidy-drivers-aggregate-refusal-envelope.md create mode 100644 packages/drivers/driver-sql/src/sql-driver-out-of-contract-aggregate-function.test.ts create mode 100644 packages/drivers/driver-turso/src/remote-transport-aggregate-function-refusal.test.ts diff --git a/.changeset/tidy-drivers-aggregate-refusal-envelope.md b/.changeset/tidy-drivers-aggregate-refusal-envelope.md new file mode 100644 index 0000000000..3056a98f6e --- /dev/null +++ b/.changeset/tidy-drivers-aggregate-refusal-envelope.md @@ -0,0 +1,19 @@ +--- +'@objectstack/driver-sql': patch +'@objectstack/driver-turso': patch +--- + +drivers(sql,turso): 聚合函数拒收带上 ADR-0112 信封,并把两类条件分开措辞 + +`SqlDriver.mapAggregateFunc()` 与 `RemoteTransport.aggregate()` 此前对同一条件各抛一个裸 +`Error`(`code`/`status` 皆 `undefined`),`mapDataError` 因此落默认分支——一条本该 4xx 的 +调用方错误以不透明 500 到达客户端。两处同时改,同一信封体例、首句逐字一致(#5240): + +- **协议未声明的函数名**(如 `median`)→ `INVALID_QUERY` / 400。这正是协议门 + (`metadata-protocol` 的 `invalidQueryError`,#4254)对同一条件已经给出的码,于是 + 进程内调用方与 REST 调用方读到同一个答案。 +- **协议已声明、本后端编不出**(`count_distinct` / `array_agg` / `string_agg`)→ + `NOT_IMPLEMENTED` / 501。这是能力缺口而不是调用方的错(`driver-mongodb` 编得出这三个), + 措辞明确说明查询拼写无误,不把作者说成打错字。 + +两面都只改拒收的身份:编得出的五个函数生成的 SQL 逐字节不变。 diff --git a/content/docs/data-modeling/queries.mdx b/content/docs/data-modeling/queries.mdx index 7f256cd5f9..23eb5fc120 100644 --- a/content/docs/data-modeling/queries.mdx +++ b/content/docs/data-modeling/queries.mdx @@ -374,9 +374,11 @@ querying the related object directly. `count_distinct` / `array_agg` / `string_agg` are only fully supported on the MongoDB driver. -The SQL driver's aggregate function mapper throws `Unsupported aggregate function` for all -three (only `count`/`sum`/`avg`/`min`/`max` are mapped), and the in-memory driver's aggregator -silently returns `null` for them. Avoid these three on SQL- or memory-backed objects. +The SQL drivers map only `count`/`sum`/`avg`/`min`/`max` and refuse all three as a +**capability gap** — `501 NOT_IMPLEMENTED`, "declared but not implemented by this backend" +— rather than as a caller mistake, because the query is spelled correctly and the gap is +the backend's (#5907). The in-memory driver's aggregator silently returns `null` for them. +Avoid these three on SQL- or memory-backed objects. diff --git a/content/docs/protocol/objectql/query-syntax.mdx b/content/docs/protocol/objectql/query-syntax.mdx index 28a18e05ca..5cc99dc358 100644 --- a/content/docs/protocol/objectql/query-syntax.mdx +++ b/content/docs/protocol/objectql/query-syntax.mdx @@ -718,10 +718,14 @@ const query: QueryAST = { **Schema enum:** `count`, `sum`, `avg`, `min`, `max`, `count_distinct`, `array_agg`, `string_agg`. -Only `count`, `sum`, `avg`, `min`, and `max` are portable. `SqlDriver.mapAggregateFunc()` -throws `Unsupported aggregate function: ` for `count_distinct`, `array_agg`, and -`string_agg`; those three are implemented by the MongoDB driver and by the engine's -in-memory aggregation fallback, but not by the SQL drivers. +Only `count`, `sum`, `avg`, `min`, and `max` are portable. `count_distinct`, `array_agg` +and `string_agg` are implemented by the MongoDB driver and by the engine's in-memory +aggregation fallback, but not by the SQL drivers — on `SqlDriver` (and on the Turso +driver, both transports) they are refused as a **capability gap**: +`501 NOT_IMPLEMENTED`, "declared but not implemented by this backend". That is +deliberately a different answer from a function the schema enum never declared +(`median`), which is `400 INVALID_QUERY` — the caller's mistake — so an author who +wrote `count_distinct` is never told they made a typo (#5907). ### Group By Multiple Fields diff --git a/packages/drivers/driver-sql/src/sql-driver-out-of-contract-aggregate-function.test.ts b/packages/drivers/driver-sql/src/sql-driver-out-of-contract-aggregate-function.test.ts new file mode 100644 index 0000000000..121987c28a --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-out-of-contract-aggregate-function.test.ts @@ -0,0 +1,222 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5907] An aggregate function this driver cannot compile is refused with a + * WIRE IDENTITY — and with the identity that matches which kind of "no" it is. + * + * # What was measured on `origin/main` @ `80f7dc6a3` + * + * One `SqlDriver` (better-sqlite3, `:memory:`) and one `RemoteTransport`, the + * same `aggregations: [{ function, field: 'stage', alias: 'n' }]`: + * + * ``` + * REMOTE median -> THREW code=undefined status=undefined msg="Unsupported aggregate function: median" + * LOCAL median -> THREW code=undefined status=undefined msg="Unsupported aggregate function: median" + * REMOTE count_distinct -> THREW code=undefined status=undefined msg="Unsupported aggregate function: count_distinct" + * LOCAL count_distinct -> THREW code=undefined status=undefined msg="Unsupported aggregate function: count_distinct" + * REMOTE array_agg -> THREW code=undefined status=undefined msg="Unsupported aggregate function: array_agg" + * LOCAL array_agg -> THREW code=undefined status=undefined msg="Unsupported aggregate function: array_agg" + * REMOTE string_agg -> THREW code=undefined status=undefined msg="Unsupported aggregate function: string_agg" + * LOCAL string_agg -> THREW code=undefined status=undefined msg="Unsupported aggregate function: string_agg" + * ``` + * + * Two defects in one line. The `code`/`status` are absent, so `mapDataError` + * falls to its default branch and a caller's `median` typo arrives as an opaque + * 500 — the #1116/#1117 gap moved from the filter door to the aggregate door. + * And the two conditions are indistinguishable: `median` is a name the Query + * Protocol never declared, while `count_distinct` IS declared (and compiled by + * `driver-mongodb`, and by `driver-memory`'s analytics face), so one message for + * both tells a dashboard author their correct query is a typo — the line #5345 + * drew in `driver-memory`'s `filter-refusal.ts` between "the protocol has no + * such operator" and "the protocol has it, this face cannot lower it". + * + * # ⚠️ Why every case asserts `code` AND `status`, never merely "it threw" + * + * Read the measurement again: the UNFIXED driver throws on all four inputs. A + * test that asserted only `rejects.toThrow()` would have been green before this + * change and green after it — permanently blind to the entire defect (#6144). + * The refusal was never missing; its wire identity was. + * + * # Reverse verification — direction predicted BEFORE it was run + * + * Prediction: with `mapAggregateFunc`'s bare `throw new Error(...)` restored and + * nothing else changed, every refusal case here goes RED on its FIRST assertion + * (`err.code` → `undefined`), and NOT ONE fails through `refusalOf`'s "expected + * a refusal, but it resolved" branch — because the un-fixed driver refuses + * exactly the same inputs, just anonymously. The controls (the five compiled + * functions, and the values they compute) must stay GREEN, pinning that the + * change moved the refusal's identity and nothing else. + * + * Measured after writing that down, with `mapAggregateFunc`'s refusal replaced + * by ``throw new Error(`Unsupported aggregate function: ${func}`)`` and nothing + * else changed: **10 failed / 4 passed** of 14. Every one of the 10 failed on + * `expected undefined to be 'INVALID_QUERY'` or `expected undefined to be + * 'NOT_IMPLEMENTED'` — not one through `refusalOf`'s "it resolved" branch, which + * is the predicted direction and the #6144 point restated as evidence: the + * refusal was already there, only its identity was missing. The 4 green are the + * three controls plus the declared-minus-compiled fixture guard, pinning that + * nothing outside the refusal's identity moved. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { SqlDriver } from './index.js'; +import { AggregationFunction } from '@objectstack/spec/data'; + +interface WireBearingError extends Error { + code?: string; + status?: number; +} + +/** + * The first sentences, spelled out here rather than imported: this is the + * contract #5240 asks for ("one condition, one wording"), and a test that read + * the same constant the producer reads would pass however the wording drifted. + * The twin in `driver-turso` repeats these bytes, and + * `remote-transport-aggregate-function-refusal.test.ts` compares the two + * RUNTIME messages so the two faces cannot drift apart either. + */ +const UNDECLARED_SENTENCE = (f: string) => + `Aggregate function "${f}" is not a declared aggregate function.`; +const UNCOMPILABLE_SENTENCE = (f: string) => + `Aggregate function "${f}" is declared but not implemented by this backend.`; + +describe('[#5907] SqlDriver refuses an aggregate function it cannot compile', () => { + 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' }, + score: { type: 'number', name: 'score' }, + }, + } as any, + ]); + await driver.create('deal', { id: '1', stage: 'won', score: 10 }); + await driver.create('deal', { id: '2', stage: 'lost', score: 20 }); + }); + + const aggregate = (fn: string, field: string | undefined = 'stage') => + driver.aggregate('deal', { + object: 'deal', + aggregations: [{ function: fn, ...(field ? { field } : {}), alias: 'n' }], + } as any); + + const refusalOf = async (fn: string): Promise => { + try { + await aggregate(fn); + } catch (e) { + return e as WireBearingError; + } + throw new Error(`expected the driver to refuse "${fn}", but it resolved`); + }; + + // ── Class 1: the Query Protocol does not declare this name ───────────────── + + describe('a function name the Query Protocol never declared', () => { + // `median` is the issue's own repro. The rest are the names a SQL-fluent + // author reaches for that `AggregationFunction` does not declare. + const UNDECLARED = ['median', 'stddev', 'percentile_cont', 'group_concat']; + + for (const fn of UNDECLARED) { + it(`refuses "${fn}" with INVALID_QUERY / 400`, async () => { + const err = await refusalOf(fn); + expect(err.code).toBe('INVALID_QUERY'); + expect(err.status).toBe(400); + expect(err.message.startsWith(UNDECLARED_SENTENCE(fn))).toBe(true); + // The remedy is in the message: what the protocol DOES declare. + for (const declared of AggregationFunction.options) { + expect(err.message).toContain(declared); + } + // …and it must not be mistaken for the capability-gap answer. + expect(err.message).not.toContain('capability gap'); + // #3867 — no driver-internal prefix on the wire. + expect(err.message).not.toContain('[sql-driver]'); + }); + } + + // The case-sensitivity ruling, pinned. `AggregationFunction` is a + // case-SENSITIVE `z.enum`, so `COUNT_DISTINCT` is not `count_distinct` and + // "declared but not implemented" would be false of it. It also keeps the two + // faces in step: the remote transport lowercases before ITS lookup, so + // classifying on each face's post-normalisation name would answer 400 here + // and 501 there for one query — the local/remote fork this issue closes. + const MISCASED = ['COUNT_DISTINCT', 'Median', 'COUNT']; + for (const fn of MISCASED) { + it(`refuses the miscased "${fn}" as UNDECLARED (400), not as a capability gap`, async () => { + const err = await refusalOf(fn); + expect(err.code).toBe('INVALID_QUERY'); + expect(err.status).toBe(400); + expect(err.message.startsWith(UNDECLARED_SENTENCE(fn))).toBe(true); + // The caller's own spelling is quoted back — that is the actionable part. + expect(err.message).toContain(`"${fn}"`); + }); + } + }); + + // ── Class 2: declared by the protocol, not compiled by this backend ──────── + + describe('a DECLARED function this backend cannot compile', () => { + // Exactly the three `AggregationFunction` declares with no SQL lowering. + const UNCOMPILABLE = ['count_distinct', 'array_agg', 'string_agg']; + + // Guard: the fixture is the real declared-minus-compiled set, derived rather + // than trusted. If the spec drops one (that decision is #6188) or this driver + // implements one, this fails HERE rather than leaving a case that passes + // because nothing is produced. + it('the fixture is exactly the declared-but-uncompiled set', () => { + const compiled = ['count', 'sum', 'avg', 'min', 'max']; + expect([...AggregationFunction.options].filter((f) => !compiled.includes(f)).sort()) + .toEqual([...UNCOMPILABLE].sort()); + }); + + for (const fn of UNCOMPILABLE) { + it(`refuses "${fn}" with NOT_IMPLEMENTED / 501`, async () => { + const err = await refusalOf(fn); + expect(err.code).toBe('NOT_IMPLEMENTED'); + expect(err.status).toBe(501); + expect(err.message.startsWith(UNCOMPILABLE_SENTENCE(fn))).toBe(true); + // ⛔ The author is NOT told they made a mistake — the whole point of + // splitting the two classes (#5345's line, applied to aggregations). + expect(err.message).not.toContain('is not a declared aggregate function'); + expect(err.message).toContain('spelled'); + expect(err.message).toContain('capability gap'); + // The functions that DO work here, so the message is actionable. + expect(err.message).toContain('count, sum, avg, min, max'); + expect(err.message).not.toContain('[sql-driver]'); + }); + } + }); + + // ── Controls: nothing but the refusal's identity moved ───────────────────── + + describe('the compiled vocabulary is untouched', () => { + it('every function this driver lowers still computes its value', async () => { + expect(await aggregate('count', 'id')).toEqual([{ n: 2 }]); + expect(await aggregate('sum', 'score')).toEqual([{ n: 30 }]); + expect(await aggregate('avg', 'score')).toEqual([{ n: 15 }]); + expect(await aggregate('min', 'score')).toEqual([{ n: 10 }]); + expect(await aggregate('max', 'score')).toEqual([{ n: 20 }]); + }); + + it('COUNT(*) — the `field`-less spelling the spec allows — still answers', async () => { + expect(await aggregate('count', undefined)).toEqual([{ n: 2 }]); + }); + + it('grouped aggregation still answers', async () => { + const rows = await driver.aggregate('deal', { + object: 'deal', + groupBy: ['stage'], + aggregations: [{ function: 'count', field: 'id', alias: 'n' }], + } as any); + expect((rows as any[]).map((r) => `${r.stage}:${r.n}`).sort()).toEqual(['lost:1', 'won:1']); + }); + }); +}); diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index 9b493fd8f9..3de513df89 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -9,6 +9,10 @@ import type { QueryAST, DriverOptions, SchemaMode } from '@objectstack/spec/data'; import { parseAutonumberFormat, renderAutonumber, missingFieldValues, isTenancyDisabled, type AutonumberToken } from '@objectstack/spec/data'; +// The DECLARED aggregate vocabulary (#5907). Read from the spec so this driver's +// "the protocol has no such function" refusal cannot drift from what +// `AggregationNodeSchema.function` actually admits. +import { AggregationFunction } from '@objectstack/spec/data'; import { STRUCTURED_JSON_TYPES, FILE_REFERENCE_TYPES, MULTI_OPTION_TYPES, NUMERIC_VALUE_TYPES } from '@objectstack/spec/data'; // `defaultValue` runtime tokens (#4560). The DDL below asks the SPEC — not a // list of its own — which `defaultValue`s are instructions rather than literals, @@ -485,6 +489,137 @@ function unsupportedFilterError(message: string): Error { return err; } +/** + * [#5907] The aggregate functions this driver LOWERS into SQL, and the SQL + * function each becomes. + * + * The refusals below read their "compiled here" list off THIS table instead of + * repeating it. A hand-written copy agrees with the compiler on the day it is + * typed and never again — the note already sitting over `driver-memory`'s + * `SUPPORTED_FIELD_OPERATORS` (#5345), applied to the aggregate vocabulary. + * + * A `Map` rather than a plain object on purpose: a caller-supplied name is + * looked up here, and `{}['constructor']` answers with a function. + */ +const SQL_AGGREGATE_FUNCTIONS: ReadonlyMap = new Map([ + ['count', 'count'], + ['sum', 'sum'], + ['avg', 'avg'], + ['min', 'min'], + ['max', 'max'], +]); + +/** + * [#5907] The aggregate vocabulary the Query Protocol DECLARES, read from the + * spec rather than restated — `AggregationNodeSchema.function` is this enum, so + * "declared" has exactly one definition and this driver cannot drift from it. + */ +const DECLARED_AGGREGATE_FUNCTIONS: readonly string[] = AggregationFunction.options; + +/** + * [#5907] Class 1 — a function name the Query Protocol does not declare. + * + * The caller wrote something no backend can run (`median`), so this is a + * request-shaped mistake: `INVALID_QUERY` / 400, the catalogued + * `StandardErrorCode` for "malformed query syntax" and a member of + * `@objectstack/rest`'s `isExpectedQueryRejection` list, so a client mistake + * stops being logged as an unhandled server fault. + * + * `INVALID_QUERY` is not a new spelling for this condition — it is the one the + * PROTOCOL DOOR already gives it. `metadata-protocol`'s `invalidQueryError` + * refuses "a function outside the spec enum" on the aggregations axis with + * exactly `400 INVALID_QUERY` (#4254), so a caller who reaches this driver + * in-process gets the same wire identity as one who came through REST: one + * condition, one code, however the caller arrived — the argument + * {@link unsupportedFilterError} makes for `INVALID_FILTER`. + * + * The FIRST SENTENCE is shared verbatim with the twin in `driver-turso`'s + * `remote-transport.ts` (#5240 — one condition, one wording): a caller must not + * be able to tell which transport answered from the words it used. The parity is + * pinned by a test that compares the two RUNTIME messages, not two copies of a + * literal (`remote-transport-aggregate-function-refusal.test.ts`). + * + * Judged against the declared enum CASE-SENSITIVELY, which is what the enum is: + * `COUNT_DISTINCT` is not `count_distinct`, and answering "declared but not + * implemented" for it would be false. It also keeps the two faces in step — + * this driver reads the name raw while the remote transport lowercases it + * before its own lookup, so classifying on each face's post-normalisation name + * would hand `COUNT_DISTINCT` a 400 here and a 501 there for one query. + */ +function undeclaredAggregateFunctionError(func: string): Error { + const err = new Error( + `Aggregate function "${func}" is not a declared aggregate function. ` + + `Declared functions: ${DECLARED_AGGREGATE_FUNCTIONS.join(', ')} ` + + `(@objectstack/spec AggregationFunction). Fix the "function" key of the aggregations[] ` + + `entry — the Query Protocol has no such function, so this is a query no backend can run, ` + + `not a gap in this one (#5907).`, + ) as Error & { code?: string; status?: number }; + err.code = StandardErrorCode.enum.INVALID_QUERY; + err.status = 400; + return err; +} + +/** + * [#5907] Class 2 — a DECLARED function this backend cannot compile. + * + * Distinct from {@link undeclaredAggregateFunctionError} on purpose, and this is + * the half that must not be collapsed into it: `count_distinct`, `array_agg` and + * `string_agg` are declared by `AggregationFunction` and implemented by other + * backends (`driver-mongodb` compiles all three, `driver-memory`'s analytics + * face compiles `count_distinct`), so telling a dashboard author their + * `count_distinct` is a typo would be false — the same line #5345 drew in + * `driver-memory`'s `filter-refusal.ts` between `unknownFieldOperatorError` and + * `uncompilableFieldOperatorError`. + * + * `NOT_IMPLEMENTED` / 501 is the answer, from the ADR-0112 STANDARD catalog + * ("Feature not yet implemented"), whose own `HttpStatusErrorCodeMap` pairs it + * with 501 — so code and status are each other's mirror by construction rather + * than by this function's choice. It is the spelling the repo already uses for + * every "not supported by this protocol/runtime" answer, and the ledger's rule + * for a generic condition is the standard catalog over a registered synonym. + * The registered alternatives were measured and rejected: `UNSUPPORTED` is a + * 400 in both places that emit it (a share link that does not expose messages), + * `UNSUPPORTED_QUERY_PARAM` is a 400 on the client-mistake list, and + * `UNSUPPORTED_TRANSFORM` belongs to `@objectstack/rest`'s import mapper. + * + * Measured consequence, recorded so it is not rediscovered as a bug: on the + * `/data` routes `mapDataError`'s generic status passthrough is 4xx-ONLY, so + * this declared 501 does not survive to the wire — it falls to + * `UNCLASSIFIED_FAULT`'s `500 INTERNAL_ERROR`. That is a gap in the REST + * boundary — #5582, which this is the first live producer for — not a reason + * for the driver to misdescribe the fault as the caller's. The driver's job is + * to state the condition truthfully at the throw site (ADR-0112), which is also + * what reaches every in-process caller and the operator log. + */ +function uncompilableAggregateFunctionError(func: string): Error { + const err = new Error( + `Aggregate function "${func}" is declared but not implemented by this backend. ` + + `Compiled here: ${[...SQL_AGGREGATE_FUNCTIONS.keys()].join(', ')}. The name is spelled ` + + `correctly and @objectstack/spec AggregationFunction 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. Aggregate with a function this backend compiles; whether the declaration ` + + `itself should stand is #6188 (ADR-0049 enforce-or-remove) (#5907).`, + ) as Error & { code?: string; status?: number }; + err.code = StandardErrorCode.enum.NOT_IMPLEMENTED; + err.status = 501; + return err; +} + +/** + * [#5907] Which refusal a name that this face cannot compile deserves. + * + * The classification is written ONCE per face and shared by both faces' + * throw sites, so "is this the caller's mistake or ours?" cannot be answered two + * ways for one query. `func` is the name the CALLER wrote — not a normalised + * form — because that is what the enum is judged against and what the message + * has to quote back. + */ +function refuseAggregateFunction(func: string): never { + throw DECLARED_AGGREGATE_FUNCTIONS.includes(func) + ? uncompilableAggregateFunctionError(func) + : undeclaredAggregateFunctionError(func); +} + /** * [#5158] A `FilterArray` reached the driver unlowered. * @@ -7055,21 +7190,21 @@ export class SqlDriver implements IDataDriver { return out; } + /** + * The SQL function a declared aggregation lowers to, or a refusal that says + * which KIND of "no" this is (#5907). + * + * The `switch` this replaced answered both conditions with one bare `Error` + * carrying no `code` and no `status`, so `mapDataError` fell to its default + * branch and a caller's `median` typo arrived as an opaque 500 — the #1116 / + * #1117 gap, at the aggregate door. The lowering table is now the single + * source of what this face compiles, and {@link refuseAggregateFunction} + * decides between the two refusals. + */ protected mapAggregateFunc(func: string): string { - switch (func) { - case 'count': - return 'count'; - case 'sum': - return 'sum'; - case 'avg': - return 'avg'; - case 'min': - return 'min'; - case 'max': - return 'max'; - default: - throw new Error(`Unsupported aggregate function: ${func}`); - } + const sql = SQL_AGGREGATE_FUNCTIONS.get(func); + if (sql !== undefined) return sql; + refuseAggregateFunction(func); } // ── Window function builder ───────────────────────────────────────────────── diff --git a/packages/drivers/driver-turso/src/remote-transport-aggregate-function-refusal.test.ts b/packages/drivers/driver-turso/src/remote-transport-aggregate-function-refusal.test.ts new file mode 100644 index 0000000000..0df1cf623c --- /dev/null +++ b/packages/drivers/driver-turso/src/remote-transport-aggregate-function-refusal.test.ts @@ -0,0 +1,244 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5907] An aggregate function this transport cannot compile is refused with a + * WIRE IDENTITY — the same one, in the same words, that the LOCAL driver gives. + * + * # Why both faces, in one issue + * + * `TursoDriver` picks its transport from `url`: a local/replica URL inherits + * `SqlDriver`, a remote one delegates here. Both faces threw a bare `Error` for + * this condition, so fixing only this one would have created the divergence + * #5769 spent a whole issue closing — one condition, two wire identities, + * decided by a connection string. Measured on `origin/main` @ `80f7dc6a3`: + * + * ``` + * REMOTE median -> THREW code=undefined status=undefined msg="Unsupported aggregate function: median" + * LOCAL median -> THREW code=undefined status=undefined msg="Unsupported aggregate function: median" + * REMOTE count_distinct -> THREW code=undefined status=undefined msg="Unsupported aggregate function: count_distinct" + * LOCAL count_distinct -> THREW code=undefined status=undefined msg="Unsupported aggregate function: count_distinct" + * ``` + * + * `code`/`status` absent on both, so `mapDataError` served an opaque 500 for + * what is a 400-class caller mistake (`median`) or a 501-class capability gap + * (`count_distinct` — declared by `AggregationFunction`, compiled by + * `driver-mongodb`). The parity test at the bottom is the half of this file that + * cannot be satisfied by editing one package. + * + * # ⚠️ Every case asserts `code` AND `status` + * + * The un-fixed transport already threw on every input below. `rejects.toThrow()` + * would have been green before and after — blind to the whole defect (#6144). + * + * # Reverse verification — direction predicted BEFORE it was run + * + * Prediction: unlike the `undefined`-comparand twin (#6050), where the un-fixed + * transport ANSWERED and the reverted tests went red by resolving, this one + * refused all along. So with the bare `throw new Error(...)` restored, every + * refusal case must fail on its FIRST assertion (`err.code` → `undefined`) and + * none through `refusalOf`'s "it resolved" branch; the parity cases must fail on + * the comparison; the controls (SQL text, computed vocabulary) stay green. + * + * Measured — and the prediction was HALF WRONG, in the way that matters most + * here, so it is recorded rather than tidied: + * + * 1. **Both faces reverted**: 8 failed / 8 passed of 16. All 8 failures on + * `expected undefined to be 'INVALID_QUERY' / 'NOT_IMPLEMENTED'`, none on + * "it resolved" — as predicted. But the four PARITY cases stayed **green**: + * with both faces anonymous they agree on `undefined`/`undefined` and (for a + * lowercase name) on the same message text. A parity test measures + * AGREEMENT, not correctness; reverting both halves keeps them agreeing. + * 2. **ONE face reverted** — measured by accident first, from a stale + * `@objectstack/driver-sql` `dist/` while this package's source was fixed: + * the four parity cases fail with `expected 'NOT_IMPLEMENTED' to be + * undefined` and `expected 'INVALID_QUERY' to be undefined`. + * + * (2) is the direction this file exists for. The defect these tests guard is not + * "the transport is silent" — it is "the two faces answer one query + * differently", so the case that must go red is the SINGLE-face change, which is + * exactly what a future PR touching only one package would produce. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { RemoteTransport } from './remote-transport.js'; +import { AggregationFunction } from '@objectstack/spec/data'; + +interface WireBearingError extends Error { + code?: string; + status?: number; +} + +/** See the twin's note: the sentences are spelled out, not imported. */ +const UNDECLARED_SENTENCE = (f: string) => + `Aggregate function "${f}" is not a declared aggregate function.`; +const UNCOMPILABLE_SENTENCE = (f: string) => + `Aggregate function "${f}" is declared but not implemented 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 }; +} + +const aggregateOn = (t: RemoteTransport, fn: string) => + t.aggregate('deal', { aggregations: [{ function: fn, field: 'stage', alias: 'n' }] } as any); + +async function refusalOf(fn: string): Promise { + const { t, calls } = transportWithCapturingClient(); + try { + await aggregateOn(t, fn); + } catch (e) { + // A refused aggregation must not have reached the database on its way to + // throwing — a statement that ran is a statement that cost a round trip and + // may have scanned the table. + expect(calls).toEqual([]); + return e as WireBearingError; + } + throw new Error(`expected the transport to refuse "${fn}", but it compiled to ${JSON.stringify(calls)}`); +} + +describe('[#5907] RemoteTransport refuses an aggregate function it cannot compile', () => { + describe('a function name the Query Protocol never declared', () => { + const UNDECLARED = ['median', 'stddev', 'percentile_cont', 'group_concat']; + + for (const fn of UNDECLARED) { + it(`refuses "${fn}" with INVALID_QUERY / 400`, async () => { + const err = await refusalOf(fn); + expect(err.code).toBe('INVALID_QUERY'); + expect(err.status).toBe(400); + expect(err.message.startsWith(UNDECLARED_SENTENCE(fn))).toBe(true); + for (const declared of AggregationFunction.options) { + expect(err.message).toContain(declared); + } + expect(err.message).not.toContain('capability gap'); + // #1116's note: this transport's refusals no longer wear a + // `[RemoteTransport]` prefix — driver-internal wording is not the + // caller's business. + expect(err.message).not.toContain('[RemoteTransport]'); + }); + } + + it('quotes the spelling the CALLER wrote, not the normalised one', async () => { + // This transport lowercases before its own lookup; the refusal is judged + // and worded on the caller's own bytes, so `COUNT_DISTINCT` is undeclared + // here exactly as it is on the local driver — the two faces agree on the + // class instead of splitting 400/501 over a normalisation difference. + const err = await refusalOf('COUNT_DISTINCT'); + expect(err.code).toBe('INVALID_QUERY'); + expect(err.status).toBe(400); + expect(err.message).toContain('"COUNT_DISTINCT"'); + expect(err.message).not.toContain('"count_distinct"'); + }); + }); + + describe('a DECLARED function this backend cannot compile', () => { + const UNCOMPILABLE = ['count_distinct', 'array_agg', 'string_agg']; + + it('the fixture is exactly the declared-but-uncompiled set', () => { + const compiled = ['count', 'sum', 'avg', 'min', 'max']; + expect([...AggregationFunction.options].filter((f) => !compiled.includes(f)).sort()) + .toEqual([...UNCOMPILABLE].sort()); + }); + + for (const fn of UNCOMPILABLE) { + it(`refuses "${fn}" with NOT_IMPLEMENTED / 501`, async () => { + const err = await refusalOf(fn); + expect(err.code).toBe('NOT_IMPLEMENTED'); + expect(err.status).toBe(501); + expect(err.message.startsWith(UNCOMPILABLE_SENTENCE(fn))).toBe(true); + expect(err.message).not.toContain('is not a declared aggregate function'); + expect(err.message).toContain('capability gap'); + expect(err.message).toContain('count, sum, avg, min, max'); + expect(err.message).not.toContain('[RemoteTransport]'); + }); + } + }); + + // ── The cross-package half: one condition, one wording ───────────────────── + + describe('local/remote parity (#5240 — one condition, one wording)', () => { + const localRefusalOf = async (fn: string): 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, + ]); + try { + await d.aggregate('deal', { aggregations: [{ function: fn, field: 'stage', alias: 'n' }] } as any); + } catch (e) { + return e as WireBearingError; + } + throw new Error(`expected the local driver to refuse "${fn}", but it resolved`); + }; + + // Compared as RUNTIME messages from the two packages, not as two copies of a + // literal — a shared constant would agree with itself no matter how far the + // two faces drifted. This is what makes "首句逐字一致" checkable. + for (const fn of ['median', 'count_distinct', 'array_agg', 'string_agg']) { + it(`"${fn}" is answered identically by the local driver and this transport`, async () => { + const remote = await refusalOf(fn); + const local = await localRefusalOf(fn); + expect(remote.code).toBe(local.code); + expect(remote.status).toBe(local.status); + // The first sentence is the contract; the tails happen to coincide too + // because both faces compile the same five functions today, so the whole + // message is compared while that holds. + expect(remote.message.split('. ')[0]).toBe(local.message.split('. ')[0]); + expect(remote.message).toBe(local.message); + }); + } + }); + + // ── Controls: nothing but the refusal's identity moved ───────────────────── + + describe('the compiled vocabulary is untouched', () => { + it('emits byte-identical SQL for the five functions it lowers', async () => { + for (const fn of ['count', 'sum', 'avg', 'min', 'max']) { + const { t, calls } = transportWithCapturingClient(); + await aggregateOn(t, fn); + expect(calls.map((c) => c.sql)).toEqual([`SELECT ${fn}("stage") AS "n" FROM "deal"`]); + } + }); + + it('the default alias still spells itself with the NORMALISED name', async () => { + const { t, calls } = transportWithCapturingClient(); + await t.aggregate('deal', { aggregations: [{ function: 'COUNT', field: 'stage' }] } as any); + expect(calls[0].sql).toBe('SELECT count("stage") AS "count_stage" FROM "deal"'); + }); + + /** + * Pinned as it IS, not as it should be. This transport lowercases the + * function name before its lookup and the local driver does not, so `COUNT` + * compiles here and is refused there — measured on `origin/main` @ + * `80f7dc6a3`, before this change and unchanged by it: + * + * ``` + * COUNT REMOTE -> RESOLVED "SELECT count(...)" LOCAL -> THREW + * ``` + * + * That fork is a normalisation question, not an envelope one: no in-repo + * caller emits a miscased name and `AggregationNodeSchema.function` cannot + * express one, so it is filed as **#6203** rather than fixed under this + * issue's envelope scope. This case exists so the next reader finds it + * recorded instead of rediscovering it, and so a change to the normalisation + * is a deliberate one that has to come here and say so. + */ + it('[filed, not fixed] `COUNT` still compiles HERE while the local driver refuses it', async () => { + const { t, calls } = transportWithCapturingClient(); + await aggregateOn(t, 'COUNT'); + expect(calls.map((c) => c.sql)).toEqual(['SELECT count("stage") AS "n" FROM "deal"']); + }); + }); +}); diff --git a/packages/drivers/driver-turso/src/remote-transport.ts b/packages/drivers/driver-turso/src/remote-transport.ts index e7cb8c3218..c940fb383d 100644 --- a/packages/drivers/driver-turso/src/remote-transport.ts +++ b/packages/drivers/driver-turso/src/remote-transport.ts @@ -15,6 +15,10 @@ import type { Client, InStatement, ResultSet } from '@libsql/client'; import { StandardErrorCode } from '@objectstack/spec/api'; import { FILTER_OPERATORS, LOGICAL_OPERATORS } from '@objectstack/spec/data'; +// The DECLARED aggregate vocabulary (#5907) — read from the spec so this +// 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 { nanoid } from 'nanoid'; /** @@ -503,6 +507,112 @@ function invalidFilterError(message: string): Error { return err; } +/** + * [#5907] The aggregate functions this TRANSPORT lowers into SQL, and the SQL + * function each becomes. + * + * The twin of `driver-sql`'s `SQL_AGGREGATE_FUNCTIONS`, and separate on purpose: + * this transport is deliberately free of knex and of `SqlDriver` (see the file + * header), so the two faces state their own capability and the refusals below + * read it off the table instead of a hand-kept list (#5345). They compile the + * same five today; a face that gains one says so here, alone. + */ +const REMOTE_AGGREGATE_FUNCTIONS: ReadonlyMap = new Map([ + ['count', 'count'], + ['sum', 'sum'], + ['avg', 'avg'], + ['min', 'min'], + ['max', 'max'], +]); + +/** + * [#5907] The aggregate vocabulary the Query Protocol DECLARES, read from the + * spec rather than restated — `AggregationNodeSchema.function` is this enum. + */ +const DECLARED_AGGREGATE_FUNCTIONS: readonly string[] = AggregationFunction.options; + +/** + * [#5907] Class 1 — a function name the Query Protocol does not declare. + * + * The twin of `driver-sql`'s `undeclaredAggregateFunctionError`, word for word, + * and that is the point of this whole issue: the same `median` had to stop + * meaning two different things depending on whether `url` put the caller on the + * local driver or on this transport. #5240's rule — one condition, one wording — + * is pinned across the two packages by + * `remote-transport-aggregate-function-refusal.test.ts`, which compares the two + * RUNTIME messages rather than two copies of a literal. + * + * `INVALID_QUERY` / 400: the caller wrote a name no backend can run, and 400 + * puts it on `@objectstack/rest`'s `isExpectedQueryRejection` list so a client + * mistake stops being logged as an unhandled server fault. It is the code the + * PROTOCOL DOOR already gives this condition — `metadata-protocol`'s + * `invalidQueryError` refuses "a function outside the spec enum" on the + * aggregations axis with `400 INVALID_QUERY` (#4254) — so the in-process caller + * and the REST caller now read the same answer. + */ +function undeclaredAggregateFunctionError(func: string): Error { + const err = new Error( + `Aggregate function "${func}" is not a declared aggregate function. ` + + `Declared functions: ${DECLARED_AGGREGATE_FUNCTIONS.join(', ')} ` + + `(@objectstack/spec AggregationFunction). Fix the "function" key of the aggregations[] ` + + `entry — the Query Protocol has no such function, so this is a query no backend can run, ` + + `not a gap in this one (#5907).`, + ) as Error & { code?: string; status?: number }; + err.code = StandardErrorCode.enum.INVALID_QUERY; + err.status = 400; + return err; +} + +/** + * [#5907] Class 2 — a DECLARED function this transport cannot compile. + * + * The twin of `driver-sql`'s `uncompilableAggregateFunctionError`; its docblock + * carries the full rationale for `NOT_IMPLEMENTED` / 501 and for why the two + * classes must not be collapsed. In one line: `count_distinct`, `array_agg` and + * `string_agg` are declared and other backends compile them, so a caller who + * wrote one has made no mistake — this is the backend's gap, and an error that + * says otherwise tells a dashboard author to fix a query that is already right. + */ +function uncompilableAggregateFunctionError(func: string): Error { + const err = new Error( + `Aggregate function "${func}" is declared but not implemented by this backend. ` + + `Compiled here: ${[...REMOTE_AGGREGATE_FUNCTIONS.keys()].join(', ')}. The name is spelled ` + + `correctly and @objectstack/spec AggregationFunction 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. Aggregate with a function this backend compiles; whether the declaration ` + + `itself should stand is #6188 (ADR-0049 enforce-or-remove) (#5907).`, + ) as Error & { code?: string; status?: number }; + err.code = StandardErrorCode.enum.NOT_IMPLEMENTED; + err.status = 501; + return err; +} + +/** + * [#5907] Which refusal a name this transport cannot compile deserves — the + * twin of `driver-sql`'s `refuseAggregateFunction`. + * + * `func` is the name the CALLER wrote, before this transport's `toLowerCase()`. + * Measured on `origin/main` @ `80f7dc6a3`, the two faces do not normalise alike: + * + * ``` + * COUNT REMOTE -> RESOLVED ("SELECT count(...)") LOCAL -> threw + * COUNT_DISTINCT REMOTE -> threw "…: count_distinct" LOCAL -> threw "…: COUNT_DISTINCT" + * ``` + * + * Judging the CALLER's spelling against the case-sensitive enum is what keeps + * that pre-existing fork from spreading into the envelope: on the lowercased + * name this transport would call `COUNT_DISTINCT` a capability gap (501) while + * the local driver, reading the name raw, called it undeclared (400) — one + * query, two wire identities, which is the fork this issue exists to close. + * The normalisation difference itself — `COUNT` compiles here and is refused by + * the local driver — is untouched by this change and filed as #6203. + */ +function refuseAggregateFunction(func: string): never { + throw DECLARED_AGGREGATE_FUNCTIONS.includes(func) + ? uncompilableAggregateFunctionError(func) + : undeclaredAggregateFunctionError(func); +} + /** * 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 @@ -721,10 +831,13 @@ export class RemoteTransport { const aggregations = query?.aggregations || query?.aggregate || []; for (const agg of aggregations) { - const funcRaw = String(agg.function || agg.func || '').toLowerCase(); - if (!['count', 'sum', 'avg', 'min', 'max'].includes(funcRaw)) { - throw new Error(`Unsupported aggregate function: ${funcRaw}`); - } + // [#5907] `funcWritten` is the caller's spelling — what the refusal quotes + // back and what the declared-vocabulary check is judged against; `funcRaw` + // is this transport's own normalised lookup key, unchanged. + const funcWritten = String(agg.function || agg.func || ''); + const funcRaw = funcWritten.toLowerCase(); + const sqlFunc = REMOTE_AGGREGATE_FUNCTIONS.get(funcRaw); + if (sqlFunc === undefined) refuseAggregateFunction(funcWritten); const field = agg.field || '*'; let fieldSql: string; if (field === '*') { @@ -733,9 +846,13 @@ export class RemoteTransport { this.assertSafeIdentifier(field); fieldSql = `"${field}"`; } + // The default alias keeps spelling itself with the normalised NAME + // (`count_stage`), unchanged; the emitted SQL uses the lowering table's + // value so that table is what decides the statement, not a membership + // check beside it. Identical text for all five entries today. const alias = agg.alias || `${funcRaw}_${field === '*' ? 'all' : field}`; this.assertSafeIdentifier(alias); - selectParts.push(`${funcRaw}(${fieldSql}) AS "${alias}"`); + selectParts.push(`${sqlFunc}(${fieldSql}) AS "${alias}"`); } if (selectParts.length === 0) selectParts.push('*'); From 1948bff47cbf952f345dcccef4d768c09f8abe83 Mon Sep 17 00:00:00 2001 From: claude Date: Fri, 7 Aug 2026 11:04:17 +0000 Subject: [PATCH 2/2] =?UTF-8?q?test(drivers):=20=E8=81=9A=E5=90=88?= =?UTF-8?q?=E6=8B=92=E6=94=B6=E7=94=A8=E4=BE=8B=E7=9A=84=20query=20options?= =?UTF-8?q?=20=E6=8C=89=E5=A5=91=E7=BA=A6=E7=BB=99=E7=B1=BB=E5=9E=8B,?= =?UTF-8?q?=E4=B8=8D=E5=86=8D=E6=93=A6=E6=88=90=20`any`=20(#5907)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `query-options-erasure` 棘轮(#4918)红:新增的两个测试文件里有 5 处 query options 调用点写成 `as any`,把测试面从 267 抬到 272。基线数字一个没动,5 处 逐一按「该给类型」还是「刻意越契约」重写: - 第 2 类(`count_distinct` / `array_agg` / `string_agg`)与全部控制组用例走 `declaredAst()`,`function` 形参类型是 `AggregationNode['function']` —— 于是 `tsc` 反过来证明这三个名字确实是已声明枚举的成员,正是第 2 类在运行时主张的 那件事;写错一个字母会在构建期红,而不是悄悄变成一条第 1 类输入却仍然让第 2 类断言「通过」。 - 第 1 类(`median` / 错误大小写)与「无 alias 默认列名」控制组按定义就构造不出 `QueryAST`,写 `as unknown as QueryAST`:命名了被绕过的契约、其余键仍受检、 可 grep 为有意为之,且不计入棘轮。 行为与断言一字未改。 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01WyvqvKMG6asi9aXjKE6xtx --- ...out-of-contract-aggregate-function.test.ts | 80 +++++++++++++----- ...ansport-aggregate-function-refusal.test.ts | 83 +++++++++++++++---- 2 files changed, 125 insertions(+), 38 deletions(-) diff --git a/packages/drivers/driver-sql/src/sql-driver-out-of-contract-aggregate-function.test.ts b/packages/drivers/driver-sql/src/sql-driver-out-of-contract-aggregate-function.test.ts index 121987c28a..79864c7746 100644 --- a/packages/drivers/driver-sql/src/sql-driver-out-of-contract-aggregate-function.test.ts +++ b/packages/drivers/driver-sql/src/sql-driver-out-of-contract-aggregate-function.test.ts @@ -61,12 +61,42 @@ import { describe, it, expect, beforeEach } from 'vitest'; import { SqlDriver } from './index.js'; import { AggregationFunction } from '@objectstack/spec/data'; +import type { AggregationNode, QueryAST } from '@objectstack/spec/data'; interface WireBearingError extends Error { code?: string; status?: number; } +/** + * [#4918] The two classes this file separates are ALSO two different type + * situations, and the query values are built to say so rather than erased to + * `any`: + * + * - {@link declaredAst} is on-contract. `function` is typed + * `AggregationNode['function']`, so `tsc` proves the class-2 fixtures + * (`count_distinct` / `array_agg` / `string_agg`) really are members of the + * declared enum — which is the exact claim class 2 makes at runtime. A typo + * there would fail the build instead of silently testing a class-1 input. + * - {@link undeclaredAst} is DELIBERATELY off-contract: `function: 'median'` + * cannot be a `QueryAST`, because that is the whole point of the test. It is + * spelled `as unknown as QueryAST` rather than `as any` — naming the contract + * being bypassed, keeping every other key checked, and greppable as an + * intentional act. + */ +const declaredAst = ( + fn: AggregationNode['function'], + field: string | null = 'stage', +): QueryAST => ({ + object: 'deal', + aggregations: [{ function: fn, ...(field ? { field } : {}), alias: 'n' }], +}); + +const undeclaredAst = (fn: string): QueryAST => ({ + object: 'deal', + aggregations: [{ function: fn, field: 'stage', alias: 'n' }], +}) as unknown as QueryAST; + /** * The first sentences, spelled out here rather than imported: this is the * contract #5240 asks for ("one condition, one wording"), and a test that read @@ -103,21 +133,22 @@ describe('[#5907] SqlDriver refuses an aggregate function it cannot compile', () await driver.create('deal', { id: '2', stage: 'lost', score: 20 }); }); - const aggregate = (fn: string, field: string | undefined = 'stage') => - driver.aggregate('deal', { - object: 'deal', - aggregations: [{ function: fn, ...(field ? { field } : {}), alias: 'n' }], - } as any); - - const refusalOf = async (fn: string): Promise => { + const refusalOfAst = async (fn: string, ast: QueryAST): Promise => { try { - await aggregate(fn); + await driver.aggregate('deal', ast); } catch (e) { return e as WireBearingError; } throw new Error(`expected the driver to refuse "${fn}", but it resolved`); }; + /** Class 1's inputs: off-contract by construction — see {@link undeclaredAst}. */ + const refusalOfUndeclared = (fn: string) => refusalOfAst(fn, undeclaredAst(fn)); + + /** Class 2's inputs: declared names, so the fixture is a real `QueryAST`. */ + const refusalOfDeclared = (fn: AggregationNode['function']) => + refusalOfAst(fn, declaredAst(fn)); + // ── Class 1: the Query Protocol does not declare this name ───────────────── describe('a function name the Query Protocol never declared', () => { @@ -127,7 +158,7 @@ describe('[#5907] SqlDriver refuses an aggregate function it cannot compile', () for (const fn of UNDECLARED) { it(`refuses "${fn}" with INVALID_QUERY / 400`, async () => { - const err = await refusalOf(fn); + const err = await refusalOfUndeclared(fn); expect(err.code).toBe('INVALID_QUERY'); expect(err.status).toBe(400); expect(err.message.startsWith(UNDECLARED_SENTENCE(fn))).toBe(true); @@ -151,7 +182,7 @@ describe('[#5907] SqlDriver refuses an aggregate function it cannot compile', () const MISCASED = ['COUNT_DISTINCT', 'Median', 'COUNT']; for (const fn of MISCASED) { it(`refuses the miscased "${fn}" as UNDECLARED (400), not as a capability gap`, async () => { - const err = await refusalOf(fn); + const err = await refusalOfUndeclared(fn); expect(err.code).toBe('INVALID_QUERY'); expect(err.status).toBe(400); expect(err.message.startsWith(UNDECLARED_SENTENCE(fn))).toBe(true); @@ -165,7 +196,15 @@ describe('[#5907] SqlDriver refuses an aggregate function it cannot compile', () describe('a DECLARED function this backend cannot compile', () => { // Exactly the three `AggregationFunction` declares with no SQL lowering. - const UNCOMPILABLE = ['count_distinct', 'array_agg', 'string_agg']; + // The TYPE is load-bearing (#4918): `AggregationNode['function']` is the + // declared enum, so a typo in this fixture — or a name that leaves the enum + // when #6188 is decided — fails `tsc` instead of quietly becoming a class-1 + // input that still passes a class-2 assertion for the wrong reason. + const UNCOMPILABLE: Array = [ + 'count_distinct', + 'array_agg', + 'string_agg', + ]; // Guard: the fixture is the real declared-minus-compiled set, derived rather // than trusted. If the spec drops one (that decision is #6188) or this driver @@ -179,7 +218,7 @@ describe('[#5907] SqlDriver refuses an aggregate function it cannot compile', () for (const fn of UNCOMPILABLE) { it(`refuses "${fn}" with NOT_IMPLEMENTED / 501`, async () => { - const err = await refusalOf(fn); + const err = await refusalOfDeclared(fn); expect(err.code).toBe('NOT_IMPLEMENTED'); expect(err.status).toBe(501); expect(err.message.startsWith(UNCOMPILABLE_SENTENCE(fn))).toBe(true); @@ -199,23 +238,24 @@ describe('[#5907] SqlDriver refuses an aggregate function it cannot compile', () describe('the compiled vocabulary is untouched', () => { it('every function this driver lowers still computes its value', async () => { - expect(await aggregate('count', 'id')).toEqual([{ n: 2 }]); - expect(await aggregate('sum', 'score')).toEqual([{ n: 30 }]); - expect(await aggregate('avg', 'score')).toEqual([{ n: 15 }]); - expect(await aggregate('min', 'score')).toEqual([{ n: 10 }]); - expect(await aggregate('max', 'score')).toEqual([{ n: 20 }]); + expect(await driver.aggregate('deal', declaredAst('count', 'id'))).toEqual([{ n: 2 }]); + expect(await driver.aggregate('deal', declaredAst('sum', 'score'))).toEqual([{ n: 30 }]); + expect(await driver.aggregate('deal', declaredAst('avg', 'score'))).toEqual([{ n: 15 }]); + expect(await driver.aggregate('deal', declaredAst('min', 'score'))).toEqual([{ n: 10 }]); + expect(await driver.aggregate('deal', declaredAst('max', 'score'))).toEqual([{ n: 20 }]); }); it('COUNT(*) — the `field`-less spelling the spec allows — still answers', async () => { - expect(await aggregate('count', undefined)).toEqual([{ n: 2 }]); + expect(await driver.aggregate('deal', declaredAst('count', null))).toEqual([{ n: 2 }]); }); it('grouped aggregation still answers', async () => { - const rows = await driver.aggregate('deal', { + const ast: QueryAST = { object: 'deal', groupBy: ['stage'], aggregations: [{ function: 'count', field: 'id', alias: 'n' }], - } as any); + }; + const rows = await driver.aggregate('deal', ast); expect((rows as any[]).map((r) => `${r.stage}:${r.n}`).sort()).toEqual(['lost:1', 'won:1']); }); }); diff --git a/packages/drivers/driver-turso/src/remote-transport-aggregate-function-refusal.test.ts b/packages/drivers/driver-turso/src/remote-transport-aggregate-function-refusal.test.ts index 0df1cf623c..5271f3acad 100644 --- a/packages/drivers/driver-turso/src/remote-transport-aggregate-function-refusal.test.ts +++ b/packages/drivers/driver-turso/src/remote-transport-aggregate-function-refusal.test.ts @@ -63,6 +63,7 @@ import { describe, it, expect, vi } from 'vitest'; import { SqlDriver } from '@objectstack/driver-sql'; import { RemoteTransport } from './remote-transport.js'; import { AggregationFunction } from '@objectstack/spec/data'; +import type { AggregationNode, QueryAST } from '@objectstack/spec/data'; interface WireBearingError extends Error { code?: string; @@ -75,6 +76,35 @@ const UNDECLARED_SENTENCE = (f: string) => const UNCOMPILABLE_SENTENCE = (f: string) => `Aggregate function "${f}" is declared but not implemented by this backend.`; +/** + * [#4918] The twins of the query builders in + * `driver-sql`'s `sql-driver-out-of-contract-aggregate-function.test.ts`, and + * for the same reason: class 2's fixtures are typed against the declared enum so + * `tsc` proves they ARE declared, while class 1's are off-contract by + * construction and say so with `as unknown as QueryAST` — the contract being + * bypassed is named, every other key stays checked. + */ +const declaredAst = (fn: AggregationNode['function']): QueryAST => ({ + object: 'deal', + aggregations: [{ function: fn, field: 'stage', alias: 'n' }], +}); + +const undeclaredAst = (fn: string): QueryAST => ({ + object: 'deal', + aggregations: [{ function: fn, field: 'stage', alias: 'n' }], +}) as unknown as QueryAST; + +/** + * The one control that is off-contract on TWO axes at once — a miscased name + * and no `alias` (which `AggregationNodeSchema` requires) — because what it pins + * is precisely how this transport names a result column when the caller gave it + * nothing to work with. Same `as unknown as QueryAST` discipline. + */ +const aliaslessAst = (fn: string): QueryAST => ({ + object: 'deal', + aggregations: [{ function: fn, field: 'stage' }], +}) as unknown as QueryAST; + function transportWithCapturingClient() { const calls: Array<{ sql: string; args: any[] }> = []; const client = { @@ -89,13 +119,10 @@ function transportWithCapturingClient() { return { t, calls }; } -const aggregateOn = (t: RemoteTransport, fn: string) => - t.aggregate('deal', { aggregations: [{ function: fn, field: 'stage', alias: 'n' }] } as any); - -async function refusalOf(fn: string): Promise { +async function refusalOfAst(fn: string, ast: QueryAST): Promise { const { t, calls } = transportWithCapturingClient(); try { - await aggregateOn(t, fn); + await t.aggregate('deal', ast); } catch (e) { // A refused aggregation must not have reached the database on its way to // throwing — a statement that ran is a statement that cost a round trip and @@ -106,13 +133,19 @@ async function refusalOf(fn: string): Promise { throw new Error(`expected the transport to refuse "${fn}", but it compiled to ${JSON.stringify(calls)}`); } +/** Class 1's inputs: off-contract by construction — see {@link undeclaredAst}. */ +const refusalOfUndeclared = (fn: string) => refusalOfAst(fn, undeclaredAst(fn)); + +/** Class 2's inputs: declared names, so the fixture is a real `QueryAST`. */ +const refusalOfDeclared = (fn: AggregationNode['function']) => refusalOfAst(fn, declaredAst(fn)); + describe('[#5907] RemoteTransport refuses an aggregate function it cannot compile', () => { describe('a function name the Query Protocol never declared', () => { const UNDECLARED = ['median', 'stddev', 'percentile_cont', 'group_concat']; for (const fn of UNDECLARED) { it(`refuses "${fn}" with INVALID_QUERY / 400`, async () => { - const err = await refusalOf(fn); + const err = await refusalOfUndeclared(fn); expect(err.code).toBe('INVALID_QUERY'); expect(err.status).toBe(400); expect(err.message.startsWith(UNDECLARED_SENTENCE(fn))).toBe(true); @@ -132,7 +165,7 @@ describe('[#5907] RemoteTransport refuses an aggregate function it cannot compil // and worded on the caller's own bytes, so `COUNT_DISTINCT` is undeclared // here exactly as it is on the local driver — the two faces agree on the // class instead of splitting 400/501 over a normalisation difference. - const err = await refusalOf('COUNT_DISTINCT'); + const err = await refusalOfUndeclared('COUNT_DISTINCT'); expect(err.code).toBe('INVALID_QUERY'); expect(err.status).toBe(400); expect(err.message).toContain('"COUNT_DISTINCT"'); @@ -141,7 +174,12 @@ describe('[#5907] RemoteTransport refuses an aggregate function it cannot compil }); describe('a DECLARED function this backend cannot compile', () => { - const UNCOMPILABLE = ['count_distinct', 'array_agg', 'string_agg']; + // Typed against the declared enum on purpose — see the twin's note (#4918). + const UNCOMPILABLE: Array = [ + 'count_distinct', + 'array_agg', + 'string_agg', + ]; it('the fixture is exactly the declared-but-uncompiled set', () => { const compiled = ['count', 'sum', 'avg', 'min', 'max']; @@ -151,7 +189,7 @@ describe('[#5907] RemoteTransport refuses an aggregate function it cannot compil for (const fn of UNCOMPILABLE) { it(`refuses "${fn}" with NOT_IMPLEMENTED / 501`, async () => { - const err = await refusalOf(fn); + const err = await refusalOfDeclared(fn); expect(err.code).toBe('NOT_IMPLEMENTED'); expect(err.status).toBe(501); expect(err.message.startsWith(UNCOMPILABLE_SENTENCE(fn))).toBe(true); @@ -166,7 +204,7 @@ describe('[#5907] RemoteTransport refuses an aggregate function it cannot compil // ── The cross-package half: one condition, one wording ───────────────────── describe('local/remote parity (#5240 — one condition, one wording)', () => { - const localRefusalOf = async (fn: string): Promise => { + const localRefusalOf = async (fn: string, ast: QueryAST): Promise => { const d = new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, @@ -176,7 +214,7 @@ describe('[#5907] RemoteTransport refuses an aggregate function it cannot compil { name: 'deal', fields: { id: { type: 'text', name: 'id' }, stage: { type: 'text', name: 'stage' } } } as any, ]); try { - await d.aggregate('deal', { aggregations: [{ function: fn, field: 'stage', alias: 'n' }] } as any); + await d.aggregate('deal', ast); } catch (e) { return e as WireBearingError; } @@ -186,10 +224,19 @@ describe('[#5907] RemoteTransport refuses an aggregate function it cannot compil // Compared as RUNTIME messages from the two packages, not as two copies of a // literal — a shared constant would agree with itself no matter how far the // two faces drifted. This is what makes "首句逐字一致" checkable. - for (const fn of ['median', 'count_distinct', 'array_agg', 'string_agg']) { + // One entry per class, each carrying the query value its class is entitled + // to: `median` cannot be a `QueryAST` (that is what class 1 means), the three + // declared names can and are. + const PARITY: Array<[fn: string, ast: QueryAST]> = [ + ['median', undeclaredAst('median')], + ['count_distinct', declaredAst('count_distinct')], + ['array_agg', declaredAst('array_agg')], + ['string_agg', declaredAst('string_agg')], + ]; + for (const [fn, ast] of PARITY) { it(`"${fn}" is answered identically by the local driver and this transport`, async () => { - const remote = await refusalOf(fn); - const local = await localRefusalOf(fn); + const remote = await refusalOfAst(fn, ast); + const local = await localRefusalOf(fn, ast); expect(remote.code).toBe(local.code); expect(remote.status).toBe(local.status); // The first sentence is the contract; the tails happen to coincide too @@ -205,16 +252,16 @@ describe('[#5907] RemoteTransport refuses an aggregate function it cannot compil describe('the compiled vocabulary is untouched', () => { it('emits byte-identical SQL for the five functions it lowers', async () => { - for (const fn of ['count', 'sum', 'avg', 'min', 'max']) { + for (const fn of ['count', 'sum', 'avg', 'min', 'max'] as const) { const { t, calls } = transportWithCapturingClient(); - await aggregateOn(t, fn); + await t.aggregate('deal', declaredAst(fn)); expect(calls.map((c) => c.sql)).toEqual([`SELECT ${fn}("stage") AS "n" FROM "deal"`]); } }); it('the default alias still spells itself with the NORMALISED name', async () => { const { t, calls } = transportWithCapturingClient(); - await t.aggregate('deal', { aggregations: [{ function: 'COUNT', field: 'stage' }] } as any); + await t.aggregate('deal', aliaslessAst('COUNT')); expect(calls[0].sql).toBe('SELECT count("stage") AS "count_stage" FROM "deal"'); }); @@ -237,7 +284,7 @@ describe('[#5907] RemoteTransport refuses an aggregate function it cannot compil */ it('[filed, not fixed] `COUNT` still compiles HERE while the local driver refuses it', async () => { const { t, calls } = transportWithCapturingClient(); - await aggregateOn(t, 'COUNT'); + await t.aggregate('deal', undeclaredAst('COUNT')); expect(calls.map((c) => c.sql)).toEqual(['SELECT count("stage") AS "n" FROM "deal"']); }); });