Fix bugs in URI constructor for MySQL connection

[MaksYermak]

In this PR I have fixed several bugs in the get_uri function in connection.py file. And also I have created unit tests for most of the use cases for this function.

Co-authored-by: Maksim Yermakou maksimy@google.com
Co-authored-by: Wojciech Januszek januszek@google.com
Co-authored-by: Lukasz Wyszomirski wyszomirski@google.com

---

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragement file, named {pr_number}.significant.rst, in newsfragments.

[potiuk]

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 am not saying it's wrong - but just - that - be cautions and try to understand how this "shared" method is used elsewhere before attempting to change it.

[MaksYermak]

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.

[potiuk]

@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.

Could you please point me to the docs/specs where conn-type://login:password@:3306?charset=utf-8 is wrong ? Is it a mysql sqlalchemy or "generic" sqlachemy problem? I am afraid that this might break something if this is mysql specific limitation rather than generic sqlalchemy one.

FWIW the URI specification https://datatracker.ietf.org/doc/html/rfc3986#section-3.3 is quite clear that the path component of the URI can be empty, and empty path is different than "root path" - (indicated by "/" ) - so I wonder if this change will not have impact on any other sqlalchemy URIs (especially with schema-less databases).

According to the RFC - conn-type://login:password@:3306?charset=utf-8 is perfectly good URI with "empty" path.

Can you trace down where the limitation of "require / path if there is a query part" comes from?

[MaksYermak]

@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.

Could you please point me to the docs/specs where conn-type://login:password@:3306?charset=utf-8 is wrong ? Is it a mysql sqlalchemy or "generic" sqlachemy problem? I am afraid that this might break something if this is mysql specific limitation rather than generic sqlalchemy one.

FWIW the URI specification https://datatracker.ietf.org/doc/html/rfc3986#section-3.3 is quite clear that the path component of the URI can be empty, and empty path is different than "root path" - (indicated by "/" ) - so I wonder if this change will not have impact on any other sqlalchemy URIs (especially with schema-less databases).

According to the RFC - conn-type://login:password@:3306?charset=utf-8 is perfectly good URI with "empty" path.

Can you trace down where the limitation of "require / path if there is a query part" comes from?

@potiuk yes of course in sqlalchemy doc https://docs.sqlalchemy.org/en/14/core/engines.html#database-urls has link to RFC-1738 spec. This spec in point 3.3 has this string If neither <path> nor <searchpart> is present, the "/" may also be omitted.. For me this string means that we should omitt / only in case when we don't have both of parameters <path> and <searchpart> if we have one of them the / should be. Correct me please if I make mistake?

[potiuk]

@potiuk yes of course in sqlalchemy doc https://docs.sqlalchemy.org/en/14/core/engines.html#database-urls has link to RFC-1738 spec. This spec in point 3.3 has this string If neither nor is present, the "/" may also be omitted.. For me this string means that we should omitt / only in case when we don't have both of parameters and if we have one of them the / should be. Correct me please if I make mistake?

Ah OK. Thanks for pointing it out. Indeed '/' was obligatory in 1738. SQL alchemy follow the older URL RFC (1738) which has actually been deprecated by URI (3986) - even though URI would be more approprite :D :

This document obsoletes [RFC2396], which merged "Uniform Resource
Locators" [RFC1738] and "Relative Uniform Resource Locators"
[RFC1808] in order to define a single, generic syntax for all URIs.
It obsoletes [RFC2732], which introduced syntax for an IPv6 address.
It excludes portions of RFC 1738 that defined the specific syntax of
individual URI schemes; those portions will be updated as separate
documents. The process for registration of new URI schemes is
defined separately by [BCP35]. Advice for designers of new URI
schemes can be found in [RFC2718]. All significant changes from RFC
2396 are noted in Appendix D.

[potiuk]

It's ok but we should change the title (as it is not MySQL -specific any more - but we can fix it at merge) and since it touches the core, I will need another approval (@uranusjr maybe?).

[github-actions]

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.