Skip to content

Fix overwrite get_uri for Oracle#48734

Merged
Lee-W merged 11 commits intoapache:mainfrom
guan404ming:add-get-uri-for-oracle
Apr 11, 2025
Merged

Fix overwrite get_uri for Oracle#48734
Lee-W merged 11 commits intoapache:mainfrom
guan404ming:add-get-uri-for-oracle

Conversation

@guan404ming
Copy link
Member

@guan404ming guan404ming commented Apr 3, 2025 •
edited
Loading

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.

h30306
h30306 reviewed Apr 8, 2025
View reviewed changes
@Lee-W Lee-W self-requested a review April 8, 2025 09:40
@guan404ming guan404ming requested a review from h30306 April 8, 2025 13:32
h30306
h30306 approved these changes Apr 8, 2025
View reviewed changes
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

Lee-W
Lee-W reviewed Apr 9, 2025
View reviewed changes
@guan404ming
Copy link
Member Author

guan404ming commented Apr 9, 2025

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
Lee-W approved these changes Apr 11, 2025
View reviewed changes
@Lee-W
Copy link
Member

Lee-W commented Apr 11, 2025

left one nit, almost good to merge 👍

jason810496
jason810496 approved these changes Apr 11, 2025
View reviewed changes
Copy link
Member

@jason810496 jason810496 left a comment •
edited
Loading

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

guan404ming commented Apr 11, 2025

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
@eladkal eladkal mentioned this pull request Apr 16, 2025
Closed
56 tasks
@cjbj
Copy link

cjbj commented Apr 21, 2025 •
edited
Loading

@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 guan404ming mentioned this pull request Apr 22, 2025
Merged
@guan404ming
Copy link
Member Author

guan404ming commented Apr 22, 2025

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
@willianzilli willianzilli mentioned this pull request May 27, 2025
Closed
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Lee-W Lee-W Lee-W approved these changes

@jason810496 jason810496 jason810496 approved these changes

+1 more reviewer

@h30306 h30306 h30306 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area:providers provider:oracle

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@guan404ming @Lee-W @cjbj @h30306 @jason810496