From 86a03851cf85e622315da3674bdbb62c983faa77 Mon Sep 17 00:00:00 2001 From: hughhhh Date: Wed, 28 Sep 2022 19:15:37 -0400 Subject: [PATCH 1/4] change reference of where to pull parameters --- superset/databases/commands/validate.py | 13 ++++++++++++- superset/db_engine_specs/gsheets.py | 1 - 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/superset/databases/commands/validate.py b/superset/databases/commands/validate.py index a8956257fa28a..853e813cc6fd2 100644 --- a/superset/databases/commands/validate.py +++ b/superset/databases/commands/validate.py @@ -16,12 +16,14 @@ # under the License. import json from contextlib import closing +from symbol import parameters from typing import Any, Dict, Optional from flask_babel import gettext as __ from superset.commands.base import BaseCommand from superset.databases.commands.exceptions import ( + DatabaseExtraJSONValidationError, DatabaseOfflineError, DatabaseTestConnectionFailedError, InvalidEngineError, @@ -37,6 +39,15 @@ BYPASS_VALIDATION_ENGINES = {"bigquery"} +def get_engine_parameters(properties: Dict[str, Any]) -> Dict[str, Any]: + try: + if properties.get("extra"): + return json.loads(properties["extra"]).get("engine_params", {}) + return {} + except: + raise DatabaseExtraJSONValidationError("Unable to parse extra_json data") + + class ValidateDatabaseParametersCommand(BaseCommand): def __init__(self, properties: Dict[str, Any]): self._properties = properties.copy() @@ -87,7 +98,7 @@ def run(self) -> None: # try to connect sqlalchemy_uri = engine_spec.build_sqlalchemy_uri( # type: ignore - self._properties.get("parameters"), + parameters, encrypted_extra, ) if self._model and sqlalchemy_uri == self._model.safe_sqlalchemy_uri(): diff --git a/superset/db_engine_specs/gsheets.py b/superset/db_engine_specs/gsheets.py index fd1a2754d76bc..5593ffe202102 100644 --- a/superset/db_engine_specs/gsheets.py +++ b/superset/db_engine_specs/gsheets.py @@ -259,7 +259,6 @@ def validate_parameters( idx = 0 for name, url in table_catalog.items(): - if not name: errors.append( SupersetError( From b458fb212084766dc9bbbd8e1ba6c1ab11fee598 Mon Sep 17 00:00:00 2001 From: lyndsiWilliams Date: Tue, 4 Oct 2022 16:19:15 -0500 Subject: [PATCH 2/4] Remove query.prevQuery check and write tests for bug fix --- .../components/ResultSet/ResultSet.test.tsx | 83 +++++++++++++++---- .../src/SqlLab/components/ResultSet/index.tsx | 6 +- 2 files changed, 70 insertions(+), 19 deletions(-) diff --git a/superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx b/superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx index 2818ed279c7d8..7869a87ab1b61 100644 --- a/superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx +++ b/superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx @@ -75,6 +75,26 @@ const newProps = { }, }, }; +const asyncQueryProps = { + ...mockedProps, + database: { allow_run_async: true }, +}; +const asyncRefetchDataPreviewProps = { + ...asyncQueryProps, + query: { + state: 'success', + results: undefined, + isDataPreview: true, + }, +}; +const asyncRefetchResultsTableProps = { + ...asyncQueryProps, + query: { + state: 'success', + results: undefined, + resultsKey: 'async results key', + }, +}; fetchMock.get('glob:*/api/v1/dataset?*', { result: [] }); const middlewares = [thunk]; @@ -86,13 +106,13 @@ const setup = (props?: any, store?: Store) => }); describe('ResultSet', () => { - it('renders a Table', async () => { + test('renders a Table', async () => { const { getByTestId } = setup(mockedProps, mockStore(initialState)); const table = getByTestId('table-container'); expect(table).toBeInTheDocument(); }); - it('should render success query', async () => { + test('should render success query', async () => { const { queryAllByText, getByTestId } = setup( mockedProps, mockStore(initialState), @@ -114,7 +134,7 @@ describe('ResultSet', () => { expect(exploreButton).toBeInTheDocument(); }); - it('should render empty results', async () => { + test('should render empty results', async () => { const props = { ...mockedProps, query: { ...mockedProps.query, results: { data: [] } }, @@ -128,7 +148,7 @@ describe('ResultSet', () => { expect(alert).toHaveTextContent('The query returned no data'); }); - it('should call reRunQuery if timed out', async () => { + test('should call reRunQuery if timed out', async () => { const store = mockStore(initialState); const propsWithError = { ...mockedProps, @@ -143,26 +163,26 @@ describe('ResultSet', () => { expect(store.getActions()[0].type).toEqual('START_QUERY'); }); - it('should not call reRunQuery if no error', async () => { + test('should not call reRunQuery if no error', async () => { const store = mockStore(initialState); setup(mockedProps, store); expect(store.getActions()).toEqual([]); }); - it('should render cached query', async () => { + test('should render cached query', async () => { const store = mockStore(initialState); const { rerender } = setup(cachedQueryProps, store); // @ts-ignore rerender(); - expect(store.getActions()).toHaveLength(1); + expect(store.getActions()).toHaveLength(2); expect(store.getActions()[0].query.results).toEqual( cachedQueryProps.query.results, ); expect(store.getActions()[0].type).toEqual('CLEAR_QUERY_RESULTS'); }); - it('should render stopped query', async () => { + test('should render stopped query', async () => { await waitFor(() => { setup(stoppedQueryProps, mockStore(initialState)); }); @@ -171,13 +191,13 @@ describe('ResultSet', () => { expect(alert).toBeInTheDocument(); }); - it('should render running/pending/fetching query', async () => { + test('should render running/pending/fetching query', async () => { const { getByTestId } = setup(runningQueryProps, mockStore(initialState)); const progressBar = getByTestId('progress-bar'); expect(progressBar).toBeInTheDocument(); }); - it('should render fetching w/ 100 progress query', async () => { + test('should render fetching w/ 100 progress query', async () => { const { getByRole, getByText } = setup( fetchingQueryProps, mockStore(initialState), @@ -187,7 +207,7 @@ describe('ResultSet', () => { expect(getByText('fetching')).toBeInTheDocument(); }); - it('should render a failed query with an error message', async () => { + test('should render a failed query with an error message', async () => { await waitFor(() => { setup(failedQueryWithErrorMessageProps, mockStore(initialState)); }); @@ -196,21 +216,56 @@ describe('ResultSet', () => { expect(screen.getByText('Something went wrong')).toBeInTheDocument(); }); - it('should render a failed query with an errors object', async () => { + test('should render a failed query with an errors object', async () => { await waitFor(() => { setup(failedQueryWithErrorsProps, mockStore(initialState)); }); expect(screen.getByText('Database error')).toBeInTheDocument(); }); - it('renders if there is no limit in query.results but has queryLimit', async () => { + test('renders if there is no limit in query.results but has queryLimit', async () => { const { getByRole } = setup(mockedProps, mockStore(initialState)); expect(getByRole('grid')).toBeInTheDocument(); }); - it('renders if there is a limit in query.results but not queryLimit', async () => { + 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)); expect(getByRole('grid')).toBeInTheDocument(); }); + + test('Async queries - renders "Fetch data preview" button when data preview has no results', () => { + setup(asyncRefetchDataPreviewProps, mockStore(initialState)); + expect( + screen.getByRole('button', { + name: /fetch data preview/i, + }), + ).toBeVisible(); + expect(screen.queryByRole('grid')).toBe(null); + }); + + test('Async queries - renders "Refetch results" button when a query has no results', () => { + setup(asyncRefetchResultsTableProps, mockStore(initialState)); + expect( + screen.getByRole('button', { + name: /refetch results/i, + }), + ).toBeVisible(); + expect(screen.queryByRole('grid')).toBe(null); + }); + + test('Async queries - renders on the first call', () => { + setup(asyncQueryProps, mockStore(initialState)); + expect(screen.getByRole('grid')).toBeVisible(); + expect( + screen.queryByRole('button', { + name: /fetch data preview/i, + }), + ).toBe(null); + expect( + screen.queryByRole('button', { + name: /refetch results/i, + }), + ).toBe(null); + }); }); diff --git a/superset-frontend/src/SqlLab/components/ResultSet/index.tsx b/superset-frontend/src/SqlLab/components/ResultSet/index.tsx index 78387f6dc44d4..0d61d8ddab648 100644 --- a/superset-frontend/src/SqlLab/components/ResultSet/index.tsx +++ b/superset-frontend/src/SqlLab/components/ResultSet/index.tsx @@ -164,11 +164,7 @@ const ResultSet = ({ setCachedData(query.results.data); dispatch(clearQueryResults(query)); } - if ( - query.resultsKey && - prevQuery?.resultsKey && - query.resultsKey !== prevQuery.resultsKey - ) { + if (query.resultsKey && query.resultsKey !== prevQuery?.resultsKey) { fetchResults(query); } }, [query, cache]); From 02b7e0aa069a3ac1552301584f9e637739e0b2cf Mon Sep 17 00:00:00 2001 From: lyndsiWilliams Date: Tue, 4 Oct 2022 16:22:38 -0500 Subject: [PATCH 3/4] Fix unintentional merge changes --- superset/databases/commands/validate.py | 12 ------------ superset/db_engine_specs/gsheets.py | 1 + 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/superset/databases/commands/validate.py b/superset/databases/commands/validate.py index 853e813cc6fd2..3921a02ab406b 100644 --- a/superset/databases/commands/validate.py +++ b/superset/databases/commands/validate.py @@ -16,14 +16,12 @@ # under the License. import json from contextlib import closing -from symbol import parameters from typing import Any, Dict, Optional from flask_babel import gettext as __ from superset.commands.base import BaseCommand from superset.databases.commands.exceptions import ( - DatabaseExtraJSONValidationError, DatabaseOfflineError, DatabaseTestConnectionFailedError, InvalidEngineError, @@ -39,15 +37,6 @@ BYPASS_VALIDATION_ENGINES = {"bigquery"} -def get_engine_parameters(properties: Dict[str, Any]) -> Dict[str, Any]: - try: - if properties.get("extra"): - return json.loads(properties["extra"]).get("engine_params", {}) - return {} - except: - raise DatabaseExtraJSONValidationError("Unable to parse extra_json data") - - class ValidateDatabaseParametersCommand(BaseCommand): def __init__(self, properties: Dict[str, Any]): self._properties = properties.copy() @@ -98,7 +87,6 @@ def run(self) -> None: # try to connect sqlalchemy_uri = engine_spec.build_sqlalchemy_uri( # type: ignore - parameters, encrypted_extra, ) if self._model and sqlalchemy_uri == self._model.safe_sqlalchemy_uri(): diff --git a/superset/db_engine_specs/gsheets.py b/superset/db_engine_specs/gsheets.py index 5593ffe202102..fd1a2754d76bc 100644 --- a/superset/db_engine_specs/gsheets.py +++ b/superset/db_engine_specs/gsheets.py @@ -259,6 +259,7 @@ def validate_parameters( idx = 0 for name, url in table_catalog.items(): + if not name: errors.append( SupersetError( From 2ee28bdcd042aecb399c7f105a18fd821a38ced6 Mon Sep 17 00:00:00 2001 From: lyndsiWilliams Date: Tue, 4 Oct 2022 16:24:41 -0500 Subject: [PATCH 4/4] Fix unintentional merge changes (missed one) --- superset/databases/commands/validate.py | 1 + 1 file changed, 1 insertion(+) diff --git a/superset/databases/commands/validate.py b/superset/databases/commands/validate.py index 3921a02ab406b..a8956257fa28a 100644 --- a/superset/databases/commands/validate.py +++ b/superset/databases/commands/validate.py @@ -87,6 +87,7 @@ def run(self) -> None: # try to connect sqlalchemy_uri = engine_spec.build_sqlalchemy_uri( # type: ignore + self._properties.get("parameters"), encrypted_extra, ) if self._model and sqlalchemy_uri == self._model.safe_sqlalchemy_uri():