ReviewOS

also looking at this

stacks/bun-query-builder

fix(sqlite): a nested query's bindings were being thrown away

#1085
Merged glennmichael123 wants to merge fix/nested-fragment-params into main
2 files +156 -6
10 days ago

Closes #1084.

The bug isn't in paginate()

The report's own arithmetic pointed at the cause: the shortfall was always exactly the number of parameters the WHERE contributed, and whereNull — the one where-form that adds SQL text but no bindings — was the one that worked.

It's in the bun:sqlite wrapper's tagged-template interpolation. The query objects that wrapper returns carry raw: () => sql so callers can read their text — which also makes them satisfy the raw-marker branch that was tested first. So interpolating one query into another took that branch: spliced the nested query's SQL text, discarded its values. The nested-query branch below, the one that merges params, was unreachable for the wrapper's own queries.

Reordered so the nested-query case is tested first, discriminating on values being an array — raw markers carry __raw and no values, and Bun's native query objects expose values as a method, so neither is captured.

The count query failed more quietly than the page query

paginate nests twice — COUNT(*) FROM (…) and … LIMIT ? OFFSET ?. bun:sqlite only arity-checks when some values are passed, so while the page query threw, the count query bound WHERE id > ? to NULL, matched nothing, and returned total: 0.

Fixing only the page query would have replaced a loud crash with a wrong answer. The tests assert meta.total explicitly for that reason.

before:  .where('id','>',10).paginate(5)  ->  THREW: expected 3 values, received 2
after:   .where('id','>',10).paginate(5)  ->  data=[11..15]  total=10

It was never just pagination

Everything that composes a query into a template was affected — including two the report doesn't mention, both silent:

  • exists() returned false for filtered queries that had rows
  • first() returned a row from outside the filter

Postgres was never affected — it uses Bun's native SQL, not this wrapper. That's also why the tests must run against the library's own connection rather than an injected new SQL(...): an injected connection routes around the wrapper entirely, which is exactly why the existing pagination tests never caught this.

Verification

8 new tests. Verified they discriminate: 5 of 8 fail against the unfixed db.ts.

Typecheck 0, lint 0, suite 1719 pass / 4 skip / 0 fail (+8).

Not in this PR

#1083 (the orWhere grouping bug) is a much larger change — the select builder has no WHERE term list at all, just an appended string, so fixing precedence means introducing one across ~40 append sites in client.ts plus the parallel implementation in orm.ts. It also changes emitted SQL for existing users, so it wants its own PR and its own changelog entry. Investigated and specced; not started.

Also found while investigating, worth separate issues: the ORM's paginate(page, perPage) takes its arguments in the opposite order to the query builder's paginate(perPage, page) — confirmed empirically, and it silently returns the wrong page rather than erroring.

🤖 Generated with Claude Code

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

Sign in to comment on this pull request.