From 196c3671e279c3c7e3f8644f43017b3f81b707c2 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Fri, 14 Oct 2022 14:55:20 -0700 Subject: [PATCH] refactor: serialize extra json in state (#21523) --- .../database/DatabaseModal/ExtraOptions.tsx | 37 +- .../{index.test.jsx => index.test.tsx} | 405 +++++++++++++++++- .../data/database/DatabaseModal/index.tsx | 173 ++++---- .../src/views/CRUD/data/database/types.ts | 67 +-- 4 files changed, 535 insertions(+), 147 deletions(-) rename superset-frontend/src/views/CRUD/data/database/DatabaseModal/{index.test.jsx => index.test.tsx} (80%) 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 349264d681..243ee27fe2 100644 --- a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/ExtraOptions.tsx +++ b/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 ( @@ -171,7 +172,7 @@ const ExtraOptions = ({ @@ -187,7 +188,7 @@ const ExtraOptions = ({ @@ -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 = ({ @@ -414,9 +413,9 @@ const ExtraOptions = ({ @@ -443,7 +442,11 @@ const ExtraOptions = ({
onExtraEditorChange({ json, name: 'metadata_params' }) @@ -465,7 +468,11 @@ const ExtraOptions = ({
onExtraEditorChange({ json, name: 'engine_params' }) @@ -490,7 +497,7 @@ const ExtraOptions = ({ diff --git a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.test.jsx b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.test.tsx similarity index 80% rename from superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.test.jsx rename to superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.test.tsx index 4fb5d60cdd..6a0173fd56 100644 --- a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.test.jsx +++ b/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.item(0)).toStrictEqual(testFile); + expect(importDbButton.files?.[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: [], + }); + }); +}); 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 37c13f17e5..003f9d64be 100644 --- a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx +++ b/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 | null, action: DBReducerActionType, ): Partial | null { @@ -210,54 +213,56 @@ function dbReducer( }; let query = {}; let query_input = ''; - let deserializeExtraJSON = { allows_virtual_table_explore: true }; - let extra_json: DatabaseObject['extra_json']; + let parametersCatalog; + const extraJson: ExtraJson = JSON.parse(trimmedState.extra || '{}'); switch (action.type) { case ActionType.extraEditorChange: + // "extra" payload in state is a string return { ...trimmedState, - extra_json: { - ...trimmedState.extra_json, + extra: JSON.stringify({ + ...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: { - ...trimmedState.extra_json, + extra: JSON.stringify({ + ...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: { - ...trimmedState.extra_json, - schemas_allowed_for_file_upload: (action.payload.value || '').split( - ',', - ), - }, + extra: JSON.stringify({ + ...extraJson, + schemas_allowed_for_file_upload: (action.payload.value || '') + .split(',') + .filter(schema => schema !== ''), + }), }; } return { ...trimmedState, - extra_json: { - ...trimmedState.extra_json, + extra: JSON.stringify({ + ...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) => { - paramatersCatalog[item.name] = item.value; - }); + parametersCatalog = catalogCopy.reduce((obj, item: any) => { + 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_json?.engine_params?.catalog || {}, - ).map(([key, value]) => ({ - name: key, - value, - })); + // "extra" payload from the api is a string + const extraJsonPayload: ExtraJson = { + ...JSON.parse((action.payload.extra as string) || '{}'), + }; + + 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: engineParamsCatalog, - parameters: action.payload.parameters || trimmedState.parameters, + catalog: engineRootCatalog, + 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 = ({ addDangerToast, addSuccessToast, @@ -498,7 +496,7 @@ const DatabaseModal: FunctionComponent = ({ 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 = ({ const onSave = async () => { // Clone DB object - const dbToUpdate = JSON.parse(JSON.stringify(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 = {}; - } + const dbToUpdate = { ...(db || {}) }; 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 = ({ if (dbToUpdate?.parameters?.catalog) { // need to stringify gsheets catalog to allow it to be seralized - dbToUpdate.extra_json = { - engine_params: JSON.stringify({ + dbToUpdate.extra = 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); diff --git a/superset-frontend/src/views/CRUD/data/database/types.ts b/superset-frontend/src/views/CRUD/data/database/types.ts index 9a9386035e..373e0dbf83 100644 --- a/superset-frontend/src/views/CRUD/data/database/types.ts +++ b/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; + 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; - catalog?: Record; + catalog?: Record; properties?: Record; warehouse?: string; role?: string; account?: string; }; - configuration_method: CONFIGURATION_METHOD; - engine?: string; - paramProperties?: Record; // 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; - - // Extra - extra_json?: { - engine_params?: { - catalog?: Record | 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 - }; + server_cert?: string; // External management is_managed_externally: boolean; @@ -100,7 +82,6 @@ export type DatabaseObject = { // Temporary storage catalog?: Array; 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; + 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; +}