-
Notifications
You must be signed in to change notification settings - Fork 134
fix: --dir silently read a populated warehouse store as empty #1204
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
| * DuckDB driver using the `duckdb` package. | ||
| */ | ||
|
|
||
| import { assertStoreExists, requireStorePath } from "./file-store" | ||
| import type { ConnectionConfig, Connector, ConnectorResult, ExecuteOptions, SchemaColumn } from "./types" | ||
| import { loadOptionalDriver } from "./resolve" | ||
|
|
||
|
|
@@ -10,7 +11,9 @@ export async function connect(config: ConnectionConfig): Promise<Connector> { | |
| duckdb = await loadOptionalDriver("duckdb", "duckdb") | ||
| duckdb = duckdb.default || duckdb | ||
|
|
||
| const dbPath = (config.path as string) ?? ":memory:" | ||
| // altimate_change start — a missing path must fail loudly, not become :memory: | ||
| const dbPath = requireStorePath(config, "DuckDB") | ||
|
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. P2: When an intentional unnamed in-memory connection omits Prompt for AI agents |
||
| // altimate_change end | ||
| let db: any | ||
| let connection: any | ||
|
|
||
|
|
@@ -48,6 +51,9 @@ export async function connect(config: ConnectionConfig): Promise<Connector> { | |
|
|
||
| return { | ||
| async connect() { | ||
| // altimate_change start — never conjure an empty store on open | ||
| assertStoreExists(config, dbPath, "DuckDB") | ||
|
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. P2: For DuckDB, a Prompt for AI agents |
||
| // altimate_change end | ||
| // altimate_change start — retry with read-only on lock errors | ||
| const tryConnect = (accessMode?: string): Promise<any> => | ||
| new Promise<any>((resolve, reject) => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| /** | ||
| * Shared guards for file-backed drivers (DuckDB, SQLite). | ||
| * | ||
| * Both engines create an empty database when asked to open a file that does | ||
| * not exist. For a warehouse connection that is never what the caller wants: | ||
| * a mistyped or mis-resolved path then yields a working connector over an | ||
| * empty database, so every query succeeds and returns nothing. An agent handed | ||
| * that result reports "no tables" instead of an error. | ||
| * | ||
| * Opening a store is therefore read-or-fail by default. Creation is opt-in via | ||
| * `create: true`, which the tools that deliberately materialize a local store | ||
| * (local test scratch databases, schema sync targets) pass explicitly. | ||
| */ | ||
|
|
||
| import * as fs from "fs" | ||
| import type { ConnectionConfig } from "./types" | ||
|
|
||
| /** | ||
| * Whether `dbPath` names a file on the local filesystem, and so can be | ||
| * existence-checked before the driver opens it. | ||
| * | ||
| * Only the exact string `:memory:` is an in-memory database. Both engines | ||
| * treat `:memory:named` — and any other colon-prefixed name — as an ordinary | ||
| * (if oddly named) file: DuckDB really does write a file called | ||
| * `:memory:named`, so those must stay inside the guard. An empty path is | ||
| * DuckDB's in-memory database and SQLite's anonymous temporary one; neither | ||
| * touches disk. | ||
| * | ||
| * A scheme-qualified target is not a local file: MotherDuck (`md:`), object | ||
| * storage (`s3://`), DuckLake, and any other scheme a DuckDB extension | ||
| * provides. Those are left to the driver, which reports an unknown scheme as a | ||
| * missing-extension error rather than silently creating anything. The pattern | ||
| * requires two or more characters before the colon so a Windows drive letter | ||
| * (`C:\data\wh.duckdb`) stays a path. | ||
| */ | ||
| export function isLocalFilePath(dbPath: string): boolean { | ||
| if (dbPath === "" || dbPath === ":memory:") return false | ||
| if (/^[a-zA-Z][a-zA-Z0-9+.-]+:/.test(dbPath)) return false | ||
|
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. P1: When SQLite is configured with a missing absolute Prompt for AI agentsThere 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. P1: On SQLite, Prompt for AI agents |
||
| return true | ||
| } | ||
|
|
||
| /** | ||
| * The store path a file-backed connection names, or a loud failure. | ||
| * | ||
| * Both drivers used to read `(config.path as string) ?? ":memory:"`. That turns | ||
| * ANY failure to carry a path — a config the registry never loaded, a field | ||
| * under the wrong name, a lookup that fell through — into a successful | ||
| * connection over an empty in-memory database. Every query then returns no rows | ||
| * and no error, which reads as a healthy warehouse that happens to be empty. | ||
| * | ||
| * It is a worse failure than creating a store on disk: a stray file can at | ||
| * least be found afterwards, whereas an in-memory database leaves nothing | ||
| * behind to explain the empty answer. `:memory:` remains available, but only | ||
| * when a caller asks for it by name. | ||
| */ | ||
| export function requireStorePath(config: ConnectionConfig, engine: string): string { | ||
| const value = config.path | ||
| if (typeof value === "string" && value !== "") return value | ||
| throw new Error( | ||
|
Comment on lines
+58
to
+59
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.
When the DuckDB addon is installed, Useful? React with 👍 / 👎. |
||
| `${engine} connection is missing its "path". A file-backed warehouse must name its database explicitly — ` + | ||
| `falling back to an in-memory database would answer every query with no rows and no error, ` + | ||
| `which is indistinguishable from a healthy but empty warehouse. ` + | ||
| `Set "path" to the database file, or to ":memory:" if a throwaway empty database is genuinely what you want.`, | ||
| ) | ||
| } | ||
|
|
||
| /** Whether the caller explicitly opted in to creating the store. */ | ||
| export function allowsCreate(config: ConnectionConfig): boolean { | ||
| return config.create === true | ||
| } | ||
|
|
||
| /** | ||
| * Throw unless the store is safe to open: it already exists, the caller opted | ||
| * in to creating it, or the path is not a local file at all. | ||
| * | ||
| * @param engine Human-readable engine name used in the error message. | ||
| * @param allowCreate Whether this open will actually create the store. Defaults | ||
| * to the config's `create` flag; a driver passes it explicitly when its own | ||
| * options can veto creation — SQLite never creates a read-only connection. | ||
| */ | ||
| export function assertStoreExists( | ||
| config: ConnectionConfig, | ||
| dbPath: string, | ||
| engine: string, | ||
| allowCreate: boolean = allowsCreate(config), | ||
| ): void { | ||
| if (allowCreate) return | ||
| if (!isLocalFilePath(dbPath)) return | ||
| if (fs.existsSync(dbPath)) return | ||
|
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. P3: fs.existsSync() also returns true when dbPath is a directory, so a wrong path pointing at a directory bypasses the guard's clear error and surfaces a generic engine error later. Check it is a file (fs.statSync(...).isFile()) before treating the path as present. Prompt for AI agentsThere 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. P2: If a DuckDB file is removed after this check but before Prompt for AI agents |
||
| throw new Error( | ||
| `${engine} database file not found: "${dbPath}". ` + | ||
| `Opening a warehouse connection never creates the database — an empty store would answer every query with no rows. ` + | ||
| `Check the "path" in your connection config (relative paths resolve against the config file's directory, not the current directory), ` + | ||
| `or pass "create": true if this store is meant to be created.`, | ||
| ) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,19 +4,32 @@ | |
| */ | ||
|
|
||
| import { Database } from "bun:sqlite" | ||
| import { allowsCreate, assertStoreExists, requireStorePath } from "./file-store" | ||
| import type { ConnectionConfig, Connector, ConnectorResult, ExecuteOptions, SchemaColumn } from "./types" | ||
|
|
||
| export async function connect(config: ConnectionConfig): Promise<Connector> { | ||
| const dbPath = (config.path as string) ?? ":memory:" | ||
| // altimate_change start — a missing path must fail loudly, not become :memory: | ||
| const dbPath = requireStorePath(config, "SQLite") | ||
| // altimate_change end | ||
| let db: Database | null = null | ||
|
|
||
| return { | ||
| async connect() { | ||
| const isReadonly = config.readonly === true | ||
| // altimate_change start — never conjure an empty store on open. | ||
| // A read-only connection never creates, so `create: true` cannot excuse a | ||
| // missing file there; the guard is told the effective decision. | ||
| const willCreate = !isReadonly && allowsCreate(config) | ||
| assertStoreExists(config, dbPath, "SQLite", willCreate) | ||
|
cubic-dev-ai[bot] marked this conversation as resolved.
|
||
| db = new Database(dbPath, { | ||
| readonly: isReadonly, | ||
| create: !isReadonly, | ||
| // `create` alone is no longer implied by "not readonly", so the | ||
| // read-write flag has to be explicit — bun:sqlite rejects an options | ||
| // object that sets no open flag at all. | ||
| readwrite: !isReadonly, | ||
| create: willCreate, | ||
|
Comment on lines
+29
to
+30
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.
When Useful? React with 👍 / 👎. |
||
| }) | ||
| // altimate_change end | ||
| if (!isReadonly) { | ||
| db.exec("PRAGMA journal_mode = WAL") | ||
| } | ||
|
|
||
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.
The warehouse documentation still tells users that
pathis optional and may be omitted for an in-memory DuckDB connection, butrequireStorePath()now rejects every omitted path. A user following this table with{ "type": "duckdb" }therefore gets a missing-path error instead of the documented in-memory database; either mark the field required and remove the omission guidance or preserve the documented fallback.Useful? React with 👍 / 👎.