From 3b427b2029cbeb5c656d20f4201ea4eada069a25 Mon Sep 17 00:00:00 2001 From: Diego Medina Date: Wed, 16 Mar 2022 22:46:06 -0400 Subject: [PATCH] fix: auto-complete of tables and names are not working in SQL lab (#19152) --- .../components/AceEditorWrapper/index.tsx | 11 ++- .../components/SqlEditorLeftBar/index.tsx | 38 +++++++-- .../TableSelector/TableSelector.test.tsx | 80 ++++++++++++++++--- .../src/components/TableSelector/index.tsx | 7 +- 4 files changed, 108 insertions(+), 28 deletions(-) diff --git a/superset-frontend/src/SqlLab/components/AceEditorWrapper/index.tsx b/superset-frontend/src/SqlLab/components/AceEditorWrapper/index.tsx index ce201e89d..8807ccdac 100644 --- a/superset-frontend/src/SqlLab/components/AceEditorWrapper/index.tsx +++ b/superset-frontend/src/SqlLab/components/AceEditorWrapper/index.tsx @@ -171,17 +171,20 @@ class AceEditorWrapper extends React.PureComponent { meta: 'schema', })); const columns = {}; - const tables = props.extendedTables || props.tables || []; + + const tables = props.tables || []; + const extendedTables = props.extendedTables || []; const tableWords = tables.map(t => { - const tableName = t.name; - const cols = t.columns || []; + const tableName = t.value; + const extendedTable = extendedTables.find(et => et.name === tableName); + const cols = (extendedTable && extendedTable.columns) || []; cols.forEach(col => { columns[col.name] = null; // using an object as a unique set }); return { - name: tableName, + name: t.label, value: tableName, score: TABLE_AUTOCOMPLETE_SCORE, meta: 'table', diff --git a/superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx b/superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx index 7bbdfcf63..0988fd968 100644 --- a/superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx +++ b/superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import React from 'react'; +import React, { useEffect, useRef, useCallback } from 'react'; import Button from 'src/components/Button'; import { t, styled, css, SupersetTheme } from '@superset-ui/core'; import Collapse from 'src/components/Collapse'; @@ -41,7 +41,10 @@ interface actionsTypes { addDangerToast: (msg: string) => void; queryEditorSetSchema: (queryEditor: QueryEditor, schema?: string) => void; queryEditorSetSchemaOptions: () => void; - queryEditorSetTableOptions: (options: Array) => void; + queryEditorSetTableOptions: ( + queryEditor: QueryEditor, + options: Array, + ) => void; resetState: () => void; } @@ -86,6 +89,13 @@ export default function SqlEditorLeftBar({ tables = [], height = 500, }: SqlEditorLeftBarProps) { + // Ref needed to avoid infinite rerenders on handlers + // that require and modify the queryEditor + const queryEditorRef = useRef(queryEditor); + useEffect(() => { + queryEditorRef.current = queryEditor; + }, [queryEditor]); + const onDbChange = ({ id: dbId }: { id: number }) => { actions.queryEditorSetDb(queryEditor, dbId); actions.queryEditorSetFunctionNames(queryEditor, dbId); @@ -132,9 +142,23 @@ export default function SqlEditorLeftBar({ const shouldShowReset = window.location.search === '?reset=1'; const tableMetaDataHeight = height - 130; // 130 is the height of the selects above - const onSchemaChange = (schema: string) => { - actions.queryEditorSetSchema(queryEditor, schema); - }; + const handleSchemaChange = useCallback( + (schema: string) => { + if (queryEditorRef.current) { + actions.queryEditorSetSchema(queryEditorRef.current, schema); + } + }, + [actions], + ); + + const handleTablesLoad = React.useCallback( + (options: Array) => { + if (queryEditorRef.current) { + actions.queryEditorSetTableOptions(queryEditorRef.current, options); + } + }, + [actions], + ); return (
@@ -143,10 +167,10 @@ export default function SqlEditorLeftBar({ getDbList={actions.setDatabases} handleError={actions.addDangerToast} onDbChange={onDbChange} - onSchemaChange={onSchemaChange} + onSchemaChange={handleSchemaChange} onSchemasLoad={actions.queryEditorSetSchemaOptions} onTableChange={onTableChange} - onTablesLoad={actions.queryEditorSetTableOptions} + onTablesLoad={handleTablesLoad} schema={queryEditor.schema} sqlLabMode /> diff --git a/superset-frontend/src/components/TableSelector/TableSelector.test.tsx b/superset-frontend/src/components/TableSelector/TableSelector.test.tsx index cd7b51d4b..013e937ed 100644 --- a/superset-frontend/src/components/TableSelector/TableSelector.test.tsx +++ b/superset-frontend/src/components/TableSelector/TableSelector.test.tsx @@ -20,12 +20,13 @@ import React from 'react'; import { render, screen, waitFor } from 'spec/helpers/testing-library'; import { SupersetClient } from '@superset-ui/core'; +import { act } from 'react-dom/test-utils'; import userEvent from '@testing-library/user-event'; import TableSelector from '.'; const SupersetClientGet = jest.spyOn(SupersetClient, 'get'); -const createProps = () => ({ +const createProps = (props = {}) => ({ database: { id: 1, database_name: 'main', @@ -34,23 +35,33 @@ const createProps = () => ({ }, schema: 'test_schema', handleError: jest.fn(), + ...props, }); -beforeAll(() => { - SupersetClientGet.mockImplementation( - async () => - ({ - json: { - options: [ - { label: 'table_a', value: 'table_a' }, - { label: 'table_b', value: 'table_b' }, - ], - }, - } as any), - ); +afterEach(() => { + jest.clearAllMocks(); }); +const getSchemaMockFunction = async () => + ({ + json: { + result: ['schema_a', 'schema_b'], + }, + } as any); + +const getTableMockFunction = async () => + ({ + json: { + options: [ + { label: 'table_a', value: 'table_a' }, + { label: 'table_b', value: 'table_b' }, + ], + }, + } as any); + test('renders with default props', async () => { + SupersetClientGet.mockImplementation(getTableMockFunction); + const props = createProps(); render(, { useRedux: true }); const databaseSelect = screen.getByRole('combobox', { @@ -70,6 +81,8 @@ test('renders with default props', async () => { }); test('renders table options', async () => { + SupersetClientGet.mockImplementation(getTableMockFunction); + const props = createProps(); render(, { useRedux: true }); const tableSelect = screen.getByRole('combobox', { @@ -85,6 +98,8 @@ test('renders table options', async () => { }); test('renders disabled without schema', async () => { + SupersetClientGet.mockImplementation(getTableMockFunction); + const props = createProps(); render(, { useRedux: true }); const tableSelect = screen.getByRole('combobox', { @@ -94,3 +109,42 @@ test('renders disabled without schema', async () => { expect(tableSelect).toBeDisabled(); }); }); + +test('table options are notified after schema selection', async () => { + SupersetClientGet.mockImplementation(getSchemaMockFunction); + + const callback = jest.fn(); + const props = createProps({ + onTablesLoad: callback, + schema: undefined, + }); + render(, { useRedux: true }); + + const schemaSelect = screen.getByRole('combobox', { + name: 'Select schema or type schema name', + }); + expect(schemaSelect).toBeInTheDocument(); + expect(callback).not.toHaveBeenCalled(); + + userEvent.click(schemaSelect); + + expect( + await screen.findByRole('option', { name: 'schema_a' }), + ).toBeInTheDocument(); + expect( + await screen.findByRole('option', { name: 'schema_b' }), + ).toBeInTheDocument(); + + SupersetClientGet.mockImplementation(getTableMockFunction); + + act(() => { + userEvent.click(screen.getAllByText('schema_a')[1]); + }); + + await waitFor(() => { + expect(callback).toHaveBeenCalledWith([ + { label: 'table_a', value: 'table_a' }, + { label: 'table_b', value: 'table_b' }, + ]); + }); +}); diff --git a/superset-frontend/src/components/TableSelector/index.tsx b/superset-frontend/src/components/TableSelector/index.tsx index 88ac9cefb..50804f7d9 100644 --- a/superset-frontend/src/components/TableSelector/index.tsx +++ b/superset-frontend/src/components/TableSelector/index.tsx @@ -208,15 +208,14 @@ const TableSelector: FunctionComponent = ({ currentTable = option; } }); - if (onTablesLoad) { - onTablesLoad(json.options); - } + + onTablesLoad?.(json.options); setTableOptions(options); setCurrentTable(currentTable); setLoadingTables(false); if (forceRefresh) addSuccessToast('List updated'); }) - .catch(e => { + .catch(() => { setLoadingTables(false); handleError(t('There was an error loading the tables')); });