From 8d1b7ecfde4eadbf74af1a467adaf69102acc404 Mon Sep 17 00:00:00 2001 From: "JUST.in DO IT" Date: Sun, 2 Oct 2022 20:00:53 -0700 Subject: [PATCH] fix(sqllab): perf regression on #21532 refactor (#21632) --- .../spec/helpers/testing-library.tsx | 2 +- .../AceEditorWrapper.test.tsx | 2 +- .../components/AceEditorWrapper/index.tsx | 18 +- .../QueryLimitSelect.test.tsx | 24 ++- .../components/QueryLimitSelect/index.tsx | 19 +- .../RunQueryActionButton.test.tsx | 177 +++++++++--------- .../components/RunQueryActionButton/index.tsx | 40 ++-- ...{SaveQuery.test.jsx => SaveQuery.test.tsx} | 55 ++++-- .../src/SqlLab/components/SaveQuery/index.tsx | 79 ++++---- ...ery.test.jsx => ShareSqlLabQuery.test.tsx} | 59 +++--- .../components/ShareSqlLabQuery/index.tsx | 21 +-- .../src/SqlLab/components/SqlEditor/index.jsx | 23 +-- .../components/SqlEditorLeftBar/index.tsx | 14 +- .../src/SqlLab/hooks/useQueryEditor/index.ts | 38 ++++ .../useQueryEditor/useQueryEditor.test.ts | 92 +++++++++ .../src/SqlLab/reducers/sqlLab.js | 2 +- 16 files changed, 408 insertions(+), 257 deletions(-) rename superset-frontend/src/SqlLab/components/SaveQuery/{SaveQuery.test.jsx => SaveQuery.test.tsx} (89%) rename superset-frontend/src/SqlLab/components/ShareSqlLabQuery/{ShareSqlLabQuery.test.jsx => ShareSqlLabQuery.test.tsx} (85%) create mode 100644 superset-frontend/src/SqlLab/hooks/useQueryEditor/index.ts create mode 100644 superset-frontend/src/SqlLab/hooks/useQueryEditor/useQueryEditor.test.ts diff --git a/superset-frontend/spec/helpers/testing-library.tsx b/superset-frontend/spec/helpers/testing-library.tsx index d489ec2deaf2a..25b0324fe1864 100644 --- a/superset-frontend/spec/helpers/testing-library.tsx +++ b/superset-frontend/spec/helpers/testing-library.tsx @@ -47,7 +47,7 @@ type Options = Omit & { store?: Store; }; -function createWrapper(options?: Options) { +export function createWrapper(options?: Options) { const { useDnd, useRedux, diff --git a/superset-frontend/src/SqlLab/components/AceEditorWrapper/AceEditorWrapper.test.tsx b/superset-frontend/src/SqlLab/components/AceEditorWrapper/AceEditorWrapper.test.tsx index 0fd5c7d3e86d8..7638003e9025c 100644 --- a/superset-frontend/src/SqlLab/components/AceEditorWrapper/AceEditorWrapper.test.tsx +++ b/superset-frontend/src/SqlLab/components/AceEditorWrapper/AceEditorWrapper.test.tsx @@ -48,7 +48,7 @@ jest.mock('src/components/AsyncAceEditor', () => ({ const setup = (queryEditor: QueryEditor, store?: Store) => render( void; onChange: (sql: string) => void; - queryEditor: QueryEditor; + queryEditorId: string; database: any; extendedTables?: Array<{ name: string; columns: any[] }>; height: string; @@ -61,7 +61,7 @@ const AceEditorWrapper = ({ autocomplete, onBlur = () => {}, onChange = () => {}, - queryEditor, + queryEditorId, database, extendedTables = [], height, @@ -69,7 +69,17 @@ const AceEditorWrapper = ({ }: AceEditorWrapperProps) => { const dispatch = useDispatch(); - const { sql: currentSql } = queryEditor; + const queryEditor = useQueryEditor(queryEditorId, [ + 'id', + 'dbId', + 'sql', + 'functionNames', + 'schemaOptions', + 'tableOptions', + 'validationResult', + 'schema', + ]); + const currentSql = queryEditor.sql ?? ''; const functionNames = queryEditor.functionNames ?? []; const schemas = queryEditor.schemaOptions ?? []; const tables = queryEditor.tableOptions ?? []; diff --git a/superset-frontend/src/SqlLab/components/QueryLimitSelect/QueryLimitSelect.test.tsx b/superset-frontend/src/SqlLab/components/QueryLimitSelect/QueryLimitSelect.test.tsx index c640d5a9dfed7..317a8d02ba51f 100644 --- a/superset-frontend/src/SqlLab/components/QueryLimitSelect/QueryLimitSelect.test.tsx +++ b/superset-frontend/src/SqlLab/components/QueryLimitSelect/QueryLimitSelect.test.tsx @@ -51,7 +51,7 @@ const defaultQueryLimit = 100; const setup = (props?: Partial, store?: Store) => render( { const queryLimit = 10; const { getByText } = setup( { - queryEditor: { - ...defaultQueryEditor, - queryLimit, - }, + queryEditorId: defaultQueryEditor.id, }, - mockStore(initialState), + mockStore({ + ...initialState, + sqlLab: { + ...initialState.sqlLab, + queryEditors: [ + { + ...defaultQueryEditor, + queryLimit, + }, + ], + }, + }), ); expect(getByText(queryLimit)).toBeInTheDocument(); }); @@ -129,7 +137,9 @@ describe('QueryLimitSelect', () => { { type: 'QUERY_EDITOR_SET_QUERY_LIMIT', queryLimit: LIMIT_DROPDOWN[expectedIndex], - queryEditor: defaultQueryEditor, + queryEditor: { + id: defaultQueryEditor.id, + }, }, ]), ); diff --git a/superset-frontend/src/SqlLab/components/QueryLimitSelect/index.tsx b/superset-frontend/src/SqlLab/components/QueryLimitSelect/index.tsx index f438ebc59edc0..886e139a98e5e 100644 --- a/superset-frontend/src/SqlLab/components/QueryLimitSelect/index.tsx +++ b/superset-frontend/src/SqlLab/components/QueryLimitSelect/index.tsx @@ -17,16 +17,16 @@ * under the License. */ import React from 'react'; -import { useSelector, useDispatch } from 'react-redux'; +import { useDispatch } from 'react-redux'; import { styled, useTheme } from '@superset-ui/core'; import { AntdDropdown } from 'src/components'; import { Menu } from 'src/components/Menu'; import Icons from 'src/components/Icons'; -import { SqlLabRootState, QueryEditor } from 'src/SqlLab/types'; import { queryEditorSetQueryLimit } from 'src/SqlLab/actions/sqlLab'; +import useQueryEditor from 'src/SqlLab/hooks/useQueryEditor'; export interface QueryLimitSelectProps { - queryEditor: QueryEditor; + queryEditorId: string; maxRow: number; defaultQueryLimit: number; } @@ -79,19 +79,12 @@ function renderQueryLimit( } const QueryLimitSelect = ({ - queryEditor, + queryEditorId, maxRow, defaultQueryLimit, }: QueryLimitSelectProps) => { - const queryLimit = useSelector( - ({ sqlLab: { unsavedQueryEditor } }) => { - const updatedQueryEditor = { - ...queryEditor, - ...(unsavedQueryEditor.id === queryEditor.id && unsavedQueryEditor), - }; - return updatedQueryEditor.queryLimit || defaultQueryLimit; - }, - ); + const queryEditor = useQueryEditor(queryEditorId, ['id', 'queryLimit']); + const queryLimit = queryEditor.queryLimit || defaultQueryLimit; const dispatch = useDispatch(); const setQueryLimit = (updatedQueryLimit: number) => dispatch(queryEditorSetQueryLimit(queryEditor, updatedQueryLimit)); diff --git a/superset-frontend/src/SqlLab/components/RunQueryActionButton/RunQueryActionButton.test.tsx b/superset-frontend/src/SqlLab/components/RunQueryActionButton/RunQueryActionButton.test.tsx index 189a5fd2f7ab9..7062ad694a2e2 100644 --- a/superset-frontend/src/SqlLab/components/RunQueryActionButton/RunQueryActionButton.test.tsx +++ b/superset-frontend/src/SqlLab/components/RunQueryActionButton/RunQueryActionButton.test.tsx @@ -41,13 +41,13 @@ jest.mock('src/components/Select/AsyncSelect', () => () => ( )); const defaultProps = { - queryEditor: defaultQueryEditor, + queryEditorId: defaultQueryEditor.id, allowAsync: false, dbId: 1, queryState: 'ready', - runQuery: jest.fn(), + runQuery: () => {}, selectedText: null, - stopQuery: jest.fn(), + stopQuery: () => {}, overlayCreateAsMenu: null, }; @@ -57,95 +57,104 @@ const setup = (props?: Partial, store?: Store) => ...(store && { store }), }); -describe('RunQueryActionButton', () => { - beforeEach(() => { - defaultProps.runQuery.mockReset(); - defaultProps.stopQuery.mockReset(); - }); +it('renders a single Button', () => { + const { getByRole } = setup({}, mockStore(initialState)); + expect(getByRole('button')).toBeInTheDocument(); +}); - it('renders a single Button', () => { - const { getByRole } = setup({}, mockStore(initialState)); - expect(getByRole('button')).toBeInTheDocument(); - }); +it('renders a label for Run Query', () => { + const { getByText } = setup({}, mockStore(initialState)); + expect(getByText('Run')).toBeInTheDocument(); +}); - it('renders a label for Run Query', () => { - const { getByText } = setup({}, mockStore(initialState)); - expect(getByText('Run')).toBeInTheDocument(); - }); +it('renders a label for Selected Query', () => { + const { getByText } = setup( + {}, + mockStore({ + ...initialState, + sqlLab: { + ...initialState.sqlLab, + unsavedQueryEditor: { + id: defaultQueryEditor.id, + selectedText: 'select * from\n-- this is comment\nwhere', + }, + }, + }), + ); + expect(getByText('Run selection')).toBeInTheDocument(); +}); - it('renders a label for Selected Query', () => { - const { getByText } = setup( - {}, - mockStore({ - ...initialState, - sqlLab: { - ...initialState.sqlLab, - unsavedQueryEditor: { - id: defaultQueryEditor.id, - selectedText: 'FROM', - }, +it('disable button when sql from unsaved changes is empty', () => { + const { getByRole } = setup( + {}, + mockStore({ + ...initialState, + sqlLab: { + ...initialState.sqlLab, + unsavedQueryEditor: { + id: defaultQueryEditor.id, + sql: '', }, - }), - ); - expect(getByText('Run selection')).toBeInTheDocument(); - }); + }, + }), + ); + const button = getByRole('button'); + expect(button).toBeDisabled(); +}); - it('disable button when sql from unsaved changes is empty', () => { - const { getByRole } = setup( - {}, - mockStore({ - ...initialState, - sqlLab: { - ...initialState.sqlLab, - unsavedQueryEditor: { - id: defaultQueryEditor.id, - sql: '', - }, +it('disable button when selectedText only contains blank contents', () => { + const { getByRole } = setup( + {}, + mockStore({ + ...initialState, + sqlLab: { + ...initialState.sqlLab, + unsavedQueryEditor: { + id: defaultQueryEditor.id, + selectedText: '-- this is comment\n\n \t', }, - }), - ); - const button = getByRole('button'); - expect(button).toBeDisabled(); - }); + }, + }), + ); + const button = getByRole('button'); + expect(button).toBeDisabled(); +}); - it('enable default button for unrelated unsaved changes', () => { - const { getByRole } = setup( - {}, - mockStore({ - ...initialState, - sqlLab: { - ...initialState.sqlLab, - unsavedQueryEditor: { - id: `${defaultQueryEditor.id}-other`, - sql: '', - }, +it('enable default button for unrelated unsaved changes', () => { + const { getByRole } = setup( + {}, + mockStore({ + ...initialState, + sqlLab: { + ...initialState.sqlLab, + unsavedQueryEditor: { + id: `${defaultQueryEditor.id}-other`, + sql: '', }, - }), - ); - const button = getByRole('button'); - expect(button).toBeEnabled(); - }); + }, + }), + ); + const button = getByRole('button'); + expect(button).toBeEnabled(); +}); - it('dispatch runQuery on click', async () => { - const { getByRole } = setup({}, mockStore(initialState)); - const button = getByRole('button'); - expect(defaultProps.runQuery).toHaveBeenCalledTimes(0); - fireEvent.click(button); - await waitFor(() => expect(defaultProps.runQuery).toHaveBeenCalledTimes(1)); - }); +it('dispatch runQuery on click', async () => { + const runQuery = jest.fn(); + const { getByRole } = setup({ runQuery }, mockStore(initialState)); + const button = getByRole('button'); + expect(runQuery).toHaveBeenCalledTimes(0); + fireEvent.click(button); + await waitFor(() => expect(runQuery).toHaveBeenCalledTimes(1)); +}); - describe('on running state', () => { - it('dispatch stopQuery on click', async () => { - const { getByRole } = setup( - { queryState: 'running' }, - mockStore(initialState), - ); - const button = getByRole('button'); - expect(defaultProps.stopQuery).toHaveBeenCalledTimes(0); - fireEvent.click(button); - await waitFor(() => - expect(defaultProps.stopQuery).toHaveBeenCalledTimes(1), - ); - }); - }); +it('dispatch stopQuery on click while running state', async () => { + const stopQuery = jest.fn(); + const { getByRole } = setup( + { queryState: 'running', stopQuery }, + mockStore(initialState), + ); + const button = getByRole('button'); + expect(stopQuery).toHaveBeenCalledTimes(0); + fireEvent.click(button); + await waitFor(() => expect(stopQuery).toHaveBeenCalledTimes(1)); }); diff --git a/superset-frontend/src/SqlLab/components/RunQueryActionButton/index.tsx b/superset-frontend/src/SqlLab/components/RunQueryActionButton/index.tsx index 5cc453f5ee93e..a85b4b6b7bb88 100644 --- a/superset-frontend/src/SqlLab/components/RunQueryActionButton/index.tsx +++ b/superset-frontend/src/SqlLab/components/RunQueryActionButton/index.tsx @@ -24,16 +24,11 @@ import Button from 'src/components/Button'; import Icons from 'src/components/Icons'; import { DropdownButton } from 'src/components/DropdownButton'; import { detectOS } from 'src/utils/common'; -import { shallowEqual, useSelector } from 'react-redux'; -import { - QueryEditor, - SqlLabRootState, - QueryButtonProps, -} from 'src/SqlLab/types'; -import { getUpToDateQuery } from 'src/SqlLab/actions/sqlLab'; +import { QueryButtonProps } from 'src/SqlLab/types'; +import useQueryEditor from 'src/SqlLab/hooks/useQueryEditor'; export interface Props { - queryEditor: QueryEditor; + queryEditorId: string; allowAsync: boolean; queryState?: string; runQuery: (c?: boolean) => void; @@ -86,29 +81,21 @@ const StyledButton = styled.span` } `; -const RunQueryActionButton = ({ +const RunQueryActionButton: React.FC = ({ allowAsync = false, - queryEditor, + queryEditorId, queryState, overlayCreateAsMenu, runQuery, stopQuery, -}: Props) => { +}) => { const theme = useTheme(); const userOS = detectOS(); - const { selectedText, sql } = useSelector< - SqlLabRootState, - Pick - >(rootState => { - const currentQueryEditor = getUpToDateQuery( - rootState, - queryEditor, - ) as unknown as QueryEditor; - return { - selectedText: currentQueryEditor.selectedText, - sql: currentQueryEditor.sql, - }; - }, shallowEqual); + + const { selectedText, sql } = useQueryEditor(queryEditorId, [ + 'selectedText', + 'sql', + ]); const shouldShowStopBtn = !!queryState && ['running', 'pending'].indexOf(queryState) > -1; @@ -117,7 +104,10 @@ const RunQueryActionButton = ({ ? (DropdownButton as React.FC) : Button; - const isDisabled = !sql || !sql.trim(); + const sqlContent = selectedText || sql || ''; + const isDisabled = + !sqlContent || + !sqlContent.replace(/(\/\*[^*]*\*\/)|(\/\/[^*]*)|(--[^.].*)/gm, '').trim(); const stopButtonTooltipText = useMemo( () => diff --git a/superset-frontend/src/SqlLab/components/SaveQuery/SaveQuery.test.jsx b/superset-frontend/src/SqlLab/components/SaveQuery/SaveQuery.test.tsx similarity index 89% rename from superset-frontend/src/SqlLab/components/SaveQuery/SaveQuery.test.jsx rename to superset-frontend/src/SqlLab/components/SaveQuery/SaveQuery.test.tsx index 2a5fcf3eb79fd..f321a54ec4dbe 100644 --- a/superset-frontend/src/SqlLab/components/SaveQuery/SaveQuery.test.jsx +++ b/superset-frontend/src/SqlLab/components/SaveQuery/SaveQuery.test.tsx @@ -25,15 +25,28 @@ import SaveQuery from 'src/SqlLab/components/SaveQuery'; import { initialState, databases } from 'src/SqlLab/fixtures'; const mockedProps = { - queryEditor: { - dbId: 1, - schema: 'main', - sql: 'SELECT * FROM t', - }, + queryEditorId: '123', animation: false, database: databases.result[0], onUpdate: () => {}, onSave: () => {}, + saveQueryWarning: null, + columns: [], +}; + +const mockState = { + ...initialState, + sqlLab: { + ...initialState.sqlLab, + queryEditors: [ + { + id: mockedProps.queryEditorId, + dbId: 1, + schema: 'main', + sql: 'SELECT * FROM t', + }, + ], + }, }; const splitSaveBtnProps = { @@ -51,7 +64,7 @@ describe('SavedQuery', () => { it('renders a non-split save button when allows_virtual_table_explore is not enabled', () => { render(, { useRedux: true, - store: mockStore(initialState), + store: mockStore(mockState), }); const saveBtn = screen.getByRole('button', { name: /save/i }); @@ -62,7 +75,7 @@ describe('SavedQuery', () => { it('renders a save query modal when user clicks save button', () => { render(, { useRedux: true, - store: mockStore(initialState), + store: mockStore(mockState), }); const saveBtn = screen.getByRole('button', { name: /save/i }); @@ -78,7 +91,7 @@ describe('SavedQuery', () => { it('renders the save query modal UI', () => { render(, { useRedux: true, - store: mockStore(initialState), + store: mockStore(mockState), }); const saveBtn = screen.getByRole('button', { name: /save/i }); @@ -111,16 +124,18 @@ describe('SavedQuery', () => { }); it('renders a "save as new" and "update" button if query already exists', () => { - const props = { - ...mockedProps, - queryEditor: { - ...mockedProps.query, - remoteId: '42', - }, - }; - render(, { + render(, { useRedux: true, - store: mockStore(initialState), + store: mockStore({ + ...mockState, + sqlLab: { + ...mockState.sqlLab, + unsavedQueryEditor: { + id: mockedProps.queryEditorId, + remoteId: '42', + }, + }, + }), }); const saveBtn = screen.getByRole('button', { name: /save/i }); @@ -136,7 +151,7 @@ describe('SavedQuery', () => { it('renders a split save button when allows_virtual_table_explore is enabled', async () => { render(, { useRedux: true, - store: mockStore(initialState), + store: mockStore(mockState), }); await waitFor(() => { @@ -151,7 +166,7 @@ describe('SavedQuery', () => { it('renders a save dataset modal when user clicks "save dataset" menu item', async () => { render(, { useRedux: true, - store: mockStore(initialState), + store: mockStore(mockState), }); await waitFor(() => { @@ -170,7 +185,7 @@ describe('SavedQuery', () => { it('renders the save dataset modal UI', async () => { render(, { useRedux: true, - store: mockStore(initialState), + store: mockStore(mockState), }); await waitFor(() => { diff --git a/superset-frontend/src/SqlLab/components/SaveQuery/index.tsx b/superset-frontend/src/SqlLab/components/SaveQuery/index.tsx index 38cd5625ecd68..eab37eceb375f 100644 --- a/superset-frontend/src/SqlLab/components/SaveQuery/index.tsx +++ b/superset-frontend/src/SqlLab/components/SaveQuery/index.tsx @@ -16,8 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import React, { useState, useEffect } from 'react'; -import { useSelector, shallowEqual } from 'react-redux'; +import React, { useState, useEffect, useMemo } from 'react'; import { Row, Col } from 'src/components'; import { Input, TextArea } from 'src/components/Input'; import { t, styled } from '@superset-ui/core'; @@ -31,10 +30,11 @@ import { ISaveableDatasource, } from 'src/SqlLab/components/SaveDatasetModal'; import { getDatasourceAsSaveableDataset } from 'src/utils/datasourceUtils'; -import { QueryEditor, SqlLabRootState } from 'src/SqlLab/types'; +import useQueryEditor from 'src/SqlLab/hooks/useQueryEditor'; +import { QueryEditor } from 'src/SqlLab/types'; interface SaveQueryProps { - queryEditor: QueryEditor; + queryEditorId: string; columns: ISaveableDatasource['columns']; onSave: (arg0: QueryPayload) => void; onUpdate: (arg0: QueryPayload) => void; @@ -43,30 +43,22 @@ interface SaveQueryProps { } type QueryPayload = { - autorun: boolean; - dbId: number; + name: string; description?: string; id?: string; - latestQueryId: string; - queryLimit: number; - remoteId: number; - schema: string; - schemaOptions: Array<{ - label: string; - title: string; - value: string; - }>; - selectedText: string | null; - sql: string; - tableOptions: Array<{ - label: string; - schema: string; - title: string; - type: string; - value: string; - }>; - name: string; -}; +} & Pick< + QueryEditor, + | 'autorun' + | 'dbId' + | 'schema' + | 'sql' + | 'selectedText' + | 'remoteId' + | 'latestQueryId' + | 'queryLimit' + | 'tableOptions' + | 'schemaOptions' +>; const Styles = styled.span` span[role='img'] { @@ -81,20 +73,33 @@ const Styles = styled.span` `; export default function SaveQuery({ - queryEditor, + queryEditorId, onSave = () => {}, onUpdate, saveQueryWarning = null, database, columns, }: SaveQueryProps) { - const query = useSelector( - ({ sqlLab: { unsavedQueryEditor } }) => ({ + const queryEditor = useQueryEditor(queryEditorId, [ + 'autorun', + 'name', + 'description', + 'remoteId', + 'dbId', + 'latestQueryId', + 'queryLimit', + 'schema', + 'schemaOptions', + 'selectedText', + 'sql', + 'tableOptions', + ]); + const query = useMemo( + () => ({ ...queryEditor, - ...(queryEditor.id === unsavedQueryEditor.id && unsavedQueryEditor), columns, }), - shallowEqual, + [queryEditor, columns], ); const defaultLabel = query.name || query.description || t('Undefined'); const [description, setDescription] = useState( @@ -114,12 +119,12 @@ export default function SaveQuery({ ); - const queryPayload = () => - ({ - ...query, - name: label, - description, - } as any as QueryPayload); + const queryPayload = () => ({ + ...query, + name: label, + description, + dbId: query.dbId ?? 0, + }); useEffect(() => { if (!isSaved) setLabel(defaultLabel); diff --git a/superset-frontend/src/SqlLab/components/ShareSqlLabQuery/ShareSqlLabQuery.test.jsx b/superset-frontend/src/SqlLab/components/ShareSqlLabQuery/ShareSqlLabQuery.test.tsx similarity index 85% rename from superset-frontend/src/SqlLab/components/ShareSqlLabQuery/ShareSqlLabQuery.test.jsx rename to superset-frontend/src/SqlLab/components/ShareSqlLabQuery/ShareSqlLabQuery.test.tsx index 22913894fbc39..2c2038f5c4138 100644 --- a/superset-frontend/src/SqlLab/components/ShareSqlLabQuery/ShareSqlLabQuery.test.jsx +++ b/superset-frontend/src/SqlLab/components/ShareSqlLabQuery/ShareSqlLabQuery.test.tsx @@ -31,30 +31,42 @@ import ShareSqlLabQuery from 'src/SqlLab/components/ShareSqlLabQuery'; import { initialState } from 'src/SqlLab/fixtures'; const mockStore = configureStore([thunk]); -const store = mockStore(initialState); -let isFeatureEnabledMock; +const defaultProps = { + queryEditorId: 'qe1', + addDangerToast: jest.fn(), +}; +const mockQueryEditor = { + id: defaultProps.queryEditorId, + dbId: 0, + name: 'query title', + schema: 'query_schema', + autorun: false, + sql: 'SELECT * FROM ...', + remoteId: 999, +}; +const disabled = { + id: 'disabledEditorId', + remoteId: undefined, +}; + +const mockState = { + ...initialState, + sqlLab: { + ...initialState.sqlLab, + queryEditors: [mockQueryEditor, disabled], + }, +}; +const store = mockStore(mockState); +let isFeatureEnabledMock: jest.SpyInstance; -const standardProvider = ({ children }) => ( +const standardProvider: React.FC = ({ children }) => ( {children} ); -const defaultProps = { - queryEditor: { - id: 'qe1', - dbId: 0, - name: 'query title', - schema: 'query_schema', - autorun: false, - sql: 'SELECT * FROM ...', - remoteId: 999, - }, - addDangerToast: jest.fn(), -}; - const unsavedQueryEditor = { - id: defaultProps.queryEditor.id, + id: defaultProps.queryEditorId, dbId: 9888, name: 'query title changed', schema: 'query_schema_updated', @@ -62,7 +74,7 @@ const unsavedQueryEditor = { autorun: true, }; -const standardProviderWithUnsaved = ({ children }) => ( +const standardProviderWithUnsaved: React.FC = ({ children }) => ( { }); afterAll(() => { - isFeatureEnabledMock.restore(); + isFeatureEnabledMock.mockReset(); }); it('calls storeQuery() with the query when getCopyUrl() is called', async () => { @@ -110,7 +122,7 @@ describe('ShareSqlLabQuery', () => { }); }); const button = screen.getByRole('button'); - const { id, remoteId, ...expected } = defaultProps.queryEditor; + const { id, remoteId, ...expected } = mockQueryEditor; const storeQuerySpy = jest.spyOn(utils, 'storeQuery'); userEvent.click(button); expect(storeQuerySpy.mock.calls).toHaveLength(1); @@ -142,7 +154,7 @@ describe('ShareSqlLabQuery', () => { }); afterAll(() => { - isFeatureEnabledMock.restore(); + isFeatureEnabledMock.mockReset(); }); it('does not call storeQuery() with the query when getCopyUrl() is called and feature is not enabled', async () => { @@ -160,10 +172,7 @@ describe('ShareSqlLabQuery', () => { it('button is disabled and there is a request to save the query', async () => { const updatedProps = { - queryEditor: { - ...defaultProps.queryEditor, - remoteId: undefined, - }, + queryEditorId: disabled.id, }; render(, { diff --git a/superset-frontend/src/SqlLab/components/ShareSqlLabQuery/index.tsx b/superset-frontend/src/SqlLab/components/ShareSqlLabQuery/index.tsx index 37481bdee9249..f1e6d13c41ac0 100644 --- a/superset-frontend/src/SqlLab/components/ShareSqlLabQuery/index.tsx +++ b/superset-frontend/src/SqlLab/components/ShareSqlLabQuery/index.tsx @@ -17,7 +17,6 @@ * under the License. */ import React from 'react'; -import { shallowEqual, useSelector } from 'react-redux'; import { t, useTheme, styled } from '@superset-ui/core'; import Button from 'src/components/Button'; import Icons from 'src/components/Icons'; @@ -26,10 +25,10 @@ import CopyToClipboard from 'src/components/CopyToClipboard'; import { storeQuery } from 'src/utils/common'; import { getClientErrorObject } from 'src/utils/getClientErrorObject'; import { FeatureFlag, isFeatureEnabled } from 'src/featureFlags'; -import { QueryEditor, SqlLabRootState } from 'src/SqlLab/types'; +import useQueryEditor from 'src/SqlLab/hooks/useQueryEditor'; interface ShareSqlLabQueryPropTypes { - queryEditor: QueryEditor; + queryEditorId: string; addDangerToast: (msg: string) => void; } @@ -44,21 +43,15 @@ const StyledIcon = styled(Icons.Link)` `; function ShareSqlLabQuery({ - queryEditor, + queryEditorId, addDangerToast, }: ShareSqlLabQueryPropTypes) { const theme = useTheme(); - const { dbId, name, schema, autorun, sql, remoteId } = useSelector< - SqlLabRootState, - Partial - >(({ sqlLab: { unsavedQueryEditor } }) => { - const { dbId, name, schema, autorun, sql, remoteId } = { - ...queryEditor, - ...(unsavedQueryEditor.id === queryEditor.id && unsavedQueryEditor), - }; - return { dbId, name, schema, autorun, sql, remoteId }; - }, shallowEqual); + const { dbId, name, schema, autorun, sql, remoteId } = useQueryEditor( + queryEditorId, + ['dbId', 'name', 'schema', 'autorun', 'sql', 'remoteId'], + ); const getCopyUrlForKvStore = (callback: Function) => { const sharedQuery = { dbId, name, schema, autorun, sql }; diff --git a/superset-frontend/src/SqlLab/components/SqlEditor/index.jsx b/superset-frontend/src/SqlLab/components/SqlEditor/index.jsx index 3e8ee84f6c500..bc4cf013d5cfb 100644 --- a/superset-frontend/src/SqlLab/components/SqlEditor/index.jsx +++ b/superset-frontend/src/SqlLab/components/SqlEditor/index.jsx @@ -163,13 +163,8 @@ const SqlEditor = ({ const theme = useTheme(); const dispatch = useDispatch(); - const { currentQueryEditor, database, latestQuery, hideLeftBar } = - useSelector(({ sqlLab: { unsavedQueryEditor, databases, queries } }) => { - const currentQueryEditor = { - ...queryEditor, - ...(queryEditor.id === unsavedQueryEditor.id && unsavedQueryEditor), - }; - + const { database, latestQuery, hideLeftBar } = useSelector( + ({ sqlLab: { unsavedQueryEditor, databases, queries } }) => { let { dbId, latestQueryId, hideLeftBar } = queryEditor; if (unsavedQueryEditor.id === queryEditor.id) { dbId = unsavedQueryEditor.dbId || dbId; @@ -177,12 +172,12 @@ const SqlEditor = ({ hideLeftBar = unsavedQueryEditor.hideLeftBar || hideLeftBar; } return { - currentQueryEditor, database: databases[dbId], latestQuery: queries[latestQueryId], hideLeftBar, }; - }); + }, + ); const queryEditors = useSelector(({ sqlLab }) => sqlLab.queryEditors); @@ -540,7 +535,7 @@ const SqlEditor = ({ @@ -576,7 +571,7 @@ const SqlEditor = ({
dispatch(updateSavedQuery(query))} @@ -585,7 +580,7 @@ const SqlEditor = ({ /> - + @@ -616,7 +611,7 @@ const SqlEditor = ({ autocomplete={autocompleteEnabled} onBlur={setQueryEditorAndSaveSql} onChange={onSqlChanged} - queryEditor={currentQueryEditor} + queryEditorId={queryEditor.id} database={database} extendedTables={tables} height={`${aceEditorHeight}px`} diff --git a/superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx b/superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx index 1a24edc65701d..06a31711db4a9 100644 --- a/superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx +++ b/superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx @@ -25,7 +25,6 @@ import React, { Dispatch, SetStateAction, } from 'react'; -import { useSelector } from 'react-redux'; import querystring from 'query-string'; import Button from 'src/components/Button'; import { t, styled, css, SupersetTheme } from '@superset-ui/core'; @@ -33,7 +32,8 @@ import Collapse from 'src/components/Collapse'; import Icons from 'src/components/Icons'; import { TableSelectorMultiple } from 'src/components/TableSelector'; import { IconTooltip } from 'src/components/IconTooltip'; -import { QueryEditor, SchemaOption, SqlLabRootState } from 'src/SqlLab/types'; +import { QueryEditor, SchemaOption } from 'src/SqlLab/types'; +import useQueryEditor from 'src/SqlLab/hooks/useQueryEditor'; import { DatabaseObject } from 'src/components/DatabaseSelector'; import { EmptyStateSmall } from 'src/components/EmptyState'; import { @@ -117,15 +117,7 @@ export default function SqlEditorLeftBar({ const [userSelectedDb, setUserSelected] = useState( null, ); - const schema = useSelector( - ({ sqlLab: { unsavedQueryEditor } }) => { - const updatedQueryEditor = { - ...queryEditor, - ...(unsavedQueryEditor.id === queryEditor.id && unsavedQueryEditor), - }; - return updatedQueryEditor.schema; - }, - ); + const { schema } = useQueryEditor(queryEditor.id, ['schema']); useEffect(() => { const bool = querystring.parse(window.location.search).db; diff --git a/superset-frontend/src/SqlLab/hooks/useQueryEditor/index.ts b/superset-frontend/src/SqlLab/hooks/useQueryEditor/index.ts new file mode 100644 index 0000000000000..7044e77798fd2 --- /dev/null +++ b/superset-frontend/src/SqlLab/hooks/useQueryEditor/index.ts @@ -0,0 +1,38 @@ +/** + * 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 pick from 'lodash/pick'; +import { shallowEqual, useSelector } from 'react-redux'; +import { SqlLabRootState, QueryEditor } from 'src/SqlLab/types'; + +export default function useQueryEditor( + sqlEditorId: string, + attributes: ReadonlyArray, +) { + return useSelector>( + ({ sqlLab: { unsavedQueryEditor, queryEditors } }) => + pick( + { + ...queryEditors.find(({ id }) => id === sqlEditorId), + ...(sqlEditorId === unsavedQueryEditor.id && unsavedQueryEditor), + }, + ['id'].concat(attributes), + ) as Pick, + shallowEqual, + ); +} diff --git a/superset-frontend/src/SqlLab/hooks/useQueryEditor/useQueryEditor.test.ts b/superset-frontend/src/SqlLab/hooks/useQueryEditor/useQueryEditor.test.ts new file mode 100644 index 0000000000000..23de4d68226cf --- /dev/null +++ b/superset-frontend/src/SqlLab/hooks/useQueryEditor/useQueryEditor.test.ts @@ -0,0 +1,92 @@ +/** + * 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 configureStore from 'redux-mock-store'; +import thunk from 'redux-thunk'; +import { initialState, defaultQueryEditor } from 'src/SqlLab/fixtures'; +import { renderHook } from '@testing-library/react-hooks'; +import { createWrapper } from 'spec/helpers/testing-library'; + +import useQueryEditor from '.'; + +const middlewares = [thunk]; +const mockStore = configureStore(middlewares); + +test('returns selected queryEditor values', () => { + const { result } = renderHook( + () => + useQueryEditor(defaultQueryEditor.id, [ + 'id', + 'name', + 'dbId', + 'schemaOptions', + ]), + { + wrapper: createWrapper({ + useRedux: true, + store: mockStore(initialState), + }), + }, + ); + expect(result.current).toEqual({ + id: defaultQueryEditor.id, + name: defaultQueryEditor.name, + dbId: defaultQueryEditor.dbId, + schemaOptions: defaultQueryEditor.schemaOptions, + }); +}); + +test('includes id implicitly', () => { + const { result } = renderHook( + () => useQueryEditor(defaultQueryEditor.id, ['name']), + { + wrapper: createWrapper({ + useRedux: true, + store: mockStore(initialState), + }), + }, + ); + expect(result.current).toEqual({ + id: defaultQueryEditor.id, + name: defaultQueryEditor.name, + }); +}); + +test('returns updated values from unsaved change', () => { + const expectedSql = 'SELECT updated_column\nFROM updated_table\nWHERE'; + const { result } = renderHook( + () => useQueryEditor(defaultQueryEditor.id, ['id', 'sql']), + { + wrapper: createWrapper({ + useRedux: true, + store: mockStore({ + ...initialState, + sqlLab: { + ...initialState.sqlLab, + unsavedQueryEditor: { + id: defaultQueryEditor.id, + sql: expectedSql, + }, + }, + }), + }), + }, + ); + expect(result.current.id).toEqual(defaultQueryEditor.id); + expect(result.current.sql).toEqual(expectedSql); +}); diff --git a/superset-frontend/src/SqlLab/reducers/sqlLab.js b/superset-frontend/src/SqlLab/reducers/sqlLab.js index bab832d6c61f9..164691c666381 100644 --- a/superset-frontend/src/SqlLab/reducers/sqlLab.js +++ b/superset-frontend/src/SqlLab/reducers/sqlLab.js @@ -449,7 +449,7 @@ export default function sqlLabReducer(state = {}, action) { ); return { ...(action.queryEditor.id === state.unsavedQueryEditor.id - ? alterInObject( + ? alterInArr( mergeUnsavedState, 'queryEditors', action.queryEditor,