This is an automated email from the ASF dual-hosted git repository. michaelsmolina pushed a commit to branch 5.0 in repository https://gitbox.apache.org/repos/asf/superset.git
commit ddecaa4c11a298b43d6c480f41f3410eae8ae3ec Author: JUST.in DO IT <[email protected]> AuthorDate: Tue Apr 8 07:12:03 2025 -0700 fix(log): Missing failed query log on async queries (#33024) (cherry picked from commit 9b15e04bc4bc7729842902dff1c06e940745fcc9) --- superset-frontend/spec/fixtures/mockDatabases.ts | 48 +++++++++++++++ superset-frontend/src/SqlLab/actions/sqlLab.js | 30 +++++----- .../src/SqlLab/actions/sqlLab.test.js | 10 +--- .../QueryAutoRefresh/QueryAutoRefresh.test.tsx | 69 ++++++++++++++++++++-- .../SqlLab/components/QueryAutoRefresh/index.tsx | 30 +++++++++- 5 files changed, 159 insertions(+), 28 deletions(-) diff --git a/superset-frontend/spec/fixtures/mockDatabases.ts b/superset-frontend/spec/fixtures/mockDatabases.ts new file mode 100644 index 0000000000..8d3edc3783 --- /dev/null +++ b/superset-frontend/spec/fixtures/mockDatabases.ts @@ -0,0 +1,48 @@ +/** + * 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. + */ + +export default { + 1: { + allow_ctas: false, + allow_cvas: false, + allow_dml: false, + allow_file_upload: false, + allow_run_async: true, + backend: 'postgresql', + database_name: 'examples', + expose_in_sqllab: true, + force_ctas_schema: null, + id: 1, + }, +}; + +export const disabledAsyncDb = { + 21: { + allow_ctas: false, + allow_cvas: false, + allow_dml: false, + allow_file_upload: false, + allow_run_async: false, + backend: 'postgresql', + database_name: 'examples', + expose_in_sqllab: true, + force_ctas_schema: null, + id: 21, + }, +}; diff --git a/superset-frontend/src/SqlLab/actions/sqlLab.js b/superset-frontend/src/SqlLab/actions/sqlLab.js index 9f62d781ed..8b54319a56 100644 --- a/superset-frontend/src/SqlLab/actions/sqlLab.js +++ b/superset-frontend/src/SqlLab/actions/sqlLab.js @@ -252,28 +252,30 @@ export function querySuccess(query, results) { return { type: QUERY_SUCCESS, query, results }; } -export function queryFailed(query, msg, link, errors) { +export function logFailedQuery(query, errors) { return function (dispatch) { const eventData = { has_err: true, start_offset: query.startDttm, ts: new Date().getTime(), }; - errors?.forEach(({ error_type: errorType, extra }) => { - const messages = extra?.issue_codes?.map(({ message }) => message) || [ - errorType, - ]; - messages.forEach(message => { - dispatch( - logEvent(LOG_ACTIONS_SQLLAB_FETCH_FAILED_QUERY, { - ...eventData, - error_type: errorType, - error_details: message, - }), - ); - }); + errors?.forEach(({ error_type: errorType, message, extra }) => { + const issueCodes = extra?.issue_codes?.map(({ code }) => code) || [-1]; + dispatch( + logEvent(LOG_ACTIONS_SQLLAB_FETCH_FAILED_QUERY, { + ...eventData, + error_type: errorType, + issue_codes: issueCodes, + error_details: message, + }), + ); }); + }; +} +export function queryFailed(query, msg, link, errors) { + return function (dispatch) { + dispatch(logFailedQuery(query, errors)); dispatch({ type: QUERY_FAILED, query, msg, link, errors }); }; } diff --git a/superset-frontend/src/SqlLab/actions/sqlLab.test.js b/superset-frontend/src/SqlLab/actions/sqlLab.test.js index abbdb0c99e..d01bfd1b1c 100644 --- a/superset-frontend/src/SqlLab/actions/sqlLab.test.js +++ b/superset-frontend/src/SqlLab/actions/sqlLab.test.js @@ -294,7 +294,7 @@ describe('async actions', () => { }); it('calls queryFailed on fetch error and logs the error details', () => { - expect.assertions(3); + expect.assertions(2); fetchMock.post( runQueryEndpoint, @@ -312,7 +312,6 @@ describe('async actions', () => { const expectedActionTypes = [ actions.START_QUERY, LOG_EVENT, - LOG_EVENT, actions.QUERY_FAILED, ]; const { dispatch } = store; @@ -320,12 +319,7 @@ describe('async actions', () => { return request(dispatch, () => initialState).then(() => { const actions = store.getActions(); expect(actions.map(a => a.type)).toEqual(expectedActionTypes); - expect(actions[1].payload.eventData.error_details).toContain( - 'Issue 1000', - ); - expect(actions[2].payload.eventData.error_details).toContain( - 'Issue 1001', - ); + expect(actions[1].payload.eventData.issue_codes).toEqual([1000, 1001]); }); }); }); diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx index 68353ad1f5..7a35adccc4 100644 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx @@ -16,10 +16,12 @@ * specific language governing permissions and limitations * under the License. */ +import { QueryState } from '@superset-ui/core'; import fetchMock from 'fetch-mock'; import configureStore from 'redux-mock-store'; import thunk from 'redux-thunk'; import { render, waitFor } from 'spec/helpers/testing-library'; +import { LOG_ACTIONS_SQLLAB_FETCH_FAILED_QUERY } from 'src/logger/LogUtils'; import { CLEAR_INACTIVE_QUERIES, REFRESH_QUERIES, @@ -31,9 +33,13 @@ import QueryAutoRefresh, { } from 'src/SqlLab/components/QueryAutoRefresh'; import { successfulQuery, runningQuery } from 'src/SqlLab/fixtures'; import { QueryDictionary } from 'src/SqlLab/types'; +import mockDatabases from 'spec/fixtures/mockDatabases'; const middlewares = [thunk]; const mockStore = configureStore(middlewares); +const mockState = { + databases: mockDatabases, +}; // NOTE: The uses of @ts-ignore in this file is to enable testing of bad inputs to verify the // function / component handles bad data elegantly @@ -106,7 +112,9 @@ describe('QueryAutoRefresh', () => { }); it('Attempts to refresh when given pending query', async () => { - const store = mockStore(); + const store = mockStore({ + sqlLab: { ...mockState }, + }); fetchMock.get(refreshApi, { result: [ { @@ -135,7 +143,7 @@ describe('QueryAutoRefresh', () => { }); it('Attempts to clear inactive queries when updated queries are empty', async () => { - const store = mockStore(); + const store = mockStore({ sqlLab: { ...mockState } }); fetchMock.get(refreshApi, { result: [], }); @@ -163,7 +171,7 @@ describe('QueryAutoRefresh', () => { }); it('Does not fail and attempts to refresh when given pending query and invalid query', async () => { - const store = mockStore(); + const store = mockStore({ sqlLab: { ...mockState } }); fetchMock.get(refreshApi, { result: [ { @@ -193,7 +201,7 @@ describe('QueryAutoRefresh', () => { }); it('Does NOT Attempt to refresh when given only completed queries', async () => { - const store = mockStore(); + const store = mockStore({ sqlLab: { ...mockState } }); fetchMock.get(refreshApi, { result: [ { @@ -220,4 +228,57 @@ describe('QueryAutoRefresh', () => { ); expect(fetchMock.calls(refreshApi)).toHaveLength(0); }); + + it('logs the failed error for async queries', async () => { + const store = mockStore({ sqlLab: { ...mockState } }); + fetchMock.get(refreshApi, { + result: [ + { + id: runningQuery.id, + dbId: 1, + state: QueryState.Failed, + extra: { + errors: [ + { + error_type: 'TEST_ERROR', + level: 'error', + message: 'Syntax invalid', + extra: { + issue_codes: [ + { + code: 102, + message: 'DB failed', + }, + ], + }, + }, + ], + }, + }, + ], + }); + render( + <QueryAutoRefresh + queries={runningQueries} + queriesLastUpdate={queriesLastUpdate} + />, + { useRedux: true, store }, + ); + await waitFor( + () => + expect(store.getActions()).toContainEqual( + expect.objectContaining({ + payload: expect.objectContaining({ + eventName: LOG_ACTIONS_SQLLAB_FETCH_FAILED_QUERY, + eventData: expect.objectContaining({ + error_type: 'TEST_ERROR', + error_details: 'Syntax invalid', + issue_codes: [102], + }), + }), + }), + ), + { timeout: QUERY_UPDATE_FREQ + 100 }, + ); + }); }); diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx index ca9906a75f..c7de1208b5 100644 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx @@ -17,7 +17,7 @@ * under the License. */ import { useRef } from 'react'; -import { useDispatch } from 'react-redux'; +import { useSelector, useDispatch } from 'react-redux'; import { isObject } from 'lodash'; import rison from 'rison'; import { @@ -25,13 +25,17 @@ import { Query, runningQueryStateList, QueryResponse, + QueryState, + lruCache, } from '@superset-ui/core'; -import { QueryDictionary } from 'src/SqlLab/types'; +import { QueryDictionary, SqlLabRootState } from 'src/SqlLab/types'; import useInterval from 'src/SqlLab/utils/useInterval'; import { refreshQueries, clearInactiveQueries, + logFailedQuery, } from 'src/SqlLab/actions/sqlLab'; +import type { DatabaseObject } from 'src/features/databases/types'; export const QUERY_UPDATE_FREQ = 2000; const QUERY_UPDATE_BUFFER_MS = 5000; @@ -67,6 +71,17 @@ function QueryAutoRefresh({ // pendingRequest check ensures we only have one active http call to check for query statuses const pendingRequestRef = useRef(false); const cleanInactiveRequestRef = useRef(false); + const failedQueries = useRef(lruCache(1000)); + const databases = useSelector<SqlLabRootState>( + ({ sqlLab }) => sqlLab.databases, + ) as Record<string, DatabaseObject>; + const asyncFetchDbs = useRef( + new Set( + Object.values(databases) + .filter(({ allow_run_async }) => Boolean(allow_run_async)) + .map(({ id }) => id), + ), + ); const dispatch = useDispatch(); const checkForRefresh = () => { @@ -97,6 +112,17 @@ function QueryAutoRefresh({ {}, ) ?? {}; dispatch(refreshQueries(queries)); + jsonPayload.result.forEach(query => { + const { id, dbId, state } = query; + if ( + asyncFetchDbs.current.has(dbId) && + !failedQueries.current.has(id) && + state === QueryState.Failed + ) { + dispatch(logFailedQuery(query, query.extra?.errors)); + failedQueries.current.set(id, true); + } + }); } else { dispatch(clearInactiveQueries(QUERY_UPDATE_FREQ)); }
