From b77533728f92aa5287160c1aab3c7cd80d345df1 Mon Sep 17 00:00:00 2001 From: Xin Hao Zhang Date: Thu, 27 Jun 2024 16:17:41 -0400 Subject: [PATCH] ui: fix query fetching store ids for db/tables These queries were updated in #118904 to make them more performant. In that change we mistakenly removed the null check before reading the `rows` field, causing pages using this request to crash if this query fails. Epic: none Fixes: #126348 Release note (bug fix): Database overview and db details pages should not crash if the range information is not available. --- .../cluster-ui/src/api/databaseDetailsApi.ts | 6 ++- .../workspaces/cluster-ui/src/api/sqlApi.ts | 5 +++ .../cluster-ui/src/api/tableDetailsApi.ts | 10 +++-- .../db-console/src/util/api.spec.ts | 37 ++++++++++++++++++- .../workspaces/db-console/src/util/fakeApi.ts | 33 ++++++++++++++++- 5 files changed, 85 insertions(+), 6 deletions(-) diff --git a/pkg/ui/workspaces/cluster-ui/src/api/databaseDetailsApi.ts b/pkg/ui/workspaces/cluster-ui/src/api/databaseDetailsApi.ts index 331e393c24d7..31a5eb6b252a 100644 --- a/pkg/ui/workspaces/cluster-ui/src/api/databaseDetailsApi.ts +++ b/pkg/ui/workspaces/cluster-ui/src/api/databaseDetailsApi.ts @@ -381,8 +381,12 @@ const getDatabaseReplicasAndRegions: DatabaseDetailsQuery { if (txnResult.error) { resp.stats.replicaData.error = txnResult.error; + // We don't expect to have any rows for this query on error. + return; + } + if (!txnResultIsEmpty(txnResult)) { + resp.stats.replicaData.storeIDs = txnResult?.rows[0]?.store_ids ?? []; } - resp.stats.replicaData.storeIDs = txnResult?.rows[0]?.store_ids ?? []; }, handleMaxSizeError: (_dbName, _response, _dbDetail) => { return Promise.resolve(false); diff --git a/pkg/ui/workspaces/cluster-ui/src/api/sqlApi.ts b/pkg/ui/workspaces/cluster-ui/src/api/sqlApi.ts index 2744daa8660f..266015a615d2 100644 --- a/pkg/ui/workspaces/cluster-ui/src/api/sqlApi.ts +++ b/pkg/ui/workspaces/cluster-ui/src/api/sqlApi.ts @@ -179,6 +179,7 @@ const UPGRADE_RELATED_ERRORS = [ ]; export function isUpgradeError(message: string): boolean { + if (message == null) return false; return UPGRADE_RELATED_ERRORS.some(err => message.search(err) !== -1); } @@ -197,6 +198,10 @@ export function isUpgradeError(message: string): boolean { * @param message */ export function sqlApiErrorMessage(message: string): string { + if (!message) { + return ""; + } + if (isUpgradeError(message)) { return "This page may not be available during an upgrade."; } diff --git a/pkg/ui/workspaces/cluster-ui/src/api/tableDetailsApi.ts b/pkg/ui/workspaces/cluster-ui/src/api/tableDetailsApi.ts index 6bc5429fb81a..5801c2df480d 100644 --- a/pkg/ui/workspaces/cluster-ui/src/api/tableDetailsApi.ts +++ b/pkg/ui/workspaces/cluster-ui/src/api/tableDetailsApi.ts @@ -464,12 +464,16 @@ const getTableReplicaStoreIDs: TableDetailsQuery = { ) => { if (txnResult.error) { resp.stats.replicaData.error = txnResult.error; + // We don't expect to have any rows for this query on error. + return; } // TODO #118957 (xinhaoz) Store IDs and node IDs cannot be used interchangeably. - resp.stats.replicaData.storeIDs = txnResult?.rows[0]?.store_ids ?? []; - resp.stats.replicaData.replicaCount = - txnResult?.rows[0]?.replica_count ?? 0; + if (!txnResultIsEmpty(txnResult)) { + resp.stats.replicaData.storeIDs = txnResult?.rows[0]?.store_ids ?? []; + resp.stats.replicaData.replicaCount = + txnResult?.rows[0]?.replica_count ?? 0; + } }, }; diff --git a/pkg/ui/workspaces/db-console/src/util/api.spec.ts b/pkg/ui/workspaces/db-console/src/util/api.spec.ts index 26f2a01d4e24..363394489e32 100644 --- a/pkg/ui/workspaces/db-console/src/util/api.spec.ts +++ b/pkg/ui/workspaces/db-console/src/util/api.spec.ts @@ -19,7 +19,7 @@ import { REMOTE_DEBUGGING_ERROR_TEXT, indexUnusedDuration, } from "src/util/constants"; -import { stubSqlApiCall } from "src/util/fakeApi"; +import { mockExecSQLErrors, stubSqlApiCall } from "src/util/fakeApi"; import * as api from "./api"; import fetchMock from "./fetch-mock"; @@ -304,6 +304,20 @@ describe("rest api", function () { done(); }); }); + + it("should not error when any query fails", async () => { + const req = { database, csIndexUnusedDuration: indexUnusedDuration }; + const mockResults = mockExecSQLErrors(6); + stubSqlApiCall( + clusterUiApi.createDatabaseDetailsReq(req), + mockResults, + ); + + // This call should not throw. + const result = await clusterUiApi.getDatabaseDetails(req); + expect(result.results).toBeDefined(); + expect(result.results.error).toBeDefined(); + }); }); describe("table details request", function () { @@ -498,6 +512,27 @@ describe("rest api", function () { done(); }); }); + + it("should not error when any query fails", async () => { + const mockResults = mockExecSQLErrors(10); + stubSqlApiCall( + clusterUiApi.createTableDetailsReq( + dbName, + tableName, + indexUnusedDuration, + ), + mockResults, + ); + + // This call should not throw. + const result = await clusterUiApi.getTableDetails({ + database: dbName, + table: tableName, + csIndexUnusedDuration: indexUnusedDuration, + }); + expect(result.results).toBeDefined(); + expect(result.results.error).toBeDefined(); + }); }); describe("events request", function () { diff --git a/pkg/ui/workspaces/db-console/src/util/fakeApi.ts b/pkg/ui/workspaces/db-console/src/util/fakeApi.ts index ba67d81e3a63..1f12924efb29 100644 --- a/pkg/ui/workspaces/db-console/src/util/fakeApi.ts +++ b/pkg/ui/workspaces/db-console/src/util/fakeApi.ts @@ -11,6 +11,7 @@ import * as $protobuf from "protobufjs"; import { api as clusterUiApi } from "@cockroachlabs/cluster-ui"; import moment from "moment-timezone"; +import { SqlTxnResult } from "@cockroachlabs/cluster-ui/dist/types/api"; import { cockroach } from "src/js/protos"; import { API_PREFIX, STATUS_PREFIX } from "src/util/api"; @@ -106,7 +107,17 @@ export function stubSqlApiCall( mockTxnResults: mockSqlTxnResult[], times = 1, ) { - const response = buildSqlExecutionResponse(mockTxnResults); + const firstError = mockTxnResults.find(mock => mock.error != null)?.error; + let err: clusterUiApi.SqlExecutionErrorMessage; + if (firstError != null) { + err = { + message: firstError.message, + code: "123", + severity: "ERROR", + source: { file: "myfile.go", line: 111, function: "myFn" }, + }; + } + const response = buildSqlExecutionResponse(mockTxnResults, err); fetchMock.mock({ headers: { Accept: "application/json", @@ -185,3 +196,23 @@ function buildSqlTxnResult( error: mock.error, }; } + +const mockStmtExecErrorResponse = ( + res: Partial>, +): SqlTxnResult => ({ + statement: res?.statement ?? 1, + tag: "SELECT", + start: "2022-01-01T00:00:00Z", + end: "2022-01-01T00:00:01Z", + error: new Error("error"), + rows_affected: 0, + ...res, +}); + +export const mockExecSQLErrors = ( + statements: number, +): mockSqlTxnResult[] => { + return Array.from({ length: statements }, (_, i) => + mockStmtExecErrorResponse({ statement: i + 1 }), + ); +};