mirror of
https://github.com/lancedb/lancedb.git
synced 2026-10-23 20:22:37 +00:00
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 <aden1350@outlook.com>
This commit is contained in:
@@ -36,6 +36,87 @@ describe("when connecting", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe.each([
|
||||
[
|
||||
"URI overload",
|
||||
(uri: string, options: { storageOptions?: Record<string, string> }) =>
|
||||
connect(uri, options),
|
||||
],
|
||||
[
|
||||
"object overload",
|
||||
async (
|
||||
uri: string,
|
||||
options: { storageOptions?: Record<string, string> },
|
||||
) => {
|
||||
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<string, string> }) =>
|
||||
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(
|
||||
|
||||
@@ -893,23 +893,24 @@ export class LocalConnection extends Connection {
|
||||
private getStorageOptions(
|
||||
options?: Partial<CreateTableOptions>,
|
||||
): Record<string, string> | 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(
|
||||
|
||||
@@ -420,10 +420,10 @@ export async function connect(
|
||||
throw new Error("uri is required");
|
||||
}
|
||||
|
||||
finalOptions = (finalOptions as ConnectionOptions) ?? {};
|
||||
(<ConnectionOptions>finalOptions).storageOptions = cleanseStorageOptions(
|
||||
(<ConnectionOptions>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<string, string>;
|
||||
}
|
||||
|
||||
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,
|
||||
|
||||
Reference in New Issue
Block a user