sadpandajoe commented on code in PR #45392: URL: https://github.com/apache/superset/pull/45392#discussion_r4298634364
########## superset-frontend/src/core/clientTools/sqlLab/index.ts: ########## @@ -0,0 +1,238 @@ +/** + * 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. + */ + +import type { chat, sqlLab as sqlLabApi } from '@apache-superset/core'; +import { navigation } from 'src/core/navigation'; +import { sqlLab } from 'src/core/sqlLab'; + +const emptySchema = { + type: 'object', + properties: {}, + additionalProperties: false, +}; +const editorIdSchema = { + type: 'object', + properties: { editor_id: { type: 'string', minLength: 1 } }, + required: ['editor_id'], + additionalProperties: false, +}; + +/** Validates inputs even when a consumer invokes a handler without schema validation. */ +function inputObject(input: unknown): Record<string, unknown> { + if (!input || typeof input !== 'object' || Array.isArray(input)) { + throw new Error('Expected an object input'); + } + return input as Record<string, unknown>; +} + +/** Reads a required string, allowing an empty SQL buffer. */ +function stringInput(input: Record<string, unknown>, key: string): string { + const value = input[key]; + if (typeof value !== 'string' || (key !== 'sql' && !value.trim())) { + throw new Error(`Expected a string for "${key}"`); + } + return value; +} + +/** Resolves the active tab at invocation time. */ +function currentTab(): sqlLabApi.Tab { + const tab = sqlLab.getCurrentTab(); + if (!tab) throw new Error('No active SQL editor tab'); + return tab; +} + +/** Resolves a target before APIs that silently ignore missing tab IDs are called. */ +function targetTab(input: unknown): sqlLabApi.Tab { + const editorId = stringInput(inputObject(input), 'editor_id'); + const tab = sqlLab.getTabs().find(item => item.id === editorId); Review Comment: `targetTab` looks tabs up through `sqlLab.getTabs()`, which drops any query editor whose `dbId` is still `undefined`. With backend-persisted tab state, every inactive tab is hydrated with `dbId: undefined` until it is first activated, so after a reload `get_editor_tabs` lists only the active tab and `switch_editor_tab`/`close_editor_tab` answer `SQL editor "<id>" not found` for tabs the user can see. The agent then cannot reach those tabs at all. Should the lookup fall back to the raw query-editor list (or `switch` hydrate the tab) so persisted-but-unvisited tabs stay addressable? ########## docs/developer_docs/extensions/extension-points/chat.md: ########## @@ -143,6 +143,39 @@ All methods are available on the `chat` namespace from `@apache-superset/core`: | `onDidUnregisterChat(listener)` | Subscribe to unregistration events. | | `onDidResizePanel(listener)` | Subscribe to panel resize events (panel mode only). Not all hosts provide a resizer — do not rely on this firing. Returns a `Disposable`. | +## Built-in SQL Lab Tools + +Use `chat.getTools()` to discover these actions alongside other registered client tools. + +Superset registers these tools at application startup, before loading extensions. +They remain discoverable on every page; invoking them outside SQL Lab returns +`{ success: false, message }`. Open SQL Lab before calling them. SQL Lab handlers +also return this error shape for invalid input, missing tabs, or API errors. Review Comment: This says SQL Lab handlers return `{ success: false, message }` for API errors, but `run_current_query` awaits `sqlLab.executeQuery()`, which dispatches the request and returns the client query ID without waiting on the HTTP result. A failed `/execute` (e.g. invalid SQL) lands in Redux as a query failure while the tool still returns `success: true`, so a caller following this page will treat the run as started and never see the error. Could this be reworded to say the result only confirms the run was initiated and point at query-completion/failure events for the outcome? The 'reports API errors' test in `sqlLab/index.test.ts` mocks a rejected `executeQuery`, which real code does not produce. ########## superset-frontend/src/extensions/ExtensionsStartup.tsx: ########## @@ -67,6 +69,24 @@ const ExtensionsStartup: React.FC<{ children?: React.ReactNode }> = ({ views, }; + // Guarded to run once per mount: this effect re-runs on every userId + // change (e.g. switching accounts without a full reload), but the core + // tool list itself never depends on which user is logged in — repeating + // this call would just re-warn-and-overwrite every tool by name and leak + // a fresh, uncaptured Disposable each time. + if (!coreClientToolsRegistered.current) { + coreClientToolsRegistered.current = true; + // "core." is added explicitly here because this call isn't + // extension-scoped (see ExtensionsLoader's per-extension registerClientTool(s) + // rebind, which does this automatically for an extension's own calls). + chat.registerClientTools( Review Comment: Nothing exercises this registration: the SQL Lab tests import the handlers directly and the existing `ExtensionsStartup` tests only check namespace presence. If this call is removed, or accidentally moved under the `EnableExtensions` check below, every test still passes while `chat.getTools()` loses all `core.sqlLab__*` tools for installs with extensions disabled. Could `ExtensionsStartup.test.tsx` mount with a logged-in user and the flag off and assert `chat.getTools()` contains `core.sqlLab__get_current_editor_id`? ########## superset-frontend/src/core/chat/ChatProvider.ts: ########## @@ -190,10 +221,52 @@ class ChatProvider { return this.resizePanelEmitter.subscribe; } + /** + * Registers a single client-side tool — mirrors commands.ts's + * registerCommand exactly: keyed by the tool's own `name` (fully-qualified, + * author-chosen — nothing prefixes or validates it here, same as a + * `Command.id`), warns and overwrites on a duplicate name, and the + * returned Disposable removes it by name unconditionally on dispose. + * + * Registering after registerChat() is safe — nothing here depends on call + * order. (An earlier version of this method warned about that ordering, + * back when ChatPanel snapshotted `chat.getTools()` once at mount via + * `useMemo(..., [])`; the actual fix was moving that snapshot to send-time + * in ChatPanel itself, which made the order genuinely not matter rather + * than just warning about it.) + */ + public registerClientTool(tool: ClientTool): Disposable { + const { name } = tool; + if (this.clientTools.has(name)) { + logging.warn( + `[Superset] Client tool "${name}" is already registered. ` + + 'Overwriting the existing tool.', + ); + } + this.clientTools.set(name, tool); + return new Disposable(() => { + this.clientTools.delete(name); Review Comment: Agreed—`registerClientTool` returns a Disposable that deletes by name only, so disposing the first registration after a duplicate-name overwrite removes the newer tool while its owner still believes it is registered. Would guarding the delete with `this.clientTools.get(name) === tool` cover it? ########## superset-frontend/src/core/clientTools/sqlLab/index.ts: ########## @@ -0,0 +1,238 @@ +/** + * 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. + */ + +import type { chat, sqlLab as sqlLabApi } from '@apache-superset/core'; +import { navigation } from 'src/core/navigation'; +import { sqlLab } from 'src/core/sqlLab'; + +const emptySchema = { + type: 'object', + properties: {}, + additionalProperties: false, +}; +const editorIdSchema = { + type: 'object', + properties: { editor_id: { type: 'string', minLength: 1 } }, + required: ['editor_id'], + additionalProperties: false, +}; + +/** Validates inputs even when a consumer invokes a handler without schema validation. */ +function inputObject(input: unknown): Record<string, unknown> { + if (!input || typeof input !== 'object' || Array.isArray(input)) { + throw new Error('Expected an object input'); + } + return input as Record<string, unknown>; +} + +/** Reads a required string, allowing an empty SQL buffer. */ +function stringInput(input: Record<string, unknown>, key: string): string { + const value = input[key]; + if (typeof value !== 'string' || (key !== 'sql' && !value.trim())) { + throw new Error(`Expected a string for "${key}"`); + } + return value; +} + +/** Resolves the active tab at invocation time. */ +function currentTab(): sqlLabApi.Tab { + const tab = sqlLab.getCurrentTab(); + if (!tab) throw new Error('No active SQL editor tab'); + return tab; +} + +/** Resolves a target before APIs that silently ignore missing tab IDs are called. */ +function targetTab(input: unknown): sqlLabApi.Tab { + const editorId = stringInput(inputObject(input), 'editor_id'); + const tab = sqlLab.getTabs().find(item => item.id === editorId); + if (!tab) throw new Error(`SQL editor "${editorId}" not found`); + return tab; +} + +/** Serializes tab metadata without waiting for inactive editors to mount. */ +function tabMetadata(tab: sqlLabApi.Tab) { + return { + editor_id: tab.id, + title: tab.title, + databaseId: tab.databaseId, + catalog: tab.catalog, + schema: tab.schema, + }; +} + +/** SQL Lab tools backed exclusively by the existing frontend API. */ +const sqlLabTools: chat.ClientTool[] = [ + { + name: 'sqlLab__get_current_editor_id', + description: 'Get the ID of the active SQL Lab editor tab.', + inputSchema: emptySchema, + annotations: { readOnlyHint: true }, + handler: () => ({ success: true, editor_id: currentTab().id }), + }, + { + name: 'sqlLab__get_editor_tabs', + description: + 'List open SQL Lab tabs and their database, catalog, and schema. Switch tabs before reading or editing their SQL.', + inputSchema: emptySchema, + annotations: { readOnlyHint: true }, + handler: () => ({ + success: true, + editors: sqlLab.getTabs().map(tabMetadata), + }), + }, + { + name: 'sqlLab__get_databases', + description: 'List database IDs and names already loaded in SQL Lab.', + inputSchema: emptySchema, + annotations: { readOnlyHint: true }, + handler: () => ({ + success: true, + databases: sqlLab.getDatabases().map(({ id, name }) => ({ id, name })), + }), + }, + { + name: 'sqlLab__get_editor_sql', + description: 'Read the active SQL Lab editor SQL, selection, and metadata.', + inputSchema: emptySchema, + annotations: { readOnlyHint: true }, + handler: async () => { + const tab = currentTab(); + const editor = await tab.getEditor(); Review Comment: Agreed—`getEditor()` only settles when the editor handle registers, and closing a tab never rejects the pending waiter, so `get_editor_sql`/`update_editor_sql` called while a freshly activated tab is still mounting and then closed leaves the tool call pending with no error result (same at the update handler). Should the handlers race the wait against a timeout or tab-close signal? ########## superset-frontend/src/core/clientTools/sqlLab/index.ts: ########## @@ -0,0 +1,238 @@ +/** + * 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. + */ + +import type { chat, sqlLab as sqlLabApi } from '@apache-superset/core'; +import { navigation } from 'src/core/navigation'; +import { sqlLab } from 'src/core/sqlLab'; + +const emptySchema = { + type: 'object', + properties: {}, + additionalProperties: false, +}; +const editorIdSchema = { + type: 'object', + properties: { editor_id: { type: 'string', minLength: 1 } }, + required: ['editor_id'], + additionalProperties: false, +}; + +/** Validates inputs even when a consumer invokes a handler without schema validation. */ +function inputObject(input: unknown): Record<string, unknown> { + if (!input || typeof input !== 'object' || Array.isArray(input)) { + throw new Error('Expected an object input'); + } + return input as Record<string, unknown>; +} + +/** Reads a required string, allowing an empty SQL buffer. */ +function stringInput(input: Record<string, unknown>, key: string): string { + const value = input[key]; + if (typeof value !== 'string' || (key !== 'sql' && !value.trim())) { + throw new Error(`Expected a string for "${key}"`); + } + return value; +} + +/** Resolves the active tab at invocation time. */ +function currentTab(): sqlLabApi.Tab { + const tab = sqlLab.getCurrentTab(); + if (!tab) throw new Error('No active SQL editor tab'); + return tab; +} + +/** Resolves a target before APIs that silently ignore missing tab IDs are called. */ +function targetTab(input: unknown): sqlLabApi.Tab { + const editorId = stringInput(inputObject(input), 'editor_id'); + const tab = sqlLab.getTabs().find(item => item.id === editorId); + if (!tab) throw new Error(`SQL editor "${editorId}" not found`); + return tab; +} + +/** Serializes tab metadata without waiting for inactive editors to mount. */ +function tabMetadata(tab: sqlLabApi.Tab) { + return { + editor_id: tab.id, + title: tab.title, + databaseId: tab.databaseId, + catalog: tab.catalog, + schema: tab.schema, + }; +} + +/** SQL Lab tools backed exclusively by the existing frontend API. */ +const sqlLabTools: chat.ClientTool[] = [ + { + name: 'sqlLab__get_current_editor_id', + description: 'Get the ID of the active SQL Lab editor tab.', + inputSchema: emptySchema, + annotations: { readOnlyHint: true }, + handler: () => ({ success: true, editor_id: currentTab().id }), + }, + { + name: 'sqlLab__get_editor_tabs', + description: + 'List open SQL Lab tabs and their database, catalog, and schema. Switch tabs before reading or editing their SQL.', + inputSchema: emptySchema, + annotations: { readOnlyHint: true }, + handler: () => ({ + success: true, + editors: sqlLab.getTabs().map(tabMetadata), + }), + }, + { + name: 'sqlLab__get_databases', + description: 'List database IDs and names already loaded in SQL Lab.', + inputSchema: emptySchema, + annotations: { readOnlyHint: true }, + handler: () => ({ + success: true, + databases: sqlLab.getDatabases().map(({ id, name }) => ({ id, name })), + }), + }, + { + name: 'sqlLab__get_editor_sql', + description: 'Read the active SQL Lab editor SQL, selection, and metadata.', + inputSchema: emptySchema, + annotations: { readOnlyHint: true }, + handler: async () => { + const tab = currentTab(); + const editor = await tab.getEditor(); + return { + success: true, + editor: { + ...tabMetadata(tab), + sql: editor.getValue(), + selectedText: editor.getSelectedText(), + }, + }; + }, + }, + { + name: 'sqlLab__update_editor_sql', + description: + 'Replace the SQL in the active SQL Lab editor without running or saving it as a saved query.', + inputSchema: { + type: 'object', + properties: { sql: { type: 'string' } }, + required: ['sql'], + additionalProperties: false, + }, + annotations: { readOnlyHint: false, destructiveHint: true }, + handler: async input => { + const sql = stringInput(inputObject(input), 'sql'); + const tab = currentTab(); + const editor = await tab.getEditor(); + editor.setValue(sql); + return { success: true, editor_id: tab.id }; + }, + }, + { + name: 'sqlLab__create_editor_tab', + description: + 'Open a SQL Lab tab without executing SQL. Use get_databases to find a database ID.', + inputSchema: { + type: 'object', + properties: { + title: { type: 'string', minLength: 1 }, + databaseId: { type: 'integer', minimum: 1 }, + sql: { type: 'string' }, + catalog: { type: 'string' }, + schema: { type: 'string' }, + }, + required: ['title', 'databaseId'], + additionalProperties: false, + }, + annotations: { readOnlyHint: false, destructiveHint: false }, + handler: async input => { + const args = inputObject(input); + const title = stringInput(args, 'title'); + const { databaseId } = args; + if ( + typeof databaseId !== 'number' || + !Number.isInteger(databaseId) || + !sqlLab.getDatabases().some(db => db.id === databaseId) + ) { + throw new Error('Expected a databaseId available in SQL Lab'); + } + const options: sqlLabApi.CreateTabOptions = { title, databaseId }; Review Comment: Agreed—`createTab` fills catalog and schema from the active tab independently of the `databaseId` override, so creating a tab for database B while A/`private` is active gives B a namespace that may not exist there. Should the tool drop inherited catalog/schema when `databaseId` differs from the active tab's? -- 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]
