-
Notifications
You must be signed in to change notification settings - Fork 18.9k
refactor: abstract SQLite behind runtime-conditional #db import #18316
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| import { Database } from "bun:sqlite" | ||
| import { drizzle } from "drizzle-orm/bun-sqlite" | ||
|
|
||
| export function init(path: string) { | ||
| const sqlite = new Database(path, { create: true }) | ||
| const db = drizzle({ client: sqlite }) | ||
| return db | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| import { DatabaseSync } from "node:sqlite" | ||
| import { drizzle } from "drizzle-orm/node-sqlite" | ||
|
|
||
| export function init(path: string) { | ||
| const sqlite = new DatabaseSync(path) | ||
| const db = drizzle({ client: sqlite }) | ||
| return db | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,4 @@ | ||||||||||||||||||||||||||||
| import { Database as BunDatabase } from "bun:sqlite" | ||||||||||||||||||||||||||||
| import { drizzle, type SQLiteBunDatabase } from "drizzle-orm/bun-sqlite" | ||||||||||||||||||||||||||||
| import { type SQLiteBunDatabase } from "drizzle-orm/bun-sqlite" | ||||||||||||||||||||||||||||
| import { migrate } from "drizzle-orm/bun-sqlite/migrator" | ||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The whole point of this PR is to abstract the runtime-specific database layer, but The migrator should either be exported from
Suggested change
And correspondingly, |
||||||||||||||||||||||||||||
| import { type SQLiteTransaction } from "drizzle-orm/sqlite-core" | ||||||||||||||||||||||||||||
| export * from "drizzle-orm" | ||||||||||||||||||||||||||||
|
|
@@ -11,10 +10,10 @@ import { NamedError } from "@opencode-ai/util/error" | |||||||||||||||||||||||||||
| import z from "zod" | ||||||||||||||||||||||||||||
| import path from "path" | ||||||||||||||||||||||||||||
| import { readFileSync, readdirSync, existsSync } from "fs" | ||||||||||||||||||||||||||||
| import * as schema from "./schema" | ||||||||||||||||||||||||||||
| import { Installation } from "../installation" | ||||||||||||||||||||||||||||
| import { Flag } from "../flag/flag" | ||||||||||||||||||||||||||||
| import { iife } from "@/util/iife" | ||||||||||||||||||||||||||||
| import { init } from "#db" | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| declare const OPENCODE_MIGRATIONS: { sql: string; timestamp: number; name: string }[] | undefined | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
|
@@ -36,17 +35,12 @@ export namespace Database { | |||||||||||||||||||||||||||
| return path.join(Global.Path.data, `opencode-${safe}.db`) | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| type Schema = typeof schema | ||||||||||||||||||||||||||||
| export type Transaction = SQLiteTransaction<"sync", void, Schema> | ||||||||||||||||||||||||||||
| export type Transaction = SQLiteTransaction<"sync", void> | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| type Client = SQLiteBunDatabase | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| type Journal = { sql: string; timestamp: number; name: string }[] | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const state = { | ||||||||||||||||||||||||||||
| sqlite: undefined as BunDatabase | undefined, | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| function time(tag: string) { | ||||||||||||||||||||||||||||
| const match = /^(\d{4})(\d{2})(\d{2})(\d{2})(\d{2})(\d{2})/.exec(tag) | ||||||||||||||||||||||||||||
| if (!match) return 0 | ||||||||||||||||||||||||||||
|
|
@@ -83,17 +77,14 @@ export namespace Database { | |||||||||||||||||||||||||||
| export const Client = lazy(() => { | ||||||||||||||||||||||||||||
| log.info("opening database", { path: Path }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const sqlite = new BunDatabase(Path, { create: true }) | ||||||||||||||||||||||||||||
| state.sqlite = sqlite | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| sqlite.run("PRAGMA journal_mode = WAL") | ||||||||||||||||||||||||||||
| sqlite.run("PRAGMA synchronous = NORMAL") | ||||||||||||||||||||||||||||
| sqlite.run("PRAGMA busy_timeout = 5000") | ||||||||||||||||||||||||||||
| sqlite.run("PRAGMA cache_size = -64000") | ||||||||||||||||||||||||||||
| sqlite.run("PRAGMA foreign_keys = ON") | ||||||||||||||||||||||||||||
| sqlite.run("PRAGMA wal_checkpoint(PASSIVE)") | ||||||||||||||||||||||||||||
| const db = init(Path) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const db = drizzle({ client: sqlite }) | ||||||||||||||||||||||||||||
| db.run("PRAGMA journal_mode = WAL") | ||||||||||||||||||||||||||||
| db.run("PRAGMA synchronous = NORMAL") | ||||||||||||||||||||||||||||
| db.run("PRAGMA busy_timeout = 5000") | ||||||||||||||||||||||||||||
| db.run("PRAGMA cache_size = -64000") | ||||||||||||||||||||||||||||
| db.run("PRAGMA foreign_keys = ON") | ||||||||||||||||||||||||||||
| db.run("PRAGMA wal_checkpoint(PASSIVE)") | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Apply schema migrations | ||||||||||||||||||||||||||||
| const entries = | ||||||||||||||||||||||||||||
|
|
@@ -117,14 +108,11 @@ export namespace Database { | |||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| export function close() { | ||||||||||||||||||||||||||||
| const sqlite = state.sqlite | ||||||||||||||||||||||||||||
| if (!sqlite) return | ||||||||||||||||||||||||||||
| sqlite.close() | ||||||||||||||||||||||||||||
| state.sqlite = undefined | ||||||||||||||||||||||||||||
| Client().$client.close() | ||||||||||||||||||||||||||||
| Client.reset() | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
Comment on lines
110
to
113
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The original implementation guarded against uninitialized state with
A simple guard can restore the original safety:
Suggested change
(Requires exposing a |
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| export type TxOrDb = SQLiteTransaction<"sync", void, any, any> | Client | ||||||||||||||||||||||||||||
| export type TxOrDb = Transaction | Client | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const ctx = Context.create<{ | ||||||||||||||||||||||||||||
| tx: TxOrDb | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ClientSQLiteBunDatabaseis imported and used as theClienttype alias (line 40:type Client = SQLiteBunDatabase). Under Node.js,init()fromdb.node.tsreturns aBetterSQLite3Database<Record<string, never>>(or equivalent drizzle node-sqlite type), not aSQLiteBunDatabase. This mismatch means allClient-typed values, including the$clientaccess inclose(), are typed against the wrong runtime object.The fix would be to either export a common
DBtype from#dbor useReturnType<typeof init>to infer the correct type.