From f30f685eb5791026ebd06f5fb034dbd262ef9d4c Mon Sep 17 00:00:00 2001 From: "JUST.in DO IT" Date: Thu, 7 Dec 2023 09:28:59 -0800 Subject: [PATCH] fix(sqllab): flaky json explore modal due to over-rendering (#26156) --- .../components/QueryAutoRefresh/index.tsx | 2 +- .../QueryHistory/QueryHistory.test.tsx | 5 +- .../SqlLab/components/QueryHistory/index.tsx | 29 +- .../SqlLab/components/QueryTable/index.tsx | 3 +- .../components/ResultSet/ResultSet.test.tsx | 341 +++++++++++++----- .../src/SqlLab/components/ResultSet/index.tsx | 52 ++- .../components/SouthPane/Results.test.tsx | 135 +++++++ .../SqlLab/components/SouthPane/Results.tsx | 106 ++++++ .../components/SouthPane/SouthPane.test.tsx | 81 ++--- .../src/SqlLab/components/SouthPane/index.tsx | 174 +++------ .../components/SqlEditor/SqlEditor.test.tsx | 7 +- .../src/SqlLab/reducers/sqlLab.js | 17 +- .../src/SqlLab/reducers/sqlLab.test.js | 5 +- 13 files changed, 679 insertions(+), 278 deletions(-) create mode 100644 superset-frontend/src/SqlLab/components/SouthPane/Results.test.tsx create mode 100644 superset-frontend/src/SqlLab/components/SouthPane/Results.tsx diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx index f4808f52fd..a2ffcf85cb 100644 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx @@ -45,7 +45,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 => - runningQueryStateList.includes(q?.state); + runningQueryStateList.includes(q?.state) && !q?.resultsKey; // returns true if at least one query is running and within the max age to poll timeframe export const shouldCheckForQueries = (queryList: QueryDictionary): boolean => { diff --git a/superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx b/superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx index 6fd84a0d2a..ad1881b5d9 100644 --- a/superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx +++ b/superset-frontend/src/SqlLab/components/QueryHistory/QueryHistory.test.tsx @@ -19,9 +19,10 @@ import React from 'react'; import { render, screen } from 'spec/helpers/testing-library'; import QueryHistory from 'src/SqlLab/components/QueryHistory'; +import { initialState } from 'src/SqlLab/fixtures'; const mockedProps = { - queries: [], + queryEditorId: 123, displayLimit: 1000, latestQueryId: 'yhMUZCGb', }; @@ -32,7 +33,7 @@ const setup = (overrides = {}) => ( describe('QueryHistory', () => { it('Renders an empty state for query history', () => { - render(setup()); + render(setup(), { useRedux: true, initialState }); const emptyStateText = screen.getByText( /run a query to display query history/i, diff --git a/superset-frontend/src/SqlLab/components/QueryHistory/index.tsx b/superset-frontend/src/SqlLab/components/QueryHistory/index.tsx index cab1160144..311a125d55 100644 --- a/superset-frontend/src/SqlLab/components/QueryHistory/index.tsx +++ b/superset-frontend/src/SqlLab/components/QueryHistory/index.tsx @@ -16,13 +16,15 @@ * specific language governing permissions and limitations * under the License. */ -import React from 'react'; +import React, { useMemo } from 'react'; +import { shallowEqual, useSelector } from 'react-redux'; import { EmptyStateMedium } from 'src/components/EmptyState'; -import { t, styled, QueryResponse } from '@superset-ui/core'; +import { t, styled } from '@superset-ui/core'; import QueryTable from 'src/SqlLab/components/QueryTable'; +import { SqlLabRootState } from 'src/SqlLab/types'; interface QueryHistoryProps { - queries: QueryResponse[]; + queryEditorId: string | number; displayLimit: number; latestQueryId: string | undefined; } @@ -39,11 +41,23 @@ const StyledEmptyStateWrapper = styled.div` `; const QueryHistory = ({ - queries, + queryEditorId, displayLimit, latestQueryId, -}: QueryHistoryProps) => - queries.length > 0 ? ( +}: QueryHistoryProps) => { + const queries = useSelector( + ({ sqlLab: { queries } }: SqlLabRootState) => queries, + shallowEqual, + ); + const editorQueries = useMemo( + () => + Object.values(queries).filter( + ({ sqlEditorId }) => String(sqlEditorId) === String(queryEditorId), + ), + [queries, queryEditorId], + ); + + return editorQueries.length > 0 ? ( @@ -67,5 +81,6 @@ const QueryHistory = ({ /> ); +}; export default QueryHistory; diff --git a/superset-frontend/src/SqlLab/components/QueryTable/index.tsx b/superset-frontend/src/SqlLab/components/QueryTable/index.tsx index 5dc8a43c19..3282a939ef 100644 --- a/superset-frontend/src/SqlLab/components/QueryTable/index.tsx +++ b/superset-frontend/src/SqlLab/components/QueryTable/index.tsx @@ -251,8 +251,7 @@ const QueryTable = ({ modalBody={ { + fetchMock.resetHistory(); +}); const middlewares = [thunk]; const mockStore = configureStore(middlewares); @@ -107,25 +133,47 @@ const setup = (props?: any, store?: Store) => describe('ResultSet', () => { test('renders a Table', async () => { - const { getByTestId } = setup(mockedProps, mockStore(initialState)); + const { getByTestId } = setup( + mockedProps, + mockStore({ + ...initialState, + user, + sqlLab: { + ...initialState.sqlLab, + queries: { + [queries[0].id]: queries[0], + }, + }, + }), + ); const table = getByTestId('table-container'); expect(table).toBeInTheDocument(); }); test('should render success query', async () => { + const query = queries[0]; const { queryAllByText, getByTestId } = setup( mockedProps, - mockStore(initialState), + mockStore({ + ...initialState, + user, + sqlLab: { + ...initialState.sqlLab, + queries: { + [query.id]: query, + }, + }, + }), ); const table = getByTestId('table-container'); expect(table).toBeInTheDocument(); const firstColumn = queryAllByText( - mockedProps.query.results?.columns[0].column_name ?? '', + query.results?.columns[0].column_name ?? '', )[0]; const secondColumn = queryAllByText( - mockedProps.query.results?.columns[1].column_name ?? '', + query.results?.columns[1].column_name ?? '', )[0]; expect(firstColumn).toBeInTheDocument(); expect(secondColumn).toBeInTheDocument(); @@ -135,12 +183,24 @@ describe('ResultSet', () => { }); test('should render empty results', async () => { - const props = { - ...mockedProps, - query: { ...mockedProps.query, results: { data: [] } }, + const query = { + ...queries[0], + results: { data: [] }, }; await waitFor(() => { - setup(props, mockStore(initialState)); + setup( + mockedProps, + mockStore({ + ...initialState, + user, + sqlLab: { + ...initialState.sqlLab, + queries: { + [query.id]: query, + }, + }, + }), + ); }); const alert = screen.getByRole('alert'); @@ -149,42 +209,70 @@ describe('ResultSet', () => { }); test('should call reRunQuery if timed out', async () => { - const store = mockStore(initialState); - const propsWithError = { - ...mockedProps, - query: { ...queries[0], errorMessage: 'Your session timed out' }, + const query = { + ...queries[0], + errorMessage: 'Your session timed out', }; + const store = mockStore({ + ...initialState, + user, + sqlLab: { + ...initialState.sqlLab, + queries: { + [query.id]: query, + }, + }, + }); - setup(propsWithError, store); + expect(fetchMock.calls(reRunQueryEndpoint)).toHaveLength(0); + setup(mockedProps, store); expect(store.getActions()).toHaveLength(1); expect(store.getActions()[0].query.errorMessage).toEqual( 'Your session timed out', ); expect(store.getActions()[0].type).toEqual('START_QUERY'); + await waitFor(() => + expect(fetchMock.calls(reRunQueryEndpoint)).toHaveLength(1), + ); }); test('should not call reRunQuery if no error', async () => { - const store = mockStore(initialState); + const query = queries[0]; + const store = mockStore({ + ...initialState, + user, + sqlLab: { + ...initialState.sqlLab, + queries: { + [query.id]: query, + }, + }, + }); setup(mockedProps, store); expect(store.getActions()).toEqual([]); + expect(fetchMock.calls(reRunQueryEndpoint)).toHaveLength(0); }); test('should render cached query', async () => { - const store = mockStore(initialState); - const { rerender } = setup(cachedQueryProps, store); + const store = mockStore(cachedQueryState); + const { rerender } = setup( + { ...mockedProps, queryId: cachedQuery.id }, + store, + ); // @ts-ignore - rerender(); - expect(store.getActions()).toHaveLength(2); - expect(store.getActions()[0].query.results).toEqual( - cachedQueryProps.query.results, - ); + rerender(); + expect(store.getActions()).toHaveLength(1); + expect(store.getActions()[0].query.results).toEqual(cachedQuery.results); expect(store.getActions()[0].type).toEqual('CLEAR_QUERY_RESULTS'); }); test('should render stopped query', async () => { await waitFor(() => { - setup(stoppedQueryProps, mockStore(initialState)); + setup( + { ...mockedProps, queryId: stoppedQuery.id }, + mockStore(stoppedQueryState), + ); }); const alert = screen.getByRole('alert'); @@ -192,15 +280,18 @@ describe('ResultSet', () => { }); test('should render running/pending/fetching query', async () => { - const { getByTestId } = setup(runningQueryProps, mockStore(initialState)); + const { getByTestId } = setup( + { ...mockedProps, queryId: runningQuery.id }, + mockStore(runningQueryState), + ); const progressBar = getByTestId('progress-bar'); expect(progressBar).toBeInTheDocument(); }); test('should render fetching w/ 100 progress query', async () => { const { getByRole, getByText } = setup( - fetchingQueryProps, - mockStore(initialState), + mockedProps, + mockStore(fetchingQueryState), ); const loading = getByRole('status'); expect(loading).toBeInTheDocument(); @@ -209,7 +300,10 @@ describe('ResultSet', () => { test('should render a failed query with an error message', async () => { await waitFor(() => { - setup(failedQueryWithErrorMessageProps, mockStore(initialState)); + setup( + { ...mockedProps, queryId: failedQueryWithErrorMessage.id }, + mockStore(failedQueryWithErrorMessageState), + ); }); expect(screen.getByText('Database error')).toBeInTheDocument(); @@ -218,44 +312,129 @@ describe('ResultSet', () => { test('should render a failed query with an errors object', async () => { await waitFor(() => { - setup(failedQueryWithErrorsProps, mockStore(initialState)); + setup( + { ...mockedProps, queryId: failedQueryWithErrors.id }, + mockStore(failedQueryWithErrorsState), + ); }); expect(screen.getByText('Database error')).toBeInTheDocument(); }); test('renders if there is no limit in query.results but has queryLimit', async () => { + const query = { + ...queries[0], + }; + await waitFor(() => { + setup( + mockedProps, + mockStore({ + ...initialState, + user, + sqlLab: { + ...initialState.sqlLab, + queries: { + [query.id]: query, + }, + }, + }), + ); + }); const { getByRole } = setup(mockedProps, mockStore(initialState)); expect(getByRole('table')).toBeInTheDocument(); }); test('renders if there is a limit in query.results but not queryLimit', async () => { - const props = { ...mockedProps, query: queryWithNoQueryLimit }; - const { getByRole } = setup(props, mockStore(initialState)); + const props = { ...mockedProps, queryId: queryWithNoQueryLimit.id }; + const { getByRole } = setup( + props, + mockStore({ + ...initialState, + user, + sqlLab: { + ...initialState.sqlLab, + queries: { + [queryWithNoQueryLimit.id]: queryWithNoQueryLimit, + }, + }, + }), + ); expect(getByRole('table')).toBeInTheDocument(); }); test('Async queries - renders "Fetch data preview" button when data preview has no results', () => { - setup(asyncRefetchDataPreviewProps, mockStore(initialState)); + const asyncRefetchDataPreviewQuery = { + ...queries[0], + state: 'success', + results: undefined, + isDataPreview: true, + }; + setup( + { ...asyncQueryProps, queryId: asyncRefetchDataPreviewQuery.id }, + mockStore({ + ...initialState, + user, + sqlLab: { + ...initialState.sqlLab, + queries: { + [asyncRefetchDataPreviewQuery.id]: asyncRefetchDataPreviewQuery, + }, + }, + }), + ); expect( screen.getByRole('button', { name: /fetch data preview/i, }), ).toBeVisible(); - expect(screen.queryByRole('grid')).toBe(null); + expect(screen.queryByRole('table')).toBe(null); }); test('Async queries - renders "Refetch results" button when a query has no results', () => { - setup(asyncRefetchResultsTableProps, mockStore(initialState)); + const asyncRefetchResultsTableQuery = { + ...queries[0], + state: 'success', + results: undefined, + resultsKey: 'async results key', + }; + + setup( + { ...asyncQueryProps, queryId: asyncRefetchResultsTableQuery.id }, + mockStore({ + ...initialState, + user, + sqlLab: { + ...initialState.sqlLab, + queries: { + [asyncRefetchResultsTableQuery.id]: asyncRefetchResultsTableQuery, + }, + }, + }), + ); expect( screen.getByRole('button', { name: /refetch results/i, }), ).toBeVisible(); - expect(screen.queryByRole('grid')).toBe(null); + expect(screen.queryByRole('table')).toBe(null); }); test('Async queries - renders on the first call', () => { - setup(asyncQueryProps, mockStore(initialState)); + const query = { + ...queries[0], + }; + setup( + { ...asyncQueryProps, queryId: query.id }, + mockStore({ + ...initialState, + user, + sqlLab: { + ...initialState.sqlLab, + queries: { + [query.id]: query, + }, + }, + }), + ); expect(screen.getByRole('table')).toBeVisible(); expect( screen.queryByRole('button', { diff --git a/superset-frontend/src/SqlLab/components/ResultSet/index.tsx b/superset-frontend/src/SqlLab/components/ResultSet/index.tsx index 58e55a1df7..69e4508434 100644 --- a/superset-frontend/src/SqlLab/components/ResultSet/index.tsx +++ b/superset-frontend/src/SqlLab/components/ResultSet/index.tsx @@ -17,14 +17,14 @@ * under the License. */ import React, { useCallback, useEffect, useState } from 'react'; -import { useDispatch } from 'react-redux'; +import { shallowEqual, useDispatch, useSelector } from 'react-redux'; import { useHistory } from 'react-router-dom'; +import pick from 'lodash/pick'; import ButtonGroup from 'src/components/ButtonGroup'; import Alert from 'src/components/Alert'; import Button from 'src/components/Button'; import shortid from 'shortid'; import { - QueryResponse, QueryState, styled, t, @@ -41,8 +41,7 @@ import { ISimpleColumn, SaveDatasetModal, } from 'src/SqlLab/components/SaveDatasetModal'; -import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; -import { EXPLORE_CHART_DEFAULT } from 'src/SqlLab/types'; +import { EXPLORE_CHART_DEFAULT, SqlLabRootState } from 'src/SqlLab/types'; import { mountExploreUrl } from 'src/explore/exploreUtils'; import { postFormData } from 'src/explore/exploreUtils/formData'; import ProgressBar from 'src/components/ProgressBar'; @@ -82,12 +81,11 @@ export interface ResultSetProps { database?: Record; displayLimit: number; height: number; - query: QueryResponse; + queryId: string; search?: boolean; showSql?: boolean; showSqlInline?: boolean; visualize?: boolean; - user: UserWithPermissionsAndRoles; defaultQueryLimit: number; } @@ -145,14 +143,44 @@ const ResultSet = ({ database = {}, displayLimit, height, - query, + queryId, search = true, showSql = false, showSqlInline = false, visualize = true, - user, defaultQueryLimit, }: ResultSetProps) => { + const user = useSelector(({ user }: SqlLabRootState) => user, shallowEqual); + const query = useSelector( + ({ sqlLab: { queries } }: SqlLabRootState) => + pick(queries[queryId], [ + 'id', + 'errorMessage', + 'cached', + 'results', + 'resultsKey', + 'dbId', + 'tab', + 'sql', + 'templateParams', + 'schema', + 'rows', + 'queryLimit', + 'limitingFactor', + 'trackingUrl', + 'state', + 'errors', + 'link', + 'ctas', + 'ctas_method', + 'tempSchema', + 'tempTable', + 'isDataPreview', + 'progress', + 'extra', + ]), + shallowEqual, + ); const ResultTable = extensionsRegistry.get('sqleditor.extension.resultTable') ?? FilterableTable; @@ -179,8 +207,8 @@ const ResultSet = ({ reRunQueryIfSessionTimeoutErrorOnMount(); }, [reRunQueryIfSessionTimeoutErrorOnMount]); - const fetchResults = (query: QueryResponse) => { - dispatch(fetchQueryResults(query, displayLimit)); + const fetchResults = (q: typeof query) => { + dispatch(fetchQueryResults(q, displayLimit)); }; const prevQuery = usePrevious(query); @@ -479,7 +507,7 @@ const ResultSet = ({ {query.errorMessage}} copyText={query.errorMessage || undefined} link={query.link} @@ -662,4 +690,4 @@ const ResultSet = ({ ); }; -export default ResultSet; +export default React.memo(ResultSet); diff --git a/superset-frontend/src/SqlLab/components/SouthPane/Results.test.tsx b/superset-frontend/src/SqlLab/components/SouthPane/Results.test.tsx new file mode 100644 index 0000000000..c70c039fe5 --- /dev/null +++ b/superset-frontend/src/SqlLab/components/SouthPane/Results.test.tsx @@ -0,0 +1,135 @@ +/** + * 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 'spec/helpers/testing-library'; +import { initialState, table, defaultQueryEditor } from 'src/SqlLab/fixtures'; +import { denormalizeTimestamp } from '@superset-ui/core'; +import { LOCALSTORAGE_MAX_QUERY_AGE_MS } from 'src/SqlLab/constants'; +import Results from './Results'; + +const mockedProps = { + queryEditorId: defaultQueryEditor.id, + latestQueryId: 'LCly_kkIN', + height: 1, + displayLimit: 1, + defaultQueryLimit: 100, +}; + +const mockedEmptyProps = { + queryEditorId: 'random_id', + latestQueryId: 'empty_query_id', + height: 100, + displayLimit: 100, + defaultQueryLimit: 100, +}; + +const mockedExpiredProps = { + ...mockedEmptyProps, + latestQueryId: 'expired_query_id', +}; + +const latestQueryProgressMsg = 'LATEST QUERY MESSAGE - LCly_kkIN'; +const expireDateTime = Date.now() - LOCALSTORAGE_MAX_QUERY_AGE_MS - 1; + +const mockState = { + ...initialState, + sqlLab: { + ...initialState, + offline: false, + tables: [ + { + ...table, + dataPreviewQueryId: '2g2_iRFMl', + queryEditorId: defaultQueryEditor.id, + }, + ], + databases: {}, + queries: { + LCly_kkIN: { + cached: false, + changed_on: denormalizeTimestamp(new Date().toISOString()), + db: 'main', + dbId: 1, + id: 'LCly_kkIN', + startDttm: Date.now(), + sqlEditorId: defaultQueryEditor.id, + extra: { progress: latestQueryProgressMsg }, + sql: 'select * from table1', + }, + lXJa7F9_r: { + cached: false, + changed_on: denormalizeTimestamp(new Date(1559238500401).toISOString()), + db: 'main', + dbId: 1, + id: 'lXJa7F9_r', + startDttm: 1559238500401, + sqlEditorId: defaultQueryEditor.id, + sql: 'select * from table2', + }, + '2g2_iRFMl': { + cached: false, + changed_on: denormalizeTimestamp(new Date(1559238506925).toISOString()), + db: 'main', + dbId: 1, + id: '2g2_iRFMl', + startDttm: 1559238506925, + sqlEditorId: defaultQueryEditor.id, + sql: 'select * from table3', + }, + expired_query_id: { + cached: false, + changed_on: denormalizeTimestamp( + new Date(expireDateTime).toISOString(), + ), + db: 'main', + dbId: 1, + id: 'expired_query_id', + startDttm: expireDateTime, + sqlEditorId: defaultQueryEditor.id, + sql: 'select * from table4', + }, + }, + }, +}; + +test('Renders an empty state for results', async () => { + const { getByText } = render(, { + useRedux: true, + initialState: mockState, + }); + const emptyStateText = getByText(/run a query to display results/i); + expect(emptyStateText).toBeVisible(); +}); + +test('Renders an empty state for expired results', async () => { + const { getByText } = render(, { + useRedux: true, + initialState: mockState, + }); + const emptyStateText = getByText(/run a query to display results/i); + expect(emptyStateText).toBeVisible(); +}); + +test('should pass latest query down to ResultSet component', async () => { + const { getByText } = render(, { + useRedux: true, + initialState: mockState, + }); + expect(getByText(latestQueryProgressMsg)).toBeVisible(); +}); diff --git a/superset-frontend/src/SqlLab/components/SouthPane/Results.tsx b/superset-frontend/src/SqlLab/components/SouthPane/Results.tsx new file mode 100644 index 0000000000..4e1b6219ae --- /dev/null +++ b/superset-frontend/src/SqlLab/components/SouthPane/Results.tsx @@ -0,0 +1,106 @@ +/** + * 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 { shallowEqual, useSelector } from 'react-redux'; +import Alert from 'src/components/Alert'; +import { EmptyStateMedium } from 'src/components/EmptyState'; +import { FeatureFlag, styled, t, isFeatureEnabled } from '@superset-ui/core'; + +import { SqlLabRootState } from 'src/SqlLab/types'; +import ResultSet from '../ResultSet'; +import { LOCALSTORAGE_MAX_QUERY_AGE_MS } from '../../constants'; + +const EXTRA_HEIGHT_RESULTS = 8; // we need extra height in RESULTS tab. because the height from props was calculated based on PREVIEW tab. + +type Props = { + latestQueryId: string; + height: number; + displayLimit: number; + defaultQueryLimit: number; +}; + +const StyledEmptyStateWrapper = styled.div` + height: 100%; + .ant-empty-image img { + margin-right: 28px; + } + + p { + margin-right: 28px; + } +`; + +const Results: React.FC = ({ + latestQueryId, + height, + displayLimit, + defaultQueryLimit, +}) => { + const databases = useSelector( + ({ sqlLab: { databases } }: SqlLabRootState) => databases, + shallowEqual, + ); + const latestQuery = useSelector( + ({ sqlLab: { queries } }: SqlLabRootState) => queries[latestQueryId || ''], + shallowEqual, + ); + + if ( + !latestQuery || + Date.now() - latestQuery.startDttm > LOCALSTORAGE_MAX_QUERY_AGE_MS + ) { + return ( + + + + ); + } + + if ( + isFeatureEnabled(FeatureFlag.SQLLAB_BACKEND_PERSISTENCE) && + latestQuery.state === 'success' && + !latestQuery.resultsKey && + !latestQuery.results + ) { + return ( + + ); + } + + return ( + + ); +}; + +export default Results; diff --git a/superset-frontend/src/SqlLab/components/SouthPane/SouthPane.test.tsx b/superset-frontend/src/SqlLab/components/SouthPane/SouthPane.test.tsx index 80a102ff21..c978a4ca32 100644 --- a/superset-frontend/src/SqlLab/components/SouthPane/SouthPane.test.tsx +++ b/superset-frontend/src/SqlLab/components/SouthPane/SouthPane.test.tsx @@ -17,15 +17,12 @@ * under the License. */ import React from 'react'; -import configureStore from 'redux-mock-store'; -import thunk from 'redux-thunk'; -import { render, screen, waitFor } from 'spec/helpers/testing-library'; -import SouthPane, { SouthPaneProps } from 'src/SqlLab/components/SouthPane'; +import { render } from 'spec/helpers/testing-library'; +import SouthPane from 'src/SqlLab/components/SouthPane'; import '@testing-library/jest-dom/extend-expect'; import { STATUS_OPTIONS } from 'src/SqlLab/constants'; import { initialState, table, defaultQueryEditor } from 'src/SqlLab/fixtures'; import { denormalizeTimestamp } from '@superset-ui/core'; -import { Store } from 'redux'; const mockedProps = { queryEditorId: defaultQueryEditor.id, @@ -37,29 +34,32 @@ const mockedProps = { const mockedEmptyProps = { queryEditorId: 'random_id', - latestQueryId: '', + latestQueryId: 'empty_query_id', height: 100, displayLimit: 100, defaultQueryLimit: 100, }; -jest.mock('src/SqlLab/components/SqlEditorLeftBar', () => jest.fn()); - const latestQueryProgressMsg = 'LATEST QUERY MESSAGE - LCly_kkIN'; -const middlewares = [thunk]; -const mockStore = configureStore(middlewares); -const store = mockStore({ +const mockState = { ...initialState, sqlLab: { - ...initialState, + ...initialState.sqlLab, offline: false, tables: [ { ...table, + name: 'table3', dataPreviewQueryId: '2g2_iRFMl', queryEditorId: defaultQueryEditor.id, }, + { + ...table, + name: 'table4', + dataPreviewQueryId: 'erWdqEWPm', + queryEditorId: defaultQueryEditor.id, + }, ], databases: {}, queries: { @@ -72,6 +72,7 @@ const store = mockStore({ startDttm: Date.now(), sqlEditorId: defaultQueryEditor.id, extra: { progress: latestQueryProgressMsg }, + sql: 'select * from table1', }, lXJa7F9_r: { cached: false, @@ -81,6 +82,7 @@ const store = mockStore({ id: 'lXJa7F9_r', startDttm: 1559238500401, sqlEditorId: defaultQueryEditor.id, + sql: 'select * from table2', }, '2g2_iRFMl': { cached: false, @@ -90,6 +92,7 @@ const store = mockStore({ id: '2g2_iRFMl', startDttm: 1559238506925, sqlEditorId: defaultQueryEditor.id, + sql: 'select * from table3', }, erWdqEWPm: { cached: false, @@ -99,44 +102,38 @@ const store = mockStore({ id: 'erWdqEWPm', startDttm: 1559238516395, sqlEditorId: defaultQueryEditor.id, + sql: 'select * from table4', }, }, }, -}); -const setup = (props: SouthPaneProps, store: Store) => - render(, { +}; + +test('should render offline when the state is offline', async () => { + const { getByText } = render(, { useRedux: true, - ...(store && { store }), + initialState: { + ...initialState, + sqlLab: { + ...initialState.sqlLab, + offline: true, + }, + }, }); -describe('SouthPane', () => { - const renderAndWait = (props: SouthPaneProps, store: Store) => - waitFor(async () => setup(props, store)); + expect(getByText(STATUS_OPTIONS.offline)).toBeVisible(); +}); - it('Renders an empty state for results', async () => { - await renderAndWait(mockedEmptyProps, store); - const emptyStateText = screen.getByText(/run a query to display results/i); - expect(emptyStateText).toBeVisible(); +test('should render tabs for table preview queries', () => { + const { getAllByRole } = render(, { + useRedux: true, + initialState: mockState, }); - it('should render offline when the state is offline', async () => { - await renderAndWait( - mockedEmptyProps, - mockStore({ - ...initialState, - sqlLab: { - ...initialState.sqlLab, - offline: true, - }, - }), - ); - - expect(screen.getByText(STATUS_OPTIONS.offline)).toBeVisible(); - }); - - it('should pass latest query down to ResultSet component', async () => { - await renderAndWait(mockedProps, store); - - expect(screen.getByText(latestQueryProgressMsg)).toBeVisible(); + const tabs = getAllByRole('tab'); + expect(tabs).toHaveLength(mockState.sqlLab.tables.length + 2); + expect(tabs[0]).toHaveTextContent('Results'); + expect(tabs[1]).toHaveTextContent('Query history'); + mockState.sqlLab.tables.forEach(({ name }, index) => { + expect(tabs[index + 2]).toHaveTextContent(`Preview: \`${name}\``); }); }); diff --git a/superset-frontend/src/SqlLab/components/SouthPane/index.tsx b/superset-frontend/src/SqlLab/components/SouthPane/index.tsx index 38a20f9f6d..0bbce99b1c 100644 --- a/superset-frontend/src/SqlLab/components/SouthPane/index.tsx +++ b/superset-frontend/src/SqlLab/components/SouthPane/index.tsx @@ -19,10 +19,8 @@ import React, { createRef, useMemo } from 'react'; import { shallowEqual, useDispatch, useSelector } from 'react-redux'; import shortid from 'shortid'; -import Alert from 'src/components/Alert'; import Tabs from 'src/components/Tabs'; -import { EmptyStateMedium } from 'src/components/EmptyState'; -import { FeatureFlag, styled, t, isFeatureEnabled } from '@superset-ui/core'; +import { styled, t } from '@superset-ui/core'; import { setActiveSouthPaneTab } from 'src/SqlLab/actions/sqlLab'; @@ -33,11 +31,11 @@ import ResultSet from '../ResultSet'; import { STATUS_OPTIONS, STATE_TYPE_MAP, - LOCALSTORAGE_MAX_QUERY_AGE_MS, STATUS_OPTIONS_LOCALIZED, } from '../../constants'; +import Results from './Results'; -const TAB_HEIGHT = 140; +const TAB_HEIGHT = 130; /* editorQueries are queries executed by users passed from SqlEditor component @@ -85,18 +83,6 @@ const StyledPane = styled.div` } `; -const EXTRA_HEIGHT_RESULTS = 24; // we need extra height in RESULTS tab. because the height from props was calculated based on PREVIEW tab. -const StyledEmptyStateWrapper = styled.div` - height: 100%; - .ant-empty-image img { - margin-right: 28px; - } - - p { - margin-right: 28px; - } -`; - const SouthPane = ({ queryEditorId, latestQueryId, @@ -105,128 +91,43 @@ const SouthPane = ({ defaultQueryLimit, }: SouthPaneProps) => { const dispatch = useDispatch(); - const user = useSelector(({ user }: SqlLabRootState) => user, shallowEqual); - const { databases, offline, queries, tables } = useSelector( - ({ sqlLab: { databases, offline, queries, tables } }: SqlLabRootState) => ({ - databases, + const { offline, tables } = useSelector( + ({ sqlLab: { offline, tables } }: SqlLabRootState) => ({ offline, - queries, tables, }), shallowEqual, ); - const editorQueries = useMemo( - () => - Object.values(queries).filter( - ({ sqlEditorId }) => sqlEditorId === queryEditorId, - ), - [queries, queryEditorId], + const queries = useSelector( + ({ sqlLab: { queries } }: SqlLabRootState) => Object.keys(queries), + shallowEqual, ); - const dataPreviewQueries = useMemo( - () => - tables - .filter( - ({ dataPreviewQueryId, queryEditorId: qeId }) => - dataPreviewQueryId && - queryEditorId === qeId && - queries[dataPreviewQueryId], - ) - .map(({ name, dataPreviewQueryId }) => ({ - ...queries[dataPreviewQueryId || ''], - tableName: name, - })), - [queries, queryEditorId, tables], - ); - const latestQuery = useMemo( - () => editorQueries.find(({ id }) => id === latestQueryId), - [editorQueries, latestQueryId], - ); - const activeSouthPaneTab = useSelector( state => state.sqlLab.activeSouthPaneTab as string, ) ?? 'Results'; + + const querySet = useMemo(() => new Set(queries), [queries]); + const dataPreviewQueries = useMemo( + () => + tables.filter( + ({ dataPreviewQueryId, queryEditorId: qeId }) => + dataPreviewQueryId && + queryEditorId === qeId && + querySet.has(dataPreviewQueryId), + ), + [queryEditorId, tables, querySet], + ); const innerTabContentHeight = height - TAB_HEIGHT; const southPaneRef = createRef(); const switchTab = (id: string) => { dispatch(setActiveSouthPaneTab(id)); }; - const renderOfflineStatus = () => ( + + return offline ? ( - ); - - const renderResults = () => { - let results; - if (latestQuery) { - if (latestQuery?.extra?.errors) { - latestQuery.errors = latestQuery.extra.errors; - } - if ( - isFeatureEnabled(FeatureFlag.SQLLAB_BACKEND_PERSISTENCE) && - latestQuery.state === 'success' && - !latestQuery.resultsKey && - !latestQuery.results - ) { - results = ( - - ); - return results; - } - if (Date.now() - latestQuery.startDttm <= LOCALSTORAGE_MAX_QUERY_AGE_MS) { - results = ( - - ); - } - } else { - results = ( - - - - ); - } - return results; - }; - - const renderDataPreviewTabs = () => - dataPreviewQueries.map(query => ( - - - - )); - return offline ? ( - renderOfflineStatus() ) : ( - {renderResults()} + {latestQueryId && ( + + )} - {renderDataPreviewTabs()} + {dataPreviewQueries.map( + ({ name, dataPreviewQueryId }) => + dataPreviewQueryId && ( + + + + ), + )} ); diff --git a/superset-frontend/src/SqlLab/components/SqlEditor/SqlEditor.test.tsx b/superset-frontend/src/SqlLab/components/SqlEditor/SqlEditor.test.tsx index 63f67170d0..6a25492ce5 100644 --- a/superset-frontend/src/SqlLab/components/SqlEditor/SqlEditor.test.tsx +++ b/superset-frontend/src/SqlLab/components/SqlEditor/SqlEditor.test.tsx @@ -145,8 +145,8 @@ describe('SqlEditor', () => { (SqlEditorLeftBar as jest.Mock).mockImplementation(() => (
)); - (ResultSet as jest.Mock).mockClear(); - (ResultSet as jest.Mock).mockImplementation(() => ( + (ResultSet as unknown as jest.Mock).mockClear(); + (ResultSet as unknown as jest.Mock).mockImplementation(() => (
)); }); @@ -182,7 +182,8 @@ describe('SqlEditor', () => { const editor = await findByTestId('react-ace'); const sql = 'select *'; const renderCount = (SqlEditorLeftBar as jest.Mock).mock.calls.length; - const renderCountForSouthPane = (ResultSet as jest.Mock).mock.calls.length; + const renderCountForSouthPane = (ResultSet as unknown as jest.Mock).mock + .calls.length; expect(SqlEditorLeftBar).toHaveBeenCalledTimes(renderCount); expect(ResultSet).toHaveBeenCalledTimes(renderCountForSouthPane); fireEvent.change(editor, { target: { value: sql } }); diff --git a/superset-frontend/src/SqlLab/reducers/sqlLab.js b/superset-frontend/src/SqlLab/reducers/sqlLab.js index 59bd0558a1..ce9eed9b9d 100644 --- a/superset-frontend/src/SqlLab/reducers/sqlLab.js +++ b/superset-frontend/src/SqlLab/reducers/sqlLab.js @@ -345,7 +345,7 @@ export default function sqlLabReducer(state = {}, action) { return state; } const alts = { - endDttm: now(), + endDttm: action?.results?.query?.endDttm || now(), progress: 100, results: action.results, rows: action?.results?.query?.rows || 0, @@ -674,7 +674,14 @@ export default function sqlLabReducer(state = {}, action) { if (!change) { newQueries = state.queries; } - return { ...state, queries: newQueries, queriesLastUpdate }; + return { + ...state, + queries: newQueries, + queriesLastUpdate: + queriesLastUpdate > state.queriesLastUpdate + ? queriesLastUpdate + : Date.now(), + }; }, [actions.CLEAR_INACTIVE_QUERIES]() { const { queries } = state; @@ -701,7 +708,11 @@ export default function sqlLabReducer(state = {}, action) { }, ]), ); - return { ...state, queries: cleanedQueries }; + return { + ...state, + queries: cleanedQueries, + queriesLastUpdate: Date.now(), + }; }, [actions.SET_USER_OFFLINE]() { return { ...state, offline: action.offline }; diff --git a/superset-frontend/src/SqlLab/reducers/sqlLab.test.js b/superset-frontend/src/SqlLab/reducers/sqlLab.test.js index e1a234734b..5a70f10bb3 100644 --- a/superset-frontend/src/SqlLab/reducers/sqlLab.test.js +++ b/superset-frontend/src/SqlLab/reducers/sqlLab.test.js @@ -20,6 +20,7 @@ import { QueryState } from '@superset-ui/core'; import sqlLabReducer from 'src/SqlLab/reducers/sqlLab'; import * as actions from 'src/SqlLab/actions/sqlLab'; import { table, initialState as mockState } from '../fixtures'; +import { QUERY_UPDATE_FREQ } from '../components/QueryAutoRefresh'; const initialState = mockState.sqlLab; @@ -404,6 +405,7 @@ describe('sqlLabReducer', () => { }; }); it('updates queries that have already been completed', () => { + const current = Date.now(); newState = sqlLabReducer( { ...newState, @@ -418,9 +420,10 @@ describe('sqlLabReducer', () => { }, }, }, - actions.clearInactiveQueries(Date.now()), + actions.clearInactiveQueries(QUERY_UPDATE_FREQ), ); expect(newState.queries.abcd.state).toBe(QueryState.SUCCESS); + expect(newState.queriesLastUpdate).toBeGreaterThanOrEqual(current); }); }); });