betodealmeida commented on a change in pull request #16628:
URL: https://github.com/apache/superset/pull/16628#discussion_r715808058
##########
File path:
superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -127,69 +173,73 @@ const CredentialsInfo = ({
</span>
</div>
) : (
- <div
- className="input-container"
- css={(theme: SupersetTheme) => infoTooltip(theme)}
- >
- <div css={{ display: 'flex', alignItems: 'center' }}>
- <FormLabel required>{t('Upload Credentials')}</FormLabel>
- <InfoTooltip
- tooltip={t(
- 'Use the JSON file you automatically downloaded when creating
your service account in Google BigQuery.',
- )}
- viewBox="0 0 24 24"
- />
- </div>
-
- {!fileToUpload && (
- <Button
- className="input-upload-btn"
- onClick={() => document?.getElementById('selectedFile')?.click()}
- >
- {t('Choose File')}
- </Button>
- )}
- {fileToUpload && (
- <div className="input-upload-current">
- {fileToUpload}
- <DeleteFilled
- onClick={() => {
- setFileToUpload(null);
- changeMethods.onParametersChange({
- target: {
- name: 'credentials_info',
- value: '',
- },
- });
- }}
+ showCredentialsInfo && (
+ <div
+ className="input-container"
+ css={(theme: SupersetTheme) => infoTooltip(theme)}
+ >
+ <div css={{ display: 'flex', alignItems: 'center' }}>
+ <FormLabel required>{t('Upload Credentials')}</FormLabel>
+ <InfoTooltip
+ tooltip={t(
+ 'Use the JSON file you automatically downloaded when
creating your service account in Google BigQuery.',
Review comment:
This now applies to GSheets as well, right?
```suggestion
'Use the JSON file you automatically downloaded when
creating your service account.',
```
##########
File path:
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx
##########
@@ -539,32 +517,37 @@ const DatabaseModal:
FunctionComponent<DatabaseModalProps> = ({
if (validationErrors && !isEmpty(validationErrors)) {
return;
}
+ const parameters_schema = isEditMode
+ ? dbToUpdate.parameters_schema.properties
+ : dbModel?.parameters.properties;
+ const additionalEncryptedExtra = JSON.parse(
+ dbToUpdate.encrypted_extra || '{}',
+ );
+ const paramConfigArray = Object.keys(parameters_schema || {});
- const engine = dbToUpdate.backend || dbToUpdate.engine;
- if (engine === 'bigquery' && dbToUpdate.parameters?.credentials_info) {
- // wrap encrypted_extra in credentials_info only for BigQuery
+ paramConfigArray.forEach(paramConfig => {
+ // we are going through the parameter configuration and seeing if this
is an encrypted field
Review comment:
A suggestion for comments: good comments describe **why** something is
being done, instead of **what** is being done. Something like this:
```js
/*
* Parameters that are annotated with the `x-encrypted-extra` properties
should be moved to
* `encrypted_extra`, so that they are stored encrypted in the backend when
the database is
* created or edited.
*/
```
##########
File path: superset/models/core.py
##########
@@ -249,6 +250,14 @@ def parameters(self) -> Dict[str, Any]:
return parameters
+ @property
+ def parameters_schema(self) -> Dict[str, Any]:
+ try:
+ parameters_schema = self.db_engine_spec.parameters_json_schema()
# type: ignore # pylint: disable=line-too-long,useless-suppression
Review comment:
This line can be cleaned up a little bit. If you're disabling the
`useless-suppression` lint rule it means there's another rule you're
suppressing unnecessarily. You can probably write this as (untested):
```suggestion
parameters_schema = self.db_engine_spec.parameters_json_schema()
# type: ignore
```
##########
File path:
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx
##########
@@ -539,32 +517,37 @@ const DatabaseModal:
FunctionComponent<DatabaseModalProps> = ({
if (validationErrors && !isEmpty(validationErrors)) {
return;
}
+ const parameters_schema = isEditMode
+ ? dbToUpdate.parameters_schema.properties
+ : dbModel?.parameters.properties;
+ const additionalEncryptedExtra = JSON.parse(
+ dbToUpdate.encrypted_extra || '{}',
+ );
+ const paramConfigArray = Object.keys(parameters_schema || {});
- const engine = dbToUpdate.backend || dbToUpdate.engine;
- if (engine === 'bigquery' && dbToUpdate.parameters?.credentials_info) {
- // wrap encrypted_extra in credentials_info only for BigQuery
+ paramConfigArray.forEach(paramConfig => {
+ // we are going through the parameter configuration and seeing if this
is an encrypted field
if (
- dbToUpdate.parameters?.credentials_info &&
- typeof dbToUpdate.parameters?.credentials_info === 'object' &&
- dbToUpdate.parameters?.credentials_info.constructor === Object
+ parameters_schema[paramConfig]['x-encrypted-extra'] &&
+ dbToUpdate.parameters?.[paramConfig]
) {
- // Don't cast if object
- dbToUpdate.encrypted_extra = JSON.stringify({
- credentials_info: dbToUpdate.parameters?.credentials_info,
- });
-
- // Convert credentials info string before updating
- dbToUpdate.parameters.credentials_info = JSON.stringify(
- dbToUpdate.parameters.credentials_info,
- );
- } else {
- dbToUpdate.encrypted_extra = JSON.stringify({
- credentials_info: JSON.parse(
- dbToUpdate.parameters?.credentials_info,
- ),
- });
+ if (typeof dbToUpdate.parameters?.[paramConfig] === 'object') {
+ // add new encrypted extra to encrypted_extra object
+ additionalEncryptedExtra[paramConfig] =
+ dbToUpdate.parameters?.[paramConfig];
+ // cast the encrypted field as a string in parameters
Review comment:
Similarly here and in line 549: it's clear that we're casting it to a
string, so the comment doesn't add much. It would be better to say something
like:
```js
// The backend expects `encrypted_extra` as a string for historical reasons.
```
##########
File path:
superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -88,9 +100,43 @@ const CredentialsInfo = ({
const [fileToUpload, setFileToUpload] = useState<string | null | undefined>(
null,
);
+ const [isPublic, setIsPublic] = useState<boolean>(true);
+ const showCredentialsInfo =
+ db?.engine === 'gsheets' ? !isEditMode && !isPublic : !isEditMode;
+ // a database that has an optional encrypted field has an encrypted_extra
that is an empty object, this checks for that.
+ const isEncrypted = isEditMode && db?.encrypted_extra !== '{}';
+ const encryptedField = db?.engine && encryptedCredentialsMap[db.engine];
+ const encryptedValue =
+ typeof db?.parameters?.[encryptedField] === 'object'
+ ? JSON.stringify(db?.parameters?.[encryptedField])
+ : db?.parameters?.[encryptedField];
return (
<CredentialInfoForm>
- {!isEditMode && (
+ {db?.engine === 'gsheets' && (
+ <div className="catalog-type-select">
+ <FormLabel
+ css={(theme: SupersetTheme) => labelMarginBotton(theme)}
+ required
+ >
+ {t('Type of Google Sheets Allowed')}
Review comment:
Small nit, let's keep the copy consistent in terms of capitalization:
```suggestion
{t('Type of Google Sheets allowed')}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
