ui: fix generated DROP INDEX statement on table details page

[zachlite]

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.

[cockroach-teamcity]

This change is 

[maryliag]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @zachlite)

---

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/helperComponents.tsx line 189 at r1 (raw file):

    switch (recommendation.type) {
      case "DROP_UNUSED":
        // Here, `tableName` is a fully qualified name whose identifiers have already been quoted.

do you think it can be confusing to identify what has been quoted or not? Can the QuoteIdentifier do the check instead, and if already quotes does nothing? This way you don't risk calling it on things already quoted and you can always use it

[zachlite]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)

---

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/helperComponents.tsx line 189 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

do you think it can be confusing to identify what has been quoted or not? Can the QuoteIdentifier do the check instead, and if already quotes does nothing? This way you don't risk calling it on things already quoted and you can always use it

I agree it's confusing. I did what you're suggesting here initially, thinking that QuoteIdentifier should be idempotent. But the problem is that it becomes difficult (impossible?) to know what to do if the caller doesn't tell us the state of the argument.

For example, consider "tablename". Without the caller telling us, it's impossible to tell if the argument is already a quoted identifier, or if the argument should treated as a non-quoted identifier and it's chars need escaping. If the former, the result should be "tablename". If the latter, the result should be """tablename""".

My solution was to document this and write a unit test.. WDYT?

[zachlite]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)

---

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/helperComponents.tsx line 189 at r1 (raw file):

Previously, zachlite wrote…

I agree it's confusing. I did what you're suggesting here initially, thinking that QuoteIdentifier should be idempotent. But the problem is that it becomes difficult (impossible?) to know what to do if the caller doesn't tell us the state of the argument.

For example, consider "tablename". Without the caller telling us, it's impossible to tell if the argument is already a quoted identifier, or if the argument should treated as a non-quoted identifier and it's chars need escaping. If the former, the result should be "tablename". If the latter, the result should be """tablename""".

My solution was to document this and write a unit test.. WDYT?

And QuoteIdentifier currently always assumes the latter.

[maryliag]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @zachlite)

---

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/helperComponents.tsx line 189 at r1 (raw file):

Previously, zachlite wrote…

And QuoteIdentifier currently always assumes the latter.

I see. Thanks for the explanation

[zachlite]

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)