rusackas commented on code in PR #41492:
URL: https://github.com/apache/superset/pull/41492#discussion_r3686080174
##########
tests/unit_tests/db_engine_specs/test_base.py:
##########
@@ -1383,3 +1383,11 @@ def
test_base_spec_public_information_includes_supports_offset() -> None:
assert "supports_offset" in info
assert info["supports_offset"] is True
+
+
+def test_get_public_information_exposes_ansi_identifier_quote() -> None:
+ """The base spec advertises ANSI double quotes for identifier quoting."""
+ assert BaseEngineSpec.get_public_information()["identifier_quote"] == {
+ "start": '"',
+ "end": '"',
+ }
Review Comment:
BaseEngineSpec is imported at the top of the file (line 36), so this doesn't
NameError, ran the test locally to confirm. Think this one's a false positive
from Copilot.
##########
superset-frontend/src/SqlLab/components/EditorWrapper/useKeywords.test.ts:
##########
@@ -210,6 +211,100 @@ test('quotes table identifiers that require quoting in
the inserted value', asyn
);
});
+test.each([
+ ['mysql', { start: '`', end: '`' }, '`COVID Vaccines`'],
+ ['mariadb', { start: '`', end: '`' }, '`COVID Vaccines`'],
+ ['mssql', { start: '[', end: ']' }, '[COVID Vaccines]'],
+ ['postgresql', { start: '"', end: '"' }, '"COVID Vaccines"'],
+])(
+ 'quotes table identifiers using the engine-provided quote characters for %s',
+ async (_dialect, identifierQuote, expectedValue) => {
+ const dbFunctionNamesApiRoute =
`glob:*/api/v1/database/${expectDbId}/function_names/`;
+ fetchMock.get(dbFunctionNamesApiRoute, fakeFunctionNamesApiResult);
+
+ const storeWithBackend = createStore(
+ {
+ ...initialState,
+ sqlLab: {
+ ...initialState.sqlLab,
+ databases: {
+ [expectDbId]: {
+ engine_information: { identifier_quote: identifierQuote },
+ },
+ },
+ },
+ },
+ reducers,
+ );
+
+ act(() => {
+ storeWithBackend.dispatch(
+ tableApiUtil.upsertQueryData(
+ 'tables',
+ { dbId: expectDbId, schema: expectSchema },
+ {
+ options: [
+ {
+ value: 'COVID Vaccines',
+ label: 'COVID Vaccines',
+ type: 'table',
+ },
+ ],
+ hasMore: false,
+ },
+ ),
+ );
+ });
+
+ const { result } = renderHook(
+ () =>
+ useKeywords({
+ queryEditorId: 'testqueryid',
+ dbId: expectDbId,
+ schema: expectSchema,
+ }),
+ {
+ wrapper: createWrapper({
+ useRedux: true,
+ store: storeWithBackend,
+ }),
+ },
+ );
+
+ await waitFor(() =>
+ expect(result.current).toContainEqual(
+ expect.objectContaining({
+ name: 'COVID Vaccines',
+ value: expectedValue,
+ meta: 'table',
+ }),
+ ),
+ );
+
+ // The caption inserted into the editor on selection is quoted with the
+ // same dialect-specific characters as `value`, not a hardcoded ANSI quote.
+ const tableKeyword = result.current.find(
+ keyword => keyword.meta === 'table' && keyword.name === 'COVID Vaccines',
+ );
+ const insertMatch = tableKeyword?.completer?.insertMatch;
Review Comment:
Good catch digging into this. AceEditorProvider's toAceKeyword does drop
completer and caption on the way to Ace, since EditorKeyword is meant to stay
provider-agnostic for the extensions API. Value already gets dialect-quoted
upstream in useKeywords before that conversion though, so the inserted text
still lands right, it's really the addTable-on-select side effect that's
silently lost through the generic adapter.
That feels like a bigger call on the editors interface (should EditorKeyword
grow an escape hatch for provider-specific callbacks) than this quote-chars fix
should take on, so I'd rather leave it for a follow-up than bolt it on here.
--
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]
