fix: make database connection modal ace fields uncontrolled (#22350)

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 (
     <Collapse
@@ -123,9 +132,9 @@ const ExtraOptions = ({
                   <input
                     type="text"
                     name="force_ctas_schema"
-                    value={db?.force_ctas_schema || ''}
                     placeholder={t('Create or select schema...')}
                     onChange={onInputChange}
+                    value={db?.force_ctas_schema || ''}
                   />
                 </div>
                 <div className="helper">
@@ -442,17 +451,17 @@ const ExtraOptions = ({
           <div className="input-container">
             <StyledJsonEditor
               name="metadata_params"
-              value={
-                !Object.keys(extraJson?.metadata_params || {}).length
-                  ? ''
-                  : extraJson?.metadata_params
-              }
               placeholder={t('Metadata Parameters')}
               onChange={(json: string) =>
                 onExtraEditorChange({ json, name: 'metadata_params' })
               }
               width="100%"
               height="160px"
+              defaultValue={
+                !Object.keys(extraJson?.metadata_params || {}).length
+                  ? ''
+                  : extraJson?.metadata_params
+              }
             />
           </div>
           <div className="helper">
@@ -468,17 +477,17 @@ const ExtraOptions = ({
           <div className="input-container">
             <StyledJsonEditor
               name="engine_params"
-              value={
-                !Object.keys(extraJson?.engine_params || {}).length
-                  ? ''
-                  : extraJson?.engine_params
-              }
               placeholder={t('Engine Parameters')}
               onChange={(json: string) =>
                 onExtraEditorChange({ json, name: 'engine_params' })
               }
               width="100%"
               height="160px"
+              defaultValue={
+                !Object.keys(extraJson?.engine_params || {}).length
+                  ? ''
+                  : extraJson?.engine_params
+              }
             />
           </div>
           <div className="helper">
@@ -497,9 +506,9 @@ const ExtraOptions = ({
             <input
               type="number"
               name="version"
-              value={extraJson?.version || ''}
               placeholder={t('Version number')}
               onChange={onExtraInputChange}
+              value={extraJson?.version || ''}
             />
           </div>
           <div className="helper">

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(

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'