Skip to content

Conversation

@guan404ming
Copy link
Member

Related Issue

Towards #38195

Why

The current implementation of DbApiHook.get_uri() incorrectly assumes that Airflow Connection URI representation is a valid SQLAlchemy URI, which is not always true. This assumption works in simple cases but fails in more complex scenarios.
For JDBC connections specifically, the entire JDBC URL is stored in the "host" field of the connection, which follows a completely different format than standard SQLAlchemy URIs.

How

  • Overrides the inherited DbApiHook.get_uri method within JdbcHook.
  • Return the host field

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

@Lee-W Lee-W marked this pull request as draft April 10, 2025 01:34
@guan404ming guan404ming force-pushed the add-get-uri-for-jdbc branch 3 times, most recently from bb61060 to a415551 Compare April 12, 2025 12:00
@guan404ming guan404ming marked this pull request as ready for review April 12, 2025 12:03
@guan404ming guan404ming requested a review from Lee-W April 12, 2025 12:03
@guan404ming guan404ming force-pushed the add-get-uri-for-jdbc branch from a415551 to 38a46ed Compare April 12, 2025 12:28
@guan404ming guan404ming force-pushed the add-get-uri-for-jdbc branch from 38a46ed to 23ac206 Compare April 12, 2025 13:24
Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. left a few nits

Co-authored-by: Wei Lee <weilee.rx@gmail.com>
@guan404ming
Copy link
Member Author

Thanks for reviewing!

@Lee-W Lee-W requested a review from jason810496 April 14, 2025 09:22
Copy link
Member

@jason810496 jason810496 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thanks.

@Lee-W Lee-W merged commit dc5d007 into apache:main Apr 14, 2025
61 checks passed
@guan404ming guan404ming deleted the add-get-uri-for-jdbc branch April 14, 2025 14:54
@guan404ming guan404ming changed the title feat: overwrite get_uri for JDBC Fix overwrite get_uri for JDBC Apr 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants