Repository navigation
fix(sql): support named parameters in Bun.SQL unsafe() method - #23375
savannahar68 wants to merge 6 commits into
Conversation
Fixes issue where named parameters with :, $, or @ prefixes were not working in Bun.SQL's unsafe() method. The method was using $apply() which wrapped objects in arrays instead of passing them directly to the underlying bun:sqlite statement methods. Changes: - Updated SQLiteQueryHandle to accept objects in addition to arrays - Fixed parameter passing to use direct calls for objects - Added tests for named parameters with strict and non-strict modes Fixes oven-sh#22799
WalkthroughAdds support for object-based named-parameter binding for SQLite by widening internal value types to accept arrays or maps, updating the SQLiteQueryHandle and createQueryHandle signatures, and branching execution paths for SELECT vs non-SELECT queries. Adds tests covering named-parameter binding across strict/non-strict modes and different prefixes. Changes
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3)src/js/internal/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/js/internal/**/*.{ts,js}📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Files:
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Files:
🧬 Code graph analysis (1)src/js/internal/sql/sqlite.ts (2)
🔇 Additional comments (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/internal/sql/sqlite.ts (1)
242-260: Always finalize prepared statements (move finalize into a finally block)If any call throws before Line 258, stmt.finalize() is skipped, leaking a prepared statement. Wrap the SELECT branch in try/finally.
Apply this diff:
- const stmt = db.prepare(sql); - let result: unknown[] | undefined; - - if (mode === SQLQueryResultMode.values) { - result = $isArray(values) ? stmt.values.$apply(stmt, values) : stmt.values(values); - } else if (mode === SQLQueryResultMode.raw) { - result = $isArray(values) ? stmt.raw.$apply(stmt, values) : stmt.raw(values); - } else { - result = $isArray(values) ? stmt.all.$apply(stmt, values) : stmt.all(values); - } - - const sqlResult = $isArray(result) ? new SQLResultArray(result) : new SQLResultArray([result]); - - sqlResult.command = commandToString(command, parsedInfo.lastToken); - sqlResult.count = $isArray(result) ? result.length : 1; - - stmt.finalize(); - query.resolve(sqlResult); + const stmt = db.prepare(sql); + try { + let result: unknown[] | undefined; + + if (mode === SQLQueryResultMode.values) { + result = $isArray(values) ? stmt.values.$apply(stmt, values) : stmt.values.$call(stmt, values); + } else if (mode === SQLQueryResultMode.raw) { + result = $isArray(values) ? stmt.raw.$apply(stmt, values) : stmt.raw.$call(stmt, values); + } else { + result = $isArray(values) ? stmt.all.$apply(stmt, values) : stmt.all.$call(stmt, values); + } + + const sqlResult = $isArray(result) ? new SQLResultArray(result) : new SQLResultArray([result]); + sqlResult.command = commandToString(command, parsedInfo.lastToken); + sqlResult.count = $isArray(result) ? result.length : 1; + query.resolve(sqlResult); + } finally { + stmt.finalize(); + }As per coding guidelines
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/js/internal/sql/sqlite.ts(4 hunks)test/js/sql/sqlite-sql.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/sql/sqlite-sql.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/sql/sqlite-sql.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/sql/sqlite-sql.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/js/sql/sqlite-sql.test.ts
test/js/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests for specific features under
test/js/by module
Files:
test/js/sql/sqlite-sql.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/js/sql/sqlite-sql.test.ts
src/js/internal/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place internal modules not exposed to users under src/js/internal/
Files:
src/js/internal/sql/sqlite.ts
src/js/internal/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Place internal-only modules under internal/
Files:
src/js/internal/sql/sqlite.ts
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use require() with string literals only (no dynamic requires)
Do not use ESM import syntax; write modules as CommonJS with export default { ... }
Export via export default {} for modules
Use .$call and .$apply; never use .call or .apply
Prefer JSC intrinsics/private $ APIs for performance (e.g., $Array.from, map.$set, $newArrayWithSize, $debug, $assert)
Validate callbacks with $isCallable and throw $ERR_INVALID_ARG_TYPE with the correct parameter name and expected type
Use process.platform and process.arch for platform detection (rely on inlining/dead-code elimination)
Files:
src/js/internal/sql/sqlite.ts
🧬 Code graph analysis (2)
test/js/sql/sqlite-sql.test.ts (1)
src/js/internal-for-testing.ts (1)
SQL(17-17)
src/js/internal/sql/sqlite.ts (2)
src/js/bun/sqlite.ts (1)
values(201-203)src/js/internal/sql/query.ts (1)
values(262-271)
🔇 Additional comments (6)
src/js/internal/sql/sqlite.ts (3)
212-219: Type-widening to support named params looks goodAccepting unknown[] | Record<string, unknown> and threading it through the handle is correct for named bindings. No issues.
349-353: API surface update is sensible; minor nit on default + nullish-coalescingSignature accepts unknown[] | Record<string, unknown> | undefined | null with default [] and then coalesces to [] again. This is fine (it preserves null→[] behavior), just noting the redundancy.
Please confirm other adapters that call createQueryHandle won’t rely on null vs [] distinction (should be none for SQLite).
246-251: Use .$call for object-binding paths to follow intrinsics guideline and avoid slow pathsDirect calls (stmt.values(obj)/raw(obj)/all(obj)) should use .$call to match how arrays use .$apply and to align with internal calling conventions.
As per coding guidelines
- result = $isArray(values) ? stmt.values.$apply(stmt, values) : stmt.values(values); + result = $isArray(values) ? stmt.values.$apply(stmt, values) : stmt.values.$call(stmt, values);- result = $isArray(values) ? stmt.raw.$apply(stmt, values) : stmt.raw(values); + result = $isArray(values) ? stmt.raw.$apply(stmt, values) : stmt.raw.$call(stmt, values);- result = $isArray(values) ? stmt.all.$apply(stmt, values) : stmt.all(values); + result = $isArray(values) ? stmt.all.$apply(stmt, values) : stmt.all.$call(stmt, values);⛔ Skipped due to learnings
Learnt from: CR PR: oven-sh/bun#0 File: src/js/CLAUDE.md:0-0 Timestamp: 2025-10-04T09:52:49.414Z Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use .$call and .$apply; never use .call or .applytest/js/sql/sqlite-sql.test.ts (3)
1359-1376: Good coverage: strict mode named parametersValidates insert and select with object bindings; closes the DB. Looks solid.
1378-1391: Prefix variations well coveredVerifies :, $, @ prefixes; order-agnostic assertion via sort is good.
1392-1407: Non‑strict mode behavior capturedRequiring prefixes in keys when strict is false is reflected here; assertions are clear.
Consider adding one small test exercising named bindings with
.values()and.raw()result modes to cover those branches too (e.g., SELECT with:idreturning.values()and.raw()).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/js/internal/sql/sqlite.ts (1)
262-264: Use .$call for object branch to follow fast-call conventions.For consistency with the array branch and to follow coding guidelines, use
db.run.$call(db, sql, values)for object parameter branch in non-SELECT query handling.As per coding guidelines
Apply this diff:
- const changes = $isArray(values) - ? db.run.$apply(db, [sql].concat(values)) - : db.run(sql, values); + const changes = $isArray(values) + ? db.run.$apply(db, [sql].concat(values)) + : db.run.$call(db, sql, values);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/js/internal/sql/sqlite.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/js/internal/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place internal modules not exposed to users under src/js/internal/
Files:
src/js/internal/sql/sqlite.ts
src/js/internal/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Place internal-only modules under internal/
Files:
src/js/internal/sql/sqlite.ts
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use require() with string literals only (no dynamic requires)
Do not use ESM import syntax; write modules as CommonJS with export default { ... }
Export via export default {} for modules
Use .$call and .$apply; never use .call or .apply
Prefer JSC intrinsics/private $ APIs for performance (e.g., $Array.from, map.$set, $newArrayWithSize, $debug, $assert)
Validate callbacks with $isCallable and throw $ERR_INVALID_ARG_TYPE with the correct parameter name and expected type
Use process.platform and process.arch for platform detection (rely on inlining/dead-code elimination)
Files:
src/js/internal/sql/sqlite.ts
🧬 Code graph analysis (1)
src/js/internal/sql/sqlite.ts (2)
src/js/bun/sqlite.ts (1)
values(201-203)src/js/internal/sql/query.ts (1)
values(262-271)
🔇 Additional comments (2)
src/js/internal/sql/sqlite.ts (2)
213-213: LGTM! Type changes correctly enable object parameter support.The field and constructor signature changes properly widen the type from
unknown[]tounknown[] | Record<string, unknown>, allowing SQLite queries to accept both positional parameters (arrays) and named parameters (objects).Also applies to: 216-216
351-356: LGTM! Method signature correctly handles object parameters and null values.The
createQueryHandlesignature properly:
- Accepts
unknown[] | Record<string, unknown> | undefined | nullto support both parameter types- Defaults to
[]when no argument is provided- Uses the nullish coalescing operator (
??) to handle explicitundefinedornullvalues
|
@robobun revive this PR and redo it |
|
Redone against current main in #37109 (original commits credited via co-author), reviews addressed and ready for a maintainer. The remaining CI red there is build-step download failures on lanes unrelated to this change; the diff itself is green. |
|
Closing this in favor of #37109, which is this change redone against current main (as Jarred asked for above). Your commits are credited there with a #37109 also covers the |
Redo of #23375 by @savannahar68 against current main, as requested there. Fixes #22799. ## Problem `Bun.SQL`'s `unsafe()` only bound array values. Passing an object of named parameters to a SELECT silently returned no rows: ```ts const sql = new SQL({ adapter: "sqlite", filename: ":memory:", strict: true }); await sql`CREATE TABLE tbl (id INTEGER, name TEXT)`; await sql.unsafe("INSERT INTO tbl VALUES (:id, :name)", { id: 1, name: "Alice" }); await sql.unsafe("SELECT * FROM tbl WHERE name = :name", { name: "Alice" }); // before: [] (no error) // after: [{ id: 1, name: "Alice" }] ``` ## Cause `SQLiteQueryHandle.run` spread values with `$apply`. A plain object is an array-like of length 0 to `apply`, so `stmt.all()` ran with nothing bound and SQLite treated the named placeholders as NULL, returning empty results instead of erroring. ## Fix In `src/js/internal/sql/sqlite.ts`, detect object bindings and pass them as a single argument to the underlying `bun:sqlite` methods (`stmt.all`/`stmt.values`/`stmt.raw`/`db.run`), which already support `:name`, `$name`, and `@name` placeholders. Arrays keep spreading into positional arguments. Statement execution is now wrapped in try/finally so a failed bind (for example a missing named parameter in strict mode) still finalizes the prepared statement. The `unsafe`/`file` type signatures are widened and the docs note that this is SQLite only and that object keys keep their prefix unless the connection sets `strict: true`. Named parameters stay SQLite only: Postgres and MySQL already reject non-array values natively (`src/sql_jsc/shared/QueryCtorArgs.rs`), and those adapters are unchanged. ## Verification Eight new tests in `test/js/sql/sqlite-sql.test.ts`: strict mode, all three placeholder prefixes, non-strict mode with prefixed keys for all three prefixes, `values()`/`raw()` result modes (raw bytes checked), the missing-binding error path, UPDATE/DELETE change counts, `unsafe()` inside a transaction, and `sql.file()` on both strict and default connections. All eight fail on main (empty results) and pass with this change; the rest of the file and the bun-types integration test pass. The pre-existing 10k-insert finalization test uses 1000 iterations on debug builds only, so the file fits the default timeout under ASAN. <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 5 · 4 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 8 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/sql/sqlite-sql.test.ts bun test v1.4.0 (fa34508) test/js/sql/sqlite-sql.test.ts: (pass) Connection & Initialization > common default connection strings > should parse common connection strings [113.02ms] (pass) Connection & Initialization > should connect to in-memory SQLite database [12.32ms] (pass) Connection & Initialization > should connect to file-based SQLite database [38.31ms] (pass) Connection & Initialization > should handle connection with options object [101.41ms] (pass) Connection & Initialization > onconnect and onclose callbacks are invoked for SQLite [16.96ms] (pass) Connection & Initialization > onconnect receives Error when open fails (readonly non-existent) [26.93ms] (pass) Connection & Initialization > should create database file if it doesn't exist [42.28ms] (pass) Connection & Initialization > should work with relative paths [33.10ms] (pass) Connection & Initialization > Environment Variable Handling > should use DATABASE_URL for SQLite when it's a SQLite URL [44.04ms] (pass) Connection & Initialization ... (truncated) release without fix: 8 FAILED bun test v1.4.0-canary.1 (da3851e) test/js/sql/sqlite-sql.test.ts: (pass) Connection & Initialization > common default connection strings > should parse common connection strings [2.16ms] (pass) Connection & Initialization > should connect to in-memory SQLite database [0.80ms] (pass) Connection & Initialization > should connect to file-based SQLite database [1.14ms] (pass) Connection & Initialization > should handle connection with options object [8.09ms] (pass) Connection & Initialization > onconnect and onclose callbacks are invoked for SQLite [0.43ms] (pass) Connection & Initialization > onconnect receives Error when open fails (readonly non-existent) [4.12ms] (pass) Connection & Initialization > should create database file if it doesn't exist [26.99ms] (pass) Connection & Initialization > should work with relative paths [19.61ms] (pass) Connection & Initialization > Environment Variable Handling > should use DATABASE_URL for SQLite when it's a SQLite URL [14.91ms] (pass) Connection & Initialization > Environment Variable Handling > should handle DATABASE_URL with :memory: [0.73ms] (pass) Connection & Initialization > Environment Variable Handling > should han ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/sql/sqlite-sql.test.ts bun test v1.4.0 (fa34508) test/js/sql/sqlite-sql.test.ts: (pass) Connection & Initialization > common default connection strings > should parse common connection strings [118.63ms] (pass) Connection & Initialization > should connect to in-memory SQLite database [13.44ms] (pass) Connection & Initialization > should connect to file-based SQLite database [53.40ms] (pass) Connection & Initialization > should handle connection with options object [106.23ms] (pass) Connection & Initialization > onconnect and onclose callbacks are invoked for SQLite [17.97ms] (pass) Connection & Initialization > onconnect receives Error when open fails (readonly non-existent) [27.97ms] (pass) Connection & Initialization > should create database file if it doesn't exist [47.47ms] (pass) Connection & Initialization > should work with relative paths [113.79ms] (pass) Connection & Initialization > Environment Variable Handling > should use DATABASE_URL for SQLite when it's a SQLite URL [67.37ms] (pass) Connection & Initialization ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision fa34508 features baseline 22 deps, 107 codegen, 1176 objects in 736ms ninja: Entering directory `/workspace/bun/build/release' [1/1194] gen bake.{client,server,error}.js -> bake.client.js, bake.server.js, bake.error.js [2/1194] cc obj/vendor/zlib/adler32.c.o [3/1194] cc obj/vendor/zlib/compress.c.o [4/1194] cc obj/vendor/zlib/crc32.c.o [5/1194] gen JSBuffer.lut.h Generating /workspace/bun/build/release/codegen/JSBuffer.lut.h from /workspace/bun/src/jsc/bindings/JSBuffer.cpp [6/1194] fetch cares [cares] up to date [7/1194] cc obj/vendor/zlib/crc32_braid_comb.c.o [8/1194] gen NodeModuleModule.lut.h Generating /workspace/bun/build/release/codegen/NodeModuleModule.lut.h from /workspace/bun/src/jsc/modules/NodeModuleModule.cpp [9/1194] cc obj/vendor/zlib/deflate_huff.c.o [10/1194] cc obj/vendor/zlib/deflate_quick.c.o [11/1194] cc obj/vendor/zlib/deflate_medium.c.o [12/1194] cc obj/vendor/zlib/deflate_slow.c.o [13/1194] cc obj/vendor/zlib/deflate_rle.c.o [14/1194] cc obj/vendor/zli ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` docs/runtime/sql.mdx | 9 +++ packages/bun-types/sql.d.ts | 14 +++- src/js/internal/sql/sqlite.ts | 29 +++++---- test/js/sql/sqlite-sql.test.ts | 141 ++++++++++++++++++++++++++++++++++++++++- 4 files changed, 177 insertions(+), 16 deletions(-) ``` </details> **gate history** · 4 passed · 3 rejected · iteration 5 <details><summary>evidence per changed file</summary> ``` file reads edits tests docs/runtime/sql.mdx 2 4 0 packages/bun-types/sql.d.ts 2 3 0 src/js/internal/sql/sqlite.ts 5 5 0 test/js/sql/sqlite-sql.test.ts 12 12 0 ``` </details> <!-- robobun:evidence:end --> --------- Co-authored-by: Savan Nahar <savannahar68@gmail.com>
3 participants
Summary
unsafe()method to support named parameters with objects:,$, or@prefixes in both strict and non-strict modesDetails
The issue was in
src/js/internal/sql/sqlite.tswhereSQLiteQueryHandlewas using$apply()to pass parameters, which incorrectly wrapped objects in arrays instead of passing them directly to the underlyingbun:sqlitestatement methods.Changes:
Record<string, unknown>in addition to arrays$apply()wrapper$apply()as before (maintains backward compatibility)Testing:
strict: truemode (standard behavior):,$,@)strict: falsemode (requires prefix in keys)Test plan
bun bd test test/js/sql/sqlite-sql.test.tsFixes #22799