Skip to content

Commit 79bd3cb

Browse files
committed
fix: review-driven fixes across adapters, config, clock, redirect, digest
- AsyncHttpxHttpClient: close httpx response (via aclose) when Status() rejects an unknown code, releasing the socket back to the pool. - AiohttpHttpClient: use Url.wire_form() to preserve userinfo in outbound requests, matching httpx/requests parity. - AiohttpHttpClient: extract Content-Length from response headers and pass to AsyncResponseBody.from_async_stream — matches httpx/requests. - Configuration: defensive-copy `overrides` dict in __post_init__ so external mutation does not leak past construction. - FakeClock: split wall-clock and monotonic counters; advance() never moves monotonic backwards. - async_redirect: reference shared _REDIRECT_STATUSES constant instead of duplicating the literal set. - DigestChallengeHandler._next_nc: clamp counter to 32 bits so it never overflows the 8-hex-digit RFC 7616 nc format. Regression tests cover each fix.
1 parent 6881b8b commit 79bd3cb

10 files changed

Lines changed: 121 additions & 22 deletions

File tree

packages/dexpace-sdk-core/src/dexpace/sdk/core/config/configuration.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,13 @@ class Configuration:
5656
overrides: dict[str, str] = field(default_factory=dict)
5757
env: EnvSource = field(default=os.environ.get, repr=False)
5858

59+
def __post_init__(self) -> None:
60+
# Defensive copy: ``frozen=True`` protects the attribute binding, not
61+
# the dict's contents. Without this, external callers can mutate
62+
# ``overrides`` after construction and observe the change through
63+
# ``get``.
64+
object.__setattr__(self, "overrides", dict(self.overrides))
65+
5966
# Well-known keys — mirror Java SDK constants where they apply.
6067
# ``ClassVar`` keeps these off the dataclass field list and out of ``__slots__``.
6168
MAX_RETRY_ATTEMPTS: ClassVar[str] = "MAX_RETRY_ATTEMPTS"

packages/dexpace-sdk-core/src/dexpace/sdk/core/http/auth/digest.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,10 @@ def _select(self, challenges: list[AuthenticateChallenge]) -> AuthenticateChalle
159159

160160
def _next_nc(self) -> str:
161161
with self._lock:
162-
self._counter += 1
162+
# Clamp to 32 bits — ``nc`` is rendered as 8 hex digits per
163+
# RFC 7616, and wrapping after 2**32-1 is acceptable since the
164+
# server hashes the value (no monotonic check).
165+
self._counter = (self._counter + 1) & 0xFFFFFFFF
163166
value = self._counter
164167
return f"{value:08x}"
165168

packages/dexpace-sdk-core/src/dexpace/sdk/core/pipeline/policies/async_redirect.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
from ...http.request.method import Method
1414
from ..async_policy import AsyncPolicy
1515
from ..stage import Stage
16-
from .redirect import RedirectPolicy
16+
from .redirect import _REDIRECT_STATUSES, RedirectPolicy
1717

1818
if TYPE_CHECKING:
1919
from ...http.request.request import Request
@@ -65,7 +65,7 @@ async def send(self, request: Request, ctx: PipelineContext) -> AsyncResponse:
6565
if hops >= cfg.max_hops:
6666
return response
6767
status = int(response.status)
68-
if status not in {301, 302, 303, 307, 308}:
68+
if status not in _REDIRECT_STATUSES:
6969
return response
7070
location = response.headers.get("Location")
7171
if location is None or not location.strip():

packages/dexpace-sdk-core/tests/config/test_configuration.py

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -190,3 +190,19 @@ def test_default_env_is_os_environ(monkeypatch: pytest.MonkeyPatch) -> None:
190190
monkeypatch.setenv("DEXPACE_TEST_CONFIG_KEY", "value-from-os")
191191
cfg = Configuration()
192192
assert cfg.get("DEXPACE_TEST_CONFIG_KEY", None) == "value-from-os"
193+
194+
195+
class TestOverrideImmutability:
196+
def test_external_mutation_of_overrides_does_not_leak(self) -> None:
197+
"""A defensive copy means callers can't mutate config after construction."""
198+
original = {"KEY": "initial"}
199+
cfg = Configuration(overrides=original)
200+
original["KEY"] = "mutated-after-construction"
201+
assert cfg.get("KEY") == "initial"
202+
203+
def test_builder_build_takes_defensive_copy(self) -> None:
204+
builder = Configuration.builder().put("KEY", "v1")
205+
cfg = builder.build()
206+
builder.put("KEY", "v2")
207+
# The originally-built config should not see subsequent builder edits.
208+
assert cfg.get("KEY") == "v1"

