[Providers/HTTP] Add adapter parameter to HttpHook to allow custom requests adapters #44302

jieyao-MilestoneHub · 2024-11-23T10:07:45Z

This PR adds an adapter parameter to the HttpHook, allowing users to mount custom adapters for HTTP request handling. This enhances flexibility and supports use cases like custom retries, timeouts, and SSL handling.

Summary of Changes:

Added adapter parameter to HttpHook to allow custom HTTP adapters.
Modified get_conn to support mounting custom adapters or using TCPKeepAliveAdapter by default.
Added comprehensive tests to validate the functionality of the adapter parameter and its integration with get_conn.
Ensured all new tests pass and maintain compatibility with existing functionality.

Issue Reference:

Closes #44285

Testing and Validation:

Added unit tests in ~/providers/tests/http/hooks/test_http.py to validate:
- Mounting custom adapters.
- Default behavior when no adapter is provided.
- Handling invalid adapter types.
Ran pytest providers/tests/http/hooks/test_http.py using breeze.
Pass pre-commit run --files providers/src/airflow/providers/http/hooks/http.py providers/tests/http/hooks/test_http.py using breeze.

Backward Compatibility:

This change is backward-compatible. The new adapter parameter is optional and defaults to None, preserving existing behavior.

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

boring-cyborg · 2024-11-23T10:07:49Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
Here are some useful points:

Pay attention to the quality of your code (ruff, mypy and type annotations). Our pre-commits will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
Be sure to read the Airflow Coding style.
Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack

shahar1

Welcome to Apache Airflow and thanks for your contribution!
Looks OK overall, I have some comments :)

providers/src/airflow/providers/http/hooks/http.py

jieyao-MilestoneHub · 2024-11-23T13:03:50Z

Thank you, @shahar1, for your detailed review and valuable feedback! I’ve addressed your comments by:

Adding a detailed docstring for the __init__ method, including the missing adapter parameter.
Removing the redundant instantiation of TCPKeepAliveAdapter in the run method.

Please let me know if there’s anything else I can improve. I really appreciate your time and support!

shahar1 · 2024-11-23T15:20:29Z

Thank you, @shahar1, for your detailed review and valuable feedback! I’ve addressed your comments by:

Adding a detailed docstring for the __init__ method, including the missing adapter parameter.

Removing the redundant instantiation of TCPKeepAliveAdapter in the run method.

Please let me know if there’s anything else I can improve. I really appreciate your time and support!

Thanks :) Regarding the 2nd point - could you please explain why it makes sense to relocate the instanation of TCPKeepAliveAdapter to the __init__?

jieyao-MilestoneHub · 2024-11-24T02:45:06Z

Thanks for the great suggestion! Moving the TCPKeepAliveAdapter to init indeed streamlines the logic—keeps get_conn focused on session setup while ensuring TCP settings are ready upfront. A cleaner and more organized approach, much appreciated! 🙌

In response to the above, I've implemented two changes in this PR:

Moved the instantiation of TCPKeepAliveAdapter to init for better organization and to keep get_conn focused on session setup.
Streamlined the handling of connection extras to make the session configuration more robust and easier to maintain.

Aligned the `get_conn` method with the adjustments specified in apache#44302, including refined handling of headers. Optimized and updated test cases to ensure compatibility and maintain robust test coverage.

- Added `adapter` parameter to `HttpHook` to allow custom HTTP adapters. - Modified `get_conn` to support mounting custom adapters or using TCPKeepAliveAdapter by default. - Added comprehensive tests to validate the functionality of the `adapter` parameter and its integration with `get_conn`. - Ensured all new tests pass and maintain compatibility with existing functionality.

…pter - Added missing `adapter` parameter description to the HttpHook class docstring. - Removed redundant instantiation of `TCPKeepAliveAdapter` in the `run` method since it's already instantiated in `get_conn`.

- Ensured proper mounting of TCP Keep-Alive adapter when enabled. - Improved handling of connection extras for cleaner session configuration.

