ReviewOS

stacks/bun-query-builder

paginate() drops the WHERE clause's bindings — throws on any filtered query

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

paginate() builds its SQL with the WHERE clause's placeholders but does not pass that clause's bindings, so the driver receives fewer values than the statement needs and throws. Any where that contributes at least one parameter makes paginate() unusable.

Found on 0.2.26. This is not new — an app in this org has carried a hand-written paginator shim since 0.1.26 specifically to work around it.

Repro

const T = 'migrations' // 155 rows

await db.selectFrom(T).selectAll().paginate(5)
// ok — data=5 total=155

await db.selectFrom(T).selectAll().where('id', '>', 0).paginate(5)
// THREW: SQLite query expected 3 values, received 2

await db.selectFrom(T).selectAll().whereIn('id', [1, 2, 3]).paginate(5)
// THREW: SQLite query expected 5 values, received 2

await db.selectFrom(T).selectAll().whereNull('executed_at').paginate(5)
// ok

The arithmetic identifies the cause

The shortfall is exactly the number of parameters the WHERE contributes, and paginate always supplies 2 (limit + offset):

where clauseparams it needsexpectedreceived
.where('id','>',0)132
.whereIn('id',[1,2,3])352
.whereNull('executed_at')0works

whereNull is the tell: it is the one where-form that adds SQL text but no bindings, and it is the one that doesn't throw. So the SQL text is being assembled from the full builder state while the binding array is being assembled from paginate's own two values only.

Impact

paginate() is only useful on an unfiltered table, which is the case that needs it least. Every real paginated list — a moderation queue, a user's own records, anything scoped to a tenant or a status — has to abandon it and hand-roll COUNT(*) plus LIMIT/OFFSET, which then has to duplicate the filter on both queries and keep them in step.

Worth noting the failure is at least loud. The sibling issue I'm filing alongside this one (orWhere grouping) is the same family of problem — builder state not surviving composition — but fails silently toward more rows, which is considerably worse.

Note on the paginate(perPage) signature

While reducing this I first called paginate({ perPage: 5, page: 1 }) and got:

[query-builder] paginate(perPage): expected positive integer, got [object Object]

Good error. But dist/src/index.js contains three different paginate shapes — paginate(perPage, page, opts), paginate(page, perPage) and paginate(pageSize, lastKey) — so an options object is an easy assumption to make. If those belong to different builders it may be worth naming them distinctly, or accepting an options object on all of them.

Sign in to comment on this issue.