From b33f9f352d22a73d6c42475319092dca74a21570 Mon Sep 17 00:00:00 2001
From: Zach Lite <zach@cockroachlabs.com>
Date: Tue, 12 Sep 2023 09:26:39 -0400
Subject: [PATCH] ui: fix generated `DROP INDEX` statement on table details
 page

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.
---
 .../cluster-ui/src/api/safesql.spec.ts        | 25 ++++++++++++++++++-
 .../databaseTablePage/helperComponents.tsx    |  4 ++-
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/pkg/ui/workspaces/cluster-ui/src/api/safesql.spec.ts b/pkg/ui/workspaces/cluster-ui/src/api/safesql.spec.ts
index a76b31535bef..e5399c99caf6 100644
--- a/pkg/ui/workspaces/cluster-ui/src/api/safesql.spec.ts
+++ b/pkg/ui/workspaces/cluster-ui/src/api/safesql.spec.ts
@@ -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", () => {
@@ -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);
+    }
+  });
 });
diff --git a/pkg/ui/workspaces/cluster-ui/src/databaseTablePage/helperComponents.tsx b/pkg/ui/workspaces/cluster-ui/src/databaseTablePage/helperComponents.tsx
index 74e3f1262c88..882da233c86d 100644
--- a/pkg/ui/workspaces/cluster-ui/src/databaseTablePage/helperComponents.tsx
+++ b/pkg/ui/workspaces/cluster-ui/src/databaseTablePage/helperComponents.tsx
@@ -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,
         )};`;
     }