Skip to content

sql(sqlite): skip a PostgreSQL, MySQL or MariaDB URL in DATABASE_URL - #45080

Open
robobun wants to merge 4 commits into
mainfrom
robobun/299e737a/sql-env-url-other-database
Open

robobun wants to merge 4 commits into
mainfrom
robobun/299e737a/sql-env-url-other-database

Conversation

@robobun

@robobun robobun commented Oct 12, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • With DATABASE_URL=postgres://127.0.0.1:1/db, new SQL({ adapter: "sqlite" }) uses that URL as its file name. The first query rejects with SQLITE_CANTOPEN. Before Fix: Make SQL connection string parsing more sensible #22260 (1.2.22) the call opened :memory:.
  • Cause: getConnectionDetailsFromEnvironment (src/js/internal/sql/shared.ts:1634-1639) returns DATABASE_URL, DATABASEURL or TLS_DATABASE_URL first, whatever adapter the options name.
  • Found by fuzzing. No user report. Three SQLite in Bun.sql #21640 tests that expect :memory: fail on main under such a DATABASE_URL.

Fix

  • For SQLite, each of the three variables counts as not set when it holds a PostgreSQL, MySQL or MariaDB URL. The lookup then reads the next one, SQLITE_URL, and the :memory: default.
  • Every other value applies as before: a SQLite URL, a path, libsql://, https://.
  • Verified: test/js/sql/adapter-env-var-precedence.test.ts (45 new tests, 38 fail on main). Also sqlite-sql, sqlite-url-parsing, adapter-override.
  • Self-reviewed: 19 concerns raised, 17 addressed. Not taken: a fold into sql: do not read the environment connection URL when options name a target #41586, and unverified code-search counts.

Background

  • The three variables are generic: every adapter reads them first. SQLITE_URL belongs to SQLite.
  • This rule is one of three candidates: the Notes start with that question.
  • Considered the same check for the server adapters: it changes TLS modes and an existing throw.

Downsides

  • Beside a server DATABASE_URL, a call with no filename opens :memory: where it failed with SQLITE_CANTOPEN. That includes an undefined filename and { adapter: "sqlite", url }.
  • SQLite, per variable with ://: 4 string calls, 2 short strings. Other adapters: 1 to 3 calls of 6 bytecode instructions per construct. Builtins: +511 bytes.
Notes

