From 222f1e7ea8e95066abd78a06f470d74777f87cc5 Mon Sep 17 00:00:00 2001 From: "JUST.in DO IT" Date: Thu, 1 Sep 2022 16:35:19 -0700 Subject: [PATCH] fix(sqllab): invalid table metadata request (#21304) --- .../src/SqlLab/actions/sqlLab.js | 5 +- .../src/SqlLab/actions/sqlLab.test.js | 74 +++++++--- .../SqlEditorLeftBar.test.jsx | 128 ++++++++++-------- .../components/SqlEditorLeftBar/index.tsx | 14 +- .../components/TabbedSqlEditors/index.jsx | 3 + .../SqlLab/components/TableElement/index.tsx | 1 + .../src/SqlLab/reducers/sqlLab.js | 20 ++- .../src/SqlLab/reducers/sqlLab.test.js | 22 +++ 8 files changed, 185 insertions(+), 82 deletions(-) diff --git a/superset-frontend/src/SqlLab/actions/sqlLab.js b/superset-frontend/src/SqlLab/actions/sqlLab.js index cb6f3c4095..5e5c530c28 100644 --- a/superset-frontend/src/SqlLab/actions/sqlLab.js +++ b/superset-frontend/src/SqlLab/actions/sqlLab.js @@ -1137,8 +1137,9 @@ function getTableExtendedMetadata(table, query, dispatch) { ); } -export function addTable(query, database, tableName, schemaName) { - return function (dispatch) { +export function addTable(queryEditor, database, tableName, schemaName) { + return function (dispatch, getState) { + const query = getUpToDateQuery(getState(), queryEditor, queryEditor.id); const table = { dbId: query.dbId, queryEditorId: query.id, diff --git a/superset-frontend/src/SqlLab/actions/sqlLab.test.js b/superset-frontend/src/SqlLab/actions/sqlLab.test.js index 4dadb9ea89..c0f3c8cd4b 100644 --- a/superset-frontend/src/SqlLab/actions/sqlLab.test.js +++ b/superset-frontend/src/SqlLab/actions/sqlLab.test.js @@ -736,29 +736,71 @@ describe('async actions', () => { const database = { disable_data_preview: true }; const tableName = 'table'; const schemaName = 'schema'; - const store = mockStore({}); + const store = mockStore(initialState); const expectedActionTypes = [ actions.MERGE_TABLE, // addTable actions.MERGE_TABLE, // getTableMetadata actions.MERGE_TABLE, // getTableExtendedMetadata actions.MERGE_TABLE, // addTable ]; - return store - .dispatch(actions.addTable(query, database, tableName, schemaName)) - .then(() => { - expect(store.getActions().map(a => a.type)).toEqual( - expectedActionTypes, - ); - expect(store.getActions()[0].prepend).toBeTruthy(); - expect(fetchMock.calls(updateTableSchemaEndpoint)).toHaveLength(1); - expect(fetchMock.calls(getTableMetadataEndpoint)).toHaveLength(1); - expect(fetchMock.calls(getExtraTableMetadataEndpoint)).toHaveLength( - 1, - ); + const request = actions.addTable( + query, + database, + tableName, + schemaName, + ); + return request(store.dispatch, store.getState).then(() => { + expect(store.getActions().map(a => a.type)).toEqual( + expectedActionTypes, + ); + expect(store.getActions()[0].prepend).toBeTruthy(); + expect(fetchMock.calls(updateTableSchemaEndpoint)).toHaveLength(1); + expect(fetchMock.calls(getTableMetadataEndpoint)).toHaveLength(1); + expect(fetchMock.calls(getExtraTableMetadataEndpoint)).toHaveLength( + 1, + ); - // tab state is not updated, since no query was run - expect(fetchMock.calls(updateTabStateEndpoint)).toHaveLength(0); - }); + // tab state is not updated, since no query was run + expect(fetchMock.calls(updateTabStateEndpoint)).toHaveLength(0); + }); + }); + + it('fetches table schema state from unsaved change', () => { + const database = { disable_data_preview: true }; + const tableName = 'table'; + const schemaName = 'schema'; + const expectedDbId = 473892; + const store = mockStore({ + ...initialState, + sqlLab: { + ...initialState.sqlLab, + unsavedQueryEditor: { + id: query.id, + dbId: expectedDbId, + }, + }, + }); + const request = actions.addTable( + query, + database, + tableName, + schemaName, + ); + return request(store.dispatch, store.getState).then(() => { + expect( + fetchMock.calls( + `glob:**/api/v1/database/${expectedDbId}/table/*/*/`, + ), + ).toHaveLength(1); + expect( + fetchMock.calls( + `glob:**/api/v1/database/${expectedDbId}/table_extra/*/*/`, + ), + ).toHaveLength(1); + + // tab state is not updated, since no query was run + expect(fetchMock.calls(updateTabStateEndpoint)).toHaveLength(0); + }); }); it('updates and runs data preview query when configured', () => { diff --git a/superset-frontend/src/SqlLab/components/SqlEditorLeftBar/SqlEditorLeftBar.test.jsx b/superset-frontend/src/SqlLab/components/SqlEditorLeftBar/SqlEditorLeftBar.test.jsx index 5e1e368b1c..79e7793421 100644 --- a/superset-frontend/src/SqlLab/components/SqlEditorLeftBar/SqlEditorLeftBar.test.jsx +++ b/superset-frontend/src/SqlLab/components/SqlEditorLeftBar/SqlEditorLeftBar.test.jsx @@ -19,14 +19,12 @@ import React from 'react'; import configureStore from 'redux-mock-store'; import fetchMock from 'fetch-mock'; -import { shallow } from 'enzyme'; import { render, screen } from 'spec/helpers/testing-library'; import userEvent from '@testing-library/user-event'; import { Provider } from 'react-redux'; import '@testing-library/jest-dom/extend-expect'; import thunk from 'redux-thunk'; import SqlEditorLeftBar from 'src/SqlLab/components/SqlEditorLeftBar'; -import TableElement from 'src/SqlLab/components/TableElement'; import { supersetTheme, ThemeProvider } from '@superset-ui/core'; import { table, @@ -46,74 +44,86 @@ const mockedProps = { const middlewares = [thunk]; const mockStore = configureStore(middlewares); const store = mockStore(initialState); + fetchMock.get('glob:*/api/v1/database/*/schemas/?*', { result: [] }); -describe('SqlEditorLeftBar', () => { - let wrapper; - - beforeEach(() => { - wrapper = shallow(, { - context: { store }, - }); - }); - - afterEach(() => { - wrapper.unmount(); - }); - - it('is valid', () => { - expect(React.isValidElement()).toBe( - true, - ); - }); - - it('renders a TableElement', () => { - expect(wrapper.find(TableElement)).toExist(); - }); +fetchMock.get('glob:*/superset/tables/**', { + json: { + options: [ + { + label: 'ab_user', + value: 'ab_user', + }, + ], + tableLength: 1, + }, }); describe('Left Panel Expansion', () => { - it('table should be visible when expanded is true', () => { - const { container } = render( + it('is valid', () => { + expect( + React.isValidElement( + + + , + ), + ).toBe(true); + }); + + it('renders a TableElement', () => { + const { queryAllByTestId } = render( , ); - const dbSelect = screen.getByRole('combobox', { - name: 'Select database or type database name', - }); - const schemaSelect = screen.getByRole('combobox', { - name: 'Select schema or type schema name', - }); - const dropdown = screen.getByText(/Select table or type table name/i); - const abUser = screen.getByText(/ab_user/i); - expect(dbSelect).toBeInTheDocument(); - expect(schemaSelect).toBeInTheDocument(); - expect(dropdown).toBeInTheDocument(); - expect(abUser).toBeInTheDocument(); - expect( - container.querySelector('.ant-collapse-content-active'), - ).toBeInTheDocument(); + expect(queryAllByTestId('table-element').length).toBeGreaterThanOrEqual(1); }); - it('should toggle the table when the header is clicked', async () => { - const collapseMock = jest.fn(); - render( - - - - - , - ); - const header = screen.getByText(/ab_user/); - userEvent.click(header); - expect(collapseMock).toHaveBeenCalled(); + describe('Left Panel Expansion', () => { + it('table should be visible when expanded is true', () => { + const { container } = render( + + + + + , + ); + const dbSelect = screen.getByRole('combobox', { + name: 'Select database or type database name', + }); + const schemaSelect = screen.getByRole('combobox', { + name: 'Select schema or type schema name', + }); + const dropdown = screen.getByText(/Select table or type table name/i); + const abUser = screen.getByText(/ab_user/i); + expect(dbSelect).toBeInTheDocument(); + expect(schemaSelect).toBeInTheDocument(); + expect(dropdown).toBeInTheDocument(); + expect(abUser).toBeInTheDocument(); + expect( + container.querySelector('.ant-collapse-content-active'), + ).toBeInTheDocument(); + }); + + it('should toggle the table when the header is clicked', async () => { + const collapseMock = jest.fn(); + render( + + + + + , + ); + const header = screen.getByText(/ab_user/); + userEvent.click(header); + expect(collapseMock).toHaveBeenCalled(); + }); }); }); diff --git a/superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx b/superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx index 97d80a4e88..b3c1bc9f63 100644 --- a/superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx +++ b/superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx @@ -25,6 +25,7 @@ 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'; @@ -32,7 +33,7 @@ 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 } from 'src/SqlLab/types'; +import { QueryEditor, SchemaOption, SqlLabRootState } from 'src/SqlLab/types'; import { DatabaseObject } from 'src/components/DatabaseSelector'; import { EmptyStateSmall } from 'src/components/EmptyState'; import { @@ -116,6 +117,15 @@ 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; + }, + ); useEffect(() => { const bool = querystring.parse(window.location.search).db; @@ -263,7 +273,7 @@ export default function SqlEditorLeftBar({ onSchemasLoad={handleSchemasLoad} onTableSelectChange={onTablesChange} onTablesLoad={handleTablesLoad} - schema={queryEditor.schema} + schema={schema} tableValue={selectedTableNames} sqlLabMode /> diff --git a/superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.jsx b/superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.jsx index c5b94f8b35..7c53181866 100644 --- a/superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.jsx +++ b/superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.jsx @@ -262,6 +262,9 @@ class TabbedSqlEditors extends React.PureComponent { const qeid = this.props.tabHistory[this.props.tabHistory.length - 1]; if (key !== qeid) { const queryEditor = this.props.queryEditors.find(qe => qe.id === key); + if (!queryEditor) { + return; + } this.props.actions.switchQueryEditor( queryEditor, this.props.displayLimit, diff --git a/superset-frontend/src/SqlLab/components/TableElement/index.tsx b/superset-frontend/src/SqlLab/components/TableElement/index.tsx index b05c875e12..1ecc782220 100644 --- a/superset-frontend/src/SqlLab/components/TableElement/index.tsx +++ b/superset-frontend/src/SqlLab/components/TableElement/index.tsx @@ -270,6 +270,7 @@ const TableElement = ({ table, actions, ...props }: TableElementProps) => { const metadata = (
setHover(true)} onMouseLeave={() => setHover(false)} css={{ paddingTop: 6 }} diff --git a/superset-frontend/src/SqlLab/reducers/sqlLab.js b/superset-frontend/src/SqlLab/reducers/sqlLab.js index a12db71a37..bab832d6c6 100644 --- a/superset-frontend/src/SqlLab/reducers/sqlLab.js +++ b/superset-frontend/src/SqlLab/reducers/sqlLab.js @@ -110,7 +110,12 @@ export default function sqlLabReducer(state = {}, action) { ); }, [actions.REMOVE_QUERY_EDITOR]() { - let newState = removeFromArr(state, 'queryEditors', action.queryEditor); + const queryEditor = { + ...action.queryEditor, + ...(action.queryEditor.id === state.unsavedQueryEditor.id && + state.unsavedQueryEditor), + }; + let newState = removeFromArr(state, 'queryEditors', queryEditor); // List of remaining queryEditor ids const qeIds = newState.queryEditors.map(qe => qe.id); @@ -127,10 +132,19 @@ export default function sqlLabReducer(state = {}, action) { // Remove associated table schemas const tables = state.tables.filter( - table => table.queryEditorId !== action.queryEditor.id, + table => table.queryEditorId !== queryEditor.id, ); - newState = { ...newState, tabHistory, tables, queries }; + newState = { + ...newState, + tabHistory, + tables, + queries, + unsavedQueryEditor: { + ...(action.queryEditor.id !== state.unsavedQueryEditor.id && + state.unsavedQueryEditor), + }, + }; return newState; }, [actions.REMOVE_QUERY]() { diff --git a/superset-frontend/src/SqlLab/reducers/sqlLab.test.js b/superset-frontend/src/SqlLab/reducers/sqlLab.test.js index 82483712f8..088d01031a 100644 --- a/superset-frontend/src/SqlLab/reducers/sqlLab.test.js +++ b/superset-frontend/src/SqlLab/reducers/sqlLab.test.js @@ -75,6 +75,28 @@ describe('sqlLabReducer', () => { initialState.queryEditors.length, ); }); + it('should remove a query editor including unsaved changes', () => { + expect(newState.queryEditors).toHaveLength( + initialState.queryEditors.length + 1, + ); + let action = { + type: actions.QUERY_EDITOR_SETDB, + queryEditor: qe, + dbId: 123, + }; + newState = sqlLabReducer(newState, action); + expect(newState.unsavedQueryEditor.dbId).toEqual(action.dbId); + action = { + type: actions.REMOVE_QUERY_EDITOR, + queryEditor: qe, + }; + newState = sqlLabReducer(newState, action); + expect(newState.queryEditors).toHaveLength( + initialState.queryEditors.length, + ); + expect(newState.unsavedQueryEditor.dbId).toBeUndefined(); + expect(newState.unsavedQueryEditor.id).toBeUndefined(); + }); it('should set q query editor active', () => { const expectedTitle = 'new updated title'; const addQueryEditorAction = {