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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