Question for the reviewer (@alii, as the author of #21640 and #22260). With adapter: "sqlite" and no filename, what do DATABASE_URL, DATABASEURL and TLS_DATABASE_URL do? Three rules are on record. Each cell is the file that the call opens:

Rule Where it is stated postgres://... sqlite://./app.db ./app.db libsql://...
(a) SQLite does not read them the doc comment of #22260, and 1.2.21 :memory: :memory: :memory: :memory:
(b) Only a SQLite URL applies docs/runtime/sql.mdx ("when it contains a SQLite-compatible URL"), closed #22242 :memory: app.db :memory: :memory:
(c) All apply but a PostgreSQL, MySQL or MariaDB URL this PR :memory: app.db app.db the URL, then SQLITE_CANTOPEN
main the URL, then SQLITE_CANTOPEN app.db app.db the URL, then SQLITE_CANTOPEN

This PR takes (c) because it changes no row that works on main. (a) and (b) make a value that opens a file today a silent :memory:. If you want (a), (b), or a throw that names the variable in place of the skip, say so and I will change it. (a) is about 2 lines. (b) is the parseDefinitelySqliteUrl test on each value. The helper gets no further special cases: the answer to the next spelling is this ruling.

Origin. An automated fuzz report. There is no user report and no GitHub issue for this input. #22147, a user report on the same lookup, is about another input (an explicit hostname). The bug reproduces on 1.4.3, 1.4.2, 1.3.14 and main. The repro script (new Bun.SQL({ adapter: "sqlite" }) in a child process, one environment variable per row) prints filename postgres://127.0.0.1:1/db | SQLITE_CANTOPEN on main for DATABASE_URL, DATABASEURL and TLS_DATABASE_URL. On a debug build of this branch it prints filename :memory: | opens for the three, and exits 0.

Who meets it. Code that did not set the variable: a tool or a test that opens a scratch database with new SQL({ adapter: "sqlite" }) in a project whose environment or .env has a server DATABASE_URL. The three tests from #21640 are such code: null with adapter defaults to :memory: and undefined with adapter defaults to :memory: (sqlite-url-parsing.test.ts) and should handle null/undefined with adapter specified (sqlite-sql.test.ts) fail on 1.4.3 with DATABASE_URL=postgres://127.0.0.1:1/db exported, and pass on this branch. On 1.2.22 to 1.4.3 the workaround is filename: ":memory:" or new SQL(":memory:").

Regression. Before #22260 the options form did not read the environment for SQLite: filename was options.filename || ":memory:". #22260 added the lookup for every adapter, with the generic variables first. Its doc comment says that with an adapter the lookup only reads the variables of that adapter. The docs say that DATABASE_URL configures SQLite "when it contains a SQLite-compatible URL". #22242 had that rule in code and was closed as inactive.

The rule. With adapter: "sqlite", each of DATABASE_URL, DATABASEURL and TLS_DATABASE_URL is checked by itself, in that order. A value counts as not set when the text before the first :// is postgres, postgresql, mysql, mysql2 or mariadb. The scheme is compared without case and without the whitespace before it, as new URL() reads it for the server adapters. The first variable that is left applies. If none is left, SQLITE_URL and SQLITEURL apply, and then the default :memory:. These values apply as before:

  • :memory:, sqlite://..., sqlite:..., file://..., file:...,
  • a path or any value with no :// (mydb, ./app.db, 127.0.0.1:1/db),
  • every other scheme: libsql://, mongodb://, mssql://, and http://, https://, ftp://. These stay file names and keep failing with SQLITE_CANTOPEN. On 1.2.21 they opened :memory: too. They are left as on main on purpose: a silent in-memory database is worse than that error, and the question above decides them. The resolver maps http, https and ftp to Postgres when no adapter is given, but they name no database.

Rows that change. A matrix of 401 constructor calls and environments, release build of main against the option resolver of this branch:

  • 29 of 401 change. All 29 have adapter: "sqlite". 25 resolve to :memory:, 2 to the file of SQLITE_URL, and 2 to the SQLite file of the next generic variable.
  • 0 of 201 must-keep rows change: no adapter in the options (69), a URL of the adapter's own kind, an adapter-specific variable, an explicit string or URL argument, no variable.
  • 0 rows of a server adapter change.
  • sslMode: 0 of 505 rows are lower or higher (the 401 rows and 104 rows built for TLS).

Measurements (Linux x64, main is 7583003d82).

  • Operations per construct (reads of the environment object / String indexOf, slice, trim, toLowerCase, startsWith, includes calls / URL constructions), main to this branch:
    • new SQL() with DATABASE_URL=postgres://: 8 / 7 / 1 to 8 / 7 / 1.
    • { adapter: "postgres" } with DATABASE_URL=postgres://: 8 / 3 / 1 to 8 / 3 / 1.
    • { adapter: "mariadb" } with DATABASE_URL=mysql://: 6 / 3 / 1 to 6 / 3 / 1.
    • { adapter: "sqlite" } with DATABASE_URL=sqlite://: 1 / 1 / 0 to 1 / 5 / 0.
    • { adapter: "sqlite" } with DATABASE_URL=:memory:: 1 / 1 / 0 to 1 / 2 / 0.
    • { adapter: "sqlite", filename } with DATABASE_URL=postgres://: 1 / 1 / 0 to 5 / 5 / 0. The lookup runs before filename is read, and it now goes on to the four variables after DATABASE_URL.
    • { adapter: "sqlite" } and no URL variable: 5 / 1 / 0 to 5 / 1 / 0.
  • Calls of the new helper per construct, for every adapter: 1 when DATABASE_URL applies, 2 when DATABASEURL does, 3 in every other case. For an adapter that is not sqlite a call runs 6 bytecode instructions: the comparison of the adapter and the return. A string or URL argument does not reach the lookup.
  • GC cells of the check (values held, full GC, heapStats().objectCount, 20,000 calls): 2 strings (slice, toLowerCase) per generic variable whose value has ://, and 3 when whitespace comes before the scheme. 0 for :memory:, for a path, for a variable that is not set, and for every other adapter.
  • Bytecode (BUN_JSC_dumpGeneratedBytecodes=1, debug builds): getConnectionDetailsFromEnvironment 156 to 168 instructions, 798 to 876 bytes. New genericUrlFor: 36 instructions, 258 bytes.
  • Builtins blob (bun scripts/build.ts --profile=release --mode=codegen, InternalModuleRegistryConstants.bin): 2,644,484 to 2,644,995 bytes.
  • System calls on the reported construct and machine instructions: not measured. strace, perf and valgrind are not installed in the build container.

The server adapters. { adapter: "mysql" } with DATABASE_URL=postgres://... takes the host, the port, the user and the database from that URL, and the same holds for each pair of server adapters. This PR leaves that as it is, on purpose:

  • For a server adapter the explicit adapter wins over the URL scheme in every input form. new SQL("postgres://...", { adapter: "mysql" }) and { adapter: "mysql", url: "postgres://..." } give a MySQL client for that host too. A rule for the environment alone makes the forms disagree.
  • A skip has no safe default there. It lowers sslMode when the skipped URL carried ?sslmode=require or came from TLS_DATABASE_URL, and { adapter: "mariadb" } with a mysql:// URL has to keep working.
  • sql: do not read the environment connection URL when options name a target #41586 does not cover it: it stops reading the environment URL only when the options name a target, and a lone adapter still reads it.

The helper takes the adapter as its first argument, so a rule for the server adapters is one more condition in it.

Not changed here.

  • The order. With adapter: "sqlite", SQLITE_URL is read only when each generic variable is not set or is skipped. DATABASE_URL=./a.db beside SQLITE_URL=./b.db opens a.db, and DATABASE_URL=https://... beside SQLITE_URL=./b.db fails. That is the order of main.
  • { adapter: "sqlite", url } still ignores url. Beside a server DATABASE_URL it now resolves to :memory:, which is what main does with no DATABASE_URL. sql: do not read the environment connection URL when options name a target #41586 and sql(sqlite): honor options.url with adapter: "sqlite" and keep "?" in plain filenames #38427 own that case.
  • DATABASE_URL=SQLITE://env.db with adapter: "sqlite" is still the file name SQLITE://env.db: the prefix match of parseDefinitelySqliteUrl has a case.
  • SQLITE_URL=postgres://... is still a file name. Existing tests pin that the name of an adapter-specific variable wins over the scheme.
  • An explicit string argument (new SQL("postgres://h/db", { adapter: "sqlite" })) is still a file name.
  • sql: select SQLite from filename or SQLITE_URL before a URL is built #44483 (a filename or SQLITE_URL selects SQLite) lists this case as not changed there. Its hunks in shared.ts do not touch this one. Both PRs add a paragraph to the filename JSDoc and a describe at the same place of the test file, so the second one to land needs a rebase.

no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/sql/adapter-env-var-precedence.test.ts

`new SQL({ adapter: "sqlite" })` took DATABASE_URL=postgres://... as its
file name, so the first query failed with SQLITE_CANTOPEN. Before #22260
the same call opened `:memory:`.

With the SQLite adapter, getConnectionDetailsFromEnvironment now skips
DATABASE_URL, DATABASEURL and TLS_DATABASE_URL when the value has the
scheme of another adapter, and reads SQLITE_URL next. A SQLite URL, a
path and a scheme that no adapter claims still apply. The other adapters
are not changed.
@robobun

robobun commented Oct 12, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 3:18 AM PT - Oct 13th, 2026

✅ @robobun, your commit 135b682fcc377d373746d52ff679dcb15bf8b665 passed in Build #125106! 🎉


🧪   To try this PR locally:

bunx bun-pr 45080

That installs a local version of the PR into your bun-45080 executable, so you can run:

bun-45080 --bun

@robobun

robobun commented Oct 12, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: reproduced on the released 1.4.3 and on a debug build of main.

With DATABASE_URL=postgres://127.0.0.1:1/db (also DATABASEURL and TLS_DATABASE_URL), new Bun.SQL({ adapter: "sqlite" }) reports options.filename === "postgres://127.0.0.1:1/db", and the first query rejects with SQLITE_CANTOPEN. Without the variable the same call opens :memory:.

With this PR the SQLite adapter skips a PostgreSQL, MySQL or MariaDB URL in each of those three variables. It then reads the next variable, SQLITE_URL, and the :memory: default. The tests are in test/js/sql/adapter-env-var-precedence.test.ts: 38 cases fail on main and all pass here.

One question is open for a maintainer. It is at the top of the Notes in the description: which values of DATABASE_URL apply to an explicit adapter: "sqlite".

The server adapters are not changed. { adapter: "mysql" } with a postgres:// DATABASE_URL still uses that URL, and #41586 does not cover that case. It is separate work.

@coderabbitai

coderabbitai Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 82fc9380-d90e-458a-918f-f0fdea94c0d6
📥 Commits

Reviewing files that changed from the base of the PR and between e849d45 and 135b682.

📒 Files selected for processing (4)
  • docs/runtime/sql.mdx
  • packages/bun-types/sql.d.ts
  • src/js/internal/sql/shared.ts
  • test/js/sql/adapter-env-var-precedence.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

Walkthrough

SQLite environment lookup now skips shared URLs with recognized PostgreSQL, MySQL, or MariaDB schemes when SQLite is selected. Lookup continues to later environment variables. Tests and documentation cover URL and filename handling.

Changes

SQLite environment URL selection

Layer / File(s) Summary
Filter shared URLs for SQLite
src/js/internal/sql/shared.ts, test/js/sql/adapter-env-var-precedence.test.ts, packages/bun-types/sql.d.ts, docs/runtime/sql.mdx
SQLite skips shared URLs with recognized PostgreSQL, MySQL, or MariaDB schemes and continues lookup. Tests cover precedence and accepted filename values. The declaration and runtime documentation describe the filename and SQLITE_URL options.

Suggested reviewers: jarred-sumner

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 135b6

The SQLite URL-selection change has no identified issue requiring correction before merge.

Pre-merge checks · Passed 4
Passed Passed checks (4 passed)
Check name Status Explanation
Linked Issues check Passed Issue #22242 is closed and supplies historical context only. No active directly linked issue remains. Therefore, this pull request has no linked-issue coding requirement to fail. The implementation an…
Out of Scope Changes check Passed The changes stay within the stated SQLite environment-variable lookup objective. genericUrlFor changes only SQLite handling in src/js/internal/sql/shared.ts. The new tests cover skipped URLs, prec…
Title check Passed The title clearly and concisely describes the main change: SQLite skips PostgreSQL, MySQL, and MariaDB URLs in DATABASE_URL.
Description check Passed The description provides detailed problem, fix, scope, trade-offs, and verification information. It does not use the template headings, but it covers the required content and is complete.
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

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

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline findings, I also checked the sslMode out-param left set to require after TLS_DATABASE_URL is skipped at src/js/internal/sql/shared.ts:1656 — the adapter === "sqlite" branch at shared.ts:1767 returns null for sslMode unconditionally, so the stale value has no consumer on the only path that can reach the reset.

Extended reasoning...

The change adds an isUrlForAnotherAdapter helper in src/js/internal/sql/shared.ts and gates the generic DATABASE_URL/DATABASEURL/TLS_DATABASE_URL early-return on it for the SQLite adapter only, plus a JSDoc line in packages/bun-types/sql.d.ts and a new describe block in test/js/sql/adapter-env-var-precedence.test.ts. It touches connection-string resolution (which database a process opens), not auth or crypto directly. Five inline findings were posted, so a human look is already signalled; this note only records the one out-param concern that was traced and ruled out.

Comment thread src/js/internal/sql/shared.ts Outdated
Comment thread src/js/internal/sql/shared.ts Outdated
Comment thread test/js/sql/adapter-env-var-precedence.test.ts Outdated
Comment thread src/js/internal/sql/shared.ts Outdated
Comment thread src/js/internal/sql/shared.ts Outdated
… URL of a server database

- A DATABASE_URL that is skipped no longer hides DATABASEURL and TLS_DATABASE_URL.
- `http`, `https` and `ftp` do not name a database, so such a URL stays a file name.
- Whitespace before the scheme does not hide it, as in `new URL()`.
- `genericUrlFor(adapter, value)` wraps the three reads in the existing block. For the SQLite
  adapter it returns null for a PostgreSQL, MySQL or MariaDB URL. The other adapters get the value.
- The doc comment of getConnectionDetailsFromEnvironment is as on main.
- The `filename` JSDoc and the docs name the skipped URLs and `SQLITE_URL`.
- Tests: a `file://` URL, the call forms with an undefined file name, and a URL with a second `://`.
@robobun robobun changed the title sql(sqlite): skip a DATABASE_URL that is for another database sql(sqlite): skip a PostgreSQL, MySQL or MariaDB URL in DATABASE_URL Oct 13, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants