[GitHub] [airflow] MaksYermak commented on pull request #24320: Fix bugs in URI constructor for MySQL connection

[GitBox <gi...@apache.org>]

MaksYermak commented on PR #24320:
URL: https://github.com/apache/airflow/pull/24320#issuecomment-1156147938

   > You do realize that "model/connection" is used by more than MySQL?
   > 
   > Are you sure you are not breaking other usages?
   > 
   > It seems it does break a lot of tests at least. I'd be really cautious to modify "airflow core" connection method to fix MySQL.. This looks (without going into details) like a need to override the method in MySQL connection to do mysql-specific stuff rather than change behaaviours of all other connections.
   
   @potiuk I think my change in `get_uri` method shouldn’t break something, conversely this change will fix current issue. Our `get_uri` function should return DB connection URL for `create_engine` function in SQLAlchemy in this format `dialect[+driver]://user:password@host/dbname[?key=value..]`. Currently Airflow has a problem, when user tries to create DB connection without `dbname`, but with `extras` user meets error. It happens, because `get_uri` function adds `extras` to the `host` without division by `/` e.g. `conn-type://login:password@:3306?charset=utf-8` is wrong URL the correct URL should look like this `conn-type://login:password@:3306/?charset=utf-8`.
   
   In last commit I have moved unit tests from `test_mysql` to `test_dbapi`, because my change relates to all DB providers.


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org