Aligned the `get_conn` method with the adjustments specified in apache#44302, including refined handling of headers. Optimized and updated test cases to ensure compatibility and maintain robust test coverage.

shahar1

Looks good to me :)
I'll be happy for an additional review though

jieyao-MilestoneHub · 2024-11-24T09:22:48Z

Sure, let's have someone else take a look. Thank you for your suggestion!

providers/src/airflow/providers/http/hooks/http.py

…TPAdapter - Changed the `adapter` parameter to accept only `HTTPAdapter` instead of `BaseAdapter`. - Strengthened `_set_base_url` validation to ensure base_url is constructed with stricter conditions. - Adjusted `_mount_adapters` to improve maintainability.

The `adapter` parameter in `HttpHook` was previously required to be explicitly set to an instance of `HTTPAdapter`. This commit modifies the `__init__` method to assign a default `HTTPAdapter` when no adapter is provided. Changes: - Removed type checks for `adapter`, as default initialization guarantees correctness. - Improved code readability and reduced potential runtime errors. No functional changes beyond defaulting `adapter` to `HTTPAdapter`.

Refactored `HttpHook` to support a custom `HTTPAdapter` through the `adapter` parameter. If no adapter is provided, it defaults to `TCPKeepAliveAdapter` when `tcp_keep_alive=True`. Test: Added `test_custom_adapter` to verify correct adapter mounting.

- Adjust the length of each line of code.

- modify `assert instance` by PEP8

Lee-W · 2024-12-02T11:16:21Z

running pre-commit run --all-files locally can help you resolve issues earlier 🙂

potiuk · 2024-12-02T20:03:48Z

running pre-commit run --all-files locally can help you resolve issues earlier 🙂

Or breeze static-checks --only-my-changes which runs only on files you changed so it is way faster.

jieyao-MilestoneHub · 2024-12-03T02:48:17Z

Thanks for your suggestion!

providers/src/airflow/providers/http/hooks/http.py

Lee-W · 2024-12-03T08:48:34Z

Thanks @jieyao-MilestoneHub ! The PR looks LGTM! I'll keep it for one or two days for folks to check. Please ping me if I forget to get back to you and merge it.

jieyao-MilestoneHub · 2024-12-03T09:18:46Z

Thank you all for taking the time to work on this together! 😊

boring-cyborg · 2024-12-03T10:44:22Z

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

shahar1 · 2024-12-03T18:22:36Z

Thank you all for taking the time to work on this together! 😊

Thank you for your great first contribution :)
Looking forward!

