From 608ffcbfb9d91aa44cdca77cc1b08fcb610209b8 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Mon, 12 Dec 2022 12:36:41 -0800 Subject: [PATCH] fix: make database connection modal ace fields uncontrolled (#22350) --- .../database/DatabaseModal/ExtraOptions.tsx | 35 +++++++---- .../database/DatabaseModal/index.test.tsx | 60 ++++++++----------- .../data/database/DatabaseModal/index.tsx | 12 +++- 3 files changed, 57 insertions(+), 50 deletions(-) diff --git a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/ExtraOptions.tsx b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/ExtraOptions.tsx index 8bff977d13c..c8c03878cd6 100644 --- a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/ExtraOptions.tsx +++ b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/ExtraOptions.tsx @@ -50,7 +50,16 @@ const ExtraOptions = ({ const createAsOpen = !!(db?.allow_ctas || db?.allow_cvas); const isFileUploadSupportedByEngine = db?.engine_information?.supports_file_upload; - const extraJson: ExtraJson = JSON.parse(db?.extra || '{}'); + + // JSON.parse will deep parse engine_params + // if it's an object, and we want to keep it a string + const extraJson: ExtraJson = JSON.parse(db?.extra || '{}', (key, value) => { + if (key === 'engine_params' && typeof value === 'object') { + // keep this as a string + return JSON.stringify(value); + } + return value; + }); return (
@@ -442,17 +451,17 @@ const ExtraOptions = ({
onExtraEditorChange({ json, name: 'metadata_params' }) } width="100%" height="160px" + defaultValue={ + !Object.keys(extraJson?.metadata_params || {}).length + ? '' + : extraJson?.metadata_params + } />
@@ -468,17 +477,17 @@ const ExtraOptions = ({
onExtraEditorChange({ json, name: 'engine_params' }) } width="100%" height="160px" + defaultValue={ + !Object.keys(extraJson?.engine_params || {}).length + ? '' + : extraJson?.engine_params + } />
@@ -497,9 +506,9 @@ const ExtraOptions = ({
diff --git a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.test.tsx b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.test.tsx index c279ea88511..79b0bd62692 100644 --- a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.test.tsx +++ b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.test.tsx @@ -25,12 +25,13 @@ import { within, cleanup, act, + waitFor, } from 'spec/helpers/testing-library'; -import * as hooks from 'src/views/CRUD/hooks'; import { DatabaseObject, CONFIGURATION_METHOD, } from 'src/views/CRUD/data/database/types'; +import * as hooks from 'src/views/CRUD/hooks'; import DatabaseModal, { dbReducer, DBReducerActionType, @@ -47,6 +48,20 @@ const dbProps = { const DATABASE_FETCH_ENDPOINT = 'glob:*/api/v1/database/10'; const AVAILABLE_DB_ENDPOINT = 'glob:*/api/v1/database/available*'; const VALIDATE_PARAMS_ENDPOINT = 'glob:*/api/v1/database/validate_parameters*'; +const DATABASE_CONNECT_ENDPOINT = 'glob:*/api/v1/database/'; + +fetchMock.post(DATABASE_CONNECT_ENDPOINT, { + id: 10, + result: { + configuration_method: 'sqlalchemy_form', + database_name: 'Other2', + driver: 'apsw', + expose_in_sqllab: true, + extra: '{"allows_virtual_table_explore":true}', + sqlalchemy_uri: 'gsheets://', + }, + json: 'foo', +}); fetchMock.config.overwriteRoutes = true; fetchMock.get(DATABASE_FETCH_ENDPOINT, { @@ -1178,6 +1193,7 @@ describe('DatabaseModal', () => { const databaseNameField = textboxes[1]; const usernameField = textboxes[2]; const passwordField = textboxes[3]; + const connectButton = screen.getByRole('button', { name: 'Connect' }); expect(hostField).toHaveValue(''); expect(portField).toHaveValue(null); @@ -1198,19 +1214,10 @@ describe('DatabaseModal', () => { expect(usernameField).toHaveValue('testdb'); expect(passwordField).toHaveValue('demoPassword'); - /* ---------- 🐞 TODO (lyndsiWilliams): function mock is not currently working 🐞 ---------- - - // Mock useSingleViewResource - const mockUseSingleViewResource = jest.fn(); - mockUseSingleViewResource.mockImplementation(useSingleViewResource); - - const { fetchResource } = mockUseSingleViewResource('database'); - - // Invalid hook call? - userEvent.click(screen.getByRole('button', { name: 'Connect' })); - expect(fetchResource).toHaveBeenCalled(); - - */ + userEvent.click(connectButton); + await waitFor(() => { + expect(fetchMock.calls(VALIDATE_PARAMS_ENDPOINT).length).toEqual(6); + }); }); }); @@ -1325,23 +1332,6 @@ describe('DatabaseModal', () => { ...jest.requireActual('src/views/CRUD/hooks'), useSingleViewResource: jest.fn(), })); - const useSingleViewResourceMock = jest.spyOn( - hooks, - 'useSingleViewResource', - ); - - useSingleViewResourceMock.mockReturnValue({ - state: { - loading: false, - resource: null, - error: { _schema: 'Test Error With Object' }, - }, - fetchResource: jest.fn(), - createResource: jest.fn(), - updateResource: jest.fn(), - clearError: jest.fn(), - setResource: jest.fn(), - }); const renderAndWait = async () => { const mounted = act(async () => { @@ -1442,7 +1432,7 @@ describe('dbReducer', () => { test('it will set state to payload from extra editor', () => { const action: DBReducerActionType = { type: ActionType.extraEditorChange, - payload: { name: 'foo', json: { bar: 1 } }, + payload: { name: 'foo', json: JSON.stringify({ bar: 1 }) }, }; const currentState = dbReducer(databaseFixture, action); // extra should be serialized @@ -1455,20 +1445,20 @@ describe('dbReducer', () => { test('it will set state to payload from editor', () => { const action: DBReducerActionType = { type: ActionType.editorChange, - payload: { name: 'foo', json: { bar: 1 } }, + payload: { name: 'foo', json: JSON.stringify({ bar: 1 }) }, }; const currentState = dbReducer(databaseFixture, action); // extra should be serialized expect(currentState).toEqual({ ...databaseFixture, - foo: { bar: 1 }, + foo: JSON.stringify({ bar: 1 }), }); }); test('it will add extra payload to existing extra data', () => { const action: DBReducerActionType = { type: ActionType.extraEditorChange, - payload: { name: 'foo', json: { bar: 1 } }, + payload: { name: 'foo', json: JSON.stringify({ bar: 1 }) }, }; // extra should be a string const currentState = dbReducer( diff --git a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx index 7cd030dab4d..095e4f0225d 100644 --- a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx +++ b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx @@ -150,7 +150,7 @@ export enum ActionType { interface DBReducerPayloadType { target?: string; name: string; - json?: {}; + json?: string; type?: string; checked?: boolean; value?: string; @@ -215,20 +215,28 @@ export function dbReducer( let query = {}; let query_input = ''; let parametersCatalog; + let actionPayloadJson; const extraJson: ExtraJson = JSON.parse(trimmedState.extra || '{}'); switch (action.type) { case ActionType.extraEditorChange: // "extra" payload in state is a string + try { + // we don't want to stringify encoded strings twice + actionPayloadJson = JSON.parse(action.payload.json || '{}'); + } catch (e) { + actionPayloadJson = action.payload.json; + } return { ...trimmedState, extra: JSON.stringify({ ...extraJson, - [action.payload.name]: action.payload.json, + [action.payload.name]: actionPayloadJson, }), }; case ActionType.extraInputChange: // "extra" payload in state is a string + if ( action.payload.name === 'schema_cache_timeout' || action.payload.name === 'table_cache_timeout'