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

Fix string/tuple/no auth on AsyncHttpConnection class #424

Merged
merged 14 commits into from
Jul 6, 2023

Conversation

dannosaur
Copy link
Contributor

Description

Fixes an error and confusing code surrounding authentication on the AsyncHttpConnection class. aiohttp wants a BasicAuth instance instead of a tuple/colon-separated-string as requests does, so this accommodates what it wants also. The code in the __init__ was lifted from the RequestsHttpConnection class as that's already a sensible implementation, but the way the resulting auth needs to be passed with the async session is slightly different.

Issues Resolved

Closes #283

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.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

This needs tests for the input scenarios between tuple, strings, etc. Also is this the same problem not in async?

opensearchpy/connection/http_async.py Show resolved Hide resolved
opensearchpy/connection/http_async.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 28, 2023

Codecov Report

Merging #424 (d104ec4) into main (2e8d5ce) will increase coverage by 0.61%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #424      +/-   ##
==========================================
+ Coverage   70.87%   71.48%   +0.61%     
==========================================
  Files          81       81              
  Lines        7664     7667       +3     
==========================================
+ Hits         5432     5481      +49     
+ Misses       2232     2186      -46     
Impacted Files Coverage Δ
opensearchpy/connection/http_async.py 53.60% <100.00%> (+38.84%) ⬆️

@dannosaur
Copy link
Contributor Author

This needs tests for the input scenarios between tuple, strings, etc. Also is this the same problem not in async?

Sync mode works just fine. When I was switching my project over to async and it refused to authenticate I went digging.

@dannosaur
Copy link
Contributor Author

I'll push changes and tests for this tomorrow.

@dblock
Copy link
Member

dblock commented Jun 28, 2023

I'll push changes and tests for this tomorrow.

Thank you for helping out @dannosaur! No rush.

@dannosaur
Copy link
Contributor Author

Updated. One question. How do I omit my email from this required DCO? I don't want that out in the public domain.

…ch-project#283

Signed-off-by: dannosaur <461956+dannosaur@users.noreply.github.com>
Signed-off-by: dannosaur <461956+dannosaur@users.noreply.github.com>
@dannosaur
Copy link
Contributor Author

How do I omit my email from this required DCO? I don't want that out in the public domain.

Nevermind. Found the Github stuff for hiding email addresses and rebased/did the DCO once again.

New to this :)

@dblock
Copy link
Member

dblock commented Jun 30, 2023

New to this :)

🤔 🦖 ;)

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Add to CHANGELOG, and consider the other suggestions? LGTM otherwise.

opensearchpy/connection/http_async.py Show resolved Hide resolved
test_opensearchpy/test_connection.py Outdated Show resolved Hide resolved
Also had to install asynctest into the dev-requirements to get access to the context managers necessary to mock out aiohttp.

Signed-off-by: dannosaur <461956+dannosaur@users.noreply.github.com>
Signed-off-by: dannosaur <461956+dannosaur@users.noreply.github.com>
Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>
@dblock
Copy link
Member

dblock commented Jun 30, 2023

Linter is complaining :( https://github.com/opensearch-project/opensearch-py/actions/runs/5427802648/jobs/9871401633?pr=424

I'm ok to merge it as is, lmk when it's ready and tests are passing.

Signed-off-by: dannosaur <461956+dannosaur@users.noreply.github.com>
Signed-off-by: dannosaur <461956+dannosaur@users.noreply.github.com>
@dblock
Copy link
Member

dblock commented Jul 5, 2023

This looks good! Thanks for hanging in here. Linter/CI are still failing, will you please fix?

I changed an option on this repo and hopefully CI will auto-run on this PR for you when you push updates.

Signed-off-by: dannosaur <461956+dannosaur@users.noreply.github.com>
@dannosaur
Copy link
Contributor Author

Looks like the asynctest package isn't compatible with Python >3.8 (my bad, I'll redo the new tests that are failing as a result of this), a bit more research suggests that unittest now has a lot of the things that that package implemented, so I'll play with those.

Also, on the Python 3.7 front, it appears the IsolatedAsyncioTestCase class was added to unittest in Python 3.8. Is it acceptable to disable those tests on Python <3.8, or would you prefer that I explore options to have them run in all supported Python version?

@dblock
Copy link
Member

dblock commented Jul 5, 2023

Also, on the Python 3.7 front, it appears the IsolatedAsyncioTestCase class was added to unittest in Python 3.8. Is it acceptable to disable those tests on Python <3.8, or would you prefer that I explore options to have them run in all supported Python version?

Ideally all tests should run in all versions unless it's not possible (e.g. uses a feature of Python 3.8+). In that case I would accept an open issue and disabling those tests for <3.8 with a proper explanation.

Signed-off-by: dannosaur <461956+dannosaur@users.noreply.github.com>
Signed-off-by: dannosaur <461956+dannosaur@users.noreply.github.com>
Signed-off-by: dannosaur <461956+dannosaur@users.noreply.github.com>
@dannosaur
Copy link
Contributor Author

Ideally all tests should run in all versions unless it's not possible (e.g. uses a feature of Python 3.8+). In that case I would accept an open issue and disabling those tests for <3.8 with a proper explanation.

Yeah, OK. I solved the 3.7 issue, but now 3.5 is complaining. I'll open a separate issue to talk about supported Python version before going any further with this.

…sts are ignored on runners <3.6

Signed-off-by: dannosaur <461956+dannosaur@users.noreply.github.com>
Signed-off-by: dannosaur <461956+dannosaur@users.noreply.github.com>
@dannosaur
Copy link
Contributor Author

dannosaur commented Jul 5, 2023

Woohoo!

@dblock I did open the issue about the older versions of Python, however I spotted that every other async-related test a) lives in the test_async directory, and b) is skipped for Python <3.6. So I moved the file, and it appears that the entire suite of checks is happy across the board. I simplified it somewhat as well, not relying on external libraries and pretty much copied the same format as the other tests were doing. The issue on removing support for long-deprecated Python versions still remain, so that discussion can happen elsewhere, and doesn't block this PR any more.

Let me know if you need anything else.

@dblock dblock merged commit 12ebe82 into opensearch-project:main Jul 6, 2023
45 checks passed
@dblock
Copy link
Member

dblock commented Jul 6, 2023

Great job 👏

@dannosaur dannosaur deleted the fix-async-http-auth branch July 6, 2023 22:29
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.

[BUG] initializing an async client without http_auth results in an error
2 participants