packages/dexpace-sdk-core/tests/conftest.py

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -28,32 +28,43 @@ def _clean_context_store() -> Iterator[None]:
2828
class FakeClock:
2929
"""Deterministic :class:`~dexpace.sdk.core.util.Clock` for unit tests.
3030
31-
Wall-clock and monotonic readings are backed by the same internal
32-
counter, which advances only via :meth:`sleep` (clamped at zero) or
33-
explicit :meth:`advance` calls. No real time elapses.
31+
Wall-clock (``now``) and monotonic readings are tracked independently so
32+
tests can model the real divergence between them (system clock jumps vs
33+
monotonic uptime). ``sleep`` advances both. ``advance`` advances wall-
34+
clock by the (possibly negative) delta but only ever advances monotonic
35+
forward — monotonic is, by contract, non-decreasing.
3436
"""
3537

36-
__slots__ = ("_t",)
38+
__slots__ = ("_monotonic", "_wall")
3739

3840
def __init__(self, start: float = 0.0) -> None:
39-
"""Initialise the fake clock at ``start`` seconds."""
40-
self._t = start
41+
"""Initialise both clocks at ``start`` seconds."""
42+
self._wall = start
43+
self._monotonic = start
4144

4245
def now(self) -> float:
4346
"""Return the current simulated wall-clock time, in seconds."""
44-
return self._t
47+
return self._wall
4548

4649
def monotonic(self) -> float:
4750
"""Return the current simulated monotonic reading, in seconds."""
48-
return self._t
51+
return self._monotonic
4952

5053
def sleep(self, duration: float) -> None:
51-
"""Advance the clock by ``duration`` seconds, clamped at zero."""
52-
self._t += max(0.0, duration)
54+
"""Advance both wall and monotonic by ``duration`` (clamped at zero)."""
55+
delta = max(0.0, duration)
56+
self._wall += delta
57+
self._monotonic += delta
5358

5459
def advance(self, duration: float) -> None:
55-
"""Advance the clock by ``duration`` seconds (may be negative)."""
56-
self._t += duration
60+
"""Advance wall-clock by ``duration`` (may be negative).
61+
62+
Monotonic is only advanced when ``duration`` is positive — modelling
63+
the platform guarantee that monotonic time never goes backwards.
64+
"""
65+
self._wall += duration
66+
if duration > 0:
67+
self._monotonic += duration
5768

5869

5970
@pytest.fixture

packages/dexpace-sdk-core/tests/util/test_clock.py

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,3 +127,19 @@ def test_fake_clock_advance(fake_clock: FakeClock) -> None:
127127
def test_fake_clock_satisfies_clock_protocol(fake_clock: FakeClock) -> None:
128128
"""``FakeClock`` is structurally a :class:`Clock`."""
129129
assert isinstance(fake_clock, Clock)
130+
131+
132+
def test_fake_clock_monotonic_does_not_decrease_on_negative_advance() -> None:
133+
"""``advance(-delta)`` moves wall back but monotonic is preserved."""
134+
clock = FakeClock(start=10.0)
135+
clock.advance(-3.0)
136+
assert clock.now() == pytest.approx(7.0)
137+
# Monotonic time never goes backwards on real systems.
138+
assert clock.monotonic() == pytest.approx(10.0)
139+
140+
141+
def test_fake_clock_sleep_advances_both_wall_and_monotonic() -> None:
142+
clock = FakeClock()
143+
clock.sleep(5.0)
144+
assert clock.now() == pytest.approx(5.0)
145+
assert clock.monotonic() == pytest.approx(5.0)

