Merge pull request #137343 from cockroachdb/blathers/backport-release…

…-24.3-136772

release-24.3: ui: use error body over status text for err messages

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",

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));
+}

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 = <P extends ProtoBuilder<P>, T extends ProtoBuilder<T>>(
+export const fetchData = async <
+  P extends ProtoBuilder<P>,
+  T extends ProtoBuilder<T>,
+>(
   respBuilder: T,
   path: string,
   reqBuilder?: P,
@@ -60,38 +63,22 @@ export const fetchData = <P extends ProtoBuilder<P>, T extends ProtoBuilder<T>>(
     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));
 };
 
 /**
  * fetchDataJSON makes a request for /api/v2 which uses content type JSON.
  * @param path relative path for requested resource.
  * @param reqPayload request payload object.
  */
-export function fetchDataJSON<ResponseType, RequestType>(
+export async function fetchDataJSON<ResponseType, RequestType>(
   path: string,
   reqPayload?: RequestType,
 ): Promise<ResponseType> {
@@ -109,15 +96,46 @@ export function fetchDataJSON<ResponseType, RequestType>(
     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<string> {
+  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();
 }

pkg/ui/workspaces/cluster-ui/src/api/schedulesApi.ts

@@ -123,19 +123,11 @@ export function getSchedule(id: Long): Promise<Schedule> {
     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 {

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",
     ),

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",
 );

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 (

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;
   }
 }