QT/superset2
1
0
Fork 0
mirror of https://github.com/apache/superset.git synced 2026-04-18 23:55:00 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(DatabaseModal): Improve database modal validation and fix visual Issues (#33826)

Browse Source
This commit is contained in:
Enzo Martellucci
2025-06-24 15:52:19 +02:00
committed by GitHub
parent 98b35125c2
commit 6db6db23f8
9 changed files with 251 additions and 234 deletions
Download Patch File Download Diff File Expand all files Collapse all files

74
superset-frontend/cypress-base/cypress/e2e/database/modal.test.ts
View File

@@ -32,53 +32,87 @@ describe('Add database', () => {
});
beforeEach(() => {
cy.intercept('POST', '**/api/v1/database/validate_parameters/**').as(
'validateParams',
);
cy.intercept('POST', '**/api/v1/database/').as('createDb');
closeModal();
cy.getBySel('btn-create-database').click();
});
it('should open dynamic form', () => {
// click postgres dynamic form
cy.get('.preferred > :nth-child(1)').click();
// make sure all the fields are rendering
cy.get('input[name="host"]').should('have.value', '');
cy.get('input[name="port"]').should('have.value', '');
cy.get('input[name="database"]').should('have.value', '');
cy.get('input[name="username"]').should('have.value', '');
cy.get('input[name="password"]').should('have.value', '');
cy.get('input[name="database_name"]').should('have.value', '');
});
it('should open sqlalchemy form', () => {
// click postgres dynamic form
cy.get('.preferred > :nth-child(1)').click();
cy.getBySel('sqla-connect-btn').click();
// check if the sqlalchemy form is showing up
cy.getBySel('database-name-input').should('be.visible');
cy.getBySel('sqlalchemy-uri-input').should('be.visible');
});
it('show error alerts on dynamic form for bad host', () => {
// click postgres dynamic form
cy.get('.preferred > :nth-child(1)').click();
cy.get('input[name="host"]').focus();
cy.focused().type('badhost', { force: true });
cy.get('input[name="port"]').focus();
cy.focused().type('5432', { force: true });
cy.get('.ant-form-item-explain-error').contains(
"The hostname provided can't be resolved",
);
cy.get('input[name="host"]').type('badhost', { force: true });
cy.get('input[name="port"]').type('5432', { force: true });
cy.get('input[name="username"]').type('testusername', { force: true });
cy.get('input[name="database"]').type('testdb', { force: true });
cy.get('input[name="password"]').type('testpass', { force: true });
cy.get('body').click(0, 0);
cy.wait('@validateParams', { timeout: 30000 });
cy.getBySel('btn-submit-connection').should('not.be.disabled');
cy.getBySel('btn-submit-connection').click({ force: true });
cy.wait('@validateParams', { timeout: 30000 }).then(() => {
cy.wait('@createDb', { timeout: 60000 }).then(() => {
cy.contains(
'.ant-form-item-explain-error',
"The hostname provided can't be resolved",
).should('exist');
});
});
});
it('show error alerts on dynamic form for bad port', () => {
// click postgres dynamic form
cy.get('.preferred > :nth-child(1)').click();
cy.get('input[name="host"]').focus();
cy.focused().type('localhost', { force: true });
cy.get('input[name="port"]').focus();
cy.focused().type('123', { force: true });
cy.get('input[name="database"]').focus();
cy.get('.ant-form-item-explain-error').contains('The port is closed');
cy.get('input[name="host"]').type('localhost', { force: true });
cy.get('body').click(0, 0);
cy.wait('@validateParams', { timeout: 30000 });
cy.get('input[name="port"]').type('5430', { force: true });
cy.get('input[name="database"]').type('testdb', { force: true });
cy.get('input[name="username"]').type('testusername', { force: true });
cy.wait('@validateParams', { timeout: 30000 });
cy.get('input[name="password"]').type('testpass', { force: true });
cy.wait('@validateParams');
cy.getBySel('btn-submit-connection').should('not.be.disabled');
cy.getBySel('btn-submit-connection').click({ force: true });
cy.wait('@validateParams', { timeout: 30000 }).then(() => {
cy.get('body').click(0, 0);
cy.getBySel('btn-submit-connection').click({ force: true });
cy.wait('@createDb', { timeout: 60000 }).then(() => {
cy.contains(
'.ant-form-item-explain-error',
'The port is closed',
).should('exist');
});
});
});
});

