korbit-ai[bot] commented on code in PR #35119: URL: https://github.com/apache/superset/pull/35119#discussion_r2344529034
########## superset-frontend/src/core/sqlLab/models.ts: ########## @@ -0,0 +1,235 @@ +/** + * 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 { sqlLab as sqlLabType, core as coreType } from '@apache-superset/core'; + +const { CTASMethod } = sqlLabType; + +export class Panel implements sqlLabType.Panel { + id: string; + + constructor(id: string) { + this.id = id; + } +} + +export class Editor implements sqlLabType.Editor { + content: string; + + databaseId: number; + + schema: string; + + // TODO: Check later if we'll use objects instead of strings. + catalog: string | null; + + table: string | null; + + constructor( + content: string, + databaseId: number, + catalog: string | null = null, + schema = '', + table: string | null = null, + ) { + this.content = content; + this.databaseId = databaseId; + this.catalog = catalog; + this.schema = schema; + this.table = table; + } +} + +export class Tab implements sqlLabType.Tab { + id: string; + + title: string; + + editor: Editor; + + panels: Panel[]; + + constructor(id: string, title: string, editor: Editor, panels: Panel[] = []) { + this.id = id; + this.title = title; + this.editor = editor; + this.panels = panels; + } +} + +export class CTAS implements sqlLabType.CTAS { + method: sqlLabType.CTASMethod; + + tempTable: string; + + constructor(asView: boolean, tempTable: string) { + this.method = asView ? CTASMethod.View : CTASMethod.Table; + this.tempTable = tempTable; + } +} + +export class QueryContext implements sqlLabType.QueryContext { + clientId: string; + + ctas: sqlLabType.CTAS | null; + + editor: Editor; + + requestedLimit: number | null; + + runAsync: boolean; + + startDttm: number; + + tab: Tab; + + private templateParams: string; + + private parsedParams: Record<string, any>; + + constructor( + clientId: string, + tab: Tab, + runAsync: boolean, + startDttm: number, + options: { + templateParams?: string; + ctasMethod?: string; + tempTable?: string; + requestedLimit?: number; + } = {}, + ) { + this.clientId = clientId; + this.tab = tab; + this.editor = tab.editor; + this.runAsync = runAsync; + this.startDttm = startDttm; + this.requestedLimit = options.requestedLimit ?? null; + this.ctas = options.tempTable + ? new CTAS(options.ctasMethod === CTASMethod.View, options.tempTable) + : null; + this.templateParams = options.templateParams ?? ''; + } + + /** + * A custom accessor is used to process JSON parsing only + * when necessary for better performance. + */ + get templateParameters() { + if (this.parsedParams) { + return this.parsedParams; + } + + let parsed = {}; + try { + parsed = JSON.parse(this.templateParams); + } catch (e) { + // ignore invalid format string. + } + this.parsedParams = parsed; + + return parsed; + } +} + +export class QueryResultContext + extends QueryContext + implements sqlLabType.QueryResultContext +{ + appliedLimit: number; + + appliedLimitingFactor: string; + + endDttm: number; + + executedSql: string; + + remoteId: number; + + result: sqlLabType.QueryResult; + + constructor( + clientId: string, + tab: Tab, + runAsync: boolean, + startDttm: number, + remoteId: number, + executedSql: string, + columns: sqlLabType.QueryResult['columns'], + data: sqlLabType.QueryResult['data'], + endDttm: number, + options: { + appliedLimit?: number; + appliedLimitingFactor?: string; + templateParams?: string; + ctasMethod?: string; + tempTable?: string; + requestedLimit?: number; + } = {}, + ) { + const { appliedLimit, appliedLimitingFactor, ...opt } = options; + super(clientId, tab, runAsync, startDttm, opt); + this.remoteId = remoteId; + this.executedSql = executedSql; + this.endDttm = endDttm; + this.result = { + columns, + data, + }; + this.appliedLimit = appliedLimit ?? data.length; + this.appliedLimitingFactor = options.appliedLimitingFactor ?? ''; + } +} + +export class QueryErrorResultContext + extends QueryContext + implements sqlLabType.QueryErrorResultContext +{ + endDttm: number; + + errorMessage: string; + + errors: coreType.SupersetError[] | null; + + executedSql: string | null; + + constructor( + clientId: string, + tab: Tab, + runAsync: boolean, + startDttm: number, + errorMessage: string, + errors: coreType.SupersetError[], + options: { + ctasMethod?: string; + executedSql?: string; + endDttm?: number; + templateParams?: string; + tempTable?: string; + requestedLimist?: number; Review Comment: ### Incorrect parameter name due to typo <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Typo in parameter name 'requestedLimist' in QueryErrorResultContext options interface should be 'requestedLimit'. ###### Why this matters This typo in the parameter name will cause inconsistency with other parts of the codebase where the correct parameter name 'requestedLimit' is used, potentially leading to runtime errors or undefined values when attempting to access this parameter. ###### Suggested change ∙ *Feature Preview* Correct the parameter name in the options interface: ```typescript options: { ... requestedLimit?: number; ... } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a3c5e73b-b115-4ebe-be7d-f2e565b69a74/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a3c5e73b-b115-4ebe-be7d-f2e565b69a74?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a3c5e73b-b115-4ebe-be7d-f2e565b69a74?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a3c5e73b-b115-4ebe-be7d-f2e565b69a74?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a3c5e73b-b115-4ebe-be7d-f2e565b69a74) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:efdaa144-bbac-4c9d-9650-8b31010fc35f --> [](efdaa144-bbac-4c9d-9650-8b31010fc35f) ########## superset-frontend/src/core/sqlLab/index.ts: ########## @@ -0,0 +1,345 @@ +/** + * 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 { sqlLab as sqlLabType } from '@apache-superset/core'; +import { + QUERY_FAILED, + QUERY_SUCCESS, + QUERY_EDITOR_SETDB, + querySuccess, + startQuery, + START_QUERY, + stopQuery, + STOP_QUERY, + createQueryFailedAction, +} from 'src/SqlLab/actions/sqlLab'; +import { RootState, store } from 'src/views/store'; +import { AnyListenerPredicate } from '@reduxjs/toolkit'; +import type { SqlLabRootState } from 'src/SqlLab/types'; +import { Disposable } from '../models'; +import { createActionListener } from '../utils'; +import { + Panel, + Editor, + Tab, + QueryContext, + QueryResultContext, + QueryErrorResultContext, +} from './models'; + +const { CTASMethod } = sqlLabType; + +const getSqlLabState = () => { + const { sqlLab }: { sqlLab: SqlLabRootState['sqlLab'] } = store.getState(); + return sqlLab; +}; + +const activeEditorId = () => { + const { tabHistory } = getSqlLabState(); + return tabHistory[tabHistory.length - 1]; +}; + +const findQueryEditor = (editorId: string) => { + const { queryEditors } = getSqlLabState(); + return queryEditors.find(editor => editor.id === editorId); +}; + +const createTab = ( + id: string, + name: string, + sql: string, + dbId: number, + catalog?: string, + schema?: string, + table?: any, +) => { + const editor = new Editor(sql, dbId, catalog, schema, table); + const panels: Panel[] = []; // TODO: Populate panels Review Comment: ### Missing Panel Population Implementation <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The panels array is being initialized as empty with a TODO comment indicating missing implementation for panel population. ###### Why this matters Without proper panel population, the tab creation functionality will be incomplete and panels may not be available to users as expected in the SQL Lab interface. ###### Suggested change ∙ *Feature Preview* Implement the panel population logic before initializing the Tab object. A suggested implementation would be: ```typescript const createTab = ( id: string, name: string, sql: string, dbId: number, catalog?: string, schema?: string, table?: any, ) => { const editor = new Editor(sql, dbId, catalog, schema, table); const panels = initializePanels(id); // Create a new function to properly initialize panels return new Tab(id, name, editor, panels); }; ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4606ad31-0769-4f57-b3b3-c4ab10352741/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4606ad31-0769-4f57-b3b3-c4ab10352741?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4606ad31-0769-4f57-b3b3-c4ab10352741?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4606ad31-0769-4f57-b3b3-c4ab10352741?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4606ad31-0769-4f57-b3b3-c4ab10352741) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:7f6771aa-b049-4104-8f3b-dfb9e93e732b --> [](7f6771aa-b049-4104-8f3b-dfb9e93e732b) ########## superset-frontend/src/core/sqlLab/index.ts: ########## @@ -0,0 +1,345 @@ +/** + * 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 { sqlLab as sqlLabType } from '@apache-superset/core'; +import { + QUERY_FAILED, + QUERY_SUCCESS, + QUERY_EDITOR_SETDB, + querySuccess, + startQuery, + START_QUERY, + stopQuery, + STOP_QUERY, + createQueryFailedAction, +} from 'src/SqlLab/actions/sqlLab'; +import { RootState, store } from 'src/views/store'; +import { AnyListenerPredicate } from '@reduxjs/toolkit'; +import type { SqlLabRootState } from 'src/SqlLab/types'; +import { Disposable } from '../models'; +import { createActionListener } from '../utils'; +import { + Panel, + Editor, + Tab, + QueryContext, + QueryResultContext, + QueryErrorResultContext, +} from './models'; + +const { CTASMethod } = sqlLabType; + +const getSqlLabState = () => { + const { sqlLab }: { sqlLab: SqlLabRootState['sqlLab'] } = store.getState(); + return sqlLab; +}; + +const activeEditorId = () => { + const { tabHistory } = getSqlLabState(); + return tabHistory[tabHistory.length - 1]; +}; + +const findQueryEditor = (editorId: string) => { + const { queryEditors } = getSqlLabState(); + return queryEditors.find(editor => editor.id === editorId); +}; + +const createTab = ( + id: string, + name: string, + sql: string, + dbId: number, + catalog?: string, + schema?: string, + table?: any, +) => { + const editor = new Editor(sql, dbId, catalog, schema, table); + const panels: Panel[] = []; // TODO: Populate panels + return new Tab(id, name, editor, panels); +}; + +const notImplemented = (): never => { + throw new Error('Not implemented yet'); +}; + +function extractBaseData(action: any): { + baseParams: [string, Tab, boolean, number]; + options: { + ctasMethod?: string; + tempTable?: string; + templateParams?: string; + requestedLimit?: number; + }; +} { + const { query } = action; + const { + id, + sql, + startDttm, + runAsync, + dbId, + catalog, + schema, + sqlEditorId, + tab: tabName, + ctas_method: ctasMethod, + tempTable, + templateParams, + queryLimit, + } = query; + + const tab = createTab(sqlEditorId, tabName, sql, dbId, catalog, schema); + + return { + baseParams: [id, tab, runAsync ?? false, startDttm ?? 0], + options: { + ctasMethod, + tempTable, + templateParams, + requestedLimit: queryLimit, + }, + }; +} + +function createQueryContext( + action: ReturnType<typeof startQuery> | ReturnType<typeof stopQuery>, +): QueryContext { + const { baseParams, options } = extractBaseData(action); + return new QueryContext(...baseParams, options); +} + +function createQueryResultContext( + action: ReturnType<typeof querySuccess>, +): QueryResultContext { + const { baseParams, options } = extractBaseData(action); + const [_, tab] = baseParams; + const { results } = action; + const { + query_id: queryId, + columns, + data, + query: { + endDttm, + executedSql, + tempTable: resultTempTable, + limit, + limitingFactor, + }, + } = results; + + return new QueryResultContext( + ...baseParams, + queryId, + executedSql ?? tab.editor.content, + columns, + data, + endDttm, + { + ...options, + tempTable: resultTempTable || options.tempTable, + appliedLimit: limit, + appliedLimitingFactor: limitingFactor, + }, + ); +} + +function createQueryErrorContext( + action: ReturnType<typeof createQueryFailedAction>, +): QueryErrorResultContext { + const { baseParams, options } = extractBaseData(action); + const { msg: errorMessage, errors, query } = action; + const { endDttm, executedSql, query_id: queryId } = query; + + return new QueryErrorResultContext(...baseParams, errorMessage, errors, { + ...options, + queryId, + executedSql: executedSql ?? null, + endDttm: endDttm ?? Date.now(), + }); +} + +const getCurrentTab: typeof sqlLabType.getCurrentTab = () => { + const queryEditor = findQueryEditor(activeEditorId()); + if (queryEditor) { + const { id, name, sql, dbId, catalog, schema } = queryEditor; + return createTab( + id, + name, + sql, + dbId!, + catalog ?? undefined, + schema ?? undefined, + undefined, + ); + } + return undefined; +}; + +const getActiveEditorImmutableId = () => { + const { tabHistory } = getSqlLabState(); + const activeEditorId = tabHistory[tabHistory.length - 1]; + const activeEditor = findQueryEditor(activeEditorId); + return activeEditor?.immutableId; +}; + +const predicate = (actionType: string): AnyListenerPredicate<RootState> => { + // Capture the immutable ID of the active editor at the time the listener is created + // This ID never changes for a tab, ensuring stable event routing + const registrationImmutableId = getActiveEditorImmutableId(); Review Comment: ### Expensive Store Lookup in Predicate Factory <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The getActiveEditorImmutableId() call involves multiple store lookups and is executed on every predicate function creation. ###### Why this matters Store access operations are relatively expensive. Creating multiple predicates leads to redundant store lookups for the same immutableId. ###### Suggested change ∙ *Feature Preview* Consider memoizing the immutableId lookup or restructuring the code to avoid repeated store access: ```typescript const memoizedGetActiveEditorImmutableId = memoize(getActiveEditorImmutableId); const predicate = (actionType: string): AnyListenerPredicate<RootState> => { const registrationImmutableId = memoizedGetActiveEditorImmutableId(); ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/82f10fcc-5926-43f7-b71f-ab1a72b43f5c/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/82f10fcc-5926-43f7-b71f-ab1a72b43f5c?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/82f10fcc-5926-43f7-b71f-ab1a72b43f5c?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/82f10fcc-5926-43f7-b71f-ab1a72b43f5c?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/82f10fcc-5926-43f7-b71f-ab1a72b43f5c) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:bfcd21c0-724c-413b-9255-551f547e5b54 --> [](bfcd21c0-724c-413b-9255-551f547e5b54) ########## superset-frontend/src/core/sqlLab/models.ts: ########## @@ -0,0 +1,235 @@ +/** + * 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 { sqlLab as sqlLabType, core as coreType } from '@apache-superset/core'; + +const { CTASMethod } = sqlLabType; + +export class Panel implements sqlLabType.Panel { + id: string; + + constructor(id: string) { + this.id = id; + } +} + +export class Editor implements sqlLabType.Editor { + content: string; + + databaseId: number; + + schema: string; + + // TODO: Check later if we'll use objects instead of strings. + catalog: string | null; + + table: string | null; + + constructor( + content: string, + databaseId: number, + catalog: string | null = null, + schema = '', + table: string | null = null, + ) { + this.content = content; + this.databaseId = databaseId; + this.catalog = catalog; + this.schema = schema; + this.table = table; + } +} + +export class Tab implements sqlLabType.Tab { + id: string; + + title: string; + + editor: Editor; + + panels: Panel[]; + + constructor(id: string, title: string, editor: Editor, panels: Panel[] = []) { + this.id = id; + this.title = title; + this.editor = editor; + this.panels = panels; + } +} + +export class CTAS implements sqlLabType.CTAS { + method: sqlLabType.CTASMethod; + + tempTable: string; + + constructor(asView: boolean, tempTable: string) { + this.method = asView ? CTASMethod.View : CTASMethod.Table; + this.tempTable = tempTable; + } +} + +export class QueryContext implements sqlLabType.QueryContext { + clientId: string; + + ctas: sqlLabType.CTAS | null; + + editor: Editor; + + requestedLimit: number | null; + + runAsync: boolean; + + startDttm: number; + + tab: Tab; + + private templateParams: string; + + private parsedParams: Record<string, any>; + + constructor( + clientId: string, + tab: Tab, + runAsync: boolean, + startDttm: number, + options: { + templateParams?: string; + ctasMethod?: string; + tempTable?: string; + requestedLimit?: number; + } = {}, + ) { + this.clientId = clientId; + this.tab = tab; + this.editor = tab.editor; + this.runAsync = runAsync; + this.startDttm = startDttm; + this.requestedLimit = options.requestedLimit ?? null; + this.ctas = options.tempTable + ? new CTAS(options.ctasMethod === CTASMethod.View, options.tempTable) + : null; + this.templateParams = options.templateParams ?? ''; + } + + /** + * A custom accessor is used to process JSON parsing only + * when necessary for better performance. + */ + get templateParameters() { + if (this.parsedParams) { + return this.parsedParams; + } + + let parsed = {}; + try { + parsed = JSON.parse(this.templateParams); + } catch (e) { + // ignore invalid format string. + } + this.parsedParams = parsed; + + return parsed; + } Review Comment: ### Silent failure in template parameter parsing <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The templateParameters getter silently swallows JSON parsing errors and returns an empty object without any logging or error reporting. ###### Why this matters Silent failure in parsing template parameters could mask potential injection attempts or malformed input that should be flagged for security monitoring. Attackers could exploit this to probe for vulnerabilities without detection. ###### Suggested change ∙ *Feature Preview* ```typescript get templateParameters() { if (this.parsedParams) { return this.parsedParams; } let parsed = {}; try { parsed = JSON.parse(this.templateParams); } catch (e) { console.warn('Invalid template parameters format:', e); // Consider adding error telemetry here } this.parsedParams = parsed; return parsed; } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/54d0b0c0-6ae2-44b4-859b-e0b88d10848f/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/54d0b0c0-6ae2-44b4-859b-e0b88d10848f?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/54d0b0c0-6ae2-44b4-859b-e0b88d10848f?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/54d0b0c0-6ae2-44b4-859b-e0b88d10848f?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/54d0b0c0-6ae2-44b4-859b-e0b88d10848f) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:aafb5957-90a5-47e2-92bd-d61d33f1aee3 --> [](aafb5957-90a5-47e2-92bd-d61d33f1aee3) -- 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]
