Uh oh!
There was an error while loading. Please reload this page.
fix(driver-turso): escape the aggregation alias instead of gating it, so remote-mode analytics cube queries stop 500ing - #14248
Conversation
📓 Docs Drift Check2 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to list — not a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run. What this run could not see
Coarse fallback — 6 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin ceff15a3e18b43bc3b7a0676e6b1a74875d3202d && git checkout ceff15a3e18b43bc3b7a0676e6b1a74875d3202d
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 74a32a2edd55f06158effef71b9151f2f2eec36a 689d223f59823940012d20e0dc679b111d1ab4d9 && git checkout -B drift-repro 74a32a2edd55f06158effef71b9151f2f2eec36a && git merge --no-ff 689d223f59823940012d20e0dc679b111d1ab4d9
node scripts/docs-audit/affected-docs.mjs --json 74a32a2edd55f06158effef71b9151f2f2eec36a |
Uh oh!
There was an error while loading. Please reload this page.
Closes#14113
RemoteTransport.aggregate(Turso remote mode) held the aggregationaliastoSAFE_IDENTIFIER(/^[a-zA-Z_][a-zA-Z0-9_]*$/). A dot fails that regex. Every analytics measure is namedCUBE.MEASUREon the wire andObjectQLStrategyuses that name verbatim as the aggregationalias, so every cube query reaching this face threw:a bare
Errorwith nocodeand nostatus, whichmapDataErrorthen served as an opaque 500 — for a query that is spelled correctly.The repair: escape the alias, do not drop the check
The alias is now escaped rather than gated — it may be any string, and the quote character is doubled (
"becomes""), the standard escape inside a quoted SQL identifier. New private helperRemoteTransport.aliasIdentifierSql, placed besideassertSafeIdentifierso the reference-versus-name pair reads as one thing.⛔ Not "drop the check". The alias reaches the statement raw inside
AS "...", so an alias containing a"would close the quoting and continue as grammar. This is the ALIAS half of the distinction #13714 drew one face over, where the same position routes through knex'swrapIdentifier(SqlDriver.aliasIdentifierSql): a qualified reference must be validated, a single output name must be quoted and escaped.AggregationNodeSchemadeclaresalias: z.string()— an output-column key — and the in-memory, MongoDB and post-#13714 SQL faces all project it verbatim. This face was the outlier.The
fieldandobjectpositions keepassertSafeIdentifierunchanged: those become column and table references, which are grammar.Verification
Tests execute against a real SQLite-backed libsql stub (
makeLibsqlSqliteStub) rather than asserting on a SQL string. That is deliberate and load-bearing: only executing the statement tells "escaped" apart from "broke out" — a string assertion passes on an alias that terminates its own quoting, because the text still looks like a select list.New suite
remote-transport-aggregation-alias-quoting.test.ts, 9 cases:showcase_delivery.countreturns the value under the caller's own key; emitted SQL pinned asSELECT count(*) AS "showcase_delivery.count" FROM "showcase_delivery"with an explicit assertion that it is not split into two segments; plus the un-bucketed grouped-cube shape end to end."is escaped:won"countcompiles toAS "won""count"and runs;bucket"; DROP TABLE showcase_delivery; --compiles to one inert column name, is returned as a column name, and the table it named still holds all 3 rows.fieldposition still refuses and sends nothing; theobjectposition still refuses; thegroupByalias position is unchanged; the default aliascount_allis byte-identical.typecheckwas confirmed to actually read the new test file (tsc --noEmit --listFilescount = 1, not 0), so "typecheck clean" is a measurement over these edits rather than a green over source nothing read.Reverse verification — direction predicted before it was run
Restoring the pre-fix two lines (
this.assertSafeIdentifier(alias);+AS "${alias}"). Predicted: the 3 dotted-alias cases and the 2 quote-escape cases go red by throwing inside the call, not on a comparison; the 3 position controls and the default-alias case stay green.Measured — 5 failed / 4 passed of 9, case for case as predicted:
The first of those is the card's reported symptom reproduced verbatim. The mutation was confirmed on disk before the run (anchor counts: gate call 0 to 1, new emission 1 to 0, old emission 0 to 1) and the restore was proven by observation afterwards —
git diff HEADempty andgit hash-objectbyte-identical to theHEADblob, not by the restore command's exit code. No rebuild leg was needed and that is measured, not assumed:packages/drivers/driver-turso/distdoes not exist, so the subject under test resolves from source.Gates
34 families derived from the real change set by
node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands(not from a hand-written diff list) and run at commit689d223f. Exit codes captured after redirecting to a file, never through a pipe.31 of 34 exited 0. The other 3 are NOT MEASURED, not red — each refuses on its own prerequisite because it reads built output the whole workspace has to produce, and each says so in its own verdict line rather than being inferred from the exit code:
check-test-completeness.mjsturbo run testlog)check:dual-build-cjs-loadscheck:type-check-debt--re-measureneeds the built workspace closure)All three are unreachable locally without a full
pnpm buildof every package, which CI performs; none is affected by the semantics of this diff. Recorded as NOT MEASURED rather than folded into the green count.Also run beyond the derived set:
node scripts/check-nul-bytes.mjs(exit 0, 7781 files) plus a direct control-byte scan of the three changed files (clean).Out of scope, filed rather than patched
#14235 — the
groupByalias is the second output-name position in this same method and is still gated, whiledriver-sqlescapes it post-#13714. It was deliberately left alone: that position carries a landed pin (#6401,remote-transport-groupby-node.test.ts) asserting the refusal, so reversing it is a judgement, not a mechanical edit. It is also not on the reproducing path — measured, not assumed:ObjectQLStrategy.resolveFieldNameresolves a dimension tomember.sqlormember.split('.')[1], so only the measure arrives dotted. A regression control in the new suite pins the position's current behaviour so it cannot drift silently while that card is open.Clause-②: yes — the guard stays intact in the
fieldposition (and inobject); this diff touches the aggregation alias position only, and both untouched positions carry explicit regression controls. The diff does not reachpackages/drivers/driver-sql(STOP condition 1 not triggered), and no open PR claimsremote-transport.ts(re-checked against the 14 open PRs immediately before push, STOP condition 3).Dispatch assumptions, measured
SAFE_IDENTIFIERis still atremote-transport.ts:71, unmoved, at base66ecc50a(main had already advanced past the1403d943the order cited).driver-turso— HOLDS. Diff is 3 files, all indriver-tursoplus the changeset.TursoDriver.supportspublishesqueryDateGranularity: {}in remote mode, so a bucketed query falls back tofind()+ in-memory bucketing and never reachesaggregate. The new suite reproduces on the un-bucketed grouped-cube shape.assertSafeIdentifieritself still throws a bareError(nocode, nostatus) for thefield,object,groupByand DDL positions — so a genuinely unsafe field on this path still reaches the caller as an opaque 500. That is the other half of the driver-sql (PG): sum/avg/min/max over a boolean column throw the raw PostgreSQL 42883 with no ADR-0112 envelope (status undefined) #11455 / A dotted WHERE key escapes as a raw dialect error with the bound literal inlined on Postgres and MySQL — pre-existing, and measured against live servers #8931 shape and it is not closed here: picking its envelope (an injection refusal is not obviouslyINVALID_QUERY/400) is a judgement no landed evidence pins, and it would change the error surface for DDL and backfill callers too. Reported rather than guessed.Generated by Claude Code