ReviewOS

stacks/bun-query-builder

unsafe() executes ?-style placeholders on Postgres, failing at the wrong token

#1171
Closed glennmichael123 opened this 9 days ago ·
9 days ago

unsafe(query, params) accepts a query whose placeholders the connection cannot possibly bind, and passes it through to the server. On Postgres the failure surfaces far from its cause.

What happens

await db.primary.unsafe('SELECT * FROM users WHERE email = ? LIMIT 1', [email])
// PostgresError: syntax error at or near "LIMIT" at character 37

Postgres has no ? parameter. It parses the token as an operator, looks for a right operand, and rejects the next word. So the error names LIMIT while the fault is the ? 37 characters earlier, and the bound value in params was never going to arrive.

db.d.ts already documents the intended shape, sql.unsafe('SELECT $2 AS a, $1 AS b', ['ONE', 'TWO']), so the contract is clear. Nothing enforces it.

Why it is worth a guard rather than a doc line

This is cheap to write by accident and expensive to diagnose:

  • It typechecks. unsafe takes a string.
  • It passes on SQLite and MySQL, which both accept ?. A cross-dialect suite goes red on one dialect only.
  • The error points at the wrong token, so the first hour goes into LIMIT.
  • The sql\`tag in a consuming project renders?for every interpolated value, which is correct while the builder re-renders it at compile time and wrong the moment someone reads.sql/.parametersoff the fragment and callsunsafe` with them. That is an easy step to take, because the fragment looks like a finished query.

It cost us a full red suite downstream: every login, password recovery, magic link and two-factor exchange in Stacks resolved its user through one such call, so the entire Postgres auth suite failed at once, 126 occurrences in a single CI run. Details and the fix in stacksjs/stacks#2842 and stacksjs/stacks@d7b49fa.

Suggested guard

When params is non-empty and the connection's dialect does not use ?, refuse rather than execute. A query with bound parameters on Postgres must contain $n, so the signal is unambiguous:

  • params.length > 0
  • dialect is Postgres
  • the query contains no $1

Throwing there costs nothing on the working path and names the real problem at the call site, the way whereRaw already does when handed a fragment carrying bound values:

[query-builder] whereRaw(fragment): this method takes a single fragment and no bindings, so the argument(s) after it would be dropped.

That error is why the equivalent mistake on whereRaw is a non-event, and it is the model worth copying here.

One caveat: a bare ? can legitimately appear inside a string literal (WHERE name = 'what?'). Keying the check on "has params, has no $n" rather than on the presence of ? alone avoids that, since such a query would not be passing bindings in the first place.

Acceptance

  • unsafe with bound params and ?-style placeholders on Postgres throws at the call site, naming the placeholder style the connection expects.
  • MySQL and SQLite are unaffected.
  • A query with no params is never inspected, so raw DDL keeps working untouched.

Sign in to comment on this issue.