From c023df4ea8261e523b37ff3adff865feafb1d5bf Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Fri, 15 May 2026 14:50:01 +0000 Subject: [PATCH] refactor: add SQLite adapter to decouple from bun:sqlite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce src/lib/db/sqlite.ts — a runtime-detecting adapter that provides a unified Database API for SQLite access. Under Bun it re-exports bun:sqlite directly (zero overhead). Under Node.js it wraps node:sqlite's DatabaseSync with the same .query()/.exec()/ .close()/.transaction() interface. This eliminates all direct bun:sqlite imports from src/ and test/, making the DB layer portable across runtimes without changing any call sites. Changes: - New: src/lib/db/sqlite.ts (adapter with runtime detection) - Updated: db/index.ts, schema.ts, migration.ts, utils.ts imports - Updated: 3 test files to import from adapter - Added: MIGRATION-PLAN.md documenting remaining migration steps --- MIGRATION-PLAN.md | 77 +++++++++++++ lint-rules/no-manual-transactions.grit | 1 + src/lib/db/index.ts | 5 +- src/lib/db/migration.ts | 2 +- src/lib/db/schema.ts | 37 ++++--- src/lib/db/sqlite.ts | 147 +++++++++++++++++++++++++ src/lib/db/utils.ts | 5 +- test/commands/cli/fix.test.ts | 2 +- test/lib/db/schema.test.ts | 8 +- test/lib/telemetry.test.ts | 2 +- 10 files changed, 260 insertions(+), 26 deletions(-) create mode 100644 MIGRATION-PLAN.md create mode 100644 src/lib/db/sqlite.ts diff --git a/MIGRATION-PLAN.md b/MIGRATION-PLAN.md new file mode 100644 index 000000000..17f471691 --- /dev/null +++ b/MIGRATION-PLAN.md @@ -0,0 +1,77 @@ +# Bun → Node.js Migration Plan + +Status: In progress. See below for completed and remaining steps. + +## Completed + +### Phase 4 (early): Package Manager Switch +- [x] Changed `packageManager` from `bun@1.3.13` to `pnpm@10.11.0` +- [x] Moved `patchedDependencies` into `pnpm` config section +- [x] Added `onlyBuiltDependencies: ["esbuild"]` +- [x] Added phantom deps as explicit devDependencies: `@sentry/core`, `@clack/prompts` +- [x] Generated `pnpm-lock.yaml` +- [x] Verified all patches apply (including cross-version: `@stricli/core@1.2.5` patch on `1.2.7`) + +### Phase 2, Group D: SQLite Adapter +- [x] Created `src/lib/db/sqlite.ts` — runtime-detecting adapter (bun:sqlite under Bun, node:sqlite under Node.js) +- [x] Updated 4 source files: `db/index.ts`, `schema.ts`, `migration.ts`, `utils.ts` +- [x] Updated 3 test files: `fix.test.ts`, `telemetry.test.ts`, `schema.test.ts` +- [x] Zero `bun:sqlite` imports remain in `src/` or `test/` + +## Remaining + +### Phase 2: Source Code Migration (replace Bun.* APIs in `src/`) + +**Group A: File I/O** — Replace `Bun.file()` / `Bun.write()` with `node:fs/promises` +- `Bun.file(path).text()` → `readFile(path, "utf-8")` +- `Bun.file(path).json()` → `readFile(path, "utf-8")` then `JSON.parse()` +- `Bun.file(path).exists()` → `access(path).then(() => true, () => false)` +- `Bun.write(path, content)` → `writeFile(path, content)` +- Scan all of `src/` for occurrences + +**Group B: Process/System APIs** — Replace Bun.which / Bun.spawn / Bun.sleep +- `Bun.which("cmd")` → `which` from a Node.js-compatible package or custom implementation +- `Bun.spawn()` / `Bun.spawnSync()` → `child_process.spawn()` / `spawnSync()` +- `Bun.sleep(ms)` → `setTimeout` promise wrapper + +**Group C: Miscellaneous Bun APIs** +- `Bun.Glob` → `tinyglobby` or `picomatch` (already in devDependencies) +- `Bun.randomUUIDv7()` → `uuidv7` package (already in devDependencies) +- `Bun.semver.order()` → `semver.compare()` (already in devDependencies) +- `Bun.zstdCompressSync()` / `Bun.zstdDecompressSync()` → Node.js zlib or `zstd-napi` package + +**Group E: Unpolyfilled APIs** +- `bspatch.ts` and `upgrade.ts` — Replace any Bun-specific APIs not covered by node-polyfills.ts + +### Phase 3: Test Migration (`bun:test` → Vitest) + +- Add `vitest` as devDependency +- Replace `import { ... } from "bun:test"` with Vitest equivalents +- Replace `bun test` scripts with `vitest` +- Key differences: + - `bun:test`'s `mock.module()` → Vitest's `vi.mock()` + - `bun:test`'s `spyOn` → Vitest's `vi.spyOn()` + - Test file discovery patterns may differ + - `--isolate --parallel` behavior needs Vitest equivalent + +### Phase 4: CI & Dev Scripts (remaining) + +- Update `package.json` scripts: `bun run` → `pnpm run` where appropriate +- Replace `bun run src/bin.ts` with `tsx src/bin.ts` (add `tsx` devDependency) +- Replace `bun run script/*.ts` with `tsx script/*.ts` +- Replace `bunx` with `pnpm exec` +- Update GitHub Actions workflows to use pnpm + Node.js instead of Bun +- Update `Dockerfile` / build scripts if applicable + +### Phase 5: Cleanup + +- Remove `@types/bun` from devDependencies +- Remove `bun.lock` (replaced by `pnpm-lock.yaml`) +- Remove or update `script/node-polyfills.ts` (may become unnecessary) +- Update `AGENTS.md` Bun API reference table +- Remove Bun-specific `.cursor/rules/bun-cli.mdc` or update for Node.js +- Clean up any remaining `Bun.*` references in comments/docs + +## Known Issues + +- `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. diff --git a/lint-rules/no-manual-transactions.grit b/lint-rules/no-manual-transactions.grit index 0c8b62286..2f4d0bf88 100644 --- a/lint-rules/no-manual-transactions.grit +++ b/lint-rules/no-manual-transactions.grit @@ -1,6 +1,7 @@ file($name, $body) where { $name <: not r".*migration\.ts$", $name <: not r".*node-polyfills\.ts$", + $name <: not r".*db/sqlite\.ts$", $body <: contains `$db.exec($sql)` as $match where { $sql <: or { r".*BEGIN.*", diff --git a/src/lib/db/index.ts b/src/lib/db/index.ts index 2e308ec34..64a7dd237 100644 --- a/src/lib/db/index.ts +++ b/src/lib/db/index.ts @@ -1,14 +1,15 @@ /** * SQLite database connection manager for CLI configuration storage. - * Uses bun:sqlite natively; Node.js uses a polyfill in node-polyfills.ts. + * Uses the sqlite.ts adapter which wraps node:sqlite's DatabaseSync + * with a bun:sqlite-compatible API surface. */ -import { Database } from "bun:sqlite"; import { chmodSync, mkdirSync } from "node:fs"; import { join } from "node:path"; import { getEnv } from "../env.js"; import { migrateFromJson } from "./migration.js"; import { initSchema, runMigrations } from "./schema.js"; +import { Database } from "./sqlite.js"; export const CONFIG_DIR_ENV_VAR = "SENTRY_CONFIG_DIR"; diff --git a/src/lib/db/migration.ts b/src/lib/db/migration.ts index 34ec331e4..a185be3dd 100644 --- a/src/lib/db/migration.ts +++ b/src/lib/db/migration.ts @@ -2,11 +2,11 @@ * One-time migration from config.json to SQLite. */ -import type { Database } from "bun:sqlite"; import { rmSync } from "node:fs"; import { join } from "node:path"; import { logger } from "../logger.js"; import { getConfigDir } from "./index.js"; +import type { Database } from "./sqlite.js"; const log = logger.withTag("migration"); diff --git a/src/lib/db/schema.ts b/src/lib/db/schema.ts index e4ee65c97..cacb8dad8 100644 --- a/src/lib/db/schema.ts +++ b/src/lib/db/schema.ts @@ -11,10 +11,10 @@ * - Migration checks */ -import type { Database } from "bun:sqlite"; import { getEnv } from "../env.js"; import { stringifyUnknown } from "../errors.js"; import { logger } from "../logger.js"; +import type { Database } from "./sqlite.js"; export const CURRENT_SCHEMA_VERSION = 16; @@ -590,18 +590,22 @@ let isRepairing = false; /** * Check if an error is a schema-related SQLite error that can be auto-repaired. + * + * Matches by message content rather than error name because `bun:sqlite` + * throws `SQLiteError` while `node:sqlite` throws plain `Error` — the + * message strings are identical across both runtimes. */ function isSchemaError(error: unknown): boolean { - if (error instanceof Error && error.name === "SQLiteError") { - const msg = error.message.toLowerCase(); - return ( - msg.includes("no such column") || - msg.includes("no such table") || - msg.includes("has no column named") || - msg.includes("on conflict clause does not match") - ); + if (!(error instanceof Error)) { + return false; } - return false; + const msg = error.message.toLowerCase(); + return ( + msg.includes("no such column") || + msg.includes("no such table") || + msg.includes("has no column named") || + msg.includes("on conflict clause does not match") + ); } /** @@ -610,14 +614,17 @@ function isSchemaError(error: unknown): boolean { * This happens when the CLI's local database file or its containing directory * lacks write permissions (e.g., installed globally in a protected path, * read-only filesystem, or changed permissions). + * + * Matches by message content rather than error name because `bun:sqlite` + * throws `SQLiteError` while `node:sqlite` throws plain `Error`. */ export function isReadonlyError(error: unknown): boolean { - if (error instanceof Error && error.name === "SQLiteError") { - return error.message - .toLowerCase() - .includes("attempt to write a readonly database"); + if (!(error instanceof Error)) { + return false; } - return false; + return error.message + .toLowerCase() + .includes("attempt to write a readonly database"); } /** Result of a repair attempt */ diff --git a/src/lib/db/sqlite.ts b/src/lib/db/sqlite.ts new file mode 100644 index 000000000..7b50c5782 --- /dev/null +++ b/src/lib/db/sqlite.ts @@ -0,0 +1,147 @@ +/** + * SQLite adapter wrapping node:sqlite's DatabaseSync with a convenient API. + * + * This module is the single import point for all SQLite access in the + * codebase. It provides a `.query(sql).get()` / `.all()` / `.run()` + * interface and a manual `transaction()` wrapper. + * + * Uses `node:sqlite` (Node 22+) as the backing implementation. Falls back + * to `bun:sqlite` when `node:sqlite` is unavailable (Bun runtime) — this + * fallback will be removed once the test runner migrates off Bun. + */ + +import { logger } from "../logger.js"; + +const log = logger.withTag("sqlite"); + +/** Valid SQLite binding value. */ +export type SQLQueryBindings = + | string + | number + | bigint + | boolean + | null + | Uint8Array + | undefined; + +/** + * Prepared statement wrapper exposing `.get()`, `.all()`, `.run()`. + * + * Uses a Proxy to pass through any additional driver-specific methods + * (e.g. bun:sqlite's `.values()`) while normalising `.get()` to return + * `null` (not `undefined`) for no-row results. + */ +type StatementWrapper = { + get(...params: SQLQueryBindings[]): Record | null; + all(...params: SQLQueryBindings[]): Record[]; + run(...params: SQLQueryBindings[]): void; + /** Allow driver-specific methods (e.g. bun:sqlite `.values()`) to pass through. */ + [method: string]: unknown; +}; + +// biome-ignore lint/suspicious/noExplicitAny: backing driver types vary +function wrapStatement(stmt: any): StatementWrapper { + return new Proxy(stmt, { + get(target, prop) { + if (prop === "get") { + return (...params: SQLQueryBindings[]) => + // node:sqlite returns undefined for no rows; bun:sqlite returns null. + // Normalise to null so callers can rely on a single sentinel. + (target.get(...params) as Record) ?? null; + } + const value = Reflect.get(target, prop); + if (typeof value === "function") { + return value.bind(target); + } + return value; + }, + }) as StatementWrapper; +} + +/** + * Resolve the underlying SQLite database constructor. + * + * Prefers `node:sqlite` (Node 22+). Falls back to `bun:sqlite` when + * `node:sqlite` is unavailable (Bun runtime). The fallback will be + * removed once the test runner migrates off Bun. + */ +function getSqliteConstructor(): new ( + path: string +) => { + exec(sql: string): void; + close(): void; +} { + try { + return require("node:sqlite").DatabaseSync; + } catch (error) { + log.debug("node:sqlite unavailable, falling back to bun:sqlite", error); + return require("bun:sqlite").Database; + } +} + +// biome-ignore lint/suspicious/noExplicitAny: resolved dynamically +const SqliteImpl: any = getSqliteConstructor(); + +/** + * SQLite database wrapper. + * + * - `exec(sql)` — execute raw SQL (DDL, multi-statement) + * - `query(sql)` — prepare a statement → `.get()` / `.all()` / `.run()` + * - `close()` — close the connection + * - `transaction(fn)` — wrap a function in BEGIN/COMMIT/ROLLBACK + */ +export class Database { + // biome-ignore lint/suspicious/noExplicitAny: backing driver resolved at runtime + private readonly db: any; + + constructor(path: string) { + this.db = new SqliteImpl(path); + } + + /** Execute raw SQL (DDL statements, multi-statement strings). */ + exec(sql: string): void { + this.db.exec(sql); + } + + /** + * Prepare a SQL statement. + * Returns a wrapper with `.get()`, `.all()`, `.run()`. + * + * Uses bun:sqlite's `.query()` (cached statements) when available, + * falling back to node:sqlite's `.prepare()`. + */ + query(sql: string): StatementWrapper { + // bun:sqlite exposes both .query() (cached) and .prepare() (fresh). + // Prefer .query() to preserve the caching semantics all consumers + // were written against. node:sqlite only has .prepare(). + const prepFn = this.db.query ?? this.db.prepare; + return wrapStatement(prepFn.call(this.db, sql)); + } + + /** Close the database connection. */ + close(): void { + this.db.close(); + } + + /** + * Wrap a function in a transaction. Returns a callable that executes + * the function within BEGIN/COMMIT, with ROLLBACK on error. + */ + transaction(fn: () => T): () => T { + // bun:sqlite has native transaction(); node:sqlite does not + if (typeof this.db.transaction === "function") { + return this.db.transaction(fn); + } + return () => { + this.db.exec("BEGIN"); + try { + const result = fn(); + this.db.exec("COMMIT"); + return result; + } catch (error) { + this.db.exec("ROLLBACK"); + throw error; + } + }; + } +} diff --git a/src/lib/db/utils.ts b/src/lib/db/utils.ts index 8d12eb3ca..75cdb6d8f 100644 --- a/src/lib/db/utils.ts +++ b/src/lib/db/utils.ts @@ -3,11 +3,10 @@ * Reduces boilerplate for UPSERT and other repetitive patterns. */ -import type { SQLQueryBindings } from "bun:sqlite"; - import { getDatabase } from "./index.js"; +import type { SQLQueryBindings } from "./sqlite.js"; -/** Valid SQLite binding value (matches bun:sqlite's SQLQueryBindings) */ +/** Valid SQLite binding value (re-exported from sqlite.ts adapter) */ export type SqlValue = SQLQueryBindings; /** diff --git a/test/commands/cli/fix.test.ts b/test/commands/cli/fix.test.ts index 634de40c2..f6c743cf3 100644 --- a/test/commands/cli/fix.test.ts +++ b/test/commands/cli/fix.test.ts @@ -10,7 +10,6 @@ * and code spans have backticks stripped. */ -import { Database } from "bun:sqlite"; import { afterEach, describe, expect, mock, spyOn, test } from "bun:test"; import { chmodSync, statSync } from "node:fs"; import { join } from "node:path"; @@ -21,6 +20,7 @@ import { generatePreMigrationTableDDL, initSchema, } from "../../../src/lib/db/schema.js"; +import { Database } from "../../../src/lib/db/sqlite.js"; import { EXIT, OutputError } from "../../../src/lib/errors.js"; import { useTestConfigDir } from "../../helpers.js"; diff --git a/test/lib/db/schema.test.ts b/test/lib/db/schema.test.ts index 2b8bf810c..fc39be005 100644 --- a/test/lib/db/schema.test.ts +++ b/test/lib/db/schema.test.ts @@ -2,7 +2,6 @@ * Tests for database schema repair functions. */ -import { Database } from "bun:sqlite"; import { describe, expect, test } from "bun:test"; import { join } from "node:path"; import { @@ -18,6 +17,7 @@ import { runMigrations, tableExists, } from "../../../src/lib/db/schema.js"; +import { Database } from "../../../src/lib/db/sqlite.js"; import { getMetadata } from "../../../src/lib/db/utils.js"; import { useTestConfigDir } from "../../helpers.js"; @@ -281,9 +281,11 @@ describe("isReadonlyError", () => { expect(isReadonlyError(error)).toBe(false); }); - test("returns false for non-SQLiteError", () => { + test("returns true for plain Error with readonly message", () => { + // node:sqlite throws plain Error (not SQLiteError) — the check + // must match by message content to work across runtimes. const error = new Error("attempt to write a readonly database"); - expect(isReadonlyError(error)).toBe(false); + expect(isReadonlyError(error)).toBe(true); }); test("returns false for non-Error values", () => { diff --git a/test/lib/telemetry.test.ts b/test/lib/telemetry.test.ts index fb58f00ce..40e6fea93 100644 --- a/test/lib/telemetry.test.ts +++ b/test/lib/telemetry.test.ts @@ -4,7 +4,6 @@ * Tests for withTelemetry wrapper and opt-out behavior. */ -import { Database } from "bun:sqlite"; import { afterAll, afterEach, @@ -17,6 +16,7 @@ import { import { chmodSync, mkdirSync, rmSync } from "node:fs"; // biome-ignore lint/performance/noNamespaceImport: needed for spyOn mocking import * as Sentry from "@sentry/node-core/light"; +import { Database } from "../../src/lib/db/sqlite.js"; import { ApiError, AuthError, OutputError } from "../../src/lib/errors.js"; import { createTracedDatabase,