feat: validation db modal (#14832)

* split db modal file

* hook up available databases

* use new validation component

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 }
+    | { onBlur: (value: any) => void }
-    | { onChange: (value: any) => string };
+    | { 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) => (
-  <>
+  <StyledFormGroup className={className}>
     <FormLabel htmlFor={id} required={required}>
       {label}
     </FormLabel>
@@ -83,7 +91,7 @@ const LabeledErrorBoundInput = ({
     >
       <StyledInput {...props} {...validationMethods} />
     </FormItem>
-  </>
+  </StyledFormGroup>
 );
 
 export default LabeledErrorBoundInput;

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;

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 ValidatedInput from 'src/components/Form/LabeledErrorBoundInput';
-import { Input } from 'src/common/components';
+import {
-import { StyledFormHeader, formScrollableStyles } from './styles';
+  StyledFormHeader,
+  formScrollableStyles,
+  validatedFormStyles,
+} from './styles';
 import { DatabaseForm } from '../types';
 
 export const FormFieldOrder = [
@@ -33,64 +36,137 @@ export const FormFieldOrder = [
   'database_name',
 ];
 
-const CHANGE_METHOD = {
+interface FieldPropTypes {
-  onChange: 'onChange',
+  required: boolean;
-  onPropertiesChange: 'onPropertiesChange',
+  changeMethods: { onParametersChange: (value: any) => string } & {
-};
+    onChange: (value: any) => string;
+  };
+  validationErrors: JsonObject | null;
+  getValidation: () => void;
+}
+
+const hostField = ({
+  required,
+  changeMethods,
+  getValidation,
+  validationErrors,
+}: FieldPropTypes) => (
+  <ValidatedInput
+    id="host"
+    name="host"
+    required={required}
+    validationMethods={{ onBlur: getValidation }}
+    errorMessage={validationErrors?.host}
+    placeholder="e.g. 127.0.0.1"
+    className="form-group-w-50"
+    label="Host"
+    onChange={changeMethods.onParametersChange}
+  />
+);
+const portField = ({
+  required,
+  changeMethods,
+  getValidation,
+  validationErrors,
+}: FieldPropTypes) => (
+  <ValidatedInput
+    id="port"
+    name="port"
+    required={required}
+    validationMethods={{ onBlur: getValidation }}
+    errorMessage={validationErrors?.port}
+    placeholder="e.g. 5432"
+    className="form-group-w-50"
+    label="Port"
+    onChange={changeMethods.onParametersChange}
+  />
+);
+const databaseField = ({
+  required,
+  changeMethods,
+  getValidation,
+  validationErrors,
+}: FieldPropTypes) => (
+  <ValidatedInput
+    id="database"
+    name="database"
+    required={required}
+    validationMethods={{ onBlur: getValidation }}
+    errorMessage={validationErrors?.database}
+    placeholder="e.g. world_population"
+    label="Database name"
+    onChange={changeMethods.onParametersChange}
+    helpText="Copy the name of the PostgreSQL database you are trying to connect to."
+  />
+);
+const usernameField = ({
+  required,
+  changeMethods,
+  getValidation,
+  validationErrors,
+}: FieldPropTypes) => (
+  <ValidatedInput
+    id="username"
+    name="username"
+    required={required}
+    validationMethods={{ onBlur: getValidation }}
+    errorMessage={validationErrors?.username}
+    placeholder="e.g. Analytics"
+    label="Username"
+    onChange={changeMethods.onParametersChange}
+  />
+);
+const passwordField = ({
+  required,
+  changeMethods,
+  getValidation,
+  validationErrors,
+}: FieldPropTypes) => (
+  <ValidatedInput
+    id="password"
+    name="password"
+    required={required}
+    validationMethods={{ onBlur: getValidation }}
+    errorMessage={validationErrors?.password}
+    placeholder="e.g. ********"
+    label="Password"
+    onChange={changeMethods.onParametersChange}
+  />
+);
+const displayField = ({
+  required,
+  changeMethods,
+  getValidation,
+  validationErrors,
+}: FieldPropTypes) => (
+  <ValidatedInput
+    id="database_name"
+    name="database_name"
+    required={required}
+    validationMethods={{ onBlur: getValidation }}
+    errorMessage={validationErrors?.database_name}
+    placeholder=""
+    label="Display Name"
+    onChange={changeMethods.onChange}
+    helpText="Pick a nickname for this database to display as in Superset."
+  />
+);
 
 const FORM_FIELD_MAP = {
-  host: {
+  host: hostField,
-    description: 'Host',
+  port: portField,
-    type: 'text',
+  database: databaseField,
-    className: 'w-50',
+  username: usernameField,
-    placeholder: 'e.g. 127.0.0.1',
+  password: passwordField,
-    changeMethod: CHANGE_METHOD.onPropertiesChange,
+  database_name: displayField,
-  },
-  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,
-  },
 };
 
 const DatabaseConnectionForm = ({
   dbModel: { name, parameters },
   onParametersChange,
   onChange,
+  validationErrors,
+  getValidation,
 }: {
   dbModel: DatabaseForm;
   onParametersChange: (
@@ -99,6 +175,8 @@ const DatabaseConnectionForm = ({
   onChange: (
     event: FormEvent<InputProps> | { target: HTMLInputElement },
   ) => void;
+  validationErrors: JsonObject | null;
+  getValidation: () => void;
 }) => (
   <>
     <StyledFormHeader>
@@ -107,52 +185,30 @@ const DatabaseConnectionForm = ({
         Need help? Learn more about connecting to {name}.
       </p>
     </StyledFormHeader>
-    <div css={formScrollableStyles}>
+    <div
+      // @ts-ignore
+      css={(theme: SupersetTheme) => [
+        formScrollableStyles,
+        validatedFormStyles(theme),
+      ]}
+    >
       {parameters &&
         FormFieldOrder.filter(
           (key: string) =>
             Object.keys(parameters.properties).includes(key) ||
             key === 'database_name',
-        ).map(field => {
+        ).map(field =>
-          const {
+          FORM_FIELD_MAP[field]({
-            className,
+            required: parameters.required.includes(field),
-            description,
+            changeMethods: { onParametersChange, onChange },
-            type,
+            validationErrors,
-            placeholder,
+            getValidation,
-            label,
+            key: field,
-            changeMethod,
+          }),
-          } = FORM_FIELD_MAP[field];
+        )}
-          const onEdit =
-            changeMethod === CHANGE_METHOD.onChange
-              ? onChange
-              : onParametersChange;
-          return (
-            <FormItem
-              className={cx(className, `form-group-${className}`)}
-              key={field}
-            >
-              <FormLabel
-                htmlFor={field}
-                required={parameters.required.includes(field)}
-              >
-                {description}
-              </FormLabel>
-              <Input
-                name={field}
-                type={type}
-                id={field}
-                autoComplete="off"
-                placeholder={placeholder}
-                onChange={onEdit}
-              />
-              <p className="helper">{label}</p>
-            </FormItem>
-          );
-        })}
     </div>
   </>
 );
-
 export const FormFieldMap = FORM_FIELD_MAP;
 
 export default DatabaseConnectionForm;

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<DatabaseObject> | 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.engine,
-          engine: trimmedState.parameters?.engine,
-        },
         configuration_method: trimmedState.configuration_method,
         ...action.payload,
       };
@@ -196,13 +192,16 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
   >(dbReducer, null);
   const [tabKey, setTabKey] = useState<string>(DEFAULT_TAB_KEY);
   const [availableDbs, getAvailableDbs] = useAvailableDatabases();
+  const [validationErrors, getValidation] = useDatabaseValidation();
   const [hasConnectedDb, setHasConnectedDb] = useState<boolean>(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<DatabaseModalProps> = ({
         // don't pass parameters if using the sqlalchemy uri
         delete update.parameters;
       }
-      updateResource(db.id as number, update as DatabaseObject).then(result => {
+      const result = await updateResource(
-        if (result) {
+        db.id as number,
-          if (onDatabaseAdd) {
+        update as DatabaseObject,
-            onDatabaseAdd();
+      );
-          }
+      if (result) {
-          onClose();
+        if (onDatabaseAdd) {
+          onDatabaseAdd();
         }
-      });
+        onClose();
+      }
     } else if (db) {
       // Create
       const dbId = await createResource(update as DatabaseObject);
@@ -300,7 +301,6 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
       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<DatabaseModalProps> = ({
         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<DatabaseModalProps> = ({
   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<DatabaseModalProps> = ({
       }
     >
       {isEditMode ? (
-        <EditHeader>
+        <TabHeader>
           <EditHeaderTitle>{db?.backend}</EditHeaderTitle>
-          <EditHeaderSubtitle>{db?.database_name}</EditHeaderSubtitle>
+          <EditHeaderSubtitle>{dbName}</EditHeaderSubtitle>
-        </EditHeader>
+        </TabHeader>
       ) : (
-        <CreateHeader>
+        <TabHeader>
           <CreateHeaderTitle>Enter Primary Credentials</CreateHeaderTitle>
           <CreateHeaderSubtitle>
             Need help? Learn how to connect your database{' '}
@@ -380,7 +383,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
             </a>
             .
           </CreateHeaderSubtitle>
-        </CreateHeader>
+        </TabHeader>
       )}
       <hr />
       <Tabs
@@ -512,6 +515,8 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
                 value: target.value,
               })
             }
+            getValidation={() => getValidation(db)}
+            validationErrors={validationErrors}
           />
           <Button
             buttonStyle="link"

superset-frontend/src/views/CRUD/data/database/DatabaseModal/styles.ts

@@ -163,13 +163,6 @@ export const formStyles = (theme: SupersetTheme) => css`
         margin-left: ${theme.gridUnit * 8}px;
       }
     }
-    .text-danger {
-      color: ${theme.colors.error.base};
-      font-size: ${theme.typography.sizes.s - 1}px;
-      strong {
-        font-weight: normal;
-      }
-    }
   }
   .control-label {
     color: ${theme.colors.grayscale.dark1};
@@ -180,16 +173,20 @@ export const formStyles = (theme: SupersetTheme) => css`
     font-size: ${theme.typography.sizes.s - 1}px;
     margin-top: ${theme.gridUnit * 1.5}px;
   }
-  .ant-modal-body {
-    padding-top: 0;
-    margin-bottom: 0;
-  }
   .ant-tabs-content-holder {
     overflow: auto;
     max-height: 475px;
   }
 `;
 
+export const validatedFormStyles = (theme: SupersetTheme) => css`
+  label {
+    color: ${theme.colors.grayscale.dark1};
+    font-size: ${theme.typography.sizes.s - 1}px;
+    margin-bottom: 0;
+  }
+`;
+
 export const StyledInputContainer = styled.div`
   margin-bottom: ${({ theme }) => theme.gridUnit * 6}px;
   &.mb-0 {
@@ -309,23 +306,13 @@ export const buttonLinkStyles = css`
   text-transform: initial;
 `;
 
-export const EditHeader = styled.div`
+export const TabHeader = styled.div`
-  display: flex;
-  flex-direction: column;
-  justify-content: center;
-  padding: 0px;
-  margin: ${({ theme }) => theme.gridUnit * 4}px
-    ${({ theme }) => theme.gridUnit * 4}px
-    ${({ theme }) => theme.gridUnit * 9}px;
-`;
-
-export const CreateHeader = styled.div`
   display: flex;
   flex-direction: column;
   justify-content: center;
   padding: 0px;
   margin: 0 ${({ theme }) => theme.gridUnit * 4}px
-    ${({ theme }) => theme.gridUnit * 6}px;
+    ${({ theme }) => theme.gridUnit * 8}px;
 `;
 
 export const CreateHeaderTitle = styled.div`
@@ -342,12 +329,12 @@ export const CreateHeaderSubtitle = styled.div`
 
 export const EditHeaderTitle = styled.div`
   color: ${({ theme }) => theme.colors.grayscale.light1};
-  font-size: ${({ theme }) => theme.typography.sizes.s}px;
+  font-size: ${({ theme }) => theme.typography.sizes.s - 1}px;
   text-transform: uppercase;
 `;
 
 export const EditHeaderSubtitle = styled.div`
   color: ${({ theme }) => theme.colors.grayscale.dark1};
-  font-size: ${({ theme }) => theme.typography.sizes.xl}px;
+  font-size: ${({ theme }) => theme.typography.sizes.l}px;
   font-weight: bold;
 `;

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

@@ -30,8 +30,9 @@ export type DatabaseObject = {
   created_by?: null | DatabaseUser;
   changed_on_delta_humanized?: string;
   changed_on?: string;
-  parameters?: { database_name?: string; engine?: string };
+  parameters?: { database_name?: string };
   configuration_method: CONFIGURATION_METHOD;
+  engine?: string;
 
   // Performance
   cache_timeout?: string;

superset-frontend/src/views/CRUD/hooks.ts

@@ -657,3 +657,60 @@ export function useAvailableDatabases() {
 
   return [availableDbs, getAvailable] as const;
 }
+
+export function useDatabaseValidation() {
+  const [validationErrors, setValidationErrors] = useState<JsonObject | null>(
+    null,
+  );
+  const getValidation = useCallback(
+    (database: Partial<DatabaseObject> | null) => {
+      SupersetClient.post({
+        endpoint: '/api/v1/database/validate_parameters',
+        body: JSON.stringify(database),
+        headers: { 'Content-Type': 'application/json' },
+      })
+        .then(() => {
+          setValidationErrors(null);
+        })
+        .catch(e => {
+          if (typeof e.json === 'function') {
+            e.json().then(({ errors = [] }: JsonObject) => {
+              const parsedErrors = errors
+                .filter(
+                  (error: { error_type: string }) =>
+                    error.error_type !== 'CONNECTION_MISSING_PARAMETERS_ERROR',
+                )
+                .reduce(
+                  (
+                    obj: {},
+                    {
+                      extra,
+                      message,
+                    }: {
+                      extra: { invalid?: string[] };
+                      message: string;
+                    },
+                  ) => {
+                    // if extra.invalid doesn't exist then the
+                    // error can't be mapped to a parameter
+                    // so leave it alone
+                    if (extra.invalid) {
+                      return { ...obj, [extra.invalid[0]]: message };
+                    }
+                    return obj;
+                  },
+                  {},
+                );
+              setValidationErrors(parsedErrors);
+            });
+          } else {
+            // eslint-disable-next-line no-console
+            console.error(e);
+          }
+        });
+    },
+    [setValidationErrors],
+  );
+
+  return [validationErrors, getValidation] as const;
+}

superset/databases/commands/validate.py

@@ -78,7 +78,9 @@ class ValidateDatabaseParametersCommand(BaseCommand):
             )
 
         # perform initial validation
-        errors = engine_spec.validate_parameters(self._properties["parameters"])
+        errors = engine_spec.validate_parameters(
+            self._properties.get("parameters", None)
+        )
         if errors:
             raise InvalidParametersError(errors)
 
@@ -90,7 +92,7 @@ class ValidateDatabaseParametersCommand(BaseCommand):
 
         # try to connect
         sqlalchemy_uri = engine_spec.build_sqlalchemy_uri(
-            self._properties["parameters"],  # type: ignore
+            self._properties.get("parameters", None),  # type: ignore
             encrypted_extra,
         )
         if self._model and sqlalchemy_uri == self._model.safe_sqlalchemy_uri():

superset/databases/schemas.py

@@ -326,6 +326,12 @@ class DatabaseValidateParametersSchema(Schema):
         allow_none=True,
         validate=server_cert_validator,
     )
+    configuration_method = EnumField(
+        ConfigurationMethod,
+        by_value=True,
+        allow_none=True,
+        description=configuration_method_description,
+    )
 
 
 class DatabasePostSchema(Schema, DatabaseParametersSchemaMixin):

superset/db_engine_specs/base.py

@@ -1402,7 +1402,7 @@ class BasicParametersMixin:
         errors: List[SupersetError] = []
 
         required = {"host", "port", "username", "database"}
-        present = {key for key in parameters if parameters[key]}  # type: ignore
+        present = {key for key in parameters if parameters.get(key, ())}  # type: ignore
         missing = sorted(required - present)
 
         if missing:
@@ -1415,7 +1415,7 @@ class BasicParametersMixin:
                 ),
             )
 
-        host = parameters["host"]
+        host = parameters.get("host", None)
         if not host:
             return errors
         if not is_hostname_valid(host):
@@ -1429,7 +1429,7 @@ class BasicParametersMixin:
             )
             return errors
 
-        port = parameters["port"]
+        port = parameters.get("port", None)
         if not port:
             return errors
         if not is_port_open(host, port):