-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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(#23176): adding URL decoding to SQLAlchemy URI #23421
Conversation
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 sensible to me, and approving the CI workflows. Pinging a few folks for review here in case they know of any fragility around URI encoding/decoding that I'm unaware of. |
Codecov Report
@@ Coverage Diff @@
## master #23421 +/- ##
==========================================
+ Coverage 67.62% 67.70% +0.07%
==========================================
Files 1908 1918 +10
Lines 73682 74135 +453
Branches 7982 8052 +70
==========================================
+ Hits 49828 50193 +365
- Misses 21806 21889 +83
- Partials 2048 2053 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 88 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Looks like the linters weren't happy with the import order. I committed a fix, and hopefully CI will pass now. Nag me for a merge or further fixes either way. Thanks for the fix!!! |
Hey, unfortunately this breaks other stuff, if I have a special character (e.g. '@') in password I need to escape it in order to work, but this effectively reverts my encoding back and then it fails because |
At risk of stating the obvious, changing the password is one workaround, though certainly not the workaround you're hoping for (sorry) :) |
I agree with @horkyada on this - this breaks connection strings with special characters, and is IMO not the correct solution to the problem: unquoting the connection string will introduce |
FYI, follow up PR merged: #30532 |
SUMMARY
Using urllib we can overcome this issue, now it is working fine
TESTING INSTRUCTIONS
set the superser config like
SQLALCHEMY_DATABASE_URI = "postgres://postgres:s3kr1t@localhost:5432/postgres?application_name=superset&options=--search_path%3D%22%24user%22,superset"
ADDITIONAL INFORMATION