Repository navigation
Conversation
for more information, see https://pre-commit.ci
alexanto
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
Transport-contract gaps break ETag caching, raise unexpectedly after retry exhaustion, corrupt non-UTF-8 feature data, and allow injected network failures to bypass metric restoration or abort initialization.
4 open issues
What changed in this PR
Extracts HTTP I/O from the synchronous API client into a transport layer, preparing the SDK for custom transports while keeping RequestsTransport as the default.
Changes:
- Adds request and response models, a transport protocol, and a requests-based implementation.
- Routes feature fetching, registration, and metrics submission through the transport.
- Adds tests for transport injection, request handling, retries, and session isolation.
| File | Description |
|---|---|
UnleashClient/_unleash_api_client.py |
Delegates HTTP calls to an injectable transport and interprets its responses. |
UnleashClient/_transport.py |
Defines transport interfaces and the default requests implementation. |
tests/unit_tests/test_unleash_api_client.py |
Tests transport delegation for all three API operations. |
tests/unit_tests/test_transport.py |
Tests request forwarding, responses, retries, errors, and session isolation. |
Review effort: Balanced
| status: int | ||
| headers: Mapping[str, str] | ||
| body: bytes |
| if request.method == "GET" and self._retries is not None: | ||
| adapter = HTTPAdapter( | ||
| max_retries=Retry( | ||
| total=self._retries, status_forcelist=[500, 502, 504] |
There was a problem hiding this comment.
legit, and this is no behavior change at this point, urllib3's Retry raises RetryError once retries run out on a 5xx
in #472 removes the old retry mechanism and for both clients the new shared retry returns the last response after it ran out
|
|
||
|
|
||
| def _text(resp: Response) -> str: | ||
| return resp.body.decode("utf-8", errors="replace") |
There was a problem hiding this comment.
i am not entirely sure how much of a real life scenario this is, request also uses utf-8 be default and I think unleash server also sends utf-8
@ppicom maybe you can help me out with the legitimacy of this?
if yes, I will fix it in a separate PR
| headers=self._headers.metrics(), | ||
| timeout=config.http.timeout, | ||
| **config.http.options, | ||
| resp = self._transport.execute( |
There was a problem hiding this comment.
this is also legit and also fixed in a later PR
i tried to keep changes as small as possible, so some PRs are kind of intermediary states :/
register() and send_metrics() log and return False, and fetch reports UNAVAILABLE in case of any error from execute()
extracting the i/o from the api_client as transport. this is the first step towards a custom HTTP transport.
so far really just refactoring, and RequestsTransport is going to be the default transport implementation for the sync client