-
Notifications
You must be signed in to change notification settings - Fork 15.2k
Ensure HttpHook.run()
does not alter extra_options
passed to it
#51893
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, I have added a few comments, the main concern being the use of copy instead of just dict.get, and a check for whether to run a specific function
HttpHook.run()
does not alter extra_options
passed to itHttpHook.run()
does not alter extra_options
passed to it
I've addressed this concerns in each of your comments, thanks for the feedback! |
Checked it out again, answered all the comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, just mark all the comments as resolved
Previously, running
HttpHook.run()
altered the dictionary that was passed viaextra_options
. This was illustrated and reproduced in #51686 using the following logic within a Task.To fix this, two major changes were made. First, the dictionary that was passed into hook via the
run
orget_conn
method was "deep copied" before being transformed. Since the previous logic relied on that dictionary being transformed in different scopes, amerged_extra
was added as an instance-level attribute to theHttpHook
class. The appropriate changes were also made to its async counterpart.These changes were tested using the logic that was provided by @brki, as well as via the unit tests for this hook.
closes: #51686