Skip to content

Commit

Permalink
ui: fix generated DROP INDEX statement on table details page
Browse files Browse the repository at this point in the history
This commit does two things:
1. Fixes the generated `DROP INDEX` statement by considering
already quoted fully-qualified names.

2. Adds unit tests for the `QuoteIdentifier` utility to clarify
its intended usage and limitations.

Epic: none
Release note (bug fix): Fixed a UI issue where the
DROP_UNUSED index recommendations produced by the
table details page produced an invalid `DROP INDEX` statement.
  • Loading branch information
Zach Lite committed Sep 12, 2023
1 parent 29046c1 commit b33f9f3
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 2 deletions.
25 changes: 24 additions & 1 deletion pkg/ui/workspaces/cluster-ui/src/api/safesql.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

import { Format, Identifier, Join, SQL } from "./safesql";
import { Format, Identifier, Join, QuoteIdentifier, SQL } from "./safesql";

describe("safesql", () => {
test("format", () => {
Expand Down Expand Up @@ -144,4 +144,27 @@ describe("safesql", () => {
expect(tc.got.SQLString()).toEqual(tc.expected);
});
});

// https://www.cockroachlabs.com/docs/stable/keywords-and-identifiers#rules-for-identifiers
// https://www.postgresql.org/docs/15/sql-syntax-lexical.html#SQL-SYNTAX-IDENTIFIERS
test("QuoteIdentifier", () => {
const testCases = [
["foobar", '"foobar"'],
['weird"table', '"weird""table"'],
['"quoted"', '"""quoted"""'],

// Anti-test cases: The following cases document the inputs that
// QuoteIdentifier can not deal with.
// 1. A fully qualified name that is not comprised of quoted identifiers.
// 2. A fully qualified name that is already made of quoted identifiers.
["schema.table", '"schema.table"'], // Invalid SQL output.
['"schema"."table"', '"""schema"".""table"""'], // Invalid SQL output.
];

for (const tcase of testCases) {
const res = QuoteIdentifier(tcase[0]);
const expected = tcase[1];
expect(res).toEqual(expected);
}
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,9 @@ export const ActionCell = ({
const query = indexStat.indexRecommendations.map(recommendation => {
switch (recommendation.type) {
case "DROP_UNUSED":
return `DROP INDEX ${QuoteIdentifier(tableName)}@${QuoteIdentifier(
// Here, `tableName` is a fully qualified name whose identifiers have already been quoted.
// See the QuoteIdentifier unit tests for more details.
return `DROP INDEX ${tableName}@${QuoteIdentifier(
indexStat.indexName,
)};`;
}
Expand Down

0 comments on commit b33f9f3

Please sign in to comment.