From dc4570362888d38b03cf25e2f87b06e6c54aae97 Mon Sep 17 00:00:00 2001 From: Eric Briscoe Date: Tue, 24 May 2022 15:29:02 -0700 Subject: [PATCH 01/11] git commit -m 'Convert QueryAutoRefresh to functional component [sc-48362]' --- .../src/SqlLab/components/App/index.jsx | 4 +- .../QueryAutoRefresh.test.jsx | 68 ---------- .../QueryAutoRefresh.test.tsx | 127 ++++++++++++++++++ .../QueryAutoRefreshContainer.tsx | 59 ++++++++ .../components/QueryAutoRefresh/index.jsx | 124 ----------------- .../components/QueryAutoRefresh/index.tsx | 118 ++++++++++++++++ superset-frontend/src/SqlLab/fixtures.ts | 94 ++++++++++++- .../src/SqlLab/utils/useInterval.ts | 47 +++++++ 8 files changed, 446 insertions(+), 195 deletions(-) delete mode 100644 superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.jsx create mode 100644 superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx create mode 100644 superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefreshContainer.tsx delete mode 100644 superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.jsx create mode 100644 superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx create mode 100644 superset-frontend/src/SqlLab/utils/useInterval.ts diff --git a/superset-frontend/src/SqlLab/components/App/index.jsx b/superset-frontend/src/SqlLab/components/App/index.jsx index 8a0ade9509ac6..36c7b5175a617 100644 --- a/superset-frontend/src/SqlLab/components/App/index.jsx +++ b/superset-frontend/src/SqlLab/components/App/index.jsx @@ -30,7 +30,7 @@ import { } from 'src/SqlLab/constants'; import * as Actions from 'src/SqlLab/actions/sqlLab'; import TabbedSqlEditors from '../TabbedSqlEditors'; -import QueryAutoRefresh from '../QueryAutoRefresh'; +import QueryAutoRefreshContainer from '../QueryAutoRefresh/QueryAutoRefreshContainer'; class App extends React.PureComponent { constructor(props) { @@ -99,7 +99,7 @@ class App extends React.PureComponent { } return (
- +
diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.jsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.jsx deleted file mode 100644 index 06bf187e1185a..0000000000000 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.jsx +++ /dev/null @@ -1,68 +0,0 @@ -/** - * 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 React from 'react'; -import { shallow } from 'enzyme'; -import sinon from 'sinon'; -import thunk from 'redux-thunk'; -import configureStore from 'redux-mock-store'; -import QueryAutoRefresh from 'src/SqlLab/components/QueryAutoRefresh'; -import { initialState, runningQuery } from 'src/SqlLab/fixtures'; - -describe('QueryAutoRefresh', () => { - const middlewares = [thunk]; - const mockStore = configureStore(middlewares); - const sqlLab = { - ...initialState.sqlLab, - queries: { - ryhMUZCGb: runningQuery, - }, - }; - const state = { - ...initialState, - sqlLab, - }; - const store = mockStore(state); - const getWrapper = () => - shallow() - .dive() - .dive(); - let wrapper; - - it('shouldCheckForQueries', () => { - wrapper = getWrapper(); - expect(wrapper.instance().shouldCheckForQueries()).toBe(true); - }); - - it('setUserOffline', () => { - wrapper = getWrapper(); - const spy = sinon.spy(wrapper.instance().props.actions, 'setUserOffline'); - - // state not changed - wrapper.setState({ - offline: false, - }); - expect(spy.called).toBe(false); - - // state is changed - wrapper.setState({ - offline: true, - }); - expect(spy.callCount).toBe(1); - }); -}); diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx new file mode 100644 index 0000000000000..def5e1ef2193b --- /dev/null +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx @@ -0,0 +1,127 @@ +/** + * 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 React from 'react'; +import { render } from '@testing-library/react'; +import QueryAutoRefresh, { + isQueryRunning, + shouldCheckForQueries, +} from 'src/SqlLab/components/QueryAutoRefresh'; +import { successfulQuery, runningQuery } from 'src/SqlLab/fixtures'; + +// 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 +describe('QueryAutoRefresh', () => { + const offline = false; + const queries = [runningQuery]; + const actions = { + setUserOffline: jest.fn(), + refreshQueries: jest.fn(), + }; + const queriesLastUpdate = Date.now(); + + it('isQueryRunning returns true for valid running query', () => { + const running = isQueryRunning(runningQuery); + expect(running).toBe(true); + }); + + it('isQueryRunning returns false for valid not-running query', () => { + const running = isQueryRunning(successfulQuery); + expect(running).toBe(false); + }); + + it('isQueryRunning returns false for invalid query', () => { + // @ts-ignore + let running = isQueryRunning(null); + expect(running).toBe(false); + // @ts-ignore + running = isQueryRunning(undefined); + expect(running).toBe(false); + // @ts-ignore + running = isQueryRunning('I Should Be An Object'); + expect(running).toBe(false); + // @ts-ignore + running = isQueryRunning({ state: { badFormat: true } }); + expect(running).toBe(false); + }); + + it('shouldCheckForQueries is true for valid running query', () => { + expect(shouldCheckForQueries([runningQuery])).toBe(true); + }); + + it('shouldCheckForQueries is false for valid completed query', () => { + expect(shouldCheckForQueries([successfulQuery])).toBe(false); + }); + + it('shouldCheckForQueries is false for invalid inputs', () => { + // @ts-ignore + expect(shouldCheckForQueries(null)).toBe(false); + // @ts-ignore + expect(shouldCheckForQueries(undefined)).toBe(false); + expect( + // @ts-ignore + shouldCheckForQueries([null, 'hello world', [], undefined, 23]), + ).toBe(false); + }); + + it('Attempts to refresh when given pending query', () => { + render( + , + ); + setTimeout(() => { + expect(actions.refreshQueries).toHaveBeenCalled(); + expect(actions.setUserOffline).not.toHaveBeenCalled(); + }, 1000); + }); + + it('Does not fail and attempts to refresh when given pending query and invlaid query', () => { + render( + , + ); + setTimeout(() => { + expect(actions.refreshQueries).toHaveBeenCalled(); + expect(actions.setUserOffline).not.toHaveBeenCalled(); + }, 1000); + }); + + it('Does NOT Attempt to refresh when given only completed queries', () => { + render( + , + ); + setTimeout(() => { + expect(actions.refreshQueries).not.toHaveBeenCalled(); + expect(actions.setUserOffline).not.toHaveBeenCalled(); + }, 1000); + }); +}); diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefreshContainer.tsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefreshContainer.tsx new file mode 100644 index 0000000000000..f84030275e986 --- /dev/null +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefreshContainer.tsx @@ -0,0 +1,59 @@ +/** + * 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 React from 'react'; +import { Dispatch, bindActionCreators } from 'redux'; +import { connect } from 'react-redux'; +import * as Actions from 'src/SqlLab/actions/sqlLab'; +import { RootState } from 'src/SqlLab/types'; +import QueryAutoRefresh, { QueryAutoRefreshProps } from '.'; + +function QueryAutoRefreshConnector({ + offline, + queries, + actions, + queriesLastUpdate, +}: QueryAutoRefreshProps) { + return ( + + ); +} + +function mapStateToProps({ sqlLab }: RootState) { + return { + offline: sqlLab.offline, + queries: sqlLab.queries, + queriesLastUpdate: sqlLab.queriesLastUpdate, + }; +} + +function mapDispatchToProps(dispatch: Dispatch) { + return { + actions: bindActionCreators(Actions, dispatch), + }; +} + +export default connect( + mapStateToProps, + mapDispatchToProps, +)(QueryAutoRefreshConnector); diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.jsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.jsx deleted file mode 100644 index b54936b691efe..0000000000000 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.jsx +++ /dev/null @@ -1,124 +0,0 @@ -/** - * 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 React from 'react'; -import PropTypes from 'prop-types'; -import { bindActionCreators } from 'redux'; -import { connect } from 'react-redux'; -import { SupersetClient } from '@superset-ui/core'; -import * as Actions from 'src/SqlLab/actions/sqlLab'; - -const QUERY_UPDATE_FREQ = 2000; -const QUERY_UPDATE_BUFFER_MS = 5000; -const MAX_QUERY_AGE_TO_POLL = 21600000; -const QUERY_TIMEOUT_LIMIT = 10000; - -class QueryAutoRefresh extends React.PureComponent { - constructor(props) { - super(props); - this.state = { - offline: props.offline, - }; - } - - UNSAFE_componentWillMount() { - this.startTimer(); - } - - componentDidUpdate(prevProps) { - if (prevProps.offline !== this.state.offline) { - this.props.actions.setUserOffline(this.state.offline); - } - } - - componentWillUnmount() { - this.stopTimer(); - } - - shouldCheckForQueries() { - // if there are started or running queries, this method should return true - const { queries } = this.props; - const now = new Date().getTime(); - const isQueryRunning = q => - ['running', 'started', 'pending', 'fetching'].indexOf(q.state) >= 0; - - return Object.values(queries).some( - q => isQueryRunning(q) && now - q.startDttm < MAX_QUERY_AGE_TO_POLL, - ); - } - - startTimer() { - if (!this.timer) { - this.timer = setInterval(this.stopwatch.bind(this), QUERY_UPDATE_FREQ); - } - } - - stopTimer() { - clearInterval(this.timer); - this.timer = null; - } - - stopwatch() { - // only poll /superset/queries/ if there are started or running queries - if (this.shouldCheckForQueries()) { - SupersetClient.get({ - endpoint: `/superset/queries/${ - this.props.queriesLastUpdate - QUERY_UPDATE_BUFFER_MS - }`, - timeout: QUERY_TIMEOUT_LIMIT, - }) - .then(({ json }) => { - if (Object.keys(json).length > 0) { - this.props.actions.refreshQueries(json); - } - this.setState({ offline: false }); - }) - .catch(() => { - this.setState({ offline: true }); - }); - } else { - this.setState({ offline: false }); - } - } - - render() { - return null; - } -} -QueryAutoRefresh.propTypes = { - offline: PropTypes.bool.isRequired, - queries: PropTypes.object.isRequired, - actions: PropTypes.object.isRequired, - queriesLastUpdate: PropTypes.number.isRequired, -}; - -function mapStateToProps({ sqlLab }) { - return { - offline: sqlLab.offline, - queries: sqlLab.queries, - queriesLastUpdate: sqlLab.queriesLastUpdate, - }; -} - -function mapDispatchToProps(dispatch) { - return { - actions: bindActionCreators(Actions, dispatch), - }; -} - -export default connect(mapStateToProps, mapDispatchToProps)(QueryAutoRefresh); diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx new file mode 100644 index 0000000000000..54ed13a65767f --- /dev/null +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx @@ -0,0 +1,118 @@ +/** + * 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 { useState, useEffect } from 'react'; +import { SupersetClient } from '@superset-ui/core'; +import * as Actions from 'src/SqlLab/actions/sqlLab'; +import { Query } from 'src/SqlLab/types'; +import useInterval from '../../utils/useInterval'; + +const QUERY_UPDATE_FREQ = 2000; +const QUERY_UPDATE_BUFFER_MS = 5000; +const MAX_QUERY_AGE_TO_POLL = 21600000; +const QUERY_TIMEOUT_LIMIT = 10000; + +interface UserOfflineFunc { + (offline: boolean): boolean; +} + +interface RefreshQueriesFunc { + (alteredQueries: any): any; +} + +interface Actions { + setUserOffline: UserOfflineFunc; + refreshQueries: RefreshQueriesFunc; +} + +export interface QueryAutoRefreshProps { + offline: boolean; + queries: Query[]; + actions: Actions; + queriesLastUpdate: number; +} + +// returns true if the Query.state matches one of the specifc values indicating the query is still processing on server +export const isQueryRunning = (q: Query): boolean => + ['running', 'started', 'pending', 'fetching'].indexOf(q?.state) >= 0; + +// returns true if at least one query is running and within the max age to poll timeframe +export const shouldCheckForQueries = (queryList: Query[]): boolean => { + let shouldCheck = false; + // if there are started or running queries, this method should return true + const now = Date.now(); + if (queryList && typeof queryList === 'object') { + shouldCheck = Object.values(queryList).some( + q => isQueryRunning(q) && now - q?.startDttm < MAX_QUERY_AGE_TO_POLL, + ); + } + return shouldCheck; +}; + +function QueryAutoRefresh({ + offline, + queries, + actions, + queriesLastUpdate, +}: QueryAutoRefreshProps) { + // In case of an error / timeout requesting query execution state + // we set isOffline to true. the interval timer will continue to run + // and update status if connection succeeds later + const [isOffline, setIsOffline] = useState(offline); + // We do not want to spam requests in the case of slow connections and potentially recieve responses out of order + // pendingRequest check ensures we only have one active http call to check for query statuses + const [pendingRequest, setPendingRequest] = useState(false); + + useEffect(() => { + if (isOffline !== offline) { + actions?.setUserOffline(isOffline); + } + }, [isOffline, offline, actions]); + + const refreshQueries = () => { + if (!pendingRequest && (shouldCheckForQueries(queries) || isOffline)) { + setPendingRequest(true); + SupersetClient.get({ + endpoint: `/superset/queries/${ + queriesLastUpdate - QUERY_UPDATE_BUFFER_MS + }`, + timeout: QUERY_TIMEOUT_LIMIT, + }) + .then(({ json }) => { + if (json && Object.keys(json)?.length > 0) { + actions?.refreshQueries(json); + } + setIsOffline(false); + setPendingRequest(false); + }) + .catch(() => { + setIsOffline(true); + setPendingRequest(false); + }); + } + }; + + // Solves issue where direct usage of setInterval in function components + // uses stale props / state from closure + // See comments in the useInterval.ts file for more information + useInterval(refreshQueries, QUERY_UPDATE_FREQ); + + return null; +} + +export default QueryAutoRefresh; diff --git a/superset-frontend/src/SqlLab/fixtures.ts b/superset-frontend/src/SqlLab/fixtures.ts index 5b12ee29213ee..1cb0f5197fcf8 100644 --- a/superset-frontend/src/SqlLab/fixtures.ts +++ b/superset-frontend/src/SqlLab/fixtures.ts @@ -19,6 +19,7 @@ import sinon from 'sinon'; import * as actions from 'src/SqlLab/actions/sqlLab'; import { ColumnKeyTypeType } from 'src/SqlLab/components/ColumnElement'; +import { Query } from './types'; export const mockedActions = sinon.stub({ ...actions }); @@ -512,7 +513,86 @@ export const failedQueryWithErrors = { tempTable: '', }; -export const runningQuery = { +const baseQuery: Query = { + queryId: 567, + dbId: 1, + sql: 'SELECT * FROM superset.slices', + sqlEditorId: 'SJ8YO72R', + tab: 'Demo', + ctas: false, + cached: false, + id: 'BkA1CLrJg', + progress: 100, + startDttm: 1476910566092.96, + state: 'success', + tempSchema: null, + tempTable: 'temp', + userId: 1, + executedSql: 'SELECT * FROM superset.slices', + rows: 42, + started: 'started', + queryLimit: 100, + endDttm: 1476910566798, + schema: 'test_schema', + errorMessage: null, + db: { key: 'main' }, + user: { key: 'admin' }, + isDataPreview: false, + resultsKey: null, + trackingUrl: null, + templateParams: null, + limitingFactor: 'capacity', + duration: '2334645675467', + time: { key: 'value' }, + querylink: { key: 'value' }, + output: { key: 'value' }, + actions: { key: 'value' }, + extra: { + progress: null, + }, + results: { + displayLimitReached: false, + query: { limit: 6 }, + columns: [ + { + is_dttm: true, + name: 'ds', + type: 'STRING', + }, + { + is_dttm: false, + name: 'gender', + type: 'STRING', + }, + ], + selected_columns: [ + { + is_dttm: true, + name: 'ds', + type: 'STRING', + }, + { + is_dttm: false, + name: 'gender', + type: 'STRING', + }, + ], + expanded_columns: [ + { + is_dttm: true, + name: 'ds', + type: 'STRING', + }, + ], + data: [ + { col1: '0', col2: '1' }, + { col1: '2', col2: '3' }, + ], + }, +}; + +export const runningQuery: Query = { + ...baseQuery, dbId: 1, cached: false, ctas: false, @@ -521,6 +601,18 @@ export const runningQuery = { state: 'running', startDttm: Date.now() - 500, }; + +export const successfulQuery: Query = { + ...baseQuery, + dbId: 1, + cached: false, + ctas: false, + id: 'ryhMUZCGb', + progress: 100, + state: 'success', + startDttm: Date.now() - 500, +}; + export const cachedQuery = { ...queries[0], cached: true }; export const user = { diff --git a/superset-frontend/src/SqlLab/utils/useInterval.ts b/superset-frontend/src/SqlLab/utils/useInterval.ts new file mode 100644 index 0000000000000..facc5d75bddcc --- /dev/null +++ b/superset-frontend/src/SqlLab/utils/useInterval.ts @@ -0,0 +1,47 @@ +/** + * 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 { useEffect, useRef } from 'react'; + +/* + * Functional components and setTimeout with useState do not play well + * and the setTimeout callback typically has stale state from a closure + * The useInterval function solves this issue. + * more info: https://overreacted.io/making-setinterval-declarative-with-react-hooks/ + */ +function useInterval(callback: Function, delay: number | null): void { + const savedCallback = useRef(null); + // Remember the latest function. + useEffect(() => { + savedCallback.current = callback; + }, [callback]); + + // Set up the interval. + useEffect(() => { + function tick() { + savedCallback?.current?.(); + } + if (delay !== null) { + const id = setInterval(tick, delay); + return () => clearInterval(id); + } + return () => {}; + }, [delay]); +} + +export default useInterval; From 42a7c739fa6e0aba548e613a4906bd32d699fdc6 Mon Sep 17 00:00:00 2001 From: Eric Briscoe Date: Tue, 24 May 2022 21:59:46 -0700 Subject: [PATCH 02/11] addressing PR comments [sc-48362] Removes unneeded props and state tracking of offline, adds finally block to simplify clearing pending request, simplifies value comparison in array by using includes in place of indexOf --- .../QueryAutoRefresh.test.tsx | 4 --- .../QueryAutoRefreshContainer.tsx | 3 -- .../components/QueryAutoRefresh/index.tsx | 33 +++++++------------ .../src/SqlLab/utils/useInterval.ts | 2 +- 4 files changed, 13 insertions(+), 29 deletions(-) diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx index def5e1ef2193b..ea0db4c682937 100644 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx @@ -27,7 +27,6 @@ import { successfulQuery, runningQuery } from 'src/SqlLab/fixtures'; // 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 describe('QueryAutoRefresh', () => { - const offline = false; const queries = [runningQuery]; const actions = { setUserOffline: jest.fn(), @@ -83,7 +82,6 @@ describe('QueryAutoRefresh', () => { render( , @@ -99,7 +97,6 @@ describe('QueryAutoRefresh', () => { , @@ -114,7 +111,6 @@ describe('QueryAutoRefresh', () => { render( , diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefreshContainer.tsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefreshContainer.tsx index f84030275e986..9f23afdefe0bd 100644 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefreshContainer.tsx +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefreshContainer.tsx @@ -24,14 +24,12 @@ import { RootState } from 'src/SqlLab/types'; import QueryAutoRefresh, { QueryAutoRefreshProps } from '.'; function QueryAutoRefreshConnector({ - offline, queries, actions, queriesLastUpdate, }: QueryAutoRefreshProps) { return ( - ['running', 'started', 'pending', 'fetching'].indexOf(q?.state) >= 0; + ['running', 'started', 'pending', 'fetching'].includes(q?.state); // returns true if at least one query is running and within the max age to poll timeframe export const shouldCheckForQueries = (queryList: Query[]): boolean => { @@ -65,27 +64,16 @@ export const shouldCheckForQueries = (queryList: Query[]): boolean => { }; function QueryAutoRefresh({ - offline, queries, actions, queriesLastUpdate, }: QueryAutoRefreshProps) { - // In case of an error / timeout requesting query execution state - // we set isOffline to true. the interval timer will continue to run - // and update status if connection succeeds later - const [isOffline, setIsOffline] = useState(offline); // We do not want to spam requests in the case of slow connections and potentially recieve responses out of order // pendingRequest check ensures we only have one active http call to check for query statuses const [pendingRequest, setPendingRequest] = useState(false); - useEffect(() => { - if (isOffline !== offline) { - actions?.setUserOffline(isOffline); - } - }, [isOffline, offline, actions]); - const refreshQueries = () => { - if (!pendingRequest && (shouldCheckForQueries(queries) || isOffline)) { + if (!pendingRequest && shouldCheckForQueries(queries)) { setPendingRequest(true); SupersetClient.get({ endpoint: `/superset/queries/${ @@ -94,14 +82,15 @@ function QueryAutoRefresh({ timeout: QUERY_TIMEOUT_LIMIT, }) .then(({ json }) => { - if (json && Object.keys(json)?.length > 0) { + if (json) { actions?.refreshQueries(json); } - setIsOffline(false); - setPendingRequest(false); + actions?.setUserOffline(false); }) .catch(() => { - setIsOffline(true); + actions?.setUserOffline(true); + }) + .finally(() => { setPendingRequest(false); }); } @@ -110,7 +99,9 @@ function QueryAutoRefresh({ // Solves issue where direct usage of setInterval in function components // uses stale props / state from closure // See comments in the useInterval.ts file for more information - useInterval(refreshQueries, QUERY_UPDATE_FREQ); + useInterval(() => { + refreshQueries(); + }, QUERY_UPDATE_FREQ); return null; } diff --git a/superset-frontend/src/SqlLab/utils/useInterval.ts b/superset-frontend/src/SqlLab/utils/useInterval.ts index facc5d75bddcc..731e6c85cf74c 100644 --- a/superset-frontend/src/SqlLab/utils/useInterval.ts +++ b/superset-frontend/src/SqlLab/utils/useInterval.ts @@ -25,7 +25,7 @@ import { useEffect, useRef } from 'react'; * more info: https://overreacted.io/making-setinterval-declarative-with-react-hooks/ */ function useInterval(callback: Function, delay: number | null): void { - const savedCallback = useRef(null); + const savedCallback = useRef(callback); // Remember the latest function. useEffect(() => { savedCallback.current = callback; From 79a30aa49dd1a6f09a47f174caf82b3e35754ffb Mon Sep 17 00:00:00 2001 From: Eric Briscoe Date: Wed, 25 May 2022 11:27:44 -0700 Subject: [PATCH 03/11] Address PR comment to use enum for QueryState [sc-48362] Original implementation had string literals used in multiple places representing Query.state value options. This commit creates a formal TypeScript enum for QueryState so we can remove string literals and ensure better consistency --- .../QueryAutoRefreshContainer.test.tsx | 123 ++++++++++++++++++ .../components/QueryAutoRefresh/index.tsx | 4 +- superset-frontend/src/SqlLab/fixtures.ts | 20 +-- superset-frontend/src/SqlLab/types.ts | 37 ++++-- 4 files changed, 163 insertions(+), 21 deletions(-) create mode 100644 superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefreshContainer.test.tsx diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefreshContainer.test.tsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefreshContainer.test.tsx new file mode 100644 index 0000000000000..ea0db4c682937 --- /dev/null +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefreshContainer.test.tsx @@ -0,0 +1,123 @@ +/** + * 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 React from 'react'; +import { render } from '@testing-library/react'; +import QueryAutoRefresh, { + isQueryRunning, + shouldCheckForQueries, +} from 'src/SqlLab/components/QueryAutoRefresh'; +import { successfulQuery, runningQuery } from 'src/SqlLab/fixtures'; + +// 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 +describe('QueryAutoRefresh', () => { + const queries = [runningQuery]; + const actions = { + setUserOffline: jest.fn(), + refreshQueries: jest.fn(), + }; + const queriesLastUpdate = Date.now(); + + it('isQueryRunning returns true for valid running query', () => { + const running = isQueryRunning(runningQuery); + expect(running).toBe(true); + }); + + it('isQueryRunning returns false for valid not-running query', () => { + const running = isQueryRunning(successfulQuery); + expect(running).toBe(false); + }); + + it('isQueryRunning returns false for invalid query', () => { + // @ts-ignore + let running = isQueryRunning(null); + expect(running).toBe(false); + // @ts-ignore + running = isQueryRunning(undefined); + expect(running).toBe(false); + // @ts-ignore + running = isQueryRunning('I Should Be An Object'); + expect(running).toBe(false); + // @ts-ignore + running = isQueryRunning({ state: { badFormat: true } }); + expect(running).toBe(false); + }); + + it('shouldCheckForQueries is true for valid running query', () => { + expect(shouldCheckForQueries([runningQuery])).toBe(true); + }); + + it('shouldCheckForQueries is false for valid completed query', () => { + expect(shouldCheckForQueries([successfulQuery])).toBe(false); + }); + + it('shouldCheckForQueries is false for invalid inputs', () => { + // @ts-ignore + expect(shouldCheckForQueries(null)).toBe(false); + // @ts-ignore + expect(shouldCheckForQueries(undefined)).toBe(false); + expect( + // @ts-ignore + shouldCheckForQueries([null, 'hello world', [], undefined, 23]), + ).toBe(false); + }); + + it('Attempts to refresh when given pending query', () => { + render( + , + ); + setTimeout(() => { + expect(actions.refreshQueries).toHaveBeenCalled(); + expect(actions.setUserOffline).not.toHaveBeenCalled(); + }, 1000); + }); + + it('Does not fail and attempts to refresh when given pending query and invlaid query', () => { + render( + , + ); + setTimeout(() => { + expect(actions.refreshQueries).toHaveBeenCalled(); + expect(actions.setUserOffline).not.toHaveBeenCalled(); + }, 1000); + }); + + it('Does NOT Attempt to refresh when given only completed queries', () => { + render( + , + ); + setTimeout(() => { + expect(actions.refreshQueries).not.toHaveBeenCalled(); + expect(actions.setUserOffline).not.toHaveBeenCalled(); + }, 1000); + }); +}); diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx index 2b359f71eb3fb..e0c80a99f14d3 100644 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx @@ -19,7 +19,7 @@ import { useState } from 'react'; import { SupersetClient } from '@superset-ui/core'; import * as Actions from 'src/SqlLab/actions/sqlLab'; -import { Query } from 'src/SqlLab/types'; +import { Query, runningQueryStateList } from 'src/SqlLab/types'; import useInterval from 'src/SqlLab/utils/useInterval'; const QUERY_UPDATE_FREQ = 2000; @@ -48,7 +48,7 @@ export interface QueryAutoRefreshProps { // returns true if the Query.state matches one of the specifc values indicating the query is still processing on server export const isQueryRunning = (q: Query): boolean => - ['running', 'started', 'pending', 'fetching'].includes(q?.state); + runningQueryStateList.includes(q?.state); // returns true if at least one query is running and within the max age to poll timeframe export const shouldCheckForQueries = (queryList: Query[]): boolean => { diff --git a/superset-frontend/src/SqlLab/fixtures.ts b/superset-frontend/src/SqlLab/fixtures.ts index 1cb0f5197fcf8..773852643754c 100644 --- a/superset-frontend/src/SqlLab/fixtures.ts +++ b/superset-frontend/src/SqlLab/fixtures.ts @@ -19,7 +19,7 @@ import sinon from 'sinon'; import * as actions from 'src/SqlLab/actions/sqlLab'; import { ColumnKeyTypeType } from 'src/SqlLab/components/ColumnElement'; -import { Query } from './types'; +import { Query, QueryState } from './types'; export const mockedActions = sinon.stub({ ...actions }); @@ -202,7 +202,7 @@ export const queries = [ id: 'BkA1CLrJg', progress: 100, startDttm: 1476910566092.96, - state: 'success', + state: QueryState.SUCCESS, changedOn: 1476910566000, tempTable: null, userId: 1, @@ -261,7 +261,7 @@ export const queries = [ id: 'S1zeAISkx', progress: 100, startDttm: 1476910570802.2, - state: 'success', + state: QueryState.SUCCESS, changedOn: 1476910572000, tempTable: null, userId: 1, @@ -295,7 +295,7 @@ export const queryWithNoQueryLimit = { id: 'BkA1CLrJg', progress: 100, startDttm: 1476910566092.96, - state: 'success', + state: QueryState.SUCCESS, changedOn: 1476910566000, tempTable: null, userId: 1, @@ -465,7 +465,7 @@ export const stoppedQuery = { sql: 'SELECT ...', sqlEditorId: 'rJaf5u9WZ', startDttm: 1497400851936, - state: 'stopped', + state: QueryState.STOPPED, tab: 'Untitled Query 2', tempTable: '', }; @@ -483,7 +483,7 @@ export const failedQueryWithErrorMessage = { sql: 'SELECT ...', sqlEditorId: 'rJaf5u9WZ', startDttm: 1497400851936, - state: 'failed', + state: QueryState.FAILED, tab: 'Untitled Query 2', tempTable: '', }; @@ -508,7 +508,7 @@ export const failedQueryWithErrors = { sql: 'SELECT ...', sqlEditorId: 'rJaf5u9WZ', startDttm: 1497400851936, - state: 'failed', + state: QueryState.FAILED, tab: 'Untitled Query 2', tempTable: '', }; @@ -524,7 +524,7 @@ const baseQuery: Query = { id: 'BkA1CLrJg', progress: 100, startDttm: 1476910566092.96, - state: 'success', + state: QueryState.SUCCESS, tempSchema: null, tempTable: 'temp', userId: 1, @@ -598,7 +598,7 @@ export const runningQuery: Query = { ctas: false, id: 'ryhMUZCGb', progress: 90, - state: 'running', + state: QueryState.RUNNING, startDttm: Date.now() - 500, }; @@ -609,7 +609,7 @@ export const successfulQuery: Query = { ctas: false, id: 'ryhMUZCGb', progress: 100, - state: 'success', + state: QueryState.SUCCESS, startDttm: Date.now() - 500, }; diff --git a/superset-frontend/src/SqlLab/types.ts b/superset-frontend/src/SqlLab/types.ts index e1714791638c8..abde992c5b1ab 100644 --- a/superset-frontend/src/SqlLab/types.ts +++ b/superset-frontend/src/SqlLab/types.ts @@ -28,15 +28,34 @@ export type Column = { is_dttm: boolean; }; -export type QueryState = - | 'stopped' - | 'failed' - | 'pending' - | 'running' - | 'scheduled' - | 'success' - | 'fetching' - | 'timed_out'; +// Possible states of a query object for processing on the server +export enum QueryState { + STARTED = 'started', + STOPPED = 'stopped', + FAILED = 'failed', + PENDING = 'pending', + RUNNING = 'running', + SCHEDULED = 'scheduled', + SUCCESS = 'success', + FETCHING = 'fetching', + TIMED_OUT = 'timed_out', +} + +// Inidcates a Query's state is still processing +export const runningQueryStateList: QueryState[] = [ + QueryState.RUNNING, + QueryState.STARTED, + QueryState.PENDING, + QueryState.FETCHING, +]; + +// Indicates a Query's state has completed processing regardless of success / failure +export const concludedQueryStateList: QueryState[] = [ + QueryState.STOPPED, + QueryState.FAILED, + QueryState.SUCCESS, + QueryState.TIMED_OUT, +]; export type Query = { cached: boolean; From fe6a853da3c51106c99464f4137e0a14a7d60118 Mon Sep 17 00:00:00 2001 From: Eric Briscoe Date: Wed, 25 May 2022 15:40:02 -0700 Subject: [PATCH 04/11] Address PR comments for object type validation [sc-48362] This commit resolves an issue why the TypeScript typing for queryList was marked as a Query[] but was actually a Dictionary (associative array) or Queries. A new type QueryDictionary has been added and the QueryAutoRefresh code was adjusted to use QueryDictionary instead of Query[] in appropriate places as well as unit tests. Commit also removes QueryAutoRefreshContainer by making the once component using QueryAutoRefresh (which is already redux connected) pass the needed values on props. this simplifies the code base and reduce files that need unit testing while keeping QueryAutoRefresh out of needing a redux connection directly. --- .../src/SqlLab/components/App/index.jsx | 13 +- .../QueryAutoRefresh.test.tsx | 29 ++++- .../QueryAutoRefreshContainer.test.tsx | 123 ------------------ .../QueryAutoRefreshContainer.tsx | 56 -------- .../components/QueryAutoRefresh/index.tsx | 34 ++--- superset-frontend/src/SqlLab/types.ts | 5 + 6 files changed, 54 insertions(+), 206 deletions(-) delete mode 100644 superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefreshContainer.test.tsx delete mode 100644 superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefreshContainer.tsx diff --git a/superset-frontend/src/SqlLab/components/App/index.jsx b/superset-frontend/src/SqlLab/components/App/index.jsx index 36c7b5175a617..d33935de575c1 100644 --- a/superset-frontend/src/SqlLab/components/App/index.jsx +++ b/superset-frontend/src/SqlLab/components/App/index.jsx @@ -30,7 +30,7 @@ import { } from 'src/SqlLab/constants'; import * as Actions from 'src/SqlLab/actions/sqlLab'; import TabbedSqlEditors from '../TabbedSqlEditors'; -import QueryAutoRefreshContainer from '../QueryAutoRefresh/QueryAutoRefreshContainer'; +import QueryAutoRefresh from '../QueryAutoRefresh'; class App extends React.PureComponent { constructor(props) { @@ -94,12 +94,17 @@ class App extends React.PureComponent { } render() { + const { queries, actions, queriesLastUpdate } = this.props; if (this.state.hash && this.state.hash === '#search') { return window.location.replace('/superset/sqllab/history/'); } return (
- +
@@ -114,10 +119,12 @@ App.propTypes = { }; function mapStateToProps(state) { - const { common, localStorageUsageInKilobytes } = state; + const { common, localStorageUsageInKilobytes, sqlLab } = state; return { common, localStorageUsageInKilobytes, + queries: sqlLab?.queries, + queriesLastUpdate: sqlLab?.queriesLastUpdate, }; } diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx index ea0db4c682937..0b0953e84518c 100644 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx @@ -23,11 +23,17 @@ import QueryAutoRefresh, { shouldCheckForQueries, } from 'src/SqlLab/components/QueryAutoRefresh'; import { successfulQuery, runningQuery } from 'src/SqlLab/fixtures'; +import { QueryDictionary } from 'src/SqlLab/types'; // 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 describe('QueryAutoRefresh', () => { - const queries = [runningQuery]; + const runningQueries: QueryDictionary = {}; + runningQueries[runningQuery.id] = runningQuery; + + const successfulQueries: QueryDictionary = {}; + successfulQueries[successfulQuery.id] = successfulQuery; + const actions = { setUserOffline: jest.fn(), refreshQueries: jest.fn(), @@ -60,11 +66,11 @@ describe('QueryAutoRefresh', () => { }); it('shouldCheckForQueries is true for valid running query', () => { - expect(shouldCheckForQueries([runningQuery])).toBe(true); + expect(shouldCheckForQueries(runningQueries)).toBe(true); }); it('shouldCheckForQueries is false for valid completed query', () => { - expect(shouldCheckForQueries([successfulQuery])).toBe(false); + expect(shouldCheckForQueries(successfulQueries)).toBe(false); }); it('shouldCheckForQueries is false for invalid inputs', () => { @@ -74,14 +80,23 @@ describe('QueryAutoRefresh', () => { expect(shouldCheckForQueries(undefined)).toBe(false); expect( // @ts-ignore - shouldCheckForQueries([null, 'hello world', [], undefined, 23]), + shouldCheckForQueries({ + // @ts-ignore + '1234': null, + // @ts-ignore + '23425': 'hello world', + // @ts-ignore + '345': [], + // @ts-ignore + '57346': undefined, + }), ).toBe(false); }); it('Attempts to refresh when given pending query', () => { render( , @@ -96,7 +111,7 @@ describe('QueryAutoRefresh', () => { render( , @@ -110,7 +125,7 @@ describe('QueryAutoRefresh', () => { it('Does NOT Attempt to refresh when given only completed queries', () => { render( , diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefreshContainer.test.tsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefreshContainer.test.tsx deleted file mode 100644 index ea0db4c682937..0000000000000 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefreshContainer.test.tsx +++ /dev/null @@ -1,123 +0,0 @@ -/** - * 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 React from 'react'; -import { render } from '@testing-library/react'; -import QueryAutoRefresh, { - isQueryRunning, - shouldCheckForQueries, -} from 'src/SqlLab/components/QueryAutoRefresh'; -import { successfulQuery, runningQuery } from 'src/SqlLab/fixtures'; - -// 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 -describe('QueryAutoRefresh', () => { - const queries = [runningQuery]; - const actions = { - setUserOffline: jest.fn(), - refreshQueries: jest.fn(), - }; - const queriesLastUpdate = Date.now(); - - it('isQueryRunning returns true for valid running query', () => { - const running = isQueryRunning(runningQuery); - expect(running).toBe(true); - }); - - it('isQueryRunning returns false for valid not-running query', () => { - const running = isQueryRunning(successfulQuery); - expect(running).toBe(false); - }); - - it('isQueryRunning returns false for invalid query', () => { - // @ts-ignore - let running = isQueryRunning(null); - expect(running).toBe(false); - // @ts-ignore - running = isQueryRunning(undefined); - expect(running).toBe(false); - // @ts-ignore - running = isQueryRunning('I Should Be An Object'); - expect(running).toBe(false); - // @ts-ignore - running = isQueryRunning({ state: { badFormat: true } }); - expect(running).toBe(false); - }); - - it('shouldCheckForQueries is true for valid running query', () => { - expect(shouldCheckForQueries([runningQuery])).toBe(true); - }); - - it('shouldCheckForQueries is false for valid completed query', () => { - expect(shouldCheckForQueries([successfulQuery])).toBe(false); - }); - - it('shouldCheckForQueries is false for invalid inputs', () => { - // @ts-ignore - expect(shouldCheckForQueries(null)).toBe(false); - // @ts-ignore - expect(shouldCheckForQueries(undefined)).toBe(false); - expect( - // @ts-ignore - shouldCheckForQueries([null, 'hello world', [], undefined, 23]), - ).toBe(false); - }); - - it('Attempts to refresh when given pending query', () => { - render( - , - ); - setTimeout(() => { - expect(actions.refreshQueries).toHaveBeenCalled(); - expect(actions.setUserOffline).not.toHaveBeenCalled(); - }, 1000); - }); - - it('Does not fail and attempts to refresh when given pending query and invlaid query', () => { - render( - , - ); - setTimeout(() => { - expect(actions.refreshQueries).toHaveBeenCalled(); - expect(actions.setUserOffline).not.toHaveBeenCalled(); - }, 1000); - }); - - it('Does NOT Attempt to refresh when given only completed queries', () => { - render( - , - ); - setTimeout(() => { - expect(actions.refreshQueries).not.toHaveBeenCalled(); - expect(actions.setUserOffline).not.toHaveBeenCalled(); - }, 1000); - }); -}); diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefreshContainer.tsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefreshContainer.tsx deleted file mode 100644 index 9f23afdefe0bd..0000000000000 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefreshContainer.tsx +++ /dev/null @@ -1,56 +0,0 @@ -/** - * 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 React from 'react'; -import { Dispatch, bindActionCreators } from 'redux'; -import { connect } from 'react-redux'; -import * as Actions from 'src/SqlLab/actions/sqlLab'; -import { RootState } from 'src/SqlLab/types'; -import QueryAutoRefresh, { QueryAutoRefreshProps } from '.'; - -function QueryAutoRefreshConnector({ - queries, - actions, - queriesLastUpdate, -}: QueryAutoRefreshProps) { - return ( - - ); -} - -function mapStateToProps({ sqlLab }: RootState) { - return { - queries: sqlLab.queries, - queriesLastUpdate: sqlLab.queriesLastUpdate, - }; -} - -function mapDispatchToProps(dispatch: Dispatch) { - return { - actions: bindActionCreators(Actions, dispatch), - }; -} - -export default connect( - mapStateToProps, - mapDispatchToProps, -)(QueryAutoRefreshConnector); diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx index e0c80a99f14d3..4cd94a9b3bf51 100644 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx @@ -17,9 +17,13 @@ * under the License. */ import { useState } from 'react'; +import { isObject } from 'lodash'; import { SupersetClient } from '@superset-ui/core'; -import * as Actions from 'src/SqlLab/actions/sqlLab'; -import { Query, runningQueryStateList } from 'src/SqlLab/types'; +import { + Query, + QueryDictionary, + runningQueryStateList, +} from 'src/SqlLab/types'; import useInterval from 'src/SqlLab/utils/useInterval'; const QUERY_UPDATE_FREQ = 2000; @@ -35,14 +39,10 @@ interface RefreshQueriesFunc { (alteredQueries: any): any; } -interface Actions { +export interface QueryAutoRefreshProps { + queries: QueryDictionary; setUserOffline: UserOfflineFunc; refreshQueries: RefreshQueriesFunc; -} - -export interface QueryAutoRefreshProps { - queries: Query[]; - actions: Actions; queriesLastUpdate: number; } @@ -51,11 +51,10 @@ export const isQueryRunning = (q: Query): boolean => runningQueryStateList.includes(q?.state); // returns true if at least one query is running and within the max age to poll timeframe -export const shouldCheckForQueries = (queryList: Query[]): boolean => { +export const shouldCheckForQueries = (queryList: QueryDictionary): boolean => { let shouldCheck = false; - // if there are started or running queries, this method should return true const now = Date.now(); - if (queryList && typeof queryList === 'object') { + if (isObject(queryList)) { shouldCheck = Object.values(queryList).some( q => isQueryRunning(q) && now - q?.startDttm < MAX_QUERY_AGE_TO_POLL, ); @@ -65,14 +64,15 @@ export const shouldCheckForQueries = (queryList: Query[]): boolean => { function QueryAutoRefresh({ queries, - actions, + refreshQueries, + setUserOffline, queriesLastUpdate, }: QueryAutoRefreshProps) { // We do not want to spam requests in the case of slow connections and potentially recieve responses out of order // pendingRequest check ensures we only have one active http call to check for query statuses const [pendingRequest, setPendingRequest] = useState(false); - const refreshQueries = () => { + const checkForRefresh = () => { if (!pendingRequest && shouldCheckForQueries(queries)) { setPendingRequest(true); SupersetClient.get({ @@ -83,12 +83,12 @@ function QueryAutoRefresh({ }) .then(({ json }) => { if (json) { - actions?.refreshQueries(json); + refreshQueries?.(json); } - actions?.setUserOffline(false); + setUserOffline(false); }) .catch(() => { - actions?.setUserOffline(true); + setUserOffline?.(true); }) .finally(() => { setPendingRequest(false); @@ -100,7 +100,7 @@ function QueryAutoRefresh({ // uses stale props / state from closure // See comments in the useInterval.ts file for more information useInterval(() => { - refreshQueries(); + checkForRefresh(); }, QUERY_UPDATE_FREQ); return null; diff --git a/superset-frontend/src/SqlLab/types.ts b/superset-frontend/src/SqlLab/types.ts index abde992c5b1ab..15ce0101e596c 100644 --- a/superset-frontend/src/SqlLab/types.ts +++ b/superset-frontend/src/SqlLab/types.ts @@ -107,6 +107,11 @@ export type Query = { actions: Record; }; +// Object as Dictionary (associative array) with Query id as the key and type Query as the value +export type QueryDictionary = { + [id: string]: Query; +}; + export interface QueryEditor { dbId?: number; title: string; From d4a58aaaede0caa610d293191a6e295a4e1f56a8 Mon Sep 17 00:00:00 2001 From: Eric Briscoe Date: Wed, 25 May 2022 15:50:54 -0700 Subject: [PATCH 05/11] Addresses PR comment to add QueryState.SCHEDULED to runningQueryStateList [sc-48362] In previous implementation 'scheduled' was not included int he list of Query States. Further investigation shows it should be added to as a running state. --- superset-frontend/src/SqlLab/types.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/superset-frontend/src/SqlLab/types.ts b/superset-frontend/src/SqlLab/types.ts index 15ce0101e596c..c0d73624142be 100644 --- a/superset-frontend/src/SqlLab/types.ts +++ b/superset-frontend/src/SqlLab/types.ts @@ -47,6 +47,7 @@ export const runningQueryStateList: QueryState[] = [ QueryState.STARTED, QueryState.PENDING, QueryState.FETCHING, + QueryState.SCHEDULED, ]; // Indicates a Query's state has completed processing regardless of success / failure From 44a9956ebd43237999c9eac50000f429ef1d6ee9 Mon Sep 17 00:00:00 2001 From: Eric Briscoe Date: Thu, 26 May 2022 13:17:29 -0700 Subject: [PATCH 06/11] Fix prettier lint error [sc-48362] --- superset-frontend/src/SqlLab/components/App/index.jsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/superset-frontend/src/SqlLab/components/App/index.jsx b/superset-frontend/src/SqlLab/components/App/index.jsx index d33935de575c1..b23de061a68db 100644 --- a/superset-frontend/src/SqlLab/components/App/index.jsx +++ b/superset-frontend/src/SqlLab/components/App/index.jsx @@ -104,7 +104,8 @@ class App extends React.PureComponent { queries={queries} refreshQueries={actions?.refreshQueries} setUserOffline={actions?.setUserOffline} - queriesLastUpdate={queriesLastUpdate}/> + queriesLastUpdate={queriesLastUpdate} + /> From 44245166b7692a597e6e358ebe91d00e0646c28a Mon Sep 17 00:00:00 2001 From: Eric Briscoe Date: Tue, 31 May 2022 11:51:50 -0700 Subject: [PATCH 07/11] Adjust unit tests for props update hoisting callbacks out of actions wrapper object [sc-48362] --- .../QueryAutoRefresh.test.tsx | 28 ++++++++++--------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx index 0b0953e84518c..b49112ceef09f 100644 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx @@ -34,10 +34,9 @@ describe('QueryAutoRefresh', () => { const successfulQueries: QueryDictionary = {}; successfulQueries[successfulQuery.id] = successfulQuery; - const actions = { - setUserOffline: jest.fn(), - refreshQueries: jest.fn(), - }; + const setUserOffline = jest.fn(); + const refreshQueries = jest.fn(); + const queriesLastUpdate = Date.now(); it('isQueryRunning returns true for valid running query', () => { @@ -97,13 +96,14 @@ describe('QueryAutoRefresh', () => { render( , ); setTimeout(() => { - expect(actions.refreshQueries).toHaveBeenCalled(); - expect(actions.setUserOffline).not.toHaveBeenCalled(); + expect(refreshQueries).toHaveBeenCalled(); + expect(setUserOffline).not.toHaveBeenCalled(); }, 1000); }); @@ -112,13 +112,14 @@ describe('QueryAutoRefresh', () => { , ); setTimeout(() => { - expect(actions.refreshQueries).toHaveBeenCalled(); - expect(actions.setUserOffline).not.toHaveBeenCalled(); + expect(refreshQueries).toHaveBeenCalled(); + expect(setUserOffline).not.toHaveBeenCalled(); }, 1000); }); @@ -126,13 +127,14 @@ describe('QueryAutoRefresh', () => { render( , ); setTimeout(() => { - expect(actions.refreshQueries).not.toHaveBeenCalled(); - expect(actions.setUserOffline).not.toHaveBeenCalled(); + expect(refreshQueries).not.toHaveBeenCalled(); + expect(setUserOffline).not.toHaveBeenCalled(); }, 1000); }); }); From 0d3153f27a10805153f9c265fd8c416499916f2e Mon Sep 17 00:00:00 2001 From: Eric Briscoe Date: Tue, 14 Jun 2022 13:58:01 -0700 Subject: [PATCH 08/11] Update with changes from master [sc-48362] Merges in updates from master and resolves conflicts from relocation of some of the Query TypeScript definitions into core --- .../superset-ui-core/src/query/types/Query.ts | 38 ++++++++++++++----- .../components/QueryAutoRefresh/index.tsx | 4 +- 2 files changed, 30 insertions(+), 12 deletions(-) diff --git a/superset-frontend/packages/superset-ui-core/src/query/types/Query.ts b/superset-frontend/packages/superset-ui-core/src/query/types/Query.ts index bcbedf536bc0b..587b9da4edd73 100644 --- a/superset-frontend/packages/superset-ui-core/src/query/types/Query.ts +++ b/superset-frontend/packages/superset-ui-core/src/query/types/Query.ts @@ -254,15 +254,35 @@ export type QueryColumn = { is_dttm: boolean; }; -export type QueryState = - | 'stopped' - | 'failed' - | 'pending' - | 'running' - | 'scheduled' - | 'success' - | 'fetching' - | 'timed_out'; +// Possible states of a query object for processing on the server +export enum QueryState { + STARTED = 'started', + STOPPED = 'stopped', + FAILED = 'failed', + PENDING = 'pending', + RUNNING = 'running', + SCHEDULED = 'scheduled', + SUCCESS = 'success', + FETCHING = 'fetching', + TIMED_OUT = 'timed_out', +} + +// Inidcates a Query's state is still processing +export const runningQueryStateList: QueryState[] = [ + QueryState.RUNNING, + QueryState.STARTED, + QueryState.PENDING, + QueryState.FETCHING, + QueryState.SCHEDULED, +]; + +// Indicates a Query's state has completed processing regardless of success / failure +export const concludedQueryStateList: QueryState[] = [ + QueryState.STOPPED, + QueryState.FAILED, + QueryState.SUCCESS, + QueryState.TIMED_OUT, +]; export type Query = { cached: boolean; diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx index 4cd94a9b3bf51..59658e5ec2dce 100644 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx @@ -18,11 +18,9 @@ */ import { useState } from 'react'; import { isObject } from 'lodash'; -import { SupersetClient } from '@superset-ui/core'; +import { SupersetClient, Query, runningQueryStateList } from '@superset-ui/core'; import { - Query, QueryDictionary, - runningQueryStateList, } from 'src/SqlLab/types'; import useInterval from 'src/SqlLab/utils/useInterval'; From ff39bc2525cf5a484ed6b0ccad590c33823166ac Mon Sep 17 00:00:00 2001 From: Eric Briscoe Date: Tue, 21 Jun 2022 20:32:23 -0700 Subject: [PATCH 09/11] Removes logic setting user offline and relying on results panel error message [sc-48362] --- .../src/SqlLab/components/App/index.jsx | 1 - .../QueryAutoRefresh.test.tsx | 7 ------- .../components/QueryAutoRefresh/index.tsx | 19 ++++++------------- 3 files changed, 6 insertions(+), 21 deletions(-) diff --git a/superset-frontend/src/SqlLab/components/App/index.jsx b/superset-frontend/src/SqlLab/components/App/index.jsx index b23de061a68db..3aae12bfb33ca 100644 --- a/superset-frontend/src/SqlLab/components/App/index.jsx +++ b/superset-frontend/src/SqlLab/components/App/index.jsx @@ -103,7 +103,6 @@ class App extends React.PureComponent { diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx index b49112ceef09f..32bf401f22139 100644 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx @@ -34,7 +34,6 @@ describe('QueryAutoRefresh', () => { const successfulQueries: QueryDictionary = {}; successfulQueries[successfulQuery.id] = successfulQuery; - const setUserOffline = jest.fn(); const refreshQueries = jest.fn(); const queriesLastUpdate = Date.now(); @@ -96,14 +95,12 @@ describe('QueryAutoRefresh', () => { render( , ); setTimeout(() => { expect(refreshQueries).toHaveBeenCalled(); - expect(setUserOffline).not.toHaveBeenCalled(); }, 1000); }); @@ -112,14 +109,12 @@ describe('QueryAutoRefresh', () => { , ); setTimeout(() => { expect(refreshQueries).toHaveBeenCalled(); - expect(setUserOffline).not.toHaveBeenCalled(); }, 1000); }); @@ -127,14 +122,12 @@ describe('QueryAutoRefresh', () => { render( , ); setTimeout(() => { expect(refreshQueries).not.toHaveBeenCalled(); - expect(setUserOffline).not.toHaveBeenCalled(); }, 1000); }); }); diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx index 59658e5ec2dce..eb3e6f4c38f87 100644 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx @@ -18,10 +18,12 @@ */ import { useState } from 'react'; import { isObject } from 'lodash'; -import { SupersetClient, Query, runningQueryStateList } from '@superset-ui/core'; import { - QueryDictionary, -} from 'src/SqlLab/types'; + SupersetClient, + Query, + runningQueryStateList, +} from '@superset-ui/core'; +import { QueryDictionary } from 'src/SqlLab/types'; import useInterval from 'src/SqlLab/utils/useInterval'; const QUERY_UPDATE_FREQ = 2000; @@ -29,17 +31,12 @@ const QUERY_UPDATE_BUFFER_MS = 5000; const MAX_QUERY_AGE_TO_POLL = 21600000; const QUERY_TIMEOUT_LIMIT = 10000; -interface UserOfflineFunc { - (offline: boolean): boolean; -} - interface RefreshQueriesFunc { (alteredQueries: any): any; } export interface QueryAutoRefreshProps { queries: QueryDictionary; - setUserOffline: UserOfflineFunc; refreshQueries: RefreshQueriesFunc; queriesLastUpdate: number; } @@ -63,7 +60,6 @@ export const shouldCheckForQueries = (queryList: QueryDictionary): boolean => { function QueryAutoRefresh({ queries, refreshQueries, - setUserOffline, queriesLastUpdate, }: QueryAutoRefreshProps) { // We do not want to spam requests in the case of slow connections and potentially recieve responses out of order @@ -83,11 +79,8 @@ function QueryAutoRefresh({ if (json) { refreshQueries?.(json); } - setUserOffline(false); - }) - .catch(() => { - setUserOffline?.(true); }) + .catch(() => {}) .finally(() => { setPendingRequest(false); }); From 81376fb035ed8f86928f9a12e5f5c2d37d4d7a24 Mon Sep 17 00:00:00 2001 From: Eric Briscoe Date: Tue, 21 Jun 2022 21:06:37 -0700 Subject: [PATCH 10/11] Fixes bad import after some TypeScript definitions were relocated to core [sc-48362] --- superset-frontend/src/SqlLab/fixtures.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset-frontend/src/SqlLab/fixtures.ts b/superset-frontend/src/SqlLab/fixtures.ts index 773852643754c..835b6652eea56 100644 --- a/superset-frontend/src/SqlLab/fixtures.ts +++ b/superset-frontend/src/SqlLab/fixtures.ts @@ -19,7 +19,7 @@ import sinon from 'sinon'; import * as actions from 'src/SqlLab/actions/sqlLab'; import { ColumnKeyTypeType } from 'src/SqlLab/components/ColumnElement'; -import { Query, QueryState } from './types'; +import { Query, QueryState } from '@superset-ui/core'; export const mockedActions = sinon.stub({ ...actions }); From 6aa313e8ad1a74d568fdf9a856df040f32d29d24 Mon Sep 17 00:00:00 2001 From: Eric Briscoe Date: Wed, 22 Jun 2022 11:17:40 -0700 Subject: [PATCH 11/11] Fixes TypeScript errors [sc-48362] --- .../superset-ui-core/src/query/types/Query.ts | 4 ++-- superset-frontend/src/SqlLab/fixtures.ts | 14 ++++++++++---- superset-frontend/src/SqlLab/types.ts | 2 +- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/superset-frontend/packages/superset-ui-core/src/query/types/Query.ts b/superset-frontend/packages/superset-ui-core/src/query/types/Query.ts index c16ea7b0fb2a7..cb90fe6f6c393 100644 --- a/superset-frontend/packages/superset-ui-core/src/query/types/Query.ts +++ b/superset-frontend/packages/superset-ui-core/src/query/types/Query.ts @@ -324,7 +324,7 @@ export type Query = { executedSql: string; output: string | Record; actions: Record; - type: DatasourceType.Query; + type: DatasourceType; columns: QueryColumn[]; }; @@ -355,7 +355,7 @@ export const testQuery: Query = { isDataPreview: false, progress: 0, resultsKey: null, - state: 'success', + state: QueryState.SUCCESS, tempSchema: null, trackingUrl: null, templateParams: null, diff --git a/superset-frontend/src/SqlLab/fixtures.ts b/superset-frontend/src/SqlLab/fixtures.ts index 835b6652eea56..58b21edd9cf73 100644 --- a/superset-frontend/src/SqlLab/fixtures.ts +++ b/superset-frontend/src/SqlLab/fixtures.ts @@ -19,7 +19,7 @@ import sinon from 'sinon'; import * as actions from 'src/SqlLab/actions/sqlLab'; import { ColumnKeyTypeType } from 'src/SqlLab/components/ColumnElement'; -import { Query, QueryState } from '@superset-ui/core'; +import { DatasourceType, QueryResponse, QueryState } from '@superset-ui/core'; export const mockedActions = sinon.stub({ ...actions }); @@ -190,6 +190,7 @@ export const defaultQueryEditor = { }, ], }; + export const queries = [ { dbId: 1, @@ -345,6 +346,7 @@ export const queryWithNoQueryLimit = { }, }, }; + export const queryWithBadColumns = { ...queries[0], results: { @@ -408,6 +410,7 @@ export const queryWithBadColumns = { ], }, }; + export const databases = { result: [ { @@ -430,6 +433,7 @@ export const databases = { }, ], }; + export const tables = { options: [ { @@ -513,7 +517,7 @@ export const failedQueryWithErrors = { tempTable: '', }; -const baseQuery: Query = { +const baseQuery: QueryResponse = { queryId: 567, dbId: 1, sql: 'SELECT * FROM superset.slices', @@ -550,6 +554,8 @@ const baseQuery: Query = { extra: { progress: null, }, + columns: [], + type: DatasourceType.Query, results: { displayLimitReached: false, query: { limit: 6 }, @@ -591,7 +597,7 @@ const baseQuery: Query = { }, }; -export const runningQuery: Query = { +export const runningQuery: QueryResponse = { ...baseQuery, dbId: 1, cached: false, @@ -602,7 +608,7 @@ export const runningQuery: Query = { startDttm: Date.now() - 500, }; -export const successfulQuery: Query = { +export const successfulQuery: QueryResponse = { ...baseQuery, dbId: 1, cached: false, diff --git a/superset-frontend/src/SqlLab/types.ts b/superset-frontend/src/SqlLab/types.ts index 7b59dfe95f85f..84406b62253e7 100644 --- a/superset-frontend/src/SqlLab/types.ts +++ b/superset-frontend/src/SqlLab/types.ts @@ -27,7 +27,7 @@ export type ExploreDatasource = Dataset | QueryResponse; // Object as Dictionary (associative array) with Query id as the key and type Query as the value export type QueryDictionary = { - [id: string]: Query; + [id: string]: QueryResponse; }; export interface QueryEditor {