ReviewOS

stacks/bun-query-builder

bugs: dialect-specific broken behaviors (whereNotBetween boolean tree, selectFromSub no-op, upsert quoting, …)

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

Cross-filed from a Stacks-side audit (stacksjs/stacks#1862). These are non-injection correctness bugs in the bun-query-builder runtime — the queries either produce wrong results or fail on specific dialects.

Findings

#File:lineIssue
#10dist/src/index.js:19288-19292ModelQueryBuilder.whereNotBetween produces a broken boolean tree (col > upper becomes top-level disjunction, bypassing prior WHERE filters).
#11dist/src/index.js:9328-9425selectFromSub returns a builder whose whereRaw / whereIn / join / groupBy / having / ~40 methods are silent no-ops (() => this placeholders).
#12dist/src/index.js:8794-8803paginate() runs count + data as two separate queries with no shared snapshot. Concurrent writes leave total and data.length inconsistent.
#13dist/src/index.js:9473-9477applyWhereCondition in selectFromSub builds IN (?) for arrays — single placeholder for the whole array.
#14dist/src/index.js:9953-9959Transaction isolation/read-only built as nested template literals. MySQL syntax differs (SET SESSION TRANSACTION …); SQLite has none. Failures are silently caught.
#15dist/src/index.js:10025insertOrIgnore hard-coded to ON CONFLICT DO NOTHING. MySQL needs INSERT IGNORE; SQLite tolerates ON CONFLICT only with a target.
#16dist/src/index.js:10060upsert builds the merge clause with EXCLUDED.${c} passed through bunSql(...) as one identifier — quoted as "EXCLUDED.column" instead of EXCLUDED."column".
#17dist/src/index.js:9876-9897advisoryLock / tryAdvisoryLock silently no-op on MySQL/SQLite, but the return type makes it look like a successful lock.
#18dist/src/index.js:6663-6671setConfig mutates module-level config3 shared by every Database instance — two Databases in the same process trample each other.
#19dist/src/index.js:8887-8898onlyTrashed rewrites SQL via text.replace(/WHERE/, …) — replaces the FIRST WHERE anywhere, including inside subquery strings.
#20dist/src/index.js:9141-9147withCTE / withRecursive interpolate name raw (no validation). Same for joinSub / crossJoinSub aliases.
#25dist/src/index.js:8609, 8614, 8758-8761limit(n) / offset / forPage interpolate n raw — NaN produces broken SQL.
#26dist/src/index.js:9070-9086count() after groupBy() returns ONE group's count, not the total.
#29dist/src/index.js:8369-8374whereDate casts via String(date) — a Date object becomes "Tue May 21 2026 …" and most DBs silently reject the comparison.
#30dist/src/index.js:8774-8782pluck(column, key) indexes by String(r?.[key]). Two rows with the same key collide — silent data loss.
#31dist/src/index.js:19389-19438eagerLoadRelations runs one query per relation per model. For N relations on a list, that's N queries, not the documented batched fetch.
  • #10 whereNotBetween: wrap in a parenthesised group; use boolean: 'and' both times with negated operators inside the group.
  • #11 selectFromSub: implement the methods on the returned builder OR remove them from the type interface so callers can't silently no-op.
  • #13 IN (?): expand placeholder list to match array length, or migrate to driver array-binding.
  • #15 insertOrIgnore: dispatch on dialect → ON CONFLICT DO NOTHING (Postgres/SQLite), INSERT IGNORE (MySQL).
  • #16 upsert: build merge columns as raw fragments per dialect; never wrap EXCLUDED.x through the identifier quoter.
  • #18 setConfig: scope config per-Database-instance rather than module-global.
  • #19 onlyTrashed: build WHERE inline at query-build time, not via post-hoc regex replace.
  • #25 LIMIT/OFFSET: parameterise.
  • #26 count + groupBy: wrap in SELECT COUNT(*) FROM (<grouped query>) AS _t.

Stacks-side status

  • Two findings (L-36 Database.close not awaited, L-37 extractTables regex doesn't parse quoted identifiers) lived in Stacks's own code and shipped in stacksjs/stacks@c245439.
  • The rest need this repo's runtime fixes.

Sign in to comment on this issue.