Skip to content

fix(sqllab): quote autocomplete table names that need it #41199

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jesperct wants to merge 1 commit into
base: master
Choose a base branch
from
+69 −3
Open

fix(sqllab): quote autocomplete table names that need it #41199

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 60 additions & 2 deletions superset-frontend/src/SqlLab/components/EditorWrapper/useKeywords.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,13 @@ const fakeTableApiResult = {
result: [
{
id: 1,
value: 'fake api result1',
value: 'fake_api_result1',
label: 'fake api label1',
type: 'table',
},
{
id: 2,
value: 'fake api result2',
value: 'fake_api_result2',
label: 'fake api label2',
type: 'table',
},
Expand Down Expand Up @@ -152,6 +152,64 @@ test('returns keywords including fetched function_names data', async () => {
});
});

test('quotes table identifiers that require quoting in the inserted value', async () => {
const dbFunctionNamesApiRoute = `glob:*/api/v1/database/${expectDbId}/function_names/`;
fetchMock.get(dbFunctionNamesApiRoute, fakeFunctionNamesApiResult);

act(() => {
store.dispatch(
tableApiUtil.upsertQueryData(
'tables',
{ dbId: expectDbId, schema: expectSchema },
{
options: [
{ value: 'COVID Vaccines', label: 'COVID Vaccines', type: 'table' },
{ value: 'simple_table', label: 'simple_table', type: 'table' },
],
hasMore: false,
},
),
);
});

const { result } = renderHook(
() =>
useKeywords({
queryEditorId: 'testqueryid',
dbId: expectDbId,
schema: expectSchema,
}),
{
wrapper: createWrapper({
useRedux: true,
store,
}),
},
);

await waitFor(() =>
expect(fetchMock.callHistory.calls(dbFunctionNamesApiRoute).length).toBe(1),
);

// A name that needs quoting is inserted as a double-quoted identifier,
// while its display name stays human-readable.
expect(result.current).toContainEqual(
expect.objectContaining({
name: 'COVID Vaccines',
value: '"COVID Vaccines"',
meta: 'table',
}),
);
// A simple identifier is inserted as-is, without quotes.
expect(result.current).toContainEqual(
expect.objectContaining({
name: 'simple_table',
value: 'simple_table',
meta: 'table',
}),
);
});

test('skip fetching if autocomplete skipped', () => {
const { result } = renderHook(
() =>
Expand Down
10 changes: 9 additions & 1 deletion superset-frontend/src/SqlLab/components/EditorWrapper/useKeywords.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,14 @@ const getHelperText = (value: string) =>
detail: value,
};

// Names that aren't simple identifiers (spaces, punctuation, leading digits)
// must be double-quoted to be valid SQL, with embedded quotes doubled.
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 extensionsRegistry = getExtensionsRegistry();

export function useKeywords(
Expand Down Expand Up @@ -197,7 +205,7 @@ export function useKeywords(
() =>
allCachedTables.map(({ value, label, schema: tableSchema }) => ({
name: label,
value,
value: quoteIdentifier(value),

@codeant-ai-for-open-source codeant-ai-for-open-source Bot Jun 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Using the quoted identifier as the table completion value breaks downstream table metadata flows because this value is also passed to addTable on selection. That stores names like "COVID Vaccines" (with quotes) in pinned table state, and later metadata requests use that quoted string as the name query param, which can fail table lookups that expect the raw table name. Keep autocomplete insertion quoting separate from the canonical table identity used by state/API calls. [api mismatch]

Severity Level: Critical 🚨 
- ❌ SQL Lab table preview fails for quoted-name tables.
- ❌ Table metadata panel shows missing columns for those tables.
- ⚠️ Column autocomplete misses columns from pinned quoted tables.
Steps of Reproduction ✅ 
1. Open SQL Lab editor, which renders `EditorWrapper` at
`superset-frontend/src/SqlLab/components/EditorWrapper/index.tsx:104-120` and passes
`keywords={keywords}` from `useKeywords(...)` at lines 26-35 and 382.

2. `useKeywords` builds `tableKeywords` in
`superset-frontend/src/SqlLab/components/EditorWrapper/useKeywords.ts:204-210`, setting
each table keyword to `{ name: label, value: quoteIdentifier(value), schema: tableSchema,
meta: 'table', completer: { insertMatch } }`, so a table like `COVID Vaccines` produces
`value: "\"COVID Vaccines\""`.

3. In the editor, trigger autocomplete and select the `COVID Vaccines` table completion;
Ace calls `insertMatch` defined in `useKeywords.ts:165-187`, which for `data.meta ===
'table'` dispatches `addTable(..., data.value, catalog ?? null, data.schema ?? schema ??
'', false)` using the quoted `data.value` (`"COVID Vaccines"`) as the `tableName`.

4. `addTable` in `superset-frontend/src/SqlLab/actions/sqlLab.ts:22-37` stores this quoted
`tableName` as `table.name`, and later `SouthPane` at
`superset-frontend/src/SqlLab/components/SouthPane/index.tsx:82-103` passes
`tableName={name}` into `TablePreview`, which uses `tableName` as the `table` parameter to
`useTableMetadataQuery` (`TablePreview/index.tsx:136-143`), leading `tables.ts:14-19` to
call `/api/v1/database/{dbId}/table_metadata/?name="%22COVID%20Vaccines%22"&schema=...`;
because the backend expects the raw table name (`COVID Vaccines`) without quotes, metadata
lookups and previews for tables pinned via autocomplete can fail.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** superset-frontend/src/SqlLab/components/EditorWrapper/useKeywords.ts
**Line:** 208:208
**Comment:**
	*Api Mismatch: Using the quoted identifier as the table completion `value` breaks downstream table metadata flows because this `value` is also passed to `addTable` on selection. That stores names like `"COVID Vaccines"` (with quotes) in pinned table state, and later metadata requests use that quoted string as the `name` query param, which can fail table lookups that expect the raw table name. Keep autocomplete insertion quoting separate from the canonical table identity used by state/API calls.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

@jesperct jesperct Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not apply on the Ace editor path. toAceKeyword in AceEditorProvider.tsx copies only name/value/score/meta/docText/docHTML and drops completer, so insertMatch never runs and addTable is not called on selection. Ace inserts value directly, which is why the fix quotes value. No quoted name reaches table state or the metadata API through this path. The addTable side effect on select is already inactive for the same reason, independent of this change.

@codeant-ai-for-open-source codeant-ai-for-open-source Bot Jun 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Customized review instruction saved!

Instruction:

Do not flag quoted table completion values in the Ace editor path when toAceKeyword strips completer; in that flow insertMatch does not run, so autocomplete insertion does not call addTable or store quoted names in table state/API calls.

Applied to:

  • superset-frontend/src/SqlLab/components/AceEditorProvider.tsx
  • superset-frontend/src/SqlLab/components/EditorWrapper/**

💡 To manage or update this instruction, visit: CodeAnt AI Settings

schema: tableSchema,
score: TABLE_AUTOCOMPLETE_SCORE,
meta: 'table',
Expand Down
Loading