Add oracledb thick mode support for oracle provider

[pauldalewilliams]

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.

[dstandish]

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?

[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]

seems you should not have to do this?

[pauldalewilliams]

I did because of the way the tests were originally structured. They create the hook in setUp (line 64 in my commit) and then modify the extra in the tests themselves. Since all the parameter parsing is now happening in the __init__ it never picks that up. This seemed to work for picking up those changes without drastically changing the tests. I considered trying to rework it to function more like the way tests are structured in https://github.com/apache/airflow/blob/main/tests/providers/ssh/hooks/test_ssh.py with a separate connection for each variation. But I'm not very confident in writing tests in the first place and didn't fully understand how to rewrite all this in that way.

I did feel a little goofy doing it this way but it seemed to work for the purpose just fine and I couldn't think of any negatives aside from it just being goofy.

[dstandish]

right so the code before, it retrieved the airflow conn object and recreated the client object every time get_conn was called.
and then they changed the airflow conn object resulting in different behavior with client creation time. and your move of the logic to __init__ changes this behavior, so that get_conn doesn't re-retrieve the object.
while it's strictly speaking a "breaking" change probably it's unlikely to trip anyone up. but i think it would be best to adjust the tests so that the code is invoked in a more normal way.
what you could is apply mock.patch decorator to patch the get_connection method (instead of doing it by assignment in setUp) and then set it to return the modified conn object, then instantiate your hook in each test method e.g. db_hook = OracleHook(...).

[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.

[josh-fell]

Looks good! Just a small question on aligning default values of boolean parameters as mentioned in docstrings and docs.