From d5485d3c0115cf56f7f690fd43498459ed282faa Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 07:32:01 +0000 Subject: [PATCH 1/4] fix(driver-turso): escape the groupBy alias on the remote transport instead of gating it (#14235) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The second output-name position of `RemoteTransport.aggregate`. #14113 moved the aggregation alias to `aliasIdentifierSql` and left this one gated because it carried a landed #6401 pin asserting the refusal; #13714 had already routed both of driver-sql's output-name positions through escaping. An output-column key is a NAME — quoted and escaped; a column reference is grammar — validated. So `field` keeps `assertSafeIdentifier` and `outKey` no longer has it. The #6401 pin is rewritten in place on the same input to assert what the transport now emits, plus dotted/spaced round-trip cases and two executing cases against the SQLite-backed libsql stub. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0112hMx9hjJ9BgB28X97DS68 --- .changeset/turso-groupby-alias-escaped.md | 35 ++++ .../src/remote-transport-groupby-node.test.ts | 182 ++++++++++++++++-- .../driver-turso/src/remote-transport.ts | 33 +++- 3 files changed, 226 insertions(+), 24 deletions(-) create mode 100644 .changeset/turso-groupby-alias-escaped.md diff --git a/.changeset/turso-groupby-alias-escaped.md b/.changeset/turso-groupby-alias-escaped.md new file mode 100644 index 0000000000..3dce8fad1f --- /dev/null +++ b/.changeset/turso-groupby-alias-escaped.md @@ -0,0 +1,35 @@ +--- +"@objectstack/driver-turso": patch +--- + +fix(driver-turso): escape the groupBy alias on the remote transport instead of gating it (#14235) + +`RemoteTransport.aggregate` emits a caller-supplied output NAME in exactly two +positions. #14113 moved the aggregation alias to escaping and deliberately left +the groupBy alias (`GroupByNodeSchema.alias`, reaching the driver as +`g.alias ?? g.field`) on `assertSafeIdentifier`, because that position carried a +landed pin asserting the refusal. So a groupBy alias that was not a bare +`[A-Za-z_][A-Za-z0-9_]*` — `'Region Name'`, `'deal.stage_bucket'` — was refused +on this face while the in-memory, MongoDB and (post-#13714) SQL faces all +project it verbatim: one query, two answers, decided by a connection string. + +The groupBy select site now emits `"" AS `, +the same quote-doubling escape the aggregation alias beside it already uses, so +both output-name positions of the method agree with `driver-sql`. The `field` +position keeps `assertSafeIdentifier` — a column REFERENCE is grammar and a +qualified one is legitimate, so it must be validated; an output NAME is one +name by definition and is quoted and escaped. `outKey === field` still emits the +alias-less `""`, byte-identical to before. + +The #6401 pin that asserted the refusal is rewritten in place, on the same +input, to assert what the transport now emits — the recorded, non-silent +reversal the card asked for rather than a rider on someone else's change. The +escaped alias is pinned against a real SQLite-backed libsql stub as well as on +the captured statement, because only executing it tells "escaped" apart from +"broke out". + +No accept set moves at the contract: `GroupByNodeSchema.alias` already declares +this key and the spec already admits these names. What moves is this driver's +accept set, toward the contract the other three faces already implement — +declared = enforced, restored. The refusal envelope for the positions that stay +gated (#14287, `INVALID_REQUEST` / 400) is untouched. diff --git a/packages/drivers/driver-turso/src/remote-transport-groupby-node.test.ts b/packages/drivers/driver-turso/src/remote-transport-groupby-node.test.ts index 83d0d9585f..df27aff1dd 100644 --- a/packages/drivers/driver-turso/src/remote-transport-groupby-node.test.ts +++ b/packages/drivers/driver-turso/src/remote-transport-groupby-node.test.ts @@ -63,7 +63,51 @@ * reversed — the condition it named ("only one face would read it") is what * stopped being true. * - * # Reverse verification — direction predicted BEFORE it was run, per case + * # `alias` is ESCAPED, not gated — as of #14235 + * + * `RemoteTransport.aggregate` emits a caller-supplied output NAME in exactly + * two positions. #13714 routed BOTH of `driver-sql`'s through + * `SqlDriver.aliasIdentifierSql`; #14113 moved this transport's AGGREGATION + * alias to the same escaping and deliberately left the groupBy alias alone, + * because the groupBy position carried the landed #6401 pin below asserting the + * refusal. #14235 is the card that reverses that pin on the record: an + * output-column key is a NAME — quoted and escaped — and a column REFERENCE is + * grammar, so `field` keeps `assertSafeIdentifier` and `outKey` no longer has + * it. `GroupByNodeSchema.alias` is the same class of key as + * `AggregationNodeSchema.alias`, and the in-memory face projects + * `g.alias ?? g.field` verbatim, so this face was the last one refusing names + * the contract permits — with a bare `Error` before #14287, an opaque 500 after + * `mapDataError`, and a 400 `INVALID_REQUEST` since. + * + * ⚠️ The `[#6401] refuses an unsafe identifier in \`alias\`` case named in the + * #6212 record below **no longer exists** — it is replaced in place by + * `[#14235] ESCAPES an alias that would close the quoting`, on the same input. + * The record is left as it was measured rather than rewritten to match today's + * cases: it is the #6212 ablation, not this one. + * + * ## Reverse verification (#14235) — direction predicted BEFORE it was run + * + * Restore the two pre-#14235 lines at the groupBy select site — + * `this.assertSafeIdentifier(outKey)` above the push, and + * `` `"${field}" AS "${outKey}"` `` as the aliased emission: + * + * - the two capture cases (`ESCAPES an alias that would close the quoting`, + * `a dotted or spaced alias round-trips`) go RED by THROWING inside the call + * — `unsafe identifier rejected: "bucket"; DROP TABLE deal; --"` / + * `"Region Name"` — not on a comparison. + * - both executing cases go RED the same way, inside `driver.aggregate`. + * - the `field`-position control (`still refuses an unsafe identifier inside a + * structured entry`) stays GREEN — untouched by this change, and that is + * exactly what it is here to hold. + * - `projects \`alias\` as the column name` and `an alias equal to the field + * name emits no self-rename` stay GREEN: `bucket` and `stage` pass + * `SAFE_IDENTIFIER` either way, so they pin byte-identical emission across + * the change. + * - every date-bucket, parity and string-form case stays GREEN. + * + * MEASURED: see the PR body for the run. + * + * # Reverse verification (#6212) — direction predicted BEFORE it was run, per case * * Restore `const groupBy: string[] = Array.isArray(query?.groupBy) ? … : []` * (with a cast, since the narrowed signature no longer permits it): @@ -115,9 +159,11 @@ * rather than merely that something was thrown. */ -import { describe, it, expect, vi } from 'vitest'; +import { describe, it, expect, vi, beforeAll, afterAll } from 'vitest'; import { SqlDriver } from '@objectstack/driver-sql'; import { RemoteTransport } from './remote-transport.js'; +import { TursoDriver } from './turso-driver.js'; +import { makeLibsqlSqliteStub, asLibsqlClient, type LibsqlSqliteStub } from './libsql-sqlite-stub.testkit.js'; interface WireBearingError extends Error { code?: string; @@ -218,25 +264,57 @@ describe('[#6212] RemoteTransport compiles the GroupByNode union', () => { expect(calls[0].sql).toBe('SELECT "stage", count("stage") AS "n" FROM "deal" GROUP BY "stage"'); }); - it('[#6401] refuses an unsafe identifier in `alias`, not only in `field`', async () => { - // The alias is caller-supplied text that now reaches the statement as a - // quoted identifier, so it needs the gate `field` already has. The - // assertion names the OFFENDING TEXT, not just the sentence (#6144): a - // `field` that is itself safe is what makes this case reach the alias - // check at all. + it('[#14235] ESCAPES an alias that would close the quoting — one inert name, one statement', async () => { + // ⚠️ This case REPLACES the #6401 pin that asserted the same input is + // REFUSED (`unsafe identifier rejected`, naming the offending text). That + // pin was the deliberate call when the alias was newly read here, and + // reversing it is a recorded, non-silent reversal rather than a rider: + // #13714 routed BOTH of driver-sql's output-name positions through + // `aliasIdentifierSql`, #14113 moved this transport's aggregation alias + // to escaping, and #14235 brings the second output-name position of the + // same method to the same line. An output-column key is a NAME: it is + // quoted and escaped, never gated. + // + // The old assertion is REPLACED, not dropped — the exact input it named + // is the input here, and what is pinned now is the statement it produces. const { t, calls } = transportWithCapturingClient(); - const err = await t - .aggregate('deal', { - groupBy: [{ field: 'stage', alias: 'bucket"; DROP TABLE deal; --' }], - aggregations: [{ function: 'count', alias: 'n' }], - }) - .then( - () => { throw new Error('expected the transport to refuse an unsafe alias'); }, - (e) => e as Error, + const rows = await t.aggregate('deal', { + groupBy: [{ field: 'stage', alias: 'bucket"; DROP TABLE deal; --' }], + aggregations: [{ function: 'count', alias: 'n' }], + }); + expect(rows).toEqual([]); + // The whole payload is ONE column name, the quote doubled — the standard + // escape inside a quoted SQL identifier. It is data, never grammar. + expect(calls).toHaveLength(1); + expect(calls[0].sql).toBe( + 'SELECT "stage" AS "bucket""; DROP TABLE deal; --", count("stage") AS "n" FROM "deal" GROUP BY "stage"', + ); + // ⛔ Not two statements: a payload that had broken out of its quoting + // would appear as a second one here. + expect(calls[0].sql).not.toContain('DROP TABLE deal;"'); + // And the grouping key is still the FIELD, exactly as for a bare alias. + expect(calls[0].sql.endsWith('GROUP BY "stage"')).toBe(true); + }); + + it('[#14235] a dotted or spaced alias round-trips as one quoted output column', async () => { + // The reachable population the card measured: a caller writing + // `groupBy: [{ field, alias }]` through the Query Protocol directly. + // Both spellings work on the in-memory, MongoDB and SQL faces and were an + // opaque 500 on this one — no `code`, no `status`, out of `mapDataError`. + for (const alias of ['Region Name', 'deal.stage_bucket']) { + const { t, calls } = transportWithCapturingClient(); + await t.aggregate('deal', { + groupBy: [{ field: 'stage', alias }], + aggregations: [{ function: 'count', field: 'stage', alias: 'n' }], + }); + expect(calls).toHaveLength(1); + expect(calls[0].sql).toBe( + `SELECT "stage" AS "${alias}", count("stage") AS "n" FROM "deal" GROUP BY "stage"`, ); - expect(err.message).toContain('unsafe identifier rejected'); - expect(err.message).toContain('bucket"; DROP TABLE deal; --'); - expect(calls).toEqual([]); + // ⛔ The dot stays INSIDE the quotes. The failure this rules out is a + // face that reads an output NAME as a qualified REFERENCE. + expect(calls[0].sql).not.toContain('"deal"."stage_bucket"'); + } }); it('still refuses an unsafe identifier inside a structured entry', async () => { @@ -394,4 +472,70 @@ describe('[#6212] RemoteTransport compiles the GroupByNode union', () => { expect(err.message).toContain("dialect 'better-sqlite3'"); }); }); + + // ── [#14235] Executed, not merely emitted ───────────────────────────────── + + /** + * ⚠️ Only EXECUTING the statement tells "escaped" apart from "broke out" — + * #14113's reasoning one position over, and the reason its pin is backed by a + * real database rather than a captured string. A capture assertion alone + * passes on an alias that terminates the quoting, because the text still + * *looks* like a select list. libsql IS SQLite, so the stub runs exactly what + * this transport emits: an alias that escaped its quoting is a syntax error + * (or a second statement better-sqlite3 refuses to prepare), and reading the + * value back under the literal alias is the proof that it did not. + */ + describe('[#14235] the escaped groupBy alias is inert against a real database', () => { + const DEAL = { + name: 'deal', + fields: { id: { type: 'string' }, stage: { type: 'string' }, amount: { type: 'number' } }, + }; + let driver: TursoDriver; + let stub: LibsqlSqliteStub; + + beforeAll(async () => { + stub = makeLibsqlSqliteStub(); + driver = new TursoDriver({ url: 'libsql://groupby-alias.turso.io', client: asLibsqlClient(stub) }); + await driver.connect(); + // The mode this block is about — the one with its own hand-written SQL. + expect(driver.transportMode).toBe('remote'); + await driver.syncSchema(DEAL.name, DEAL); + for (const row of [ + { id: '1', stage: 'won', amount: 10 }, + { id: '2', stage: 'won', amount: 20 }, + { id: '3', stage: 'lost', amount: 30 }, + ]) { + await driver.create(DEAL.name, { ...row }); + } + }); + + afterAll(async () => { + await driver.disconnect(); + stub.close(); + }); + + it('a dotted alias comes back under the result key the caller asked for, on rows', async () => { + const rows = (await driver.aggregate(DEAL.name, { + object: DEAL.name, + groupBy: [{ field: 'stage', alias: 'deal.stage_bucket' }], + aggregations: [{ function: 'sum', field: 'amount', alias: 'deal.total' }], + } as never)) as Array>; + const byBucket = Object.fromEntries(rows.map((r) => [r['deal.stage_bucket'], r['deal.total']])); + // A dot is inert inside a quoted identifier — the whole claim of the card. + expect(byBucket).toEqual({ won: 30, lost: 30 }); + }); + + it('an alias that tries to close the quoting and append a statement leaves the table standing', async () => { + const alias = 'bucket"; DROP TABLE deal; --'; + const rows = (await driver.aggregate(DEAL.name, { + object: DEAL.name, + groupBy: [{ field: 'stage', alias }], + aggregations: [{ function: 'count', alias: 'n' }], + } as never)) as Array>; + // The payload came back as a COLUMN NAME — it was data, never grammar. + expect(rows.map((r) => r[alias]).sort()).toEqual(['lost', 'won']); + // And the table it named is still there, with every row. + expect(stub.raw.prepare('select count(*) as c from deal').all()).toEqual([{ c: 3 }]); + }); + }); }); diff --git a/packages/drivers/driver-turso/src/remote-transport.ts b/packages/drivers/driver-turso/src/remote-transport.ts index 2b8d1bc117..7de7f70ffa 100644 --- a/packages/drivers/driver-turso/src/remote-transport.ts +++ b/packages/drivers/driver-turso/src/remote-transport.ts @@ -1369,12 +1369,28 @@ export class RemoteTransport { }); for (const { field, outKey } of groupBy) { + // The FIELD is a column REFERENCE — grammar — so it keeps the gate. this.assertSafeIdentifier(field); - // The alias reaches the statement as a quoted identifier, so it is held - // to the same gate as every other one — an alias is caller-supplied text - // and `assertSafeIdentifier` is what keeps it out of the SQL grammar. - this.assertSafeIdentifier(outKey); - selectParts.push(outKey === field ? `"${field}"` : `"${field}" AS "${outKey}"`); + // [#14235] The output NAME is ESCAPED, not gated — see + // {@link RemoteTransport.aliasIdentifierSql}. This is the second of the + // two output-name positions in this method; #14113 moved the aggregation + // alias one loop down, and this one was filed rather than folded in + // because it carried a landed pin asserting the refusal. + // + // `assertSafeIdentifier(outKey)` used to sit here, with the note "an + // alias is caller-supplied text and `assertSafeIdentifier` is what keeps + // it out of the SQL grammar". The first half is true and the second is + // what moved: quoting-with-escaping is what keeps a NAME out of the + // grammar, and it does so without refusing names the contract permits. + // `GroupByNodeSchema.alias` is an output-column key — the in-memory face + // projects `g.alias ?? g.field` verbatim, `driver-sql` routes the same + // position through `SqlDriver.aliasIdentifierSql` post-#13714, and this + // face refused anything that was not a bare `[A-Za-z_][A-Za-z0-9_]*` + // with an opaque 500 out of `mapDataError`. The reference-versus-name + // line #13714 and #14113 drew puts this position on the NAME side. + selectParts.push( + outKey === field ? `"${field}"` : `"${field}" AS ${this.aliasIdentifierSql(outKey)}`, + ); } // [#6321] Was `query?.aggregations || query?.aggregate` — see the twin note @@ -1941,6 +1957,13 @@ export class RemoteTransport { * A dot is inert inside `AS "…"`, and `.` is the name EVERY * analytics measure arrives under. * + * [#14235] Both output-name positions of `aggregate` route through here now, + * not just the aggregation alias: the groupBy select site emits + * `"" AS ()`. `GroupByNodeSchema.alias` is the same + * class of key as `AggregationNodeSchema.alias`, and `driver-sql` has routed + * both through its own `aliasIdentifierSql` since #13714 — so the two faces + * agree on the whole method rather than on one loop of it. + * * ## ⛔ Escaping is not "dropping the check" * * The alias reaches the statement RAW inside `AS "…"`, so an alias From 15435ba96854ae4debfb65a5c64551b7efc0c38b Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 07:44:22 +0000 Subject: [PATCH 2/4] test(driver-turso): pin the emitted statement for the escaped groupBy alias (#14235) The input the rewritten #6401 pin carries has no aggregation `field`, so the emission is `count(*)`, not `count("stage")`; and the one-statement assertion now counts SELECTs rather than probing for a quote that the escape never produces. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0112hMx9hjJ9BgB28X97DS68 --- .../src/remote-transport-groupby-node.test.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/drivers/driver-turso/src/remote-transport-groupby-node.test.ts b/packages/drivers/driver-turso/src/remote-transport-groupby-node.test.ts index df27aff1dd..15879d6460 100644 --- a/packages/drivers/driver-turso/src/remote-transport-groupby-node.test.ts +++ b/packages/drivers/driver-turso/src/remote-transport-groupby-node.test.ts @@ -287,11 +287,13 @@ describe('[#6212] RemoteTransport compiles the GroupByNode union', () => { // escape inside a quoted SQL identifier. It is data, never grammar. expect(calls).toHaveLength(1); expect(calls[0].sql).toBe( - 'SELECT "stage" AS "bucket""; DROP TABLE deal; --", count("stage") AS "n" FROM "deal" GROUP BY "stage"', + 'SELECT "stage" AS "bucket""; DROP TABLE deal; --", count(*) AS "n" FROM "deal" GROUP BY "stage"', ); - // ⛔ Not two statements: a payload that had broken out of its quoting - // would appear as a second one here. - expect(calls[0].sql).not.toContain('DROP TABLE deal;"'); + // ⛔ ONE statement, not two: a payload that had broken out of its quoting + // would appear as a second one here. The executing block at the foot of + // this file proves the same thing against a real database, which is the + // only instrument that tells "escaped" apart from "broke out". + expect(calls[0].sql.match(/SELECT/g)).toHaveLength(1); // And the grouping key is still the FIELD, exactly as for a bare alias. expect(calls[0].sql.endsWith('GROUP BY "stage"')).toBe(true); }); From 2a1da8f5bf6bda34c28c0256f65403d77d067b78 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 07:45:47 +0000 Subject: [PATCH 3/4] docs(driver-turso): record the #14235 ablation in the suite header (#14235) 4 failed / 14 passed of 18, case for case as predicted; every failure throws inside the call rather than failing a comparison, and the `field`-position control stays green. Notes why no `dist/` leg applies and how the mutation and the restore were each proved on disk. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0112hMx9hjJ9BgB28X97DS68 --- .../src/remote-transport-groupby-node.test.ts | 30 ++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/packages/drivers/driver-turso/src/remote-transport-groupby-node.test.ts b/packages/drivers/driver-turso/src/remote-transport-groupby-node.test.ts index 15879d6460..dfe00dbfa5 100644 --- a/packages/drivers/driver-turso/src/remote-transport-groupby-node.test.ts +++ b/packages/drivers/driver-turso/src/remote-transport-groupby-node.test.ts @@ -105,7 +105,35 @@ * the change. * - every date-bucket, parity and string-form case stays GREEN. * - * MEASURED: see the PR body for the run. + * MEASURED, case for case as predicted — **4 failed / 14 passed of 18**: + * + * ``` + * ESCAPES an alias that would close Error: RemoteTransport: unsafe identifier + * the quoting rejected: "bucket"; DROP TABLE deal; --" + * a dotted or spaced alias …rejected: "Region Name", then + * round-trips …rejected: "deal.stage_bucket" + * a dotted alias comes back under …rejected: "deal.stage_bucket" + * the result key (executing) + * an alias that tries to close the …rejected: "bucket"; DROP TABLE deal; --" + * quoting (executing) + * ── green ── + * still refuses an unsafe identifier inside a structured entry (the CONTROL) + * projects `alias` as the column name · no self-rename for alias === field + * the string-form control · all five date-bucket refusals · both parity cases + * ``` + * + * Not one failure came through a comparison: the restored gate throws before + * any SQL is built, which is why the guard could not simply be deleted from the + * `field` position and why the control above is the case that holds it. + * + * ⚠️ No `dist/` leg applies to this ablation. The suite imports the mutated + * unit as `./remote-transport.js` — a RELATIVE, in-package specifier that + * vitest resolves to `src/remote-transport.ts`, not through the package's + * `exports` — and `vitest.config.ts` declares no alias (it sets + * `disableConsoleIntercept` and nothing else). The mutation was proved on disk + * by grep counts on both the injected and the removed text and by + * `git hash-object` against the HEAD blob, and the restore by the same hash + * matching again plus an empty `git diff HEAD`. * * # Reverse verification (#6212) — direction predicted BEFORE it was run, per case * From ffabb9991c5eefbb6020632a829dcc39725debc2 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 08:17:36 +0000 Subject: [PATCH 4/4] test(driver-turso): close the two groupBy-alias scope lines #14113 and #14287 left (#14235) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both controls were hand-offs addressed to this card in their own comments — #14113's "this control records the state it was left in", #14287's "the day that card lands, this is the test that has to be rewritten deliberately rather than found red by surprise". PM ruling on the stop condition (comment 5506404777) amended this card's file surface to include them. Each file has exactly one case rewritten, on the same input, to assert the quoted emission instead of the refusal; the envelope file also records in its `## The accept set is UNTOUCHED` paragraph that the groupBy OUT KEY position moved. Every other refusal position — object, field, groupBy FIELD, aggregation alias, DDL — still asserts INVALID_REQUEST / 400, the accept-set describe that drives the groupBy FIELD is byte-identical, and `unsafeIdentifierError` / `UNSAFE_IDENTIFIER_CODE` are untouched. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0112hMx9hjJ9BgB28X97DS68 --- ...ransport-aggregation-alias-quoting.test.ts | 65 ++++++++++--------- ...ansport-unsafe-identifier-envelope.test.ts | 55 ++++++++++------ 2 files changed, 70 insertions(+), 50 deletions(-) diff --git a/packages/drivers/driver-turso/src/remote-transport-aggregation-alias-quoting.test.ts b/packages/drivers/driver-turso/src/remote-transport-aggregation-alias-quoting.test.ts index d4a75cdaa5..75c4d58e2c 100644 --- a/packages/drivers/driver-turso/src/remote-transport-aggregation-alias-quoting.test.ts +++ b/packages/drivers/driver-turso/src/remote-transport-aggregation-alias-quoting.test.ts @@ -269,37 +269,44 @@ describe('[#14113] RemoteTransport — the aggregation alias is escaped, not gat expect(seen).toEqual([]); }); - it('the groupBy alias position is UNCHANGED — still refused, and that is a separate card', async () => { - // ⚠️ Deliberate scope line, pinned so it cannot drift silently. The - // groupBy `alias` is the same class of thing (an output NAME) and - // `driver-sql` escapes it post-#13714 (`aliasIdentifierSql` at its - // groupBy select site), so this face diverges there too — but that - // position carries a LANDED pin (#6401, `remote-transport-groupby-node`) - // asserting the refusal, so reversing it is a judgement this card was not - // dispatched to make. Filed separately rather than patched inline; this - // control records the state it was left in. + it('[#14235] the groupBy alias position is now ESCAPED TOO — the scope line this card left is closed', async () => { + // ⚠️ This case REPLACES the deliberate scope line #14113 left here, which + // asserted this same input is REFUSED and said of itself: "Filed + // separately rather than patched inline; this control records the state + // it was left in." #14235 is the card it was waiting for. It reached the + // same reference-versus-name line by the same reasoning — an output NAME + // is quoted and escaped, a column REFERENCE is validated — so BOTH + // output-name positions of `aggregate` now route through + // `aliasIdentifierSql`, and this face agrees with `driver-sql` (which has + // routed both since #13714) on the whole method rather than one loop. // - // It is also NOT on the reproducing path: `ObjectQLStrategy` resolves a - // dimension to a bare column name, so no analytics query sends a dotted - // groupBy alias. + // What is pinned here is what the two positions do TOGETHER in one + // statement: a dotted dimension alias beside a dotted measure alias, both + // quoted, neither refused. That is the assertion #14113's control could + // not make while it was holding the scope line. const { t, seen } = await capturing(); - const err = await t - .aggregate(DELIVERY_OBJECT.name, { - object: DELIVERY_OBJECT.name, - groupBy: [{ field: 'region', alias: 'showcase_delivery.region' }], - aggregations: [{ function: 'count', alias: 'showcase_delivery.count' }], - } as never) - .then( - () => { throw new Error('expected the groupBy alias to still be refused') }, - (e) => e as Error, - ); - expect(err.message).toContain('unsafe identifier rejected'); - expect(err.message).toContain('showcase_delivery.region'); - // [#14287] The GATING is what this control holds; the ENVELOPE is what - // that card added to it. Both are pinned, so the open gating card cannot - // be mistaken for having landed. - expect(envelopeOf(err)).toEqual(ENVELOPE); - expect(seen).toEqual([]); + const rows = await t.aggregate(DELIVERY_OBJECT.name, { + object: DELIVERY_OBJECT.name, + groupBy: [{ field: 'region', alias: 'showcase_delivery.region' }], + aggregations: [{ function: 'count', alias: 'showcase_delivery.count' }], + } as never); + expect(seen).toEqual([ + 'SELECT "region" AS "showcase_delivery.region", count(*) AS "showcase_delivery.count" ' + + 'FROM "showcase_delivery" GROUP BY "region"', + ]); + // ⛔ Neither dot may become a qualified reference: both stay INSIDE their + // own quotes. + expect(seen[0]).not.toContain('"showcase_delivery"."region"'); + expect(seen[0]).not.toContain('"showcase_delivery"."count"'); + // Executed, not merely emitted — the values come back under the caller's + // own keys, and GROUP BY still keys on the FIELD. + const byRegion = Object.fromEntries( + (rows as Array>).map((r) => [ + r['showcase_delivery.region'], + r['showcase_delivery.count'], + ]), + ); + expect(byRegion).toEqual({ west: 2, east: 1 }); }); it('the default alias is byte-identical to what it was before', async () => { diff --git a/packages/drivers/driver-turso/src/remote-transport-unsafe-identifier-envelope.test.ts b/packages/drivers/driver-turso/src/remote-transport-unsafe-identifier-envelope.test.ts index 94fe9e38ad..4bb698ef48 100644 --- a/packages/drivers/driver-turso/src/remote-transport-unsafe-identifier-envelope.test.ts +++ b/packages/drivers/driver-turso/src/remote-transport-unsafe-identifier-envelope.test.ts @@ -54,7 +54,12 @@ * The last two `describe`s pin both halves: the same inputs are refused, and * safe identifiers still compile and run. That is what makes this an envelope * change rather than a gating change — the `groupBy` alias GATING question is - * a separate card and stays open. + * a separate card and stays open. [#14235] That card has since landed: the + * `groupBy` OUT KEY position is ESCAPED rather than refused now, so its case + * below asserts the quoted emission instead of a refusal — every OTHER + * position named above (`object`, `field`, the `groupBy` FIELD, the DDL table, + * column and index names) is untouched and still refuses with this envelope, + * and the accept-set `describe` below still drives the `groupBy` FIELD. * * ## Reverse verification — direction predicted BEFORE it was run * @@ -234,27 +239,35 @@ describe('[#14287] RemoteTransport — an unsafe identifier is a 400 INVALID_REQ expect(seen).toEqual([]); }); - it('the `groupBy` OUT KEY refuses with the envelope — gating unchanged, envelope added', async () => { - // ⚠️ Deliberate scope line. Whether this position should be ESCAPED - // rather than refused (as `driver-sql` escapes it post-#13714, and as - // #14113 changed the aggregation alias to do one position over) is a - // separate, still-open card: it MOVES the accept set. This case pins that - // the refusal itself is unchanged and only its envelope is new — so the - // day that card lands, this is the test that has to be rewritten - // deliberately rather than found red by surprise. + it('[#14235] the `groupBy` OUT KEY is ESCAPED, not refused — the one position that moved', async () => { + // ⚠️ This case is the deliberate rewrite #14287's scope line asked for. + // It read: "the day that card lands, this is the test that has to be + // rewritten deliberately rather than found red by surprise" — #14235 is + // that card, and this is that rewrite, on the same input. + // + // ⛔ It is the ONLY position that moved. Every sibling case in this + // describe — `object`, the aggregation `field`, the `groupBy` FIELD — and + // every DDL case below still refuses with the envelope, unchanged; + // `unsafeIdentifierError` and `UNSAFE_IDENTIFIER_CODE` are untouched. + // What #14235 decided is which POSITIONS the gate governs, not what the + // gate answers when it fires: an output NAME is quoted and escaped, a + // column REFERENCE is validated. const { t, seen } = await capturing(); - const err = await refusalFrom('groupBy alias', () => - t.aggregate(OBJECT_DEF.name, { - object: OBJECT_DEF.name, - groupBy: [{ field: 'region', alias: 'envelope_probe.region' }], - aggregations: [{ function: 'count', alias: 'n' }], - } as never), - ); - expect(envelopeOf(err)).toEqual({ - ...ENVELOPE, - message: 'RemoteTransport: unsafe identifier rejected: "envelope_probe.region"', - }); - expect(seen).toEqual([]); + const rows = await t.aggregate(OBJECT_DEF.name, { + object: OBJECT_DEF.name, + groupBy: [{ field: 'region', alias: 'envelope_probe.region' }], + aggregations: [{ function: 'count', alias: 'n' }], + } as never); + // No refusal, and the alias reaches the database as ONE quoted name. + expect(seen).toEqual([ + 'SELECT "region" AS "envelope_probe.region", count(*) AS "n" ' + + 'FROM "envelope_probe" GROUP BY "region"', + ]); + // Executed, not merely emitted: the value comes back under the caller's + // own key. (`region` is a real column here, so the statement runs.) + expect( + (rows as Array>).every((r) => 'envelope_probe.region' in r), + ).toBe(true); }); });