refactor: serialize extra json in state (#21523)

superset-frontend/src/views/CRUD/data/database/DatabaseModal/ExtraOptions.tsx

@@ -29,7 +29,7 @@ import {
   antdCollapseStyles,
   no_margin_bottom,
 } from './styles';
-import { DatabaseObject } from '../types';
+import { DatabaseObject, ExtraJson } from '../types';
 
 const ExtraOptions = ({
   db,
@@ -50,6 +50,7 @@ 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 || '{}');
 
   return (
     <Collapse
@@ -155,7 +156,7 @@ const ExtraOptions = ({
                 <IndeterminateCheckbox
                   id="cost_estimate_enabled"
                   indeterminate={false}
-                  checked={!!db?.extra_json?.cost_estimate_enabled}
+                  checked={!!extraJson?.cost_estimate_enabled}
                   onChange={onExtraInputChange}
                   labelText={t('Enable query cost estimation')}
                 />
@@ -171,7 +172,7 @@ const ExtraOptions = ({
                 <IndeterminateCheckbox
                   id="allows_virtual_table_explore"
                   indeterminate={false}
-                  checked={!!db?.extra_json?.allows_virtual_table_explore}
+                  checked={!!extraJson?.allows_virtual_table_explore}
                   onChange={onExtraInputChange}
                   labelText={t('Allow this database to be explored')}
                 />
@@ -187,7 +188,7 @@ const ExtraOptions = ({
                 <IndeterminateCheckbox
                   id="disable_data_preview"
                   indeterminate={false}
-                  checked={!!db?.extra_json?.disable_data_preview}
+                  checked={!!extraJson?.disable_data_preview}
                   onChange={onExtraInputChange}
                   labelText={t('Disable SQL Lab data preview queries')}
                 />
@@ -240,8 +241,7 @@ const ExtraOptions = ({
               type="number"
               name="schema_cache_timeout"
               value={
-                db?.extra_json?.metadata_cache_timeout?.schema_cache_timeout ||
+                extraJson?.metadata_cache_timeout?.schema_cache_timeout || ''
-                ''
               }
               placeholder={t('Enter duration in seconds')}
               onChange={onExtraInputChange}
@@ -262,8 +262,7 @@ const ExtraOptions = ({
               type="number"
               name="table_cache_timeout"
               value={
-                db?.extra_json?.metadata_cache_timeout?.table_cache_timeout ||
+                extraJson?.metadata_cache_timeout?.table_cache_timeout || ''
-                ''
               }
               placeholder={t('Enter duration in seconds')}
               onChange={onExtraInputChange}
@@ -301,7 +300,7 @@ const ExtraOptions = ({
             <IndeterminateCheckbox
               id="cancel_query_on_windows_unload"
               indeterminate={false}
-              checked={!!db?.extra_json?.cancel_query_on_windows_unload}
+              checked={!!extraJson?.cancel_query_on_windows_unload}
               onChange={onExtraInputChange}
               labelText={t('Cancel query on window unload event')}
             />
@@ -414,9 +413,9 @@ const ExtraOptions = ({
               <input
                 type="text"
                 name="schemas_allowed_for_file_upload"
-                value={(
+                value={(extraJson?.schemas_allowed_for_file_upload || []).join(
-                  db?.extra_json?.schemas_allowed_for_file_upload || []
+                  ',',
-                ).join(',')}
+                )}
                 placeholder="schema1,schema2"
                 onChange={onExtraInputChange}
               />
@@ -443,7 +442,11 @@ const ExtraOptions = ({
           <div className="input-container">
             <StyledJsonEditor
               name="metadata_params"
-              value={db?.extra_json?.metadata_params || ''}
+              value={
+                !Object.keys(extraJson?.metadata_params || {}).length
+                  ? ''
+                  : extraJson?.metadata_params
+              }
               placeholder={t('Metadata Parameters')}
               onChange={(json: string) =>
                 onExtraEditorChange({ json, name: 'metadata_params' })
@@ -465,7 +468,11 @@ const ExtraOptions = ({
           <div className="input-container">
             <StyledJsonEditor
               name="engine_params"
-              value={db?.extra_json?.engine_params || ''}
+              value={
+                !Object.keys(extraJson?.engine_params || {}).length
+                  ? ''
+                  : JSON.stringify(extraJson?.engine_params)
+              }
               placeholder={t('Engine Parameters')}
               onChange={(json: string) =>
                 onExtraEditorChange({ json, name: 'engine_params' })
@@ -490,7 +497,7 @@ const ExtraOptions = ({
             <input
               type="number"
               name="version"
-              value={db?.extra_json?.version || ''}
+              value={extraJson?.version || ''}
               placeholder={t('Version number')}
               onChange={onExtraInputChange}
             />

superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.test.tsx

@@ -27,12 +27,21 @@ import {
   act,
 } from 'spec/helpers/testing-library';
 import * as hooks from 'src/views/CRUD/hooks';
-import DatabaseModal from './index';
+import {
+  DatabaseObject,
+  CONFIGURATION_METHOD,
+} from 'src/views/CRUD/data/database/types';
+import DatabaseModal, {
+  dbReducer,
+  DBReducerActionType,
+  ActionType,
+} from './index';
 
 const dbProps = {
   show: true,
   database_name: 'my database',
   sqlalchemy_uri: 'postgres://superset:superset@something:1234/superset',
+  onHide: () => {},
 };
 
 const DATABASE_FETCH_ENDPOINT = 'glob:*/api/v1/database/10';
@@ -223,6 +232,14 @@ fetchMock.post(VALIDATE_PARAMS_ENDPOINT, {
   message: 'OK',
 });
 
+const databaseFixture: DatabaseObject = {
+  backend: 'postgres',
+  configuration_method: CONFIGURATION_METHOD.DYNAMIC_FORM,
+  database_name: 'Postgres',
+  name: 'PostgresDB',
+  is_managed_externally: false,
+};
+
 describe('DatabaseModal', () => {
   const renderAndWait = async () => {
     const mounted = act(async () => {
@@ -640,8 +657,6 @@ describe('DatabaseModal', () => {
         checkboxOffSVGs[2],
         checkboxOffSVGs[3],
         checkboxOffSVGs[4],
-        checkboxOffSVGs[5],
-        checkboxOffSVGs[6],
         tooltipIcons[0],
         tooltipIcons[1],
         tooltipIcons[2],
@@ -670,14 +685,13 @@ describe('DatabaseModal', () => {
         allowDbExplorationCheckbox,
         disableSQLLabDataPreviewQueriesCheckbox,
       ];
-
       visibleComponents.forEach(component => {
         expect(component).toBeVisible();
       });
       invisibleComponents.forEach(component => {
         expect(component).not.toBeVisible();
       });
-      expect(checkboxOffSVGs).toHaveLength(7);
+      expect(checkboxOffSVGs).toHaveLength(5);
       expect(tooltipIcons).toHaveLength(7);
     });
 
@@ -1169,7 +1183,9 @@ describe('DatabaseModal', () => {
 
     describe('Import database flow', () => {
       test('imports a file', async () => {
-        const importDbButton = screen.getByTestId('import-database-btn');
+        const importDbButton = screen.getByTestId(
+          'import-database-btn',
+        ) as HTMLInputElement;
         expect(importDbButton).toBeVisible();
 
         const testFile = new File([new ArrayBuffer(1)], 'model_export.zip');
@@ -1177,8 +1193,8 @@ describe('DatabaseModal', () => {
         userEvent.click(importDbButton);
         userEvent.upload(importDbButton, testFile);
 
-        expect(importDbButton.files[0]).toStrictEqual(testFile);
+        expect(importDbButton.files?.[0]).toStrictEqual(testFile);
-        expect(importDbButton.files.item(0)).toStrictEqual(testFile);
+        expect(importDbButton.files?.item(0)).toStrictEqual(testFile);
         expect(importDbButton.files).toHaveLength(1);
       });
     });
@@ -1291,6 +1307,7 @@ describe('DatabaseModal', () => {
       createResource: jest.fn(),
       updateResource: jest.fn(),
       clearError: jest.fn(),
+      setResource: jest.fn(),
     });
 
     const renderAndWait = async () => {
@@ -1335,6 +1352,7 @@ describe('DatabaseModal', () => {
       createResource: jest.fn(),
       updateResource: jest.fn(),
       clearError: jest.fn(),
+      setResource: jest.fn(),
     });
 
     const renderAndWait = async () => {
@@ -1361,3 +1379,374 @@ describe('DatabaseModal', () => {
     });
   });
 });
+
+describe('dbReducer', () => {
+  test('it will reset state to null', () => {
+    const action: DBReducerActionType = { type: ActionType.reset };
+    const currentState = dbReducer(databaseFixture, action);
+    expect(currentState).toBeNull();
+  });
+
+  test('it will set state to payload from fetched', () => {
+    const action: DBReducerActionType = {
+      type: ActionType.fetched,
+      payload: databaseFixture,
+    };
+    const currentState = dbReducer({}, action);
+    expect(currentState).toEqual({
+      ...databaseFixture,
+      engine: 'postgres',
+      masked_encrypted_extra: '',
+      parameters: undefined,
+      query_input: '',
+    });
+  });
+
+  test('it will set state to payload from extra editor', () => {
+    const action: DBReducerActionType = {
+      type: ActionType.extraEditorChange,
+      payload: { name: 'foo', json: { bar: 1 } },
+    };
+    const currentState = dbReducer(databaseFixture, action);
+    // extra should be serialized
+    expect(currentState).toEqual({
+      ...databaseFixture,
+      extra: '{"foo":{"bar":1}}',
+    });
+  });
+
+  test('it will set state to payload from editor', () => {
+    const action: DBReducerActionType = {
+      type: ActionType.editorChange,
+      payload: { name: 'foo', json: { bar: 1 } },
+    };
+    const currentState = dbReducer(databaseFixture, action);
+    // extra should be serialized
+    expect(currentState).toEqual({
+      ...databaseFixture,
+      foo: { bar: 1 },
+    });
+  });
+
+  test('it will add extra payload to existing extra data', () => {
+    const action: DBReducerActionType = {
+      type: ActionType.extraEditorChange,
+      payload: { name: 'foo', json: { bar: 1 } },
+    };
+    // extra should be a string
+    const currentState = dbReducer(
+      {
+        ...databaseFixture,
+        extra: JSON.stringify({ name: 'baz', json: { fiz: 2 } }),
+      },
+      action,
+    );
+    // extra should be serialized
+    expect(currentState).toEqual({
+      ...databaseFixture,
+      extra: '{"name":"baz","json":{"fiz":2},"foo":{"bar":1}}',
+    });
+  });
+
+  test('it will set state to payload from extra input change', () => {
+    const action: DBReducerActionType = {
+      type: ActionType.extraInputChange,
+      payload: { name: 'foo', value: 'bar' },
+    };
+    const currentState = dbReducer(databaseFixture, action);
+
+    // extra should be serialized
+    expect(currentState).toEqual({
+      ...databaseFixture,
+      extra: '{"foo":"bar"}',
+    });
+  });
+
+  test('it will set state to payload from extra input change when checkbox', () => {
+    const action: DBReducerActionType = {
+      type: ActionType.extraInputChange,
+      payload: { name: 'foo', type: 'checkbox', checked: true },
+    };
+    const currentState = dbReducer(databaseFixture, action);
+
+    // extra should be serialized
+    expect(currentState).toEqual({
+      ...databaseFixture,
+      extra: '{"foo":true}',
+    });
+  });
+
+  test('it will set state to payload from extra input change when schema_cache_timeout', () => {
+    const action: DBReducerActionType = {
+      type: ActionType.extraInputChange,
+      payload: { name: 'schema_cache_timeout', value: 'bar' },
+    };
+    const currentState = dbReducer(databaseFixture, action);
+
+    // extra should be serialized
+    expect(currentState).toEqual({
+      ...databaseFixture,
+      extra: '{"metadata_cache_timeout":{"schema_cache_timeout":"bar"}}',
+    });
+  });
+
+  test('it will set state to payload from extra input change when table_cache_timeout', () => {
+    const action: DBReducerActionType = {
+      type: ActionType.extraInputChange,
+      payload: { name: 'table_cache_timeout', value: 'bar' },
+    };
+    const currentState = dbReducer(databaseFixture, action);
+
+    // extra should be serialized
+    expect(currentState).toEqual({
+      ...databaseFixture,
+      extra: '{"metadata_cache_timeout":{"table_cache_timeout":"bar"}}',
+    });
+  });
+
+  test('it will overwrite state to payload from extra input change when table_cache_timeout', () => {
+    const action: DBReducerActionType = {
+      type: ActionType.extraInputChange,
+      payload: { name: 'table_cache_timeout', value: 'bar' },
+    };
+    const currentState = dbReducer(
+      {
+        ...databaseFixture,
+        extra: '{"metadata_cache_timeout":{"table_cache_timeout":"foo"}}',
+      },
+      action,
+    );
+
+    // extra should be serialized
+    expect(currentState).toEqual({
+      ...databaseFixture,
+      extra: '{"metadata_cache_timeout":{"table_cache_timeout":"bar"}}',
+    });
+  });
+
+  test(`it will set state to payload from extra
+  input change when schemas_allowed_for_file_upload`, () => {
+    const action: DBReducerActionType = {
+      type: ActionType.extraInputChange,
+      payload: { name: 'schemas_allowed_for_file_upload', value: 'bar' },
+    };
+    const currentState = dbReducer(databaseFixture, action);
+
+    // extra should be serialized
+    expect(currentState).toEqual({
+      ...databaseFixture,
+      extra: '{"schemas_allowed_for_file_upload":["bar"]}',
+    });
+  });
+
+  test(`it will overwrite state to payload from extra
+  input change when schemas_allowed_for_file_upload`, () => {
+    const action: DBReducerActionType = {
+      type: ActionType.extraInputChange,
+      payload: { name: 'schemas_allowed_for_file_upload', value: 'bar' },
+    };
+    const currentState = dbReducer(
+      {
+        ...databaseFixture,
+        extra: '{"schemas_allowed_for_file_upload":["foo"]}',
+      },
+      action,
+    );
+
+    // extra should be serialized
+    expect(currentState).toEqual({
+      ...databaseFixture,
+      extra: '{"schemas_allowed_for_file_upload":["bar"]}',
+    });
+  });
+
+  test(`it will set state to payload from extra
+  input change when schemas_allowed_for_file_upload
+  with blank list`, () => {
+    const action: DBReducerActionType = {
+      type: ActionType.extraInputChange,
+      payload: { name: 'schemas_allowed_for_file_upload', value: 'bar,' },
+    };
+    const currentState = dbReducer(databaseFixture, action);
+
+    // extra should be serialized
+    expect(currentState).toEqual({
+      ...databaseFixture,
+      extra: '{"schemas_allowed_for_file_upload":["bar"]}',
+    });
+  });
+
+  test('it will set state to payload from input change', () => {
+    const action: DBReducerActionType = {
+      type: ActionType.inputChange,
+      payload: { name: 'foo', value: 'bar' },
+    };
+    const currentState = dbReducer(databaseFixture, action);
+
+    expect(currentState).toEqual({
+      ...databaseFixture,
+      foo: 'bar',
+    });
+  });
+
+  test('it will set state to payload from input change for checkbox', () => {
+    const action: DBReducerActionType = {
+      type: ActionType.inputChange,
+      payload: { name: 'foo', type: 'checkbox', checked: true },
+    };
+    const currentState = dbReducer(databaseFixture, action);
+
+    expect(currentState).toEqual({
+      ...databaseFixture,
+      foo: true,
+    });
+  });
+
+  test('it will change state to payload from input change for checkbox', () => {
+    const action: DBReducerActionType = {
+      type: ActionType.inputChange,
+      payload: { name: 'allow_ctas', type: 'checkbox', checked: false },
+    };
+    const currentState = dbReducer(
+      {
+        ...databaseFixture,
+        allow_ctas: true,
+      },
+      action,
+    );
+
+    expect(currentState).toEqual({
+      ...databaseFixture,
+      allow_ctas: false,
+    });
+  });
+
+  test('it will add a parameter', () => {
+    const action: DBReducerActionType = {
+      type: ActionType.parametersChange,
+      payload: { name: 'host', value: '127.0.0.1' },
+    };
+    const currentState = dbReducer(databaseFixture, action);
+
+    expect(currentState).toEqual({
+      ...databaseFixture,
+      parameters: {
+        host: '127.0.0.1',
+      },
+    });
+  });
+
+  test('it will add a parameter with existing parameters', () => {
+    const action: DBReducerActionType = {
+      type: ActionType.parametersChange,
+      payload: { name: 'port', value: '1234' },
+    };
+    const currentState = dbReducer(
+      {
+        ...databaseFixture,
+        parameters: {
+          host: '127.0.0.1',
+        },
+      },
+      action,
+    );
+
+    expect(currentState).toEqual({
+      ...databaseFixture,
+      parameters: {
+        host: '127.0.0.1',
+        port: '1234',
+      },
+    });
+  });
+
+  test('it will change a parameter with existing parameters', () => {
+    const action: DBReducerActionType = {
+      type: ActionType.parametersChange,
+      payload: { name: 'host', value: 'localhost' },
+    };
+    const currentState = dbReducer(
+      {
+        ...databaseFixture,
+        parameters: {
+          host: '127.0.0.1',
+        },
+      },
+      action,
+    );
+
+    expect(currentState).toEqual({
+      ...databaseFixture,
+      parameters: {
+        host: 'localhost',
+      },
+    });
+  });
+
+  test('it will set state to payload from parametersChange with catalog', () => {
+    const action: DBReducerActionType = {
+      type: ActionType.parametersChange,
+      payload: { name: 'name', type: 'catalog-0', value: 'bar' },
+    };
+    const currentState = dbReducer(
+      { ...databaseFixture, catalog: [{ name: 'foo', value: 'baz' }] },
+      action,
+    );
+
+    expect(currentState).toEqual({
+      ...databaseFixture,
+      catalog: [{ name: 'bar', value: 'baz' }],
+      parameters: {
+        catalog: {
+          bar: 'baz',
+        },
+      },
+    });
+  });
+
+  test('it will add a new catalog array when empty', () => {
+    const action: DBReducerActionType = {
+      type: ActionType.addTableCatalogSheet,
+    };
+    const currentState = dbReducer(databaseFixture, action);
+
+    expect(currentState).toEqual({
+      ...databaseFixture,
+      catalog: [{ name: '', value: '' }],
+    });
+  });
+
+  test('it will add a new catalog array when one exists', () => {
+    const action: DBReducerActionType = {
+      type: ActionType.addTableCatalogSheet,
+    };
+    const currentState = dbReducer(
+      { ...databaseFixture, catalog: [{ name: 'foo', value: 'baz' }] },
+      action,
+    );
+
+    expect(currentState).toEqual({
+      ...databaseFixture,
+      catalog: [
+        { name: 'foo', value: 'baz' },
+        { name: '', value: '' },
+      ],
+    });
+  });
+
+  test('it will remove a catalog when one exists', () => {
+    const action: DBReducerActionType = {
+      type: ActionType.removeTableCatalogSheet,
+      payload: { indexToDelete: 0 },
+    };
+    const currentState = dbReducer(
+      { ...databaseFixture, catalog: [{ name: 'foo', value: 'baz' }] },
+      action,
+    );
+
+    expect(currentState).toEqual({
+      ...databaseFixture,
+      catalog: [],
+    });
+  });
+});

superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx

@@ -60,6 +60,7 @@ import {
   CONFIGURATION_METHOD,
   CatalogObject,
   Engines,
+  ExtraJson,
 } from 'src/views/CRUD/data/database/types';
 import Loading from 'src/components/Loading';
 import ExtraOptions from './ExtraOptions';
@@ -88,6 +89,8 @@ import {
 } from './styles';
 import ModalHeader, { DOCUMENTATION_LINK } from './ModalHeader';
 
+const DEFAULT_EXTRA = JSON.stringify({ allows_virtual_table_explore: true });
+
 const engineSpecificAlertMapping = {
   [Engines.GSheet]: {
     message: 'Why do I need to create a database?',
@@ -121,14 +124,14 @@ const ErrorAlertContainer = styled.div`
 interface DatabaseModalProps {
   addDangerToast: (msg: string) => void;
   addSuccessToast: (msg: string) => void;
-  onDatabaseAdd?: (database?: DatabaseObject) => void; // TODO: should we add a separate function for edit?
+  onDatabaseAdd?: (database?: DatabaseObject) => void;
   onHide: () => void;
   show: boolean;
   databaseId: number | undefined; // If included, will go into edit mode
   dbEngine: string | undefined; // if included goto step 2 with engine already set
 }
 
-enum ActionType {
+export enum ActionType {
   configMethodChange,
   dbSelected,
   editorChange,
@@ -153,7 +156,7 @@ interface DBReducerPayloadType {
   value?: string;
 }
 
-type DBReducerActionType =
+export type DBReducerActionType =
   | {
       type:
         | ActionType.extraEditorChange
@@ -201,7 +204,7 @@ const StyledBtns = styled.div`
   margin-left: ${({ theme }) => theme.gridUnit * 3}px;
 `;
 
-function dbReducer(
+export function dbReducer(
   state: Partial<DatabaseObject> | null,
   action: DBReducerActionType,
 ): Partial<DatabaseObject> | null {
@@ -210,54 +213,56 @@ function dbReducer(
   };
   let query = {};
   let query_input = '';
-  let deserializeExtraJSON = { allows_virtual_table_explore: true };
+  let parametersCatalog;
-  let extra_json: DatabaseObject['extra_json'];
+  const extraJson: ExtraJson = JSON.parse(trimmedState.extra || '{}');
 
   switch (action.type) {
     case ActionType.extraEditorChange:
+      // "extra" payload in state is a string
       return {
         ...trimmedState,
-        extra_json: {
+        extra: JSON.stringify({
-          ...trimmedState.extra_json,
+          ...extraJson,
           [action.payload.name]: action.payload.json,
-        },
+        }),
       };
     case ActionType.extraInputChange:
+      // "extra" payload in state is a string
       if (
         action.payload.name === 'schema_cache_timeout' ||
         action.payload.name === 'table_cache_timeout'
       ) {
         return {
           ...trimmedState,
-          extra_json: {
+          extra: JSON.stringify({
-            ...trimmedState.extra_json,
+            ...extraJson,
             metadata_cache_timeout: {
-              ...trimmedState.extra_json?.metadata_cache_timeout,
+              ...extraJson?.metadata_cache_timeout,
               [action.payload.name]: action.payload.value,
             },
-          },
+          }),
         };
       }
       if (action.payload.name === 'schemas_allowed_for_file_upload') {
         return {
           ...trimmedState,
-          extra_json: {
+          extra: JSON.stringify({
-            ...trimmedState.extra_json,
+            ...extraJson,
-            schemas_allowed_for_file_upload: (action.payload.value || '').split(
+            schemas_allowed_for_file_upload: (action.payload.value || '')
-              ',',
+              .split(',')
-            ),
+              .filter(schema => schema !== ''),
-          },
+          }),
         };
       }
       return {
         ...trimmedState,
-        extra_json: {
+        extra: JSON.stringify({
-          ...trimmedState.extra_json,
+          ...extraJson,
           [action.payload.name]:
             action.payload.type === 'checkbox'
               ? action.payload.checked
               : action.payload.value,
-        },
+        }),
       };
     case ActionType.inputChange:
       if (action.payload.type === 'checkbox') {
@@ -271,26 +276,36 @@ function dbReducer(
         [action.payload.name]: action.payload.value,
       };
     case ActionType.parametersChange:
+      // catalog params will always have a catalog state for
+      // dbs that use a catalog, i.e., gsheets, even if the
+      // fields are empty strings
       if (
-        trimmedState.catalog !== undefined &&
+        action.payload.type?.startsWith('catalog') &&
-        action.payload.type?.startsWith('catalog')
+        trimmedState.catalog !== undefined
       ) {
         // Formatting wrapping google sheets table catalog
+        const catalogCopy: CatalogObject[] = [...trimmedState.catalog];
         const idx = action.payload.type?.split('-')[1];
-        const catalogToUpdate = trimmedState?.catalog[idx] || {};
+        const catalogToUpdate: CatalogObject = catalogCopy[idx] || {};
         catalogToUpdate[action.payload.name] = action.payload.value;
 
-        const paramatersCatalog = {};
+        // insert updated catalog to existing state
+        catalogCopy.splice(parseInt(idx, 10), 1, catalogToUpdate);
+
+        // format catalog for state
         // eslint-disable-next-line array-callback-return
-        trimmedState.catalog?.map((item: CatalogObject) => {
+        parametersCatalog = catalogCopy.reduce((obj, item: any) => {
-          paramatersCatalog[item.name] = item.value;
+          const catalog = { ...obj };
-        });
+          catalog[item.name] = item.value;
+          return catalog;
+        }, {});
 
         return {
           ...trimmedState,
+          catalog: catalogCopy,
           parameters: {
             ...trimmedState.parameters,
-            catalog: paramatersCatalog,
+            catalog: parametersCatalog,
           },
         };
       }
@@ -301,6 +316,7 @@ function dbReducer(
           [action.payload.name]: action.payload.value,
         },
       };
+
     case ActionType.addTableCatalogSheet:
       if (trimmedState.catalog !== undefined) {
         return {
@@ -337,22 +353,6 @@ function dbReducer(
         [action.payload.name]: action.payload.value,
       };
     case ActionType.fetched:
-      // convert all the keys in this payload into strings
-      if (action.payload.extra) {
-        extra_json = {
-          ...JSON.parse(action.payload.extra || ''),
-        } as DatabaseObject['extra_json'];
-
-        deserializeExtraJSON = {
-          ...deserializeExtraJSON,
-          ...JSON.parse(action.payload.extra || ''),
-          metadata_params: JSON.stringify(extra_json?.metadata_params),
-          engine_params: JSON.stringify(extra_json?.engine_params),
-          schemas_allowed_for_file_upload:
-            extra_json?.schemas_allowed_for_file_upload,
-        };
-      }
-
       // convert query to a string and store in query_input
       query = action.payload?.parameters?.query || {};
       query_input = Object.entries(query)
@@ -364,19 +364,26 @@ function dbReducer(
         action.payload.configuration_method ===
           CONFIGURATION_METHOD.DYNAMIC_FORM
       ) {
-        const engineParamsCatalog = Object.entries(
+        // "extra" payload from the api is a string
-          extra_json?.engine_params?.catalog || {},
+        const extraJsonPayload: ExtraJson = {
-        ).map(([key, value]) => ({
+          ...JSON.parse((action.payload.extra as string) || '{}'),
-          name: key,
+        };
-          value,
+
-        }));
+        const payloadCatalog = extraJsonPayload.engine_params?.catalog;
+
+        const engineRootCatalog = Object.entries(payloadCatalog || {}).map(
+          ([name, value]: string[]) => ({ name, value }),
+        );
+
         return {
           ...action.payload,
           engine: action.payload.backend || trimmedState.engine,
           configuration_method: action.payload.configuration_method,
-          extra_json: deserializeExtraJSON,
+          catalog: engineRootCatalog,
-          catalog: engineParamsCatalog,
+          parameters: {
-          parameters: action.payload.parameters || trimmedState.parameters,
+            ...(action.payload.parameters || trimmedState.parameters),
+            catalog: payloadCatalog,
+          },
           query_input,
         };
       }
@@ -385,16 +392,17 @@ function dbReducer(
         masked_encrypted_extra: action.payload.masked_encrypted_extra || '',
         engine: action.payload.backend || trimmedState.engine,
         configuration_method: action.payload.configuration_method,
-        extra_json: deserializeExtraJSON,
         parameters: action.payload.parameters || trimmedState.parameters,
         query_input,
       };
 
     case ActionType.dbSelected:
+      // set initial state for blank form
       return {
         ...action.payload,
+        extra: DEFAULT_EXTRA,
+        expose_in_sqllab: true,
       };
-
     case ActionType.configMethodChange:
       return {
         ...action.payload,
@@ -408,16 +416,6 @@ function dbReducer(
 
 const DEFAULT_TAB_KEY = '1';
 
-const serializeExtra = (extraJson: DatabaseObject['extra_json']) =>
-  JSON.stringify({
-    ...extraJson,
-    metadata_params: JSON.parse((extraJson?.metadata_params as string) || '{}'),
-    engine_params: JSON.parse((extraJson?.engine_params as string) || '{}'),
-    schemas_allowed_for_file_upload: (
-      extraJson?.schemas_allowed_for_file_upload || []
-    ).filter(schema => schema !== ''),
-  });
-
 const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
   addDangerToast,
   addSuccessToast,
@@ -498,7 +496,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
       sqlalchemy_uri: db?.sqlalchemy_uri || '',
       database_name: db?.database_name?.trim() || undefined,
       impersonate_user: db?.impersonate_user || undefined,
-      extra: serializeExtra(db?.extra_json) || undefined,
+      extra: db?.extra,
       masked_encrypted_extra: db?.masked_encrypted_extra || '',
       server_cert: db?.server_cert || undefined,
     };
@@ -567,28 +565,25 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
 
   const onSave = async () => {
     // Clone DB object
-    const dbToUpdate = JSON.parse(JSON.stringify(db || {}));
+    const dbToUpdate = { ...(db || {}) };
-
-    if (dbToUpdate.catalog) {
-      // convert catalog to fit /validate_parameters endpoint
-      dbToUpdate.catalog = Object.assign(
-        {},
-        ...dbToUpdate.catalog.map((x: { name: string; value: string }) => ({
-          [x.name]: x.value,
-        })),
-      );
-    } else {
-      dbToUpdate.catalog = {};
-    }
 
     if (dbToUpdate.configuration_method === CONFIGURATION_METHOD.DYNAMIC_FORM) {
       // Validate DB before saving
+      if (dbToUpdate?.parameters?.catalog) {
+        // need to stringify gsheets catalog to allow it to be serialized
+        dbToUpdate.extra = JSON.stringify({
+          ...JSON.parse(dbToUpdate.extra || '{}'),
+          engine_params: {
+            catalog: dbToUpdate.parameters.catalog,
+          },
+        });
+      }
       const errors = await getValidation(dbToUpdate, true);
       if ((validationErrors && !isEmpty(validationErrors)) || errors) {
         return;
       }
       const parameters_schema = isEditMode
-        ? dbToUpdate.parameters_schema.properties
+        ? dbToUpdate.parameters_schema?.properties
         : dbModel?.parameters.properties;
       const additionalEncryptedExtra = JSON.parse(
         dbToUpdate.masked_encrypted_extra || '{}',
@@ -633,16 +628,12 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
 
     if (dbToUpdate?.parameters?.catalog) {
       // need to stringify gsheets catalog to allow it to be seralized
-      dbToUpdate.extra_json = {
+      dbToUpdate.extra = JSON.stringify({
-        engine_params: JSON.stringify({
+        ...JSON.parse(dbToUpdate.extra || '{}'),
+        engine_params: {
           catalog: dbToUpdate.parameters.catalog,
-        }),
+        },
-      };
+      });
-    }
-
-    if (dbToUpdate?.extra_json) {
-      // convert extra_json to back to string
-      dbToUpdate.extra = serializeExtra(dbToUpdate?.extra_json);
     }
 
     setLoading(true);

superset-frontend/src/views/CRUD/data/database/types.ts

@@ -28,14 +28,18 @@ export type CatalogObject = {
 
 export type DatabaseObject = {
   // Connection + general
-  id?: number;
-  database_name: string;
-  name: string; // synonym to database_name
-  sqlalchemy_uri?: string;
   backend?: string;
-  created_by?: null | DatabaseUser;
-  changed_on_delta_humanized?: string;
   changed_on?: string;
+  changed_on_delta_humanized?: string;
+  configuration_method: CONFIGURATION_METHOD;
+  created_by?: null | DatabaseUser;
+  database_name: string;
+  engine?: string;
+  extra?: string;
+  id?: number;
+  name: string; // synonym to database_name
+  paramProperties?: Record<string, any>;
+  sqlalchemy_uri?: string;
   parameters?: {
     database_name?: string;
     host?: string;
@@ -47,52 +51,30 @@ export type DatabaseObject = {
     credentials_info?: string;
     service_account_info?: string;
     query?: Record<string, string>;
-    catalog?: Record<string, string>;
+    catalog?: Record<string, string | undefined>;
     properties?: Record<string, any>;
     warehouse?: string;
     role?: string;
     account?: string;
   };
-  configuration_method: CONFIGURATION_METHOD;
-  engine?: string;
-  paramProperties?: Record<string, any>;
 
   // Performance
   cache_timeout?: string;
   allow_run_async?: boolean;
 
   // SQL Lab
-  expose_in_sqllab?: boolean;
   allow_ctas?: boolean;
   allow_cvas?: boolean;
   allow_dml?: boolean;
+  expose_in_sqllab?: boolean;
   force_ctas_schema?: string;
 
   // Security
-  masked_encrypted_extra?: string;
-  server_cert?: string;
   allow_file_upload?: boolean;
   impersonate_user?: boolean;
+  masked_encrypted_extra?: string;
   parameters_schema?: Record<string, any>;
-
+  server_cert?: string;
-  // Extra
-  extra_json?: {
-    engine_params?: {
-      catalog?: Record<any, any> | string;
-    };
-    metadata_params?: {} | string;
-    metadata_cache_timeout?: {
-      schema_cache_timeout?: number; // in Performance
-      table_cache_timeout?: number; // in Performance
-    }; // No field, holds schema and table timeout
-    allows_virtual_table_explore?: boolean; // in SQL Lab
-    schemas_allowed_for_file_upload?: string[]; // in Security
-    cancel_query_on_windows_unload?: boolean; // in Performance
-
-    version?: string;
-    cost_estimate_enabled?: boolean; // in SQL Lab
-    disable_data_preview?: boolean; // in SQL Lab
-  };
 
   // External management
   is_managed_externally: boolean;
@@ -100,7 +82,6 @@ export type DatabaseObject = {
   // Temporary storage
   catalog?: Array<CatalogObject>;
   query_input?: string;
-  extra?: string;
 
   // DB Engine Spec information
   engine_information?: {
@@ -170,3 +151,23 @@ export enum Engines {
   GSheet = 'gsheets',
   Snowflake = 'snowflake',
 }
+
+export interface ExtraJson {
+  allows_virtual_table_explore?: boolean; // in SQL Lab
+  cancel_query_on_windows_unload?: boolean; // in Performance
+  cost_estimate_enabled?: boolean; // in SQL Lab
+  disable_data_preview?: boolean; // in SQL Lab
+  engine_params?: {
+    catalog?: Record<string, string>;
+    connect_args?: {
+      http_path?: string;
+    };
+  };
+  metadata_params?: {};
+  metadata_cache_timeout?: {
+    schema_cache_timeout?: number; // in Performance
+    table_cache_timeout?: number; // in Performance
+  }; // No field, holds schema and table timeout
+  schemas_allowed_for_file_upload?: string[]; // in Security
+  version?: string;
+}