Repository navigation
Conversation
… plain filenames
parseConnectionDetailsFromOptionsOrEnvironment only read `filename` when
the adapter was sqlite, so { adapter: "sqlite", url: "sqlite://a.db" }
fell through to the ":memory:" default while { url: "sqlite://a.db" }
opened the file. The sqlite branch now resolves filename, then url, the
same as the adapter-less branch.
parseSQLiteOptions split every input at the first "?", so a plain path
such as "what?.db" opened "what". The query string is now only split off
URL instances and sqlite:/file: strings; plain paths reach SQLite
unchanged, matching bun:sqlite.
|
Status: reproduced on the released bun 1.4.0 with CI (build 95683) finished with 177 of 179 jobs passed, including the new tests on every Linux and Windows lane. The build is marked failed only because the |
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
There was a problem hiding this comment.
LGTM — focused fix for two clear option-parsing bugs in the SQL sqlite adapter.
What was reviewed:
parseConnectionDetailsFromOptionsOrEnvironment: merging the sqlite branch into the unknown-adapter branch is behavior-preserving for the unknown and non-sqlite paths; only sqlite gains theurlfallback.parseSQLiteOptions: verified the?mode=query matrix insqlite-url-parsing.test.ts:82uses only the foursqliteProtocolsprefixes, sohasSqliteProtocolcovers every existing case;URLinstances still split via theinstanceofcheck.- Tests: use
tempDir/await using, assert actual file creation (not justoptions.filename), the?-in-filename open test is correctlyskipIf(isWindows), and precedence offilenameoverurlis pinned.
Extended reasoning...
Overview
Two changes in src/js/internal/sql/shared.ts, both in the Bun.SQL option-parsing path for the sqlite adapter:
parseConnectionDetailsFromOptionsOrEnvironment(~line 1625): the three-wayifonoptions.adaptercollapses the=== "sqlite"and!options.adapterbranches into one, so{ adapter: "sqlite", url }now readsurlafterfilename, exactly as the adapter-less branch already did. The non-sqlite branch is byte-identical to before (only readsurl). The unknown-adapter branch is unchanged. This fixes the silent fallback to:memory:when onlyurlis given.parseSQLiteOptions(~line 1460): the redundantoriginalUrllocal and the always-truetypeof filename === "string"guards are removed, and the?-split now only runs when the input is aURLinstance or matches one of the foursqliteProtocolsprefixes via a newhasSqliteProtocolhelper (which reuses the existingsqliteProtocolstable — single source of truth). Plain paths reach SQLite verbatim, matchingbun:sqlite'snew Database().
Five new tests land in the existing test/js/sql/sqlite-sql.test.ts in the appropriate describe blocks.
Security risks
None. This is TypeScript option-normalization code with no shell/SQL interpolation, no auth, no network. The change narrows (not widens) what gets treated as a URL query string, and the resulting filename is handed to bun:sqlite the same way a direct new Database(filename) call would be.
Level of scrutiny
Low-medium. Pure JS/TS logic in a builtin module, no native code touched. The primary risk with option-parsing changes is silently altering behavior for existing inputs; I checked the two mechanisms:
- The branch merge: for
adapter === "sqlite", the only new path is falling through tooptions.urlwhenfilenameis absent — previously that fell through to:memory:, which is the bug being fixed.filenamestill wins when present (pinned by the new precedence test). - The
?-split gate: I verifiedsqlite-url-parsing.test.ts:82'sprotocolsWithQueryarray is exactly the four prefixes insqliteProtocols, so every existing?mode=test case still splits. The only inputs that stop splitting are un-prefixed plain paths, which is the second bug being fixed.
Other factors
- Tests follow repo conventions:
tempDir+using,await usingonSQLhandles, assertions on real filesystem effects (existsSync(dbPath), negative check on the truncated name), Windows skip for the?-in-filename open test with a stated reason. - The PR description states
sqlite-url-parsing.test.ts,adapter-env-var-precedence.test.ts, andadapter-override.test.tsall pass (272 total), and the four new bug-repro tests fail on the released bun — satisfying the "fails for the right reason" requirement. - No outstanding reviewer comments; only the robobun status comment is on the timeline.
|
Updated 5:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit 10e779c has some failures in 🧪 To try this PR locally: bunx bun-pr 38427That installs a local version of the PR into your bun-38427 --bun |
|
Heads-up: #41586 adopts the same |
Problem
new SQL({ adapter: "sqlite", url: "sqlite://a.db" })opens:memory:(sql.options.filename === ":memory:"), so everything written is gone at close anda.dbis never created. The same object withoutadapter, and{ adapter: "sqlite" }plusDATABASE_URL=sqlite://a.db, both opena.db. AURLinstance or a bare path inurlis dropped the same way.parseConnectionDetailsFromOptionsOrEnvironment(src/js/internal/sql/shared.ts:1626) only readsoptions.filenamewhen the adapter is sqlite, sourlis never looked at and the filename falls through to the:memory:default.new SQL({ adapter: "sqlite", filename: "what?.db" })opens a file namedwhat(sql.options.filename === "what");new SQL("what?.db", { adapter: "sqlite" })does the same.bun:sqlite'snew Database("what?.db")openswhat?.db.parseSQLiteOptions(shared.ts:1467) cuts every input at the first?to pull out?mode=, whether or not the input is a URL. Before Fix: Make SQL connection string parsing more sensible #22260 only protocol-prefixed strings were cut.Fix
filename, thenurl, which is exactly what the adapter-less branch already did (the two branches are merged). Non-sqlite adapters are unchanged and still read onlyurl.filenamestill wins when both are given, andurloverrides the first constructor argument the same way it does for postgres/mysql/mariadb.parseSQLiteOptionsonly splits off a query string when the input is aURLinstance or starts withsqlite://,sqlite:,file://orfile:. Plain paths reach SQLite unchanged.?mode=handling for URL-shaped input is the same as before (the 28-case query matrix in sqlite-url-parsing.test.ts still passes).filenamestays the documented sqlite option; this change stopsurlfrom being silently discarded when it is passed anyway.should open options.url when adapter is sqlite,should accept the same url shapes with adapter: sqlite as without it,should keep ? in a plain filename,should open the file named by a plain filename containing ?(skipped on Windows, where?is not a legal filename character), plusshould prefer options.filename over options.urlpinning the precedence. The four fail on the released bun (:memory:/ file not created) and pass with this change; the whole file passes (244).Background
Bun.SQLaccepts a connection string, aURL, or an options object, andparseOptionsin shared.ts picks an adapter (postgres, mysql, mariadb or sqlite) and normalizes the input into that adapter's options.parseConnectionDetailsFromOptionsOrEnvironmentdecides which option (filename,url, the first argument, or an env var) is the connection target;parseSQLiteOptionsthen turns that target into abun:sqlitefilename plusreadonly/createflags.sqlite://data.db?mode=ro,file:///data.db) or a plain path (./data.db). Only the URL form has a query string;?mode=ro|rw|rwcmaps onto the open flags. A plain path is handed to SQLite verbatim, and?is an ordinary character in a path.?mode=rwmaps to; it edits a different part ofparseSQLiteOptions.Before/after on the original report
Not changed here:
{ filename: "./dev.db" }with no adapter still resolves to postgres;sqlite://paths are still not percent-decoded and keep#, as the existing url-parsing tests require.