fix(migration): Support non-alphanumeric passwords in alembic.

[ramki88]

This commit modifies alembic's env.py, so that it supports non-alphanumeric passwords.

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

• [ x] Has associated issue: Fixes URL encoded passwords are getting decoded in env.py  #26029
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[craig-rueda]

This looks like it might break existing functionality. Should we instead do the replace on the "decoded_url" var?

[ramki88]

@craig-rueda You mean use the same var name? Or apply the replace on top of urllib.parse.unquote(DATABASE_URI).

[craig-rueda]

Like this:

decoded_uri = urllib.parse.unquote(DATABASE_URI)
decoded_uri = decoded_uri.replace('%', '%%')
config.set_main_option("sqlalchemy.url", decoded_uri)

So basically just add the replace in there.

Note that if your PWD has a % symbol in it, you may need to "enquote" it going in, i.e. some_pass_with_%_in_it would need to be url encoded as some_pass_with_%25_in_it

[ramki88]

Ahhh.. I got it now. Sorry I should have tried to explain in detail the need for this change.

Example : Password with @ in them super2p@ss. As suggested in SQLAlechemy Docs if the password is either escaped and passed to superset or in superset_config.py if the user escapes the special char using urllib.parse.quote_plus("super2p@ss") alembic's env.py is going to unquote it and we end up with "super2p@ss" which will not be parsed by psycopg2.

1. user passes -> "super2p@ss"
2. superset_config.py parsers -> "super2p%40ss"
3. superset_config.py sets DATABASE_URI -> "postgresql+psycopg2://user:super2p%40ss@localhost:5432/superset"
4. env.py decodes DATABASE_URI into decoded_uri -> "postgresql+psycopg2://user:super2p@ss@localhost:5432/superset"

And superset app starts up, but the migration fails with the error "psycopg2.OperationalError: could not translate host name "ss@localhost" to address" as it splits at symbol "@".

Just by removing the unquote in env.py will not fix the problem as the password is being set as a config, which uses configparser hence the "%" has to be double escaped into %% for it to be valid.

so the flow becomes..

1. user passes -> "super2p@ss"
2. superset_config.py parsers -> "super2p%40ss"
3. superset_config.py sets DATABASE_URI -> "postgresql+psycopg2://user:super2p%40ss@localhost:5432/superset"
4. env.py doubles escapes DATABASE_URI into escaped_uri -> "postgresql+psycopg2://user:super2p%%40ss@localhost:5432/superset"

References:
psycopg/psycopg2#1546
sqlalchemy/alembic#700 (comment)

[craig-rueda]

Ok, makes sense. From what it looks like here, we're just trying to address an errant @ from finding its way into the connection string.

Hoping this doesn't break anyone 🙏

CC - @john-bodley @dpgaspar ??

[codecov]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (57d61df) 69.09% compared to head (626ea48) 70.64%.
Report is 362 commits behind head on master.

Files	Patch %	Lines
superset/migrations/env.py	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #26094      +/-   ##
==========================================
+ Coverage   69.09%   70.64%   +1.54%     
==========================================
  Files        1940     1945       +5     
  Lines       75867    82222    +6355     
  Branches     8444     8444              
==========================================
+ Hits        52423    58087    +5664     
- Misses      21268    21959     +691     
  Partials     2176     2176              

Flag	Coverage Δ
hive	54.20% <0.00%> (+0.51%)	⬆️
mysql	77.97% <0.00%> (-0.19%)	⬇️
postgres	79.36% <0.00%> (+1.10%)	⬆️
presto	54.24% <0.00%> (+0.60%)	⬆️
python	83.86% <0.00%> (+0.91%)	⬆️
sqlite	78.71% <0.00%> (+1.79%)	⬆️
unit	56.16% <0.00%> (+0.36%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[rusackas]

Hmm... title looks fine... but other CI tasks are stuck too. Closing/reopening to kick this. It might need a rebase, and @craig-rueda might want to take a fresh look.

[rusackas]

@ramki88 would you mind running the pre-commit hooks on this so it can pass CI/linting?

[ramki88]

@ramki88 would you mind running the pre-commit hooks on this so it can pass CI/linting?

Sure, I've ran it and rebased with master.

[rusackas]

Running CI. Fingers crossed!

[kahlua-kol]

Hey! Do you have any news on this topic? :) Would be really helpful to have a working fix for special characters

[rusackas]

I think the fact that some tests aren't running, and we have a weird (possibly unrelated) MySQL test error, indicates that some of these CI tasks have been fixed on master but aren't being reflected here. A rebase of this branch/PR (again, I know, sorry...) might resolve that. CI has been a bit turbulent lately.

[ramki88]

I think the fact that some tests aren't running, and we have a weird (possibly unrelated) MySQL test error, indicates that some of these CI tasks have been fixed on master but aren't being reflected here. A rebase of this branch/PR (again, I know, sorry...) might resolve that. CI has been a bit turbulent lately.

Sure, I have rebased with master 8fb2cae