…quests adapters (apache#44302) * feat(http-hook): add adapter parameter to HttpHook and enhance get_conn - Added `adapter` parameter to `HttpHook` to allow custom HTTP adapters. - Modified `get_conn` to support mounting custom adapters or using TCPKeepAliveAdapter by default. - Added comprehensive tests to validate the functionality of the `adapter` parameter and its integration with `get_conn`. - Ensured all new tests pass and maintain compatibility with existing functionality. * fix(http_hook): Update docstring and remove redundant TCPKeepAliveAdapter - Added missing `adapter` parameter description to the HttpHook class docstring. - Removed redundant instantiation of `TCPKeepAliveAdapter` in the `run` method since it's already instantiated in `get_conn`. * fix(http_hook): improve get_conn session setup and TCP adapter logic - Ensured proper mounting of TCP Keep-Alive adapter when enabled. - Improved handling of connection extras for cleaner session configuration. * feat(http): update get_conn logic and corresponding tests (apache#44302) Aligned the `get_conn` method with the adjustments specified in apache#44302, including refined handling of headers. Optimized and updated test cases to ensure compatibility and maintain robust test coverage. * refactor(http_hook): simplify HttpHook by reverting BaseAdapter to HTTPAdapter - Changed the `adapter` parameter to accept only `HTTPAdapter` instead of `BaseAdapter`. - Strengthened `_set_base_url` validation to ensure base_url is constructed with stricter conditions. - Adjusted `_mount_adapters` to improve maintainability. * refactor(http_hook): simplify HttpHook by reverting BaseAdapter to HTTPAdapter - Changed the `adapter` parameter to accept only `HTTPAdapter` instead of `BaseAdapter`. - Strengthened `_set_base_url` validation to ensure base_url is constructed with stricter conditions. - Adjusted `_mount_adapters` to improve maintainability. * Merge: new main * refactor: improve function naming and add type annotations - Changed the function prefix from `_set` to `_configure_session_from` to enhance readability and better reflect its purpose. - Added static type annotations for input parameters and return values. - Included comments to document the design rationale following coding standards. - Improved error message: replaced generic text with detailed and actionable messages. * fix: simplify the change of session - Added a variable `session` after the change of session member * fix: Adjust response format. * fix: simplify the logic * fix(hook): ensure default HTTPAdapter in HttpHook init The `adapter` parameter in `HttpHook` was previously required to be explicitly set to an instance of `HTTPAdapter`. This commit modifies the `__init__` method to assign a default `HTTPAdapter` when no adapter is provided. Changes: - Removed type checks for `adapter`, as default initialization guarantees correctness. - Improved code readability and reduced potential runtime errors. No functional changes beyond defaulting `adapter` to `HTTPAdapter`. * feat(http_hook): add support for custom adapter in initialization Refactored `HttpHook` to support a custom `HTTPAdapter` through the `adapter` parameter. If no adapter is provided, it defaults to `TCPKeepAliveAdapter` when `tcp_keep_alive=True`. Test: Added `test_custom_adapter` to verify correct adapter mounting. * fix: CI image checks / Static checks - Adjust the length of each line of code. * fix: Adjust indent style - modify `assert instance` by PEP8 * fix: ruff error about `from requests.adapters import HTTPAdapter` --------- Co-authored-by: jiao <yhocotw31016@example.com>

boring-cyborg bot added area:providers provider:http labels Nov 23, 2024

jieyao-MilestoneHub force-pushed the feature/http-hook-custom-adapter branch 2 times, most recently from 1697dc2 to 7938e2b Compare November 23, 2024 11:04

jieyao-MilestoneHub mentioned this pull request Nov 23, 2024

allow extending HttpHook with requests adapters #44285

Closed

2 tasks

shahar1 reviewed Nov 23, 2024

View reviewed changes

providers/src/airflow/providers/http/hooks/http.py Outdated Show resolved Hide resolved

providers/src/airflow/providers/http/hooks/http.py Outdated Show resolved Hide resolved

jieyao-MilestoneHub force-pushed the feature/http-hook-custom-adapter branch from f94eca0 to ff2adff Compare November 23, 2024 13:08

shahar1 approved these changes Nov 24, 2024

View reviewed changes

jiao added 4 commits November 24, 2024 16:29

fix(http_hook): improve get_conn session setup and TCP adapter logic

943d2cf

- Ensured proper mounting of TCP Keep-Alive adapter when enabled. - Improved handling of connection extras for cleaner session configuration.

feat(http): update get_conn logic and corresponding tests (apache#44302)

93c23dc

Aligned the `get_conn` method with the adjustments specified in apache#44302, including refined handling of headers. Optimized and updated test cases to ensure compatibility and maintain robust test coverage.

jieyao-MilestoneHub force-pushed the feature/http-hook-custom-adapter branch from 3b5ebf1 to 93c23dc Compare November 24, 2024 08:30

jieyao-MilestoneHub requested a review from shahar1 November 24, 2024 08:32

shahar1 approved these changes Nov 24, 2024

View reviewed changes

Lee-W self-requested a review November 25, 2024 02:41

ashb reviewed Nov 25, 2024

View reviewed changes

providers/src/airflow/providers/http/hooks/http.py Outdated Show resolved Hide resolved

providers/src/airflow/providers/http/hooks/http.py Outdated Show resolved Hide resolved

jiao added 3 commits November 25, 2024 21:11

Merge branch 'main' into feature/http-hook-custom-adapter

151b402

jieyao-MilestoneHub force-pushed the feature/http-hook-custom-adapter branch from 853ad14 to 151b402 Compare November 25, 2024 13:28

jiao added 3 commits November 25, 2024 22:01

Merge branch 'main' into feature/http-hook-custom-adapter

504a77b

Merge: new main

5f63f29

Merge remote-tracking branch 'upstream/main'

1932fb3

jiao added 12 commits November 30, 2024 11:46

Merge remote-tracking branch 'upstream/main'

b3a5506

Merge branch 'main' into feature/http-hook-custom-adapter

0ccaed3

Merge remote-tracking branch 'upstream/main'

dca9209

Merge branch 'main' into feature/http-hook-custom-adapter

2c89df2

fix: CI image checks / Static checks

0a051b3

- Adjust the length of each line of code.

Merge remote-tracking branch 'upstream/main'

980bac0

Merge branch 'main' into feature/http-hook-custom-adapter

42861f7

fix: Adjust indent style

fd82b41

- modify `assert instance` by PEP8

Merge remote-tracking branch 'upstream/main'

23f61b6

Merge branch 'main' into feature/http-hook-custom-adapter

9de57c2

jiao added 3 commits December 3, 2024 11:20

fix: ruff error about from requests.adapters import HTTPAdapter

e8ab104

Merge remote-tracking branch 'upstream/main'

06192d0

Merge branch 'main' into feature/http-hook-custom-adapter

a5428cd

jieyao-MilestoneHub requested a review from Lee-W December 3, 2024 07:51

Lee-W reviewed Dec 3, 2024

View reviewed changes

providers/src/airflow/providers/http/hooks/http.py Show resolved Hide resolved

Lee-W approved these changes Dec 3, 2024

View reviewed changes

potiuk approved these changes Dec 3, 2024

View reviewed changes

potiuk merged commit 71fec4e into apache:main Dec 3, 2024
65 checks passed

jieyao-MilestoneHub deleted the feature/http-hook-custom-adapter branch December 3, 2024 13:12

eladkal mentioned this pull request Dec 22, 2024

Status of testing Providers that were prepared on December 22, 2024 #45148

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Providers/HTTP] Add adapter parameter to HttpHook to allow custom requests adapters #44302

[Providers/HTTP] Add adapter parameter to HttpHook to allow custom requests adapters #44302

jieyao-MilestoneHub commented Nov 23, 2024

boring-cyborg bot commented Nov 23, 2024

shahar1 left a comment

jieyao-MilestoneHub commented Nov 23, 2024

shahar1 commented Nov 23, 2024

jieyao-MilestoneHub commented Nov 24, 2024

shahar1 left a comment

jieyao-MilestoneHub commented Nov 24, 2024

Lee-W commented Dec 2, 2024

potiuk commented Dec 2, 2024

jieyao-MilestoneHub commented Dec 3, 2024

Lee-W commented Dec 3, 2024

jieyao-MilestoneHub commented Dec 3, 2024

boring-cyborg bot commented Dec 3, 2024

shahar1 commented Dec 3, 2024

[Providers/HTTP] Add adapter parameter to HttpHook to allow custom requests adapters #44302

[Providers/HTTP] Add adapter parameter to HttpHook to allow custom requests adapters #44302

Conversation

jieyao-MilestoneHub commented Nov 23, 2024

Summary of Changes:

Issue Reference:

Testing and Validation:

Backward Compatibility:

boring-cyborg bot commented Nov 23, 2024

shahar1 left a comment

Choose a reason for hiding this comment

jieyao-MilestoneHub commented Nov 23, 2024

shahar1 commented Nov 23, 2024

jieyao-MilestoneHub commented Nov 24, 2024

shahar1 left a comment

Choose a reason for hiding this comment

jieyao-MilestoneHub commented Nov 24, 2024

Lee-W commented Dec 2, 2024

potiuk commented Dec 2, 2024

jieyao-MilestoneHub commented Dec 3, 2024

Lee-W commented Dec 3, 2024

jieyao-MilestoneHub commented Dec 3, 2024

boring-cyborg bot commented Dec 3, 2024

shahar1 commented Dec 3, 2024