Skip to content

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

jroachgolf84
Copy link
Collaborator

Previously, running HttpHook.run() altered the dictionary that was passed via extra_options. This was illustrated and reproduced in #51686 using the following logic within a Task.

...

extra_options = {"verify_ssl": None, "proxies": None}
original_extra_options = deepcopy(extra_options)

# jsonplaceholder Connection has
# * Host = https://jsonplaceholder.typicode.com
# * Extra = {"foo": "bar"}
# Note: some Extra value is necessary, otherwise the extra_options passed to run are not used, see https://github.com/apache/airflow/issues/51685
http_hook = HttpHook(method='GET', http_conn_id='jsonplaceholder')

for _ in range(2):
    http_hook.run(
        endpoint=f"/posts/1",
        extra_options=extra_options,
    )
    assert extra_options == original_extra_options, \
        f"extra_options: {extra_options}, original_extra_options: {original_extra_options}"

To fix this, two major changes were made. First, the dictionary that was passed into hook via the run or get_conn method was "deep copied" before being transformed. Since the previous logic relied on that dictionary being transformed in different scopes, a merged_extra was added as an instance-level attribute to the HttpHook 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

Copy link

@Nataneljpwd Nataneljpwd left a 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

@jroachgolf84 jroachgolf84 changed the title Ensuring HttpHook.run() does not alter extra_options passed to it Ensure HttpHook.run() does not alter extra_options passed to it Jun 19, 2025
@jroachgolf84
Copy link
Collaborator Author

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

I've addressed this concerns in each of your comments, thanks for the feedback!

@Nataneljpwd
Copy link

Checked it out again, answered all the comments.
Looks good to me!

Copy link

@Nataneljpwd Nataneljpwd left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HttpHook.run() alters the extra_options dict passed to it
3 participants