Skip to content

Conversation

@guan404ming
Copy link
Member

@guan404ming guan404ming commented Apr 3, 2025

Related Issue

Towards #38195

Why

The base DbApiHook.get_uri incorrectly assumes that Airflow connection URI representation is a valid SQLAlchemy URI, but this is not always true, especially for complex cases. For Oracle connections, the URI format needs to handle various connection scenarios including SID, service name, and PDB configurations.

This PR implements a proper get_uri method for the OracleHook class, ensuring that Oracle connection URIs are correctly formatted according to Oracle's connection string requirements.

How

  • Implements the get_uri method in OracleHook to properly generate Oracle connection URIs
  • Handles different Oracle connection scenarios:
  • Adds parametrized unit tests to validate URI generation for various configurations

Ref: https://docs.oracle.com/en/database/other-databases/essbase/21/essoa/connection-string-formats.html

^ 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 self-requested a review April 8, 2025 09:40
Co-Authored-By: Howard W. Chung <33175898+h30306@users.noreply.github.com>
@guan404ming guan404ming requested a review from h30306 April 8, 2025 13:32
Copy link
Contributor

@h30306 h30306 left a comment

Choose a reason for hiding this comment

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

LGTM

guan404ming and others added 2 commits April 9, 2025 22:52
@guan404ming
Copy link
Member Author

Thanks for your reviewing, I have updated the PR with you suggestion!

@guan404ming guan404ming requested a review from Lee-W April 9, 2025 15:07
@guan404ming guan404ming force-pushed the add-get-uri-for-oracle branch from 423199b to 701b4c1 Compare April 9, 2025 16:14
@Lee-W
Copy link
Member

Lee-W commented Apr 11, 2025

left one nit, almost good to merge 👍

Co-authored-by: Wei Lee <weilee.rx@gmail.com>
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 for the PR.

@guan404ming
Copy link
Member Author

Thanks for reviewing!

@guan404ming guan404ming changed the title fix: overwrite get-uri for Oracle fix: overwrite get_uri for Oracle Apr 11, 2025
@Lee-W Lee-W merged commit 9991880 into apache:main Apr 11, 2025
61 checks passed
@cjbj
Copy link

cjbj commented Apr 21, 2025

@guan404ming @Lee-W @h30306 This (merged) PR raised some questions. If get_uri() is for "a valid SQLAlchemy URI" then the change in the MR:

uri = f"oracle://{login}:{password}@{host}:{port}"
if sid:
    uri = f"{uri}/{sid}"
elif service_name:
    uri = f"{uri}/{service_name}"
elif conn.schema:
    uri = f"{uri}/{conn.schema}"

may not be correct since it doesn't follow documented SQLAlchemy connection syntax for the python-oracledb driver.

  • Since airflow 3 now uses python-oracledb, the URI should begin with oracle+oracledb: otherwise the obsolete cx_Oracle driver would be needed.

  • SQLAlchemy needs service names to be prefixed with ?service_name=.

  • Service names are the new way forward (as of a few decades ago), so service_name should be first choice, not sid in the if/else block.

  • I forget the history of Airflow and schemas, so can't comment on whether the final elif block needs some tweaks to be valid SQLAlchemy syntax.

Code should be like:

uri = f"oracle+oracledb://{login}:{password}@{host}:{port}"
if service_name:
    uri = f"{uri}?service_name={service_name}"
elif sid:
    uri = f"{uri}/{sid}"
# and maybe this, depending what is in conn.schema????    
#elif conn.schema:
#    uri = f"{uri}/{conn.schema}"

@guan404ming
Copy link
Member Author

Thanks for pointing out, I've open a PR for the fix. Please take another look if available, thanks!

@guan404ming guan404ming deleted the add-get-uri-for-oracle branch April 22, 2025 02:45
@guan404ming guan404ming changed the title fix: overwrite get_uri for Oracle Fix overwrite get_uri for Oracle 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.

5 participants