Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: SSH Tunnel creation with dynamic form #24196

Merged
merged 17 commits into from
Jul 3, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
* under the License.
*/
import React from 'react';
import { isEmpty } from 'lodash';
import { SupersetTheme, t } from '@superset-ui/core';
import { AntdSwitch } from 'src/components';
import InfoTooltip from 'src/components/InfoTooltip';
Expand Down Expand Up @@ -250,3 +251,34 @@ export const forceSSLField = ({
/>
</div>
);

export const SSHTunnelSwitch = ({
isEditMode,
changeMethods,
db,
}: FieldPropTypes) =>
true ? (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the ternary operator here if the condition is always true?

<div css={(theme: SupersetTheme) => infoTooltip(theme)}>
<AntdSwitch
disabled={isEditMode && !isEmpty(db?.ssh_tunnel)}
checked={db?.parameters?.ssh}
onChange={changed => {
changeMethods.onParametersChange({
target: {
type: 'toggle',
name: 'ssh',
checked: true,
value: changed,
},
});
}}
data-test="ssh-tunnel-switch"
/>
<span css={toggleStyle}>{t('SSH Tunnel')}</span>
<InfoTooltip
tooltip={t('SSH Tunnel configuration parameters')}
placement="right"
viewBox="0 -5 24 24"
/>
</div>
) : null;
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import {
portField,
queryField,
usernameField,
SSHTunnelSwitch,
} from './CommonParameters';
import { validatedInputField } from './ValidatedInputField';
import { EncryptedField } from './EncryptedField';
Expand All @@ -55,6 +56,7 @@ export const FormFieldOrder = [
'account',
'warehouse',
'role',
'ssh',
];

export interface FieldPropTypes {
Expand Down Expand Up @@ -102,6 +104,7 @@ const FORM_FIELD_MAP = {
warehouse: validatedInputField,
role: validatedInputField,
account: validatedInputField,
ssh: SSHTunnelSwitch,
};

interface DatabaseConnectionFormProps {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,10 @@ fetchMock.mock(AVAILABLE_DB_ENDPOINT, {
description: 'Additional parameters',
type: 'object',
},
ssh: {
description: 'Create SSH Tunnel',
type: 'boolean',
},
username: {
description: 'Username',
nullable: true,
Expand Down
31 changes: 12 additions & 19 deletions superset-frontend/src/features/databases/DatabaseModal/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -763,15 +763,19 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
});
}

// make sure that button spinner animates
setLoading(true);
const errors = await getValidation(dbToUpdate, true);
if ((validationErrors && !isEmpty(validationErrors)) || errors) {
// only do validation for non ssh tunnel connections
console.log(dbToUpdate);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leftover

if (!dbToUpdate?.ssh_tunnel) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hughhhh why can't we run the validation for SSH tunnel connections, assuming the user has provided that info?

I think the logic should be: if the user has enabled SSH, we don't perform any validation until SSH credentials are entered.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the initial implementation, we were okay with doing validation on just connect. Right now users are not able to connect at all via dynamic forms.

I'm open to this fix but that's going to take longer to implement vs. fixing the issue at hand.

Let me know what you think

// make sure that button spinner animates
setLoading(true);
const errors = await getValidation(dbToUpdate, true);
if ((validationErrors && !isEmpty(validationErrors)) || errors) {
setLoading(false);
return;
}
// end spinner animation
setLoading(false);
return;
}
setLoading(false);
// end spinner animation

const parameters_schema = isEditMode
? dbToUpdate.parameters_schema?.properties
Expand Down Expand Up @@ -1633,18 +1637,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
validationErrors={validationErrors}
getPlaceholder={getPlaceholder}
/>
<SSHTunnelContainer>
<SSHTunnelSwitchComponent
isEditMode={isEditMode}
dbFetched={dbFetched}
disableSSHTunnelingForEngine={disableSSHTunnelingForEngine}
useSSHTunneling={useSSHTunneling}
setUseSSHTunneling={setUseSSHTunneling}
setDB={setDB}
isSSHTunneling={isSSHTunneling}
/>
</SSHTunnelContainer>
{useSSHTunneling && (
{db?.parameters?.ssh && (
<SSHTunnelContainer>{renderSSHTunnelForm()}</SSHTunnelContainer>
)}
</>
Expand Down
1 change: 1 addition & 0 deletions superset-frontend/src/features/databases/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ export type DatabaseObject = {
warehouse?: string;
role?: string;
account?: string;
ssh?: boolean;
};

// Performance
Expand Down
1 change: 1 addition & 0 deletions superset-frontend/src/types/Database.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,5 @@ export default interface Database {
server_cert: string;
sqlalchemy_uri: string;
catalog: object;
ssh_tunnel: object;
}
3 changes: 3 additions & 0 deletions superset-frontend/src/views/CRUD/hooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -753,6 +753,9 @@ export function useDatabaseValidation() {
.catch(e => {
if (typeof e.json === 'function') {
return e.json().then(({ errors = [] }: JsonObject) => {
if (database?.ssh_tunnel) {
return [];
}
const parsedErrors = errors
.filter((error: { error_type: string }) => {
const skipValidationError = ![
Expand Down
4 changes: 4 additions & 0 deletions superset/db_engine_specs/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -1888,6 +1888,10 @@ class BasicParametersSchema(Schema):
required=False,
metadata={"description": __("Use an encrypted connection to the database")},
)
ssh = fields.Boolean(
required=False,
metadata={"description": __("Use an ssh tunnel connection to the database")},
)


class BasicParametersType(TypedDict, total=False):
Expand Down
12 changes: 12 additions & 0 deletions tests/integration_tests/databases/api_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -2885,6 +2885,10 @@ def test_available(self, app, get_available_engine_specs):
"description": "Additional parameters",
"type": "object",
},
"ssh": {
"description": "Use an ssh tunnel connection to the database",
"type": "boolean",
},
"username": {
"description": "Username",
"nullable": True,
Expand Down Expand Up @@ -2960,6 +2964,10 @@ def test_available(self, app, get_available_engine_specs):
"description": "Additional parameters",
"type": "object",
},
"ssh": {
"description": "Use an ssh tunnel connection to the database",
"type": "boolean",
},
"username": {
"description": "Username",
"nullable": True,
Expand Down Expand Up @@ -3035,6 +3043,10 @@ def test_available(self, app, get_available_engine_specs):
"description": "Additional parameters",
"type": "object",
},
"ssh": {
"description": "Use an ssh tunnel connection to the database",
"type": "boolean",
},
"username": {
"description": "Username",
"nullable": True,
Expand Down
4 changes: 4 additions & 0 deletions tests/integration_tests/db_engine_specs/postgres_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,10 @@ def test_base_parameters_mixin():
"description": "Additional parameters",
"additionalProperties": {},
},
"ssh": {
"description": "Use an ssh tunnel connection to the database",
"type": "boolean",
},
},
"required": ["database", "host", "port", "username"],
}
Expand Down