Skip to content

Improve DbApi code readability#18660

Closed
mariotaddeucci wants to merge 6 commits intoapache:mainfrom
mariotaddeucci:dbapi-hook-code-readability
Closed

Improve DbApi code readability#18660
mariotaddeucci wants to merge 6 commits intoapache:mainfrom
mariotaddeucci:dbapi-hook-code-readability

Conversation

@mariotaddeucci
Copy link
Copy Markdown
Contributor

Improve DbApi code readability


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@ephraimbuddy
Copy link
Copy Markdown
Contributor

Please rebase.

cc: @ashb

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Oct 1, 2021
@github-actions
Copy link
Copy Markdown

github-actions bot commented Oct 1, 2021

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@ephraimbuddy
Copy link
Copy Markdown
Contributor

Closing and reopening to trigger the CI

@potiuk
Copy link
Copy Markdown
Member

potiuk commented Oct 3, 2021

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

Copy link
Copy Markdown
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless there are good reasons, I tink we should keep backwards-compatibility for the info messages (reasons explained in the comment).

@mariotaddeucci mariotaddeucci deleted the dbapi-hook-code-readability branch October 5, 2021 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

full tests needed We need to run full set of tests for this PR to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants