sql: support named parameters in SQLite unsafe() queries

[robobun]

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:

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.

---

[review] gate passed · iteration 5 · 4 files touched

fails on main (without fix)

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 (fa34508cc)

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 (da3851e57)

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)

passes on PR (with fix)

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 (fa34508cc)

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     fa34508ccd
  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)

diff hotspot

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(-)

gate history · 4 passed · 3 rejected · iteration 5

evidence per changed file

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

[coderabbitai]

Walkthrough

SQLite SQL.unsafe and SQL.file now accept named parameter records. SQLite execution supports prefixed and strict-mode bare keys across query modes, mutations, transactions, and SQL files. Documentation and tests cover the new behavior.

Changes

SQLite named parameters

Layer / File(s)	Summary
Named parameter contract packages/bun-types/sql.d.ts, docs/runtime/sql.mdx	SQL.unsafe and SQL.file accept positional arrays or named records. Documentation describes SQLite placeholder prefixes and strict-mode key handling.
SQLite execution and validation src/js/internal/sql/sqlite.ts, test/js/sql/sqlite-sql.test.ts	SQLite query handles use $apply for arrays and $call for named objects. Tests cover result modes, mutations, transactions, SQL files, and debug test sizing.

Suggested reviewers: alii, jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The implementation satisfies issue #22799 by adding SQLite named bindings while preserving positional arrays and excluding unsupported wire protocols.
Out of Scope Changes check	✅ Passed	The code, type, documentation, and test changes are directly related to SQLite named-parameter support and its verification.
Title check	✅ Passed	The title clearly summarizes the main change: SQLite named-parameter support for unsafe() queries.
Description check	✅ Passed	The description explains the problem, fix, scope, verification steps, and test coverage in sufficient detail.

---

_{Comment @coderabbitai help to get the list of available commands.}

[savannahar68]

lgtm

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🟡 test/js/sql/sqlite-sql.test.ts:2122-2123 — This raises the per-test timeout to 60s on a pre-existing test unrelated to the named-parameter feature, which conflicts with test/CLAUDE.md ("CRITICAL: Do not set a timeout on tests") and REVIEW.md ("Don't raise per-test timeouts to make a slow test pass; shrink the workload"). The idiomatic fix is to lower iterations (e.g. 10000 → ~2000) so the test finishes under the default in debug+ASAN — a 60s ceiling multiplies across retries on a genuine hang and hides future perf regressions in this codepath.

  Extended reasoning...

  What changed

  Commit 53173688 ("test: allow 60s for the 10k-insert statement finalization test") adds a 60_000 ms per-test timeout to the pre-existing "properly finalizes prepared statements" test in test/js/sql/sqlite-sql.test.ts, with the trailing comment // 10k awaited inserts need more than the 5s default under debug + ASAN. This test predates the PR and is unrelated to the named-parameter feature being implemented — it's a drive-by adjustment to make the file pass under debug+ASAN.

  Why this conflicts with repo rules

  Two repo guidelines cover this exact pattern:

  • test/CLAUDE.md:120: "CRITICAL: Do not set a timeout on tests. Bun already has timeouts."
  • REVIEW.md (Tests reviewers reject): "Don't raise per-test timeouts to make a slow test pass; shrink the workload."

  The comment justifying the timeout ("10k awaited inserts need more than the 5s default under debug + ASAN") is precisely the rationale the REVIEW.md rule says should trigger a workload reduction rather than a ceiling raise. There is minor tension with the root CLAUDE.md note that a per-test timeout is acceptable "only for the rare outlier", but the more-specific test/CLAUDE.md and the explicit REVIEW.md rule both point the other way here — this is not an outlier with an irreducible workload, it's a loop counter that can simply be smaller.

  Step-by-step

  1. The test at sqlite-sql.test.ts:2096 sets const iterations = 10000 and awaits an INSERT per iteration in a serial for loop.
  2. Under a debug+ASAN build (which CLAUDE.md notes runs 10-100× slower than release), 10k awaited round-trips through SQLiteQueryHandle.run → db.prepare/db.run exceed the 5s default.
  3. Rather than reducing iterations, the PR adds }, 60_000); at line 2123 to raise the ceiling.
  4. If this test ever genuinely hangs (e.g. a future regression in statement finalization), each attempt now burns 60s; multiplied across the retry policy this adds minutes to a red CI run.
  5. It also masks perf regressions: a change that makes each insert 5× slower would still pass under a 60s budget where it would have failed under the default.

  Impact

  Test-hygiene only — nothing in shipped runtime code is affected, so this doesn't block correctness. But it violates a rule the repo marks CRITICAL, and it's an unrelated drive-by change in a feature PR, which makes it easy to revert without touching the actual fix.

  Suggested fix

  Drop the 60_000 argument and the trailing comment, and reduce the workload instead — e.g. const iterations = 2000; (or whatever value comfortably clears the default timeout under debug+ASAN while still exercising "many" prepared statements). The test's purpose is to verify statements are finalized across many iterations; 2k iterations demonstrates that as well as 10k does.

[robobun]

Addressed the timeout finding in 206263b: dropped the 60s per-test timeout and shrank the insert loop to 1000 iterations, which fits the default timeout on a debug ASAN build while still exercising statement finalization.

[claude]

LGTM — all prior feedback addressed (docs now note the strict: true prefix requirement, sql.file() has a named-params test, the long comment is one line, and the finalization test's iteration count was cut to fit the default timeout).

What was reviewed:

• The $isArray branch in SQLiteQueryHandle.run for all four call sites (all/values/raw/db.run) — arrays still spread, objects pass as a single arg.
• unsafeQuery's values?.length ?? 0 check sets the simple flag for objects, but the SQLite handle ignores flags so it's harmless.
• New sql.file test uses the same await using tempDir + path.join(dir, …) pattern already established in this file.

Extended reasoning...

Overview

Small, focused bugfix: Bun.SQL's SQLite adapter was spreading unsafe()/file() bindings via $apply, which treats a plain object as a zero-length array-like and silently binds nothing. The fix adds an $isArray(values) branch — arrays spread as before, objects are passed as a single argument to stmt.all/stmt.values/stmt.raw/db.run, which bun:sqlite already supports natively for :name/$name/@name placeholders. Type declarations for unsafe() and file() are widened to any[] | Record<string, any>, and docs/JSDoc note the SQLite-only behavior and the strict: true prefix caveat.

Security risks

None. Values are still bound as parameters by the underlying bun:sqlite layer, not concatenated into SQL. unsafe() remains the explicit "you own the query string" escape hatch it already was; this change only affects how the bindings argument is forwarded.

Level of scrutiny

Low-to-medium. The runtime change is ~10 lines in one file, mechanically branching on $isArray. The rest is docs, types, and 7 new tests. This is a redo of a previously accepted community PR (#23375). The evidence block shows all new tests fail on main and pass with the fix, and the full sqlite-sql.test.ts suite passes on both debug-ASAN and release.

Other factors

All four review threads from the prior pass are resolved and verified against the current diff:

• The docs example at sql.mdx:440-445 now constructs the connection with strict: true and the prose explains prefixed vs. bare keys.
• The JSDoc @example in sql.d.ts uses { ":id": 1 } and states keys keep the prefix unless strict: true.
• A sql.file() named-params test was added alongside the existing file tests, using the same await using tempDir + path.join(dir, …) idiom already used earlier in the file (tempDir returns a DisposableString extending String, so path.join accepts it).
• The 10k-insert finalization test was reduced to 1000 iterations with a comment, replacing the earlier 60s per-test timeout.

I also checked that unsafeQuery in src/js/bun/sql.ts:134 reading values?.length (which is undefined on a plain object, so the simple flag gets set) does not affect the SQLite path — SQLiteQueryHandle.run never consults query flags. The values ?? [] default in createQueryHandle keeps the null/undefined case on the array branch.

[claude]

LGTM — the two nits from the earlier pass (sql.file() coverage, strict-mode caveat in docs/JSDoc) are addressed, and the loop shrink replaces the timeout bump per convention.

What was reviewed:

• $isArray branch in SQLiteQueryHandle.run: array path unchanged ($apply), object path uses $call to hand the bindings object to bun:sqlite's existing named-parameter support; same shape applied to db.run.
• Test matrix covers strict/non-strict, all three prefixes, .values()/.raw(), UPDATE/DELETE counts, transaction unsafe, and sql.file(); tempDir/path.join usage matches the file's existing pattern.
• The 10000→1000 iteration shrink keeps the iterations - 300 assertion arithmetically consistent and still cycles enough statements to exercise finalization.

Extended reasoning...

Overview

Four files touched: src/js/internal/sql/sqlite.ts (~18 lines — widen the values type and branch on $isArray to pass named-parameter objects via $call instead of spreading via $apply), packages/bun-types/sql.d.ts (widen unsafe/file signatures + JSDoc), docs/runtime/sql.mdx (SQLite named-parameter example with the strict-mode prefix caveat), and test/js/sql/sqlite-sql.test.ts (7 new tests + one unrelated iteration shrink).

Security risks

None introduced. sql.unsafe() is already the documented escape hatch for raw SQL; this only changes how the bindings argument is forwarded to bun:sqlite, which already supports object bindings. The $isArray/$call/$apply intrinsics follow the tamper-resistance convention for src/js/ builtins. No native code touched.

Level of scrutiny

Low-to-moderate. The runtime change is a small conditional whose array branch is byte-identical to the previous behavior, and the object branch delegates to an existing, well-tested bun:sqlite feature. The type/docs changes are declarative. The one adjacent change — shrinking the finalization loop from 10000 to 1000 — follows the REVIEW.md rule ("shrink the workload" over raising a timeout) and the finalCount === iterations - 300 assertion stays valid.

Other factors

Both prior inline nits from the earlier bug-hunt pass are addressed and resolved: the docs/JSDoc now state that keys keep the prefix unless strict: true (with the example showing strict: true), and a sql.file() named-parameter test was added. The comment-cop feedback on the inline comment was also addressed (trimmed to one line). Test coverage is thorough across the variant matrix, and the PR evidence shows the new tests fail on main and pass with the fix. New test code matches existing file conventions (await using dir = tempDir(...), path.join(dir, ...)).

[claude]

LGTM — thanks for addressing the earlier feedback (docs/JSDoc now clarify the strict: true requirement, sql.file() is covered, the comment is one line, and the finalization test shrinks the loop instead of raising the timeout).

What was reviewed:

• SQLiteQueryHandle.run — $isArray branch correctly routes objects through $call and arrays through $apply for all four sinks (all/values/raw/db.run); tamper-proof intrinsics used throughout.
• Traced sql.unsafe/sql.file and their transaction/reserved twins through normalizeQuery → createQueryHandle: the string-input path returns values unchanged, so objects reach the handle intact; the simple flag set when values.length is undefined is harmless for SQLite (adapter ignores flags).
• Seven new tests cover strict/non-strict, all three prefixes, values()/raw(), UPDATE/DELETE counts, transactions, and sql.file().

Extended reasoning...

Overview

This PR fixes #22799: Bun.SQL's unsafe()/file() silently dropped named-parameter objects for SQLite because $apply treated a plain object as a zero-length array-like. The fix is an $isArray(values) branch in SQLiteQueryHandle.run that passes objects via $call (single argument) instead of $apply (spread), delegating to bun:sqlite's existing named-parameter support. Type signatures for unsafe()/file() are widened to any[] | Record<string, any>, and docs/JSDoc explain the SQLite-only behavior and the strict: true requirement for bare keys.

Files touched: ~18 LOC in src/js/internal/sql/sqlite.ts, type widening in packages/bun-types/sql.d.ts, one docs paragraph in docs/runtime/sql.mdx, and 7 new tests plus a shrunk iteration count in test/js/sql/sqlite-sql.test.ts.

Prior review resolution

I left two nits on the previous revision, both now addressed:

• The docs example now sets strict: true and the prose explains that keys keep the prefix ({ ":id": 1 }) without it; the JSDoc @example uses the prefixed form so it works under the default.
• A sql.file() test with a named-parameter object was added.

The comment-cop bot's note was addressed (comment reduced to one line stating the $apply hazard), and the statement-finalization test now shrinks its loop from 10000 to 1000 rather than raising the timeout — matching REVIEW.md's "shrink the workload" guidance.

Security risks

None introduced. unsafe() is already the escape hatch for raw SQL; this change only fixes how bound parameter values reach bun:sqlite, which handles the actual binding. No new user-controlled data reaches an interpolation site. Postgres/MySQL adapters are untouched.

Level of scrutiny

Low-to-moderate. The runtime change is a four-line conditional in built-in JS that delegates to already-tested bun:sqlite machinery. I traced the value flow end-to-end: sql.unsafe/sql.file (and transaction/reserved variants) → Query → adapter.normalizeQuery (returns object unchanged for string input) → createQueryHandle → SQLiteQueryHandle.run. The SQLQueryFlags.simple flag that gets set when values.length is undefined is a no-op for SQLite since createQueryHandle ignores the flags argument. Tamper-proof intrinsics ($isArray, $call, $apply) are used correctly per the built-ins conventions.

Other factors

• Test coverage is thorough for the variant matrix: strict and non-strict modes, all three placeholder prefixes, .values()/.raw() result modes, INSERT/UPDATE/DELETE change counts, inside a transaction, and via sql.file(). Verified in the PR evidence to fail on main and pass with the fix under both debug+ASAN and release.
• The change is a redo of #23375 against current main as requested there.
• No open reviewer comments remain; all inline threads are resolved.

[coderabbitai]

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 `@docs/runtime/sql.mdx`:
- Around line 440-445: Update the “Queries in files” documentation to include
SQLite named-object bindings supported by sql.file(), including the placeholder
formats and strict-mode bare-key behavior. Keep the existing positional $1/$2
parameter documentation intact.

In `@test/js/sql/sqlite-sql.test.ts`:
- Around line 1389-1399: Add `$name` and `@name` binding cases to the non-strict
`unsafe` named-parameter test alongside the existing `:id` case, using matching
`$name` and `@name` keys in the parameter objects and verifying the inserted or
queried values. Keep coverage focused on prefix retention for all three
supported binding prefixes.

🪄 Autofix

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: d355a600-5ab2-42fa-b876-91ac7fcc348e

📥 Commits

Reviewing files that changed from the base of the PR and between 45ee955 and 0ca8766.

📒 Files selected for processing (4)

• docs/runtime/sql.mdx
• packages/bun-types/sql.d.ts
• src/js/internal/sql/sqlite.ts
• test/js/sql/sqlite-sql.test.ts

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

test/js/sql/sqlite-sql.test.ts (1)

1606-1618: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Add non-strict sql.file() coverage.

This test uses only strict: true and bare object keys. It does not prove that sql.file() works in default mode with prefixed keys. Add a default-mode case such as { ":id": 1 } and assert the filtered row.

As per coding guidelines, “Tests must cover the complete relevant variant matrix, including sibling APIs and flag states.”

🤖 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 `@test/js/sql/sqlite-sql.test.ts` around lines 1606 - 1618, Add non-strict
coverage alongside the existing “file with named parameters” test by creating
SQL without strict mode, passing prefixed parameter keys such as { ":name":
"Alice" } to sql.file(), and asserting the filtered row result. Keep the
existing strict-mode test unchanged.

Source: Coding guidelines

🤖 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.

Outside diff comments:
In `@test/js/sql/sqlite-sql.test.ts`:
- Around line 1606-1618: Add non-strict coverage alongside the existing “file
with named parameters” test by creating SQL without strict mode, passing
prefixed parameter keys such as { ":name": "Alice" } to sql.file(), and
asserting the filtered row result. Keep the existing strict-mode test unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 63066fff-2bc1-4fc8-8f6e-3e12e31cba0e

📥 Commits

Reviewing files that changed from the base of the PR and between 0ca8766 and c1381c3.

📒 Files selected for processing (2)

• docs/runtime/sql.mdx
• test/js/sql/sqlite-sql.test.ts

[robobun]

Added non-strict sql.file() coverage in ad89b39: the same query file now runs against a default-mode connection with prefixed keys.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

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/sqlite.ts`:
- Around line 248-254: Wrap all prepared-statement execution after
db.prepare(sql), including the values, raw, and all $call/$apply branches, in a
try/finally block and move stmt.finalize() into finally. Preserve the existing
result handling while ensuring named-binding exceptions and other failures
finalize the statement before propagating.

🪄 Autofix

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: b1d59e35-2637-42a4-a7b3-fa53fe882d60

📥 Commits

Reviewing files that changed from the base of the PR and between 52bf09c and 4fa58ea.

📒 Files selected for processing (4)

• docs/runtime/sql.mdx
• packages/bun-types/sql.d.ts
• src/js/internal/sql/sqlite.ts
• test/js/sql/sqlite-sql.test.ts

[cirospaciari]

looks like this is a behavior only in sqlite should we add it even if postgres and mysql dont have?

[robobun]

@cirospaciari Right, this is SQLite only. Some context for the call:

unsafe() is already adapter specific in practice, since the placeholder syntax differs per database ($1 for Postgres, ? for MySQL and SQLite). Postgres and MySQL already throw values must be an array when an object is passed (checked natively in src/sql_jsc/shared/QueryCtorArgs.rs), and that is unchanged here. The SQLite adapter is a thin wrapper over bun:sqlite, which has supported object bindings for a long time, so this passes them through rather than adding a new dialect. The docs and types say it is SQLite only.

What makes the current state worth changing either way is that on main, passing an object to the SQLite adapter silently binds nothing and a SELECT returns [] with no error (that is what #22799 hit).

If you would rather keep the adapters uniform, the alternative is to make SQLite throw the same values must be an array error as the other two. That is a small change and I am happy to switch this PR to that instead, your call. I went with pass-through because #22799 asked for it and Jarred asked to revive the original PR.

[jakeboone02]

looks like this is a behavior only in sqlite should we add it even if postgres and mysql dont have?

@cirospaciari #22799 explains the rationale: ability to use the same import/API for multiple databases. I would love to see this implemented as I have a project with integration tests on PostgreSQL, SQLite, and several other platforms. More consistency would be nice.

[cirospaciari]

@robobun check code reviews here plz

[robobun]

@cirospaciari Reviews are addressed, all threads resolved. The one code change that came out of them is in 1c8f66e: 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, with a test for that path. The rest were test and doc tightening. Full sqlite-sql.test.ts passes locally (239 tests), CI is running on the latest push.

[robobun]

The red on the latest build is three build steps failing to download vendored artifacts from GitHub after 5 attempts (lolhtml tarball on freebsd x64 and linux x64-musl, the WebKit prebuilt on windows aarch64), so those lanes never got to run tests. Nothing in this change is involved; every lane that built is passing, and I already used my one retrigger on an earlier build. A job retry on those three steps should clear it.

[robobun]

^{Updated 6:05 PM PT - Aug 12th, 2026}

@robobun, your commit c2d9272 is building: #93644