Skip to content

Commit

Permalink
fix: make database connection modal ace fields uncontrolled (#22350)
Browse files Browse the repository at this point in the history
  • Loading branch information
@eschutho
eschutho authored Dec 12, 2022
1 parent c3a6327 commit 608ffcb
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 50 deletions.
35 changes: 22 additions & 13 deletions superset-frontend/src/views/CRUD/data/database/DatabaseModal/ExtraOptions.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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">
Expand Down Expand Up @@ -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">
Expand All @@ -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">
Expand All @@ -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">
Expand Down
60 changes: 25 additions & 35 deletions superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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, {
Expand Down Expand Up @@ -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);
Expand All @@ -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);
});
});
});

Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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
Expand All @@ -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(
Expand Down
12 changes: 10 additions & 2 deletions superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ export enum ActionType {
interface DBReducerPayloadType {
target?: string;
name: string;
json?: {};
json?: string;
type?: string;
checked?: boolean;
value?: string;
Expand Down Expand Up @@ -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'
Expand Down

0 comments on commit 608ffcb

Please sign in to comment.