From f3161a37321d8da52984572aab4bef49d3706a9c Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 13:36:28 +0000 Subject: [PATCH] fix(driver-turso)!: remote stops lowercasing aggregate function names (#6203) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `TursoDriver` picks its face from `url`: a local/replica URL inherits `SqlDriver`, a remote one delegates to `RemoteTransport`. The two faces normalised the aggregate function name differently — the remote transport lowercased it before its lowering-table lookup, the local driver looked up whatever it was handed — so one driver answered one query two ways, decided by a connection string. Measured on origin/main @ d367f03d6: COUNT REMOTE -> RESOLVED "SELECT count(\"stage\") AS \"n\" FROM \"deal\"" LOCAL -> THREW INVALID_QUERY / 400 Count REMOTE -> RESOLVED (same) LOCAL -> THREW INVALID_QUERY / 400 Delete the `.toLowerCase()`. `AggregationFunction` is a case-sensitive `z.enum` (`AggregationFunction.parse('COUNT')` throws), so `COUNT` is a spelling the Query Protocol never declared: what the remote transport accepted was a private dialect, and PD#12 rejects widening a consumer to keep one alive. Teaching the local driver the same dialect would have fossilised it into a second de-facto contract instead. No new classification logic was needed. #5907 (PR #6204) already classifies on the CALLER's spelling, so with the lookup no longer normalising, `COUNT` falls straight onto class 1 — INVALID_QUERY / 400, "not a declared aggregate function" — the same envelope, in the same words, that the local driver has been giving it. The default alias is unchanged for every input that still compiles: only a name already in the lowering table reaches that line, and every key there is lowercase. Tests: the `[filed, not fixed]` control #5907 left here recording `COUNT` compiling on this face is flipped into a four-case local/remote parity block asserting code AND status AND first sentence (#6144), plus a control that the declared lowercase spelling still compiles on both faces. The aliasless fixture drops its miscased axis, which stopped being expressible. Reverse verification, direction predicted before running and recorded in the test docblock because the four cases do NOT move together: with the `toLowerCase()` restored, `COUNT`/`Count` go red on "but it compiled" (2/4), while `COUNT_DISTINCT`/`Median` stay green — lowercasing them still misses the lowering table and #5907 already judged them on the caller's spelling. Measured: exactly that, 2 failed / 806 passed. Both are kept: the insensitive pair is the standing guard on #5907's half of the same fork. driver-sql carries a comment-only correction — a docblock there described the remote transport's lowercasing as current fact. Fixes #6203 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01WyvqvKMG6asi9aXjKE6xtx --- .../turso-aggregate-case-normalization.md | 31 +++ packages/drivers/driver-sql/src/sql-driver.ts | 12 +- ...ansport-aggregate-function-refusal.test.ts | 208 ++++++++++++++---- .../driver-turso/src/remote-transport.ts | 63 ++++-- 4 files changed, 240 insertions(+), 74 deletions(-) create mode 100644 .changeset/turso-aggregate-case-normalization.md diff --git a/.changeset/turso-aggregate-case-normalization.md b/.changeset/turso-aggregate-case-normalization.md new file mode 100644 index 0000000000..4a9d36cc9c --- /dev/null +++ b/.changeset/turso-aggregate-case-normalization.md @@ -0,0 +1,31 @@ +--- +'@objectstack/driver-turso': patch +--- + +drivers(turso): remote 聚合函数名不再大小写归一化,两面只认协议声明的小写拼写 (#6203) + +`TursoDriver` 按连接串 `url` 选面:本地/副本继承 `SqlDriver`,远程委派 `RemoteTransport`。 +两面此前对聚合函数名的归一化不一致 —— remote 先 `.toLowerCase()` 再查自己的编译表,local +拿到什么查什么。于是同一个驱动、同一条查询,答案取决于连接串: + +``` +COUNT REMOTE -> RESOLVED "SELECT count(\"stage\") AS \"n\" FROM \"deal\"" + LOCAL -> THREW INVALID_QUERY / 400 +``` + +本次删掉 remote 侧的 `.toLowerCase()`。`AggregationFunction` 是**大小写敏感**的 `z.enum` +(`AggregationFunction.parse('COUNT')` 直接抛错),`COUNT` 是协议从未声明的拼写,remote +多认的是一种私有方言;按契约优先(PD#12)收紧消费端,而不是把方言固化成第二套事实契约。 + +**升级说明(user-visible)**:remote 连接不再接受大写或混合大小写的聚合函数名。 +`COUNT` / `Count` / `SUM` 等此前在 remote 能编出 SQL 的拼写,现在与 local 一样统一落 +`INVALID_QUERY` / 400(「不是已声明的聚合函数」)。**作者侧修法是改用小写** —— 把 +`aggregations[].function` 写成协议声明的 `count` / `sum` / `avg` / `min` / `max` +(以及已声明但本后端未实现的 `count_distinct` / `array_agg` / `string_agg`)。 + +经 REST/协议门进来的查询不受影响:大写拼写在 `AggregationNodeSchema` 就被拒,到不了驱动; +仓内亦无任何发送大写拼写的调用方。受影响的只有绕过 spec 校验、直接调用远程驱动且依赖该 +归一化的进程内调用方。 + +`#5907` 落地的拒收信封(第 1 类 `INVALID_QUERY`/400、第 2 类 `NOT_IMPLEMENTED`/501、 +按调用方原始拼写分类)与默认 alias 的拼法均未改动。 diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index 885b8b4872..0babd8979e 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -541,10 +541,14 @@ const DECLARED_AGGREGATE_FUNCTIONS: readonly string[] = AggregationFunction.opti * * 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. + * implemented" for it would be false. When this was written the remote transport + * still lowercased the name before its own lookup while this driver read it raw, + * so classifying on each face's post-normalisation name would have handed + * `COUNT_DISTINCT` a 400 here and a 501 there for one query. #6203 has since + * removed that lowercasing, so the two faces now normalise alike — but the + * case-sensitive judgement here is not merely a survivor of that fork: the enum + * IS case-sensitive (`AggregationFunction.parse('COUNT')` throws), so this is + * what "declared" means, whatever any transport does before asking. */ function undeclaredAggregateFunctionError(func: string): Error { const err = new Error( 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 5271f3acad..2df61256a2 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 @@ -57,6 +57,30 @@ * "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. + * + * --- + * + * # [#6203] The other half of the same fork: which names COMPILE + * + * #5907 made the two faces agree on how a refusal is SPELLED. It deliberately + * did not touch which names each face compiles, and recorded the residue here as + * a `[filed, not fixed]` control: this transport lowercased the function name + * before its lookup and the local driver did not, so `COUNT` compiled here and + * was refused there. Measured on `origin/main` @ `d367f03d6`: + * + * ``` + * COUNT REMOTE -> RESOLVED "SELECT count(\"stage\") AS \"n\" FROM \"deal\"" + * LOCAL -> THREW INVALID_QUERY/400 "…\"COUNT\" is not a declared aggregate function" + * Count REMOTE -> RESOLVED (same) LOCAL -> THREW INVALID_QUERY/400 + * ``` + * + * #6203 closes it by DELETING the `toLowerCase()` — the contract-first + * direction of the two available. `AggregationFunction` is a case-sensitive + * `z.enum`, so `COUNT` is a spelling the Query Protocol never declared and what + * this transport accepted was a private dialect; teaching the local driver the + * same dialect instead would have fossilised it into a second de-facto contract + * (PD#12). The control is flipped into the `[#6203]` block at the bottom, whose + * own docblock carries the per-case reverse-verification prediction. */ import { describe, it, expect, vi } from 'vitest'; @@ -95,10 +119,13 @@ const undeclaredAst = (fn: string): QueryAST => ({ }) 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. + * The control fixture for the default alias: off-contract on the one axis it + * needs (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. + * + * [#6203] It used to be off-contract on a SECOND axis — the name was miscased — + * which stopped being expressible the moment a miscased name became a refusal. */ const aliaslessAst = (fn: string): QueryAST => ({ object: 'deal', @@ -139,6 +166,28 @@ 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)); +/** + * The same query put to the OTHER face of `TursoDriver` — the local/replica one, + * which inherits `SqlDriver`. Module-scoped because two blocks below need it: + * #5240's wording parity and #6203's case parity. + */ +async function localRefusalOf(fn: string, ast: QueryAST): 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', ast); + } catch (e) { + return e as WireBearingError; + } + throw new Error(`expected the local driver to refuse "${fn}", but it resolved`); +} + 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']; @@ -160,11 +209,15 @@ describe('[#5907] RemoteTransport refuses an aggregate function it cannot compil }); } - 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. + it('quotes the spelling the CALLER wrote, not a normalised one', async () => { + // 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. #5907 established this while this transport + // still lowercased for its LOOKUP; #6203 removed that lookup + // normalisation, so the guard is now double-locked rather than moot: it + // still fails on a `toLowerCase()` reintroduced anywhere between the + // caller's value and the message. const err = await refusalOfUndeclared('COUNT_DISTINCT'); expect(err.code).toBe('INVALID_QUERY'); expect(err.status).toBe(400); @@ -204,23 +257,6 @@ 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, ast: QueryAST): 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', ast); - } 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. @@ -259,33 +295,111 @@ describe('[#5907] RemoteTransport refuses an aggregate function it cannot compil } }); - it('the default alias still spells itself with the NORMALISED name', async () => { + /** + * [#6203] Was `aliaslessAst('COUNT')` — off-contract on two axes at once, a + * miscased name AND no `alias`. The miscased half is now refused, so the + * fixture drops to the one axis it was actually pinning: how this transport + * NAMES a result column when the caller gave it nothing to work with. The + * expected SQL is unchanged, and that is the point of the case — the default + * alias used to be built from the lowercased name and is now built from the + * caller's own, which is the same string for every input that still + * compiles, because every key in the lowering table is lowercase. + */ + it('the default alias still spells itself from the function name', async () => { const { t, calls } = transportWithCapturingClient(); - await t.aggregate('deal', aliaslessAst('COUNT')); + await t.aggregate('deal', aliaslessAst('count')); 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 () => { + // ── #6203: one spelling, both faces ─────────────────────────────────────── + + /** + * [#6203] A miscased function name is refused by BOTH faces, with one wire + * identity — the half of the local/remote fork #5907 left open. + * + * This block replaces the `[filed, not fixed]` control that #5907's PR left + * here recording `COUNT` compiling on this transport. That case pinned exactly + * the limb this issue deletes, so re-spelling it was not an option: it is + * flipped, not adjusted. + * + * # Reverse verification — direction predicted BEFORE it was run + * + * Restore `REMOTE_AGGREGATE_FUNCTIONS.get(func.toLowerCase())` and the four + * cases below do NOT move together. Predicted, per case: + * + * - `COUNT`, `Count` — RED, and not on a comparison: the transport RESOLVES, + * so `refusalOfAst` throws its own "but it compiled to […]" error. These two + * are the cases this issue exists for; a name that differs from a compiled + * one only by case is the entire population the `toLowerCase()` moved. + * - `COUNT_DISTINCT`, `Median` — GREEN, unchanged. Lowercasing them still + * misses the lowering table (`count_distinct`/`median` are not in it), and + * #5907 already classifies on the CALLER's spelling, so both faces answered + * `INVALID_QUERY`/400 with identical text before this change too. + * + * Measured after writing the above: exactly that — 2 of 4 red on the revert. + * They are kept as one family regardless, because what the family asserts is + * "no miscased spelling gets a different answer from the two faces", and the + * two insensitive members are the standing guard on #5907's half of it: they + * go red the day a `toLowerCase()` reappears in the CLASSIFICATION rather than + * in the lookup, which the sensitive pair cannot see. + */ + describe('[#6203] a miscased name gets ONE answer, not one per connection string', () => { + // Every spelling here is off-contract by construction: `AggregationFunction` + // is a case-sensitive `z.enum` (`AggregationFunction.parse('COUNT')` throws, + // pinned in `packages/spec/src/data/query.test.ts`), so none of these can be + // a `QueryAST` — hence `undeclaredAst`'s `as unknown as QueryAST`. + const MISCASED = [ + 'COUNT', // differs from a COMPILED name only by case + 'Count', // …and in mixed case + 'COUNT_DISTINCT', // differs from a DECLARED-but-uncompiled name only by case + 'Median', // differs from an undeclared name only by case + ]; + + for (const fn of MISCASED) { + it(`"${fn}" is refused by both faces with the same code, status and wording`, async () => { + const remote = await refusalOfUndeclared(fn); + const local = await localRefusalOf(fn, undeclaredAst(fn)); + + // #6144: `code` AND `status`, never "it threw" — the local face already + // threw for all four before this change, so a bare `rejects.toThrow()` + // would have been green throughout and blind to the whole defect. + expect(remote.code).toBe('INVALID_QUERY'); + expect(remote.status).toBe(400); + expect(local.code).toBe(remote.code); + expect(local.status).toBe(remote.status); + + // Class 1, on both faces: a spelling the protocol never declared is a + // query no backend can run, not a gap in this one — so never the 501. + expect(remote.message.startsWith(UNDECLARED_SENTENCE(fn))).toBe(true); + expect(remote.message.split('. ')[0]).toBe(local.message.split('. ')[0]); + expect(remote.message).toBe(local.message); + expect(remote.message).not.toContain('capability gap'); + + // The refusal quotes the caller's own bytes — nothing normalised one of + // them into a name the caller never wrote. + expect(remote.message).toContain(`"${fn}"`); + expect(remote.message).not.toContain(`"${fn.toLowerCase()}"`); + }); + } + + it('the lowercase spelling the protocol DOES declare still compiles on both faces', async () => { + // The control that keeps the above from being satisfiable by refusing + // everything: the fork is closed by narrowing what this transport accepts, + // not by breaking the vocabulary both faces share. const { t, calls } = transportWithCapturingClient(); - await t.aggregate('deal', undeclaredAst('COUNT')); + await t.aggregate('deal', declaredAst('count')); expect(calls.map((c) => c.sql)).toEqual(['SELECT count("stage") AS "n" FROM "deal"']); + + 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, + ]); + await expect(d.aggregate('deal', declaredAst('count'))).resolves.toEqual([{ n: 0 }]); }); }); }); diff --git a/packages/drivers/driver-turso/src/remote-transport.ts b/packages/drivers/driver-turso/src/remote-transport.ts index c940fb383d..374144df0e 100644 --- a/packages/drivers/driver-turso/src/remote-transport.ts +++ b/packages/drivers/driver-turso/src/remote-transport.ts @@ -591,21 +591,28 @@ function uncompilableAggregateFunctionError(func: string): Error { * [#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: + * `func` is the name the CALLER wrote — which, since #6203, is also the name + * this transport looked up. Judging the caller's own spelling against the + * case-sensitive enum is what keeps a miscased name from splitting the envelope: + * on a 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. + * + * [#6203] The other half of that fork — the LOOKUP being case-insensitive here + * and case-sensitive on the local driver — is now closed too, in this direction: + * the `toLowerCase()` in {@link RemoteTransport.aggregate} is gone. Measured on + * `origin/main` @ `d367f03d6`, before that removal: * * ``` - * COUNT REMOTE -> RESOLVED ("SELECT count(...)") LOCAL -> threw - * COUNT_DISTINCT REMOTE -> threw "…: count_distinct" LOCAL -> threw "…: COUNT_DISTINCT" + * COUNT REMOTE -> RESOLVED "SELECT count(\"stage\") …" LOCAL -> THREW INVALID_QUERY/400 + * Count REMOTE -> RESOLVED "SELECT count(\"stage\") …" LOCAL -> THREW INVALID_QUERY/400 * ``` * - * 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. + * `AggregationFunction` is a CASE-SENSITIVE `z.enum` and `COUNT` is a spelling + * the Query Protocol never declared, so what this transport used to accept was a + * dialect of its own — a lenient consumer, which PD#12 rejects. Both faces now + * read only the declared lowercase spelling, and `COUNT` lands on class 1 above + * on both. */ function refuseAggregateFunction(func: string): never { throw DECLARED_AGGREGATE_FUNCTIONS.includes(func) @@ -831,13 +838,20 @@ export class RemoteTransport { const aggregations = query?.aggregations || query?.aggregate || []; for (const agg of aggregations) { - // [#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); + // [#5907] The caller's spelling is what the refusal quotes back and what + // the declared-vocabulary check is judged against. + // + // [#6203] It is now also the LOOKUP KEY. This read used to be + // `funcWritten.toLowerCase()`, which made `COUNT` compile here while the + // local driver — same `TursoDriver`, different `url` — refused it: one + // query, two answers, decided by a connection string. `AggregationFunction` + // is a case-sensitive `z.enum`, so the lowercased read was accepting a + // 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 || ''); + const sqlFunc = REMOTE_AGGREGATE_FUNCTIONS.get(func); + if (sqlFunc === undefined) refuseAggregateFunction(func); const field = agg.field || '*'; let fieldSql: string; if (field === '*') { @@ -846,11 +860,14 @@ 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}`; + // The default alias spells itself with the function NAME (`count_stage`) + // while 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. Unchanged by #6203: only a name already + // in the table reaches this line, and every key there is lowercase, so the + // alias this produces is byte-identical to the pre-#6203 one for every + // input that still compiles. + const alias = agg.alias || `${func}_${field === '*' ? 'all' : field}`; this.assertSafeIdentifier(alias); selectParts.push(`${sqlFunc}(${fieldSql}) AS "${alias}"`); }