From fe977193235e13dbc04c57a5bdd185cf48e88344 Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Thu, 8 Oct 2026 16:10:35 +0200 Subject: [PATCH 1/6] feat(appkit): migrate files to modular @databricks/sdk-files Swap the facade's files accessor to a lazily built FilesClient (sdk-files 0.52.0, accepts sdk-core/auth/options 0.51.0). - list/download/createDirectory/delete use listDirectoryContentsIter, downloadFile, createDirectory, deleteFile; results are mapped back to the legacy snake_case / header-keyed shapes. - metadata uses a raw HEAD via request(): the generated getFileMetadata parses the empty HEAD body and drops the metadata headers. - Modular ApiError (httpStatusCode) is converted to the wrapper ApiError so exists() and the plugin's 4xx mapping keep working. - DirectoryEntry / DownloadResponse are now AppKit-owned types with the same shape. Co-authored-by: Isaac Signed-off-by: MarioCadenas --- knip.json | 1 + packages/appkit/package.json | 1 + .../appkit/src/connectors/files/client.ts | 91 +++-- .../src/connectors/files/tests/client.test.ts | 372 +++++++++++------- .../src/connectors/files/tests/utils.ts | 11 + .../src/plugins/files/tests/delete.test.ts | 11 +- .../files/tests/download-endpoint.test.ts | 15 +- .../files/tests/error-handling.test.ts | 7 +- .../files/tests/path-validation.test.ts | 15 +- .../files/tests/plugin.integration.test.ts | 111 +++--- .../src/plugins/files/tests/plugin.test.ts | 111 +++--- .../plugins/files/tests/raw-endpoint.test.ts | 17 +- .../src/plugins/files/tests/shutdown.test.ts | 7 +- .../src/plugins/files/tests/upload.test.ts | 7 +- .../appkit/src/plugins/files/tests/utils.ts | 5 +- .../plugins/files/tests/volume-config.test.ts | 7 +- packages/appkit/src/plugins/files/types.ts | 26 +- packages/shared/package.json | 1 + .../shared/src/workspace-client/client.ts | 11 +- packages/shared/src/workspace-client/index.ts | 2 +- .../shared/src/workspace-client/modular.ts | 15 + packages/shared/src/workspace-client/types.ts | 11 +- pnpm-lock.yaml | 19 + 23 files changed, 542 insertions(+), 332 deletions(-) diff --git a/knip.json b/knip.json index 14df12e42..e42dd21c6 100644 --- a/knip.json +++ b/knip.json @@ -13,6 +13,7 @@ "@databricks/sdk-auth", "@databricks/sdk-core", "@databricks/sdk-experimental", + "@databricks/sdk-files", "@databricks/sdk-genie", "@databricks/sdk-jobs", "@databricks/sdk-options", diff --git a/packages/appkit/package.json b/packages/appkit/package.json index 33899de06..5a34f1797 100644 --- a/packages/appkit/package.json +++ b/packages/appkit/package.json @@ -74,6 +74,7 @@ "@databricks/sdk-auth": "0.51.0", "@databricks/sdk-core": "0.51.0", "@databricks/sdk-experimental": "0.17.0", + "@databricks/sdk-files": "0.52.0", "@databricks/sdk-genie": "0.54.0", "@databricks/sdk-jobs": "0.57.0", "@databricks/sdk-options": "0.51.0", diff --git a/packages/appkit/src/connectors/files/client.ts b/packages/appkit/src/connectors/files/client.ts index ffdc06dac..e5a98921d 100644 --- a/packages/appkit/src/connectors/files/client.ts +++ b/packages/appkit/src/connectors/files/client.ts @@ -58,6 +58,47 @@ export function runWithFilesSpanAttributes( return filesSpanAttributesStorage.run(attributes, fn); } +/** + * The modular SDK throws `@databricks/sdk-core`'s `ApiError` (status on + * `httpStatusCode`, code on `.code`), a different class from the wrapper's + * `ApiError`. Convert it so `exists()` and the plugin's `instanceof ApiError` + * / `.statusCode` mapping keep returning upstream 4xx statuses. + */ +function toWrapperApiError(error: unknown): unknown { + if ( + error instanceof Error && + !(error instanceof ApiError) && + "httpStatusCode" in error && + typeof error.httpStatusCode === "number" && + error.httpStatusCode > 0 + ) { + const code = "code" in error ? String(error.code) : "UNKNOWN"; + return new ApiError( + error.message, + code, + error.httpStatusCode, + undefined, + [], + ); + } + return error; +} + +/** Stream a file, mapped back to the legacy `DownloadResponse` shape. */ +async function downloadFile( + client: WorkspaceClient, + resolvedPath: string, +): Promise { + const res = await client.files.downloadFile({ filePath: resolvedPath }); + return { + "content-length": + res.contentLength === undefined ? undefined : Number(res.contentLength), + "content-type": res.contentType, + contents: res.contents, + "last-modified": res.lastModified, + }; +} + interface FilesConnectorConfig { defaultVolume?: string; timeout?: number; @@ -157,7 +198,8 @@ export class FilesConnector { success = true; span.setStatus({ code: SpanStatusCode.OK }); return result; - } catch (error) { + } catch (thrown) { + const error = toWrapperApiError(thrown); span.recordException(error as Error); span.setStatus({ code: SpanStatusCode.ERROR, @@ -192,10 +234,16 @@ export class FilesConnector { return this.traced("list", { "files.path": resolvedPath }, async () => { const entries: DirectoryEntry[] = []; - for await (const entry of client.files.listDirectoryContents({ - directory_path: resolvedPath, + for await (const entry of client.files.listDirectoryContentsIter({ + directoryPath: resolvedPath, })) { - entries.push(entry); + entries.push({ + file_size: entry.fileSize, + is_directory: entry.isDirectory, + last_modified: entry.lastModified, + name: entry.name, + path: entry.path, + }); } return entries; }); @@ -239,11 +287,9 @@ export class FilesConnector { filePath: string, ): Promise { const resolvedPath = this.resolvePath(filePath); - return this.traced("download", { "files.path": resolvedPath }, async () => { - return client.files.download({ - file_path: resolvedPath, - }); - }); + return this.traced("download", { "files.path": resolvedPath }, async () => + downloadFile(client, resolvedPath), + ); } async exists(client: WorkspaceClient, filePath: string): Promise { @@ -267,17 +313,22 @@ export class FilesConnector { ): Promise { const resolvedPath = this.resolvePath(filePath); return this.traced("metadata", { "files.path": resolvedPath }, async () => { - const response = await client.files.getMetadata({ - file_path: resolvedPath, + // The modular `getFileMetadata` parses the empty HEAD body and drops the + // response headers that carry the metadata, so read them off a raw HEAD. + const res = await client.request({ + method: "HEAD", + path: `/api/2.0/fs/files${resolvedPath}`, }); + const contentLength = res.headers.get("content-length"); return { - contentLength: response["content-length"], + contentLength: + contentLength === null ? undefined : Number(contentLength), contentType: contentTypeFromPath( filePath, - response["content-type"], + res.headers.get("content-type") ?? undefined, this.customContentTypes, ), - lastModified: response["last-modified"], + lastModified: res.headers.get("last-modified") ?? undefined, }; }); } @@ -353,9 +404,7 @@ export class FilesConnector { "createDirectory", { "files.path": resolvedPath }, async () => { - await client.files.createDirectory({ - directory_path: resolvedPath, - }); + await client.files.createDirectory({ directoryPath: resolvedPath }); }, ); } @@ -363,9 +412,7 @@ export class FilesConnector { async delete(client: WorkspaceClient, filePath: string): Promise { const resolvedPath = this.resolvePath(filePath); return this.traced("delete", { "files.path": resolvedPath }, async () => { - await client.files.delete({ - file_path: resolvedPath, - }); + await client.files.deleteFile({ filePath: resolvedPath }); }); } @@ -384,9 +431,7 @@ export class FilesConnector { return { ...meta, textPreview: null, isText: false, isImage }; } - const response = await client.files.download({ - file_path: resolvedPath, - }); + const response = await downloadFile(client, resolvedPath); if (!response.contents) { return { ...meta, textPreview: "", isText: true, isImage: false }; } diff --git a/packages/appkit/src/connectors/files/tests/client.test.ts b/packages/appkit/src/connectors/files/tests/client.test.ts index 09be16e7c..a47faca5a 100644 --- a/packages/appkit/src/connectors/files/tests/client.test.ts +++ b/packages/appkit/src/connectors/files/tests/client.test.ts @@ -5,16 +5,15 @@ import { createApiError } from "../../../testing"; import type { WorkspaceClient } from "../../../workspace-client"; import { ApiError } from "../../../workspace-client"; import { FilesConnector } from "../client"; -import { streamFromChunks, streamFromString } from "./utils"; +import { headResponse, streamFromChunks, streamFromString } from "./utils"; const { mockFilesApi, mockConfig, mockClient } = vi.hoisted(() => { const mockFilesApi = { - listDirectoryContents: vi.fn(), - download: vi.fn(), - getMetadata: vi.fn(), + listDirectoryContentsIter: vi.fn(), + downloadFile: vi.fn(), upload: vi.fn(), createDirectory: vi.fn(), - delete: vi.fn(), + deleteFile: vi.fn(), }; const mockConfig = { @@ -64,11 +63,11 @@ describe("FilesConnector", () => { defaultVolume: "/Volumes/catalog/schema/vol", }); - mockFilesApi.download.mockResolvedValue({ contents: null }); + mockFilesApi.downloadFile.mockResolvedValue({ contents: null }); connector.download(mockClient, "/Volumes/other/path/file.txt"); - expect(mockFilesApi.download).toHaveBeenCalledWith({ - file_path: "/Volumes/other/path/file.txt", + expect(mockFilesApi.downloadFile).toHaveBeenCalledWith({ + filePath: "/Volumes/other/path/file.txt", }); }); @@ -77,11 +76,11 @@ describe("FilesConnector", () => { defaultVolume: "/Volumes/catalog/schema/vol", }); - mockFilesApi.download.mockResolvedValue({ contents: null }); + mockFilesApi.downloadFile.mockResolvedValue({ contents: null }); connector.download(mockClient, "subdir/file.txt"); - expect(mockFilesApi.download).toHaveBeenCalledWith({ - file_path: "/Volumes/catalog/schema/vol/subdir/file.txt", + expect(mockFilesApi.downloadFile).toHaveBeenCalledWith({ + filePath: "/Volumes/catalog/schema/vol/subdir/file.txt", }); }); @@ -163,45 +162,54 @@ describe("FilesConnector", () => { }); }); - test("collects async iterator entries", async () => { - const entries = [ + test("collects async iterator entries, mapped to the snake_case wire shape", async () => { + mockFilesApi.listDirectoryContentsIter.mockReturnValue( + (async function* () { + yield { + name: "file1.txt", + path: "/Volumes/catalog/schema/vol/file1.txt", + isDirectory: false, + fileSize: 42, + lastModified: 1700000000000, + }; + yield { + name: "subdir", + path: "/Volumes/catalog/schema/vol/subdir", + isDirectory: true, + }; + })(), + ); + + const result = await connector.list(mockClient); + + expect(result).toEqual([ { name: "file1.txt", path: "/Volumes/catalog/schema/vol/file1.txt", is_directory: false, + file_size: 42, + last_modified: 1700000000000, }, { name: "subdir", path: "/Volumes/catalog/schema/vol/subdir", is_directory: true, }, - ]; - - mockFilesApi.listDirectoryContents.mockReturnValue( - (async function* () { - for (const entry of entries) { - yield entry; - } - })(), - ); - - const result = await connector.list(mockClient); - - expect(result).toEqual(entries); - expect(mockFilesApi.listDirectoryContents).toHaveBeenCalledWith({ - directory_path: "/Volumes/catalog/schema/vol", + ]); + expect(mockFilesApi.listDirectoryContentsIter).toHaveBeenCalledWith({ + directoryPath: "/Volumes/catalog/schema/vol", }); }); test("uses defaultVolume when no path provided", async () => { - mockFilesApi.listDirectoryContents.mockReturnValue( + mockFilesApi.listDirectoryContentsIter.mockReturnValue( (async function* () {})(), ); await connector.list(mockClient); - expect(mockFilesApi.listDirectoryContents).toHaveBeenCalledWith({ - directory_path: "/Volumes/catalog/schema/vol", + expect(mockFilesApi.listDirectoryContentsIter).toHaveBeenCalledWith({ + directoryPath: "/Volumes/catalog/schema/vol", }); }); @@ -214,31 +222,31 @@ describe("FilesConnector", () => { }); test("uses provided absolute path", async () => { - mockFilesApi.listDirectoryContents.mockReturnValue( + mockFilesApi.listDirectoryContentsIter.mockReturnValue( (async function* () {})(), ); await connector.list(mockClient, "/Volumes/other/path"); - expect(mockFilesApi.listDirectoryContents).toHaveBeenCalledWith({ - directory_path: "/Volumes/other/path", + expect(mockFilesApi.listDirectoryContentsIter).toHaveBeenCalledWith({ + directoryPath: "/Volumes/other/path", }); }); test("resolves relative path with defaultVolume", async () => { - mockFilesApi.listDirectoryContents.mockReturnValue( + mockFilesApi.listDirectoryContentsIter.mockReturnValue( (async function* () {})(), ); await connector.list(mockClient, "subdir"); - expect(mockFilesApi.listDirectoryContents).toHaveBeenCalledWith({ - directory_path: "/Volumes/catalog/schema/vol/subdir", + expect(mockFilesApi.listDirectoryContentsIter).toHaveBeenCalledWith({ + directoryPath: "/Volumes/catalog/schema/vol/subdir", }); }); test("returns empty array for empty directory", async () => { - mockFilesApi.listDirectoryContents.mockReturnValue( + mockFilesApi.listDirectoryContentsIter.mockReturnValue( (async function* () {})(), ); @@ -260,7 +268,7 @@ describe("FilesConnector", () => { test("decodes ReadableStream to UTF-8 string", async () => { const content = "Hello, world!"; - mockFilesApi.download.mockResolvedValue({ + mockFilesApi.downloadFile.mockResolvedValue({ contents: streamFromString(content), }); @@ -270,7 +278,7 @@ describe("FilesConnector", () => { }); test("returns empty string when contents is null", async () => { - mockFilesApi.download.mockResolvedValue({ contents: null }); + mockFilesApi.downloadFile.mockResolvedValue({ contents: null }); const result = await connector.read(mockClient, "empty.txt"); @@ -278,7 +286,7 @@ describe("FilesConnector", () => { }); test("concatenates multiple chunks correctly", async () => { - mockFilesApi.download.mockResolvedValue({ + mockFilesApi.downloadFile.mockResolvedValue({ contents: streamFromChunks(["Hello, ", "world", "!"]), }); @@ -289,7 +297,7 @@ describe("FilesConnector", () => { test("handles multi-byte UTF-8 characters", async () => { const content = "Héllo wârld 🌍"; - mockFilesApi.download.mockResolvedValue({ + mockFilesApi.downloadFile.mockResolvedValue({ contents: streamFromString(content), }); @@ -309,26 +317,55 @@ describe("FilesConnector", () => { }); }); - test("calls client.files.download with resolved path", async () => { - const response = { contents: streamFromString("data") }; - mockFilesApi.download.mockResolvedValue(response); + test("calls client.files.downloadFile with resolved path", async () => { + const contents = streamFromString("data"); + mockFilesApi.downloadFile.mockResolvedValue({ + contentLength: 4n, + contentType: "text/plain", + lastModified: "Mon, 01 Jan 2025 00:00:00 GMT", + contents, + }); const result = await connector.download(mockClient, "file.txt"); - expect(mockFilesApi.download).toHaveBeenCalledWith({ - file_path: "/Volumes/catalog/schema/vol/file.txt", + expect(mockFilesApi.downloadFile).toHaveBeenCalledWith({ + filePath: "/Volumes/catalog/schema/vol/file.txt", + }); + expect(result).toEqual({ + "content-length": 4, + "content-type": "text/plain", + "last-modified": "Mon, 01 Jan 2025 00:00:00 GMT", + contents, }); - expect(result).toBe(response); + expect(result.contents).toBe(contents); + }); + + test("converts a modular-SDK 404 error into the wrapper ApiError", async () => { + mockFilesApi.downloadFile.mockRejectedValue( + Object.assign(new Error("File not found"), { + httpStatusCode: 404, + code: "NOT_FOUND", + }), + ); + + const error = await connector + .download(mockClient, "missing.txt") + .catch((e) => e); + + expect(error).toBeInstanceOf(ApiError); + expect(error.statusCode).toBe(404); + expect(error.errorCode).toBe("NOT_FOUND"); + expect(error.message).toBe("File not found"); }); test("passes absolute path directly", async () => { const response = { contents: null }; - mockFilesApi.download.mockResolvedValue(response); + mockFilesApi.downloadFile.mockResolvedValue(response); await connector.download(mockClient, "/Volumes/other/file.txt"); - expect(mockFilesApi.download).toHaveBeenCalledWith({ - file_path: "/Volumes/other/file.txt", + expect(mockFilesApi.downloadFile).toHaveBeenCalledWith({ + filePath: "/Volumes/other/file.txt", }); }); }); @@ -344,11 +381,13 @@ describe("FilesConnector", () => { }); test("returns true when metadata succeeds", async () => { - mockFilesApi.getMetadata.mockResolvedValue({ - "content-length": 100, - "content-type": "text/plain", - "last-modified": "2025-01-01", - }); + mockRequest.mockResolvedValue( + headResponse({ + "content-length": 100, + "content-type": "text/plain", + "last-modified": "2025-01-01", + }), + ); const result = await connector.exists(mockClient, "file.txt"); @@ -356,7 +395,7 @@ describe("FilesConnector", () => { }); test("returns false on 404 ApiError", async () => { - mockFilesApi.getMetadata.mockRejectedValue( + mockRequest.mockRejectedValue( createApiError({ message: "Not found", errorCode: "NOT_FOUND", @@ -370,7 +409,7 @@ describe("FilesConnector", () => { }); test("rethrows non-404 ApiError", async () => { - mockFilesApi.getMetadata.mockRejectedValue( + mockRequest.mockRejectedValue( createApiError({ message: "Server error", errorCode: "SERVER_ERROR", @@ -384,7 +423,7 @@ describe("FilesConnector", () => { }); test("rethrows generic errors", async () => { - mockFilesApi.getMetadata.mockRejectedValue(new Error("Network failure")); + mockRequest.mockRejectedValue(new Error("Network failure")); await expect(connector.exists(mockClient, "file.txt")).rejects.toThrow( "Network failure", @@ -403,11 +442,13 @@ describe("FilesConnector", () => { }); test("maps SDK response to FileMetadata", async () => { - mockFilesApi.getMetadata.mockResolvedValue({ - "content-length": 1234, - "content-type": "application/json", - "last-modified": "2025-06-15T10:00:00Z", - }); + mockRequest.mockResolvedValue( + headResponse({ + "content-length": 1234, + "content-type": "application/json", + "last-modified": "2025-06-15T10:00:00Z", + }), + ); const result = await connector.metadata(mockClient, "data.json"); @@ -419,11 +460,13 @@ describe("FilesConnector", () => { }); test("uses contentTypeFromPath to resolve octet-stream", async () => { - mockFilesApi.getMetadata.mockResolvedValue({ - "content-length": 500, - "content-type": "application/octet-stream", - "last-modified": "2025-01-01", - }); + mockRequest.mockResolvedValue( + headResponse({ + "content-length": 500, + "content-type": "application/octet-stream", + "last-modified": "2025-01-01", + }), + ); const result = await connector.metadata(mockClient, "image.png"); @@ -431,11 +474,13 @@ describe("FilesConnector", () => { }); test("handles undefined content-type from SDK", async () => { - mockFilesApi.getMetadata.mockResolvedValue({ - "content-length": 100, - "content-type": undefined, - "last-modified": "2025-01-01", - }); + mockRequest.mockResolvedValue( + headResponse({ + "content-length": 100, + "content-type": undefined, + "last-modified": "2025-01-01", + }), + ); const result = await connector.metadata(mockClient, "data.csv"); @@ -443,16 +488,41 @@ describe("FilesConnector", () => { }); test("resolves relative path via defaultVolume", async () => { - mockFilesApi.getMetadata.mockResolvedValue({ - "content-length": 0, - "content-type": "text/plain", - "last-modified": "2025-01-01", - }); + mockRequest.mockResolvedValue( + headResponse({ + "content-length": 0, + "content-type": "text/plain", + "last-modified": "2025-01-01", + }), + ); await connector.metadata(mockClient, "notes.txt"); - expect(mockFilesApi.getMetadata).toHaveBeenCalledWith({ - file_path: "/Volumes/catalog/schema/vol/notes.txt", + expect(mockRequest).toHaveBeenCalledWith({ + method: "HEAD", + path: "/api/2.0/fs/files/Volumes/catalog/schema/vol/notes.txt", + }); + }); + + // Regression: the modular SDK's getFileMetadata drops HEAD response + // headers, so metadata must come from the raw HEAD response headers. + test("reads metadata from the HEAD response headers", async () => { + mockRequest.mockResolvedValue( + new Response(null, { + headers: { + "content-length": "2048", + "content-type": "text/plain", + "last-modified": "Sun, 15 Jun 2025 10:00:00 GMT", + }, + }), + ); + + const result = await connector.metadata(mockClient, "notes.txt"); + + expect(result).toEqual({ + contentLength: 2048, + contentType: "text/plain", + lastModified: "Sun, 15 Jun 2025 10:00:00 GMT", }); }); }); @@ -598,7 +668,7 @@ describe("FilesConnector", () => { await connector.createDirectory(mockClient, "new-dir"); expect(mockFilesApi.createDirectory).toHaveBeenCalledWith({ - directory_path: "/Volumes/catalog/schema/vol/new-dir", + directoryPath: "/Volumes/catalog/schema/vol/new-dir", }); }); @@ -611,7 +681,7 @@ describe("FilesConnector", () => { ); expect(mockFilesApi.createDirectory).toHaveBeenCalledWith({ - directory_path: "/Volumes/other/path/new-dir", + directoryPath: "/Volumes/other/path/new-dir", }); }); }); @@ -626,23 +696,23 @@ describe("FilesConnector", () => { }); }); - test("calls client.files.delete with resolved path", async () => { - mockFilesApi.delete.mockResolvedValue(undefined); + test("calls client.files.deleteFile with resolved path", async () => { + mockFilesApi.deleteFile.mockResolvedValue(undefined); await connector.delete(mockClient, "file.txt"); - expect(mockFilesApi.delete).toHaveBeenCalledWith({ - file_path: "/Volumes/catalog/schema/vol/file.txt", + expect(mockFilesApi.deleteFile).toHaveBeenCalledWith({ + filePath: "/Volumes/catalog/schema/vol/file.txt", }); }); test("uses absolute path when provided", async () => { - mockFilesApi.delete.mockResolvedValue(undefined); + mockFilesApi.deleteFile.mockResolvedValue(undefined); await connector.delete(mockClient, "/Volumes/other/file.txt"); - expect(mockFilesApi.delete).toHaveBeenCalledWith({ - file_path: "/Volumes/other/file.txt", + expect(mockFilesApi.deleteFile).toHaveBeenCalledWith({ + filePath: "/Volumes/other/file.txt", }); }); }); @@ -660,12 +730,14 @@ describe("FilesConnector", () => { test("text files return truncated preview (max 1024 chars)", async () => { const longText = "A".repeat(2000); - mockFilesApi.getMetadata.mockResolvedValue({ - "content-length": 2000, - "content-type": "text/plain", - "last-modified": "2025-01-01", - }); - mockFilesApi.download.mockResolvedValue({ + mockRequest.mockResolvedValue( + headResponse({ + "content-length": 2000, + "content-type": "text/plain", + "last-modified": "2025-01-01", + }), + ); + mockFilesApi.downloadFile.mockResolvedValue({ contents: streamFromString(longText), }); @@ -678,12 +750,14 @@ describe("FilesConnector", () => { }); test("text/html files are treated as text", async () => { - mockFilesApi.getMetadata.mockResolvedValue({ - "content-length": 30, - "content-type": "text/html", - "last-modified": "2025-01-01", - }); - mockFilesApi.download.mockResolvedValue({ + mockRequest.mockResolvedValue( + headResponse({ + "content-length": 30, + "content-type": "text/html", + "last-modified": "2025-01-01", + }), + ); + mockFilesApi.downloadFile.mockResolvedValue({ contents: streamFromString("

Hello

"), }); @@ -694,12 +768,14 @@ describe("FilesConnector", () => { }); test("application/json files are treated as text", async () => { - mockFilesApi.getMetadata.mockResolvedValue({ - "content-length": 20, - "content-type": "application/json", - "last-modified": "2025-01-01", - }); - mockFilesApi.download.mockResolvedValue({ + mockRequest.mockResolvedValue( + headResponse({ + "content-length": 20, + "content-type": "application/json", + "last-modified": "2025-01-01", + }), + ); + mockFilesApi.downloadFile.mockResolvedValue({ contents: streamFromString('{"key":"value"}'), }); @@ -710,12 +786,14 @@ describe("FilesConnector", () => { }); test("application/xml files are treated as text", async () => { - mockFilesApi.getMetadata.mockResolvedValue({ - "content-length": 30, - "content-type": "application/xml", - "last-modified": "2025-01-01", - }); - mockFilesApi.download.mockResolvedValue({ + mockRequest.mockResolvedValue( + headResponse({ + "content-length": 30, + "content-type": "application/xml", + "last-modified": "2025-01-01", + }), + ); + mockFilesApi.downloadFile.mockResolvedValue({ contents: streamFromString(""), }); @@ -726,11 +804,13 @@ describe("FilesConnector", () => { }); test("image files return isImage: true, textPreview: null", async () => { - mockFilesApi.getMetadata.mockResolvedValue({ - "content-length": 5000, - "content-type": "image/png", - "last-modified": "2025-01-01", - }); + mockRequest.mockResolvedValue( + headResponse({ + "content-length": 5000, + "content-type": "image/png", + "last-modified": "2025-01-01", + }), + ); const result = await connector.preview(mockClient, "image.png"); @@ -740,11 +820,13 @@ describe("FilesConnector", () => { }); test("other files return isText: false, isImage: false, textPreview: null", async () => { - mockFilesApi.getMetadata.mockResolvedValue({ - "content-length": 1000, - "content-type": "application/pdf", - "last-modified": "2025-01-01", - }); + mockRequest.mockResolvedValue( + headResponse({ + "content-length": 1000, + "content-type": "application/pdf", + "last-modified": "2025-01-01", + }), + ); const result = await connector.preview(mockClient, "doc.pdf"); @@ -754,12 +836,14 @@ describe("FilesConnector", () => { }); test("empty file contents return empty string preview", async () => { - mockFilesApi.getMetadata.mockResolvedValue({ - "content-length": 0, - "content-type": "text/plain", - "last-modified": "2025-01-01", - }); - mockFilesApi.download.mockResolvedValue({ + mockRequest.mockResolvedValue( + headResponse({ + "content-length": 0, + "content-type": "text/plain", + "last-modified": "2025-01-01", + }), + ); + mockFilesApi.downloadFile.mockResolvedValue({ contents: null, }); @@ -771,12 +855,14 @@ describe("FilesConnector", () => { }); test("preview spreads metadata into result", async () => { - mockFilesApi.getMetadata.mockResolvedValue({ - "content-length": 42, - "content-type": "text/plain", - "last-modified": "2025-06-15T10:00:00Z", - }); - mockFilesApi.download.mockResolvedValue({ + mockRequest.mockResolvedValue( + headResponse({ + "content-length": 42, + "content-type": "text/plain", + "last-modified": "2025-06-15T10:00:00Z", + }), + ); + mockFilesApi.downloadFile.mockResolvedValue({ contents: streamFromString("hello"), }); @@ -790,12 +876,14 @@ describe("FilesConnector", () => { test("short text file returns full content", async () => { const content = "Short file."; - mockFilesApi.getMetadata.mockResolvedValue({ - "content-length": content.length, - "content-type": "text/plain", - "last-modified": "2025-01-01", - }); - mockFilesApi.download.mockResolvedValue({ + mockRequest.mockResolvedValue( + headResponse({ + "content-length": content.length, + "content-type": "text/plain", + "last-modified": "2025-01-01", + }), + ); + mockFilesApi.downloadFile.mockResolvedValue({ contents: streamFromString(content), }); diff --git a/packages/appkit/src/connectors/files/tests/utils.ts b/packages/appkit/src/connectors/files/tests/utils.ts index d204d8dad..47a2c1158 100644 --- a/packages/appkit/src/connectors/files/tests/utils.ts +++ b/packages/appkit/src/connectors/files/tests/utils.ts @@ -20,3 +20,14 @@ export function streamFromChunks(chunks: string[]): ReadableStream { }, }); } + +// Builds the HEAD response the connector's metadata() reads headers from. +export function headResponse( + headers: Record, +): Response { + const h = new Headers(); + for (const [key, value] of Object.entries(headers)) { + if (value !== undefined) h.set(key, String(value)); + } + return new Response(null, { headers: h }); +} diff --git a/packages/appkit/src/plugins/files/tests/delete.test.ts b/packages/appkit/src/plugins/files/tests/delete.test.ts index 9ef00f942..a87c45987 100644 --- a/packages/appkit/src/plugins/files/tests/delete.test.ts +++ b/packages/appkit/src/plugins/files/tests/delete.test.ts @@ -29,12 +29,11 @@ describe("FilesPlugin delete", () => { client = createMockWorkspaceClient({ strict: true, responses: { - "files.listDirectoryContents": undefined, - "files.download": undefined, - "files.getMetadata": undefined, + "files.listDirectoryContentsIter": undefined, + "files.downloadFile": undefined, "files.upload": undefined, "files.createDirectory": undefined, - "files.delete": undefined, + "files.deleteFile": undefined, }, }); serviceContextMock = await setupTestEnv(client); @@ -52,7 +51,7 @@ describe("FilesPlugin delete", () => { const generateKey = vi.spyOn(testCache.current, "generateKey"); const del = vi.spyOn(testCache.current, "delete"); - getMock(client, "files.delete").mockResolvedValue(undefined); + getMock(client, "files.deleteFile").mockResolvedValue(undefined); await handler( mockReq("uploads", { @@ -86,7 +85,7 @@ describe("FilesPlugin delete", () => { const handler = getRouteHandler(plugin, "delete", ""); const res = mockRes(); - getMock(client, "files.delete").mockRejectedValue( + getMock(client, "files.deleteFile").mockRejectedValue( createApiError({ statusCode: 404, message: "Not found", diff --git a/packages/appkit/src/plugins/files/tests/download-endpoint.test.ts b/packages/appkit/src/plugins/files/tests/download-endpoint.test.ts index 622aa702d..f166bface 100644 --- a/packages/appkit/src/plugins/files/tests/download-endpoint.test.ts +++ b/packages/appkit/src/plugins/files/tests/download-endpoint.test.ts @@ -28,12 +28,11 @@ describe("FilesPlugin download endpoint Content-Disposition", () => { client = createMockWorkspaceClient({ strict: true, responses: { - "files.listDirectoryContents": undefined, - "files.download": undefined, - "files.getMetadata": undefined, + "files.listDirectoryContentsIter": undefined, + "files.downloadFile": undefined, "files.upload": undefined, "files.createDirectory": undefined, - "files.delete": undefined, + "files.deleteFile": undefined, }, }); serviceContextMock = await setupTestEnv(client); @@ -48,7 +47,7 @@ describe("FilesPlugin download endpoint Content-Disposition", () => { const handler = getRouteHandler(plugin, "get", "/download"); const res = mockRes(); - getMock(client, "files.download").mockResolvedValue( + getMock(client, "files.downloadFile").mockResolvedValue( makeStreamResponse("file data"), ); @@ -70,7 +69,7 @@ describe("FilesPlugin download endpoint Content-Disposition", () => { const handler = getRouteHandler(plugin, "get", "/download"); const res = mockRes(); - getMock(client, "files.download").mockResolvedValue( + getMock(client, "files.downloadFile").mockResolvedValue( makeStreamResponse("data"), ); @@ -92,7 +91,7 @@ describe("FilesPlugin download endpoint Content-Disposition", () => { const handler = getRouteHandler(plugin, "get", "/download"); const res = mockRes(); - getMock(client, "files.download").mockResolvedValue( + getMock(client, "files.downloadFile").mockResolvedValue( makeStreamResponse("{}"), ); @@ -132,7 +131,7 @@ describe("FilesPlugin download endpoint Content-Disposition", () => { const res = mockRes(); // Response with no contents field (empty file) - getMock(client, "files.download").mockResolvedValue({}); + getMock(client, "files.downloadFile").mockResolvedValue({}); await handler( mockReq("uploads", { diff --git a/packages/appkit/src/plugins/files/tests/error-handling.test.ts b/packages/appkit/src/plugins/files/tests/error-handling.test.ts index 296b6f2b8..1c5652327 100644 --- a/packages/appkit/src/plugins/files/tests/error-handling.test.ts +++ b/packages/appkit/src/plugins/files/tests/error-handling.test.ts @@ -28,12 +28,11 @@ describe("FilesPlugin error handling", () => { client = createMockWorkspaceClient({ strict: true, responses: { - "files.listDirectoryContents": undefined, - "files.download": undefined, - "files.getMetadata": undefined, + "files.listDirectoryContentsIter": undefined, + "files.downloadFile": undefined, "files.upload": undefined, "files.createDirectory": undefined, - "files.delete": undefined, + "files.deleteFile": undefined, }, }); serviceContextMock = await setupTestEnv(client); diff --git a/packages/appkit/src/plugins/files/tests/path-validation.test.ts b/packages/appkit/src/plugins/files/tests/path-validation.test.ts index 42e21b883..c5dd40a79 100644 --- a/packages/appkit/src/plugins/files/tests/path-validation.test.ts +++ b/packages/appkit/src/plugins/files/tests/path-validation.test.ts @@ -28,12 +28,11 @@ describe("FilesPlugin path validation", () => { client = createMockWorkspaceClient({ strict: true, responses: { - "files.listDirectoryContents": undefined, - "files.download": undefined, - "files.getMetadata": undefined, + "files.listDirectoryContentsIter": undefined, + "files.downloadFile": undefined, "files.upload": undefined, "files.createDirectory": undefined, - "files.delete": undefined, + "files.deleteFile": undefined, }, }); serviceContextMock = await setupTestEnv(client); @@ -46,13 +45,13 @@ describe("FilesPlugin path validation", () => { // Defends against regressions where the handler calls the SDK and *also* // returns 400 β€” the status assertion alone wouldn't catch that. function expectNoSdkCall() { - expect(getMock(client, "files.download")).not.toHaveBeenCalled(); + expect(getMock(client, "files.downloadFile")).not.toHaveBeenCalled(); expect(getMock(client, "files.upload")).not.toHaveBeenCalled(); - expect(getMock(client, "files.delete")).not.toHaveBeenCalled(); + expect(getMock(client, "files.deleteFile")).not.toHaveBeenCalled(); expect(getMock(client, "files.createDirectory")).not.toHaveBeenCalled(); - expect(getMock(client, "files.getMetadata")).not.toHaveBeenCalled(); + expect(getMock(client, "request")).not.toHaveBeenCalled(); expect( - getMock(client, "files.listDirectoryContents"), + getMock(client, "files.listDirectoryContentsIter"), ).not.toHaveBeenCalled(); } diff --git a/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts b/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts index 2fd955297..b60fdbfc7 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts @@ -20,20 +20,20 @@ import { createApp } from "../../../core"; import { createApiError } from "../../../testing"; import { server as serverPlugin } from "../../server"; import { files } from "../index"; -import { streamFromString } from "./utils"; +import { headResponse, streamFromString } from "./utils"; const { mockFilesApi, mockSdkClient } = vi.hoisted(() => { const mockFilesApi = { - listDirectoryContents: vi.fn(), - download: vi.fn(), - getMetadata: vi.fn(), + listDirectoryContentsIter: vi.fn(), + downloadFile: vi.fn(), upload: vi.fn(), createDirectory: vi.fn(), - delete: vi.fn(), + deleteFile: vi.fn(), }; const mockSdkClient = { files: mockFilesApi, + request: vi.fn(), config: { host: "https://test.databricks.com", authenticate: vi.fn(), @@ -101,12 +101,12 @@ describe("Files Plugin Integration", () => { }); beforeEach(() => { - mockFilesApi.listDirectoryContents.mockReset(); - mockFilesApi.download.mockReset(); - mockFilesApi.getMetadata.mockReset(); + mockFilesApi.listDirectoryContentsIter.mockReset(); + mockFilesApi.downloadFile.mockReset(); + mockSdkClient.request.mockReset().mockResolvedValue(new Response(null)); mockFilesApi.upload.mockReset(); mockFilesApi.createDirectory.mockReset(); - mockFilesApi.delete.mockReset(); + mockFilesApi.deleteFile.mockReset(); }); describe("Volumes Endpoint", () => { @@ -148,10 +148,11 @@ describe("Files Plugin Integration", () => { }, ]; - mockFilesApi.listDirectoryContents.mockReturnValue( + mockFilesApi.listDirectoryContentsIter.mockReturnValue( (async function* () { - for (const entry of MOCKED_ENTRIES) { - yield entry; + // Modular SDK yields camelCase; the HTTP body stays snake_case. + for (const { is_directory, ...rest } of MOCKED_ENTRIES) { + yield { ...rest, isDirectory: is_directory }; } })(), ); @@ -166,7 +167,7 @@ describe("Files Plugin Integration", () => { }); test(`GET /api/files/${VOL}/list?path=/abs/path uses provided path`, async () => { - mockFilesApi.listDirectoryContents.mockReturnValue( + mockFilesApi.listDirectoryContentsIter.mockReturnValue( (async function* () {})(), ); @@ -176,15 +177,15 @@ describe("Files Plugin Integration", () => { ); expect(response.status).toBe(200); - expect(mockFilesApi.listDirectoryContents).toHaveBeenCalledWith({ - directory_path: "/Volumes/other/path", + expect(mockFilesApi.listDirectoryContentsIter).toHaveBeenCalledWith({ + directoryPath: "/Volumes/other/path", }); }); }); describe("Read File", () => { test(`GET /api/files/${VOL}/read?path=/file.txt returns text content`, async () => { - mockFilesApi.download.mockResolvedValue({ + mockFilesApi.downloadFile.mockResolvedValue({ contents: streamFromString("file content here"), }); @@ -211,11 +212,13 @@ describe("Files Plugin Integration", () => { describe("Exists", () => { test(`GET /api/files/${VOL}/exists returns { exists: true }`, async () => { - mockFilesApi.getMetadata.mockResolvedValue({ - "content-length": 100, - "content-type": "text/plain", - "last-modified": "2025-01-01", - }); + mockSdkClient.request.mockResolvedValue( + headResponse({ + "content-length": 100, + "content-type": "text/plain", + "last-modified": "2025-01-01", + }), + ); const response = await fetch( `${baseUrl}/api/files/${VOL}/exists?path=/Volumes/catalog/schema/vol/file.txt`, @@ -228,7 +231,7 @@ describe("Files Plugin Integration", () => { }); test(`GET /api/files/${VOL}/exists returns { exists: false } on 404`, async () => { - mockFilesApi.getMetadata.mockRejectedValue( + mockSdkClient.request.mockRejectedValue( createApiError({ statusCode: 404, message: "Not found", @@ -249,11 +252,13 @@ describe("Files Plugin Integration", () => { describe("Metadata", () => { test(`GET /api/files/${VOL}/metadata returns correct metadata`, async () => { - mockFilesApi.getMetadata.mockResolvedValue({ - "content-length": 256, - "content-type": "application/json", - "last-modified": "2025-06-15T10:00:00Z", - }); + mockSdkClient.request.mockResolvedValue( + headResponse({ + "content-length": 256, + "content-type": "application/json", + "last-modified": "2025-06-15T10:00:00Z", + }), + ); const response = await fetch( `${baseUrl}/api/files/${VOL}/metadata?path=/Volumes/catalog/schema/vol/file.json`, @@ -272,12 +277,14 @@ describe("Files Plugin Integration", () => { describe("Preview", () => { test(`GET /api/files/${VOL}/preview returns text preview`, async () => { - mockFilesApi.getMetadata.mockResolvedValue({ - "content-length": 20, - "content-type": "text/plain", - "last-modified": "2025-01-01", - }); - mockFilesApi.download.mockResolvedValue({ + mockSdkClient.request.mockResolvedValue( + headResponse({ + "content-length": 20, + "content-type": "text/plain", + "last-modified": "2025-01-01", + }), + ); + mockFilesApi.downloadFile.mockResolvedValue({ contents: streamFromString("Hello preview!"), }); @@ -298,11 +305,13 @@ describe("Files Plugin Integration", () => { }); test(`GET /api/files/${VOL}/preview returns image metadata`, async () => { - mockFilesApi.getMetadata.mockResolvedValue({ - "content-length": 5000, - "content-type": "image/png", - "last-modified": "2025-01-01", - }); + mockSdkClient.request.mockResolvedValue( + headResponse({ + "content-length": 5000, + "content-type": "image/png", + "last-modified": "2025-01-01", + }), + ); const response = await fetch( `${baseUrl}/api/files/${VOL}/preview?path=/Volumes/catalog/schema/vol/image.png`, @@ -323,7 +332,7 @@ describe("Files Plugin Integration", () => { describe("Raw Endpoint Security Headers", () => { test("safe type (image/png) sets security headers without Content-Disposition", async () => { - mockFilesApi.download.mockResolvedValue({ + mockFilesApi.downloadFile.mockResolvedValue({ contents: streamFromString("PNG data"), }); @@ -340,7 +349,7 @@ describe("Files Plugin Integration", () => { }); test("dangerous type (text/html) forces download via Content-Disposition", async () => { - mockFilesApi.download.mockResolvedValue({ + mockFilesApi.downloadFile.mockResolvedValue({ contents: streamFromString(""), }); @@ -359,7 +368,7 @@ describe("Files Plugin Integration", () => { }); test("SVG (image/svg+xml) is treated as dangerous", async () => { - mockFilesApi.download.mockResolvedValue({ + mockFilesApi.downloadFile.mockResolvedValue({ contents: streamFromString(""), }); @@ -377,7 +386,7 @@ describe("Files Plugin Integration", () => { }); test("JavaScript (text/javascript) is treated as dangerous", async () => { - mockFilesApi.download.mockResolvedValue({ + mockFilesApi.downloadFile.mockResolvedValue({ contents: streamFromString("alert('xss')"), }); @@ -395,7 +404,7 @@ describe("Files Plugin Integration", () => { }); test("safe type (application/json) is served inline", async () => { - mockFilesApi.download.mockResolvedValue({ + mockFilesApi.downloadFile.mockResolvedValue({ contents: streamFromString('{"key":"value"}'), }); @@ -414,7 +423,7 @@ describe("Files Plugin Integration", () => { describe("Download Endpoint Security Headers", () => { test("sets X-Content-Type-Options: nosniff", async () => { - mockFilesApi.download.mockResolvedValue({ + mockFilesApi.downloadFile.mockResolvedValue({ contents: streamFromString("file data"), }); @@ -433,7 +442,7 @@ describe("Files Plugin Integration", () => { describe("Service principal execution", () => { test("header-less request + default publicRead() + list β†’ 200 (policy decides)", async () => { - mockFilesApi.listDirectoryContents.mockReturnValue( + mockFilesApi.listDirectoryContentsIter.mockReturnValue( (async function* () { yield { name: "sp-file.txt", @@ -523,7 +532,7 @@ describe("Files Plugin Integration", () => { const port = await getListeningPort(appkit.server.getServer()); const localBase = `http://127.0.0.1:${port}`; - mockFilesApi.listDirectoryContents.mockReturnValue( + mockFilesApi.listDirectoryContentsIter.mockReturnValue( (async function* () { yield { name: "spy-file.txt", @@ -552,7 +561,7 @@ describe("Files Plugin Integration", () => { }); test("requests with user headers also succeed", async () => { - mockFilesApi.listDirectoryContents.mockReturnValue( + mockFilesApi.listDirectoryContentsIter.mockReturnValue( (async function* () { yield { name: "file.txt", @@ -631,7 +640,7 @@ describe("Files Plugin Integration", () => { describe("Error Handling", () => { test("SDK exceptions return 500 with generic error", async () => { - mockFilesApi.getMetadata.mockRejectedValue( + mockSdkClient.request.mockRejectedValue( new Error("SDK connection failed"), ); @@ -647,7 +656,7 @@ describe("Files Plugin Integration", () => { }); test("list errors return 500", async () => { - mockFilesApi.listDirectoryContents.mockRejectedValue( + mockFilesApi.listDirectoryContentsIter.mockRejectedValue( new Error("Permission denied"), ); @@ -663,7 +672,7 @@ describe("Files Plugin Integration", () => { }); test("ApiError 404 preserves upstream status code", async () => { - mockFilesApi.getMetadata.mockRejectedValue( + mockSdkClient.request.mockRejectedValue( createApiError({ statusCode: 404, message: "Not found", @@ -686,7 +695,7 @@ describe("Files Plugin Integration", () => { }); test("ApiError 409 preserves upstream status code", async () => { - mockFilesApi.getMetadata.mockRejectedValue( + mockSdkClient.request.mockRejectedValue( createApiError({ statusCode: 409, message: "Conflict", diff --git a/packages/appkit/src/plugins/files/tests/plugin.test.ts b/packages/appkit/src/plugins/files/tests/plugin.test.ts index f9ae1ad01..f11c96233 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.test.ts @@ -21,12 +21,11 @@ import { PolicyDeniedError, policy, type FilePolicyUser } from "../policy"; const { mockClient, MockApiError } = await vi.hoisted(async () => { const mockFilesApi = { - listDirectoryContents: vi.fn(), - download: vi.fn(), - getMetadata: vi.fn(), + listDirectoryContentsIter: vi.fn(), + downloadFile: vi.fn(), upload: vi.fn(), createDirectory: vi.fn(), - delete: vi.fn(), + deleteFile: vi.fn(), }; const mockClient = { @@ -227,10 +226,10 @@ describe("FilesPlugin", () => { for (const item of items) yield item; }, }); - mockClient.files.listDirectoryContents.mockReturnValueOnce( + mockClient.files.listDirectoryContentsIter.mockReturnValueOnce( asyncIterable([{ path: "uploads-file" }]), ); - mockClient.files.listDirectoryContents.mockReturnValueOnce( + mockClient.files.listDirectoryContentsIter.mockReturnValueOnce( asyncIterable([{ path: "exports-file" }]), ); @@ -700,7 +699,7 @@ describe("FilesPlugin", () => { const handler = getRouteHandlerForTimeout(plugin, "get", "/list"); const res = mockRes(); - mockClient.files.listDirectoryContents.mockImplementation( + mockClient.files.listDirectoryContentsIter.mockImplementation( async function* () { yield { name: "file.txt", path: "/file.txt", is_directory: false }; }, @@ -726,7 +725,7 @@ describe("FilesPlugin", () => { // Simulate an SDK call that rejects (e.g. network error). // Returns an async iterable whose first iteration throws. - mockClient.files.listDirectoryContents.mockReturnValue({ + mockClient.files.listDirectoryContentsIter.mockReturnValue({ [Symbol.asyncIterator]: () => ({ next: () => Promise.reject(new Error("network failure")), }), @@ -751,7 +750,9 @@ describe("FilesPlugin", () => { const handler = getRouteHandlerForTimeout(plugin, "get", "/read"); const res = mockRes(); - mockClient.files.download.mockRejectedValue(new Error("network failure")); + mockClient.files.downloadFile.mockRejectedValue( + new Error("network failure"), + ); const handlerPromise = handler( mockReq("uploads", { query: { path: "test.txt" } }), @@ -773,9 +774,7 @@ describe("FilesPlugin", () => { const handler = getRouteHandlerForTimeout(plugin, "get", "/exists"); const res = mockRes(); - mockClient.files.getMetadata.mockRejectedValue( - new Error("network failure"), - ); + mockClient.request.mockRejectedValue(new Error("network failure")); const handlerPromise = handler( mockReq("uploads", { query: { path: "test.txt" } }), @@ -797,9 +796,7 @@ describe("FilesPlugin", () => { const handler = getRouteHandlerForTimeout(plugin, "get", "/metadata"); const res = mockRes(); - mockClient.files.getMetadata.mockRejectedValue( - new Error("network failure"), - ); + mockClient.request.mockRejectedValue(new Error("network failure")); const handlerPromise = handler( mockReq("uploads", { query: { path: "test.txt" } }), @@ -821,7 +818,9 @@ describe("FilesPlugin", () => { const handler = getRouteHandlerForTimeout(plugin, "get", "/download"); const res = mockRes(); - mockClient.files.download.mockRejectedValue(new Error("network failure")); + mockClient.files.downloadFile.mockRejectedValue( + new Error("network failure"), + ); const handlerPromise = handler( mockReq("uploads", { query: { path: "big.bin" } }), @@ -888,7 +887,7 @@ describe("FilesPlugin", () => { const handler = getRouteHandlerForTimeout(plugin, "get", "/list"); const res = mockRes(); - mockClient.files.listDirectoryContents.mockReturnValue({ + mockClient.files.listDirectoryContentsIter.mockReturnValue({ [Symbol.asyncIterator]: () => ({ next: () => Promise.reject(new Error("internal: secret connection string xyz")), @@ -918,7 +917,7 @@ describe("FilesPlugin", () => { let signalWasAborted = false; const { promise, capturedReject } = hangingWithAbort(); - mockClient.files.listDirectoryContents.mockReturnValue({ + mockClient.files.listDirectoryContentsIter.mockReturnValue({ [Symbol.asyncIterator]: () => ({ next: () => { // Simulate: we set up a timeout that rejects the hanging promise, @@ -1050,7 +1049,7 @@ describe("FilesPlugin", () => { const handler = getRouteHandler(plugin, "get", "/list"); const res = mockRes(); - mockClient.files.listDirectoryContents.mockImplementation( + mockClient.files.listDirectoryContentsIter.mockImplementation( async function* () { yield { name: "h.txt", path: "/h.txt", is_directory: false }; }, @@ -1165,7 +1164,7 @@ describe("FilesPlugin", () => { const handler = getRouteHandler(plugin, "get", "/list"); const res = mockRes(); - mockClient.files.listDirectoryContents.mockImplementation( + mockClient.files.listDirectoryContentsIter.mockImplementation( async function* () { yield { name: "g.txt", path: "/g.txt", is_directory: false }; }, @@ -1253,7 +1252,7 @@ describe("FilesPlugin", () => { const handler = getRouteHandler(plugin, "get", "/list"); const res = mockRes(); - mockClient.files.listDirectoryContents.mockImplementation( + mockClient.files.listDirectoryContentsIter.mockImplementation( async function* () { yield { name: "a.txt", path: "/a.txt", is_directory: false }; }, @@ -1289,7 +1288,7 @@ describe("FilesPlugin", () => { const handler = getRouteHandler(plugin, "get", "/list"); const res = mockRes(); - mockClient.files.listDirectoryContents.mockImplementation( + mockClient.files.listDirectoryContentsIter.mockImplementation( async function* () { yield { name: "b.txt", path: "/b.txt", is_directory: false }; }, @@ -1310,7 +1309,7 @@ describe("FilesPlugin", () => { const handler = getRouteHandler(plugin, "get", "/list"); const res = mockRes(); - mockClient.files.listDirectoryContents.mockImplementation( + mockClient.files.listDirectoryContentsIter.mockImplementation( async function* () { yield { name: "c.txt", path: "/c.txt", is_directory: false }; }, @@ -1452,7 +1451,7 @@ describe("FilesPlugin", () => { const exported = plugin.exports(); const handle = exported("public"); - mockClient.files.listDirectoryContents.mockImplementation( + mockClient.files.listDirectoryContentsIter.mockImplementation( async function* () { yield { name: "d.txt", path: "/d.txt", is_directory: false }; }, @@ -1629,7 +1628,7 @@ describe("FilesPlugin", () => { const handler = getRouteHandler(plugin, "get", "/read"); const res = mockRes(); - mockClient.files.download.mockResolvedValue({ + mockClient.files.downloadFile.mockResolvedValue({ contents: new ReadableStream({ start(controller) { controller.enqueue(new TextEncoder().encode("file content")); @@ -2061,7 +2060,7 @@ describe("FilesPlugin", () => { const handler = getRouteHandler(plugin, "get", "/list"); const res = mockRes(); - mockClient.files.listDirectoryContents.mockImplementation( + mockClient.files.listDirectoryContentsIter.mockImplementation( async function* () { yield { name: "o.txt", path: "/o.txt", is_directory: false }; }, @@ -2122,7 +2121,7 @@ describe("FilesPlugin", () => { // Policy must not have been evaluated and the SDK must not have been // called. expect(policySpy).not.toHaveBeenCalled(); - expect(mockClient.files.listDirectoryContents).not.toHaveBeenCalled(); + expect(mockClient.files.listDirectoryContentsIter).not.toHaveBeenCalled(); }); test("OBO volume + missing token + NODE_ENV === 'development' β†’ exactly one warn, SP fallback proceeds", async () => { @@ -2138,7 +2137,7 @@ describe("FilesPlugin", () => { const handler = getRouteHandler(plugin, "get", "/list"); const res = mockRes(); - mockClient.files.listDirectoryContents.mockImplementation( + mockClient.files.listDirectoryContentsIter.mockImplementation( async function* () { yield { name: "d.txt", path: "/d.txt", is_directory: false }; }, @@ -2322,7 +2321,7 @@ describe("FilesPlugin", () => { // assert that the SDK call ran inside `runInUserContext` with the // expected user identity. const observedUserIds: string[] = []; - mockClient.files.listDirectoryContents.mockImplementation( + mockClient.files.listDirectoryContentsIter.mockImplementation( async function* () { // getCurrentPrincipalId() inside the wrapped fn should resolve to alice. const ctx = await import("../../../context"); @@ -2361,9 +2360,9 @@ describe("FilesPlugin", () => { const handler = getRouteHandler(plugin, "get", "/read"); const res = mockRes(); - // The connector reads via files.download β€” return a valid 200-ish + // The connector reads via files.downloadFile β€” return a valid 200-ish // response with content body. - mockClient.files.download.mockImplementation(async () => ({ + mockClient.files.downloadFile.mockImplementation(async () => ({ contents: new ReadableStream({ start(controller) { controller.enqueue(new TextEncoder().encode("hello")); @@ -2426,7 +2425,7 @@ describe("FilesPlugin", () => { }); const handler = getRouteHandler(plugin, "get", "/list"); - mockClient.files.listDirectoryContents.mockImplementation( + mockClient.files.listDirectoryContentsIter.mockImplementation( async function* () { yield { name: "f.txt", path: "/f.txt", is_directory: false }; }, @@ -2455,7 +2454,9 @@ describe("FilesPlugin", () => { // Cache is disabled on OBO: `getOrExecute` is bypassed. The SDK // must execute on every request β€” no cross-user staleness possible. expect(getOrExecute).not.toHaveBeenCalled(); - expect(mockClient.files.listDirectoryContents).toHaveBeenCalledTimes(2); + expect(mockClient.files.listDirectoryContentsIter).toHaveBeenCalledTimes( + 2, + ); }); test("SP volume reads still use the cache (cache is only disabled for OBO)", async () => { @@ -2474,7 +2475,7 @@ describe("FilesPlugin", () => { const listHandler = getRouteHandler(plugin, "get", "/list"); - mockClient.files.listDirectoryContents.mockImplementation( + mockClient.files.listDirectoryContentsIter.mockImplementation( async function* () { yield { name: "f.txt", path: "/f.txt", is_directory: false }; }, @@ -2535,7 +2536,7 @@ describe("FilesPlugin", () => { }); const handler = getRouteHandler(plugin, "get", "/list"); - mockClient.files.listDirectoryContents.mockImplementation( + mockClient.files.listDirectoryContentsIter.mockImplementation( async function* () { yield { name: "f.txt", path: "/f.txt", is_directory: false }; }, @@ -2896,7 +2897,7 @@ describe("FilesPlugin", () => { await useRealGetCurrentPrincipalId(); await useRealGetWorkspaceClient(); - // Distinct user-token client with a `files.delete` that mimics a UC + // Distinct user-token client with a `files.deleteFile` that mimics a UC // failure (e.g. 403 from UC because the user lacks privilege). const userClient = { config: { @@ -2906,7 +2907,7 @@ describe("FilesPlugin", () => { }), }, files: { - delete: vi.fn(async () => { + deleteFile: vi.fn(async () => { throw new MockApiError("UC denied", 403); }), }, @@ -2946,8 +2947,8 @@ describe("FilesPlugin", () => { ); // The user-token client was used, not the SP one. - expect(userClient.files.delete).toHaveBeenCalledTimes(1); - expect(mockClient.files.delete).not.toHaveBeenCalled(); + expect(userClient.files.deleteFile).toHaveBeenCalledTimes(1); + expect(mockClient.files.deleteFile).not.toHaveBeenCalled(); // The UC error surfaced as 403 (the ApiError statusCode). expect(res.status).toHaveBeenCalledWith(403); @@ -3208,7 +3209,7 @@ describe("FilesPlugin", () => { // [{...}]. We toggle the return AFTER alice's upload has reached // the SDK to simulate cross-user freshness. let postUploadVisible = false; - mockClient.files.listDirectoryContents.mockImplementation( + mockClient.files.listDirectoryContentsIter.mockImplementation( async function* () { if (postUploadVisible) { yield { @@ -3329,7 +3330,7 @@ describe("FilesPlugin", () => { // `.files` β€” making any leak through the SP path crash loudly). await useRealGetWorkspaceClient(); - // Distinct user-token client whose `listDirectoryContents` is the + // Distinct user-token client whose `listDirectoryContentsIter` is the // one we expect `asUser` to route through. const userListSpy = vi.fn(async function* () { yield { name: "user.txt", path: "/user.txt", is_directory: false }; @@ -3339,7 +3340,7 @@ describe("FilesPlugin", () => { host: "https://test.databricks.com", authenticate: vi.fn(), }, - files: { listDirectoryContents: userListSpy }, + files: { listDirectoryContentsIter: userListSpy }, }; // Wire `_buildUserContextOrNull β†’ ServiceContext.createUserContext` to @@ -3405,7 +3406,7 @@ describe("FilesPlugin", () => { const spListSpy = vi.fn(async function* () { yield { name: "sp.txt", path: "/sp.txt", is_directory: false }; }); - mockClient.files.listDirectoryContents.mockImplementation(spListSpy); + mockClient.files.listDirectoryContentsIter.mockImplementation(spListSpy); // Spy on createUserContext to confirm no wrap happened. serviceContextMock.createUserContextSpy.mockClear(); @@ -3442,7 +3443,7 @@ describe("FilesPlugin", () => { const spListSpy = vi.fn(async function* () { yield { name: "sp.txt", path: "/sp.txt", is_directory: false }; }); - mockClient.files.listDirectoryContents.mockImplementation(spListSpy); + mockClient.files.listDirectoryContentsIter.mockImplementation(spListSpy); serviceContextMock.createUserContextSpy.mockClear(); @@ -3497,12 +3498,12 @@ describe("FilesPlugin", () => { process.env.NODE_ENV = "development"; const policySpy = vi.fn().mockReturnValue(true); - // Use the default mocked SP client; spy on its listDirectoryContents + // Use the default mocked SP client; spy on its listDirectoryContentsIter // to confirm the SDK call resolved against the SP path. const spListSpy = vi.fn(async function* () { yield { name: "sp.txt", path: "/sp.txt", is_directory: false }; }); - mockClient.files.listDirectoryContents.mockImplementation(spListSpy); + mockClient.files.listDirectoryContentsIter.mockImplementation(spListSpy); serviceContextMock.createUserContextSpy.mockClear(); @@ -3654,7 +3655,7 @@ describe("FilesPlugin", () => { }); const calls = spyOnTelemetry(plugin); - mockClient.files.listDirectoryContents.mockImplementation( + mockClient.files.listDirectoryContentsIter.mockImplementation( async function* () { yield { name: "f.txt", path: "/f.txt", is_directory: false }; }, @@ -3680,7 +3681,7 @@ describe("FilesPlugin", () => { const plugin = new FilesPlugin(VOLUMES_CONFIG); const calls = spyOnTelemetry(plugin); - mockClient.files.listDirectoryContents.mockImplementation( + mockClient.files.listDirectoryContentsIter.mockImplementation( async function* () { yield { name: "f.txt", path: "/f.txt", is_directory: false }; }, @@ -3706,7 +3707,7 @@ describe("FilesPlugin", () => { const plugin = new FilesPlugin(VOLUMES_CONFIG); const calls = spyOnTelemetry(plugin); - mockClient.files.listDirectoryContents.mockImplementation( + mockClient.files.listDirectoryContentsIter.mockImplementation( async function* () { yield { name: "f.txt", path: "/f.txt", is_directory: false }; }, @@ -3743,7 +3744,7 @@ describe("FilesPlugin", () => { const plugin = new FilesPlugin(VOLUMES_CONFIG); const calls = spyOnTelemetry(plugin); - mockClient.files.listDirectoryContents.mockImplementation( + mockClient.files.listDirectoryContentsIter.mockImplementation( async function* () { yield { name: "f.txt", path: "/f.txt", is_directory: false }; }, @@ -3772,7 +3773,7 @@ describe("FilesPlugin", () => { const plugin = new FilesPlugin(VOLUMES_CONFIG); const calls = spyOnTelemetry(plugin); - mockClient.files.listDirectoryContents.mockImplementation( + mockClient.files.listDirectoryContentsIter.mockImplementation( async function* () { yield { name: "f.txt", path: "/f.txt", is_directory: false }; }, @@ -3805,7 +3806,7 @@ describe("FilesPlugin", () => { }); const calls = spyOnTelemetry(plugin); - mockClient.files.listDirectoryContents.mockImplementation( + mockClient.files.listDirectoryContentsIter.mockImplementation( async function* () { yield { name: "f.txt", path: "/f.txt", is_directory: false }; }, @@ -3863,7 +3864,7 @@ describe("FilesPlugin", () => { const plugin = new FilesPlugin({ volumes: { uploads: { policy: recorder }, exports: {} }, }); - mockClient.files.listDirectoryContents.mockImplementation( + mockClient.files.listDirectoryContentsIter.mockImplementation( async function* () {}, ); const handle = plugin.exports()("uploads"); @@ -3900,7 +3901,7 @@ describe("FilesPlugin", () => { const plugin = new FilesPlugin({ volumes: { uploads: { policy: spOnly }, exports: {} }, }); - mockClient.files.listDirectoryContents.mockImplementation( + mockClient.files.listDirectoryContentsIter.mockImplementation( async function* () {}, ); const handle = plugin.exports()("uploads"); @@ -3914,7 +3915,7 @@ describe("FilesPlugin", () => { test("appkit.asUser(req) tags files.auth_mode as on-behalf-of-user", async () => { const plugin = new FilesPlugin(VOLUMES_CONFIG); const calls = spyOnTelemetry(plugin); - mockClient.files.listDirectoryContents.mockImplementation( + mockClient.files.listDirectoryContentsIter.mockImplementation( async function* () {}, ); diff --git a/packages/appkit/src/plugins/files/tests/raw-endpoint.test.ts b/packages/appkit/src/plugins/files/tests/raw-endpoint.test.ts index 9a2e1587a..a288dbb27 100644 --- a/packages/appkit/src/plugins/files/tests/raw-endpoint.test.ts +++ b/packages/appkit/src/plugins/files/tests/raw-endpoint.test.ts @@ -28,12 +28,11 @@ describe("FilesPlugin raw endpoint security headers", () => { client = createMockWorkspaceClient({ strict: true, responses: { - "files.listDirectoryContents": undefined, - "files.download": undefined, - "files.getMetadata": undefined, + "files.listDirectoryContentsIter": undefined, + "files.downloadFile": undefined, "files.upload": undefined, "files.createDirectory": undefined, - "files.delete": undefined, + "files.deleteFile": undefined, }, }); serviceContextMock = await setupTestEnv(client); @@ -48,7 +47,7 @@ describe("FilesPlugin raw endpoint security headers", () => { const handler = getRouteHandler(plugin, "get", "/raw"); const res = mockRes(); - getMock(client, "files.download").mockResolvedValue( + getMock(client, "files.downloadFile").mockResolvedValue( makeStreamResponse("data"), ); @@ -70,7 +69,7 @@ describe("FilesPlugin raw endpoint security headers", () => { const handler = getRouteHandler(plugin, "get", "/raw"); const res = mockRes(); - getMock(client, "files.download").mockResolvedValue( + getMock(client, "files.downloadFile").mockResolvedValue( makeStreamResponse("PNG data"), ); @@ -98,7 +97,7 @@ describe("FilesPlugin raw endpoint security headers", () => { const handler = getRouteHandler(plugin, "get", "/raw"); const res = mockRes(); - getMock(client, "files.download").mockResolvedValue( + getMock(client, "files.downloadFile").mockResolvedValue( makeStreamResponse(""), ); @@ -125,7 +124,7 @@ describe("FilesPlugin raw endpoint security headers", () => { const handler = getRouteHandler(plugin, "get", "/raw"); const res = mockRes(); - getMock(client, "files.download").mockResolvedValue( + getMock(client, "files.downloadFile").mockResolvedValue( makeStreamResponse(""), ); @@ -147,7 +146,7 @@ describe("FilesPlugin raw endpoint security headers", () => { const handler = getRouteHandler(plugin, "get", "/raw"); const res = mockRes(); - getMock(client, "files.download").mockResolvedValue( + getMock(client, "files.downloadFile").mockResolvedValue( makeStreamResponse("content"), ); diff --git a/packages/appkit/src/plugins/files/tests/shutdown.test.ts b/packages/appkit/src/plugins/files/tests/shutdown.test.ts index 31edbb8d0..d987336cf 100644 --- a/packages/appkit/src/plugins/files/tests/shutdown.test.ts +++ b/packages/appkit/src/plugins/files/tests/shutdown.test.ts @@ -16,12 +16,11 @@ describe("FilesPlugin shutdown and trackWrite", () => { client = createMockWorkspaceClient({ strict: true, responses: { - "files.listDirectoryContents": undefined, - "files.download": undefined, - "files.getMetadata": undefined, + "files.listDirectoryContentsIter": undefined, + "files.downloadFile": undefined, "files.upload": undefined, "files.createDirectory": undefined, - "files.delete": undefined, + "files.deleteFile": undefined, }, }); serviceContextMock = await setupTestEnv(client); diff --git a/packages/appkit/src/plugins/files/tests/upload.test.ts b/packages/appkit/src/plugins/files/tests/upload.test.ts index 9aa0c6357..c483a59ca 100644 --- a/packages/appkit/src/plugins/files/tests/upload.test.ts +++ b/packages/appkit/src/plugins/files/tests/upload.test.ts @@ -24,12 +24,11 @@ describe("FilesPlugin upload", () => { client = createMockWorkspaceClient({ strict: true, responses: { - "files.listDirectoryContents": undefined, - "files.download": undefined, - "files.getMetadata": undefined, + "files.listDirectoryContentsIter": undefined, + "files.downloadFile": undefined, "files.upload": undefined, "files.createDirectory": undefined, - "files.delete": undefined, + "files.deleteFile": undefined, }, }); serviceContextMock = await setupTestEnv(client); diff --git a/packages/appkit/src/plugins/files/tests/utils.ts b/packages/appkit/src/plugins/files/tests/utils.ts index beb947a87..9bd7b35d6 100644 --- a/packages/appkit/src/plugins/files/tests/utils.ts +++ b/packages/appkit/src/plugins/files/tests/utils.ts @@ -1 +1,4 @@ -export { streamFromString } from "../../../connectors/files/tests/utils"; +export { + headResponse, + streamFromString, +} from "../../../connectors/files/tests/utils"; diff --git a/packages/appkit/src/plugins/files/tests/volume-config.test.ts b/packages/appkit/src/plugins/files/tests/volume-config.test.ts index 8ddba4b88..c1250a6d9 100644 --- a/packages/appkit/src/plugins/files/tests/volume-config.test.ts +++ b/packages/appkit/src/plugins/files/tests/volume-config.test.ts @@ -17,12 +17,11 @@ describe("FilesPlugin volume config surface", () => { client = createMockWorkspaceClient({ strict: true, responses: { - "files.listDirectoryContents": undefined, - "files.download": undefined, - "files.getMetadata": undefined, + "files.listDirectoryContentsIter": undefined, + "files.downloadFile": undefined, "files.upload": undefined, "files.createDirectory": undefined, - "files.delete": undefined, + "files.deleteFile": undefined, }, }); serviceContextMock = await setupTestEnv(client); diff --git a/packages/appkit/src/plugins/files/types.ts b/packages/appkit/src/plugins/files/types.ts index ee58c8c04..6fb235c99 100644 --- a/packages/appkit/src/plugins/files/types.ts +++ b/packages/appkit/src/plugins/files/types.ts @@ -1,6 +1,5 @@ import type { BasePluginConfig, IAppRequest } from "shared"; -import type { files } from "../../workspace-client"; import type { FilePolicy } from "./policy"; /** @@ -159,11 +158,28 @@ export interface IFilesConfig extends BasePluginConfig { auth?: "service-principal" | "on-behalf-of-user"; } -/** A single entry returned when listing a directory. Re-exported from `@databricks/sdk-experimental`. */ -export type DirectoryEntry = files.DirectoryEntry; +/** A single entry returned when listing a directory (Files API wire shape). */ +export interface DirectoryEntry { + /** The length of the file in bytes. Omitted for directories. */ + file_size?: number; + /** True if the path is a directory. */ + is_directory?: boolean; + /** Last modification time in milliseconds since unix epoch. */ + last_modified?: number; + /** The name of the file or directory (last path component). */ + name?: string; + /** The absolute path of the file or directory. */ + path?: string; +} -/** Response object for file downloads containing a readable stream. Re-exported from `@databricks/sdk-experimental`. */ -export type DownloadResponse = files.DownloadResponse; +/** Response object for file downloads containing a readable stream. */ +export interface DownloadResponse { + "content-length"?: number; + "content-type"?: string; + contents?: ReadableStream; + /** Last modified time in HTTP-date (RFC 7231) format. */ + "last-modified"?: string; +} /** * Metadata for a file stored in a Unity Catalog volume. diff --git a/packages/shared/package.json b/packages/shared/package.json index 01938030b..451fbcbc0 100644 --- a/packages/shared/package.json +++ b/packages/shared/package.json @@ -51,6 +51,7 @@ "@databricks/sdk-auth": "0.51.0", "@databricks/sdk-core": "0.51.0", "@databricks/sdk-experimental": "0.17.0", + "@databricks/sdk-files": "0.52.0", "@databricks/sdk-genie": "0.54.0", "@databricks/sdk-jobs": "0.57.0", "@databricks/sdk-options": "0.51.0", diff --git a/packages/shared/src/workspace-client/client.ts b/packages/shared/src/workspace-client/client.ts index 92ca56b11..ff0074ca5 100644 --- a/packages/shared/src/workspace-client/client.ts +++ b/packages/shared/src/workspace-client/client.ts @@ -22,6 +22,8 @@ import { type GenieClient, buildJobsClient, type JobsClient, + buildFilesClient, + type FilesClient, type StatementExecutionClient, type WarehousesClient, type WorkspaceAuth, @@ -38,13 +40,18 @@ export class AppKitWorkspaceClient implements WorkspaceClient { #currentUser?: ScimClient; #genie?: GenieClient; #jobs?: JobsClient; + #files?: FilesClient; constructor(opts: WorkspaceClientOptions) { this.#opts = opts; } - get files() { - return this.#getLegacy().files; + // Migrated to the modular SDK β€” built lazily, independent of the legacy client. + get files(): FilesClient { + if (!this.#files) { + this.#files = buildFilesClient(this.#opts); + } + return this.#files; } // Migrated to the modular SDK β€” built lazily, independent of the legacy client. diff --git a/packages/shared/src/workspace-client/index.ts b/packages/shared/src/workspace-client/index.ts index d55adf9db..b4075398c 100644 --- a/packages/shared/src/workspace-client/index.ts +++ b/packages/shared/src/workspace-client/index.ts @@ -21,6 +21,6 @@ export { Time, TimeUnits, } from "./legacy"; -export type { files, serving, sql, WorkspaceClient } from "./types"; +export type { serving, sql, WorkspaceClient } from "./types"; // Modular SDK client + model types (warehouses). export type * from "./modular"; diff --git a/packages/shared/src/workspace-client/modular.ts b/packages/shared/src/workspace-client/modular.ts index 3449d4633..e701e473d 100644 --- a/packages/shared/src/workspace-client/modular.ts +++ b/packages/shared/src/workspace-client/modular.ts @@ -45,6 +45,7 @@ import { newFetchHttpClient, } from "@databricks/sdk-core/http"; import { type ProfileOptions, resolve } from "@databricks/sdk-core/profiles"; +import type { FilesClient } from "@databricks/sdk-files/v2"; import type { GenieClient } from "@databricks/sdk-genie/v1"; import type { JobsClient } from "@databricks/sdk-jobs/v2"; import type { ClientOptions } from "@databricks/sdk-options/client"; @@ -553,7 +554,21 @@ export function buildJobsClient(opts: WorkspaceClientOptions): JobsClient { ); } +/** + * Build a modular Files client from wrapper options. `getFileMetadata` is not + * used: its generated HEAD call parses the (empty) body and drops the response + * headers that carry the metadata, so the connector issues that HEAD through + * {@link WorkspaceAuth.request} instead. + */ +export function buildFilesClient(opts: WorkspaceClientOptions): FilesClient { + return lazyClient( + () => import("@databricks/sdk-files/v2").then((m) => m.FilesClient), + mapToClientOptions(opts), + ); +} + // ── Client type re-exports (for the facade accessor types) ─────────────── +export type { FilesClient } from "@databricks/sdk-files/v2"; export type { GenieClient } from "@databricks/sdk-genie/v1"; export type { JobsClient } from "@databricks/sdk-jobs/v2"; export type { ScimClient } from "@databricks/sdk-scim/v1"; diff --git a/packages/shared/src/workspace-client/types.ts b/packages/shared/src/workspace-client/types.ts index f347dbc01..f32f5b517 100644 --- a/packages/shared/src/workspace-client/types.ts +++ b/packages/shared/src/workspace-client/types.ts @@ -8,7 +8,7 @@ * service means changing its accessor's type here, its getter in `client.ts`, * and the connector that consumes it. * - * Type-namespace re-exports (`files`, `serving`, `sql`) point at + * Type-namespace re-exports (`serving`, `sql`) point at * `@databricks/sdk-experimental` so existing AppKit call-site shapes stay * stable. They move to the modular `@databricks/sdk-` model exports * as each service migrates. @@ -21,6 +21,7 @@ import type { ScimClient, GenieClient, JobsClient, + FilesClient, } from "./modular"; // Legacy SDK type namespaces for un-migrated services, re-exported so AppKit @@ -31,8 +32,8 @@ import type { // as `sql.EndpointInfo[]`. It is not on the modular `listWarehouses` because // that request has no `skip_cannot_use` filter, so it could pick a warehouse // the caller can't use. Statement + warehouse service types come from `./modular`. -export type { files, serving, sql } from "@databricks/sdk-experimental"; -// Modular SDK client + model types (warehouses, statementExecution, genie, jobs). +export type { serving, sql } from "@databricks/sdk-experimental"; +// Modular SDK client + model types (warehouses, statementExecution, genie, jobs, files). export type * from "./modular"; /** @@ -44,8 +45,8 @@ export type * from "./modular"; * client); see the module docblock. */ export interface WorkspaceClient extends WorkspaceAuth { - /** UC Volumes / Files API. */ - readonly files: LegacyWorkspaceClient["files"]; + /** UC Volumes / Files API (modular SDK). */ + readonly files: FilesClient; /** SQL Warehouses (modular SDK). */ readonly warehouses: WarehousesClient; diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index c1b0f795e..3c6b8a5cf 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -289,6 +289,9 @@ importers: '@databricks/sdk-experimental': specifier: 0.17.0 version: 0.17.0 + '@databricks/sdk-files': + specifier: 0.52.0 + version: 0.52.0 '@databricks/sdk-genie': specifier: 0.54.0 version: 0.54.0(patch_hash=664f3e13b155eb1b5499486a4f6fc1ab9a6a502e2c6b200d06749bd25dd9ae54) @@ -628,6 +631,9 @@ importers: '@databricks/sdk-experimental': specifier: 0.17.0 version: 0.17.0 + '@databricks/sdk-files': + specifier: 0.52.0 + version: 0.52.0 '@databricks/sdk-genie': specifier: 0.54.0 version: 0.54.0(patch_hash=664f3e13b155eb1b5499486a4f6fc1ab9a6a502e2c6b200d06749bd25dd9ae54) @@ -2027,6 +2033,10 @@ packages: resolution: {integrity: sha512-dOJIt4F2nBk6HKObnv7Xbmy/qLYTy2835qhXSuW0Qw1QAXui9plmCet1KqG3yeQcMTyncWGbnhjGdQi8GEGQSA==} engines: {node: '>=22.0', npm: '>=10.0.0'} + '@databricks/sdk-files@0.52.0': + resolution: {integrity: sha512-H3/uAkkl7EI+60/q1FRqtKrG7UdJfWKLug4/87q+P7OXJMF3J8kgECIg1LfU2Ih2Qj42LNhqdjos7LJSC68gyw==} + engines: {node: '>=22.0.0'} + '@databricks/sdk-genie@0.54.0': resolution: {integrity: sha512-xT+dyuXocgwPJ5VA1Kr5/dSdKzzR9hRAkEOybeO2i+v891kuuFzVd/M9OM31PtO2/YZp95vdFH5Sb+jVGtm4nw==} engines: {node: '>=22.0.0'} @@ -14347,6 +14357,15 @@ snapshots: transitivePeerDependencies: - supports-color + '@databricks/sdk-files@0.52.0': + dependencies: + '@databricks/sdk-auth': 0.51.0 + '@databricks/sdk-core': 0.51.0 + '@databricks/sdk-options': 0.51.0 + '@js-temporal/polyfill': 0.5.1 + json-bigint: 1.0.0 + zod: 4.3.6 + '@databricks/sdk-genie@0.54.0(patch_hash=664f3e13b155eb1b5499486a4f6fc1ab9a6a502e2c6b200d06749bd25dd9ae54)': dependencies: '@databricks/sdk-auth': 0.51.0 From 3c96603e2435e51ee5e532fe6a91b25e4f311b6c Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Tue, 13 Oct 2026 12:40:34 +0200 Subject: [PATCH 2/6] test(playground): seed request for the files metadata testing-kit examples Files metadata now reads a raw HEAD through client.request, so the files.getMetadata seed never reached the route. Co-authored-by: Isaac Signed-off-by: MarioCadenas --- .../server/testing-kit.integration.test.ts | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/apps/dev-playground/server/testing-kit.integration.test.ts b/apps/dev-playground/server/testing-kit.integration.test.ts index fa644ba4f..f3a8c4ffd 100644 --- a/apps/dev-playground/server/testing-kit.integration.test.ts +++ b/apps/dev-playground/server/testing-kit.integration.test.ts @@ -38,22 +38,25 @@ const filesPlugin = () => files({ volumes: { reports: { policy: files.policy.allowAll() } } }); describe("getMock β€” assert the workspace-client call a route made", () => { - test("GET /api/files/reports/metadata calls files.getMetadata", async () => { + // Metadata is a raw HEAD (the typed call drops the headers), so seed `request`. + test("GET /api/files/reports/metadata issues a HEAD request", async () => { await using app = await createTestApp({ plugins: [filesPlugin()], env: FILES_ENV, responses: { - "files.getMetadata": { - "content-length": "42", - "content-type": "text/plain", - }, + request: () => + new Response(null, { + headers: { "content-length": "42", "content-type": "text/plain" }, + }), }, }); const res = await app.get("/api/files/reports/metadata?path=report.csv"); expect(res.status).toBe(200); - expect(getMock(app.client, "files.getMetadata")).toHaveBeenCalled(); + expect(getMock(app.client, "request")).toHaveBeenCalledWith( + expect.objectContaining({ method: "HEAD" }), + ); }); }); @@ -90,7 +93,7 @@ describe("createApiError β€” simulate a typed workspace error", () => { plugins: [filesPlugin()], env: FILES_ENV, responses: { - "files.getMetadata": () => { + request: () => { throw createApiError({ statusCode: 404, message: "No such file", From f0a3644dd10288fe1bfd413abe67ce89917d1897 Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Wed, 14 Oct 2026 11:23:09 +0200 Subject: [PATCH 3/6] feat(appkit): expose modular files types, deprecate legacy shapes Export FilesDirectoryEntry / FilesDownloadResponse (modular SDK shapes) from the package root alongside the legacy DirectoryEntry / DownloadResponse, now marked @deprecated. list() and download() keep returning the legacy shapes. Add TODOs for the /list camelCase migration and for switching metadata to getFileMetadata once sdk-files reads the HEAD headers. Co-authored-by: Isaac Signed-off-by: MarioCadenas --- docs/docs/plugins/files.md | 23 +++++++++++++-- .../appkit/src/connectors/files/client.ts | 5 ++++ packages/appkit/src/index.ts | 6 ++++ packages/appkit/src/plugins/files/types.ts | 28 +++++++++++++++++-- .../shared/src/workspace-client/modular.ts | 4 +++ 5 files changed, 61 insertions(+), 5 deletions(-) diff --git a/docs/docs/plugins/files.md b/docs/docs/plugins/files.md index 5deec0adc..85ad34ea8 100644 --- a/docs/docs/plugins/files.md +++ b/docs/docs/plugins/files.md @@ -485,9 +485,26 @@ The `list()` method with no arguments lists the volume root. ## Types ```ts -// Re-exported from @databricks/sdk-experimental -type DirectoryEntry = files.DirectoryEntry; -type DownloadResponse = files.DownloadResponse; +/** @deprecated Use FilesDirectoryEntry. Still returned by list() and /list. */ +interface DirectoryEntry { + file_size?: number; + is_directory?: boolean; + last_modified?: number; + name?: string; + path?: string; +} + +/** @deprecated Use FilesDownloadResponse. Still returned by download(). */ +interface DownloadResponse { + "content-length"?: number; + "content-type"?: string; + contents?: ReadableStream; + "last-modified"?: string; +} + +// Modular SDK shapes (camelCase; contentLength is bigint) +type FilesDirectoryEntry = { fileSize?: number; isDirectory?: boolean; lastModified?: number; name?: string; path?: string }; +type FilesDownloadResponse = { contentLength?: bigint; contentType?: string; contents?: ReadableStream; lastModified?: string }; interface FileMetadata { /** File size in bytes. */ diff --git a/packages/appkit/src/connectors/files/client.ts b/packages/appkit/src/connectors/files/client.ts index e5a98921d..3d249cbfe 100644 --- a/packages/appkit/src/connectors/files/client.ts +++ b/packages/appkit/src/connectors/files/client.ts @@ -233,6 +233,9 @@ export class FilesConnector { } return this.traced("list", { "files.path": resolvedPath }, async () => { + // TODO: return the SDK's camelCase entries as-is once appkit-ui's file + // browser and the template's FilesPage read camelCase. Changing the + // `/list` JSON is breaking for app code reading `is_directory` etc. const entries: DirectoryEntry[] = []; for await (const entry of client.files.listDirectoryContentsIter({ directoryPath: resolvedPath, @@ -315,6 +318,8 @@ export class FilesConnector { return this.traced("metadata", { "files.path": resolvedPath }, async () => { // The modular `getFileMetadata` parses the empty HEAD body and drops the // response headers that carry the metadata, so read them off a raw HEAD. + // TODO: switch to `client.files.getFileMetadata` once sdk-files builds the + // response from the HEAD headers (as `downloadFile` already does). const res = await client.request({ method: "HEAD", path: `/api/2.0/fs/files${resolvedPath}`, diff --git a/packages/appkit/src/index.ts b/packages/appkit/src/index.ts index 6bc5f1b28..fabe656b3 100644 --- a/packages/appkit/src/index.ts +++ b/packages/appkit/src/index.ts @@ -93,6 +93,12 @@ export { READ_ACTIONS, WRITE_ACTIONS, } from "./plugins/files/policy"; +export type { + DirectoryEntry, + DownloadResponse, + FilesDirectoryEntry, + FilesDownloadResponse, +} from "./plugins/files/types"; export * from "./plugins/ga-exports.generated"; export type { IJobsConfig, diff --git a/packages/appkit/src/plugins/files/types.ts b/packages/appkit/src/plugins/files/types.ts index 6fb235c99..9fd365e59 100644 --- a/packages/appkit/src/plugins/files/types.ts +++ b/packages/appkit/src/plugins/files/types.ts @@ -1,5 +1,9 @@ import type { BasePluginConfig, IAppRequest } from "shared"; +import type { + FilesDirectoryEntry as WorkspaceFilesDirectoryEntry, + FilesDownloadResponse as WorkspaceFilesDownloadResponse, +} from "../../workspace-client"; import type { FilePolicy } from "./policy"; /** @@ -158,7 +162,24 @@ export interface IFilesConfig extends BasePluginConfig { auth?: "service-principal" | "on-behalf-of-user"; } -/** A single entry returned when listing a directory (Files API wire shape). */ +/** + * A single directory entry in the modular SDK shape (camelCase). `list()` and + * the `/list` route still return the legacy {@link DirectoryEntry}; they move to + * this type in a future release. + */ +export type FilesDirectoryEntry = WorkspaceFilesDirectoryEntry; + +/** + * Download response in the modular SDK shape (`contentLength` is a `bigint`). + * `download()` still returns the legacy {@link DownloadResponse}; it moves to this + * type in a future release. + */ +export type FilesDownloadResponse = WorkspaceFilesDownloadResponse; + +/** + * A single entry returned when listing a directory (Files API wire shape). + * @deprecated Legacy snake_case shape; use {@link FilesDirectoryEntry}. + */ export interface DirectoryEntry { /** The length of the file in bytes. Omitted for directories. */ file_size?: number; @@ -172,7 +193,10 @@ export interface DirectoryEntry { path?: string; } -/** Response object for file downloads containing a readable stream. */ +/** + * Response object for file downloads containing a readable stream. + * @deprecated Legacy header-keyed shape; use {@link FilesDownloadResponse}. + */ export interface DownloadResponse { "content-length"?: number; "content-type"?: string; diff --git a/packages/shared/src/workspace-client/modular.ts b/packages/shared/src/workspace-client/modular.ts index e701e473d..1d1cebb21 100644 --- a/packages/shared/src/workspace-client/modular.ts +++ b/packages/shared/src/workspace-client/modular.ts @@ -569,6 +569,10 @@ export function buildFilesClient(opts: WorkspaceClientOptions): FilesClient { // ── Client type re-exports (for the facade accessor types) ─────────────── export type { FilesClient } from "@databricks/sdk-files/v2"; +export type { + DirectoryEntry as FilesDirectoryEntry, + DownloadFileResponse as FilesDownloadResponse, +} from "@databricks/sdk-files/v2"; export type { GenieClient } from "@databricks/sdk-genie/v1"; export type { JobsClient } from "@databricks/sdk-jobs/v2"; export type { ScimClient } from "@databricks/sdk-scim/v1"; From a5186d8ac867e148023eaa4ca8f2c854ae2de6cb Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Wed, 14 Oct 2026 15:37:03 +0200 Subject: [PATCH 4/6] feat(appkit): upload files through the modular uploadFile call Replace the raw PUT workaround in FilesConnector.upload() with client.files.uploadFile. Buffer/string bodies are wrapped in a stream (uploadFile only takes a ReadableStream). Errors keep the UPLOAD_FAILED re-wrap with the upstream status and 200-char safe message. The route now passes the execute() timeout signal through. uploadFile does not go through executeCall, so the SDK never retries it, and FILES_WRITE_DEFAULTS disables AppKit retry; a consumed stream body is never replayed. Co-authored-by: Isaac Signed-off-by: MarioCadenas --- .../appkit/src/connectors/files/client.ts | 68 ++++----- .../src/connectors/files/tests/client.test.ts | 135 +++++++----------- packages/appkit/src/plugins/files/plugin.ts | 14 +- .../src/plugins/files/tests/delete.test.ts | 2 +- .../files/tests/download-endpoint.test.ts | 2 +- .../files/tests/error-handling.test.ts | 2 +- .../files/tests/path-validation.test.ts | 4 +- .../files/tests/plugin.integration.test.ts | 4 +- .../src/plugins/files/tests/plugin.test.ts | 114 ++++----------- .../plugins/files/tests/raw-endpoint.test.ts | 2 +- .../src/plugins/files/tests/shutdown.test.ts | 2 +- .../src/plugins/files/tests/upload.test.ts | 2 +- .../plugins/files/tests/volume-config.test.ts | 2 +- 13 files changed, 125 insertions(+), 228 deletions(-) diff --git a/packages/appkit/src/connectors/files/client.ts b/packages/appkit/src/connectors/files/client.ts index 3d249cbfe..0d7c16631 100644 --- a/packages/appkit/src/connectors/files/client.ts +++ b/packages/appkit/src/connectors/files/client.ts @@ -342,57 +342,39 @@ export class FilesConnector { client: WorkspaceClient, filePath: string, contents: ReadableStream | Buffer | string, - options?: { overwrite?: boolean }, + options?: { overwrite?: boolean; signal?: AbortSignal }, ): Promise { const resolvedPath = this.resolvePath(filePath); return this.traced("upload", { "files.path": resolvedPath }, async () => { - const body = contents; - const overwrite = options?.overwrite ?? true; - - // Workaround: The SDK's files.upload() has two bugs: - // 1. It ignores the `contents` field (sets body to undefined) - // 2. apiClient.request() checks `instanceof` against its own ReadableStream - // subclass, so standard ReadableStream instances get JSON.stringified to "{}" - // Bypass both by calling the REST API directly with SDK-provided auth. - const hostValue = client.config.host; - if (!hostValue) { - throw new Error( - "Databricks host is not configured. Set DATABRICKS_HOST or configure client.config.host.", + // `uploadFile` takes only a ReadableStream body. + const body = + contents instanceof ReadableStream + ? contents + : new Blob([contents]).stream(); + try { + await client.files.uploadFile( + { + filePath: resolvedPath, + contents: body, + overwrite: options?.overwrite ?? true, + }, + { signal: options?.signal }, ); - } - const host = hostValue.startsWith("http") - ? hostValue - : `https://${hostValue}`; - const url = new URL(`/api/2.0/fs/files${resolvedPath}`, host); - url.searchParams.set("overwrite", String(overwrite)); - - const headers = new Headers({ - "Content-Type": "application/octet-stream", - "User-Agent": client.apiClient.userAgent(), - }); - const fetchOptions: RequestInit = { method: "PUT", headers, body }; - - if (body instanceof ReadableStream) { - fetchOptions.duplex = "half"; - } else if (body instanceof Buffer) { - headers.set("Content-Length", String(body.length)); - } else if (typeof body === "string") { - headers.set("Content-Length", String(Buffer.byteLength(body))); - } - - await client.config.authenticate(headers); - - const res = await fetch(url.toString(), fetchOptions); - - if (!res.ok) { - const text = await res.text(); - logger.error(`Upload failed (${res.status}): ${text}`); - const safeMessage = text.length > 200 ? `${text.slice(0, 200)}…` : text; + } catch (e) { + const wrapped = toWrapperApiError(e); + if (!(wrapped instanceof ApiError)) throw e; + logger.error( + `Upload failed (${wrapped.statusCode}): ${wrapped.message}`, + ); + const safeMessage = + wrapped.message.length > 200 + ? `${wrapped.message.slice(0, 200)}…` + : wrapped.message; throw new ApiError( `Upload failed: ${safeMessage}`, "UPLOAD_FAILED", - res.status, + wrapped.statusCode, undefined, [], ); diff --git a/packages/appkit/src/connectors/files/tests/client.test.ts b/packages/appkit/src/connectors/files/tests/client.test.ts index a47faca5a..ade45c5ed 100644 --- a/packages/appkit/src/connectors/files/tests/client.test.ts +++ b/packages/appkit/src/connectors/files/tests/client.test.ts @@ -7,30 +7,22 @@ import { ApiError } from "../../../workspace-client"; import { FilesConnector } from "../client"; import { headResponse, streamFromChunks, streamFromString } from "./utils"; -const { mockFilesApi, mockConfig, mockClient } = vi.hoisted(() => { +const { mockFilesApi, mockRequest, mockClient } = vi.hoisted(() => { const mockFilesApi = { listDirectoryContentsIter: vi.fn(), downloadFile: vi.fn(), - upload: vi.fn(), + uploadFile: vi.fn(), createDirectory: vi.fn(), deleteFile: vi.fn(), }; - const mockConfig = { - host: "https://test.databricks.com", - authenticate: vi.fn(), - }; - - const mockApiClient = { - userAgent: vi.fn(() => "@databricks/appkit/9.9.9"), - }; + const mockRequest = vi.fn(); const mockClient = { files: mockFilesApi, - config: mockConfig, - apiClient: mockApiClient, + request: mockRequest, } as unknown as WorkspaceClient; - return { mockFilesApi, mockConfig, mockClient }; + return { mockFilesApi, mockRequest, mockClient }; }); vi.mock("../../../workspace-client", async (importOriginal) => { @@ -529,68 +521,54 @@ describe("FilesConnector", () => { describe("upload()", () => { let connector: FilesConnector; - let fetchSpy: ReturnType; beforeEach(() => { vi.clearAllMocks(); connector = new FilesConnector({ defaultVolume: "/Volumes/catalog/schema/vol", }); - mockConfig.authenticate.mockResolvedValue(undefined); - fetchSpy = vi.fn().mockResolvedValue({ ok: true }); - vi.stubGlobal("fetch", fetchSpy); + mockFilesApi.uploadFile.mockResolvedValue({}); }); - afterEach(() => { - vi.unstubAllGlobals(); - }); + async function sentBody(): Promise { + const { contents } = mockFilesApi.uploadFile.mock.calls[0][0]; + return new Response(contents).text(); + } - test("handles string input", async () => { + test("wraps string input in a stream", async () => { await connector.upload(mockClient, "file.txt", "hello world"); - expect(fetchSpy).toHaveBeenCalledWith( - expect.stringContaining( - "/api/2.0/fs/files/Volumes/catalog/schema/vol/file.txt", - ), + expect(mockFilesApi.uploadFile).toHaveBeenCalledWith( expect.objectContaining({ - method: "PUT", - body: "hello world", + filePath: "/Volumes/catalog/schema/vol/file.txt", + contents: expect.any(ReadableStream), }), + { signal: undefined }, ); + expect(await sentBody()).toBe("hello world"); }); - test("handles Buffer input", async () => { - const buf = Buffer.from("buffer data"); - await connector.upload(mockClient, "file.bin", buf); - - expect(fetchSpy).toHaveBeenCalledWith( - expect.any(String), - expect.objectContaining({ - method: "PUT", - body: buf, - }), + test("wraps Buffer input in a stream", async () => { + await connector.upload( + mockClient, + "file.bin", + Buffer.from("buffer data"), ); + + expect(await sentBody()).toBe("buffer data"); }); - test("handles ReadableStream input (streams directly)", async () => { + test("passes a ReadableStream through unchanged", async () => { const stream = streamFromString("stream data"); await connector.upload(mockClient, "file.txt", stream); - expect(fetchSpy).toHaveBeenCalledWith( - expect.any(String), - expect.objectContaining({ - method: "PUT", - body: expect.any(ReadableStream), - duplex: "half", - }), - ); + expect(mockFilesApi.uploadFile.mock.calls[0][0].contents).toBe(stream); }); test("defaults overwrite to true", async () => { await connector.upload(mockClient, "file.txt", "data"); - const url = fetchSpy.mock.calls[0][0] as string; - expect(url).toContain("overwrite=true"); + expect(mockFilesApi.uploadFile.mock.calls[0][0].overwrite).toBe(true); }); test("sets overwrite=false when specified", async () => { @@ -598,57 +576,50 @@ describe("FilesConnector", () => { overwrite: false, }); - const url = fetchSpy.mock.calls[0][0] as string; - expect(url).toContain("overwrite=false"); + expect(mockFilesApi.uploadFile.mock.calls[0][0].overwrite).toBe(false); }); - test("calls config.authenticate on the headers", async () => { - await connector.upload(mockClient, "file.txt", "data"); + test("forwards the abort signal", async () => { + const signal = new AbortController().signal; + await connector.upload(mockClient, "file.txt", "data", { signal }); - expect(mockConfig.authenticate).toHaveBeenCalledWith(expect.any(Headers)); + expect(mockFilesApi.uploadFile.mock.calls[0][1]).toEqual({ signal }); }); - test("stamps the AppKit User-Agent from the SDK apiClient", async () => { - await connector.upload(mockClient, "file.txt", "data"); - - const init = fetchSpy.mock.calls[0][1] as RequestInit; - const headers = init.headers as Headers; - expect(headers.get("User-Agent")).toBe("@databricks/appkit/9.9.9"); - }); - - test("builds URL from client.config.host", async () => { - await connector.upload(mockClient, "file.txt", "data"); + test("re-wraps a modular SDK error as UPLOAD_FAILED with its status", async () => { + mockFilesApi.uploadFile.mockRejectedValue( + Object.assign(new Error("x".repeat(300)), { + httpStatusCode: 403, + code: "PERMISSION_DENIED", + }), + ); - const url = fetchSpy.mock.calls[0][0] as string; - expect(url).toMatch( - /^https:\/\/test\.databricks\.com\/api\/2\.0\/fs\/files/, + const error = await connector + .upload(mockClient, "file.txt", "data") + .catch((e: unknown) => e); + expect(error).toBeInstanceOf(ApiError); + expect((error as ApiError).errorCode).toBe("UPLOAD_FAILED"); + expect((error as ApiError).statusCode).toBe(403); + expect((error as ApiError).message).toBe( + `Upload failed: ${"x".repeat(200)}…`, ); }); - test("throws ApiError on non-ok response", async () => { - fetchSpy.mockResolvedValue({ - ok: false, - status: 403, - text: () => Promise.resolve("Forbidden"), - }); + test("rethrows non-API errors unchanged", async () => { + const boom = new Error("network down"); + mockFilesApi.uploadFile.mockRejectedValue(boom); await expect( connector.upload(mockClient, "file.txt", "data"), - ).rejects.toThrow("Upload failed: Forbidden"); - - try { - await connector.upload(mockClient, "file.txt", "data"); - } catch (error) { - expect(error).toBeInstanceOf(ApiError); - expect((error as any).statusCode).toBe(403); - } + ).rejects.toBe(boom); }); test("resolves absolute paths directly", async () => { await connector.upload(mockClient, "/Volumes/other/vol/file.txt", "data"); - const url = fetchSpy.mock.calls[0][0] as string; - expect(url).toContain("/api/2.0/fs/files/Volumes/other/vol/file.txt"); + expect(mockFilesApi.uploadFile.mock.calls[0][0].filePath).toBe( + "/Volumes/other/vol/file.txt", + ); }); }); diff --git a/packages/appkit/src/plugins/files/plugin.ts b/packages/appkit/src/plugins/files/plugin.ts index 1800b6265..35cda311f 100644 --- a/packages/appkit/src/plugins/files/plugin.ts +++ b/packages/appkit/src/plugins/files/plugin.ts @@ -1233,13 +1233,15 @@ export class FilesPlugin extends Plugin implements ToolProvider { ); const settings = this._writeSettings(mode); // The connector's `upload` resolves `getWorkspaceClient()` and - // `client.config.authenticate(headers)` synchronously inside this - // callback. When `_runWithAuth` wraps us in `runInCallerContext`, that - // chain produces user-token Authorization headers on the outgoing - // `fetch PUT`. The OBO upload-headers test pins this contract. + // calls `client.files.uploadFile(...)` inside this callback. When + // `_runWithAuth` wraps us in `runInCallerContext`, that chain sends the + // PUT on the user-token client. The OBO upload identity test pins this + // contract. const result = await this.trackWrite(() => - this.execute(async () => { - await connector.upload(getWorkspaceClient(), path, webStream); + this.execute(async (signal) => { + await connector.upload(getWorkspaceClient(), path, webStream, { + signal, + }); return { success: true as const }; }, settings), ); diff --git a/packages/appkit/src/plugins/files/tests/delete.test.ts b/packages/appkit/src/plugins/files/tests/delete.test.ts index a87c45987..504b049e9 100644 --- a/packages/appkit/src/plugins/files/tests/delete.test.ts +++ b/packages/appkit/src/plugins/files/tests/delete.test.ts @@ -31,7 +31,7 @@ describe("FilesPlugin delete", () => { responses: { "files.listDirectoryContentsIter": undefined, "files.downloadFile": undefined, - "files.upload": undefined, + "files.uploadFile": undefined, "files.createDirectory": undefined, "files.deleteFile": undefined, }, diff --git a/packages/appkit/src/plugins/files/tests/download-endpoint.test.ts b/packages/appkit/src/plugins/files/tests/download-endpoint.test.ts index f166bface..d5a217fda 100644 --- a/packages/appkit/src/plugins/files/tests/download-endpoint.test.ts +++ b/packages/appkit/src/plugins/files/tests/download-endpoint.test.ts @@ -30,7 +30,7 @@ describe("FilesPlugin download endpoint Content-Disposition", () => { responses: { "files.listDirectoryContentsIter": undefined, "files.downloadFile": undefined, - "files.upload": undefined, + "files.uploadFile": undefined, "files.createDirectory": undefined, "files.deleteFile": undefined, }, diff --git a/packages/appkit/src/plugins/files/tests/error-handling.test.ts b/packages/appkit/src/plugins/files/tests/error-handling.test.ts index 1c5652327..20fd722fb 100644 --- a/packages/appkit/src/plugins/files/tests/error-handling.test.ts +++ b/packages/appkit/src/plugins/files/tests/error-handling.test.ts @@ -30,7 +30,7 @@ describe("FilesPlugin error handling", () => { responses: { "files.listDirectoryContentsIter": undefined, "files.downloadFile": undefined, - "files.upload": undefined, + "files.uploadFile": undefined, "files.createDirectory": undefined, "files.deleteFile": undefined, }, diff --git a/packages/appkit/src/plugins/files/tests/path-validation.test.ts b/packages/appkit/src/plugins/files/tests/path-validation.test.ts index c5dd40a79..79229e2ee 100644 --- a/packages/appkit/src/plugins/files/tests/path-validation.test.ts +++ b/packages/appkit/src/plugins/files/tests/path-validation.test.ts @@ -30,7 +30,7 @@ describe("FilesPlugin path validation", () => { responses: { "files.listDirectoryContentsIter": undefined, "files.downloadFile": undefined, - "files.upload": undefined, + "files.uploadFile": undefined, "files.createDirectory": undefined, "files.deleteFile": undefined, }, @@ -46,7 +46,7 @@ describe("FilesPlugin path validation", () => { // returns 400 β€” the status assertion alone wouldn't catch that. function expectNoSdkCall() { expect(getMock(client, "files.downloadFile")).not.toHaveBeenCalled(); - expect(getMock(client, "files.upload")).not.toHaveBeenCalled(); + expect(getMock(client, "files.uploadFile")).not.toHaveBeenCalled(); expect(getMock(client, "files.deleteFile")).not.toHaveBeenCalled(); expect(getMock(client, "files.createDirectory")).not.toHaveBeenCalled(); expect(getMock(client, "request")).not.toHaveBeenCalled(); diff --git a/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts b/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts index b60fdbfc7..ffd91cde4 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts @@ -26,7 +26,7 @@ const { mockFilesApi, mockSdkClient } = vi.hoisted(() => { const mockFilesApi = { listDirectoryContentsIter: vi.fn(), downloadFile: vi.fn(), - upload: vi.fn(), + uploadFile: vi.fn(), createDirectory: vi.fn(), deleteFile: vi.fn(), }; @@ -104,7 +104,7 @@ describe("Files Plugin Integration", () => { mockFilesApi.listDirectoryContentsIter.mockReset(); mockFilesApi.downloadFile.mockReset(); mockSdkClient.request.mockReset().mockResolvedValue(new Response(null)); - mockFilesApi.upload.mockReset(); + mockFilesApi.uploadFile.mockReset(); mockFilesApi.createDirectory.mockReset(); mockFilesApi.deleteFile.mockReset(); }); diff --git a/packages/appkit/src/plugins/files/tests/plugin.test.ts b/packages/appkit/src/plugins/files/tests/plugin.test.ts index f11c96233..0767bf053 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.test.ts @@ -23,17 +23,14 @@ const { mockClient, MockApiError } = await vi.hoisted(async () => { const mockFilesApi = { listDirectoryContentsIter: vi.fn(), downloadFile: vi.fn(), - upload: vi.fn(), + uploadFile: vi.fn(), createDirectory: vi.fn(), deleteFile: vi.fn(), }; const mockClient = { files: mockFilesApi, - config: { - host: "https://test.databricks.com", - authenticate: vi.fn(), - }, + request: vi.fn(async () => new Response(null)), }; class MockApiError extends Error { @@ -2659,7 +2656,7 @@ describe("FilesPlugin", () => { * while calls outside the user context fall back to the * service-principal client from `ServiceContext.get()`. Required to * exercise the real `_runWithAuth β†’ runInUserContext β†’ getWorkspaceClient - * β†’ client.config.authenticate β†’ fetch headers` chain. + * β†’ client.files.uploadFile` chain. */ async function useRealGetWorkspaceClient() { const actual = @@ -2690,49 +2687,24 @@ describe("FilesPlugin", () => { }); /** - * NON-NEGOTIABLE upload-headers contract. + * NON-NEGOTIABLE upload identity contract. * - * `_handleUpload` does a hand-rolled `fetch PUT` (not a typed SDK call) - * via the connector's `upload()`. Inside `_runWithAuth` on an OBO volume, - * the chain is: - * - * getWorkspaceClient() β†’ user-token WorkspaceClient - * client.config.authenticate(h) β†’ injects "Bearer " - * fetch(url, { headers }) β†’ outgoing request as the user - * - * This test pins that chain end-to-end. If any future SDK upgrade or - * refactor changes `client.config.authenticate`'s signature, removes the - * `runInUserContext` wrap from `_handleUpload`, or rewires - * `getWorkspaceClient()` so it returns the SP client inside the OBO - * scope, the user-token Authorization header will not reach `fetch` and - * this assertion fails. SP-token would silently leak to UC otherwise. + * `_handleUpload` calls the connector's `upload()`, which sends + * `client.files.uploadFile(...)` on whatever `getWorkspaceClient()` + * returns. Inside `_runWithAuth` on an OBO volume that must be the + * user-token client (PAT auth for the user's token). If a refactor removes + * the `runInUserContext` wrap or rewires `getWorkspaceClient()` to the SP + * client inside the OBO scope, the PUT goes out on the SP client and this + * test fails. SP-token would silently leak to UC otherwise. */ - test("OBO upload: outgoing fetch PUT carries user-token Authorization header (not SP)", async () => { + test("OBO upload: outgoing PUT goes through the user-token client (not SP)", async () => { await useRealGetCurrentPrincipalId(); await useRealGetWorkspaceClient(); - // SP-token marker β€” what the existing mockClient would inject if the - // OBO wrap leaked. We assert this NEVER reaches the outgoing fetch. - mockClient.config.authenticate.mockImplementation( - async (headers: Headers) => { - headers.set("Authorization", "Bearer SP-TOKEN"); - }, - ); - - // User-token marker β€” what the OBO scope MUST inject. const userClient = { - config: { - host: "https://test.databricks.com", - authenticate: vi.fn(async (headers: Headers) => { - headers.set("Authorization", "Bearer USER-TOKEN-FOO"); - }), - }, // `_handleUpload` only routes through the connector's `upload()`, - // which uses host + authenticate + apiClient.userAgent() + fetch. No - // `files.*` accessor is touched on the user client during this path. - apiClient: { - userAgent: vi.fn(() => "@databricks/appkit/9.9.9"), - }, + // which makes one `files.uploadFile` call. + files: { uploadFile: vi.fn(async () => ({})) }, }; // Wire `_buildUserContextOrNull β†’ ServiceContext.createUserContext` to @@ -2747,12 +2719,6 @@ describe("FilesPlugin", () => { }), ); - // Capture the outgoing PUT. - const fetchSpy = vi - .fn() - .mockResolvedValue({ ok: true, status: 200, text: async () => "" }); - vi.stubGlobal("fetch", fetchSpy); - const plugin = new FilesPlugin({ volumes: { obo_vol: { @@ -2779,31 +2745,22 @@ describe("FilesPlugin", () => { res, ); - // The user-token authenticator was consulted exactly when upload ran. - expect(userClient.config.authenticate).toHaveBeenCalledTimes(1); - - // The hand-rolled fetch PUT happened exactly once. - expect(fetchSpy).toHaveBeenCalledTimes(1); - const fetchArgs = fetchSpy.mock.calls[0]; - const init = fetchArgs[1] as RequestInit & { headers: Headers }; - expect(init.method).toBe("PUT"); - - // The contract β€” proves the user-token Authorization header reached - // fetch. Toggling the `_runWithAuth` wrap off in `_handleUpload` - // breaks this assertion (fetch would carry "Bearer SP-TOKEN" instead). - expect(init.headers.get("Authorization")).toBe("Bearer USER-TOKEN-FOO"); - expect(init.headers.get("Authorization")).not.toBe("Bearer SP-TOKEN"); + // The PUT happened exactly once, on the user-token client. + expect(userClient.files.uploadFile).toHaveBeenCalledTimes(1); + expect(userClient.files.uploadFile).toHaveBeenCalledWith( + expect.objectContaining({ + filePath: expect.stringContaining("/Volumes/"), + }), + expect.anything(), + ); - // Defense-in-depth: SP authenticator was NOT called along the OBO path. - expect(mockClient.config.authenticate).not.toHaveBeenCalled(); + // The contract: the SP client never sent it. + expect(mockClient.files.uploadFile).not.toHaveBeenCalled(); }); test("OBO upload + missing token + NODE_ENV=production β†’ 401 before any SDK or fetch call", async () => { process.env.NODE_ENV = "production"; - const fetchSpy = vi.fn(); - vi.stubGlobal("fetch", fetchSpy); - const plugin = new FilesPlugin({ volumes: { obo_vol: { @@ -2839,9 +2796,8 @@ describe("FilesPlugin", () => { plugin: "files", }); - // Neither the SDK upload nor the hand-rolled fetch ran. - expect(mockClient.files.upload).not.toHaveBeenCalled(); - expect(fetchSpy).not.toHaveBeenCalled(); + // The SDK upload never ran. + expect(mockClient.files.uploadFile).not.toHaveBeenCalled(); }); test("OBO mkdir + policy denies β†’ 403 PolicyDeniedError; SDK not invoked", async () => { @@ -2856,9 +2812,6 @@ describe("FilesPlugin", () => { const handler = getRouteHandler(plugin, "post", "/mkdir"); const res = mockRes(); - const fetchSpy = vi.fn(); - vi.stubGlobal("fetch", fetchSpy); - await handler( mockReq( "obo_vol", @@ -2888,9 +2841,9 @@ describe("FilesPlugin", () => { }), ); - // SDK + fetch not invoked. + // SDK + raw request not invoked. expect(mockClient.files.createDirectory).not.toHaveBeenCalled(); - expect(fetchSpy).not.toHaveBeenCalled(); + expect(mockClient.request).not.toHaveBeenCalled(); }); test("OBO delete + valid token + UC denies β†’ user-token client invoked, error propagated", async () => { @@ -3220,13 +3173,6 @@ describe("FilesPlugin", () => { } }, ); - mockClient.config.authenticate.mockImplementation(async (h: Headers) => { - h.set("Authorization", "Bearer ALICE"); - }); - const fetchSpy = vi - .fn() - .mockResolvedValue({ ok: true, status: 200, text: async () => "" }); - vi.stubGlobal("fetch", fetchSpy); // Bob's first list β€” empty. const bobRes1 = mockRes(); @@ -3336,10 +3282,6 @@ describe("FilesPlugin", () => { yield { name: "user.txt", path: "/user.txt", is_directory: false }; }); const userClient = { - config: { - host: "https://test.databricks.com", - authenticate: vi.fn(), - }, files: { listDirectoryContentsIter: userListSpy }, }; diff --git a/packages/appkit/src/plugins/files/tests/raw-endpoint.test.ts b/packages/appkit/src/plugins/files/tests/raw-endpoint.test.ts index a288dbb27..9eb91381b 100644 --- a/packages/appkit/src/plugins/files/tests/raw-endpoint.test.ts +++ b/packages/appkit/src/plugins/files/tests/raw-endpoint.test.ts @@ -30,7 +30,7 @@ describe("FilesPlugin raw endpoint security headers", () => { responses: { "files.listDirectoryContentsIter": undefined, "files.downloadFile": undefined, - "files.upload": undefined, + "files.uploadFile": undefined, "files.createDirectory": undefined, "files.deleteFile": undefined, }, diff --git a/packages/appkit/src/plugins/files/tests/shutdown.test.ts b/packages/appkit/src/plugins/files/tests/shutdown.test.ts index d987336cf..61a96948e 100644 --- a/packages/appkit/src/plugins/files/tests/shutdown.test.ts +++ b/packages/appkit/src/plugins/files/tests/shutdown.test.ts @@ -18,7 +18,7 @@ describe("FilesPlugin shutdown and trackWrite", () => { responses: { "files.listDirectoryContentsIter": undefined, "files.downloadFile": undefined, - "files.upload": undefined, + "files.uploadFile": undefined, "files.createDirectory": undefined, "files.deleteFile": undefined, }, diff --git a/packages/appkit/src/plugins/files/tests/upload.test.ts b/packages/appkit/src/plugins/files/tests/upload.test.ts index c483a59ca..42477ea7d 100644 --- a/packages/appkit/src/plugins/files/tests/upload.test.ts +++ b/packages/appkit/src/plugins/files/tests/upload.test.ts @@ -26,7 +26,7 @@ describe("FilesPlugin upload", () => { responses: { "files.listDirectoryContentsIter": undefined, "files.downloadFile": undefined, - "files.upload": undefined, + "files.uploadFile": undefined, "files.createDirectory": undefined, "files.deleteFile": undefined, }, diff --git a/packages/appkit/src/plugins/files/tests/volume-config.test.ts b/packages/appkit/src/plugins/files/tests/volume-config.test.ts index c1250a6d9..42bfe2651 100644 --- a/packages/appkit/src/plugins/files/tests/volume-config.test.ts +++ b/packages/appkit/src/plugins/files/tests/volume-config.test.ts @@ -19,7 +19,7 @@ describe("FilesPlugin volume config surface", () => { responses: { "files.listDirectoryContentsIter": undefined, "files.downloadFile": undefined, - "files.upload": undefined, + "files.uploadFile": undefined, "files.createDirectory": undefined, "files.deleteFile": undefined, }, From f770294db13720561d6928ef1ca9e54a8aecb051 Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Thu, 15 Oct 2026 17:00:53 +0200 Subject: [PATCH 5/6] fix(appkit): encode file path segments in files metadata HEAD request metadata() sent the raw resolved path through client.request(), whose new URL(path, host) parses '#' and '?' in a file name as fragment/query. 'report#1.csv' hit '.../report', so exists() could report a different file. Encode each segment like the typed SDK calls do. Co-authored-by: Isaac Signed-off-by: MarioCadenas --- .../appkit/src/connectors/files/client.ts | 8 ++++- .../src/connectors/files/tests/client.test.ts | 35 +++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/packages/appkit/src/connectors/files/client.ts b/packages/appkit/src/connectors/files/client.ts index 0d7c16631..087767add 100644 --- a/packages/appkit/src/connectors/files/client.ts +++ b/packages/appkit/src/connectors/files/client.ts @@ -320,9 +320,15 @@ export class FilesConnector { // response headers that carry the metadata, so read them off a raw HEAD. // TODO: switch to `client.files.getFileMetadata` once sdk-files builds the // response from the HEAD headers (as `downloadFile` already does). + // Encode each segment so `#`, `?` and `%` in a name aren't parsed as URL + // syntax by the raw seam (the typed SDK calls do the same). + const encodedPath = resolvedPath + .split("/") + .map(encodeURIComponent) + .join("/"); const res = await client.request({ method: "HEAD", - path: `/api/2.0/fs/files${resolvedPath}`, + path: `/api/2.0/fs/files${encodedPath}`, }); const contentLength = res.headers.get("content-length"); return { diff --git a/packages/appkit/src/connectors/files/tests/client.test.ts b/packages/appkit/src/connectors/files/tests/client.test.ts index ade45c5ed..fa608f319 100644 --- a/packages/appkit/src/connectors/files/tests/client.test.ts +++ b/packages/appkit/src/connectors/files/tests/client.test.ts @@ -421,6 +421,25 @@ describe("FilesConnector", () => { "Network failure", ); }); + + test("does not resolve a different file for a name containing '#'", async () => { + // Mirror the raw seam's `new URL(path, host)`: only `.../report` exists. + mockRequest.mockImplementation(async ({ path }: { path: string }) => { + const { pathname } = new URL(path, "https://host"); + if ( + pathname === "/api/2.0/fs/files/Volumes/catalog/schema/vol/report" + ) { + return headResponse({ "content-length": 1 }); + } + throw createApiError({ + message: "Not found", + errorCode: "NOT_FOUND", + statusCode: 404, + }); + }); + + expect(await connector.exists(mockClient, "report#1.csv")).toBe(false); + }); }); describe("metadata()", () => { @@ -496,6 +515,22 @@ describe("FilesConnector", () => { }); }); + test.each([ + ["report#1.csv", "report%231.csv"], + ["report?final.csv", "report%3Ffinal.csv"], + ["100%25.csv", "100%2525.csv"], + ["Invoice #123.pdf", "Invoice%20%23123.pdf"], + ])("encodes path segments: %s", async (name, encoded) => { + mockRequest.mockResolvedValue(headResponse({ "content-length": 1 })); + + await connector.metadata(mockClient, name); + + expect(mockRequest).toHaveBeenCalledWith({ + method: "HEAD", + path: `/api/2.0/fs/files/Volumes/catalog/schema/vol/${encoded}`, + }); + }); + // Regression: the modular SDK's getFileMetadata drops HEAD response // headers, so metadata must come from the raw HEAD response headers. test("reads metadata from the HEAD response headers", async () => { From 84fa5599c9f3fd14e68c52254fd103369ba9a67f Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Fri, 16 Oct 2026 16:42:18 +0200 Subject: [PATCH 6/6] fix(appkit): upload in-memory files as bytes so they keep Content-Length Buffer and string uploads were wrapped as a ReadableStream for uploadFile, which sends them chunked with no Content-Length and makes them impossible to retry. Send in-memory contents as bytes through the request seam instead; real streams still go through uploadFile. Co-authored-by: Isaac Signed-off-by: MarioCadenas --- .../appkit/src/connectors/files/client.ts | 42 ++++++--- .../src/connectors/files/tests/client.test.ts | 92 ++++++++++++------- 2 files changed, 88 insertions(+), 46 deletions(-) diff --git a/packages/appkit/src/connectors/files/client.ts b/packages/appkit/src/connectors/files/client.ts index 087767add..af0480309 100644 --- a/packages/appkit/src/connectors/files/client.ts +++ b/packages/appkit/src/connectors/files/client.ts @@ -353,20 +353,36 @@ export class FilesConnector { const resolvedPath = this.resolvePath(filePath); return this.traced("upload", { "files.path": resolvedPath }, async () => { - // `uploadFile` takes only a ReadableStream body. - const body = - contents instanceof ReadableStream - ? contents - : new Blob([contents]).stream(); + const overwrite = options?.overwrite ?? true; try { - await client.files.uploadFile( - { - filePath: resolvedPath, - contents: body, - overwrite: options?.overwrite ?? true, - }, - { signal: options?.signal }, - ); + if (contents instanceof ReadableStream) { + await client.files.uploadFile( + { filePath: resolvedPath, contents, overwrite }, + { signal: options?.signal }, + ); + } else { + // `uploadFile` takes only a ReadableStream, which goes out chunked + // and can't be retried. Send in-memory contents as bytes instead, so + // the request carries Content-Length and the transport can retry it. + const encodedPath = resolvedPath + .split("/") + .map(encodeURIComponent) + .join("/"); + const res = await client.request({ + method: "PUT", + path: `/api/2.0/fs/files${encodedPath}`, + query: { overwrite: String(overwrite) }, + headers: new Headers({ + "Content-Type": "application/octet-stream", + }), + body: + typeof contents === "string" + ? new TextEncoder().encode(contents) + : new Uint8Array(contents), + signal: options?.signal, + }); + await res.body?.cancel(); + } } catch (e) { const wrapped = toWrapperApiError(e); if (!(wrapped instanceof ApiError)) throw e; diff --git a/packages/appkit/src/connectors/files/tests/client.test.ts b/packages/appkit/src/connectors/files/tests/client.test.ts index fa608f319..e44330c5e 100644 --- a/packages/appkit/src/connectors/files/tests/client.test.ts +++ b/packages/appkit/src/connectors/files/tests/client.test.ts @@ -563,46 +563,65 @@ describe("FilesConnector", () => { defaultVolume: "/Volumes/catalog/schema/vol", }); mockFilesApi.uploadFile.mockResolvedValue({}); - }); - - async function sentBody(): Promise { - const { contents } = mockFilesApi.uploadFile.mock.calls[0][0]; - return new Response(contents).text(); + mockRequest.mockResolvedValue(new Response(null, { status: 204 })); + }); + + /** The single PUT the in-memory path sent through the request seam. */ + function sentPut() { + expect(mockRequest).toHaveBeenCalledTimes(1); + return mockRequest.mock.calls[0][0] as { + method: string; + path: string; + query: Record; + body: Uint8Array; + signal?: AbortSignal; + }; } - test("wraps string input in a stream", async () => { + test("sends string input as bytes (known length, retryable) via the request seam", async () => { await connector.upload(mockClient, "file.txt", "hello world"); - expect(mockFilesApi.uploadFile).toHaveBeenCalledWith( - expect.objectContaining({ - filePath: "/Volumes/catalog/schema/vol/file.txt", - contents: expect.any(ReadableStream), - }), - { signal: undefined }, + const put = sentPut(); + expect(put.method).toBe("PUT"); + expect(put.path).toBe( + "/api/2.0/fs/files/Volumes/catalog/schema/vol/file.txt", ); - expect(await sentBody()).toBe("hello world"); + expect(put.body).toBeInstanceOf(Uint8Array); + expect(new TextDecoder().decode(put.body)).toBe("hello world"); + expect(mockFilesApi.uploadFile).not.toHaveBeenCalled(); }); - test("wraps Buffer input in a stream", async () => { + test("sends Buffer input as bytes", async () => { await connector.upload( mockClient, "file.bin", Buffer.from("buffer data"), ); - expect(await sentBody()).toBe("buffer data"); + expect(new TextDecoder().decode(sentPut().body)).toBe("buffer data"); + }); + + test("encodes path segments so # and ? stay in the file name", async () => { + await connector.upload(mockClient, "report#1?.csv", "data"); + + expect(sentPut().path).toBe( + "/api/2.0/fs/files/Volumes/catalog/schema/vol/report%231%3F.csv", + ); }); - test("passes a ReadableStream through unchanged", async () => { + test("passes a ReadableStream through unchanged to uploadFile", async () => { const stream = streamFromString("stream data"); await connector.upload(mockClient, "file.txt", stream); expect(mockFilesApi.uploadFile.mock.calls[0][0].contents).toBe(stream); + expect(mockRequest).not.toHaveBeenCalled(); }); test("defaults overwrite to true", async () => { await connector.upload(mockClient, "file.txt", "data"); + expect(sentPut().query).toEqual({ overwrite: "true" }); + await connector.upload(mockClient, "s.txt", streamFromString("s")); expect(mockFilesApi.uploadFile.mock.calls[0][0].overwrite).toBe(true); }); @@ -611,38 +630,45 @@ describe("FilesConnector", () => { overwrite: false, }); - expect(mockFilesApi.uploadFile.mock.calls[0][0].overwrite).toBe(false); + expect(sentPut().query).toEqual({ overwrite: "false" }); }); test("forwards the abort signal", async () => { const signal = new AbortController().signal; await connector.upload(mockClient, "file.txt", "data", { signal }); + expect(sentPut().signal).toBe(signal); + await connector.upload(mockClient, "s.txt", streamFromString("s"), { + signal, + }); expect(mockFilesApi.uploadFile.mock.calls[0][1]).toEqual({ signal }); }); test("re-wraps a modular SDK error as UPLOAD_FAILED with its status", async () => { - mockFilesApi.uploadFile.mockRejectedValue( + const apiError = () => Object.assign(new Error("x".repeat(300)), { httpStatusCode: 403, code: "PERMISSION_DENIED", - }), - ); - - const error = await connector - .upload(mockClient, "file.txt", "data") - .catch((e: unknown) => e); - expect(error).toBeInstanceOf(ApiError); - expect((error as ApiError).errorCode).toBe("UPLOAD_FAILED"); - expect((error as ApiError).statusCode).toBe(403); - expect((error as ApiError).message).toBe( - `Upload failed: ${"x".repeat(200)}…`, - ); + }); + mockRequest.mockRejectedValue(apiError()); + mockFilesApi.uploadFile.mockRejectedValue(apiError()); + + for (const contents of ["data", streamFromString("s")]) { + const error = await connector + .upload(mockClient, "file.txt", contents) + .catch((e: unknown) => e); + expect(error).toBeInstanceOf(ApiError); + expect((error as ApiError).errorCode).toBe("UPLOAD_FAILED"); + expect((error as ApiError).statusCode).toBe(403); + expect((error as ApiError).message).toBe( + `Upload failed: ${"x".repeat(200)}…`, + ); + } }); test("rethrows non-API errors unchanged", async () => { const boom = new Error("network down"); - mockFilesApi.uploadFile.mockRejectedValue(boom); + mockRequest.mockRejectedValue(boom); await expect( connector.upload(mockClient, "file.txt", "data"), @@ -652,8 +678,8 @@ describe("FilesConnector", () => { test("resolves absolute paths directly", async () => { await connector.upload(mockClient, "/Volumes/other/vol/file.txt", "data"); - expect(mockFilesApi.uploadFile.mock.calls[0][0].filePath).toBe( - "/Volumes/other/vol/file.txt", + expect(sentPut().path).toBe( + "/api/2.0/fs/files/Volumes/other/vol/file.txt", ); }); });