From 537f0fd2dbe19e6d0fbf136c484150e567b0961f Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Thu, 13 Aug 2026 14:04:38 -0700 Subject: [PATCH] fix(sqllab): use dialect-specific quote chars for autocomplete identifiers (#41492) Co-authored-by: Claude Opus 4.8 Co-authored-by: Amin Ghadersohi Co-authored-by: Joe Li --- .../src/components/AsyncAceEditor/index.tsx | 3 + .../src/components/AsyncAceEditor/types.ts | 3 + .../EditorWrapper/useKeywords.test.ts | 160 ++++++++++++++++++ .../components/EditorWrapper/useKeywords.ts | 63 +++++-- .../src/core/editors/AceEditorProvider.tsx | 14 +- superset/databases/schemas.py | 24 +++ superset/db_engine_specs/base.py | 19 +++ superset/db_engine_specs/bigquery.py | 7 + superset/db_engine_specs/databricks.py | 5 + superset/db_engine_specs/hive.py | 6 + superset/db_engine_specs/mssql.py | 5 + superset/db_engine_specs/mysql.py | 4 + superset/db_engine_specs/sybase.py | 5 + superset/sqllab/schemas.py | 36 +++- superset/sqllab/utils.py | 1 + .../integration_tests/databases/api_tests.py | 40 +++++ tests/unit_tests/databases/api_test.py | 10 ++ tests/unit_tests/databases/test_schemas.py | 54 ++++++ tests/unit_tests/db_engine_specs/test_base.py | 10 ++ .../db_engine_specs/test_bigquery.py | 12 ++ .../db_engine_specs/test_databricks.py | 17 ++ tests/unit_tests/db_engine_specs/test_hive.py | 23 +++ .../unit_tests/db_engine_specs/test_mssql.py | 11 ++ .../unit_tests/db_engine_specs/test_mysql.py | 11 ++ .../unit_tests/db_engine_specs/test_sybase.py | 29 ++++ tests/unit_tests/sqllab/test_schemas.py | 54 ++++++ tests/unit_tests/sqllab/test_utils.py | 52 ++++++ 27 files changed, 661 insertions(+), 17 deletions(-) create mode 100644 tests/unit_tests/db_engine_specs/test_sybase.py create mode 100644 tests/unit_tests/sqllab/test_schemas.py create mode 100644 tests/unit_tests/sqllab/test_utils.py diff --git a/superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx b/superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx index 30d5ce01c2a..2165426d5ee 100644 --- a/superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx +++ b/superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx @@ -57,6 +57,9 @@ export interface AceCompleterKeywordData { meta: string; docText?: string; docHTML?: string; + // The label Ace displays in the autocomplete popup and passes back to + // `insertMatch`; may differ from `value`, the text actually inserted. + caption?: string; } export type TextMode = OrigTextMode & { $id: string }; diff --git a/superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/types.ts b/superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/types.ts index 17a4210b2ba..27a9bf16be3 100644 --- a/superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/types.ts +++ b/superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/types.ts @@ -29,6 +29,9 @@ export interface AceCompleterKeywordData { meta: string; docText?: string; docHTML?: string; + // The label Ace displays in the autocomplete popup and passes back to + // `insertMatch`; may differ from `value`, the text actually inserted. + caption?: string; } export type TextMode = OrigTextMode & { $id: string }; diff --git a/superset-frontend/src/SqlLab/components/EditorWrapper/useKeywords.test.ts b/superset-frontend/src/SqlLab/components/EditorWrapper/useKeywords.test.ts index 7462c043118..18910f0cada 100644 --- a/superset-frontend/src/SqlLab/components/EditorWrapper/useKeywords.test.ts +++ b/superset-frontend/src/SqlLab/components/EditorWrapper/useKeywords.test.ts @@ -19,6 +19,8 @@ import fetchMock from 'fetch-mock'; import { act, renderHook, waitFor } from '@testing-library/react'; import { getExtensionsRegistry } from '@superset-ui/core'; +import type { Editor } from '@superset-ui/core/components'; +import { toAceKeyword } from 'src/core/editors/AceEditorProvider'; import { createWrapper, defaultStore as store, @@ -29,6 +31,7 @@ import { schemaApiUtil } from 'src/hooks/apiResources/schemas'; import { tableApiUtil } from 'src/hooks/apiResources/tables'; import { addTable } from 'src/SqlLab/actions/sqlLab'; import { initialState } from 'src/SqlLab/fixtures'; +import type { SqlLabRootState } from 'src/SqlLab/types'; import reducers from 'spec/helpers/reducerIndex'; import { SCHEMA_AUTOCOMPLETE_SCORE, @@ -210,6 +213,163 @@ test('quotes table identifiers that require quoting in the inserted value', asyn ); }); +type IdentifierQuoteFixture = { + start: string; + end: string; + escape_by_doubling?: boolean; +}; + +// Shared setup for the dialect-specific quoting tests below: seeds a single +// table into the store with the given engine-provided quote chars, then +// renders the hook so keyword autocomplete for that table can be asserted. +function renderKeywordsForTable( + identifierQuote: IdentifierQuoteFixture, + tableName: string, +) { + 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: tableName, label: tableName, type: 'table' }], + hasMore: false, + }, + ), + ); + }); + + return { + store: storeWithBackend, + ...renderHook( + () => + useKeywords({ + queryEditorId: 'testqueryid', + dbId: expectDbId, + schema: expectSchema, + }), + { + wrapper: createWrapper({ + useRedux: true, + store: storeWithBackend, + }), + }, + ), + }; +} + +test.each([ + ['mysql', { start: '`', end: '`' }, '`COVID Vaccines`'], + ['mariadb', { start: '`', end: '`' }, '`COVID Vaccines`'], + ['mssql', { start: '[', end: ']' }, '[COVID Vaccines]'], + ['postgresql', { start: '"', end: '"' }, '"COVID Vaccines"'], + [ + 'bigquery', + { start: '`', end: '`', escape_by_doubling: false }, + '`COVID Vaccines`', + ], +])( + 'quotes table identifiers using the engine-provided quote characters for %s', + async (_dialect, identifierQuote, expectedValue) => { + const { result, store: storeWithBackend } = renderKeywordsForTable( + identifierQuote, + 'COVID Vaccines', + ); + + 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. + // Goes through the real AceEditorProvider `toAceKeyword` conversion + // (the shipped Ace path) rather than invoking the hook's completer + // directly, since that conversion is what production code actually + // hands to Ace, and it's what's historically dropped the `completer` + // callback that dispatches `addTable` on table selection. + const tableKeyword = result.current.find( + keyword => keyword.meta === 'table' && keyword.name === 'COVID Vaccines', + ); + const aceKeyword = toAceKeyword(tableKeyword!); + const insertMatch = aceKeyword.completer?.insertMatch; + const editor = { + completer: { insertMatch: jest.fn() }, + } as unknown as Editor; + act(() => { + insertMatch?.(editor, aceKeyword); + }); + expect(editor.completer.insertMatch).toHaveBeenCalledWith( + `${expectedValue} `, + ); + + // Selecting a table also dispatches `addTable`; this side effect lives + // on the same `completer` that `toAceKeyword` must carry through. + const { sqlLab } = + storeWithBackend.getState() as unknown as SqlLabRootState; + expect( + sqlLab.tables.some( + (queryEditorTable: { name: string }) => + queryEditorTable.name === expectedValue, + ), + ).toBe(true); + }, +); + +test.each([ + [ + 'mysql', + { start: '`', end: '`', escape_by_doubling: true }, + '`COVID``Vaccines`', + ], + [ + 'bigquery', + { start: '`', end: '`', escape_by_doubling: false }, + '`COVID\\`Vaccines`', + ], +])( + 'escapes an embedded closing-quote character per the %s engine-provided escape strategy', + async (_dialect, identifierQuote, expectedValue) => { + const { result } = renderKeywordsForTable( + identifierQuote, + 'COVID`Vaccines', + ); + + await waitFor(() => + expect(result.current).toContainEqual( + expect.objectContaining({ + name: 'COVID`Vaccines', + value: expectedValue, + meta: 'table', + }), + ), + ); + }, +); + test('skip fetching if autocomplete skipped', () => { const { result } = renderHook( () => diff --git a/superset-frontend/src/SqlLab/components/EditorWrapper/useKeywords.ts b/superset-frontend/src/SqlLab/components/EditorWrapper/useKeywords.ts index afef249a20e..5faad2c0527 100644 --- a/superset-frontend/src/SqlLab/components/EditorWrapper/useKeywords.ts +++ b/superset-frontend/src/SqlLab/components/EditorWrapper/useKeywords.ts @@ -17,12 +17,12 @@ * under the License. */ import { useEffect, useMemo, useRef } from 'react'; -import { useStore } from 'react-redux'; +import { useSelector, useStore } from 'react-redux'; import { useAppDispatch } from 'src/SqlLab/hooks/useAppDispatch'; import { t } from '@apache-superset/core/translation'; import { getExtensionsRegistry } from '@superset-ui/core'; -import type { Editor } from '@superset-ui/core/components'; +import type { AceCompleterKeyword, Editor } from '@superset-ui/core/components'; import sqlKeywords from 'src/SqlLab/utils/sqlKeywords'; import { addTable, addDangerToast } from 'src/SqlLab/actions/sqlLab'; import { @@ -35,6 +35,7 @@ import { schemaEndpoints } from 'src/hooks/apiResources'; import { api } from 'src/hooks/apiResources/queryApi'; import { useDatabaseFunctionsQuery } from 'src/hooks/apiResources/databaseFunctions'; import useEffectEvent from 'src/hooks/useEffectEvent'; +import type { SqlLabRootState } from 'src/SqlLab/types'; type Params = { queryEditorId: string | number; @@ -54,19 +55,41 @@ 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 (and how an embedded +// closing-quote character is escaped) are dialect-specific and are provided +// by the backend's database engine spec via `engine_information` so the +// mapping isn't duplicated here. Most dialects (ANSI double quotes, +// MySQL/MariaDB backticks, SQL Server square brackets) escape by doubling +// the closing character; BigQuery's GoogleSQL backtick identifiers are the +// documented exception, escaping with a backslash instead. +type IdentifierQuote = { + start: string; + end: string; + escape_by_doubling?: boolean; +}; +const ANSI_QUOTE: IdentifierQuote = { + start: '"', + end: '"', + escape_by_doubling: true, +}; const SIMPLE_IDENTIFIER_RE = /^[A-Za-z_][A-Za-z0-9_]*$/; -const quoteIdentifier = (identifier: string) => - SIMPLE_IDENTIFIER_RE.test(identifier) - ? identifier - : `"${identifier.replace(/"/g, '""')}"`; +const quoteIdentifier = ( + identifier: string, + { start, end, escape_by_doubling = true }: IdentifierQuote = ANSI_QUOTE, +) => { + if (SIMPLE_IDENTIFIER_RE.test(identifier)) { + return identifier; + } + const escapedEnd = escape_by_doubling ? `${end}${end}` : `\\${end}`; + return `${start}${identifier.split(end).join(escapedEnd)}${end}`; +}; const extensionsRegistry = getExtensionsRegistry(); export function useKeywords( { queryEditorId, dbId, catalog, schema, tabViewId }: Params, skip = false, -) { +): AceCompleterKeyword[] { const useCustomKeywords = extensionsRegistry.get( 'sqleditor.extension.customAutocomplete', ); @@ -106,6 +129,16 @@ export function useKeywords( const store = useStore(); const apiState = store.getState()[api.reducerPath]; + // Dialect-specific identifier quote characters, provided by the backend's + // database engine spec, used to quote non-simple identifiers on insert. + const identifierQuote = useSelector< + SqlLabRootState, + IdentifierQuote | undefined + >( + ({ sqlLab }) => + sqlLab?.databases?.[dbId ?? '']?.engine_information?.identifier_quote, + ); + // Normalize catalog for comparison (null/undefined both mean "no catalog") const normalizedCatalog = catalog ?? null; @@ -175,9 +208,11 @@ export function useKeywords( ); } - let { caption } = data; - if (data.meta === 'table' && caption.includes(' ')) { - caption = `"${caption}"`; + // `caption` is optional on AceCompleterKeywordData; fall back to `name`/ + // `value` so a missing caption can't quote `undefined` into the editor. + let caption = data.caption ?? data.name ?? data.value ?? ''; + if (data.meta === 'table') { + caption = quoteIdentifier(caption, identifierQuote); } // executing https://github.com/thlorenz/brace/blob/3a00c5d59777f9d826841178e1eb36694177f5e6/ext/language_tools.js#L1448 @@ -205,7 +240,7 @@ export function useKeywords( () => allCachedTables.map(({ value, label, schema: tableSchema }) => ({ name: label, - value: quoteIdentifier(value), + value: quoteIdentifier(value, identifierQuote), schema: tableSchema, score: TABLE_AUTOCOMPLETE_SCORE, meta: 'table', @@ -214,7 +249,7 @@ export function useKeywords( }, ...getHelperText(value), })), - [allCachedTables, insertMatch], + [allCachedTables, identifierQuote, insertMatch], ); const columnKeywords = useMemo( @@ -244,7 +279,7 @@ export function useKeywords( [functionNames, insertMatch], ); - const keywords = useMemo( + const keywords = useMemo( () => columnKeywords .concat(schemaKeywords) diff --git a/superset-frontend/src/core/editors/AceEditorProvider.tsx b/superset-frontend/src/core/editors/AceEditorProvider.tsx index 3deb345a217..ae0dd322bed 100644 --- a/superset-frontend/src/core/editors/AceEditorProvider.tsx +++ b/superset-frontend/src/core/editors/AceEditorProvider.tsx @@ -223,14 +223,26 @@ const createAceEditorHandle = ( /** * Converts generic EditorKeyword to Ace's AceCompleterKeyword format. + * + * `completer` and `caption` aren't part of the provider-agnostic + * EditorKeyword contract (other editor providers have no notion of an Ace + * completer), but this is specifically the Ace provider, and Ace-aware + * producers such as SQL Lab's useKeywords already build AceCompleterKeyword + * objects and pass them in as EditorKeyword. Carry those two fields through + * when present so per-keyword selection behavior isn't silently dropped. */ -const toAceKeyword = (keyword: EditorKeyword): AceCompleterKeyword => ({ +export const toAceKeyword = ( + keyword: EditorKeyword & + Partial>, +): AceCompleterKeyword => ({ name: keyword.name, value: keyword.value ?? keyword.name, score: keyword.score ?? 0, meta: keyword.meta ?? '', docText: keyword.detail, docHTML: keyword.documentation, + completer: keyword.completer, + caption: keyword.caption, }); /** diff --git a/superset/databases/schemas.py b/superset/databases/schemas.py index fcc83009a3a..4cb6f3b40ba 100644 --- a/superset/databases/schemas.py +++ b/superset/databases/schemas.py @@ -1172,6 +1172,24 @@ class DatabaseSchemaAccessForFileUploadResponse(Schema): ) +class IdentifierQuoteSchema(Schema): + start = fields.String( + metadata={"description": "Character that opens a quoted identifier"} + ) + end = fields.String( + metadata={"description": "Character that closes a quoted identifier"} + ) + escape_by_doubling = fields.Boolean( + metadata={ + "description": ( + "Whether an embedded closing-quote character is escaped by " + "doubling it (True) or with a backslash escape (False, e.g. " + "BigQuery's GoogleSQL backtick identifiers)" + ) + } + ) + + class EngineInformationSchema(Schema): supports_file_upload = fields.Boolean( metadata={"description": "Users can upload files to the database"} @@ -1198,6 +1216,12 @@ class EngineInformationSchema(Schema): ) } ) + identifier_quote = fields.Nested( + IdentifierQuoteSchema, + metadata={ + "description": "Characters used to quote identifiers for this dialect" + }, + ) class DatabaseConnectionSchema(Schema): diff --git a/superset/db_engine_specs/base.py b/superset/db_engine_specs/base.py index 2340a593b8a..ebd85bd43a4 100644 --- a/superset/db_engine_specs/base.py +++ b/superset/db_engine_specs/base.py @@ -575,6 +575,20 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods arraysize = 0 max_column_name_length: int | None = None + # Characters used to quote identifiers (table/column names) that aren't simple. + # Defaults to ANSI double quotes; dialects that differ override these — e.g. + # MySQL/MariaDB use backticks and SQL Server uses square brackets. These are + # surfaced to the client (see `get_public_information`) so identifier quoting + # stays owned by the engine spec rather than duplicated per client. + identifier_quote_start: str = '"' + identifier_quote_end: str = '"' + # How an embedded closing-quote character is escaped within a quoted + # identifier. Most dialects (ANSI, MySQL/MariaDB backticks, SQL Server + # brackets) escape by doubling the closing character. BigQuery's GoogleSQL + # backtick identifiers are the exception, escaping with a backslash instead, + # so it overrides this to False. + identifier_quote_escape_by_doubling: bool = True + # Some databases (e.g. Druid, Pinot) build cursor.description by inspecting # the values in the first returned row rather than from query-plan metadata. # For those engines WHERE FALSE returns no rows and therefore leaves @@ -2761,6 +2775,11 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods "supports_oauth2": cls.supports_oauth2, "supports_schemas": cls.supports_schemas, "supports_offset": cls.supports_offset, + "identifier_quote": { + "start": cls.identifier_quote_start, + "end": cls.identifier_quote_end, + "escape_by_doubling": cls.identifier_quote_escape_by_doubling, + }, } @classmethod diff --git a/superset/db_engine_specs/bigquery.py b/superset/db_engine_specs/bigquery.py index 1aebd02b835..1fac3223a50 100644 --- a/superset/db_engine_specs/bigquery.py +++ b/superset/db_engine_specs/bigquery.py @@ -228,6 +228,13 @@ class BigQueryEngineSpec(BaseEngineSpec): # pylint: disable=too-many-public-met max_column_name_length = 128 disable_ssh_tunneling = True + # BigQuery quotes identifiers with backticks rather than ANSI double quotes, + # and escapes an embedded backtick with a backslash rather than by + # doubling it (GoogleSQL, unlike MySQL/MariaDB backticks). + identifier_quote_start: str = "`" + identifier_quote_end: str = "`" + identifier_quote_escape_by_doubling: bool = False + parameters_schema = BigQueryParametersSchema() default_driver = "bigquery" sqlalchemy_uri_placeholder = "bigquery://{project_id}" diff --git a/superset/db_engine_specs/databricks.py b/superset/db_engine_specs/databricks.py index 24108318ae2..c11f66a36f4 100644 --- a/superset/db_engine_specs/databricks.py +++ b/superset/db_engine_specs/databricks.py @@ -259,6 +259,11 @@ class DatabricksHiveEngineSpec(HiveEngineSpec): class DatabricksBaseEngineSpec(BaseEngineSpec): _time_grain_expressions = time_grain_expressions + # Databricks SQL is Spark SQL under the hood: identifiers are quoted with + # backticks, not the inherited ANSI double quotes. + identifier_quote_start: str = "`" + identifier_quote_end: str = "`" + @classmethod def convert_dttm( cls, target_type: str, dttm: datetime, db_extra: dict[str, Any] | None = None diff --git a/superset/db_engine_specs/hive.py b/superset/db_engine_specs/hive.py index a806155b0a4..be93d0de3d7 100644 --- a/superset/db_engine_specs/hive.py +++ b/superset/db_engine_specs/hive.py @@ -94,6 +94,12 @@ class HiveEngineSpec(PrestoEngineSpec): allows_alias_to_source_column = True allows_hidden_orderby_agg = False + # Unlike the ANSI-compliant Presto/Trino this spec is otherwise based on, + # HiveQL and Spark SQL quote identifiers with backticks; double quotes are + # treated as string literals by default. + identifier_quote_start: str = "`" + identifier_quote_end: str = "`" + supports_dynamic_schema = True supports_cross_catalog_queries = False # Explicitly opt out (overriding the inherited PrestoEngineSpec value): diff --git a/superset/db_engine_specs/mssql.py b/superset/db_engine_specs/mssql.py index 3a8bdfa8dbe..e6ac3a05c96 100644 --- a/superset/db_engine_specs/mssql.py +++ b/superset/db_engine_specs/mssql.py @@ -53,6 +53,11 @@ class MssqlEngineSpec(BaseEngineSpec): engine = "mssql" engine_name = "Microsoft SQL Server" + # SQL Server quotes identifiers with square brackets; only the closing bracket + # needs escaping (by doubling). + identifier_quote_start: str = "[" + identifier_quote_end: str = "]" + metadata = { "description": ( "Microsoft SQL Server is a relational database management system." diff --git a/superset/db_engine_specs/mysql.py b/superset/db_engine_specs/mysql.py index 459f9b847dd..93f764703d0 100644 --- a/superset/db_engine_specs/mysql.py +++ b/superset/db_engine_specs/mysql.py @@ -81,6 +81,10 @@ class MySQLEngineSpec(BasicParametersMixin, BaseEngineSpec): engine_name = "MySQL" max_column_name_length = 64 + # MySQL/MariaDB quote identifiers with backticks rather than ANSI double quotes. + identifier_quote_start: str = "`" + identifier_quote_end: str = "`" + default_driver = "mysqldb" sqlalchemy_uri_placeholder = ( "mysql://user:password@host:port/dbname[?key=value&key=value...]" diff --git a/superset/db_engine_specs/sybase.py b/superset/db_engine_specs/sybase.py index f03c6febd65..092a3cad7a6 100644 --- a/superset/db_engine_specs/sybase.py +++ b/superset/db_engine_specs/sybase.py @@ -30,6 +30,11 @@ class SybaseEngineSpec(MssqlEngineSpec): engine = "sybase" engine_name = "SAP Sybase" engine_aliases = {"sybase_sqlany"} # Support SQL Anywhere dialect too + + # SAP ASE's `quoted_identifier` session option defaults to OFF, in which + # case double-quoted strings are parsed as string literals rather than + # identifiers. Square brackets, inherited from MssqlEngineSpec, work + # regardless of that setting, so no override is needed here. default_driver = "pyodbc" metadata = { diff --git a/superset/sqllab/schemas.py b/superset/sqllab/schemas.py index 4adb07e810a..92686838274 100644 --- a/superset/sqllab/schemas.py +++ b/superset/sqllab/schemas.py @@ -16,7 +16,7 @@ # under the License. from marshmallow import fields, Schema, validate -from superset.databases.schemas import ImportV1DatabaseSchema +from superset.databases.schemas import EngineInformationSchema # Restricts the optional CTAS target name to a bare SQL identifier. Shared by the # SQL Lab execute payload schemas so both request paths validate it identically. @@ -155,13 +155,45 @@ class TabStateSchema(Schema): user_id = fields.Integer() +class SQLLabBootstrapDatabaseSchema(Schema): + """ + Shape of a single database entry in the SQL Lab bootstrap response. + + This mirrors ``superset.sqllab.utils.DATABASE_KEYS`` (plus + ``allows_virtual_table_explore``), which is what actually populates the + payload. It's intentionally distinct from ``ImportV1DatabaseSchema``, + which documents the unrelated database import/export contract. + """ + + id = fields.Integer(metadata={"description": "The database id"}) + database_name = fields.String(metadata={"description": "The database name"}) + backend = fields.String(metadata={"description": "The database backend"}) + allow_file_upload = fields.Boolean() + allow_ctas = fields.Boolean() + allow_cvas = fields.Boolean() + allow_dml = fields.Boolean() + allow_run_async = fields.Boolean() + allow_multi_catalog = fields.Boolean() + allows_cost_estimate = fields.Boolean(allow_none=True) + allows_subquery = fields.Boolean() + allows_virtual_table_explore = fields.Boolean() + disable_data_preview = fields.Boolean() + disable_drill_to_detail = fields.Boolean() + expose_in_sqllab = fields.Boolean() + force_ctas_schema = fields.String(allow_none=True) + engine_information = fields.Nested( + EngineInformationSchema, + metadata={"description": "Dialect capabilities, including identifier_quote"}, + ) + + class SQLLabBootstrapSchema(Schema): active_tab = fields.Nested(TabStateSchema) databases = fields.Dict( keys=fields.String( metadata={"description": "Database id"}, ), - values=fields.Nested(ImportV1DatabaseSchema), + values=fields.Nested(SQLLabBootstrapDatabaseSchema), ) queries = fields.Dict( keys=fields.String( diff --git a/superset/sqllab/utils.py b/superset/sqllab/utils.py index cfcf02b2c2c..bedd5ca36c7 100644 --- a/superset/sqllab/utils.py +++ b/superset/sqllab/utils.py @@ -41,6 +41,7 @@ DATABASE_KEYS = [ "disable_data_preview", "disable_drill_to_detail", "allow_multi_catalog", + "engine_information", ] diff --git a/tests/integration_tests/databases/api_tests.py b/tests/integration_tests/databases/api_tests.py index b53f2c9d814..48646ce2933 100644 --- a/tests/integration_tests/databases/api_tests.py +++ b/tests/integration_tests/databases/api_tests.py @@ -3545,6 +3545,11 @@ class TestDatabaseApi(SupersetTestCase): "supports_oauth2": False, "supports_offset": True, "supports_schemas": True, + "identifier_quote": { + "start": '"', + "end": '"', + "escape_by_doubling": True, + }, }, "supports_oauth2": False, }, @@ -3575,6 +3580,11 @@ class TestDatabaseApi(SupersetTestCase): "supports_oauth2": False, "supports_offset": True, "supports_schemas": True, + "identifier_quote": { + "start": "`", + "end": "`", + "escape_by_doubling": False, + }, }, "supports_oauth2": False, }, @@ -3635,6 +3645,11 @@ class TestDatabaseApi(SupersetTestCase): "supports_oauth2": False, "supports_offset": True, "supports_schemas": True, + "identifier_quote": { + "start": '"', + "end": '"', + "escape_by_doubling": True, + }, }, "supports_oauth2": False, }, @@ -3682,6 +3697,11 @@ class TestDatabaseApi(SupersetTestCase): "supports_oauth2": True, "supports_offset": True, "supports_schemas": True, + "identifier_quote": { + "start": '"', + "end": '"', + "escape_by_doubling": True, + }, }, "supports_oauth2": True, }, @@ -3742,6 +3762,11 @@ class TestDatabaseApi(SupersetTestCase): "supports_oauth2": False, "supports_offset": True, "supports_schemas": True, + "identifier_quote": { + "start": "`", + "end": "`", + "escape_by_doubling": True, + }, }, "supports_oauth2": False, }, @@ -3758,6 +3783,11 @@ class TestDatabaseApi(SupersetTestCase): "supports_oauth2": False, "supports_offset": True, "supports_schemas": True, + "identifier_quote": { + "start": '"', + "end": '"', + "escape_by_doubling": True, + }, }, "supports_oauth2": False, }, @@ -3794,6 +3824,11 @@ class TestDatabaseApi(SupersetTestCase): "supports_oauth2": False, "supports_offset": True, "supports_schemas": True, + "identifier_quote": { + "start": "`", + "end": "`", + "escape_by_doubling": True, + }, }, "supports_oauth2": False, }, @@ -3810,6 +3845,11 @@ class TestDatabaseApi(SupersetTestCase): "supports_oauth2": False, "supports_offset": True, "supports_schemas": True, + "identifier_quote": { + "start": '"', + "end": '"', + "escape_by_doubling": True, + }, }, "supports_oauth2": False, }, diff --git a/tests/unit_tests/databases/api_test.py b/tests/unit_tests/databases/api_test.py index 69c7f43e563..2ebb88f1ee1 100644 --- a/tests/unit_tests/databases/api_test.py +++ b/tests/unit_tests/databases/api_test.py @@ -245,6 +245,11 @@ def test_database_connection( "supports_oauth2": True, "supports_offset": True, "supports_schemas": True, + "identifier_quote": { + "start": '"', + "end": '"', + "escape_by_doubling": True, + }, }, "expose_in_sqllab": True, "extra": '{\n "metadata_params": {},\n "engine_params": {},\n "metadata_cache_timeout": {},\n "schemas_allowed_for_file_upload": []\n}\n', # noqa: E501 @@ -336,6 +341,11 @@ def test_database_connection( "supports_oauth2": True, "supports_offset": True, "supports_schemas": True, + "identifier_quote": { + "start": '"', + "end": '"', + "escape_by_doubling": True, + }, }, "expose_in_sqllab": True, "force_ctas_schema": None, diff --git a/tests/unit_tests/databases/test_schemas.py b/tests/unit_tests/databases/test_schemas.py index da29a11c144..323807148a5 100644 --- a/tests/unit_tests/databases/test_schemas.py +++ b/tests/unit_tests/databases/test_schemas.py @@ -35,3 +35,57 @@ def test_engine_information_schema_includes_supports_offset() -> None: ) assert result["supports_offset"] is False + + +def test_engine_information_schema_includes_identifier_quote() -> None: + """ + identifier_quote is a nested field that must pass the dialect-specific + quote characters through unchanged. + """ + from superset.databases.schemas import EngineInformationSchema + + schema = EngineInformationSchema() + result = schema.dump( + { + "supports_file_upload": True, + "disable_ssh_tunneling": False, + "supports_dynamic_catalog": False, + "supports_oauth2": False, + "supports_offset": False, + "identifier_quote": {"start": "`", "end": "`"}, + } + ) + + assert result["identifier_quote"] == {"start": "`", "end": "`"} + + +def test_engine_information_schema_includes_identifier_quote_escape_by_doubling() -> ( + None +): + """ + escape_by_doubling tells the client whether an embedded closing-quote + character is escaped by doubling it or with a backslash (e.g. BigQuery). + """ + from superset.databases.schemas import EngineInformationSchema + + schema = EngineInformationSchema() + result = schema.dump( + { + "supports_file_upload": True, + "disable_ssh_tunneling": False, + "supports_dynamic_catalog": False, + "supports_oauth2": False, + "supports_offset": False, + "identifier_quote": { + "start": "`", + "end": "`", + "escape_by_doubling": False, + }, + } + ) + + assert result["identifier_quote"] == { + "start": "`", + "end": "`", + "escape_by_doubling": False, + } diff --git a/tests/unit_tests/db_engine_specs/test_base.py b/tests/unit_tests/db_engine_specs/test_base.py index dfd221dcf08..01f9d46242f 100644 --- a/tests/unit_tests/db_engine_specs/test_base.py +++ b/tests/unit_tests/db_engine_specs/test_base.py @@ -1480,3 +1480,13 @@ def test_get_parameters_from_uri_keeps_unrelated_query_parameters() -> None: ) assert parameters["query"] == {"application_name": "superset"} + + +def test_get_public_information_exposes_ansi_identifier_quote() -> None: + """The base spec advertises ANSI double quotes for identifier quoting, + escaped by doubling the closing character.""" + assert BaseEngineSpec.get_public_information()["identifier_quote"] == { + "start": '"', + "end": '"', + "escape_by_doubling": True, + } diff --git a/tests/unit_tests/db_engine_specs/test_bigquery.py b/tests/unit_tests/db_engine_specs/test_bigquery.py index a8402bc8bb4..a1afed11b98 100644 --- a/tests/unit_tests/db_engine_specs/test_bigquery.py +++ b/tests/unit_tests/db_engine_specs/test_bigquery.py @@ -1101,3 +1101,15 @@ def test_monkeypatch_handles_missing_bigquery_package() -> None: with mock.patch("builtins.__import__", side_effect=mock_import): # Should not raise — the except ImportError branch handles it _monkeypatch_bigquery_string_literal() + + +def test_identifier_quote_uses_backticks() -> None: + """BigQuery quotes identifiers with backticks, not ANSI double quotes, and + escapes an embedded backtick with a backslash rather than by doubling it.""" + from superset.db_engine_specs.bigquery import BigQueryEngineSpec + + assert BigQueryEngineSpec.get_public_information()["identifier_quote"] == { + "start": "`", + "end": "`", + "escape_by_doubling": False, + } diff --git a/tests/unit_tests/db_engine_specs/test_databricks.py b/tests/unit_tests/db_engine_specs/test_databricks.py index e8fd643a229..a963a764d57 100644 --- a/tests/unit_tests/db_engine_specs/test_databricks.py +++ b/tests/unit_tests/db_engine_specs/test_databricks.py @@ -1092,3 +1092,20 @@ def test_get_oauth2_fresh_token_python( }, timeout=30.0, ) + + +def test_identifier_quote_uses_backticks() -> None: + """Databricks SQL is Spark SQL under the hood, so identifiers are quoted + with backticks, not the inherited ANSI double quotes.""" + assert DatabricksNativeEngineSpec.get_public_information()["identifier_quote"] == { + "start": "`", + "end": "`", + "escape_by_doubling": True, + } + assert DatabricksPythonConnectorEngineSpec.get_public_information()[ + "identifier_quote" + ] == { + "start": "`", + "end": "`", + "escape_by_doubling": True, + } diff --git a/tests/unit_tests/db_engine_specs/test_hive.py b/tests/unit_tests/db_engine_specs/test_hive.py index 56b059b3ead..e9b2cbec8de 100644 --- a/tests/unit_tests/db_engine_specs/test_hive.py +++ b/tests/unit_tests/db_engine_specs/test_hive.py @@ -251,3 +251,26 @@ def test_partition_query_escapes_identifiers() -> None: database=None, # type: ignore ) assert result == "SHOW PARTITIONS `no_schema_tbl`" + + +def test_identifier_quote_uses_backticks() -> None: + """HiveQL/Spark SQL quote identifiers with backticks, not ANSI double + quotes, unlike the PrestoEngineSpec parent class it otherwise reuses.""" + from superset.db_engine_specs.hive import HiveEngineSpec + + assert HiveEngineSpec.get_public_information()["identifier_quote"] == { + "start": "`", + "end": "`", + "escape_by_doubling": True, + } + + +def test_spark_identifier_quote_uses_backticks() -> None: + """SparkEngineSpec inherits the backtick quoting from HiveEngineSpec.""" + from superset.db_engine_specs.spark import SparkEngineSpec + + assert SparkEngineSpec.get_public_information()["identifier_quote"] == { + "start": "`", + "end": "`", + "escape_by_doubling": True, + } diff --git a/tests/unit_tests/db_engine_specs/test_mssql.py b/tests/unit_tests/db_engine_specs/test_mssql.py index 6e1decf63a0..49bf0580544 100644 --- a/tests/unit_tests/db_engine_specs/test_mssql.py +++ b/tests/unit_tests/db_engine_specs/test_mssql.py @@ -430,3 +430,14 @@ def test_denormalize_name(name: str, expected_result: str): from superset.db_engine_specs.mssql import MssqlEngineSpec as spec # noqa: N813 assert spec.denormalize_name(mssql.dialect(), name) == expected_result + + +def test_identifier_quote_uses_square_brackets() -> None: + """SQL Server quotes identifiers with square brackets.""" + from superset.db_engine_specs.mssql import MssqlEngineSpec + + assert MssqlEngineSpec.get_public_information()["identifier_quote"] == { + "start": "[", + "end": "]", + "escape_by_doubling": True, + } diff --git a/tests/unit_tests/db_engine_specs/test_mysql.py b/tests/unit_tests/db_engine_specs/test_mysql.py index 2ea2137dfce..e9566335110 100644 --- a/tests/unit_tests/db_engine_specs/test_mysql.py +++ b/tests/unit_tests/db_engine_specs/test_mysql.py @@ -390,3 +390,14 @@ def test_compile_timegrain_expression_preserves_date_truncation() -> None: assert expected in proxied, ( f"DATE_FORMAT truncation was dropped in proxied expression. Got: {proxied}" ) + + +def test_identifier_quote_uses_backticks() -> None: + """MySQL/MariaDB quote identifiers with backticks.""" + from superset.db_engine_specs.mysql import MySQLEngineSpec + + assert MySQLEngineSpec.get_public_information()["identifier_quote"] == { + "start": "`", + "end": "`", + "escape_by_doubling": True, + } diff --git a/tests/unit_tests/db_engine_specs/test_sybase.py b/tests/unit_tests/db_engine_specs/test_sybase.py new file mode 100644 index 00000000000..9878cf68b8d --- /dev/null +++ b/tests/unit_tests/db_engine_specs/test_sybase.py @@ -0,0 +1,29 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + + +def test_identifier_quote_uses_square_brackets() -> None: + """SAP ASE's `quoted_identifier` option defaults to OFF, so double quotes + aren't safe for autocomplete-inserted identifiers. Square brackets work + unconditionally, so SybaseEngineSpec keeps the MssqlEngineSpec default.""" + from superset.db_engine_specs.sybase import SybaseEngineSpec + + assert SybaseEngineSpec.get_public_information()["identifier_quote"] == { + "start": "[", + "end": "]", + "escape_by_doubling": True, + } diff --git a/tests/unit_tests/sqllab/test_schemas.py b/tests/unit_tests/sqllab/test_schemas.py new file mode 100644 index 00000000000..2c924c077a3 --- /dev/null +++ b/tests/unit_tests/sqllab/test_schemas.py @@ -0,0 +1,54 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +def test_sqllab_bootstrap_database_schema_includes_engine_information() -> None: + """ + The OpenAPI contract for `GET /api/v1/sqllab/` documents `engine_information` + (including `identifier_quote`) on each database entry, matching what + `bootstrap_sqllab_data` actually returns. + """ + from superset.sqllab.schemas import SQLLabBootstrapDatabaseSchema + + schema = SQLLabBootstrapDatabaseSchema() + result = schema.dump( + { + "id": 1, + "database_name": "my_database", + "backend": "mysql", + "allow_file_upload": False, + "allow_ctas": False, + "allow_cvas": False, + "allow_dml": False, + "allow_run_async": False, + "allow_multi_catalog": False, + "allows_cost_estimate": None, + "allows_subquery": True, + "allows_virtual_table_explore": True, + "disable_data_preview": False, + "disable_drill_to_detail": False, + "expose_in_sqllab": True, + "force_ctas_schema": None, + "engine_information": { + "supports_offset": True, + "identifier_quote": {"start": "`", "end": "`"}, + }, + } + ) + + assert result["engine_information"] == { + "supports_offset": True, + "identifier_quote": {"start": "`", "end": "`"}, + } diff --git a/tests/unit_tests/sqllab/test_utils.py b/tests/unit_tests/sqllab/test_utils.py new file mode 100644 index 00000000000..daf8a361764 --- /dev/null +++ b/tests/unit_tests/sqllab/test_utils.py @@ -0,0 +1,52 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from unittest import mock + +from superset.sqllab.utils import bootstrap_sqllab_data + + +def test_bootstrap_sqllab_data_includes_non_ansi_identifier_quote() -> None: + """ + The ``/api/v1/sqllab/`` bootstrap payload (backed by this function) must + surface a database's dialect-specific identifier_quote characters, not + just the ANSI default, since it seeds the initial autocomplete store + before the async database-list fetch refreshes it. + """ + database = mock.MagicMock() + database.id = 1 + # `to_json()` (Flask-AppBuilder's column-based serializer) doesn't know + # about the computed `engine_information` property, mirroring production: + # `bootstrap_sqllab_data` falls back to `getattr(database, k)` for it. + database.to_json.return_value = {} + database.engine_information = { + "supports_offset": True, + "identifier_quote": {"start": "`", "end": "`"}, + } + database.allows_virtual_table_explore = True + + with ( + mock.patch( + "superset.sqllab.utils.DatabaseDAO.find_all", return_value=[database] + ), + mock.patch("superset.sqllab.utils.is_feature_enabled", return_value=False), + ): + result = bootstrap_sqllab_data(user_id=1) + + assert result["databases"][1]["engine_information"] == { + "supports_offset": True, + "identifier_quote": {"start": "`", "end": "`"}, + }