Skip to content

refactor: use HTTP status code descriptions in files plugin #258

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
atilafassina merged 3 commits into from
Apr 8, 2026
+30 −42
Merged

refactor: use HTTP status code descriptions in files plugin #258

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1b71a3b
refactor: use HTTP status code descriptions instead of hardcoded erro…
atilafassina Apr 7, 2026
971abe2
chore: dry status error responses in files plugin
atilafassina Apr 8, 2026
9770eb1
fix: standardize stream error to use _sendStatusError helper
atilafassina Apr 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 37 additions & 9 deletions packages/appkit/src/plugins/files/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { STATUS_CODES } from "node:http";
import { Readable } from "node:stream";
import { ApiError } from "@databricks/sdk-experimental";
import type express from "express";
Expand Down Expand Up @@ -448,7 +449,10 @@ export class FilesPlugin extends Plugin {
if (!result.ok) {
res
.status(result.status)
.json({ error: "List failed", plugin: this.name });
.json({
error: STATUS_CODES[result.status] ?? "Unknown Error",
plugin: this.name,
});
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The status-to-message mapping logic is duplicated across multiple handlers (list/read/exists/metadata/preview/upload/mkdir/delete). Consider extracting a small helper (e.g., this._statusDescription(status) or this._sendStatusError(res, status)) to avoid repetition and ensure consistent behavior (including what to do for unknown/non-standard status codes).

Copilot uses AI. Check for mistakes.
return;
}
res.json(result.data);
Expand Down Expand Up @@ -486,7 +490,10 @@ export class FilesPlugin extends Plugin {
if (!result.ok) {
res
.status(result.status)
.json({ error: "Read failed", plugin: this.name });
.json({
error: STATUS_CODES[result.status] ?? "Unknown Error",
plugin: this.name,
});
return;
}
res.type("text/plain").send(result.data);
Expand Down Expand Up @@ -552,7 +559,10 @@ export class FilesPlugin extends Plugin {
if (!response.ok) {
res
.status(response.status)
.json({ error: `${label} failed`, plugin: this.name });
.json({
error: STATUS_CODES[response.status] ?? "Unknown Error",
plugin: this.name,
});
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This handler now returns STATUS_CODES[response.status] for !response.ok, but there are still 500 error responses in the same flow that use hardcoded messages (e.g., the stream error event uses ${label} failed). If the goal is to standardize error strings to HTTP status descriptions, please update those remaining paths (and/or route everything through a shared error helper) so clients don’t see mixed error formats for the same status code.

Copilot uses AI. Check for mistakes.
return;
}

Expand Down Expand Up @@ -633,7 +643,10 @@ export class FilesPlugin extends Plugin {
if (!result.ok) {
res
.status(result.status)
.json({ error: "Exists check failed", plugin: this.name });
.json({
error: STATUS_CODES[result.status] ?? "Unknown Error",
plugin: this.name,
});
return;
}
res.json({ exists: result.data });
Expand Down Expand Up @@ -671,7 +684,10 @@ export class FilesPlugin extends Plugin {
if (!result.ok) {
res
.status(result.status)
.json({ error: "Metadata fetch failed", plugin: this.name });
.json({
error: STATUS_CODES[result.status] ?? "Unknown Error",
plugin: this.name,
});
return;
}
res.json(result.data);
Expand Down Expand Up @@ -709,7 +725,10 @@ export class FilesPlugin extends Plugin {
if (!result.ok) {
res
.status(result.status)
.json({ error: "Preview failed", plugin: this.name });
.json({
error: STATUS_CODES[result.status] ?? "Unknown Error",
plugin: this.name,
});
return;
}
res.json(result.data);
Expand Down Expand Up @@ -809,7 +828,10 @@ export class FilesPlugin extends Plugin {
);
res
.status(result.status)
.json({ error: "Upload failed", plugin: this.name });
.json({
error: STATUS_CODES[result.status] ?? "Unknown Error",
plugin: this.name,
});
return;
}

Expand Down Expand Up @@ -864,7 +886,10 @@ export class FilesPlugin extends Plugin {
if (!result.ok) {
res
.status(result.status)
.json({ error: "Create directory failed", plugin: this.name });
.json({
error: STATUS_CODES[result.status] ?? "Unknown Error",
plugin: this.name,
});
return;
}

Expand Down Expand Up @@ -911,7 +936,10 @@ export class FilesPlugin extends Plugin {
if (!result.ok) {
res
.status(result.status)
.json({ error: "Delete failed", plugin: this.name });
.json({
error: STATUS_CODES[result.status] ?? "Unknown Error",
plugin: this.name,
});
return;
}

Expand Down
8 changes: 4 additions & 4 deletions packages/appkit/src/plugins/files/tests/plugin.integration.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,7 +567,7 @@ describe("Files Plugin Integration", () => {

expect(response.status).toBe(500);
const data = (await response.json()) as { error: string; plugin: string };
expect(data.error).toBe("Metadata fetch failed");
expect(data.error).toBe("Internal Server Error");
expect(data.plugin).toBe("files");
});

Expand All @@ -583,7 +583,7 @@ describe("Files Plugin Integration", () => {

expect(response.status).toBe(500);
const data = (await response.json()) as { error: string; plugin: string };
expect(data.error).toBe("List failed");
expect(data.error).toBe("Internal Server Error");
expect(data.plugin).toBe("files");
});

Expand All @@ -602,7 +602,7 @@ describe("Files Plugin Integration", () => {
error: string;
plugin: string;
};
expect(data.error).toBe("Metadata fetch failed");
expect(data.error).toBe("Internal Server Error");
expect(data.plugin).toBe("files");
});

Expand All @@ -622,7 +622,7 @@ describe("Files Plugin Integration", () => {
error: string;
plugin: string;
};
expect(data.error).toBe("Create directory failed");
expect(data.error).toBe("Internal Server Error");
expect(data.plugin).toBe("files");
});
});
Expand Down
16 changes: 8 additions & 8 deletions packages/appkit/src/plugins/files/tests/plugin.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -674,7 +674,7 @@ describe("FilesPlugin", () => {
expect(res.status).toHaveBeenCalledWith(500);
expect(res.json).toHaveBeenCalledWith(
expect.objectContaining({
error: "List failed",
error: "Internal Server Error",
plugin: "files",
}),
);
Expand All @@ -698,7 +698,7 @@ describe("FilesPlugin", () => {

expect(res.status).toHaveBeenCalledWith(500);
expect(res.json).toHaveBeenCalledWith(
expect.objectContaining({ error: "Read failed" }),
expect.objectContaining({ error: "Internal Server Error" }),
);
});

Expand All @@ -722,7 +722,7 @@ describe("FilesPlugin", () => {

expect(res.status).toHaveBeenCalledWith(500);
expect(res.json).toHaveBeenCalledWith(
expect.objectContaining({ error: "Exists check failed" }),
expect.objectContaining({ error: "Internal Server Error" }),
);
});

Expand All @@ -746,7 +746,7 @@ describe("FilesPlugin", () => {

expect(res.status).toHaveBeenCalledWith(500);
expect(res.json).toHaveBeenCalledWith(
expect.objectContaining({ error: "Metadata fetch failed" }),
expect.objectContaining({ error: "Internal Server Error" }),
);
});

Expand All @@ -768,7 +768,7 @@ describe("FilesPlugin", () => {

expect(res.status).toHaveBeenCalledWith(500);
expect(res.json).toHaveBeenCalledWith(
expect.objectContaining({ error: "Download failed" }),
expect.objectContaining({ error: "Internal Server Error" }),
);
});

Expand All @@ -791,7 +791,7 @@ describe("FilesPlugin", () => {

expect(res.status).toHaveBeenCalledWith(500);
expect(res.json).toHaveBeenCalledWith(
expect.objectContaining({ error: "Create directory failed" }),
expect.objectContaining({ error: "Internal Server Error" }),
);
});

Expand Down Expand Up @@ -835,7 +835,7 @@ describe("FilesPlugin", () => {
await handlerPromise;

const errorBody = res.json.mock.calls[0][0];
expect(errorBody.error).toBe("List failed");
expect(errorBody.error).toBe("Internal Server Error");
expect(errorBody.error).not.toContain("secret");
expect(errorBody.error).not.toContain("internal");
});
Expand Down Expand Up @@ -876,7 +876,7 @@ describe("FilesPlugin", () => {
expect(signalWasAborted).toBe(true);
expect(res.status).toHaveBeenCalledWith(500);
expect(res.json).toHaveBeenCalledWith(
expect.objectContaining({ error: "List failed" }),
expect.objectContaining({ error: "Internal Server Error" }),
);
});

Expand Down