ReviewOS

also looking at this

stacks/bun-query-builder

fix(client): place clauses by statement structure, not by text search

#1129
Merged chris wants to merge fix/1121-shared-sql-scanner into main
3 files +392 -72
8 days ago

Closes #1121.

Three scanners decided where a clause goes, each with its own idea of what SQL is:

siteparen depthstring literalscomments
firstTailIndexyesnono
insertJoinyesnono
computeReorderedClausesyesyesno

A raw fragment is arbitrary caller text, so a keyword inside a string literal or a comment was read as a clause and the splice landed inside it. The statement stayed valid, which is what made this the worst of the family:

SELECT *, 'a WHERE id = $1 limit 3 b' as t FROM users        -- no WHERE; returns every row
SELECT *, 'a INNER JOIN posts ON … where b' as tag FROM users -- never joins

Neither errors, and no parameter mismatch catches them: a $1 inside a literal is not a placeholder, and the surplus binding is ignored rather than rejected. A filter silently not applying on a read is the shape that leaks rows across a tenant or permission boundary — which is why this ranked above #1113 despite being narrower to trigger.

One scanner

All three now call scanTopLevelKeywords(), which skips everything that is not statement structure:

  • parenthesised sub-expressions, so a subquery's own ORDER BY is not ours
  • '…', "…", `…` runs, where doubling escapes the delimiter
  • -- … line comments and /* … */ block comments, which nest, as Postgres allows
  • $$…$$ / $tag$…$tag$ dollar-quoted strings — without mistaking a $1 placeholder for the start of one, which would have skipped the rest of the statement

Backslash escapes inside a literal are deliberately not honoured. Whether 'a\' closes a literal depends on the dialect and, on Postgres, on standard_conforming_strings — so this matches the standard and the one scanner that was already literal-aware, rather than inventing a rule that is wrong somewhere.

insertJoin also spelled its multi-word keywords with a single literal space, so GROUP\n BY in a fragment was missed entirely. The shared patterns take any whitespace.

Performance

The scanners this replaces jumped between matches of a single alternation regex — which is fast, and is also precisely why they could not skip literals. Walking characters costs more, and the first cut was ~50% slower on select building. Recovered by:

  • rejecting positions on charCode before any regex runs (every clause keyword starts with a letter)
  • hoisting the keyword tables out of the per-query path — a regex literal is constructed each time its expression is evaluated, so building them inside the builder cost ~19 allocations per query
  • letting the two first-hit-only callers stop at the first match instead of scanning the rest

20k select builds, three runs each: 73–75ms on this branch, 64–71ms on main — down from 101ms before those three changes. I'd call that parity within noise; if you want it exactly flat I can add a no-quotes/no-comments fast path, but it would only help statements with no raw fragments, which are the ones that were never at risk.

Verification

17 new tests — 4 on the builders, 13 on the scanner directly (nested comments, doubled quotes, $1 vs $$, unterminated literals). All 4 builder tests and the comment cases fail without this change, verified by reverting client.ts alone so the export still resolves.

The builder assertions are on row counts against the control query, not emitted text: the whole point is that the bad SQL runs fine, so only the rows settle it.

Full suite 2116 pass / 0 fail, typecheck clean, pickier 0 errors.

🤖 Generated with Claude Code

3 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.