Skip to content
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

Support Callable as http_auth type in AIOHttpConnection #892

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

superleesa
Copy link

@superleesa superleesa commented Feb 22, 2025

Description

Support Callable http_auth in AIOHttpConnection, similar to how Urllib3HttpConnection handles http_auth.

Reference

Urllib3HttpConnection handles Callable http_auth.

if self.http_auth is not None:
if isinstance(self.http_auth, Callable): # type: ignore
pass
elif isinstance(self.http_auth, (tuple, list)):
self.headers.update(
urllib3.make_headers(basic_auth=":".join(http_auth))
)
else:
self.headers.update(urllib3.make_headers(basic_auth=http_auth))

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@superleesa superleesa changed the title Support Callable and tupe/list as http_auth types in AIOHttpConnection Support Callable as http_auth types in AIOHttpConnection Feb 22, 2025
@superleesa superleesa changed the title Support Callable as http_auth types in AIOHttpConnection Support Callable as http_auth type in AIOHttpConnection Feb 22, 2025
Copy link

codecov bot commented Feb 22, 2025

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 70.38%. Comparing base (ba715b9) to head (f06d442).
Report is 82 commits behind head on main.

Files with missing lines Patch % Lines
opensearchpy/_async/http_aiohttp.py 75.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #892      +/-   ##
==========================================
- Coverage   71.95%   70.38%   -1.57%     
==========================================
  Files          91      125      +34     
  Lines        8001     9294    +1293     
==========================================
+ Hits         5757     6542     +785     
- Misses       2244     2752     +508     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dblock
Copy link
Member

dblock commented Feb 22, 2025

I like this, needs tests and a CHANGELOG.

Signed-off-by: Satoshi Kashima <6world4trigger@gmail.com>
@superleesa superleesa force-pushed the feature/support-different-formats-of-http_auth branch from 79de2f9 to f06d442 Compare February 26, 2025 17:58
@superleesa
Copy link
Author

@dblock
Hi! I was reading the codebase to add tests, but noticed that AsyncHTTPConnection already supports Callable http_auth. Why is this not the default connection in AsyncTransport? ref:

DEFAULT_CONNECTION_CLASS = AIOHttpConnection

Another thing is, the code for AsyncHTTPConnection and AIOHTTPConnection seem very similar, with only differences being 1) supports Callable http_auth, 2) support setting trust_env, 3) uses yarl. Most parts seem copied and pasted. Are there any reasons for this? If not, I thought maybe I could port most parts of the code in AsyncHTTPConnection to AIOHTTPConnection for maintainability.

@dblock
Copy link
Member

dblock commented Feb 26, 2025

@superleesa The reasons are likely completely historical, feel free to do the right thing (TM) and fix this.

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

Successfully merging this pull request may close these issues.

None yet

2 participants