[GitHub] [airflow] dstandish commented on issue #15016: OdbcHook string values in connect_kwargs dict converts to None

[GitBox <gi...@apache.org>]

dstandish commented on issue #15016:
URL: https://github.com/apache/airflow/issues/15016#issuecomment-837110088


   > Why is clean_bool being called in the first place for a user-provided dictionary? I'm not sure how this is necessary because the user can provide a literal boolean value in the dictionary if needed, no? If in the event that a driver needs to take a case-sensitive boolean string for some parameter, then clean_bool would make it impossible to provide such a value.
   
   TLDR I agree we should remove `clean_bool`
   
   As to why it was put in, in the first place, here's what I think happened.
   
   Observe what happens with airflow's connection URI format when we try to pass a boolean value:
   
   ```python
   >>> c = Connection(conn_id='hello', uri='hello://?my_val=True')
   >>> c.extra_dejson
   {'my_val': 'True'}
   ```
   
   It's impossible to produce a json object with boolean values.
   
   So when you are using top level key-value pairs in conn `extra` then in some cases it makes sense to cast to bool.
   
   I suspect maybe initially in the development of this hook the connect kwargs were top level within `extra`, where doing this cast would make sense.
   
   But when dealing with nested json, the boolean vs. string issue becomes irrelevant and you have new problems to solve.  Namely, at the time this hook was merged, airflow's conn URI did not support nested json.  So this hook did not actually allow for storage of `connect_kwargs` in extra when using the URI format.  For that, you'd have to add a json.loads-if-str conversion [here](https://github.com/apache/airflow/blob/2bee173ef71ab67a288a5513df37914e3dd4a6ce/airflow/providers/odbc/hooks/odbc.py#L165).   But now that we have [support for arbitrary json in conn extra](https://github.com/apache/airflow/pull/15100), there's no need for such a conversion.
   
   So since `connect_kwargs` is nested json, there's no valid reason for converting to bool, and I suspect this was just an oversight, and accordingly, even though it could be fixed, it is best to remove.
   


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org