fix(sql): support named parameters in Bun.SQL unsafe() method #23375
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
1 participant
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