From f86c1949e788eb34f8eecd80981ce0bb0345afa4 Mon Sep 17 00:00:00 2001 From: Xin Hao Zhang Date: Wed, 22 Sep 2021 15:20:41 -0400 Subject: [PATCH] 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` --- .../cluster-ui/src/api/fetchData.spec.ts | 39 ------- .../cluster-ui/src/api/fetchData.ts | 13 --- .../cluster-ui/src/api/statementsApi.ts | 5 +- .../src/sessions/sessionDetails.tsx | 3 +- .../statementDetails.selectors.ts | 15 ++- .../src/statementDetails/statementDetails.tsx | 6 +- .../statementsPage.selectors.ts | 4 +- .../src/statementsPage/statementsPage.tsx | 7 +- .../statementsTableContent.tsx | 66 +++++++---- .../cluster-ui/src/util/query/query.spec.ts | 30 ++++- .../cluster-ui/src/util/query/query.ts | 26 +++-- pkg/ui/workspaces/db-console/src/app.tsx | 28 ++--- .../src/routes/RedirectToStatementDetails.tsx | 37 ++++++ .../db-console/src/util/api.spec.ts | 106 ++---------------- pkg/ui/workspaces/db-console/src/util/api.ts | 16 +-- .../db-console/src/util/query.spec.ts | 99 +++++++++++++++- .../workspaces/db-console/src/util/query.ts | 24 ++-- .../src/views/statements/statementDetails.tsx | 19 ++-- 18 files changed, 286 insertions(+), 257 deletions(-) delete mode 100644 pkg/ui/workspaces/cluster-ui/src/api/fetchData.spec.ts create mode 100644 pkg/ui/workspaces/db-console/src/routes/RedirectToStatementDetails.tsx diff --git a/pkg/ui/workspaces/cluster-ui/src/api/fetchData.spec.ts b/pkg/ui/workspaces/cluster-ui/src/api/fetchData.spec.ts deleted file mode 100644 index 1bd6eab700e9..000000000000 --- a/pkg/ui/workspaces/cluster-ui/src/api/fetchData.spec.ts +++ /dev/null @@ -1,39 +0,0 @@ -// Copyright 2021 The Cockroach Authors. -// -// Use of this software is governed by the Business Source License -// included in the file licenses/BSL.txt. -// -// As of the Change Date specified in that file, in accordance with -// the Business Source License, use of this software will be governed -// by the Apache License, Version 2.0, included in the file -// licenses/APL.txt. - -import { propsToQueryString } from "./fetchData"; - -describe("fetchData functions", () => { - 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); - }); - }); -}); diff --git a/pkg/ui/workspaces/cluster-ui/src/api/fetchData.ts b/pkg/ui/workspaces/cluster-ui/src/api/fetchData.ts index b0ecc8c5c2ee..4669c4782f1d 100644 --- a/pkg/ui/workspaces/cluster-ui/src/api/fetchData.ts +++ b/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; diff --git a/pkg/ui/workspaces/cluster-ui/src/api/statementsApi.ts b/pkg/ui/workspaces/cluster-ui/src/api/statementsApi.ts index 4eff1f9acf3d..1c500dd562ba 100644 --- a/pkg/ui/workspaces/cluster-ui/src/api/statementsApi.ts +++ b/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, diff --git a/pkg/ui/workspaces/cluster-ui/src/sessions/sessionDetails.tsx b/pkg/ui/workspaces/cluster-ui/src/sessions/sessionDetails.tsx index 78ced333c249..a3f5652d0cde 100644 --- a/pkg/ui/workspaces/cluster-ui/src/sessions/sessionDetails.tsx +++ b/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 { statementNoConstants: stmt.sql_no_constants, implicitTxn: session.active_txn?.implicit, app: "", - search: "", })} onClick={() => this.props.onStatementClick && this.props.onStatementClick() diff --git a/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.selectors.ts b/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.selectors.ts index 8633cc8fb68f..79e5f9e3e07d 100644 --- a/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.selectors.ts +++ b/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, + 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,7 +132,11 @@ 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 { @@ -137,7 +144,7 @@ export const selectStatement = createSelector( 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), diff --git a/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx b/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx index d014b6f0e4ad..38c893300dca 100644 --- a/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx +++ b/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 (
@@ -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 ( diff --git a/pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.selectors.ts b/pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.selectors.ts index c77ac67b517b..a86f4dbe7664 100644 --- a/pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.selectors.ts +++ b/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); diff --git a/pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx b/pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx index f35783b68924..f3dcddfb2192 100644 --- a/pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx @@ -36,6 +36,7 @@ import { calculateTotalWorkload, unique, containAny, + queryByName, } from "src/util"; import { AggregateStatistics, @@ -433,7 +434,7 @@ export class StatementsPage extends React.Component< } = this.props; const appAttrValue = getMatchParamByName(match, appAttr); const selectedApp = appAttrValue || ""; - const appOptions = [{ value: "All", label: "All" }]; + const appOptions = [{ value: "all", label: "All" }]; this.props.apps.forEach(app => appOptions.push({ value: app, label: app })); const data = this.filteredStatementsData(); const totalWorkload = calculateTotalWorkload(data); @@ -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 (
diff --git a/pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTableContent.tsx b/pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTableContent.tsx index 175111ef2d32..789b233185a4 100644 --- a/pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTableContent.tsx +++ b/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 ( - +
{ - 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", () => { diff --git a/pkg/ui/workspaces/cluster-ui/src/util/query/query.ts b/pkg/ui/workspaces/cluster-ui/src/util/query/query.ts index 60beb417154a..3425fe0786c8 100644 --- a/pkg/ui/workspaces/cluster-ui/src/util/query/query.ts +++ b/pkg/ui/workspaces/cluster-ui/src/util/query/query.ts @@ -15,15 +15,16 @@ interface ParamsObj { [key: string]: string; } -/* eslint-disable no-prototype-builtins,no-restricted-syntax */ -export function queryToString(obj: any) { - const str = []; - for (const p in obj) { - if (obj.hasOwnProperty(p)) { - str.push(`${encodeURIComponent(p)}=${encodeURIComponent(obj[p])}`); - } - } - return str.join("&"); +// 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 }): string { + const params = new URLSearchParams(); + Object.entries(props).forEach( + ([k, v]: [string, any]) => v != null && params.set(k, v.toString()), + ); + return params.toString(); } export function queryToObj(location: Location, key: string, value: string) { @@ -44,12 +45,15 @@ export function queryToObj(location: Location, key: string, value: string) { return paramObj; } -export function queryByName(location: Location, key: string) { +export function queryByName(location: Location, key: string): string { const urlParams = new URLSearchParams(location.search); return urlParams.get(key); } -export function getMatchParamByName(match: Match, key: string) { +export function getMatchParamByName( + match: Match, + key: string, +): string | null { const param = match.params[key]; if (param) { return decodeURIComponent(param); diff --git a/pkg/ui/workspaces/db-console/src/app.tsx b/pkg/ui/workspaces/db-console/src/app.tsx index e774d5d67e52..a4a3a14177ba 100644 --- a/pkg/ui/workspaces/db-console/src/app.tsx +++ b/pkg/ui/workspaces/db-console/src/app.tsx @@ -65,6 +65,7 @@ import SessionsPage from "src/views/sessions/sessionsPage"; import SessionDetails from "src/views/sessions/sessionDetails"; import TransactionsPage from "src/views/transactions/transactionsPage"; import StatementsDiagnosticsHistoryView from "src/views/reports/containers/statementDiagnosticsHistory"; +import { RedirectToStatementDetails } from "src/routes/RedirectToStatementDetails"; import "styl/app.styl"; // NOTE: If you are adding a new path to the router, and that path contains any @@ -185,39 +186,38 @@ export const App: React.FC = (props: AppProps) => { /> - } + path={`/statements/:${appAttr}/:${databaseAttr}/:${implicitTxnAttr}/:${statementAttr}`} + render={RedirectToStatementDetails} /> } /> {/* sessions */} diff --git a/pkg/ui/workspaces/db-console/src/routes/RedirectToStatementDetails.tsx b/pkg/ui/workspaces/db-console/src/routes/RedirectToStatementDetails.tsx new file mode 100644 index 000000000000..a58057ce8e56 --- /dev/null +++ b/pkg/ui/workspaces/db-console/src/routes/RedirectToStatementDetails.tsx @@ -0,0 +1,37 @@ +// Copyright 2021 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +import React from "react"; +import { Redirect, match as Match } from "react-router-dom"; +import { StatementLinkTarget } from "@cockroachlabs/cluster-ui"; +import { getMatchParamByName } from "src/util/query"; +import { + appAttr, + databaseAttr, + implicitTxnAttr, + statementAttr, +} from "src/util/constants"; + +type Props = { + match: Match; +}; + +// RedirectToStatementDetails is designed to route old versions of StatementDetails routes +// where app and database are route params, to the new StatementDetails route. +export function RedirectToStatementDetails({ match }: Props) { + const linkProps = { + statement: getMatchParamByName(match, statementAttr), + app: getMatchParamByName(match, appAttr), + implicitTxn: getMatchParamByName(match, implicitTxnAttr) === "true", + database: getMatchParamByName(match, databaseAttr), + }; + + return ; +} 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 8e74b92f08b7..8db994343a46 100644 --- a/pkg/ui/workspaces/db-console/src/util/api.spec.ts +++ b/pkg/ui/workspaces/db-console/src/util/api.spec.ts @@ -22,100 +22,6 @@ import { REMOTE_DEBUGGING_ERROR_TEXT } from "src/util/constants"; import Severity = cockroach.util.log.Severity; describe("rest api", function() { - describe("propsToQueryString", function() { - interface PropBag { - [k: string]: string; - } - - // helper decoding function used to doublecheck querystring generation - function decodeQueryString(qs: string): PropBag { - return _.reduce( - qs.split("&"), - (memo: PropBag, v: string) => { - const [key, value] = v.split("="); - memo[decodeURIComponent(key)] = decodeURIComponent(value); - return memo; - }, - {}, - ); - } - - it("creates an appropriate querystring", function() { - const testValues: { [k: string]: any } = { - a: "testa", - b: "testb", - }; - - const querystring = api.propsToQueryString(testValues); - - assert(/a=testa/.test(querystring)); - assert(/b=testb/.test(querystring)); - assert.lengthOf(querystring.match(/=/g), 2); - assert.lengthOf(querystring.match(/&/g), 1); - assert.deepEqual(testValues, decodeQueryString(querystring)); - }); - - it("handles falsy values correctly", function() { - const testValues: { [k: string]: any } = { - // null and undefined should be ignored - undefined: undefined, - null: null, - // other values should be added - false: false, - "": "", - 0: 0, - }; - - const querystring = api.propsToQueryString(testValues); - - assert(/false=false/.test(querystring)); - assert(/0=0/.test(querystring)); - assert(/([^A-Za-z]|^)=([^A-Za-z]|$)/.test(querystring)); - assert.lengthOf(querystring.match(/=/g), 3); - assert.lengthOf(querystring.match(/&/g), 2); - assert.notOk(/undefined/.test(querystring)); - assert.notOk(/null/.test(querystring)); - assert.deepEqual( - { false: "false", "": "", 0: "0" }, - decodeQueryString(querystring), - ); - }); - - it("handles special characters", function() { - const key = "!@#$%^&*()=+-_\\|\"`'?/<>"; - const value = key - .split("") - .reverse() - .join(""); // key reversed - const testValues: { [k: string]: any } = { - [key]: value, - }; - - const querystring = api.propsToQueryString(testValues); - - assert(querystring.match(/%/g).length > (key + value).match(/%/g).length); - assert.deepEqual(testValues, decodeQueryString(querystring)); - }); - - it("handles non-string values", function() { - const testValues: { [k: string]: any } = { - boolean: true, - number: 1, - emptyObject: {}, - emptyArray: [], - objectWithProps: { a: 1, b: 2 }, - arrayWithElts: [1, 2, 3], - long: Long.fromNumber(1), - }; - - const querystring = api.propsToQueryString(testValues); - assert.deepEqual( - _.mapValues(testValues, _.toString), - decodeQueryString(querystring), - ); - }); - }); - describe("databases request", function() { afterEach(fetchMock.restore); @@ -652,7 +558,9 @@ describe("rest api", function() { response: (_url: string, requestObj: RequestInit) => { assert.isUndefined(requestObj.body); const encodedResponse = protos.cockroach.server.serverpb.ClusterResponse.encode( - { cluster_id: clusterID }, + { + cluster_id: clusterID, + }, ).finish(); return { body: api.toArrayBuffer(encodedResponse), @@ -735,7 +643,9 @@ describe("rest api", function() { response: (_url: string, requestObj: RequestInit) => { assert.isUndefined(requestObj.body); const encodedResponse = protos.cockroach.server.serverpb.MetricMetadataResponse.encode( - { metadata }, + { + metadata, + }, ).finish(); return { body: api.toArrayBuffer(encodedResponse), @@ -829,7 +739,9 @@ describe("rest api", function() { response: (_url: string, requestObj: RequestInit) => { assert.isUndefined(requestObj.body); const logsResponse = protos.cockroach.server.serverpb.LogEntriesResponse.encode( - { entries: [logEntry] }, + { + entries: [logEntry], + }, ).finish(); return { body: api.toArrayBuffer(logsResponse), diff --git a/pkg/ui/workspaces/db-console/src/util/api.ts b/pkg/ui/workspaces/db-console/src/util/api.ts index bbdab5cdef07..f1563bc1160e 100644 --- a/pkg/ui/workspaces/db-console/src/util/api.ts +++ b/pkg/ui/workspaces/db-console/src/util/api.ts @@ -17,6 +17,7 @@ import moment from "moment"; import * as protos from "src/js/protos"; import { FixLong } from "src/util/fixLong"; +import { propsToQueryString } from "src/util/query"; export type DatabasesRequestMessage = protos.cockroach.server.serverpb.DatabasesRequest; export type DatabasesResponseMessage = protos.cockroach.server.serverpb.DatabasesResponse; @@ -254,19 +255,6 @@ export type APIRequestFn = ( timeout?: moment.Duration, ) => Promise; -// 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 }) { - return _.compact( - _.map(props, (v: any, k: string) => - !_.isNull(v) && !_.isUndefined(v) - ? `${encodeURIComponent(k)}=${encodeURIComponent(v.toString())}` - : null, - ), - ).join("&"); -} /** * ENDPOINTS */ @@ -676,7 +664,7 @@ export function getStatements( timeout?: moment.Duration, ): Promise { const queryStr = propsToQueryString({ - combined: true, + combined: req.combined, start: req.start.toInt(), end: req.end.toInt(), }); diff --git a/pkg/ui/workspaces/db-console/src/util/query.spec.ts b/pkg/ui/workspaces/db-console/src/util/query.spec.ts index 9d468c897e31..4007f4e443ac 100644 --- a/pkg/ui/workspaces/db-console/src/util/query.spec.ts +++ b/pkg/ui/workspaces/db-console/src/util/query.spec.ts @@ -9,8 +9,10 @@ // licenses/APL.txt. import { assert } from "chai"; -import { queryToString, queryByName } from "./query"; +import { propsToQueryString, queryByName } from "./query"; import { Location } from "history"; +import _ from "lodash"; +import Long from "long"; const location: Location = { pathname: "/debug/chart", @@ -22,11 +24,96 @@ 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", function() { + interface PropBag { + [k: string]: string; + } + + // helper decoding function used to doublecheck querystring generation + function decodeQueryString(qs: string): PropBag { + return qs.split("&").reduce((memo: PropBag, v: string) => { + const [key, value] = v.split("="); + memo[decodeURIComponent(key)] = decodeURIComponent(value).replace( + "%20", + "+", + ); + return memo; + }, {}); + } + + it("creates an appropriate querystring", function() { + const testValues: { [k: string]: any } = { + a: "testa", + b: "testb", + }; + + const querystring = propsToQueryString(testValues); + + assert(/a=testa/.test(querystring)); + assert(/b=testb/.test(querystring)); + assert.lengthOf(querystring.match(/=/g), 2); + assert.lengthOf(querystring.match(/&/g), 1); + assert.deepEqual(testValues, decodeQueryString(querystring)); + }); + + it("handles falsy values correctly", function() { + const testValues: { [k: string]: any } = { + // null and undefined should be ignored + undefined: undefined, + null: null, + // other values should be added + false: false, + "": "", + 0: 0, + }; + + const querystring = propsToQueryString(testValues); + + assert(/false=false/.test(querystring)); + assert(/0=0/.test(querystring)); + assert(/([^A-Za-z]|^)=([^A-Za-z]|$)/.test(querystring)); + assert.lengthOf(querystring.match(/=/g), 3); + assert.lengthOf(querystring.match(/&/g), 2); + assert.notOk(/undefined/.test(querystring)); + assert.notOk(/null/.test(querystring)); + assert.deepEqual( + { false: "false", "": "", 0: "0" }, + decodeQueryString(querystring), + ); + }); + + it("handles special characters", function() { + const key = "!@#$%^&*()=+-_\\|\"`'?/<>"; + const value = key + .split("") + .reverse() + .join(""); // key reversed + const testValues: { [k: string]: any } = { + [key]: value, + }; + + const querystring = propsToQueryString(testValues); + + assert(querystring.match(/%/g).length > (key + value).match(/%/g).length); + assert.deepEqual(testValues, decodeQueryString(querystring)); + }); + + it("handles non-string values", function() { + const testValues: { [k: string]: any } = { + boolean: true, + number: 1, + emptyObject: {}, + emptyArray: [], + objectWithProps: { a: 1, b: 2 }, + arrayWithElts: [1, 2, 3], + long: Long.fromNumber(1), + }; + + const querystring = propsToQueryString(testValues); + assert.deepEqual( + _.mapValues(testValues, _.toString), + decodeQueryString(querystring), + ); }); }); describe("queryByName", () => { diff --git a/pkg/ui/workspaces/db-console/src/util/query.ts b/pkg/ui/workspaces/db-console/src/util/query.ts index 93c891044b4b..5478bb82e75f 100644 --- a/pkg/ui/workspaces/db-console/src/util/query.ts +++ b/pkg/ui/workspaces/db-console/src/util/query.ts @@ -9,6 +9,7 @@ // licenses/APL.txt. import { Location } from "history"; +import _ from "lodash"; import { match as Match } from "react-router-dom"; interface ParamsObj { @@ -19,15 +20,18 @@ interface URLSearchParamsWithKeys extends URLSearchParams { keys: () => string; } -/* eslint-disable no-prototype-builtins,no-restricted-syntax */ -export function queryToString(obj: any) { - const str = []; - for (const p in obj) { - if (obj.hasOwnProperty(p)) { - str.push(`${encodeURIComponent(p)}=${encodeURIComponent(obj[p])}`); - } - } - return str.join("&"); +// 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 }): string { + return _.compact( + _.map(props, (v: any, k: string) => + !_.isNull(v) && !_.isUndefined(v) + ? `${encodeURIComponent(k)}=${encodeURIComponent(v.toString())}` + : null, + ), + ).join("&"); } export function queryToObj(location: Location, key: string, value: string) { @@ -48,7 +52,7 @@ export function queryToObj(location: Location, key: string, value: string) { return paramObj; } -export function queryByName(location: Location, key: string) { +export function queryByName(location: Location, key: string): string | null { const urlParams = new URLSearchParams(location.search); return urlParams.get(key); } diff --git a/pkg/ui/workspaces/db-console/src/views/statements/statementDetails.tsx b/pkg/ui/workspaces/db-console/src/views/statements/statementDetails.tsx index 5f029efaf702..c51cb2a8b446 100644 --- a/pkg/ui/workspaces/db-console/src/views/statements/statementDetails.tsx +++ b/pkg/ui/workspaces/db-console/src/views/statements/statementDetails.tsx @@ -13,6 +13,7 @@ import { match as Match, withRouter, } from "react-router-dom"; +import { Location } from "history"; import { createSelector } from "reselect"; import _ from "lodash"; @@ -41,7 +42,7 @@ import { statementAttr, } from "src/util/constants"; import { FixLong } from "src/util/fixLong"; -import { getMatchParamByName } from "src/util/query"; +import { getMatchParamByName, queryByName } from "src/util/query"; import { selectDiagnosticsReportsByStatementFingerprint } from "src/redux/statements/statementsSelectors"; import { StatementDetails, @@ -122,12 +123,13 @@ function fractionMatching( function filterByRouterParamsPredicate( match: Match, + 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 && @@ -163,9 +165,12 @@ export const selectStatement = createSelector( const internalAppNamePrefix = statementsState.data?.internal_app_name_prefix; const flattened = flattenStatementStats(statements); - const results = _.filter( - flattened, - filterByRouterParamsPredicate(props.match, internalAppNamePrefix), + const results = flattened.filter( + filterByRouterParamsPredicate( + props.match, + props.location, + internalAppNamePrefix, + ), ); const statement = getMatchParamByName(props.match, statementAttr); return { @@ -173,7 +178,7 @@ export const selectStatement = createSelector( 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),