[GitHub] [airflow] potiuk commented on pull request #18660: Improve DbApi code readability

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

potiuk commented on pull request #18660:
URL: https://github.com/apache/airflow/pull/18660#issuecomment-933029882


   THis change does a little bit more than just refactor - now also get_records and get_first will log at "info" level the "Running statemant" and  number of rows affected as well:
   
   ```
           self.log.info("Running statement: %s, parameters: %s", sql_statement, parameters)
           ....
           # According to PEP 249, this is -1 when query result is not applicable.
           if cur.rowcount >= 0:
               self.log.info("Rows affected: %s", cur.rowcount)
   ```
   
   You could consider it as "good" for understanding  what's going on, but this might be an undesireable side-effect of this change (especially that info is the default log level for most users). This also has a non-negligible performance impact -  if your query has multiple parameters, all of them will be formatted and printed which might give a significant performance hit. Even worse (and that's disqualification IMHO), it has non-trivial security implicatons - I can easily imagine someone passing some sensitive data as parameters of the SQL query params. This might not be the best practice, but it might be unavidable (and those parameters might not come from connections/secret variables so our secret masker might not mask it). This might be totally unexpected if it is changed without warning.
   
   I think the best solution would be to add optional "print_info_log" or something parameters with the right defaults for different methods and pass them down to keep backwards-compatible behaviour. Also that would add explicit parameter - it is not clear currently whether the info will be logged or not and having this parameter with explicit backwards-compatible default values should do much better in communicating it.
   
   I would not be such picky in other parts of code, but DBApi is pretty important and crucial part that is extended and used by a number of providers, and I think we should avold surprises here (also see the comment just above the DBApi class) -  we already had a few cases where we got some backwards-incompatibility problems with this class so we have to be extra careful here.


-- 
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: commits-unsubscribe@airflow.apache.org

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