ui/cluster-ui: fix routing to statement details page

Partially addresses #68843

Previously, we used two different bases for the statement details page, which
depended on which route parameters were included. `/statements/` was used when
the app name was included in the path, and otherwise `/statement/` was used.
The database name was also optionally included in the path name, further
complicating routing to the statement details page as these optional route
params lead to the need to include all combinations of route parameters for
statement detail paths,

This commit turns all optional route parameters into query string parameters,
removing the necessity for different base paths and route param combinations.

Release note (ui change): For statement detail URLs, the app name and database
name are now query string parameters. The route to statement details is
now definitively `/statement/:implicitTxn/:statement?{queryStringParams}`.

e.g.
`statement/true/SELECT%20city%2C%20id%20FROM%20vehicles%20WHERE%20city%20%3D%20%241?database=movr&app=movr`

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

@@ -11,7 +11,6 @@
 import { cockroach } from "@cockroachlabs/crdb-protobuf-client";
 import { RequestError } from "../util";
 import { getBasePath } from "./basePath";
-import { stringify } from "querystring";
 
 interface ProtoBuilder<
   P extends ConstructorType,
@@ -30,18 +29,6 @@ export function toArrayBuffer(encodedRequest: Uint8Array): ArrayBuffer {
   );
 }
 
-// propsToQueryString is a helper function that converts a set of object
-// properties to a query string
-// - keys with null or undefined values will be skipped
-// - non-string values will be toString'd
-export function propsToQueryString(props: { [k: string]: any }) {
-  const params = new URLSearchParams();
-  Object.entries(props).forEach(
-    ([k, v]: [string, any]) => v != null && params.set(k, v.toString()),
-  );
-  return params.toString();
-}
-
 /**
  * @param RespBuilder expects protobuf stub class to build decode response;
  * @param path relative URL path for requested resource;

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

@@ -9,7 +9,8 @@
 // licenses/APL.txt.
 
 import { cockroach } from "@cockroachlabs/crdb-protobuf-client";
-import { fetchData, propsToQueryString } from "src/api";
+import { fetchData } from "src/api";
+import { propsToQueryString } from "src/util";
 
 const STATEMENTS_PATH = "/_status/statements";
 
@@ -28,7 +29,7 @@ export const getCombinedStatements = (
   const queryStr = propsToQueryString({
     start: req.start.toInt(),
     end: req.end.toInt(),
-    combined: true,
+    combined: req.combined,
   });
   return fetchData(
     cockroach.server.serverpb.StatementsResponse,

pkg/ui/workspaces/cluster-ui/src/sessions/sessionDetails.tsx

@@ -14,7 +14,7 @@ import { sessionAttr } from "src/util/constants";
 import { Helmet } from "react-helmet";
 import { Loading } from "../loading";
 import _ from "lodash";
-import { Link, RouteComponentProps, withRouter } from "react-router-dom";
+import { Link, RouteComponentProps } from "react-router-dom";
 
 import { SessionInfo } from "./sessionsTable";
 
@@ -320,7 +320,6 @@ export class SessionDetails extends React.Component<SessionDetailsProps> {
                     statementNoConstants: stmt.sql_no_constants,
                     implicitTxn: session.active_txn?.implicit,
                     app: "",
-                    search: "",
                   })}
                   onClick={() =>
                     this.props.onStatementClick && this.props.onStatementClick()

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.selectors.ts

@@ -10,6 +10,7 @@
 
 import { createSelector } from "@reduxjs/toolkit";
 import { RouteComponentProps, match as Match } from "react-router-dom";
+import { Location } from "history";
 import _ from "lodash";
 import { AppState } from "../store";
 import {
@@ -24,6 +25,7 @@ import {
   databaseAttr,
   StatementStatistics,
   statementKey,
+  queryByName,
 } from "../util";
 import { AggregateStatistics } from "../statementsTable";
 import { Fraction } from "./statementDetails";
@@ -87,12 +89,13 @@ function fractionMatching(
 
 function filterByRouterParamsPredicate(
   match: Match<any>,
+  location: Location,
   internalAppNamePrefix: string,
 ): (stat: ExecutionStatistics) => boolean {
   const statement = getMatchParamByName(match, statementAttr);
   const implicitTxn = getMatchParamByName(match, implicitTxnAttr) === "true";
-  const database = getMatchParamByName(match, databaseAttr);
-  let app = getMatchParamByName(match, appAttr);
+  const database = queryByName(location, databaseAttr);
+  let app = queryByName(location, appAttr);
 
   const filterByKeys = (stmt: ExecutionStatistics) =>
     stmt.statement === statement &&
@@ -129,15 +132,19 @@ export const selectStatement = createSelector(
     const flattened = flattenStatementStats(statements);
     const results = _.filter(
       flattened,
-      filterByRouterParamsPredicate(props.match, internalAppNamePrefix),
+      filterByRouterParamsPredicate(
+        props.match,
+        props.location,
+        internalAppNamePrefix,
+      ),
     );
     const statement = getMatchParamByName(props.match, statementAttr);
     return {
       statement,
       stats: combineStatementStats(results.map(s => s.stats)),
       byNode: coalesceNodeStats(results),
       app: _.uniq(results.map(s => s.app)),
-      database: getMatchParamByName(props.match, databaseAttr),
+      database: queryByName(props.location, databaseAttr),
       distSQL: fractionMatching(results, s => s.distSQL),
       vec: fractionMatching(results, s => s.vec),
       implicit_txn: fractionMatching(results, s => s.implicit_txn),

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx

@@ -30,11 +30,11 @@ import {
   NumericStat,
   StatementStatistics,
   stdDev,
-  getMatchParamByName,
   formatNumberForDisplay,
   calculateTotalWorkload,
   unique,
   summarize,
+  queryByName,
 } from "src/util";
 import { Loading } from "src/loading";
 import { Button } from "src/button";
@@ -389,7 +389,7 @@ export class StatementDetails extends React.Component<
   };
 
   render(): React.ReactElement {
-    const app = getMatchParamByName(this.props.match, appAttr);
+    const app = queryByName(this.props.location, appAttr);
     return (
       <div className={cx("root")}>
         <Helmet title={`Details | ${app ? `${app} App |` : ""} Statements`} />
@@ -444,7 +444,7 @@ export class StatementDetails extends React.Component<
     } = this.props.statement;
 
     if (!stats) {
-      const sourceApp = getMatchParamByName(this.props.match, appAttr);
+      const sourceApp = queryByName(this.props.location, appAttr);
       const listUrl = "/statements" + (sourceApp ? "/" + sourceApp : "");
 
       return (

pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.selectors.ts

@@ -17,7 +17,7 @@ import {
   ExecutionStatistics,
   flattenStatementStats,
   formatDate,
-  getMatchParamByName,
+  queryByName,
   statementKey,
   StatementStatistics,
   TimestampToMoment,
@@ -142,7 +142,7 @@ export const selectStatements = createSelector(
       return null;
     }
     let statements = flattenStatementStats(state.data.statements);
-    const app = getMatchParamByName(props.match, appAttr);
+    const app = queryByName(props.location, appAttr);
     const isInternal = (statement: ExecutionStatistics) =>
       statement.app.startsWith(state.data.internal_app_name_prefix);
 

pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx

@@ -36,6 +36,7 @@ import {
   calculateTotalWorkload,
   unique,
   containAny,
+  queryByName,
 } from "src/util";
 import {
   AggregateStatistics,
@@ -421,7 +422,7 @@ export class StatementsPage extends React.Component<
     const {
       statements,
       databases,
-      match,
+      location,
       lastReset,
       onDiagnosticsReportDownload,
       onStatementClick,
@@ -431,7 +432,7 @@ export class StatementsPage extends React.Component<
       nodeRegions,
       isTenant,
     } = this.props;
-    const appAttrValue = getMatchParamByName(match, appAttr);
+    const appAttrValue = queryByName(location, appAttr);
     const selectedApp = appAttrValue || "";
     const appOptions = [{ value: "All", label: "All" }];
     this.props.apps.forEach(app => appOptions.push({ value: app, label: app }));
@@ -577,12 +578,12 @@ export class StatementsPage extends React.Component<
 
   render() {
     const {
-      match,
+      location,
       refreshStatementDiagnosticsRequests,
       onActivateStatementDiagnostics,
       onDiagnosticsModalOpen,
     } = this.props;
-    const app = getMatchParamByName(match, appAttr);
+    const app = queryByName(location, appAttr);
     return (
       <div className={cx("root", "table-area")}>
         <Helmet title={app ? `${app} App | Statements` : "Statements"} />

pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTableContent.tsx

@@ -22,7 +22,13 @@ import { Dropdown } from "src/dropdown";
 import { Button } from "src/button";
 
 import { Tooltip } from "@cockroachlabs/ui-components";
-import { summarize, TimestampToMoment } from "src/util";
+import {
+  appAttr,
+  databaseAttr,
+  propsToQueryString,
+  summarize,
+  TimestampToMoment,
+} from "src/util";
 import { shortStatement } from "./statementsTable";
 import styles from "./statementsTableContent.module.scss";
 import { cockroach } from "@cockroachlabs/crdb-protobuf-client";
@@ -121,39 +127,43 @@ export const StatementTableCell = {
   ),
 };
 
-interface StatementLinkProps {
+type StatementLinkTargetProps = {
   statement: string;
   app: string;
   implicitTxn: boolean;
-  search: string;
   statementNoConstants?: string;
   database?: string;
-  onClick?: (statement: string) => void;
-}
+};
 
 // StatementLinkTarget returns the link to the relevant statement page, given
 // the input statement details.
-export const StatementLinkTarget = (props: StatementLinkProps) => {
-  let base: string;
-  if (props.app && props.app.length > 0) {
-    base = `/statements/${props.app}`;
-  } else {
-    base = `/statement`;
-  }
-  if (props.database && props.database.length > 0) {
-    base = base + `/${props.database}/${props.implicitTxn}`;
-  } else {
-    base = base + `/${props.implicitTxn}`;
-  }
+export const StatementLinkTarget = (
+  props: StatementLinkTargetProps,
+): string => {
+  const base = `/statement/${props.implicitTxn}`;
+  const linkStatement = props.statementNoConstants || props.statement;
+
+  const searchParams = propsToQueryString({
+    [databaseAttr]: props.database,
+    [appAttr]: props.app,
+  });
 
-  let linkStatement = props.statement;
-  if (props.statementNoConstants) {
-    linkStatement = props.statementNoConstants;
-  }
-  return `${base}/${encodeURIComponent(linkStatement)}`;
+  return `${base}/${encodeURIComponent(linkStatement)}?${searchParams}`;
 };
 
-export const StatementLink = (props: StatementLinkProps) => {
+interface StatementLinkProps {
+  statement: string;
+  app: string;
+  implicitTxn: boolean;
+  search: string;
+  statementNoConstants?: string;
+  database?: string;
+  onClick?: (statement: string) => void;
+}
+
+export const StatementLink = (
+  props: StatementLinkProps,
+): React.ReactElement => {
   const summary = summarize(props.statement);
   const { onClick, statement } = props;
   const onStatementClick = React.useCallback(() => {
@@ -162,8 +172,16 @@ export const StatementLink = (props: StatementLinkProps) => {
     }
   }, [onClick, statement]);
 
+  const linkProps = {
+    statement: props.statement,
+    app: props.app,
+    implicitTxn: props.implicitTxn,
+    statementNoConstants: props.statementNoConstants,
+    database: props.database,
+  };
+
   return (
-    <Link to={StatementLinkTarget(props)} onClick={onStatementClick}>
+    <Link to={StatementLinkTarget(linkProps)} onClick={onStatementClick}>
       <div>
         <Tooltip
           placement="bottom"

pkg/ui/workspaces/cluster-ui/src/util/query/query.spec.ts

@@ -9,7 +9,7 @@
 // licenses/APL.txt.
 
 import { assert } from "chai";
-import { queryToString, queryByName } from "./query";
+import { propsToQueryString, queryByName } from "./query";
 import { Location } from "history";
 
 const location: Location = {
@@ -22,11 +22,29 @@ const location: Location = {
 };
 
 describe("Query utils", () => {
-  describe("queryToString", () => {
-    it("make query to string", () => {
-      assert.equal(queryToString({ a: "test" }), "a=test");
-      assert.equal(queryToString({ a: "test", b: "test" }), "a=test&b=test");
-      assert.equal(queryToString({ a: undefined }), "a=undefined");
+  describe("propsToQueryString", () => {
+    it("creates query string from object", () => {
+      const obj = {
+        start: 100,
+        end: 200,
+        strParam: "hello",
+        bool: false,
+      };
+      const expected = "start=100&end=200&strParam=hello&bool=false";
+      const res = propsToQueryString(obj);
+      expect(res).toEqual(expected);
+    });
+
+    it("skips entries with nullish values", () => {
+      const obj = {
+        start: 100,
+        end: 200,
+        strParam: null as any,
+        hello: undefined as any,
+      };
+      const expected = "start=100&end=200";
+      const res = propsToQueryString(obj);
+      expect(res).toEqual(expected);
     });
   });
   describe("queryByName", () => {