Add oracledb thick mode support for oracle provider

[pauldalewilliams]

trafficstars

closes: #24618

Adds support for oracledb thick mode in the oracle provider, along with the option to set some defaults supported by oracledb (fetch_decimals and fetch_lobs).

[potiuk]

Some errors :(

[pauldalewilliams]

@potiuk Not related to my changes but I'll keep updating from main until resolved.

[Taragolis]

@potiuk Not related to my changes but I'll keep updating from main until resolved.

Seems like it is somehow related to #25980 I will have a look to it.

[pauldalewilliams]

you are duplicating some code from get_conn

get_conn is by convention the method you call to get the client. can you update that code instead of adding a separate client creation path?

@dstandish Apologies - I took @potiuk 's comments on #24618 to mean it should go in the __init__ for the hook. I'm fine moving it within the get_conn method. The duplicated code was just to get access to the connection's extra options to grab the parameters. I'm not sure init_oracle_client returns a client - seems to be just setting up the parameters for calling the thick client. I think?

[dstandish]

I'm not sure init_oracle_client returns a client - seems to be just setting up the parameters for calling the thick client. I think?

doesn't really matter. call it client generation / construction / auth / whathaveyou... in any case why retrieve the conn in two places, in two different ways? if you do it in __init__.py, why do we do it again in get_conn?

what's the difference between the way we handle the airflow conn in init vs in get_conn? probably there shouldn't be any difference, because probably it should just be done once and in one place right?

[dstandish]

I took @potiuk 's comments on #24618 to mean it should go in the __init__ for the hook

looks like you're referring to this comment. he's suggesting adding another param but not necessarily suggesting add more airflow conn retrieval. you can store the mode param as an instance attr and look at it in get_conn.

[pauldalewilliams]

@dstandish Do you mean just move the connection retrieval to the __init__, store the connection and extra_options as instance variables, and just reference them in get_conn instead of calling get_connection again? Maybe move a lot of the parameter parsing in get_conn to __init__ similar to how it's handled in the ssh provider?

[pauldalewilliams]

@dstandish Let me know if my most recent commit is more in line with what you were thinking. Tests passed locally but we'll see how the full suite goes.

[dstandish]

So you have done quite a bit of refactoring, and added, it seems, quite a bit of new functionality, but you have only added a test for the thick mode part. In writing hooks, particularly when you need to reconcile information between hook params and airflow conn attrs (which might both be supplied), it's generally desired to test that behavior to verify that you resolve it properly and what the order of precedence is.

What I would like to suggest is, just keep this PR confined to adding thick mode support. And do it as I suggested before, by adding a parameter or two to __init__ but keep the airflow conn parsing logic confined to get_conn. It seems this could be a much simpler change and easier to review. Then in a separate PR you can add the other functionality you desire and it can be considered independently. You might consider separating the followup PR into two, one simply adding the extra parameters you desire and the other being the refactor that possibly moves things from get_conn to init.

[pauldalewilliams]

So you have done quite a bit of refactoring, and added, it seems, quite a bit of new functionality, but you have only added a test for the thick mode part. In writing hooks, particularly when you need to reconcile information between hook params and airflow conn attrs (which might both be supplied), it's generally desired to test that behavior to verify that you resolve it properly and what the order of precedence is.

What I would like to suggest is, just keep this PR confined to adding thick mode support. And do it as I suggested before, by adding a parameter or two to __init__ but keep the airflow conn parsing logic confined to get_conn. It seems this could be a much simpler change and easier to review. Then in a separate PR you can add the other functionality you desire and it can be considered independently. You might consider separating the followup PR into two, one simply adding the extra parameters you desire and the other being the refactor that possibly moves things from get_conn to init.

@dstandish I don't understand how I'd add the parameters to check for the thick_mode stuff to init without duplicating the get_connection stuff. Why not just add it in get_conn for now? Then I don't need to do my goofy init call in the tests either.