ReviewOS

stacks/bun-query-builder

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

#1010
Closed glennmichael123 opened this 2 months ago ·
glennmichael123 2 months 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.

Recommended fixes

  • #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.
glennmichael123 9 days ago

Many broken behaviors fixed. Closed across 6 commits on main (9983c7f → 426f12d):

FindingCommitNote
#10 whereNotBetween broken boolean tree9983c7fReplaced flat-clause flow with a single raw: '(col < ? OR col > ?)' so the disjunction stays parenthesised.
#15 insertOrIgnore hardcoded ON CONFLICT DO NOTHING (MySQL syntax error)c99c040Now dispatches on dialect: INSERT IGNORE INTO for MySQL, ON CONFLICT DO NOTHING for Postgres/SQLite.
#16 upsert quoted EXCLUDED.col as one identifier; broken on MySQL entirelyc99c040Builds the SET list as a raw fragment per dialect: col = EXCLUDED.col for Postgres/SQLite (with proper per-column quoting), col = VALUES(col) for MySQL.
#17 advisoryLock silent no-op on MySQL / SQLite426f12dMySQL implemented via GET_LOCK(name, timeout) (-1 for blocking, 0 for try). SQLite throws on advisoryLock (loud failure for distributed-locking callers) and returns false on tryAdvisoryLock (graceful degradation).
#19 onlyTrashed regex-replaced WHERE in subqueriesc99c040Replaced with a paren-depth scanner that finds the outermost WHERE (depth 0, whitespace-bounded) and splices there.
#20 withCTE / withRecursive / joinSub identifiers raw4b0f857All routed through validateIdentifier (or validateQualifiedIdentifier for table-qualified ON columns).
#25 limit(n) / offset(n) accepted NaN / negativec99c040Both methods enforce Number.isFinite(n) && n >= 0 && Number.isInteger(n); throw on bad input.
#26 count() returned one group's count after groupByc99c040Wraps in SELECT COUNT(*) AS c FROM (<original>) AS _bqb_count_sub when GROUP BY present.
#29 whereDate String(new Date()) produced unparseable SQL2e81638Coerces Date → toISOString(); rejects other shapes.
#30 pluck(column, key) silently overwrote on duplicate keys248fa8bThrows with a clear message on the first collision so callers see the assumption-break.

Still open

  • #11 selectFromSub returns a builder whose many methods are silent no-ops. Requires implementing the methods or removing them from the type interface — a substantial refactor.
  • #12 paginate count + data race. Needs transactional wrapping.
  • #13 applyWhereCondition IN (?) arrays (in selectFromSub) — depends on #11.
  • #14 Transaction isolation/read-only SQL cross-driver. Each dialect speaks a different syntax for SET TRANSACTION; needs dialect dispatch + a "not supported" path for SQLite.
  • #18 setConfig module-global shared state. Architecture change; touches every Database instance consumer.
  • #31 eagerLoadRelations runs one query per relation per model. Perf optimization — needs a dataloader-style batch fetch.

All 1150 tests pass after every commit.

glennmichael123 9 days ago

Substantial closing-out across 7 commits. All audit-flagged correctness bugs that fix cleanly in a single commit now have a fix shipped; the remaining items need a future major-version refactor and have warnings/JSDoc surfacing the limitation today.

FindingCommitStatus
#10 whereNotBetween broken boolean tree9983c7f✅ Fixed
#11 selectFromSub no-op methods9e57ce0✅ Each unsupported method now throws with migration guidance
#12 paginate race8f4e1b6⚠ Args validated, race documented in JSDoc; transactional fix deferred to major-version refactor (needs tx param to compose with caller's own transactions)
#13 IN (?) arrays in selectFromSub9e57ce0✅ Unreachable now — selectFromSub throws on the path that hit this
#14 transaction isolation cross-driverb3b6037✅ Dialect dispatch for Postgres/MySQL; SQLite throws for unsupported combinations
#15 insertOrIgnore hardcoded ON CONFLICTc99c040✅ MySQL gets INSERT IGNORE
#16 upsert EXCLUDED.col quoting + MySQLc99c040✅ Per-dialect raw fragments
#17 advisoryLock no-op on MySQL/SQLite426f12d✅ MySQL impl via GET_LOCK; SQLite throws (graceful tryAdvisoryLock returns false)
#18 setConfig module-global8f4e1b6⚠ Once-per-conflict warning + JSDoc; per-instance config deferred to major-version refactor
#19 onlyTrashed regex corruptionc99c040✅ Paren-depth scanner finds outermost WHERE
#20 withCTE/joinSub identifier raw4b0f857✅
#25 LIMIT/OFFSET non-integerc99c040✅ Args validated
#26 count() after groupByc99c040✅ Wraps in subquery
#29 whereDate Date coercion2e81638✅
#30 pluck collision248fa8b✅ Throws on duplicate
#31 eagerLoad N+1(no-op)Already batched per-relation; audit description over-stated the issue

Tests: 1150/1150 passing after every commit.

Remaining items requiring future work

  • #12 — a transactional paginate that accepts an optional tx parameter (so callers using their own transaction can pass it through). Touches the public signature; better as part of a v2.
  • #18 — per-instance config. Touches every getPlaceholder / getBunSql / dialect dispatch call site. Significant refactor.

Both are surfaced via runtime warnings + JSDoc today so consumers know the limitation; closing this issue for the bug-fix work.

Sign in to comment on this issue.