h30306 commented on code in PR #48734:
URL: https://github.com/apache/airflow/pull/48734#discussion_r2032797557


##########
providers/oracle/src/airflow/providers/oracle/hooks/oracle.py:
##########
@@ -443,3 +443,27 @@ def handler(cursor):
         )
 
         return result
+
+    def get_uri(self):
+        """Get the URI for the Oracle connection."""
+        conn = self.get_connection(self.oracle_conn_id)
+        login = conn.login
+        password = conn.password
+        host = conn.host
+        port = conn.port or 1521
+        service_name = conn.extra_dejson.get("service_name")
+        sid = conn.extra_dejson.get("sid")
+
+        if sid and not service_name:
+            schema = sid
+        elif service_name and not sid:
+            schema = service_name
+        else:
+            schema = conn.schema or ""

Review Comment:
   In the current conditional logic, when both `sid` and `service_name` are 
provided, the fallback uses `conn.schema or ""` (line 462), `sid` and 
`service_name` will be useless here, is it expected behavior?
   
   If not, I recommend simplifying the logic to make the priority clearer:
   
   ```python
   if sid:
       schema = sid
   elif service_name:
       schema = service_name
   else:
       schema = conn.schema or ""
   ```
   Alternatively, you could raise an error if both sid and service_name are set 
to enforce exclusivity:
   ```python
   if sid and service_name:
       raise ValueError("Cannot specify both SID and Service Name. Choose one.")
   ```



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to