diff --git a/src/commands/app/uninstall.test.ts b/src/commands/app/uninstall.test.ts new file mode 100644 index 000000000..8c66effa0 --- /dev/null +++ b/src/commands/app/uninstall.test.ts @@ -0,0 +1,60 @@ +import { describe, expect, it } from "@jest/globals"; +import { mapUninstallError } from "./uninstall.js"; + +function apiErrorLike(options: { + message?: string; + status: number; + type?: string; +}) { + return { + response: { + data: { + message: options.message, + type: options.type, + }, + status: options.status, + }, + }; +} + +describe("mapUninstallError", () => { + it("maps matching HTTP 412 primary-database preconditions", () => { + const original = apiErrorLike({ + status: 412, + type: "PreconditionFailed", + message: "App has a linked primary database", + }); + const mapped = mapUninstallError(original); + + expect(mapped).toBeInstanceOf(Error); + expect((mapped as Error).message).toContain("cannot uninstall"); + expect((mapped as Error).message).toContain("primary database"); + // The original error is preserved as the cause. + expect((mapped as Error).cause).toBe(original); + }); + + it("passes through unrelated HTTP 412 errors unchanged", () => { + const original = apiErrorLike({ + status: 412, + type: "PreconditionFailed", + message: "Some other precondition failed", + }); + const mapped = mapUninstallError(original); + + expect(mapped).toBe(original); + }); + + it("passes through non-412 API errors unchanged", () => { + const original = apiErrorLike({ status: 409, type: "Conflict" }); + const mapped = mapUninstallError(original); + + expect(mapped).toBe(original); + }); + + it("passes through unrelated errors unchanged", () => { + const original = new Error("something else"); + const mapped = mapUninstallError(original); + + expect(mapped).toBe(original); + }); +}); diff --git a/src/commands/app/uninstall.ts b/src/commands/app/uninstall.ts index d25e0bdce..7e0f9652c 100644 --- a/src/commands/app/uninstall.ts +++ b/src/commands/app/uninstall.ts @@ -1,7 +1,33 @@ import { assertStatus } from "@mittwald/api-client-commons"; import { DeleteBaseCommand } from "../../lib/basecommands/DeleteBaseCommand.js"; +import { matchesAPIError } from "../../lib/error/apiError.js"; import { appInstallationArgs } from "../../lib/resources/app/flags.js"; +/** + * Maps an error raised while uninstalling an app installation to a more + * actionable one. The API answers with an HTTP 412 (Precondition Failed) when + * the app still has a linked _primary_ database. We therefore require both the + * precondition status and a matching error signature before rewriting the + * message, so unrelated 412 cases are not accidentally swallowed. + */ +export function mapUninstallError(err: unknown): unknown { + if ( + matchesAPIError(err, { + status: 412, + keywords: ["primary", "database"], + }) + ) { + return new Error( + "cannot uninstall: app has a linked primary database — " + + "unlink or repurpose it first (a primary database must be " + + "repurposed to custom or cache before it can be unlinked)", + { cause: err }, + ); + } + + return err; +} + export default class Uninstall extends DeleteBaseCommand { static description = "Uninstall an app"; static resourceName = "app installation"; @@ -12,10 +38,15 @@ export default class Uninstall extends DeleteBaseCommand { protected async deleteResource(): Promise { const appInstallationId = await this.withAppInstallationId(Uninstall); - const response = await this.apiClient.app.uninstallAppinstallation({ - appInstallationId, - }); - assertStatus(response, 204); + try { + const response = await this.apiClient.app.uninstallAppinstallation({ + appInstallationId, + }); + + assertStatus(response, 204); + } catch (err) { + throw mapUninstallError(err); + } } } diff --git a/src/lib/basecommands/DeleteBaseCommand.test.tsx b/src/lib/basecommands/DeleteBaseCommand.test.tsx new file mode 100644 index 000000000..347bab0a4 --- /dev/null +++ b/src/lib/basecommands/DeleteBaseCommand.test.tsx @@ -0,0 +1,97 @@ +import { beforeEach, describe, expect, it, jest } from "@jest/globals"; + +/* + * Regression test for the bug where a failing deletion (e.g. an app uninstall + * rejected with HTTP 412) still printed "Process completed successfully" and + * exited with code 0. The command must instead surface the error via + * process.error() and exit with a non-zero code. + */ + +const fakeStep = { + complete: jest.fn(), + error: jest.fn(), +}; + +const fakeProcess = { + start: jest.fn(), + addStep: jest.fn(() => fakeStep), + addConfirmation: jest.fn(async () => true), + addInfo: jest.fn(), + complete: jest.fn(async (_summary: unknown) => {}), + error: jest.fn(async (_err: unknown) => {}), +}; + +const makeProcessRenderer = jest.fn(() => fakeProcess); + +jest.unstable_mockModule("../../rendering/process/process_flags.js", () => ({ + __esModule: true, + makeProcessRenderer, + processFlags: {}, +})); + +const { DeleteBaseCommand } = await import("./DeleteBaseCommand.js"); + +class TestDelete extends DeleteBaseCommand { + static resourceName = "test resource"; + public shouldThrow: unknown = undefined; + + protected async deleteResource(): Promise { + if (this.shouldThrow !== undefined) { + throw this.shouldThrow; + } + } +} + +function makeInstance(): TestDelete { + const instance = Object.create(TestDelete.prototype) as TestDelete; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (instance as any).flags = { force: true }; + return instance; +} + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +async function runExec(instance: TestDelete): Promise { + try { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + await (instance as any).exec(); + return undefined; + } catch (err) { + return err; + } +} + +describe("DeleteBaseCommand", () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it("reports success and does not exit non-zero when deletion succeeds", async () => { + const instance = makeInstance(); + + const thrown = await runExec(instance); + + expect(fakeStep.complete).toHaveBeenCalled(); + expect(fakeProcess.complete).toHaveBeenCalledTimes(1); + expect(fakeProcess.error).not.toHaveBeenCalled(); + // no ux.exit() -> no thrown ExitError + expect(thrown).toBeUndefined(); + }); + + it("surfaces the error and exits non-zero when deletion fails", async () => { + const instance = makeInstance(); + const failure = new Error("boom"); + instance.shouldThrow = failure; + + const thrown = await runExec(instance); + + // Must NOT falsely report success ... + expect(fakeProcess.complete).not.toHaveBeenCalled(); + // ... must surface the actual error ... + expect(fakeStep.error).toHaveBeenCalledWith(failure); + expect(fakeProcess.error).toHaveBeenCalledWith(failure); + // ... and must exit with a non-zero code (ux.exit(1) throws an ExitError). + expect(thrown).toBeDefined(); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + expect((thrown as any).oclif?.exit).toBe(1); + }); +}); diff --git a/src/lib/basecommands/DeleteBaseCommand.tsx b/src/lib/basecommands/DeleteBaseCommand.tsx index 129d2c3d6..8215b5ad8 100644 --- a/src/lib/basecommands/DeleteBaseCommand.tsx +++ b/src/lib/basecommands/DeleteBaseCommand.tsx @@ -7,7 +7,6 @@ import { processFlags, } from "../../rendering/process/process_flags.js"; import { Success } from "../../rendering/react/components/Success.js"; -import { Text } from "ink"; import React from "react"; export abstract class DeleteBaseCommand< @@ -56,7 +55,7 @@ export abstract class DeleteBaseCommand< ); } catch (err) { deletingStep.error(err); - process.complete(Failed to delete {resourceName}); + await process.error(err); ux.exit(1); } } diff --git a/src/lib/error/apiError.test.ts b/src/lib/error/apiError.test.ts new file mode 100644 index 000000000..3aa5d50e5 --- /dev/null +++ b/src/lib/error/apiError.test.ts @@ -0,0 +1,74 @@ +import { describe, expect, it } from "@jest/globals"; +import { getAPIErrorDetails, matchesAPIError } from "./apiError.js"; + +function apiErrorLike(options: { + message?: string; + status: number; + type?: string; +}) { + return { + response: { + data: { + message: options.message, + type: options.type, + }, + status: options.status, + }, + }; +} + +describe("getAPIErrorDetails", () => { + it("extracts status and body from API-like errors", () => { + const details = getAPIErrorDetails( + apiErrorLike({ + status: 412, + type: "PreconditionFailed", + message: "primary database is linked", + }), + ); + + expect(details).toEqual({ + status: 412, + body: { + type: "PreconditionFailed", + message: "primary database is linked", + }, + }); + }); + + it("returns null for non-API errors", () => { + expect(getAPIErrorDetails(new Error("no response"))).toBeNull(); + }); +}); + +describe("matchesAPIError", () => { + it("matches by status and keywords", () => { + const err = apiErrorLike({ + status: 412, + type: "PreconditionFailed", + message: "App has linked primary database", + }); + + expect( + matchesAPIError(err, { + status: 412, + keywords: ["primary", "database"], + }), + ).toBe(true); + }); + + it("does not match when keywords are missing", () => { + const err = apiErrorLike({ + status: 412, + type: "PreconditionFailed", + message: "Some other precondition failed", + }); + + expect( + matchesAPIError(err, { + status: 412, + keywords: ["primary", "database"], + }), + ).toBe(false); + }); +}); diff --git a/src/lib/error/apiError.ts b/src/lib/error/apiError.ts new file mode 100644 index 000000000..d3e0c1ab4 --- /dev/null +++ b/src/lib/error/apiError.ts @@ -0,0 +1,66 @@ +interface APIErrorBody { + message?: string; + type?: string; +} + +interface APIErrorLike { + response?: { + data?: unknown; + status?: number; + }; +} + +export interface APIErrorDetails { + body: APIErrorBody | undefined; + status: number; +} + +export function getAPIErrorDetails(err: unknown): APIErrorDetails | null { + if (typeof err !== "object" || err === null) { + return null; + } + + const response = (err as APIErrorLike).response; + if (typeof response?.status !== "number") { + return null; + } + + const body = + typeof response.data === "object" && response.data !== null + ? (response.data as APIErrorBody) + : undefined; + + return { + status: response.status, + body, + }; +} + +export function matchesAPIError( + err: unknown, + options: { + keywords?: string[]; + status?: number; + }, +): boolean { + const details = getAPIErrorDetails(err); + if (details === null) { + return false; + } + + if (options.status !== undefined && details.status !== options.status) { + return false; + } + + if (!options.keywords || options.keywords.length === 0) { + return true; + } + + const signature = `${details.body?.type ?? ""} ${details.body?.message ?? ""}` + .toLowerCase() + .trim(); + + return options.keywords.every((keyword) => + signature.includes(keyword.toLowerCase()), + ); +}