packages/dexpace-sdk-http-aiohttp/src/dexpace/sdk/http/aiohttp/client.py

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ async def execute(self, request: Request) -> AsyncResponse:
7878
try:
7979
ctx = session.request(
8080
method=str(request.method),
81-
url=str(request.url),
81+
url=request.url.wire_form(),
8282
headers=_request_headers(request.headers),
8383
data=data,
8484
timeout=timeout_cfg,
@@ -161,7 +161,10 @@ def _wrap_response(request: Request, aio_response: aiohttp.ClientResponse) -> As
161161
) from err
162162
headers = Headers(tuple(aio_response.headers.items()))
163163
reason = aio_response.reason
164-
body = AsyncResponseBody.from_async_stream(_StreamReaderAdapter(aio_response))
164+
content_length = _content_length(aio_response)
165+
body = AsyncResponseBody.from_async_stream(
166+
_StreamReaderAdapter(aio_response), content_length=content_length
167+
)
165168
return AsyncResponse(
166169
request=request,
167170
protocol=Protocol.HTTP_1_1,
@@ -204,4 +207,15 @@ async def close(self) -> None:
204207
self._response.release()
205208

206209

210+
def _content_length(aio_response: aiohttp.ClientResponse) -> int:
211+
"""Extract ``Content-Length`` from the response, or ``-1`` when absent/invalid."""
212+
raw = aio_response.headers.get("Content-Length")
213+
if raw is None:
214+
return -1
215+
try:
216+
return max(0, int(raw))
217+
except ValueError:
218+
return -1
219+
220+
207221
__all__ = ["AiohttpHttpClient"]

packages/dexpace-sdk-http-aiohttp/tests/test_aiohttp_client.py

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,3 +157,13 @@ async def test_shared_session_is_not_closed_on_aclose() -> None:
157157
assert not session.closed
158158
finally:
159159
await session.close()
160+
161+
162+
async def test_content_length_extracted_from_response(base_url: str) -> None:
163+
"""Aiohttp client should populate AsyncResponseBody.content_length from headers."""
164+
async with AiohttpHttpClient() as client:
165+
request = Request(method=Method.GET, url=Url.parse(f"{base_url}/ok"))
166+
async with await client.execute(request) as response:
167+
assert response.body is not None
168+
# /ok returns a small JSON; Content-Length should be set by aiohttp.
169+
assert response.body.content_length() > 0

packages/dexpace-sdk-http-httpx/src/dexpace/sdk/http/httpx/async_.py

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ async def execute(self, request: Request) -> AsyncResponse:
111111
raise ServiceRequestError(str(err), error=err) from err
112112
except httpx.RequestError as err:
113113
raise ServiceRequestError(str(err), error=err) from err
114-
return _build_async_response(request, httpx_response)
114+
return await _build_async_response(request, httpx_response)
115115

116116
def _build_request(self, request: Request) -> httpx.Request:
117117
headers = _headers_to_pairs(request.headers)
@@ -200,13 +200,13 @@ async def close(self) -> None:
200200
await self._response.aclose()
201201

202202

203-
def _build_async_response(request: Request, httpx_response: httpx.Response) -> AsyncResponse:
203+
async def _build_async_response(request: Request, httpx_response: httpx.Response) -> AsyncResponse:
204204
try:
205205
status = Status(int(httpx_response.status_code))
206206
except ValueError as err:
207-
# ``aclose`` is async; schedule a synchronous close fallback via
208-
# the underlying response's ``close`` since we cannot await here.
209-
# Best effort — the failing status code is the user-visible signal.
207+
# Release the underlying socket back to the pool; httpx async
208+
# responses reject sync ``close``, so use ``aclose``.
209+
await httpx_response.aclose()
210210
raise ServiceResponseError(
211211
f"Unknown status code: {httpx_response.status_code}", error=err
212212
) from err

packages/dexpace-sdk-http-httpx/tests/test_async_httpx_client.py

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,3 +185,25 @@ def handler(request: httpx.Request) -> httpx.Response:
185185
request = Request(method=Method.GET, url=Url.parse("http://example.test/"))
186186
async with await client.execute(request) as response:
187187
assert response.reason == "OK"
188+
189+
190+
async def test_async_unknown_status_closes_response() -> None:
191+
"""When Status() rejects an unknown code, the response must be released."""
192+
from dexpace.sdk.core.errors import ServiceResponseError
193+
194+
closed = {"yes": False}
195+
196+
class _TrackedResponse(httpx.Response):
197+
async def aclose(self) -> None:
198+
closed["yes"] = True
199+
await super().aclose()
200+
201+
def handler(request: httpx.Request) -> httpx.Response:
202+
return _TrackedResponse(999, content=b"")
203+
204+
transport = httpx.MockTransport(handler)
205+
async with AsyncHttpxHttpClient(transport=transport) as client:
206+
request = Request(method=Method.GET, url=Url.parse("http://example.test/"))
207+
with pytest.raises(ServiceResponseError):
208+
await client.execute(request)
209+
assert closed["yes"], "Response should be closed when status mapping fails"

0 commit comments

Comments
 (0)