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}"`); }