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
chore(dbc-ui): encode characters before building sqlalchemy uri #17969
chore(dbc-ui): encode characters before building sqlalchemy uri #17969
Conversation
a6d077d
to
44ff243
Compare
44ff243
to
ebaedb6
Compare
Codecov Report
@@ Coverage Diff @@
## master #17969 +/- ##
==========================================
+ Coverage 67.07% 67.61% +0.54%
==========================================
Files 1609 1610 +1
Lines 64905 66562 +1657
Branches 6868 6868
==========================================
+ Hits 43537 45009 +1472
- Misses 19502 19687 +185
Partials 1866 1866
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
superset/databases/utils.py
Outdated
|
||
|
||
def encode_parameters(parameters: Dict[str, Any]) -> Dict[str, Any]: | ||
return {k: urllib.parse.quote(v) for k, v in parameters.items()} |
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.
Let's use quote_plus
here, since it's what SQLAlchemy recommends: https://docs.sqlalchemy.org/en/14/core/engines.html?highlight=quote_plus#database-urls
@@ -99,9 +100,12 @@ def run(self) -> None: | |||
except json.decoder.JSONDecodeError: | |||
encrypted_extra = {} | |||
|
|||
# encode parameters | |||
params = encode_parameters(self._properties.get("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.
does this value get saved somewhere or is this just for validation?
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.
encode paramaters returns the encode keys in a new dictionary (params) which is used to build the sqlalchemy url a few lines below
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.
Hugh, I think the problem is that the validation command doesn't save the database, so the encoding will only apply during validation but not later. You also need to update DatabaseParametersSchemaMixin
.
It might be simpler to implement the encoding logic inside build_sqlalchemy_uri
. This way it will work with both the validator command and the APIs, and we can have DB-specific encoding logic, which could be needed in the future.
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.
We need to remove this line now, otherwise we'll end up double encoding the parameters when we call build_sqlalchemy_uri
below.
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.
@hughhhh I think it would be better to implement the encoding inside the build_sqlalchemy_uri
methods. Even though we would have to implement it for each DB engine spec (BQ, Gsheets, Snowflake and the generic one we use for Postgres/MySQL) it keeps the logic more contained.
.pylintrc
Outdated
@@ -214,7 +214,7 @@ max-nested-blocks=5 | |||
[FORMAT] | |||
|
|||
# Maximum number of characters on a single line. | |||
max-line-length=90 | |||
max-line-length=120 |
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.
@hughhhh 90 is already pretty generous, let's keep this as is.
@@ -99,9 +100,12 @@ def run(self) -> None: | |||
except json.decoder.JSONDecodeError: | |||
encrypted_extra = {} | |||
|
|||
# encode parameters | |||
params = encode_parameters(self._properties.get("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.
Hugh, I think the problem is that the validation command doesn't save the database, so the encoding will only apply during validation but not later. You also need to update DatabaseParametersSchemaMixin
.
It might be simpler to implement the encoding logic inside build_sqlalchemy_uri
. This way it will work with both the validator command and the APIs, and we can have DB-specific encoding logic, which could be needed in the future.
c920537
to
e65f2f3
Compare
ddff2a2
to
ba92f41
Compare
ba92f41
to
0d03f08
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.
I think we're double encoding the parameters now on validation.
@@ -99,9 +100,12 @@ def run(self) -> None: | |||
except json.decoder.JSONDecodeError: | |||
encrypted_extra = {} | |||
|
|||
# encode parameters | |||
params = encode_parameters(self._properties.get("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.
We need to remove this line now, otherwise we'll end up double encoding the parameters when we call build_sqlalchemy_uri
below.
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.
Hugh, we're missing BQ, Gsheets and Snowflake — we need to update their build_sqlalchemy_uri
methods to encode the parameters as well.
@@ -254,6 +255,10 @@ def build_sqlalchemy_uri( | |||
the constructed SQLAlchemy URI to be passed. | |||
""" | |||
parameters = data.pop("parameters", {}) | |||
|
|||
# encode parameters | |||
parameters = encode_parameters(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.
We also need to add this to DB engine specs that implement their own build_sqlalchemy_uri
.
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.
i added the encode_params to snowflake but, for gsheets and bigquery we don't leverage parameters to build the uri.
gsheets we always just return "gsheets://
and for biguery we just leverage encrypted_extra
f7bc8aa
to
9fcfc46
Compare
closing for now |
SUMMARY
Create util function to encode/escape specific characters in our parameters. This will solve the problem for customers wanting use
@
or$
in their password or hostnames.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION