From 8cc97e47906f83b763cc018b9ba5ddbb3bb9aea7 Mon Sep 17 00:00:00 2001
From: Elizabeth Thompson
Date: Tue, 1 Jun 2021 15:18:17 -0700
Subject: [PATCH] feat: validation db modal (#14832)
* split db modal file
* hook up available databases
* use new validation component
---
.../Form/LabeledErrorBoundInput.tsx | 18 +-
.../views/CRUD/data/database/DatabaseList.tsx | 3 +-
.../DatabaseModal/DatabaseConnectionForm.tsx | 240 +++++++++++-------
.../data/database/DatabaseModal/index.tsx | 49 ++--
.../data/database/DatabaseModal/styles.ts | 37 +--
.../src/views/CRUD/data/database/types.ts | 3 +-
superset-frontend/src/views/CRUD/hooks.ts | 57 +++++
superset/databases/commands/validate.py | 6 +-
superset/databases/schemas.py | 6 +
superset/db_engine_specs/base.py | 6 +-
10 files changed, 274 insertions(+), 151 deletions(-)
diff --git a/superset-frontend/src/components/Form/LabeledErrorBoundInput.tsx b/superset-frontend/src/components/Form/LabeledErrorBoundInput.tsx
index 8569b554a0..75df2bb088 100644
--- a/superset-frontend/src/components/Form/LabeledErrorBoundInput.tsx
+++ b/superset-frontend/src/components/Form/LabeledErrorBoundInput.tsx
@@ -25,17 +25,18 @@ import FormLabel from './FormLabel';
export interface LabeledErrorBoundInputProps {
label?: string;
validationMethods:
- | { onBlur: (value: any) => string }
- | { onChange: (value: any) => string };
+ | { onBlur: (value: any) => void }
+ | { onChange: (value: any) => void };
errorMessage: string | null;
helpText?: string;
required?: boolean;
id?: string;
+ classname?: string;
[x: string]: any;
}
const StyledInput = styled(Input)`
- margin: 8px 0;
+ margin: ${({ theme }) => `${theme.gridUnit}px 0 ${theme.gridUnit * 2}px`};
`;
const alertIconStyles = (theme: SupersetTheme, hasError: boolean) => css`
@@ -60,6 +61,12 @@ const alertIconStyles = (theme: SupersetTheme, hasError: boolean) => css`
}
}`}
`;
+const StyledFormGroup = styled('div')`
+ margin-bottom: ${({ theme }) => theme.gridUnit * 5}px;
+ .ant-form-item {
+ margin-bottom: 0;
+ }
+`;
const LabeledErrorBoundInput = ({
label,
@@ -68,9 +75,10 @@ const LabeledErrorBoundInput = ({
helpText,
required = false,
id,
+ className,
...props
}: LabeledErrorBoundInputProps) => (
- <>
+
{label}
@@ -83,7 +91,7 @@ const LabeledErrorBoundInput = ({
>
- >
+
);
export default LabeledErrorBoundInput;
diff --git a/superset-frontend/src/views/CRUD/data/database/DatabaseList.tsx b/superset-frontend/src/views/CRUD/data/database/DatabaseList.tsx
index d1105e7062..fda65baa27 100644
--- a/superset-frontend/src/views/CRUD/data/database/DatabaseList.tsx
+++ b/superset-frontend/src/views/CRUD/data/database/DatabaseList.tsx
@@ -29,8 +29,9 @@ import { Tooltip } from 'src/components/Tooltip';
import Icons from 'src/components/Icons';
import ListView, { FilterOperator, Filters } from 'src/components/ListView';
import { commonMenuData } from 'src/views/CRUD/data/common';
-import DatabaseModal from 'src/views/CRUD/data/database/DatabaseModal';
import ImportModelsModal from 'src/components/ImportModal/index';
+import DatabaseModal from './DatabaseModal';
+
import { DatabaseObject } from './types';
const PAGE_SIZE = 25;
diff --git a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
index f0542e481c..5c7c729da5 100644
--- a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
+++ b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
@@ -17,11 +17,14 @@
* under the License.
*/
import React, { FormEvent } from 'react';
-import cx from 'classnames';
+import { SupersetTheme, JsonObject } from '@superset-ui/core';
import { InputProps } from 'antd/lib/input';
-import { FormLabel, FormItem } from 'src/components/Form';
-import { Input } from 'src/common/components';
-import { StyledFormHeader, formScrollableStyles } from './styles';
+import ValidatedInput from 'src/components/Form/LabeledErrorBoundInput';
+import {
+ StyledFormHeader,
+ formScrollableStyles,
+ validatedFormStyles,
+} from './styles';
import { DatabaseForm } from '../types';
export const FormFieldOrder = [
@@ -33,64 +36,137 @@ export const FormFieldOrder = [
'database_name',
];
-const CHANGE_METHOD = {
- onChange: 'onChange',
- onPropertiesChange: 'onPropertiesChange',
-};
+interface FieldPropTypes {
+ required: boolean;
+ changeMethods: { onParametersChange: (value: any) => string } & {
+ onChange: (value: any) => string;
+ };
+ validationErrors: JsonObject | null;
+ getValidation: () => void;
+}
+
+const hostField = ({
+ required,
+ changeMethods,
+ getValidation,
+ validationErrors,
+}: FieldPropTypes) => (
+
+);
+const portField = ({
+ required,
+ changeMethods,
+ getValidation,
+ validationErrors,
+}: FieldPropTypes) => (
+
+);
+const databaseField = ({
+ required,
+ changeMethods,
+ getValidation,
+ validationErrors,
+}: FieldPropTypes) => (
+
+);
+const usernameField = ({
+ required,
+ changeMethods,
+ getValidation,
+ validationErrors,
+}: FieldPropTypes) => (
+
+);
+const passwordField = ({
+ required,
+ changeMethods,
+ getValidation,
+ validationErrors,
+}: FieldPropTypes) => (
+
+);
+const displayField = ({
+ required,
+ changeMethods,
+ getValidation,
+ validationErrors,
+}: FieldPropTypes) => (
+
+);
const FORM_FIELD_MAP = {
- host: {
- description: 'Host',
- type: 'text',
- className: 'w-50',
- placeholder: 'e.g. 127.0.0.1',
- changeMethod: CHANGE_METHOD.onPropertiesChange,
- },
- port: {
- description: 'Port',
- type: 'text',
- className: 'w-50',
- placeholder: 'e.g. 5432',
- changeMethod: CHANGE_METHOD.onPropertiesChange,
- },
- database: {
- description: 'Database name',
- type: 'text',
- label:
- 'Copy the name of the PostgreSQL database you are trying to connect to.',
- placeholder: 'e.g. world_population',
- changeMethod: CHANGE_METHOD.onPropertiesChange,
- },
- username: {
- description: 'Username',
- type: 'text',
- placeholder: 'e.g. Analytics',
- changeMethod: CHANGE_METHOD.onPropertiesChange,
- },
- password: {
- description: 'Password',
- type: 'text',
- placeholder: 'e.g. ********',
- changeMethod: CHANGE_METHOD.onPropertiesChange,
- },
- database_name: {
- description: 'Display Name',
- type: 'text',
- label: 'Pick a nickname for this database to display as in Superset.',
- changeMethod: CHANGE_METHOD.onChange,
- },
- query: {
- additionalProperties: {},
- description: 'Additional parameters',
- type: 'object',
- changeMethod: CHANGE_METHOD.onPropertiesChange,
- },
+ host: hostField,
+ port: portField,
+ database: databaseField,
+ username: usernameField,
+ password: passwordField,
+ database_name: displayField,
};
const DatabaseConnectionForm = ({
dbModel: { name, parameters },
onParametersChange,
onChange,
+ validationErrors,
+ getValidation,
}: {
dbModel: DatabaseForm;
onParametersChange: (
@@ -99,6 +175,8 @@ const DatabaseConnectionForm = ({
onChange: (
event: FormEvent | { target: HTMLInputElement },
) => void;
+ validationErrors: JsonObject | null;
+ getValidation: () => void;
}) => (
<>
@@ -107,52 +185,30 @@ const DatabaseConnectionForm = ({
Need help? Learn more about connecting to {name}.
-
+
[
+ formScrollableStyles,
+ validatedFormStyles(theme),
+ ]}
+ >
{parameters &&
FormFieldOrder.filter(
(key: string) =>
Object.keys(parameters.properties).includes(key) ||
key === 'database_name',
- ).map(field => {
- const {
- className,
- description,
- type,
- placeholder,
- label,
- changeMethod,
- } = FORM_FIELD_MAP[field];
- const onEdit =
- changeMethod === CHANGE_METHOD.onChange
- ? onChange
- : onParametersChange;
- return (
-
-
- {description}
-
-
- {label}
-
- );
- })}
+ ).map(field =>
+ FORM_FIELD_MAP[field]({
+ required: parameters.required.includes(field),
+ changeMethods: { onParametersChange, onChange },
+ validationErrors,
+ getValidation,
+ key: field,
+ }),
+ )}
>
);
-
export const FormFieldMap = FORM_FIELD_MAP;
export default DatabaseConnectionForm;
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 545cba00a7..265a856192 100644
--- a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx
+++ b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx
@@ -33,6 +33,7 @@ import {
testDatabaseConnection,
useSingleViewResource,
useAvailableDatabases,
+ useDatabaseValidation,
} from 'src/views/CRUD/hooks';
import { useCommonConf } from 'src/views/CRUD/data/database/state';
import {
@@ -50,10 +51,9 @@ import {
antDModalStyles,
antDTabsStyles,
buttonLinkStyles,
- CreateHeader,
+ TabHeader,
CreateHeaderSubtitle,
CreateHeaderTitle,
- EditHeader,
EditHeaderSubtitle,
EditHeaderTitle,
formHelperStyles,
@@ -109,7 +109,7 @@ type DBReducerActionType =
| {
type: ActionType.dbSelected;
payload: {
- parameters: { engine?: string };
+ engine?: string;
configuration_method: CONFIGURATION_METHOD;
};
}
@@ -127,8 +127,6 @@ function dbReducer(
): Partial
| null {
const trimmedState = {
...(state || {}),
- database_name: state?.database_name?.trim() || '',
- sqlalchemy_uri: state?.sqlalchemy_uri || '',
};
switch (action.type) {
@@ -163,9 +161,7 @@ function dbReducer(
};
case ActionType.fetched:
return {
- parameters: {
- engine: trimmedState.parameters?.engine,
- },
+ engine: trimmedState.engine,
configuration_method: trimmedState.configuration_method,
...action.payload,
};
@@ -196,13 +192,16 @@ const DatabaseModal: FunctionComponent = ({
>(dbReducer, null);
const [tabKey, setTabKey] = useState(DEFAULT_TAB_KEY);
const [availableDbs, getAvailableDbs] = useAvailableDatabases();
+ const [validationErrors, getValidation] = useDatabaseValidation();
const [hasConnectedDb, setHasConnectedDb] = useState(false);
+ const [dbName, setDbName] = useState('');
const conf = useCommonConf();
const isEditMode = !!databaseId;
const useSqlAlchemyForm =
db?.configuration_method === CONFIGURATION_METHOD.SQLALCHEMY_URI;
const useTabLayout = isEditMode || useSqlAlchemyForm;
+
// Database fetch logic
const {
state: { loading: dbLoading, resource: dbFetched },
@@ -248,14 +247,16 @@ const DatabaseModal: FunctionComponent = ({
// don't pass parameters if using the sqlalchemy uri
delete update.parameters;
}
- updateResource(db.id as number, update as DatabaseObject).then(result => {
- if (result) {
- if (onDatabaseAdd) {
- onDatabaseAdd();
- }
- onClose();
+ const result = await updateResource(
+ db.id as number,
+ update as DatabaseObject,
+ );
+ if (result) {
+ if (onDatabaseAdd) {
+ onDatabaseAdd();
}
- });
+ onClose();
+ }
} else if (db) {
// Create
const dbId = await createResource(update as DatabaseObject);
@@ -300,7 +301,6 @@ const DatabaseModal: FunctionComponent = ({
setDB({
type: ActionType.dbSelected,
payload: {
- parameters: {},
configuration_method: CONFIGURATION_METHOD.SQLALCHEMY_URI,
}, // todo hook this up to step 1
});
@@ -316,6 +316,9 @@ const DatabaseModal: FunctionComponent = ({
type: ActionType.fetched,
payload: dbFetched,
});
+ // keep a copy of the name separate for display purposes
+ // because it shouldn't change when the form is updated
+ setDbName(dbFetched.database_name);
}
}, [dbFetched]);
@@ -326,7 +329,7 @@ const DatabaseModal: FunctionComponent = ({
const dbModel: DatabaseForm =
availableDbs?.databases?.find(
(available: { engine: string | undefined }) =>
- available.engine === db?.parameters?.engine,
+ available.engine === db?.engine,
) || {};
const disableSave =
@@ -362,12 +365,12 @@ const DatabaseModal: FunctionComponent = ({
}
>
{isEditMode ? (
-
+
{db?.backend}
- {db?.database_name}
-
+ {dbName}
+
) : (
-
+
Enter Primary Credentials
Need help? Learn how to connect your database{' '}
@@ -380,7 +383,7 @@ const DatabaseModal: FunctionComponent = ({
.
-
+
)}
= ({
value: target.value,
})
}
+ getValidation={() => getValidation(db)}
+ validationErrors={validationErrors}
/>