ReviewOS

stacks/bun-query-builder

SELECT builder emits clauses in chain-call order, not canonical SQL order (orderBy then where → invalid SQL)

#1018
Closed glennmichael123 opened this 22 days ago · 0 comments
22 days ago

Summary

The SELECT query builder appends clauses to the SQL string in the order methods are chained, not the canonical SQL order (WHEREGROUP BYHAVINGORDER BYLIMITOFFSET). Chaining .orderBy() before .where() produces SQL like:

```sql SELECT ... FROM users ORDER BY id DESC WHERE email LIKE ? ```

SQLite rejects with near "WHERE": syntax error. The same query works if you call .where() first, then .orderBy().

Reproducer

```ts import { db } from '@stacksjs/database'

// Crashes — chain-call order leaks into SQL db.selectFrom('users').select(['id']).orderBy('id', 'desc').where('email', 'like', '%foo%').toSQL() // → SELECT id FROM users ORDER BY id DESC WHERE email like ?

// Works — chain happens to match canonical order db.selectFrom('users').select(['id']).where('email', 'like', '%foo%').orderBy('id', 'desc').toSQL() // → SELECT id FROM users WHERE email like ? ORDER BY id DESC ```

Impact

This is a real footgun for the common case of incremental query construction:

```ts let q = db.selectFrom('users').select([...]).orderBy('id', 'desc')

if (req.q) q = q.where('email', 'like', %${req.q}%).orWhere('name', 'like', %${req.q}%)

await q.limit(perPage).offset(offset).execute() // CRASH ```

The natural mental model — "build the base query, then conditionally add filters" — produces invalid SQL because the conditional where() runs after the initial orderBy() in the chain. Caller has to remember to defer .orderBy() until after all conditional .where() blocks. This is unusual; most query builders (Knex, Kysely, Drizzle, Laravel) reorder clauses to canonical SQL at compile time.

Caught while implementing an admin user-list endpoint with optional search filter — the unfiltered path returned 200, every filtered path 500'd.

Suggested fix

In the SELECT builder's toSQL() / build() step, split the accumulated text into clauses and re-assemble them in canonical SQL order:

``` SELECT [columns] FROM [table] [JOINs] [WHERE ... ] [GROUP BY ... ] [HAVING ... ] [ORDER BY ... ] [LIMIT ... ] [OFFSET ... ] ```

…regardless of method-call order. Either:

  1. Buffer per-clause state (e.g. whereParts, orderByParts, etc.) and only assemble the SQL string on build() / toSQL() / execute(). Most query builders work this way; it's the standard fix and removes the chain-order pitfall entirely.
  2. Sort the existing text-mutation pass — when adding a clause, splice it into the right position in the existing string. Bigger refactor since the current code does text += '...' direct concatenation.

Option 1 is the canonical refactor. Option 2 is a smaller patch but loses the clarity of per-clause state.

Workaround

Always chain .where() / .orWhere() / .whereIn() BEFORE .orderBy(). If you want to add where conditionally, build the where calls first, then apply .orderBy() / .limit() / .offset() last. Doesn't compose well with helper functions that return a partial builder.

Environment

  • bun-query-builder vendored via stacks pantry (symlinked to a local checkout)
  • Bun 1.3.13, SQLite driver

Sign in to comment on this issue.