Merged
Conversation
Contributor
Reviewer's GuideRefactors httpx client construction to always pass a single proxy string via a shared helper, updates the few direct httpx.Client call sites to use it, and adds focused tests for the new behavior. Class diagram for HttpxTransport and httpx.Client constructionclassDiagram
class HttpxTransport {
+timeout
+proxies
+proxy
+verify
-_client
+HttpxTransport(timeout, proxies, proxy, verify, client)
+request(method, url, params, data, content, headers)
}
class _httpx_client_kwargs {
<<function>>
+_httpx_client_kwargs(proxies, verify, timeout) dict
}
class DataKiosk {
+proxies
+verify
+get_document(download, file, **kwargs)
}
class Reports {
+proxies
+verify
+get_report_document(download, file, timeout, stream, **kwargs)
}
HttpxTransport ..> _httpx_client_kwargs : uses for httpx.Client kwargs
DataKiosk ..> _httpx_client_kwargs : uses for httpx.Client kwargs
Reports ..> _httpx_client_kwargs : uses for httpx.Client kwargs
class httpx_Client {
+Client(proxy, verify, timeout)
+get(url)
+stream(method, url)
}
HttpxTransport ..> httpx_Client : constructs
DataKiosk ..> httpx_Client : constructs with _httpx_client_kwargs
Reports ..> httpx_Client : constructs with _httpx_client_kwargs
Flow diagram for _httpx_client_kwargs proxy and timeout selectionflowchart TD
A_start[Start _httpx_client_kwargs] --> B_init[Initialize client_kwargs with verify]
B_init --> C_timeout_check{Is timeout provided?}
C_timeout_check -- Yes --> D_set_timeout[Set client_kwargs.timeout]
C_timeout_check -- No --> E_skip_timeout[Skip setting timeout]
D_set_timeout --> F_proxies_check{Are proxies provided?}
E_skip_timeout --> F_proxies_check
F_proxies_check -- No --> J_return_no_proxy[Return client_kwargs without proxy]
F_proxies_check -- Yes --> G_type_check{Are proxies list or tuple?}
G_type_check -- Yes --> H_choice[Select proxy_value with random.choice]
G_type_check -- No --> I_direct[Set proxy_value to proxies]
H_choice --> K_set_proxy[Set client_kwargs.proxy to proxy_value]
I_direct --> K_set_proxy
K_set_proxy --> L_return_with_proxy[Return client_kwargs with proxy]
J_return_no_proxy --> M_end[End]
L_return_with_proxy --> M_end
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Consider handling the case where
proxiesis an empty list/tuple in_httpx_client_kwargs, sincerandom.choice([])will raise anIndexErrorand could surface as a hard-to-diagnose runtime error. - It may be helpful to add simple type hints to
_httpx_client_kwargs(particularly forproxiesand the return type) to make the expected proxy formats and behavior clearer at call sites inHttpxTransport,DataKiosk, andReports.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider handling the case where `proxies` is an empty list/tuple in `_httpx_client_kwargs`, since `random.choice([])` will raise an `IndexError` and could surface as a hard-to-diagnose runtime error.
- It may be helpful to add simple type hints to `_httpx_client_kwargs` (particularly for `proxies` and the return type) to make the expected proxy formats and behavior clearer at call sites in `HttpxTransport`, `DataKiosk`, and `Reports`.
## Individual Comments
### Comment 1
<location> `sp_api/base/_transport_httpx.py:6-11` </location>
<code_context>
import httpx
+def _httpx_client_kwargs(*, proxies=None, verify=True, timeout=None):
+ client_kwargs = {"verify": verify}
+ if timeout is not None:
+ client_kwargs["timeout"] = timeout
+ if proxies is not None:
+ if isinstance(proxies, (list, tuple)):
+ proxy_value = random.choice(proxies)
+ else:
</code_context>
<issue_to_address>
**issue:** Guard against empty proxy sequences before calling `random.choice`.
If `proxies` is an empty list/tuple, `random.choice(proxies)` will raise `IndexError`. Consider either skipping proxy configuration when the sequence is empty or raising a clearer error by validating that it’s non-empty (e.g. `if proxies:`) before calling `random.choice` or at the call site.
</issue_to_address>
### Comment 2
<location> `tests/api/test_httpx_transport.py:3` </location>
<code_context>
+import httpx
+
from sp_api.base import Client, sp_endpoint, fill_query_params, ApiResponse
+from sp_api.base._transport_httpx import _httpx_client_kwargs
</code_context>
<issue_to_address>
**suggestion (testing):** Add unit tests that exercise `HttpxTransport` itself, not just the helper function
These tests only cover `_httpx_client_kwargs`, while the behavior change is in `HttpxTransport`. Please add tests that construct `HttpxTransport` with various `proxy`/`proxies`/`timeout`/`verify` combinations and assert that `httpx.Client` is called with the expected kwargs (e.g., by monkeypatching `httpx.Client`). This will verify the helper is wired correctly and that proxy precedence and defaults behave as intended.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+6
to
+11
| def _httpx_client_kwargs(*, proxies=None, verify=True, timeout=None): | ||
| client_kwargs = {"verify": verify} | ||
| if timeout is not None: | ||
| client_kwargs["timeout"] = timeout | ||
| if proxies is not None: | ||
| if isinstance(proxies, (list, tuple)): |
Contributor
There was a problem hiding this comment.
issue: Guard against empty proxy sequences before calling random.choice.
If proxies is an empty list/tuple, random.choice(proxies) will raise IndexError. Consider either skipping proxy configuration when the sequence is empty or raising a clearer error by validating that it’s non-empty (e.g. if proxies:) before calling random.choice or at the call site.
| @@ -0,0 +1,24 @@ | |||
| import random | |||
|
|
|||
| from sp_api.base._transport_httpx import _httpx_client_kwargs | |||
Contributor
There was a problem hiding this comment.
suggestion (testing): Add unit tests that exercise HttpxTransport itself, not just the helper function
These tests only cover _httpx_client_kwargs, while the behavior change is in HttpxTransport. Please add tests that construct HttpxTransport with various proxy/proxies/timeout/verify combinations and assert that httpx.Client is called with the expected kwargs (e.g., by monkeypatching httpx.Client). This will verify the helper is wired correctly and that proxy precedence and defaults behave as intended.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
httpx.Clientby always passing a single proxy string becausehttpxexpects aproxystring rather than a mapping.Description
_httpx_client_kwargs(*, proxies=None, verify=True, timeout=None)insp_api/base/_transport_httpx.pythat returns adictwith a singleproxystring, selecting one at random ifproxiesis alistortupleand otherwise using the provided value.HttpxTransportconstructs its client via**_httpx_client_kwargs(...)soproxyis always a single string.sp_api/api/reports/reports.pyandsp_api/api/data_kiosk/data_kiosk.pyto use**_httpx_client_kwargs(...)instead of passingproxiesdirectly.tests/api/test_httpx_transport.pycovering a single proxy string and selection from a list viarandom.choice(monkeypatched in the test).Testing
pytest tests/api/test_httpx_transport.pyand the suite passed with2 passedtests.Codex Task
Summary by Sourcery
Unify httpx client proxy handling around a single proxy value and apply it consistently across HTTP transport and synchronous document downloads.
Enhancements:
Tests: