ReviewOS

also looking at this

stacks/bun-query-builder

fix(postgres): skip literals and comments when numbering placeholders

#1151
Merged glennmichael123 wants to merge fix/pg-placeholders-skip-literals into main
7 files +224 -45
9 days ago

Follow-up to #1148, from the review of that fix.

Every ? was a placeholder, including the ones that were not

toPostgresPlaceholders was sql.replace(/\?/g, () => '$' + ++i). The ORM only emits ? as a parameter marker, but whereRaw fragments are caller text, so on live Postgres:

Model.query().whereRaw("label = '?'")            // sent as label = '$1' — matched nothing, no error
Model.query().whereRaw('id = ? /* or ? */', 1)   // the comment took $2: could not determine data type of parameter $2

The builders' { sql, parameters } expressions had the same shape of bug one level down: expression.sql.replace(/\?/g, …) numbered the first N markers wherever they were, so a literal '?' ahead of a real placeholder stole its number and left the placeholder unbound.

sql-fragments.ts already had a literal-aware toDialectPlaceholders — the ORM just never used it, and it did not skip comments.

One scanner

mapPlaceholders(sql, replace) walks the statement and hands each unquoted ? to the callback with its ordinal. It skips what is data or prose rather than a parameter:

  • '…', "…" and `…` runs, where doubling escapes the delimiter
  • $$…$$ / $tag$…$tag$ dollar-quoted strings
  • -- … line comments and /* … */ block comments, which nest

Backslash escapes are still not honoured, for the reason already given on scanTopLevelKeywords: whether 'a\' ends the string depends on the dialect and, on Postgres, on a server setting.

Now used by toDialectPlaceholders, the ORM's Postgres rewrite, the builders' bound expressions, and whereRaw's lone-undefined rule from #1148 (which was fragment.includes('?'), so whereRaw("name <> '?'", bindings) read the literal as a placeholder).

jsonb ?

Postgres' ?, ?| and ?& operators cannot be told apart from placeholders, and could not be used through the builder before this either. The docs now point at the function forms: jsonb_exists(col, ?), jsonb_exists_any(col, ?), jsonb_exists_all(col, ?).

Tests

  • test/placeholders.test.ts — line and nested block comments, backtick runs, an unterminated comment, plus countPlaceholders and mapPlaceholders directly.
  • test/orm.pg-placeholder-literals.pg.test.ts — against live Postgres in a subprocess: a ? in a literal, a ? in a comment beside real bindings, a literal fragment followed by where(), jsonb_exists, and a builder update whose bound expression contains a literal '?'. Every one of those cases fails against main — silently [], 42P18, 08P01, or a syntax error.
  • test/orm.where-raw-bindings.test.ts — the undefined rule ignores a ? inside a literal or comment.

Locally: full suite 2226 pass / 0 fail / 10 skip, tsc clean, pickier . at its 330-warning baseline.

🤖 Generated with Claude Code

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

Sign in to comment on this pull request.