-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
feat: Add Private Google Sheets to dynamic form #16628
feat: Add Private Google Sheets to dynamic form #16628
Conversation
6c0dea2
to
aee5355
Compare
c201af7
to
f7cc7ac
Compare
2221c0c
to
40662ea
Compare
Codecov Report
@@ Coverage Diff @@
## master #16628 +/- ##
==========================================
- Coverage 77.05% 76.83% -0.22%
==========================================
Files 1021 1021
Lines 54693 54709 +16
Branches 7457 7457
==========================================
- Hits 42141 42033 -108
- Misses 12307 12431 +124
Partials 245 245
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
af2c6be
to
34ad067
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I just have a few questions. Also, let's leave all those empty lines you deleted, they help with readability.
superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
Outdated
Show resolved
Hide resolved
const [isPublic, setIsPublic] = useState<boolean>(true); | ||
const showCredentialsInfo = | ||
db?.engine === 'gsheets' ? !isEditMode && !isPublic : !isEditMode; | ||
const isEncrypted = db?.encrypted_extra && db?.encrypted_extra?.length > 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't an encrypted_extra
field with a single key/value pair considered encrypted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the backend we make encrypted_extra into a string of an empty object. So "{}".
enum CredentialInfoOptions { | ||
jsonUpload, | ||
copyPaste, | ||
} | ||
|
||
const setStringToBoolean = (optionValue: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const setStringToBoolean = (optionValue: string) => { | |
const castStringToBoolean = (optionValue: string) => { |
@@ -277,7 +277,6 @@ export function useSingleViewResource<D extends object = any>( | |||
updateState({ | |||
loading: true, | |||
}); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code also needs to breath... let's leave these empty lines! Usually empty lines separate logical blocks that are (slightly) unrelated.
superset/databases/api.py
Outdated
@@ -920,7 +921,6 @@ def available(self) -> Response: | |||
for engine_spec, drivers in get_available_engine_specs().items(): | |||
if not drivers: | |||
continue | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
superset/databases/api.py
Outdated
@@ -943,7 +943,6 @@ def available(self) -> Response: | |||
payload[ | |||
"sqlalchemy_uri_placeholder" | |||
] = engine_spec.sqlalchemy_uri_placeholder # type: ignore | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto. Is your editor automatically removing these lines?
f49bb8c
to
07baa49
Compare
/testenv up |
@hughhhh Ephemeral environment spinning up at http://34.222.153.226:8080. Credentials are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, I left just a few comments.
css={(theme: SupersetTheme) => labelMarginBotton(theme)} | ||
required | ||
> | ||
{t('Type of Google Sheets Allowed')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit, let's keep the copy consistent in terms of capitalization:
{t('Type of Google Sheets Allowed')} | |
{t('Type of Google Sheets allowed')} |
<FormLabel required>{t('Upload Credentials')}</FormLabel> | ||
<InfoTooltip | ||
tooltip={t( | ||
'Use the JSON file you automatically downloaded when creating your service account in Google BigQuery.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This now applies to GSheets as well, right?
'Use the JSON file you automatically downloaded when creating your service account in Google BigQuery.', | |
'Use the JSON file you automatically downloaded when creating your service account.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this, originally I had a separate component for Gsheets, but I forgot to go and update this when I combined all the encrypted fields.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A suggestion for comments: good comments describe why something is being done, instead of what is being done. Something like this:
/*
* 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.
*/
// add new encrypted extra to encrypted_extra object | ||
additionalEncryptedExtra[paramConfig] = | ||
dbToUpdate.parameters?.[paramConfig]; | ||
// cast the encrypted field as a string in parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
// The backend expects `encrypted_extra` as a string for historical reasons.
superset/models/core.py
Outdated
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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):
parameters_schema = self.db_engine_spec.parameters_json_schema() # type: ignore # pylint: disable=line-too-long,useless-suppression | |
parameters_schema = self.db_engine_spec.parameters_json_schema() # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when I had this with just type: ignore it was failing the python lint test. So I emulated this line:
https://github.com/apache/superset/blob/master/superset/models/core.py#L246
07baa49
to
6bfa79e
Compare
dca4017
to
4b42aba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Thanks for all the work, @AAfghahi !
4b42aba
to
2eb3834
Compare
2eb3834
to
e56ea3f
Compare
e56ea3f
to
f594706
Compare
Ephemeral environment shutdown and build artifacts deleted. |
* first pass private gsheets * made encrypted extra into string, refactored onParametersChanged * private sheets working, credential_info errors * all but test connection working * first pass private gsheets * made encrypted extra into string, refactored onParametersChanged * private sheets working, credential_info errors * all but test connection working * Regenerate package-lock.json Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
@AAfghahi I don't see a package.json file change in here.. was the package-lock an oversight? |
No, it was failing e2e tests so Beto regenerated the package-lock here: e56ea3f so that it would pass the tests. |
* first pass private gsheets * made encrypted extra into string, refactored onParametersChanged * private sheets working, credential_info errors * all but test connection working * first pass private gsheets * made encrypted extra into string, refactored onParametersChanged * private sheets working, credential_info errors * all but test connection working * Regenerate package-lock.json Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
* first pass private gsheets * made encrypted extra into string, refactored onParametersChanged * private sheets working, credential_info errors * all but test connection working * first pass private gsheets * made encrypted extra into string, refactored onParametersChanged * private sheets working, credential_info errors * all but test connection working * Regenerate package-lock.json Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
SUMMARY
This PR does two things, firstly it adds support for private gsheets as a dynamic form in the DatabaseConnection modal. Secondly, it refactors how we pass encrypted fields in said modal.
Previously this sections were very narrow. What this PR does is, during the onSave action after all parameters have been validated, goes through the dbModel or a new calculated column named parameters_schema and checks to see if each parameter is encrypted. It uses the 'x-encrypted-extra' field that is exposed through the /available endpoint.
One point of confusion that might arise from reading the code, we pass in encrypted fields as strings in the frontend, but then convert them to dictionaries in the backend, thus when fetching encrypted fields, the fields have to be serialized in order to be properly saved.
Currently the only other database that uses encrypted fields is Big Query, the previous logic used on creation and edit have been preserved and are applied to gsheets.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
adding a private sheet:
Screen.Recording.2021-09-23.at.5.58.47.PM.mov
adding a private sheet to a existing database that has only public sheets:
Screen.Recording.2021-09-23.at.5.55.25.PM.mov
TESTING INSTRUCTIONS
I am able to create any database that is able to be added dynamically. I tested this on Gsheets, BigQuery, and Postgres (using a very basic localhost postgres).
I was able to add a private gsheet. Am able to go back and edit it, and still be able to connect and finish.
Create a database that only has public gsheets.
Create a database that only has public gsheets, and then when editing it add a private gsheet with a proper encrypted json.
ADDITIONAL INFORMATION