Skip to content

Commit d6d69e3

Browse files
authored
refactor: add SQLite adapter to decouple from bun:sqlite (#970)
## Summary Second step of the Bun → Node.js migration (follows #967). Introduces a SQLite adapter layer that decouples the codebase from direct `bun:sqlite` imports, making the database layer portable across runtimes. ### Changes - **New**: `src/lib/db/sqlite.ts` — SQLite adapter using `node:sqlite` (Node 22+) - Falls back to `bun:sqlite` when `node:sqlite` is unavailable (during Bun test runner transition only — fallback will be removed once tests migrate to Vitest) - Handles API differences: prefers `bun:sqlite`'s cached `.query()` when available, falls back to `node:sqlite`'s `.prepare()`; provides manual BEGIN/COMMIT/ROLLBACK wrapper for `node:sqlite` (which lacks native `.transaction()`) - Normalizes `get()` return value to `null` (not `undefined`) for no-row results, matching codebase convention - Exports `Database` class and `SQLQueryBindings` type - **Fixed**: `isSchemaError()` and `isReadonlyError()` in `schema.ts` — now match by error message content instead of `error.name === "SQLiteError"`, making auto-repair and readonly detection work under `node:sqlite` (which throws plain `Error`, not `SQLiteError`) - **Updated imports** in 4 source files: `db/index.ts`, `schema.ts`, `migration.ts`, `utils.ts` - **Updated imports** in 3 test files: `fix.test.ts`, `telemetry.test.ts`, `schema.test.ts` - **Updated**: `lint-rules/no-manual-transactions.grit` — excludes `db/sqlite.ts` (manual transactions are necessary in the adapter for `node:sqlite` compatibility) - **Added**: `MIGRATION-PLAN.md` documenting remaining migration steps ### Design Call sites continue using `.query(sql).get()` / `.all()` / `.run()` and `db.transaction()` exactly as before — no migration churn needed. Zero `bun:sqlite` imports remain in `src/` or `test/`.
1 parent 840acef commit d6d69e3

10 files changed

Lines changed: 260 additions & 26 deletions

File tree

MIGRATION-PLAN.md

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
# Bun → Node.js Migration Plan
2+
3+
Status: In progress. See below for completed and remaining steps.
4+
5+
## Completed
6+
7+
### Phase 4 (early): Package Manager Switch
8+
- [x] Changed `packageManager` from `bun@1.3.13` to `pnpm@10.11.0`
9+
- [x] Moved `patchedDependencies` into `pnpm` config section
10+
- [x] Added `onlyBuiltDependencies: ["esbuild"]`
11+
- [x] Added phantom deps as explicit devDependencies: `@sentry/core`, `@clack/prompts`
12+
- [x] Generated `pnpm-lock.yaml`
13+
- [x] Verified all patches apply (including cross-version: `@stricli/core@1.2.5` patch on `1.2.7`)
14+
15+
### Phase 2, Group D: SQLite Adapter
16+
- [x] Created `src/lib/db/sqlite.ts` — runtime-detecting adapter (bun:sqlite under Bun, node:sqlite under Node.js)
17+
- [x] Updated 4 source files: `db/index.ts`, `schema.ts`, `migration.ts`, `utils.ts`
18+
- [x] Updated 3 test files: `fix.test.ts`, `telemetry.test.ts`, `schema.test.ts`
19+
- [x] Zero `bun:sqlite` imports remain in `src/` or `test/`
20+
21+
## Remaining
22+
23+
### Phase 2: Source Code Migration (replace Bun.* APIs in `src/`)
24+
25+
**Group A: File I/O** — Replace `Bun.file()` / `Bun.write()` with `node:fs/promises`
26+
- `Bun.file(path).text()``readFile(path, "utf-8")`
27+
- `Bun.file(path).json()``readFile(path, "utf-8")` then `JSON.parse()`
28+
- `Bun.file(path).exists()``access(path).then(() => true, () => false)`
29+
- `Bun.write(path, content)``writeFile(path, content)`
30+
- Scan all of `src/` for occurrences
31+
32+
**Group B: Process/System APIs** — Replace Bun.which / Bun.spawn / Bun.sleep
33+
- `Bun.which("cmd")``which` from a Node.js-compatible package or custom implementation
34+
- `Bun.spawn()` / `Bun.spawnSync()``child_process.spawn()` / `spawnSync()`
35+
- `Bun.sleep(ms)``setTimeout` promise wrapper
36+
37+
**Group C: Miscellaneous Bun APIs**
38+
- `Bun.Glob``tinyglobby` or `picomatch` (already in devDependencies)
39+
- `Bun.randomUUIDv7()``uuidv7` package (already in devDependencies)
40+
- `Bun.semver.order()``semver.compare()` (already in devDependencies)
41+
- `Bun.zstdCompressSync()` / `Bun.zstdDecompressSync()` → Node.js zlib or `zstd-napi` package
42+
43+
**Group E: Unpolyfilled APIs**
44+
- `bspatch.ts` and `upgrade.ts` — Replace any Bun-specific APIs not covered by node-polyfills.ts
45+
46+
### Phase 3: Test Migration (`bun:test` → Vitest)
47+
48+
- Add `vitest` as devDependency
49+
- Replace `import { ... } from "bun:test"` with Vitest equivalents
50+
- Replace `bun test` scripts with `vitest`
51+
- Key differences:
52+
- `bun:test`'s `mock.module()` → Vitest's `vi.mock()`
53+
- `bun:test`'s `spyOn` → Vitest's `vi.spyOn()`
54+
- Test file discovery patterns may differ
55+
- `--isolate --parallel` behavior needs Vitest equivalent
56+
57+
### Phase 4: CI & Dev Scripts (remaining)
58+
59+
- Update `package.json` scripts: `bun run``pnpm run` where appropriate
60+
- Replace `bun run src/bin.ts` with `tsx src/bin.ts` (add `tsx` devDependency)
61+
- Replace `bun run script/*.ts` with `tsx script/*.ts`
62+
- Replace `bunx` with `pnpm exec`
63+
- Update GitHub Actions workflows to use pnpm + Node.js instead of Bun
64+
- Update `Dockerfile` / build scripts if applicable
65+
66+
### Phase 5: Cleanup
67+
68+
- Remove `@types/bun` from devDependencies
69+
- Remove `bun.lock` (replaced by `pnpm-lock.yaml`)
70+
- Remove or update `script/node-polyfills.ts` (may become unnecessary)
71+
- Update `AGENTS.md` Bun API reference table
72+
- Remove Bun-specific `.cursor/rules/bun-cli.mdc` or update for Node.js
73+
- Clean up any remaining `Bun.*` references in comments/docs
74+
75+
## Known Issues
76+
77+
- `test/lib/index.test.ts``sdk.run throws when auth is required but missing` fails under pnpm's strict `node_modules`. The mock fetch returns empty 200s which prevents the expected auth error from being thrown. Pre-existing test fragility, not caused by migration changes.

lint-rules/no-manual-transactions.grit

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
file($name, $body) where {
22
$name <: not r".*migration\.ts$",
33
$name <: not r".*node-polyfills\.ts$",
4+
$name <: not r".*db/sqlite\.ts$",
45
$body <: contains `$db.exec($sql)` as $match where {
56
$sql <: or {
67
r".*BEGIN.*",

src/lib/db/index.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,15 @@
11
/**
22
* SQLite database connection manager for CLI configuration storage.
3-
* Uses bun:sqlite natively; Node.js uses a polyfill in node-polyfills.ts.
3+
* Uses the sqlite.ts adapter which wraps node:sqlite's DatabaseSync
4+
* with a bun:sqlite-compatible API surface.
45
*/
56

6-
import { Database } from "bun:sqlite";
77
import { chmodSync, mkdirSync } from "node:fs";
88
import { join } from "node:path";
99
import { getEnv } from "../env.js";
1010
import { migrateFromJson } from "./migration.js";
1111
import { initSchema, runMigrations } from "./schema.js";
12+
import { Database } from "./sqlite.js";
1213

1314
export const CONFIG_DIR_ENV_VAR = "SENTRY_CONFIG_DIR";
1415

src/lib/db/migration.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,11 @@
22
* One-time migration from config.json to SQLite.
33
*/
44

5-
import type { Database } from "bun:sqlite";
65
import { rmSync } from "node:fs";
76
import { join } from "node:path";
87
import { logger } from "../logger.js";
98
import { getConfigDir } from "./index.js";
9+
import type { Database } from "./sqlite.js";
1010

1111
const log = logger.withTag("migration");
1212

src/lib/db/schema.ts

Lines changed: 22 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,10 @@
1111
* - Migration checks
1212
*/
1313

14-
import type { Database } from "bun:sqlite";
1514
import { getEnv } from "../env.js";
1615
import { stringifyUnknown } from "../errors.js";
1716
import { logger } from "../logger.js";
17+
import type { Database } from "./sqlite.js";
1818

1919
export const CURRENT_SCHEMA_VERSION = 16;
2020

@@ -590,18 +590,22 @@ let isRepairing = false;
590590

591591
/**
592592
* Check if an error is a schema-related SQLite error that can be auto-repaired.
593+
*
594+
* Matches by message content rather than error name because `bun:sqlite`
595+
* throws `SQLiteError` while `node:sqlite` throws plain `Error` — the
596+
* message strings are identical across both runtimes.
593597
*/
594598
function isSchemaError(error: unknown): boolean {
595-
if (error instanceof Error && error.name === "SQLiteError") {
596-
const msg = error.message.toLowerCase();
597-
return (
598-
msg.includes("no such column") ||
599-
msg.includes("no such table") ||
600-
msg.includes("has no column named") ||
601-
msg.includes("on conflict clause does not match")
602-
);
599+
if (!(error instanceof Error)) {
600+
return false;
603601
}
604-
return false;
602+
const msg = error.message.toLowerCase();
603+
return (
604+
msg.includes("no such column") ||
605+
msg.includes("no such table") ||
606+
msg.includes("has no column named") ||
607+
msg.includes("on conflict clause does not match")
608+
);
605609
}
606610

607611
/**
@@ -610,14 +614,17 @@ function isSchemaError(error: unknown): boolean {
610614
* This happens when the CLI's local database file or its containing directory
611615
* lacks write permissions (e.g., installed globally in a protected path,
612616
* read-only filesystem, or changed permissions).
617+
*
618+
* Matches by message content rather than error name because `bun:sqlite`
619+
* throws `SQLiteError` while `node:sqlite` throws plain `Error`.
613620
*/
614621
export function isReadonlyError(error: unknown): boolean {
615-
if (error instanceof Error && error.name === "SQLiteError") {
616-
return error.message
617-
.toLowerCase()
618-
.includes("attempt to write a readonly database");
622+
if (!(error instanceof Error)) {
623+
return false;
619624
}
620-
return false;
625+
return error.message
626+
.toLowerCase()
627+
.includes("attempt to write a readonly database");
621628
}
622629

623630
/** Result of a repair attempt */

src/lib/db/sqlite.ts

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
/**
2+
* SQLite adapter wrapping node:sqlite's DatabaseSync with a convenient API.
3+
*
4+
* This module is the single import point for all SQLite access in the
5+
* codebase. It provides a `.query(sql).get()` / `.all()` / `.run()`
6+
* interface and a manual `transaction()` wrapper.
7+
*
8+
* Uses `node:sqlite` (Node 22+) as the backing implementation. Falls back
9+
* to `bun:sqlite` when `node:sqlite` is unavailable (Bun runtime) — this
10+
* fallback will be removed once the test runner migrates off Bun.
11+
*/
12+
13+
import { logger } from "../logger.js";
14+
15+
const log = logger.withTag("sqlite");
16+
17+
/** Valid SQLite binding value. */
18+
export type SQLQueryBindings =
19+
| string
20+
| number
21+
| bigint
22+
| boolean
23+
| null
24+
| Uint8Array
25+
| undefined;
26+
27+
/**
28+
* Prepared statement wrapper exposing `.get()`, `.all()`, `.run()`.
29+
*
30+
* Uses a Proxy to pass through any additional driver-specific methods
31+
* (e.g. bun:sqlite's `.values()`) while normalising `.get()` to return
32+
* `null` (not `undefined`) for no-row results.
33+
*/
34+
type StatementWrapper = {
35+
get(...params: SQLQueryBindings[]): Record<string, SQLQueryBindings> | null;
36+
all(...params: SQLQueryBindings[]): Record<string, SQLQueryBindings>[];
37+
run(...params: SQLQueryBindings[]): void;
38+
/** Allow driver-specific methods (e.g. bun:sqlite `.values()`) to pass through. */
39+
[method: string]: unknown;
40+
};
41+
42+
// biome-ignore lint/suspicious/noExplicitAny: backing driver types vary
43+
function wrapStatement(stmt: any): StatementWrapper {
44+
return new Proxy(stmt, {
45+
get(target, prop) {
46+
if (prop === "get") {
47+
return (...params: SQLQueryBindings[]) =>
48+
// node:sqlite returns undefined for no rows; bun:sqlite returns null.
49+
// Normalise to null so callers can rely on a single sentinel.
50+
(target.get(...params) as Record<string, SQLQueryBindings>) ?? null;
51+
}
52+
const value = Reflect.get(target, prop);
53+
if (typeof value === "function") {
54+
return value.bind(target);
55+
}
56+
return value;
57+
},
58+
}) as StatementWrapper;
59+
}
60+
61+
/**
62+
* Resolve the underlying SQLite database constructor.
63+
*
64+
* Prefers `node:sqlite` (Node 22+). Falls back to `bun:sqlite` when
65+
* `node:sqlite` is unavailable (Bun runtime). The fallback will be
66+
* removed once the test runner migrates off Bun.
67+
*/
68+
function getSqliteConstructor(): new (
69+
path: string
70+
) => {
71+
exec(sql: string): void;
72+
close(): void;
73+
} {
74+
try {
75+
return require("node:sqlite").DatabaseSync;
76+
} catch (error) {
77+
log.debug("node:sqlite unavailable, falling back to bun:sqlite", error);
78+
return require("bun:sqlite").Database;
79+
}
80+
}
81+
82+
// biome-ignore lint/suspicious/noExplicitAny: resolved dynamically
83+
const SqliteImpl: any = getSqliteConstructor();
84+
85+
/**
86+
* SQLite database wrapper.
87+
*
88+
* - `exec(sql)` — execute raw SQL (DDL, multi-statement)
89+
* - `query(sql)` — prepare a statement → `.get()` / `.all()` / `.run()`
90+
* - `close()` — close the connection
91+
* - `transaction(fn)` — wrap a function in BEGIN/COMMIT/ROLLBACK
92+
*/
93+
export class Database {
94+
// biome-ignore lint/suspicious/noExplicitAny: backing driver resolved at runtime
95+
private readonly db: any;
96+
97+
constructor(path: string) {
98+
this.db = new SqliteImpl(path);
99+
}
100+
101+
/** Execute raw SQL (DDL statements, multi-statement strings). */
102+
exec(sql: string): void {
103+
this.db.exec(sql);
104+
}
105+
106+
/**
107+
* Prepare a SQL statement.
108+
* Returns a wrapper with `.get()`, `.all()`, `.run()`.
109+
*
110+
* Uses bun:sqlite's `.query()` (cached statements) when available,
111+
* falling back to node:sqlite's `.prepare()`.
112+
*/
113+
query(sql: string): StatementWrapper {
114+
// bun:sqlite exposes both .query() (cached) and .prepare() (fresh).
115+
// Prefer .query() to preserve the caching semantics all consumers
116+
// were written against. node:sqlite only has .prepare().
117+
const prepFn = this.db.query ?? this.db.prepare;
118+
return wrapStatement(prepFn.call(this.db, sql));
119+
}
120+
121+
/** Close the database connection. */
122+
close(): void {
123+
this.db.close();
124+
}
125+
126+
/**
127+
* Wrap a function in a transaction. Returns a callable that executes
128+
* the function within BEGIN/COMMIT, with ROLLBACK on error.
129+
*/
130+
transaction<T>(fn: () => T): () => T {
131+
// bun:sqlite has native transaction(); node:sqlite does not
132+
if (typeof this.db.transaction === "function") {
133+
return this.db.transaction(fn);
134+
}
135+
return () => {
136+
this.db.exec("BEGIN");
137+
try {
138+
const result = fn();
139+
this.db.exec("COMMIT");
140+
return result;
141+
} catch (error) {
142+
this.db.exec("ROLLBACK");
143+
throw error;
144+
}
145+
};
146+
}
147+
}

src/lib/db/utils.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,10 @@
33
* Reduces boilerplate for UPSERT and other repetitive patterns.
44
*/
55

6-
import type { SQLQueryBindings } from "bun:sqlite";
7-
86
import { getDatabase } from "./index.js";
7+
import type { SQLQueryBindings } from "./sqlite.js";
98

10-
/** Valid SQLite binding value (matches bun:sqlite's SQLQueryBindings) */
9+
/** Valid SQLite binding value (re-exported from sqlite.ts adapter) */
1110
export type SqlValue = SQLQueryBindings;
1211

1312
/**

test/commands/cli/fix.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010
* and code spans have backticks stripped.
1111
*/
1212

13-
import { Database } from "bun:sqlite";
1413
import { afterEach, describe, expect, mock, spyOn, test } from "bun:test";
1514
import { chmodSync, statSync } from "node:fs";
1615
import { join } from "node:path";
@@ -21,6 +20,7 @@ import {
2120
generatePreMigrationTableDDL,
2221
initSchema,
2322
} from "../../../src/lib/db/schema.js";
23+
import { Database } from "../../../src/lib/db/sqlite.js";
2424
import { EXIT, OutputError } from "../../../src/lib/errors.js";
2525
import { useTestConfigDir } from "../../helpers.js";
2626

test/lib/db/schema.test.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
* Tests for database schema repair functions.
33
*/
44

5-
import { Database } from "bun:sqlite";
65
import { describe, expect, test } from "bun:test";
76
import { join } from "node:path";
87
import {
@@ -18,6 +17,7 @@ import {
1817
runMigrations,
1918
tableExists,
2019
} from "../../../src/lib/db/schema.js";
20+
import { Database } from "../../../src/lib/db/sqlite.js";
2121
import { getMetadata } from "../../../src/lib/db/utils.js";
2222
import { useTestConfigDir } from "../../helpers.js";
2323

@@ -281,9 +281,11 @@ describe("isReadonlyError", () => {
281281
expect(isReadonlyError(error)).toBe(false);
282282
});
283283

284-
test("returns false for non-SQLiteError", () => {
284+
test("returns true for plain Error with readonly message", () => {
285+
// node:sqlite throws plain Error (not SQLiteError) — the check
286+
// must match by message content to work across runtimes.
285287
const error = new Error("attempt to write a readonly database");
286-
expect(isReadonlyError(error)).toBe(false);
288+
expect(isReadonlyError(error)).toBe(true);
287289
});
288290

289291
test("returns false for non-Error values", () => {

0 commit comments

Comments
 (0)