sadpandajoe commented on code in PR #45313: URL: https://github.com/apache/superset/pull/45313#discussion_r4449765388
########## superset-frontend/src/SqlLab/components/SqlEditor/useQueryCostWarning.ts: ########## @@ -0,0 +1,259 @@ +/** + * 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 { + useCallback, + useEffect, + useLayoutEffect, + useRef, + useState, +} from 'react'; +import { useStore } from 'react-redux'; +import { t } from '@apache-superset/core/translation'; +import { + FeatureFlag, + getClientErrorObject, + isFeatureEnabled, + SupersetClient, +} from '@superset-ui/core'; +import type { DatabaseObject } from 'src/features/databases/types'; +import { addDangerToast } from 'src/components/MessageToasts/actions'; +import { + CtasEnum, + buildSqlEditorQuery, + getUpToDateQuery, + Query, + runQuery, + setActiveSouthPaneTab, +} from 'src/SqlLab/actions/sqlLab'; +import { useAppDispatch } from 'src/SqlLab/hooks/useAppDispatch'; +import type { QueryEditor, SqlLabRootState } from 'src/SqlLab/types'; + +export type QueryWarningConfirmation = { + action: 'confirm'; + reason: 'warning' | 'unavailable'; + message: string; + details?: { label: string; value: string }[]; +}; +type CheckResult = QueryWarningConfirmation | { action: 'proceed' }; +type Attempt = { query: Query; key: string; result?: QueryWarningConfirmation }; + +/** Reject incomplete responses rather than treating them as permission to execute. */ +export function parseWarningResult(result: unknown): CheckResult { + const invalid = () => new Error(t('The query warning response was invalid.')); + if (!result || typeof result !== 'object' || Array.isArray(result)) + throw invalid(); + const value = result as Record<string, unknown>; + if (value.action === 'proceed') return { action: 'proceed' }; + if ( + value.action !== 'confirm' || + (value.reason !== 'warning' && value.reason !== 'unavailable') || + typeof value.message !== 'string' || + !value.message.trim() || + (value.details !== undefined && + (!Array.isArray(value.details) || + !value.details.every( + (detail: unknown) => + detail !== null && + typeof detail === 'object' && + !Array.isArray(detail) && + 'label' in detail && + 'value' in detail && + typeof detail.label === 'string' && + typeof detail.value === 'string', + ))) + ) + throw invalid(); + return value as QueryWarningConfirmation; +} + +/** Own one captured Run attempt; cancellation invalidates callbacks, not database jobs. */ +export default function useQueryCostWarning( + queryEditor: QueryEditor, + defaultQueryLimit: number, +) { + const store = useStore<SqlLabRootState>(); + const dispatch = useAppDispatch(); + const activeAttemptRef = useRef<Attempt>(); + const mounted = useRef(false); + const editorInputsRef = useRef({ queryEditor, defaultQueryLimit }); + const [pendingAttempt, setPendingAttempt] = useState<Attempt>(); + + const readInputs = useCallback(() => { + const state = store.getState(); + const editorInputs = editorInputsRef.current; + const editor = getUpToDateQuery(state, editorInputs.queryEditor); + const database: DatabaseObject | undefined = + state.sqlLab.databases[editor.dbId || '']; + const enabled = isFeatureEnabled(FeatureFlag.SqlLabQueryCostWarnings); + const key = JSON.stringify({ + id: editor.id, + active: state.sqlLab.tabHistory.slice(-1)[0], + sql: editor.sql, + selectedText: editor.selectedText, + dbId: editor.dbId, + catalog: editor.catalog, + schema: editor.schema, + templateParams: editor.templateParams, + limit: editor.queryLimit || editorInputs.defaultQueryLimit, + async: database?.allow_run_async, + tab: editor.name, + tabViewId: editor.tabViewId, Review Comment: `tabViewId` is part of the attempt key, but with backend persistence `syncQueryEditor` assigns it after the tab is created (`tabViewId: json.id.toString()`). If Run is clicked on a new or duplicated tab before that sync lands, the store subscription sees a different key and calls `cancel()`: the dialog disappears, and a late `proceed` response executes nothing even though the user asked to run. Should the key use an identifier that does not change when the tab is persisted, such as `immutableId`, rather than `tabViewId`? ########## superset-frontend/src/SqlLab/components/SqlEditor/useQueryCostWarning.ts: ########## @@ -0,0 +1,259 @@ +/** + * 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 { + useCallback, + useEffect, + useLayoutEffect, + useRef, + useState, +} from 'react'; +import { useStore } from 'react-redux'; +import { t } from '@apache-superset/core/translation'; +import { + FeatureFlag, + getClientErrorObject, + isFeatureEnabled, + SupersetClient, +} from '@superset-ui/core'; +import type { DatabaseObject } from 'src/features/databases/types'; +import { addDangerToast } from 'src/components/MessageToasts/actions'; +import { + CtasEnum, + buildSqlEditorQuery, + getUpToDateQuery, + Query, + runQuery, + setActiveSouthPaneTab, +} from 'src/SqlLab/actions/sqlLab'; +import { useAppDispatch } from 'src/SqlLab/hooks/useAppDispatch'; +import type { QueryEditor, SqlLabRootState } from 'src/SqlLab/types'; + +export type QueryWarningConfirmation = { + action: 'confirm'; + reason: 'warning' | 'unavailable'; + message: string; + details?: { label: string; value: string }[]; +}; +type CheckResult = QueryWarningConfirmation | { action: 'proceed' }; +type Attempt = { query: Query; key: string; result?: QueryWarningConfirmation }; + +/** Reject incomplete responses rather than treating them as permission to execute. */ +export function parseWarningResult(result: unknown): CheckResult { + const invalid = () => new Error(t('The query warning response was invalid.')); + if (!result || typeof result !== 'object' || Array.isArray(result)) + throw invalid(); + const value = result as Record<string, unknown>; + if (value.action === 'proceed') return { action: 'proceed' }; + if ( + value.action !== 'confirm' || + (value.reason !== 'warning' && value.reason !== 'unavailable') || + typeof value.message !== 'string' || + !value.message.trim() || + (value.details !== undefined && + (!Array.isArray(value.details) || + !value.details.every( + (detail: unknown) => + detail !== null && + typeof detail === 'object' && + !Array.isArray(detail) && + 'label' in detail && + 'value' in detail && + typeof detail.label === 'string' && + typeof detail.value === 'string', + ))) + ) + throw invalid(); + return value as QueryWarningConfirmation; +} + +/** Own one captured Run attempt; cancellation invalidates callbacks, not database jobs. */ +export default function useQueryCostWarning( + queryEditor: QueryEditor, + defaultQueryLimit: number, +) { + const store = useStore<SqlLabRootState>(); + const dispatch = useAppDispatch(); + const activeAttemptRef = useRef<Attempt>(); + const mounted = useRef(false); + const editorInputsRef = useRef({ queryEditor, defaultQueryLimit }); + const [pendingAttempt, setPendingAttempt] = useState<Attempt>(); + + const readInputs = useCallback(() => { + const state = store.getState(); + const editorInputs = editorInputsRef.current; + const editor = getUpToDateQuery(state, editorInputs.queryEditor); + const database: DatabaseObject | undefined = + state.sqlLab.databases[editor.dbId || '']; + const enabled = isFeatureEnabled(FeatureFlag.SqlLabQueryCostWarnings); + const key = JSON.stringify({ + id: editor.id, + active: state.sqlLab.tabHistory.slice(-1)[0], + sql: editor.sql, + selectedText: editor.selectedText, + dbId: editor.dbId, + catalog: editor.catalog, + schema: editor.schema, + templateParams: editor.templateParams, + limit: editor.queryLimit || editorInputs.defaultQueryLimit, + async: database?.allow_run_async, + tab: editor.name, + tabViewId: editor.tabViewId, + immutableId: editor.immutableId, + backend: database?.backend, + rule: database?.query_cost_warning, + warningState: database?.query_warning_state, + enabled, + }); + return { + editor, + database, + enabled, + key, + active: state.sqlLab.tabHistory.slice(-1)[0] === editor.id, + }; + }, [store]); + + const cancel = useCallback(() => { + activeAttemptRef.current = undefined; + setPendingAttempt(undefined); + }, []); + const isCurrent = useCallback( + (attempt: Attempt) => + mounted.current && + activeAttemptRef.current === attempt && + readInputs().key === attempt.key, + [readInputs], + ); + useLayoutEffect(() => { + editorInputsRef.current = { queryEditor, defaultQueryLimit }; + }, [queryEditor, defaultQueryLimit]); + useEffect(() => { + mounted.current = true; + const unsubscribe = store.subscribe(() => { + if (activeAttemptRef.current && !isCurrent(activeAttemptRef.current)) + cancel(); + }); + return () => { + mounted.current = false; + activeAttemptRef.current = undefined; + unsubscribe(); + }; + }, [cancel, isCurrent, store]); + useEffect(() => { + if (activeAttemptRef.current && !isCurrent(activeAttemptRef.current)) + cancel(); + }); + + const execute = useCallback( + (query: Query) => { + dispatch(runQuery(query)); + dispatch(setActiveSouthPaneTab('Results')); + }, + [dispatch], + ); + const continueRun = useCallback(() => { + const attempt = activeAttemptRef.current; + if (!attempt?.result || !isCurrent(attempt)) { + cancel(); + return; + } + cancel(); + execute(attempt.query); + }, [cancel, execute, isCurrent]); + + const start = useCallback( + async ( + tempTable = '', + ctas = false, + ctasMethod: string = CtasEnum.Table, + isAutomaticRun = false, + sql?: string, + ) => { + if (activeAttemptRef.current) return; + const { editor, database, enabled, key, active } = readInputs(); + if (!database || (!active && !isAutomaticRun)) return; + const query = buildSqlEditorQuery( + { id: database.id, allow_run_async: Boolean(database.allow_run_async) }, + sql === undefined ? editor : { ...editor, selectedText: sql }, + editorInputsRef.current.defaultQueryLimit, + tempTable, + ctas, + ctasMethod, + ); + if ( + ctas || + isAutomaticRun || + !enabled || + database.query_warning_state === 'disabled' || + database.query_warning_state === 'unsupported' + ) { + execute(query); + return; + } + const attempt: Attempt = { query, key }; + activeAttemptRef.current = attempt; + setPendingAttempt(attempt); + try { + const { json } = await SupersetClient.post({ Review Comment: Every checked Run now calls `/api/v1/sqllab/estimate/`, which is gated by `can_estimate_query_cost`, while execution is gated by `can_execute_sql_query`. For a custom role that can execute SQL but was never granted estimation, enabling a connection's warning makes every deliberate Run fail with a 403 toast and nothing executes. Should the check treat a permission failure as unavailable and let the user confirm, or is it intended that the warning requires the estimate permission, in which case should the docs say so? -- 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]