152
superset-frontend/packages/superset-ui-core/src/components/Form/LabeledErrorBoundInput.tsx
View File

@@ -16,9 +16,8 @@
* specific language governing permissions and limitations
* under the License.
*/
import { styled, css, SupersetTheme, t } from '../..';
import { error as errorIcon } from '../assets/svgs';
import { Button, Icons, InfoTooltip, Tooltip } from '..';
import { styled, t } from '@superset-ui/core';
import { Button, Icons, InfoTooltip, Tooltip, Flex } from '..';
import { Input } from '../Input';
import { FormLabel } from './FormLabel';
import { FormItem } from './FormItem';
@@ -32,28 +31,6 @@ const StyledInputPassword = styled(Input.Password)`
margin: ${({ theme }) => `${theme.sizeUnit}px 0 ${theme.sizeUnit * 2}px`};
`;
const alertIconStyles = (theme: SupersetTheme, hasError: boolean) => css`
.ant-form-item-children-icon {
display: none;
}
${hasError &&
`.ant-form-item-control-input-content {
position: relative;
&:after {
content: ' ';
display: inline-block;
background: ${theme.colorError};
mask: url(${errorIcon});
mask-size: cover;
width: ${theme.sizeUnit * 4}px;
height: ${theme.sizeUnit * 4}px;
position: absolute;
right: ${theme.sizeUnit * 1.25}px;
top: ${theme.sizeUnit * 2.75}px;
}
}`}
`;
const StyledFormGroup = styled('div')`
input::-webkit-outer-spin-button,
input::-webkit-inner-spin-button {
@@ -66,21 +43,10 @@ const StyledFormGroup = styled('div')`
}
`;
const StyledAlignment = styled.div`
display: flex;
align-items: center;
`;
const StyledFormLabel = styled(FormLabel)`
margin-bottom: 0;
`;
const iconReset = css`
&.anticon > * {
line-height: 0;
}
`;
export const LabeledErrorBoundInput = ({
label,
validationMethods,
@@ -94,60 +60,62 @@ export const LabeledErrorBoundInput = ({
visibilityToggle,
get_url,
description,
isValidating = false,
...props
}: LabeledErrorBoundInputProps) => (
<StyledFormGroup className={className}>
<StyledAlignment>
<StyledFormLabel htmlFor={id} required={required}>
{label}
</StyledFormLabel>
{hasTooltip && <InfoTooltip tooltip={`${tooltipText}`} />}
</StyledAlignment>
<FormItem
css={(theme: SupersetTheme) => alertIconStyles(theme, !!errorMessage)}
validateTrigger={Object.keys(validationMethods)}
validateStatus={errorMessage ? 'error' : 'success'}
help={errorMessage || helpText}
hasFeedback={!!errorMessage}
>
{visibilityToggle || props.name === 'password' ? (
<StyledInputPassword
{...props}
{...validationMethods}
iconRender={visible =>
visible ? (
<Tooltip title={t('Hide password.')}>
<Icons.EyeInvisibleOutlined iconSize="m" css={iconReset} />
</Tooltip>
) : (
<Tooltip title={t('Show password.')}>
<Icons.EyeOutlined
iconSize="m"
css={iconReset}
data-test="icon-eye"
/>
</Tooltip>
)
}
role="textbox"
/>
) : (
<StyledInput {...props} {...validationMethods} />
)}
{get_url && description ? (
<Button
htmlType="button"
buttonStyle="secondary"
onClick={() => {
window.open(get_url);
return true;
}}
>
Get {description}
</Button>
) : (
<br />
)}
</FormItem>
</StyledFormGroup>
);
}: LabeledErrorBoundInputProps) => {
const hasError = !!errorMessage;
return (
<StyledFormGroup className={className}>
<Flex align="center">
<StyledFormLabel htmlFor={id} required={required}>
{label}
</StyledFormLabel>
{hasTooltip && <InfoTooltip tooltip={`${tooltipText}`} />}
</Flex>
<FormItem
validateTrigger={Object.keys(validationMethods)}
validateStatus={
isValidating ? 'validating' : hasError ? 'error' : 'success'
}
help={errorMessage || helpText}
hasFeedback={!!hasError}
>
{visibilityToggle || props.name === 'password' ? (
<StyledInputPassword
{...props}
{...validationMethods}
iconRender={visible =>
visible ? (
<Tooltip title={t('Hide password.')}>
<Icons.EyeInvisibleOutlined iconSize="m" />
</Tooltip>
) : (
<Tooltip title={t('Show password.')}>
<Icons.EyeOutlined iconSize="m" data-test="icon-eye" />
</Tooltip>
)
}
role="textbox"
/>
) : (
<StyledInput {...props} {...validationMethods} />
)}
{get_url && description ? (
<Button
type="link"
htmlType="button"
onClick={() => {
window.open(get_url);
return true;
}}
>
Get {description}
</Button>
) : (
<br />
)}
</FormItem>
</StyledFormGroup>
);
};
export default LabeledErrorBoundInput;

24
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/CommonParameters.tsx
View File

@@ -31,8 +31,10 @@ export const hostField = ({
getValidation,
validationErrors,
db,
isValidating,
}: FieldPropTypes) => (
<ValidatedInput
isValidating={isValidating}
id="host"
name="host"
value={db?.parameters?.host}
@@ -56,12 +58,14 @@ export const portField = ({
getValidation,
validationErrors,
db,
isValidating,
}: FieldPropTypes) => (
<>
<ValidatedInput
id="port"
name="port"
type="number"
isValidating={isValidating}
required={required}
value={db?.parameters?.port as number}
validationMethods={{ onBlur: getValidation }}
@@ -79,10 +83,12 @@ export const httpPath = ({
getValidation,
validationErrors,
db,
isValidating,
}: FieldPropTypes) => {
const extraJson = JSON.parse(db?.extra || '{}');
return (
<ValidatedInput
isValidating={isValidating}
id="http_path"
name="http_path"
required={required}
@@ -103,8 +109,10 @@ export const databaseField = ({
validationErrors,
placeholder,
db,
isValidating,
}: FieldPropTypes) => (
<ValidatedInput
isValidating={isValidating}
id="database"
name="database"
required={required}
@@ -123,8 +131,10 @@ export const defaultCatalogField = ({
getValidation,
validationErrors,
db,
isValidating,
}: FieldPropTypes) => (
<ValidatedInput
isValidating={isValidating}
id="default_catalog"
name="default_catalog"
required={required}
@@ -143,11 +153,13 @@ export const defaultSchemaField = ({
getValidation,
validationErrors,
db,
isValidating,
}: FieldPropTypes) => (
<ValidatedInput
id="default_schema"
name="default_schema"
required={required}
isValidating={isValidating}
value={db?.parameters?.default_schema}
validationMethods={{ onBlur: getValidation }}
errorMessage={validationErrors?.default_schema}
@@ -163,11 +175,13 @@ export const httpPathField = ({
getValidation,
validationErrors,
db,
isValidating,
}: FieldPropTypes) => (
<ValidatedInput
id="http_path_field"
name="http_path_field"
required={required}
isValidating={isValidating}
value={db?.parameters?.http_path_field}
validationMethods={{ onBlur: getValidation }}
errorMessage={validationErrors?.http_path}
@@ -183,11 +197,13 @@ export const usernameField = ({
getValidation,
validationErrors,
db,
isValidating,
}: FieldPropTypes) => (
<ValidatedInput
id="username"
name="username"
required={required}
isValidating={isValidating}
value={db?.parameters?.username}
validationMethods={{ onBlur: getValidation }}
errorMessage={validationErrors?.username}
@@ -203,11 +219,13 @@ export const passwordField = ({
validationErrors,
db,
isEditMode,
isValidating,
}: FieldPropTypes) => (
<ValidatedInput
id="password"
name="password"
required={required}
isValidating={isValidating}
visibilityToggle={!isEditMode}
value={db?.parameters?.password}
validationMethods={{ onBlur: getValidation }}
@@ -251,12 +269,14 @@ export const displayField = ({
getValidation,
validationErrors,
db,
isValidating,
}: FieldPropTypes) => (
<>
<ValidatedInput
id="database_name"
name="database_name"
required
isValidating={isValidating}
value={db?.database_name}
validationMethods={{ onBlur: getValidation }}
errorMessage={validationErrors?.database_name}
@@ -276,11 +296,13 @@ export const queryField = ({
getValidation,
validationErrors,
db,
isValidating,
}: FieldPropTypes) => (
<ValidatedInput
id="query_input"
name="query_input"
required={required}
isValidating={isValidating}
value={db?.query_input || ''}
validationMethods={{ onBlur: getValidation }}
errorMessage={validationErrors?.query}
@@ -325,12 +347,14 @@ export const projectIdfield = ({
getValidation,
validationErrors,
db,
isValidating,
}: FieldPropTypes) => (
<>
<ValidatedInput
id="project_id"
name="project_id"
required
isValidating={isValidating}
value={db?.parameters?.project_id}
validationMethods={{ onBlur: getValidation }}
errorMessage={validationErrors?.project_id}

1
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.test.tsx
View File

@@ -43,6 +43,7 @@ describe('OAuth2ClientField', () => {
getValidation: jest.fn(),
clearValidationErrors: jest.fn(),
field: 'test',
isValidating: false,
db: {
configuration_method: 'dynamic_form',
database_name: 'test',

2
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/index.tsx
View File

@@ -40,6 +40,7 @@ const DatabaseConnectionForm = ({
sslForced,
validationErrors,
clearValidationErrors,
isValidating,
}: DatabaseConnectionFormProps) => {
const parameters = dbModel?.parameters as {
properties: {
@@ -90,6 +91,7 @@ const DatabaseConnectionForm = ({
isEditMode,
sslForced,
editNewDb,
isValidating,
placeholder: getPlaceholder ? getPlaceholder(field) : undefined,
}),
)}

7
superset-frontend/src/features/databases/DatabaseModal/index.test.tsx
View File

@@ -1353,12 +1353,16 @@ describe('DatabaseModal', () => {
expect(usernameField).toHaveValue('');
expect(passwordField).toHaveValue('');
expect(connectButton).toBeDisabled();
userEvent.type(hostField, 'localhost');
userEvent.type(portField, '5432');
userEvent.type(databaseNameField, 'postgres');
userEvent.type(usernameField, 'testdb');
userEvent.type(passwordField, 'demoPassword');
await waitFor(() => expect(connectButton).toBeEnabled());
expect(await screen.findByDisplayValue(/5432/i)).toBeInTheDocument();
expect(hostField).toHaveValue('localhost');
expect(portField).toHaveValue(5432);
@@ -1366,9 +1370,10 @@ describe('DatabaseModal', () => {
expect(usernameField).toHaveValue('testdb');
expect(passwordField).toHaveValue('demoPassword');
expect(connectButton).toBeEnabled();
userEvent.click(connectButton);
await waitFor(() => {
expect(fetchMock.calls(VALIDATE_PARAMS_ENDPOINT).length).toEqual(6);
expect(fetchMock.calls(VALIDATE_PARAMS_ENDPOINT).length).toEqual(5);
});
});
});

28
superset-frontend/src/features/databases/DatabaseModal/index.tsx
View File

@@ -594,8 +594,14 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
const [tabKey, setTabKey] = useState<string>(DEFAULT_TAB_KEY);
const [availableDbs, getAvailableDbs] = useAvailableDatabases();
const [validationErrors, getValidation, setValidationErrors] =
useDatabaseValidation();
const [
validationErrors,
getValidation,
setValidationErrors,
isValidating,
hasValidated,
setHasValidated,
] = useDatabaseValidation();
const [hasConnectedDb, setHasConnectedDb] = useState<boolean>(false);
const [showCTAbtns, setShowCTAbtns] = useState(false);
const [dbName, setDbName] = useState('');
@@ -754,7 +760,8 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
const handleClearValidationErrors = useCallback(() => {
setValidationErrors(null);
}, [setValidationErrors]);
setHasValidated(false);
}, [setValidationErrors, setHasValidated]);
const handleParametersChange = useCallback(
({ target }: { target: HTMLInputElement }) => {
@@ -812,7 +819,6 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
const onSave = async () => {
let dbConfigExtraExtensionOnSaveError;
setLoading(true);
dbConfigExtraExtension
@@ -1208,10 +1214,18 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
{t('Back')}
</StyledFooterButton>
<StyledFooterButton
data-test="btn-submit-connection"
key="submit"
buttonStyle="primary"
onClick={onSave}
loading={isLoading}
disabled={
!!(
!hasValidated ||
isValidating ||
(validationErrors && Object.keys(validationErrors).length > 0)
)
}
>
{t('Connect')}
</StyledFooterButton>
@@ -1452,6 +1466,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
onChange={(event: ChangeEvent<HTMLInputElement>) =>
setPasswords({ ...passwords, [database]: event.target.value })
}
isValidating={isValidating}
validationMethods={{ onBlur: () => {} }}
errorMessage={validationErrors?.password_needed}
label={t('%s PASSWORD', database.slice(10))}
@@ -1460,6 +1475,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
)}
{sshTunnelPasswordFields?.indexOf(database) >= 0 && (
<ValidatedInput
isValidating={isValidating}
id="ssh_tunnel_password_needed"
name="ssh_tunnel_password_needed"
required
@@ -1480,6 +1496,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
<ValidatedInput
id="ssh_tunnel_private_key_needed"
name="ssh_tunnel_private_key_needed"
isValidating={isValidating}
required
value={sshTunnelPrivateKeys[database]}
onChange={(event: ChangeEvent<HTMLInputElement>) =>
@@ -1498,6 +1515,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
<ValidatedInput
id="ssh_tunnel_private_key_password_needed"
name="ssh_tunnel_private_key_password_needed"
isValidating={isValidating}
required
value={sshTunnelPrivateKeyPasswords[database]}
onChange={(event: ChangeEvent<HTMLInputElement>) =>
@@ -1553,6 +1571,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
<ValidatedInput
id="confirm_overwrite"
name="confirm_overwrite"
isValidating={isValidating}
required
validationMethods={{ onBlur: () => {} }}
errorMessage={validationErrors?.confirm_overwrite}
@@ -1688,6 +1707,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
const renderDatabaseConnectionForm = () => (
<>
<DatabaseConnectionForm
isValidating={isValidating}
isEditMode={isEditMode}
db={db as DatabaseObject}
sslForced={false}

2
superset-frontend/src/features/databases/types.ts
View File

@@ -321,6 +321,7 @@ export interface FieldPropTypes {
sslForced?: boolean;
defaultDBName?: string;
editNewDb?: boolean;
isValidating: boolean;
}
type ChangeMethodsType = FieldPropTypes['changeMethods'];
@@ -343,6 +344,7 @@ export interface DatabaseConnectionFormProps {
editNewDb?: boolean;
dbModel: DatabaseForm;
db: Partial<DatabaseObject> | null;
isValidating: boolean;
onParametersChange: (
event: FormEvent<InputProps> | { target: HTMLInputElement },
) => void;

195
superset-frontend/src/views/CRUD/hooks.ts
View File

@@ -760,137 +760,98 @@ export function useDatabaseValidation() {
const [validationErrors, setValidationErrors] = useState<JsonObject | null>(
null,
);
const [isValidating, setIsValidating] = useState(false);
const [hasValidated, setHasValidated] = useState(false);
const getValidation = useCallback(
(database: Partial<DatabaseObject> | null, onCreate = false) => {
async (database: Partial<DatabaseObject> | null, onCreate = false) => {
if (database?.parameters?.ssh) {
// TODO: /validate_parameters/ and related utils should support ssh tunnel
setValidationErrors(null);
return [];
setIsValidating(false);
setHasValidated(true);
return Promise.resolve([]);
}
return (
SupersetClient.post({
setIsValidating(true);
try {
await SupersetClient.post({
endpoint: '/api/v1/database/validate_parameters/',
body: JSON.stringify(transformDB(database)),
headers: { 'Content-Type': 'application/json' },
})
.then(() => {
setValidationErrors(null);
})
// eslint-disable-next-line consistent-return
.catch(e => {
if (typeof e.json === 'function') {
return 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;
})
.reduce(
(
obj: {},
{
error_type,
extra,
message,
}: {
error_type: string;
extra: {
invalid?: string[];
missing?: string[];
name: string;
catalog: {
name: string;
url: string;
idx: number;
};
issue_codes?: {
code?: number;
message?: string;
}[];
};
message: string;
},
) => {
if (extra.catalog) {
if (extra.catalog.name) {
return {
...obj,
error_type,
[extra.catalog.idx]: {
name: message,
},
};
}
if (extra.catalog.url) {
return {
...obj,
error_type,
[extra.catalog.idx]: {
url: message,
},
};
}
});
setValidationErrors(null);
setIsValidating(false);
setHasValidated(true);
return [];
} catch (error) {
if (typeof error.json === 'function') {
return error.json().then(({ errors = [] }) => {
const parsedErrors = errors
.filter((err: { error_type: string }) => {
const allowed = [
'CONNECTION_MISSING_PARAMETERS_ERROR',
'CONNECTION_ACCESS_DENIED_ERROR',
'INVALID_PAYLOAD_SCHEMA_ERROR',
];
return allowed.includes(err.error_type) || onCreate;
})
.reduce((acc: JsonObject, err_2: any) => {
const { message, extra } = err_2;
return {
...obj,
error_type,
[extra.catalog.idx]: {
name: message,
url: message,
},
};
}
// 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,
error_type,
};
}
if (extra.missing) {
return {
...obj,
error_type,
...Object.assign(
{},
...extra.missing.map(field => ({
[field]: 'This is a required field',
})),
),
};
}
if (extra.issue_codes?.length) {
return {
...obj,
error_type,
description: message || extra.issue_codes[0]?.message,
};
}
if (extra?.catalog) {
const { idx } = extra.catalog;
acc[idx] = {
...acc[idx],
...(extra.catalog.name ? { name: message } : {}),
...(extra.catalog.url ? { url: message } : {}),
};
return acc;
}
return obj;
},
{},
);
setValidationErrors(parsedErrors);
return parsedErrors;
});
}
// eslint-disable-next-line no-console
console.error(e);
})
);
if (extra?.invalid) {
extra.invalid.forEach((field: string) => {
acc[field] = message;
});
}
if (extra?.missing) {
extra.missing.forEach((field_1: string) => {
acc[field_1] = 'This is a required field';
});
}
if (extra?.issue_codes?.length) {
acc.description = message || extra.issue_codes[0]?.message;
}
return acc;
}, {});
setValidationErrors(parsedErrors);
setIsValidating(false);
setHasValidated(true);
return parsedErrors;
});
}
console.error('Unexpected error during validation:', error);
setIsValidating(false);
setHasValidated(true);
return {};
}
},
[setValidationErrors],
);
return [validationErrors, getValidation, setValidationErrors] as const;
return [
validationErrors,
getValidation,
setValidationErrors,
isValidating,
hasValidated,
setHasValidated,
] as const;
}
export const reportSelector = (