diff --git a/pkg/ui/workspaces/cluster-ui/package.json b/pkg/ui/workspaces/cluster-ui/package.json index 3de411209bf2..1b966b6bc40b 100644 --- a/pkg/ui/workspaces/cluster-ui/package.json +++ b/pkg/ui/workspaces/cluster-ui/package.json @@ -1,6 +1,6 @@ { "name": "@cockroachlabs/cluster-ui", - "version": "24.3.4", + "version": "24.3.5", "description": "Cluster UI is a library of large features shared between CockroachDB and CockroachCloud", "repository": { "type": "git", diff --git a/pkg/ui/workspaces/cluster-ui/src/api/fetchData.spec.ts b/pkg/ui/workspaces/cluster-ui/src/api/fetchData.spec.ts new file mode 100644 index 000000000000..4c1b0f5d2857 --- /dev/null +++ b/pkg/ui/workspaces/cluster-ui/src/api/fetchData.spec.ts @@ -0,0 +1,161 @@ +// Copyright 2024 The Cockroach Authors. +// +// Use of this software is governed by the CockroachDB Software License +// included in the /LICENSE file. + +import { cockroach } from "@cockroachlabs/crdb-protobuf-client"; +import fetchMock from "jest-fetch-mock"; + +import { RequestError } from "src/util"; + +import { fetchDataJSON, fetchData } from "./fetchData"; + +describe("fetchDataJSON", () => { + beforeAll(fetchMock.enableMocks); + beforeEach(() => { + fetchMock.resetMocks(); + }); + afterAll(fetchMock.disableMocks); + + it("should fetch data from APIs returning JSON", async () => { + // Mock the fetch function to return a successful response. + fetchMock.mockResponseOnce(JSON.stringify({ data: "data" }), { + status: 200, + }); + + const data = await fetchDataJSON("json"); + expect(data).toEqual({ data: "data" }); + }); + + it.each([ + { + description: "500 status with status text", + response: { status: 500, statusText: "error1" }, + body: "", + expected: new RequestError(500, "error1"), + }, + { + description: "500 status with plain text error", + response: { status: 500, headers: { "Content-Type": "text/plain" } }, + body: "error2", + expected: new RequestError(500, "error2"), + }, + { + description: "500 status with JSON error", + response: { + status: 500, + headers: { "Content-Type": "application/json" }, + }, + body: JSON.stringify({ error: "error" }), + expected: new RequestError(500, JSON.stringify({ error: "error" })), + }, + { + description: "401 status with JSON error on POST", + request: { data: "data" }, + response: { + status: 401, + headers: { "Content-Type": "application/json" }, + }, + body: JSON.stringify({ error: "jsonError2" }), + expected: new RequestError(401, JSON.stringify({ error: "jsonError2" })), + }, + ])( + "should throw error when encountering non-200 status: $description", + async ({ request, response, body, expected }) => { + fetchMock.mockResponse(() => + Promise.resolve({ + body: body as string, + init: { ...response }, + }), + ); + + try { + await fetchDataJSON("fetchJson", request); + } catch (e) { + const err = e as RequestError; + expect(err.status).toBe(response.status); + expect(err.message).toBe(expected.message); + } + }, + ); +}); + +describe("fetchData", () => { + beforeAll(fetchMock.enableMocks); + beforeEach(() => { + fetchMock.resetMocks(); + }); + afterAll(fetchMock.disableMocks); + + it("should fetch data and decode response from protobuf API", async () => { + const mockResp = uint8ArrayToString( + cockroach.server.serverpb.SettingsResponse.encode({ + key_values: { key: { value: "data" } }, + }).finish(), + ); + fetchMock.mockResponseOnce(mockResp, { + status: 200, + headers: { "Content-Type": "application/x-protobuf" }, + }); + + const data = await fetchData( + cockroach.server.serverpb.SettingsResponse, + "api/protobuf", + ); + expect(data.key_values).toEqual({ key: { value: "data" } }); + }); + + it.each([ + { + description: "500 status with protobuf error", + response: { + status: 500, + headers: { "Content-Type": "application/x-protobuf" }, + }, + body: "Error in response", + expected: new RequestError(500, "Error in response"), + }, + { + description: "401 status with protobuf error on POST", + request: {}, + response: { + status: 401, + headers: { "Content-Type": "application/x-protobuf" }, + }, + body: "Error in response", + expected: new RequestError(401, "Error in response"), + }, + ])( + "should throw error when encountering non-200 status: $description", + async ({ request, response, body, expected }) => { + const mockResp = uint8ArrayToString( + cockroach.server.serverpb.ResponseError.encode({ + error: body, + message: "Error in response", + }).finish(), + ); + + fetchMock.mockResponseOnce(mockResp, { + status: response.status, + headers: { "Content-Type": "application/x-protobuf" }, + }); + + try { + await fetchData( + cockroach.server.serverpb.SettingsResponse, + "api/protobuf", + cockroach.server.serverpb.SettingsRequest, + request, + ); + } catch (e) { + const err = e as RequestError; + expect(err.status).toBe(response.status); + expect(err.message).toBe(expected.message); + } + }, + ); +}); + +function uint8ArrayToString(uint8Array: Uint8Array): string { + return String.fromCharCode(...Array.from(uint8Array)); +} diff --git a/pkg/ui/workspaces/cluster-ui/src/api/fetchData.ts b/pkg/ui/workspaces/cluster-ui/src/api/fetchData.ts index b180c981b2d6..92066f079acd 100644 --- a/pkg/ui/workspaces/cluster-ui/src/api/fetchData.ts +++ b/pkg/ui/workspaces/cluster-ui/src/api/fetchData.ts @@ -37,7 +37,10 @@ export function toArrayBuffer(encodedRequest: Uint8Array): ArrayBuffer { * TimeoutUnit ( Hour → "H", Minute → "M", Second → "S", Millisecond → "m" ), * e.g. "1M" (1 minute), default value "30S" (30 seconds); **/ -export const fetchData =

, T extends ProtoBuilder>( +export const fetchData = async < + P extends ProtoBuilder

, + T extends ProtoBuilder, +>( respBuilder: T, path: string, reqBuilder?: P, @@ -60,30 +63,14 @@ export const fetchData =

, T extends ProtoBuilder>( params.body = toArrayBuffer(encodedRequest); } - return fetch(withBasePath(path), params) - .then(response => { - if (!response.ok) { - return response.arrayBuffer().then(buffer => { - let respError; - try { - respError = cockroach.server.serverpb.ResponseError.decode( - new Uint8Array(buffer), - ); - } catch { - respError = new cockroach.server.serverpb.ResponseError({ - error: response.statusText, - }); - } - throw new RequestError( - response.statusText, - response.status, - respError.error, - ); - }); - } - return response.arrayBuffer(); - }) - .then(buffer => respBuilder.decode(new Uint8Array(buffer))); + const response = await fetch(withBasePath(path), params); + if (!response.ok) { + const errorBody = await getErrorBodyFromResponse(response); + throw new RequestError(response.status, errorBody); + } + + const buffer = await response.arrayBuffer(); + return respBuilder.decode(new Uint8Array(buffer)); }; /** @@ -91,7 +78,7 @@ export const fetchData =

, T extends ProtoBuilder>( * @param path relative path for requested resource. * @param reqPayload request payload object. */ -export function fetchDataJSON( +export async function fetchDataJSON( path: string, reqPayload?: RequestType, ): Promise { @@ -109,15 +96,46 @@ export function fetchDataJSON( params.body = JSON.stringify(reqPayload); } - return fetch(withBasePath(path), params).then(response => { - if (!response.ok) { - throw new RequestError( - response.statusText, - response.status, - response.statusText, + const response = await fetch(withBasePath(path), params); + if (!response.ok) { + const errorBody = await getErrorBodyFromResponse(response); + throw new RequestError(response.status, errorBody); + } + return await response.json(); +} + +// getErrorBodyFromResponse attempts to extract an error message from a +// failed RPC call. It looks for this error in the following places: +// - If the response is a text or json blob, it returns them directly. +// - If the response can be deserialized to a proto Error type, it returns +// the error message field. +// - If the response has a statusText property then it is used. +// - If none of the above options were found, the status code is returned. +async function getErrorBodyFromResponse(response: Response): Promise { + const contentType = response.headers.get("Content-Type"); + try { + if (contentType.includes("application/json")) { + const json = await response.json(); + return json ? JSON.stringify(json) : response.statusText; + } + + if (contentType.includes("text/plain")) { + const errText = await response.text(); + return errText || response.statusText; + } + + if (contentType.includes("application/x-protobuf")) { + const buffer = await response.arrayBuffer(); + const error = cockroach.server.serverpb.ResponseError.decode( + new Uint8Array(buffer), ); + return error.error || response.statusText; } + } catch { + // If we can't parse the error body, we'll just return the status text. + // Note that statusText is not available in http2 responses so we'll fall + // back to status code. + } - return response.json(); - }); + return response.statusText || response.status.toString(); } diff --git a/pkg/ui/workspaces/cluster-ui/src/api/schedulesApi.ts b/pkg/ui/workspaces/cluster-ui/src/api/schedulesApi.ts index af96cae95aa8..1b8d17e2e25f 100644 --- a/pkg/ui/workspaces/cluster-ui/src/api/schedulesApi.ts +++ b/pkg/ui/workspaces/cluster-ui/src/api/schedulesApi.ts @@ -123,19 +123,11 @@ export function getSchedule(id: Long): Promise { const txnResults = result.execution.txn_results; if (txnResults.length === 0 || !txnResults[0].rows) { // No data. - throw new RequestError( - "Bad Request", - 400, - "No schedule found with this ID.", - ); + throw new RequestError(400, "No schedule found with this ID."); } if (txnResults[0].rows.length > 1) { - throw new RequestError( - "Internal Server Error", - 500, - "Multiple schedules found for ID.", - ); + throw new RequestError(500, "Multiple schedules found for ID."); } const row = txnResults[0].rows[0]; return { diff --git a/pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.fixture.ts b/pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.fixture.ts index debf0e527161..e95096ac7f1c 100644 --- a/pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.fixture.ts +++ b/pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.fixture.ts @@ -415,7 +415,6 @@ export const statementsPagePropsWithRequestError: StatementsPageProps = { lastUpdated, valid: true, error: new RequestError( - "request_error", 403, "this operation requires admin privilege", ), diff --git a/pkg/ui/workspaces/cluster-ui/src/transactionDetails/transactionDetails.fixture.ts b/pkg/ui/workspaces/cluster-ui/src/transactionDetails/transactionDetails.fixture.ts index b565aab1f9b8..b80c3650cb00 100644 --- a/pkg/ui/workspaces/cluster-ui/src/transactionDetails/transactionDetails.fixture.ts +++ b/pkg/ui/workspaces/cluster-ui/src/transactionDetails/transactionDetails.fixture.ts @@ -44,7 +44,6 @@ export const nodeRegions = { }; export const error = new RequestError( - "Forbidden", 403, "this operation requires admin privilege", ); diff --git a/pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.stories.tsx b/pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.stories.tsx index ceaf9664fb1a..9cbff9b8ab0c 100644 --- a/pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.stories.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.stories.tsx @@ -189,11 +189,7 @@ storiesOf("Transactions Page", module) inFlight: true, data: undefined, lastUpdated, - error: new RequestError( - "Forbidden", - 403, - "this operation requires admin privilege", - ), + error: new RequestError(403, "this operation requires admin privilege"), }; return ( diff --git a/pkg/ui/workspaces/cluster-ui/src/util/requestError.ts b/pkg/ui/workspaces/cluster-ui/src/util/requestError.ts index c3bf4455d101..73e4ef66eddd 100644 --- a/pkg/ui/workspaces/cluster-ui/src/util/requestError.ts +++ b/pkg/ui/workspaces/cluster-ui/src/util/requestError.ts @@ -5,11 +5,10 @@ export class RequestError extends Error { status: number; - constructor(statusText: string, status: number, message?: string) { - super(statusText); + constructor(status: number, message: string) { + super(message); this.status = status; this.name = "RequestError"; - this.message = message; } }