sql: throw validation errors for degenerate helper inputs instead of raw TypeErrors and engine errors#32156
sql: throw validation errors for degenerate helper inputs instead of raw TypeErrors and engine errors#32156robobun wants to merge 2 commits into
Conversation
|
Updated 3:52 PM PT - Jun 12th, 2026
✅ @robobun, your commit e533f884a915060facf30eca890d8a38b52f6eb0 passed in 🧪 To try this PR locally: bunx bun-pr 32156That installs a local version of the PR into your bun-32156 --bun |
WalkthroughThis PR adds input validation to SQL helper functions to reject null and undefined items in WHERE IN, INSERT, and UPDATE operations, throwing explicit SyntaxError at query normalization time. It refactors SQLite's empty-update detection to use a boolean flag instead of case-sensitive string matching, and provides comprehensive test coverage including cross-adapter and regression tests. ChangesSQL helper input validation and error handling
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/internal/sql/shared.ts`:
- Around line 194-200: buildDefinedColumnsAndQuery currently only checks for
null/undefined when items is an array, so a single-item call (e.g., sql(null,
"id")) will hit items[column] and throw an unhandled TypeError; update the
validation to also reject non-array null/undefined inputs by adding a guard
before any indexing: if (! $isArray(items) && (items == null)) throw new
SyntaxError("Cannot use null or undefined as an item in INSERT helper"); ensure
this check is placed before the code that accesses items[column] so both array
and single-item flows produce the same explicit SyntaxError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 06cbcca9-8fd6-4085-acc3-b38ac1fcea52
📒 Files selected for processing (6)
src/js/internal/sql/mysql.tssrc/js/internal/sql/postgres.tssrc/js/internal/sql/shared.tssrc/js/internal/sql/sqlite.tstest/js/sql/sql-helpers-validation.test.tstest/js/sql/sqlite-sql.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward null guards converting raw TypeErrors into clear validation errors, plus aligning sqlite's empty-update check with the postgres/mysql hasValues pattern.
Extended reasoning...
Overview
This PR adds input validation to the SQL helper normalization paths across all three adapters (postgres, mysql, sqlite) and shared.ts. The changes are:
- WHERE IN keyed helper:
nullitems now throw aSyntaxErrorinstead of a rawTypeError: null is not an objectfromvalue[columns[0]]. - UPDATE helper:
null/undefineditems throw aSyntaxErrorbefore property access. - INSERT helper (
buildDefinedColumnsAndQueryin shared.ts): array items are scanned fornull/undefinedup front. - sqlite: replaces the case-sensitive
query.endsWith("SET ")check with the samehasValuesflag postgres/mysql already use, and drops a deadtypeof columnValue === "undefined"branch that was unreachable after the precedingcontinue. - New test file (
sql-helpers-validation.test.ts) covers all adapters viadescribe.each, plus sqlite-only behavior-preservation tests.
Security risks
None. These are pure validation guards on error paths — all values still flow through the existing parameter binding/escaping. No new string interpolation into SQL. The sqlite hasValues change only affects when a validation error fires, not what SQL is emitted for valid inputs.
Level of scrutiny
Low. Every change converts a previously-broken path (raw TypeError or malformed SQL reaching the engine) into a clear validation error; happy paths are untouched. The sqlite refactor brings it to parity with the identical code in postgres.ts/mysql.ts, so it's pattern-following rather than novel logic. The diff is ~30 lines of logic across 4 files.
Other factors
- Bug hunting system found no issues.
- CodeRabbit's one suggestion (non-array guard in
buildDefinedColumnsAndQuery) was correctly rebutted as dead code (call sites always wrap to[value]) and withdrawn; the thread is resolved. - CI failures in Build #62031 are infrastructure issues (agent creation failures, LTO linking on musl) unrelated to this diff.
- No CODEOWNERS for
src/js/internal/sql/. - Test coverage is thorough: validation matrix across all three adapters plus sqlite execution tests confirming lowercase
setwith defined values still works and thatundefined/non-keyednullstill bind NULL.
…case-sensitive empty update check in sqlite
A null item in a keyed WHERE IN helper, and null or undefined items in
INSERT and keyed UPDATE helpers, surfaced a raw TypeError from the
property access inside normalizeQuery. They now throw SyntaxErrors that
name the misuse, consistent with the other helper validation errors.
The sqlite adapter's throwIfUpdateEmpty detected an empty update helper
with a case-sensitive query.endsWith("SET ") check, so only the
uppercase spelling threw the intended error while lowercase reached the
engine as malformed SQL. It now checks the hasValues flag like the
BaseSQLAdapter default used by postgres and mysql.
Fixes #32155
robobun
force-pushed
the
farm/c52df286/sql-helper-validation-errors
branch
from
1cf2f3a to
19d9753
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/internal/sql/shared.ts`:
- Around line 1141-1144: Loops that iterate over this.connections dereference
connection.state without guarding for sparse/undefined slots, causing potential
TypeError during pool transitions; update each loop in methods isConnected,
flush, `#close`, and connect to first check that the connection variable is not
null/undefined (e.g., if (!connection) continue) before accessing
connection.state or other properties, and apply the same guard to the other
similar loops noted (around the blocks represented by the snippet and the other
occurrences) so sparse array holes are skipped safely.
- Around line 1218-1223: The timeout validation for close({ timeout }) currently
converts to Number and only rejects NaN/out-of-range but allows fractional
values; update the predicate to require an integer (e.g., use
Number.isInteger(timeout) or an equivalent check) after Number(timeout) and
throw the same $ERR_INVALID_ARG_VALUE("options.timeout", timeout, "must be a
non-negative integer less than 2^31") when the value is not an integer,
negative, >= 2**31, or NaN so fractional timeouts like 0.5 are rejected; modify
the block that handles timeout (the variable named timeout in this function) to
include that integer check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 377b5fc1-97fc-4434-ad5b-23c986754d77
📒 Files selected for processing (3)
src/js/internal/sql/shared.tssrc/js/internal/sql/sqlite.tstest/js/sql/sql-helpers-validation.test.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/internal/sql/shared.ts`:
- Around line 1141-1144: Loops that iterate over this.connections dereference
connection.state without guarding for sparse/undefined slots, causing potential
TypeError during pool transitions; update each loop in methods isConnected,
flush, `#close`, and connect to first check that the connection variable is not
null/undefined (e.g., if (!connection) continue) before accessing
connection.state or other properties, and apply the same guard to the other
similar loops noted (around the blocks represented by the snippet and the other
occurrences) so sparse array holes are skipped safely.
- Around line 1218-1223: The timeout validation for close({ timeout }) currently
converts to Number and only rejects NaN/out-of-range but allows fractional
values; update the predicate to require an integer (e.g., use
Number.isInteger(timeout) or an equivalent check) after Number(timeout) and
throw the same $ERR_INVALID_ARG_VALUE("options.timeout", timeout, "must be a
non-negative integer less than 2^31") when the value is not an integer,
negative, >= 2**31, or NaN so fractional timeouts like 0.5 are rejected; modify
the block that handles timeout (the variable named timeout in this function) to
include that integer check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 377b5fc1-97fc-4434-ad5b-23c986754d77
📒 Files selected for processing (3)
src/js/internal/sql/shared.tssrc/js/internal/sql/sqlite.tstest/js/sql/sql-helpers-validation.test.ts
🛑 Comments failed to post (2)
src/js/internal/sql/shared.ts (2)
1141-1144:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard sparse pool slots before reading
connection.state.Line 1118 already documents that
this.connectionscan contain unassigned holes during startup/re-entrant release, but these loops still dereferenceconnection.statewithout checking forundefined. That can throw a runtimeTypeErrorinisConnected(),flush(),#close(), orconnect()while the pool is transitioning.💡 Suggested fix
isConnected() { if (this.readyConnections.size > 0) { return true; } if (this.poolStarted) { const pollSize = this.connections.length; for (let i = 0; i < pollSize; i++) { const connection = this.connections[i]; + if (!connection) continue; if (connection.state === PooledConnectionState.connected) { return true; } } } return false; } flush() { if (this.closed) { return; } if (this.poolStarted) { const pollSize = this.connections.length; for (let i = 0; i < pollSize; i++) { const connection = this.connections[i]; + if (!connection) continue; if (connection.state === PooledConnectionState.connected) { connection.connection?.flush(); } } } } async `#close`() { // ... if (this.poolStarted) { this.poolStarted = false; const pollSize = this.connections.length; for (let i = 0; i < pollSize; i++) { const connection = this.connections[i]; + if (!connection) { + // `@ts-ignore` + this.connections[i] = null; + continue; + } switch (connection.state) { case PooledConnectionState.pending: case PooledConnectionState.connected: { // ... } } // `@ts-ignore` this.connections[i] = null; } } // ... } connect(onConnected: (err: Error | null, result: any) => void, reserved: boolean = false) { // ... if (this.poolStarted) { const pollSize = this.connections.length; for (let i = 0; i < pollSize; i++) { const connection = this.connections[i]; + if (!connection) { + all_closed = false; + continue; + } if (connection.state === PooledConnectionState.closed) { // ...Also applies to: 1156-1160, 1183-1187, 1284-1289
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js/internal/sql/shared.ts` around lines 1141 - 1144, Loops that iterate over this.connections dereference connection.state without guarding for sparse/undefined slots, causing potential TypeError during pool transitions; update each loop in methods isConnected, flush, `#close`, and connect to first check that the connection variable is not null/undefined (e.g., if (!connection) continue) before accessing connection.state or other properties, and apply the same guard to the other similar loops noted (around the blocks represented by the snippet and the other occurrences) so sparse array holes are skipped safely.
1218-1223:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
close({ timeout })allows fractional values despite an integer-only contract.Line 1222 says the value “must be a non-negative integer,” but the predicate only rejects NaN/range and currently accepts floats (e.g.
0.5).💡 Suggested fix
- if (timeout > 2 ** 31 || timeout < 0 || timeout !== timeout) { + if (!Number.isInteger(timeout) || timeout > 2 ** 31 || timeout < 0) { throw $ERR_INVALID_ARG_VALUE("options.timeout", timeout, "must be a non-negative integer less than 2^31"); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let timeout = options?.timeout; if (timeout) { timeout = Number(timeout); if (!Number.isInteger(timeout) || timeout > 2 ** 31 || timeout < 0) { throw $ERR_INVALID_ARG_VALUE("options.timeout", timeout, "must be a non-negative integer less than 2^31"); }🧰 Tools
🪛 Biome (2.4.16)
[error] 1221-1221: This comparison uses the same expression on both sides.
(lint/suspicious/noSelfCompare)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js/internal/sql/shared.ts` around lines 1218 - 1223, The timeout validation for close({ timeout }) currently converts to Number and only rejects NaN/out-of-range but allows fractional values; update the predicate to require an integer (e.g., use Number.isInteger(timeout) or an equivalent check) after Number(timeout) and throw the same $ERR_INVALID_ARG_VALUE("options.timeout", timeout, "must be a non-negative integer less than 2^31") when the value is not an integer, negative, >= 2**31, or NaN so fractional timeouts like 0.5 are rejected; modify the block that handles timeout (the variable named timeout in this function) to include that integer check.
|
The two findings from the latest CodeRabbit review (unguarded sparse pool-slot dereferences in the BasePooledConnection loops, and close({ timeout }) accepting fractional values despite its integer error message) are in the pool plumbing that #32145 consolidated into shared.ts. They entered this review's commit range via the rebase; this PR's diff only touches normalizeQuery/buildDefinedColumnsAndQuery and the sqlite throwIfUpdateEmpty hook, and both items reproduce on current main. Tracked in #32198. |
sqlite previously executed the helper-last form of a literal assignment plus an all-undefined helper while throwing for the helper-first form; postgres and mysql threw for both. The hasValues check makes all three adapters throw for both orders, and a helper with defined values mixed with literal assignments keeps working.
There was a problem hiding this comment.
LGTM — the new empty update helper throws even alongside a literal assignment test pins the behavior I flagged, so the sqlite change is now explicit and consistent with the other adapters.
Extended reasoning...
Overview
Three small null/undefined guards in src/js/internal/sql/shared.ts (buildDefinedColumnsAndQuery, the keyed WHERE IN branch, and the keyed UPDATE branch of normalizeQuery) plus a one-line change to SQLiteAdapter.throwIfUpdateEmpty to gate on hasValues instead of a case-sensitive query.endsWith("SET "). A new test file covers all three adapters via describe.each and adds sqlite-only behavior-preservation tests.
Security risks
None. The guards reject degenerate inputs earlier with clearer errors; no new query-building surface, no identifier/value handling changes.
Level of scrutiny
Low–moderate. The shared normalizeQuery is on the hot path for all three adapters, but the diff is four narrowly-scoped == null checks and one hook body swap that mirrors the existing BaseSQLAdapter default verbatim. The PR description traces each guard to a concrete repro and the test matrix exercises both the new throws and the behaviors that must keep working (undefined items / null column values / non-keyed null still bind NULL; lowercase set with defined values still updates).
Other factors
- Bug hunter found nothing.
- CodeRabbit's non-array-input suggestion was correctly rebutted (call sites always wrap to an array) and withdrawn.
- My earlier heads-up about the literal-assignment + empty-helper edge case was addressed in commit e533f88: the author added a cross-adapter test pinning the throw for both helper-first and helper-last orders with an explanatory comment, plus a positive test confirming the literal+helper combo still works when the helper has defined values. That makes the cross-adapter alignment an explicit, tested choice rather than an undocumented side effect. The error message wording is still slightly misleading for that one case, but it's pre-existing for postgres/mysql and not worth blocking on.
- The two unrelated CodeRabbit findings on pool plumbing came in via the rebase onto #32145, reproduce on main, and are tracked in #32198.
1 participant
Fixes #32155
Repro
Both reproduce on 1.4.0 and main, on all three adapters for the first case.
Cause
The keyed WHERE IN branch of the shared
normalizeQuery(src/js/internal/sql/shared.ts) only guards items againstundefinedbefore readingvalue[columns[0]], so anullitem throws a raw TypeError. The same property-access-on-null pattern exists in the INSERT item scan (buildDefinedColumnsAndQuery) and in the keyed UPDATE branch, sosql([{...}, null])for INSERT andsql(null, "col")/sql([undefined], "col")for UPDATE hit the same TypeError.The sqlite adapter's
throwIfUpdateEmptyhook detects an empty update helper withquery.endsWith("SET "), a case-sensitive check against the user's original spelling. Command detection is case-insensitive, so lowercasesettakes the helper path but misses the check and the malformed SQL reaches the engine. postgres and mysql use theBaseSQLAdapterdefault, which gates on thehasValuesflag instead.Fix
All in
src/js/internal/sql/shared.tsplus the sqlite hook (after #32145 consolidatednormalizeQuery, these are single sites covering all three adapters):nullitem now throwsSyntaxError: Cannot use null as an item in WHERE IN helper with a column.undefineditems keep binding NULL (existing behavior), as donullitems in the non-keyed formsql([null])andnullvalues under the key (sql([{ id: null }], "id")).buildDefinedColumnsAndQuery):null/undefineditems throwSyntaxError: Cannot use null or undefined as an item in INSERT helper. Previously both were raw TypeErrors.null/undefineditem throwsSyntaxError: Cannot use null or undefined as an item in UPDATE helper.SQLiteAdapter.throwIfUpdateEmptynow checks thehasValuesflag like theBaseSQLAdapterdefault (sqlite implements the adapter interface standalone, so the override stays but with the correct body), making the empty-helper error spelling-independent.One deliberate sqlite behavior change beyond the lowercase fix, raised in review: a literal assignment followed by an all-undefined helper,
UPDATE t SET updated_at = CURRENT_TIMESTAMP, ${sql({ name: undefined })}, previously executed on sqlite (the trailing comma was stripped and the suffix check missed). The old behavior was order-dependent (the helper-first form already threw), and postgres/mysql throw for both orders, so sqlite now throws for both as well. A helper with defined values mixed with literal assignments keeps working; both are pinned by tests.The mysql
ON DUPLICATE KEY UPDATEsuffix check (isUpsertUpdate) has a similar case-sensitivity problem, tracked separately in #32035 (fix in #32040), so it is not touched here.Tests
test/js/sql/sql-helpers-validation.test.ts(new): the validation contract is identical across the three adapters, so it is tested as onedescribe.eachmatrix (null item in keyed WHERE IN, null/undefined items in INSERT and keyed UPDATE, empty update helper for lowercaseset, uppercaseSET, the helper-emitted SET form, and the empty helper alongside a literal assignment in both orders), plus sqlite-only tests for the behaviors that must keep working (lowercasesetwith defined values updates, a helper with defined values alongside a literal assignment updates, undefined items / null column values / non-keyed null still bind NULL). Normalization runs when a query is first awaited, before any connection is attempted, so the postgres and mysql rows run without a server; sqlite uses:memory:.On the unfixed build, 11 of the 18 tests fail (raw
TypeError: null is not an objectinstead of the validation errors, and the empty-helper updates reaching the engine); the other 7 are behavior-preservation and cross-adapter parity guards. All 18 pass with the fix.Rebase note
Originally the guards were duplicated across
sqlite.ts/postgres.ts/mysql.ts; after #32145 landed (consolidatingnormalizeQueryintoshared.ts), the branch was rebased and the same fix re-applied at the now-shared sites, shrinking the diff toshared.ts+ the sqlitethrowIfUpdateEmptyhook.