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 e1b23358ca87b..4f5a0cc0775f1 100644 --- a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx +++ b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx @@ -74,6 +74,24 @@ import { } from './styles'; import ModalHeader, { DOCUMENTATION_LINK } from './ModalHeader'; +const errorAlertMapping = { + CONNECTION_MISSING_PARAMETERS_ERROR: { + message: 'Missing Required Fields', + description: 'Please complete all required fields.', + }, + CONNECTION_INVALID_HOSTNAME_ERROR: { + message: 'Could not verify the host', + description: + 'The host is invalid. Please verify that this field is entered correctly.', + }, + CONNECTION_PORT_CLOSED_ERROR: { + message: 'Port is closed', + description: 'Please verify that port is open to connect.', + }, + CONNECTION_INVALID_PORT_ERROR: { + message: 'The port must be a whole number less than or equal to 65535.', + }, +}; interface DatabaseModalProps { addDangerToast: (msg: string) => void; addSuccessToast: (msg: string) => void; @@ -316,7 +334,7 @@ const DatabaseModal: FunctionComponent = ({ // Database fetch logic const { - state: { loading: dbLoading, resource: dbFetched, error: dbError }, + state: { loading: dbLoading, resource: dbFetched, error: dbErrors }, fetchResource, createResource, updateResource, @@ -326,12 +344,15 @@ const DatabaseModal: FunctionComponent = ({ addDangerToast, ); + const showDBError = validationErrors || dbErrors; + const isEmpty = (data?: Object | null) => + data && Object.keys(data).length === 0; + const dbModel: DatabaseForm = availableDbs?.databases?.find( (available: { engine: string | undefined }) => // TODO: we need a centralized engine in one place - available.engine === - (isEditMode || editNewDb ? db?.backend || db?.engine : db?.engine), + available.engine === (isEditMode ? db?.backend : db?.engine), ) || {}; // Test Connection logic @@ -370,7 +391,7 @@ const DatabaseModal: FunctionComponent = ({ // Validate DB before saving await getValidation(dbToUpdate, true); - if (validationErrors) { + if (validationErrors && !isEmpty(validationErrors)) { return; } @@ -416,6 +437,7 @@ const DatabaseModal: FunctionComponent = ({ const result = await updateResource( db.id as number, dbToUpdate as DatabaseObject, + true, ); if (result) { if (onDatabaseAdd) { @@ -443,7 +465,7 @@ const DatabaseModal: FunctionComponent = ({ }); } setLoading(true); - const dbId = await createResource(dbToUpdate as DatabaseObject); + const dbId = await createResource(dbToUpdate as DatabaseObject, true); if (dbId) { setHasConnectedDb(true); if (onDatabaseAdd) { @@ -651,15 +673,44 @@ const DatabaseModal: FunctionComponent = ({ setTabKey(key); }; - const errorAlert = () => ( - antDErrorAlertStyles(theme)} - message="Missing Required Fields" - description="Please complete all required fields." - showIcon - /> - ); + const errorAlert = () => { + if ( + isEmpty(dbErrors) || + (isEmpty(validationErrors) && + !(validationErrors?.error_type in errorAlertMapping)) + ) { + return <>; + } + + if (validationErrors) { + return ( + antDErrorAlertStyles(theme)} + message={ + errorAlertMapping[validationErrors?.error_type]?.message || + validationErrors?.error_type + } + description={ + errorAlertMapping[validationErrors?.error_type]?.description || + JSON.stringify(validationErrors) + } + showIcon + closable={false} + /> + ); + } + + const message: Array = Object.values(dbErrors); + return ( + antDErrorAlertStyles(theme)} + message="Database Creation Error" + description={message[0]} + /> + ); + }; const renderFinishState = () => { if (!editNewDb) { @@ -719,7 +770,6 @@ const DatabaseModal: FunctionComponent = ({ } getValidation={() => getValidation(db)} validationErrors={validationErrors} - editNewDb={editNewDb} /> ); }; @@ -897,7 +947,7 @@ const DatabaseModal: FunctionComponent = ({ onChange(ActionType.extraEditorChange, payload); }} /> - {dbError && errorAlert()} + {showDBError && errorAlert()} @@ -1023,7 +1073,7 @@ const DatabaseModal: FunctionComponent = ({ /> {/* Step 2 */} - {dbError && errorAlert()} + {showDBError && errorAlert()} ))} diff --git a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/styles.ts b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/styles.ts index 8bb958badfeac..cd42f108e0dc0 100644 --- a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/styles.ts +++ b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/styles.ts @@ -181,7 +181,7 @@ export const antDAlertStyles = (theme: SupersetTheme) => css` `; export const antDErrorAlertStyles = (theme: SupersetTheme) => css` - border: ${theme.colors.error.base} 2px solid; + border: ${theme.colors.error.base} 1px solid; padding: ${theme.gridUnit * 4}px; margin: ${theme.gridUnit * 8}px ${theme.gridUnit * 4}px; color: ${theme.colors.error.dark2}; diff --git a/superset-frontend/src/views/CRUD/hooks.ts b/superset-frontend/src/views/CRUD/hooks.ts index 207dce68b62d1..0d216ed3df061 100644 --- a/superset-frontend/src/views/CRUD/hooks.ts +++ b/superset-frontend/src/views/CRUD/hooks.ts @@ -211,7 +211,7 @@ export function useListViewResource( interface SingleViewResourceState { loading: boolean; resource: D | null; - error: string | Record | null; + error: any | null; } export function useSingleViewResource( @@ -269,7 +269,7 @@ export function useSingleViewResource( ); const createResource = useCallback( - (resource: D) => { + (resource: D, hideToast = false) => { // Set loading state updateState({ loading: true, @@ -289,13 +289,16 @@ export function useSingleViewResource( return json.id; }, createErrorHandler((errMsg: Record) => { - handleErrorMsg( - t( - 'An error occurred while creating %ss: %s', - resourceLabel, - parsedErrorMessage(errMsg), - ), - ); + // we did not want toasts for db-connection-ui but did not want to disable it everywhere + if (!hideToast) { + handleErrorMsg( + t( + 'An error occurred while creating %ss: %s', + resourceLabel, + parsedErrorMessage(errMsg), + ), + ); + } updateState({ error: errMsg, @@ -310,7 +313,7 @@ export function useSingleViewResource( ); const updateResource = useCallback( - (resourceID: number, resource: D) => { + (resourceID: number, resource: D, hideToast = false) => { // Set loading state updateState({ loading: true, @@ -330,13 +333,15 @@ export function useSingleViewResource( return json.result; }, createErrorHandler(errMsg => { - handleErrorMsg( - t( - 'An error occurred while fetching %ss: %s', - resourceLabel, - JSON.stringify(errMsg), - ), - ); + if (!hideToast) { + handleErrorMsg( + t( + 'An error occurred while fetching %ss: %s', + resourceLabel, + JSON.stringify(errMsg), + ), + ); + } updateState({ error: errMsg, @@ -651,20 +656,20 @@ export function useDatabaseValidation() { if (typeof e.json === 'function') { e.json().then(({ errors = [] }: JsonObject) => { const parsedErrors = errors - .filter((error: { error_type: string }) => { - const skipValidationError = ![ - 'CONNECTION_MISSING_PARAMETERS_ERROR', - 'CONNECTION_ACCESS_DENIED_ERROR', - ].includes(error.error_type); - return skipValidationError || onCreate; - }) + .filter( + (error: { error_type: string }) => + error.error_type !== + 'CONNECTION_MISSING_PARAMETERS_ERROR' || onCreate, + ) .reduce( ( obj: {}, { + error_type, extra, message, }: { + error_type: string; extra: { invalid?: string[]; missing?: string[] }; message: string; }, @@ -673,11 +678,16 @@ export function useDatabaseValidation() { // error can't be mapped to a parameter // so leave it alone if (extra.invalid) { - return { ...obj, [extra.invalid[0]]: message }; + return { + ...obj, + [extra.invalid[0]]: message, + error_type, + }; } if (extra.missing) { return { ...obj, + error_type, ...Object.assign( {}, ...extra.missing.map(field => ({ diff --git a/superset/databases/commands/exceptions.py b/superset/databases/commands/exceptions.py index c8245ab07eafd..9cd8b3306f62e 100644 --- a/superset/databases/commands/exceptions.py +++ b/superset/databases/commands/exceptions.py @@ -39,7 +39,7 @@ class DatabaseExistsValidationError(ValidationError): def __init__(self) -> None: super().__init__( - _("A database with the same name already exists"), + _("A database with the same name already exists."), field_name="database_name", ) diff --git a/tests/databases/api_tests.py b/tests/databases/api_tests.py index 0dfbf322b0991..9247a527fccbc 100644 --- a/tests/databases/api_tests.py +++ b/tests/databases/api_tests.py @@ -420,7 +420,9 @@ def test_create_database_unique_validate(self): rv = self.client.post(uri, json=database_data) response = json.loads(rv.data.decode("utf-8")) expected_response = { - "message": {"database_name": "A database with the same name already exists"} + "message": { + "database_name": "A database with the same name already exists." + } } self.assertEqual(rv.status_code, 422) self.assertEqual(response, expected_response) @@ -566,7 +568,9 @@ def test_update_database_uniqueness(self): rv = self.client.put(uri, json=database_data) response = json.loads(rv.data.decode("utf-8")) expected_response = { - "message": {"database_name": "A database with the same name already exists"} + "message": { + "database_name": "A database with the same name already exists." + } } self.assertEqual(rv.status_code, 422) self.assertEqual(response, expected_response)