Customization of httpx.AsyncClient behaviour #185
Conversation
|
Thanks a lot! It looks good to me, but I feel like it's a lot of changes to do something rather simple, don't you think? It feels a little bit weird (though I understand that it's due to the client being instantiated on multiple places) that some methods should ask for a httpx client, a common user of the library should probably be a little bit shielded from that. I am seriously considering using a factory method instead, you could initialize SSOBase with a http client factory, what do you think? Am I overthinking this? 😄 |
|
The factory method is a better approach. I was in a rush because I wanted to finish the implementation before my holidays (which also explains the delay in the answer), and I thought that was good enough. However, I did not have the complete picture as I only use Microsoft SSO. |
WalkthroughThe changes enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FastAPI
participant HttpxClient
User->>FastAPI: Initiate Login
FastAPI->>HttpxClient: Call process_login with parameters
HttpxClient-->>FastAPI: Return response
FastAPI->>User: Send login response
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
fastapi_sso/sso/base.py (1)
314-318: Enhance documentation forhttpx_client_kwargsThe documentation for the
httpx_client_kwargsparameter is minimal across the methods where it's introduced. Given the importance of this parameter for configuring the HTTP client, detailed documentation would help developers understand how to use it effectively. It would be beneficial to include examples of common configurations, such as setting up proxies or disabling SSL verification, in the docstrings.Also applies to: 326-330, 340-341, 351-351, 361-361, 386-386, 418-423, 432-437, 442-452, 463-464, 477-477, 497-497, 530-530
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- fastapi_sso/sso/base.py (12 hunks)
Additional comments not posted (2)
fastapi_sso/sso/base.py (2)
23-40: New TypedDictHttpxClientKwargsTypefor HTTP client configurationThe introduction of
HttpxClientKwargsTypeis a positive change as it provides a structured way to pass HTTP client configurations. This approach enhances type safety and makes the API more expressive and easier to use correctly. The fields cover common configurations like SSL verification, client certificates, proxy settings, and timeouts, which are essential for flexible HTTP client operations.
314-318: Verify and enhance error handlingThe addition of
httpx_client_kwargsallows for more flexible HTTP client configurations, which is a significant improvement. However, the error handling related to HTTP client interactions seems minimal. Given the complexity and variety of network-related errors that can occur, especially with proxies and SSL configurations, robust error handling is crucial.It would be beneficial to add more comprehensive error handling around the HTTP client interactions to manage exceptions like
httpx.HTTPError, timeouts, and other network issues. This would improve the robustness of the SSO functionality and provide clearer error messages to the users.Also applies to: 326-330, 340-341, 351-351, 361-361, 386-386, 418-423, 432-437, 442-452, 463-464, 477-477, 497-497, 530-530
fastapi_sso/sso/base.py
Outdated
| params: Optional[Dict[str, Any]], | ||
| headers: Optional[Dict[str, Any]], | ||
| redirect_uri: Optional[str], | ||
| convert_response: Literal[True], | ||
| httpx_client_kwargs: Optional[HttpxClientKwargsType], |
There was a problem hiding this comment.
Refactor suggestion: Consolidate httpx_client_kwargs handling
The httpx_client_kwargs parameter has been added across multiple methods (verify_and_process, process_login) to customize the HTTP client behavior. While this addition is beneficial, the handling of this parameter is scattered and repetitive across methods. This could be streamlined by centralizing the creation and configuration of the httpx.AsyncClient instance, possibly using a factory method or a helper function within the class. This would reduce redundancy and improve maintainability.
Consider implementing a method like get_http_client that takes httpx_client_kwargs and returns a configured httpx.AsyncClient instance. This method can then be used across all functions needing an HTTP client.
Also applies to: 326-330, 340-341, 351-351, 361-361, 386-386, 418-423, 432-437, 442-452, 463-464, 477-477, 497-497, 530-530
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- fastapi_sso/sso/base.py (9 hunks)
- tests/test_providers.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- fastapi_sso/sso/base.py
Additional context used
Ruff
tests/test_providers.py
141-141: Line too long (121 > 120)
(E501)
Additional comments not posted (1)
tests/test_providers.py (1)
153-153: Good encapsulation practice in test setup.The change to use
monkeypatch.setattron theget_async_clientmethod of thessoobject is a good practice. It improves the encapsulation and modularity of the test setup, making it easier to maintain and adapt to changes in the underlying client setup.
tests/test_providers.py
Outdated
| @@ -138,8 +138,7 @@ async def test_login_url_scope_additional(self, Provider: Type[SSOBase]): | |||
| async def test_process_login(self, Provider: Type[SSOBase], monkeypatch: pytest.MonkeyPatch): | |||
| sso = Provider("client_id", "client_secret") | |||
| FakeAsyncClient = make_fake_async_client( | |||
| returns_post=Response(url="https://localhost", json_content={"access_token": "token"}), | |||
| returns_get=Response( | |||
| returns_post=Response(url="https://localhost", json_content={"access_token": "token"}), returns_get=Response( | |||
There was a problem hiding this comment.
Consider breaking long lines for better readability.
This line exceeds the recommended length of 120 characters, which can affect readability and maintainability. Consider breaking it into multiple lines.
Apply this diff to break the line into a more manageable length:
-returns_post=Response(url="https://localhost", json_content={"access_token": "token"}), returns_get=Response(
+returns_post=Response(url="https://localhost", json_content={"access_token": "token"}),
+returns_get=Response(Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| returns_post=Response(url="https://localhost", json_content={"access_token": "token"}), returns_get=Response( | |
| returns_post=Response(url="https://localhost", json_content={"access_token": "token"}), | |
| returns_get=Response( |
Tools
Ruff
141-141: Line too long (121 > 120)
(E501)
|
Hello again! I think it's better this way and I hope @tomasvotava thinks the same way. Thank you again. |
santibreo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- fastapi_sso/sso/base.py (9 hunks)
- tests/test_providers.py (1 hunks)
Files skipped from review due to trivial changes (1)
- tests/test_providers.py
Files skipped from review as they are similar to previous changes (1)
- fastapi_sso/sso/base.py
Argument `get_httpx_client` incorporated to `SSOBase` to allow customization of `httpx.AsyncClient` used to call auth provider
First of all, congratz for this awesome library.
I was using it for a non-public application, and I encountered issues when using it behind a proxy and also related with SSL certificates, everything related with the
httpx.AsyncClientused here so I just added a new parameter with keyword arguments that is passed directly to that class (that is the reason why I do not have added any tests).Additionally, I have solved some typing issues, but they were related to
pyrightand you might not want to incorporate those.That's it. Thank you for your work 😄
Summary by CodeRabbit
New Features
Improvements
verify_and_processandprocess_loginmethods to require explicit values for several parameters, enhancing clarity.convert_responseparameter.Tests