From 13d54db59411029f6c80755b7453090e17beb417 Mon Sep 17 00:00:00 2001 From: adenzhou1350 Date: Fri, 23 Oct 2026 03:13:46 +0800 Subject: [PATCH] fix(node): preserve caller-owned storage options (#4480) `getStorageOptions` writes typed creation options back into the caller's `storageOptions` object. This makes frozen options throw and causes overrides to leak into later calls that reuse the storage map. For example, using `{ storageOptions: shared, enableV2ManifestPaths: true }` changes `shared.newTableEnableV2ManifestPaths` to `"true"`. A subsequent creation with just `{ storageOptions: shared }` then uses V2 manifest paths even if `shared` originally requested `"false"`. Merge the typed overrides into a local map instead. Their existing precedence, key normalization, and undefined/default behavior remain unchanged. This affects both `createTable` and `createEmptyTable`; no native/Rust changes. Adds six native-database regressions covering frozen creation options, frozen nested storage options with conflicting defaults, and cross-call reuse for both APIs. Validation (Linux x64 / Node 24.19.0 / official 0.41.0-beta.0 native library): - Before: all six new regressions fail; an existing namespace storage-options control passes. - After: the full connection test file passes, 43/43 (current TypeScript source, transpile-only). - Same fix on the matching 0.41.0-beta.0 source: TypeScript compilation, strict ts-jest connection tests (43/43), and TypeDoc with warnings-as-errors pass. - Biome on changed files and whitespace checks pass. A current-main native rebuild was not run. Released native declarations predate six unrelated lazy-iterator signatures; current-main typechecking reports the same six errors before and after this fix (only downstream line coordinates shift). ### Connection entry-point follow-up The same ownership problem occurs in `connect(uri, options)` and `connectNamespace`: normalization assigns a new storage map into the caller's options. Copy the options before normalization instead. The `connect(options)` overload already copies its input and is retained as a control. Nine added native-database cases cover frozen outer options (with/without storage options) and repeated reuse, including table creation and row-count checks. Before this entry-point fix, the URI/namespace cases fail (6), while the object-overload controls pass (3). Afterward the complete connection file passes 52/52 on current TypeScript with the official 0.41.0-beta.0 native library. Applying the entry-point fix to matching-release source also passes strict TypeScript, strict ts-jest (52/52), and TypeDoc. Biome passes on both newly changed files. The same six current-main/released-native lazy-iterator declaration errors remain unchanged. No Rust/native rebuild or GPU claim is added. --------- Signed-off-by: Xucheng Zhou --- nodejs/__test__/connection.test.ts | 144 +++++++++++++++++++++++++++++ nodejs/lancedb/connection.ts | 23 ++--- nodejs/lancedb/index.ts | 17 ++-- 3 files changed, 164 insertions(+), 20 deletions(-) diff --git a/nodejs/__test__/connection.test.ts b/nodejs/__test__/connection.test.ts index 355b86ccb..e32e7e626 100644 --- a/nodejs/__test__/connection.test.ts +++ b/nodejs/__test__/connection.test.ts @@ -36,6 +36,87 @@ describe("when connecting", () => { }); }); +describe.each([ + [ + "URI overload", + (uri: string, options: { storageOptions?: Record }) => + connect(uri, options), + ], + [ + "object overload", + async ( + uri: string, + options: { storageOptions?: Record }, + ) => { + const input = { uri, ...options }; + if (Object.isFrozen(options)) { + Object.freeze(input); + } + const db = await connect(input); + expect(input).toEqual({ uri, ...options }); + expect(input.storageOptions).toBe(options.storageOptions); + return db; + }, + ], + [ + "namespace", + (uri: string, options: { storageOptions?: Record }) => + connectNamespace("dir", { root: uri }, options), + ], +])("%s connection options", (_name, open) => { + let tmpDir: tmp.DirResult; + beforeEach(() => { + tmpDir = tmp.dirSync({ unsafeCleanup: true }); + }); + afterEach(() => tmpDir.removeCallback()); + + it("should accept frozen options without storage options", async () => { + const options = Object.freeze({}); + const db = await open(tmpDir.name, options); + try { + await db.createTable("frozen", [{ id: 1 }]); + await expect(db.tableNames()).resolves.toContain("frozen"); + expect(options).toEqual({}); + } finally { + await db.close(); + } + }); + + it("should accept frozen options with storage options", async () => { + const storageOptions = Object.freeze({ + newTableDataStorageVersion: "stable", + }); + const options = Object.freeze({ storageOptions }); + const db = await open(tmpDir.name, options); + try { + const table = await db.createTable("frozen", [{ id: 1 }]); + await expect(table.countRows()).resolves.toBe(1); + expect(options.storageOptions).toBe(storageOptions); + expect(storageOptions).toEqual({ newTableDataStorageVersion: "stable" }); + } finally { + await db.close(); + } + }); + + it("should preserve reused options and their storage options", async () => { + const storageOptions = { newTableDataStorageVersion: "stable" }; + const options = { storageOptions }; + for (let i = 0; i < 2; i++) { + const db = await open(tmpDir.name, options); + try { + const table = await db.createTable(`reused_${i}`, [{ id: i }]); + await expect(table.countRows()).resolves.toBe(1); + expect(options.storageOptions).toBe(storageOptions); + expect(storageOptions).toEqual({ + newTableDataStorageVersion: "stable", + }); + } finally { + await db.close(); + } + } + }); +}); + describe("given a connection", () => { let tmpDir: tmp.DirResult; let db: Connection; @@ -367,6 +448,69 @@ describe("given a connection", () => { }); }); + describe.each(["createTable", "createEmptyTable"] as const)( + "%s storage options", + (method) => { + const schema = new Schema([new Field("id", new Float64(), true)]); + + it("should accept frozen creation options without storage options", async () => { + const options = Object.freeze({ + dataStorageVersion: "stable", + enableV2ManifestPaths: true, + }); + const table = + method === "createTable" + ? await db.createTable("frozen_options", [{ id: 1 }], options) + : await db.createEmptyTable("frozen_options", schema, options); + expect(await (table as LocalTable).usesV2ManifestPaths()).toBe(true); + expect(options).not.toHaveProperty("storageOptions"); + }); + + it("should accept frozen storage options and preserve override precedence", async () => { + const storageOptions = Object.freeze({ + newTableDataStorageVersion: "legacy", + newTableEnableV2ManifestPaths: "false", + }); + const options = Object.freeze({ + storageOptions, + dataStorageVersion: "stable", + enableV2ManifestPaths: true, + }); + const table = + method === "createTable" + ? await db.createTable("frozen_storage", [{ id: 1 }], options) + : await db.createEmptyTable("frozen_storage", schema, options); + expect(await (table as LocalTable).usesV2ManifestPaths()).toBe(true); + expect(options.storageOptions).toBe(storageOptions); + expect(storageOptions).toEqual({ + newTableDataStorageVersion: "legacy", + newTableEnableV2ManifestPaths: "false", + }); + }); + + it("should not retain a previous override when reusing storage options", async () => { + const storageOptions = { newTableEnableV2ManifestPaths: "false" }; + const create = async (name: string, enableV2ManifestPaths?: boolean) => + method === "createTable" + ? db.createTable(name, [{ id: 1 }], { + storageOptions, + enableV2ManifestPaths, + }) + : db.createEmptyTable(name, schema, { + storageOptions, + enableV2ManifestPaths, + }); + const first = await create("override", true); + expect(await (first as LocalTable).usesV2ManifestPaths()).toBe(true); + const second = await create("without_override"); + expect(await (second as LocalTable).usesV2ManifestPaths()).toBe(false); + expect(storageOptions).toEqual({ + newTableEnableV2ManifestPaths: "false", + }); + }); + }, + ); + it("should be able to migrate tables to the V2 manifest paths", async () => { const db = await connect(tmpDir.name); const table = (await db.createEmptyTable( diff --git a/nodejs/lancedb/connection.ts b/nodejs/lancedb/connection.ts index f866d1ecd..44fde96f1 100644 --- a/nodejs/lancedb/connection.ts +++ b/nodejs/lancedb/connection.ts @@ -893,23 +893,24 @@ export class LocalConnection extends Connection { private getStorageOptions( options?: Partial, ): Record | undefined { + let storageOptions = options?.storageOptions; if (options?.dataStorageVersion !== undefined) { - if (options.storageOptions === undefined) { - options.storageOptions = {}; - } - options.storageOptions["newTableDataStorageVersion"] = - options.dataStorageVersion; + storageOptions = { + ...storageOptions, + newTableDataStorageVersion: options.dataStorageVersion, + }; } if (options?.enableV2ManifestPaths !== undefined) { - if (options.storageOptions === undefined) { - options.storageOptions = {}; - } - options.storageOptions["newTableEnableV2ManifestPaths"] = - options.enableV2ManifestPaths ? "true" : "false"; + storageOptions = { + ...storageOptions, + newTableEnableV2ManifestPaths: options.enableV2ManifestPaths + ? "true" + : "false", + }; } - return cleanseStorageOptions(options?.storageOptions); + return cleanseStorageOptions(storageOptions); } async createTable( diff --git a/nodejs/lancedb/index.ts b/nodejs/lancedb/index.ts index 1057e2d53..4ad8b2a93 100644 --- a/nodejs/lancedb/index.ts +++ b/nodejs/lancedb/index.ts @@ -420,10 +420,10 @@ export async function connect( throw new Error("uri is required"); } - finalOptions = (finalOptions as ConnectionOptions) ?? {}; - (finalOptions).storageOptions = cleanseStorageOptions( - (finalOptions).storageOptions, - ); + finalOptions = { + ...finalOptions, + storageOptions: cleanseStorageOptions(finalOptions.storageOptions), + }; // Create native header provider if one was provided let nativeProvider: NativeJsHeaderProvider | undefined; @@ -630,11 +630,10 @@ export async function connectNamespace( properties = configOrProperties as Record; } - const finalOptions: ConnectNamespaceOptions = (options ?? - {}) as ConnectNamespaceOptions; - finalOptions.storageOptions = cleanseStorageOptions( - finalOptions.storageOptions, - ); + const finalOptions: ConnectNamespaceOptions = { + ...options, + storageOptions: cleanseStorageOptions(options?.storageOptions), + }; const nativeConn = await LanceDbConnection.newWithNamespace( implName,