aminghadersohi commented on code in PR #41492:
URL: https://github.com/apache/superset/pull/41492#discussion_r3686742066
##########
superset-frontend/src/SqlLab/components/EditorWrapper/useKeywords.ts:
##########
@@ -54,19 +55,28 @@ const getHelperText = (value: string) =>
};
// Names that aren't simple identifiers (spaces, punctuation, leading digits)
-// must be double-quoted to be valid SQL, with embedded quotes doubled.
+// must be quoted to be valid SQL. The quote characters are dialect-specific
+// (e.g. ANSI double quotes, MySQL/MariaDB backticks, SQL Server square
brackets)
+// and are provided by the backend's database engine spec via
`engine_information`
+// so the mapping isn't duplicated here. Embedded quote characters are escaped
by
+// doubling the closing character.
+type IdentifierQuote = { start: string; end: string };
+const ANSI_QUOTE: IdentifierQuote = { start: '"', end: '"' };
const SIMPLE_IDENTIFIER_RE = /^[A-Za-z_][A-Za-z0-9_]*$/;
-const quoteIdentifier = (identifier: string) =>
+const quoteIdentifier = (
+ identifier: string,
+ { start, end }: IdentifierQuote = ANSI_QUOTE,
+) =>
SIMPLE_IDENTIFIER_RE.test(identifier)
? identifier
- : `"${identifier.replace(/"/g, '""')}"`;
+ : `${start}${identifier.split(end).join(`${end}${end}`)}${end}`;
Review Comment:
Supporting @sadpandajoe's point on the escaping strategy here: GoogleSQL
does escape an embedded backtick with a backslash (`\``), not by doubling, so
`.split(end).join(`${end}${end}`)` is the wrong rule for BigQuery specifically.
More generally the escape convention isn't derivable from the delimiter char
alone — MySQL uses the same backtick delimiter but escapes by doubling, which
this code handles correctly.
For severity: the concrete BigQuery breakage looks effectively unreachable —
BigQuery table/column identifiers can't contain a backtick (grave accent isn't
in the allowed identifier character set), so no real catalog name reaches the
divergent-escaping branch. MySQL, where embedded backticks are legal, is the
reachable case and it's escaped correctly. So this reads as a
latent-correctness follow-up (carry the escape strategy in `engine_information`
if more dialects with non-doubling escapes are added) rather than a blocker for
this fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
