From 458d86813fe7948bc98e27cf88e336b0843f1adc Mon Sep 17 00:00:00 2001 From: Matthew Spah Date: Sat, 22 Aug 2026 18:20:58 -0700 Subject: [PATCH] refactor(async): move retries and client ownership onto a shared transport AsyncMlb's ownership pointed the wrong way: __init__ built the v1 adapter with client=client, let that adapter create the shared httpx.AsyncClient, then reached back into self._mlb_adapter_v1._client to construct the v1.1 adapter. aclose() delegated the parent's shutdown to one of its children, with the other child required not to close the thing it shares. Underneath that, AsyncMlbDataAdapter._owns_client answered two different questions at once ("who closes this client" and "am I allowed to retry"), which is why _set_retries_enabled() existed at all: to undo the wrong conclusion the v1.1 adapter reached about retry eligibility after receiving a non-None client. Mlb does not have this problem because it does not implement retries -- it configures them, once, by mounting an HTTPAdapter carrying the retry policy onto the Session it creates. HTTPX has the same extension seam: AsyncClient(transport=...) accepts any AsyncBaseTransport, the position HTTPAdapter occupies in Requests. Moving the retry loop there makes retries a property of the client, so two adapters sharing one client share one policy by construction and cannot disagree about it. Adds a private mlbstatsapi/_async_transport.py: - MlbAsyncRetryTransport wraps an inner transport (default httpx.AsyncHTTPTransport()) with the existing create_retry_policy() budget and backoff accounting, moved verbatim from AsyncMlbDataAdapter._request_with_retries /_sleep_before_retry. On an exhausted budget it re-raises the underlying httpx exception rather than translating it -- transports are contractually expected to raise httpx errors, translation stays with the adapter. - create_library_async_client() is the async counterpart of _configure_library_session(): builds a client with the package User-Agent and MlbAsyncRetryTransport() mounted. AsyncMlb.__init__ now owns the client exactly as Mlb.__init__ owns the Session (self._owns_client, self._client, self._closed, created via create_library_async_client() when the caller passes none) and hands that one client to both adapters. aclose() closes self._client directly instead of delegating to the v1 adapter. AsyncMlbDataAdapter's public constructor is unchanged. get() now calls self._client.get() directly, wrapped in the same two except clauses the sync adapter's error path mirrors (TimeoutException -> MlbTimeoutError, RequestError -> MlbTransportError; ConnectTimeout/ConnectError fall into the right one because ConnectTimeout is a TimeoutException subclass). Deleted _set_retries_enabled, _retries_enabled, _retry_policy, _request_with_retries, and _sleep_before_retry. Retargets the test seam from patching httpx.AsyncClient to patching httpx.AsyncHTTPTransport inside _async_transport, so tests exercise the real client, the real retry transport, and the real headers with a MockTransport at the bottom. _owned_adapter/_injected_adapter keep their names; a new _retry_policy_of() accessor reads the policy from the client's transport for tests that mutate it. Replaces the two _set_retries_enabled tests with three that assert the new shape: a library-created client mounts MlbAsyncRetryTransport, an injected client's transport is left exactly as supplied, and mounting MlbAsyncRetryTransport on an injected client makes it retry (the caller-facing opt-in). In test_async_mlb.py, ownership assertions move from the adapters to AsyncMlb itself, and the cleanup-failure test mocks mlb.aclose rather than an adapter's. Testing: - poetry run pytest tests/ --ignore=tests/external_tests: 973 passed before this change (checked out from the unmodified branch tip), 974 after (net +1: two _set_retries_enabled tests removed, three new transport-ownership tests added). - tests/test_sync_async_parity.py: 96 passed, unchanged -- AsyncMlb's observable behavior did not move. - tests/external_tests/async_mlb/: 26 passed against the live API. Risk: internal-only change to a private transport layer behind AsyncMlb's and AsyncMlbDataAdapter's unchanged public constructors. Retry budgets, backoff timing, exception mapping, 404/strict_http behavior, the User-Agent, and caller-injected-client ownership rules are all covered by tests and were not changed on purpose. Intentionally left out: MlbAsyncRetryTransport stays private. Exporting it would turn "an injected client gets no library retries" from a limitation into a documented opt-in, which is a public-API decision -- see the PR description. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01BtqT3jocrSJRouEew7Nnto --- docs/public-api.md | 18 +-- mlbstatsapi/_async_transport.py | 165 +++++++++++++++++++++++++ mlbstatsapi/async_mlb.py | 36 +++--- mlbstatsapi/async_mlb_dataadapter.py | 177 ++++----------------------- tests/test_async_mlb.py | 68 +++++----- tests/test_async_mlb_dataadapter.py | 96 ++++++++------- 6 files changed, 307 insertions(+), 253 deletions(-) create mode 100644 mlbstatsapi/_async_transport.py diff --git a/docs/public-api.md b/docs/public-api.md index 93bbdfd..6a3d6a7 100644 --- a/docs/public-api.md +++ b/docs/public-api.md @@ -272,13 +272,17 @@ promised. `AsyncMlb` constructs internal adapters for both `v1` and `v1.1` that share one HTTPX client, mirroring `Mlb`'s shared-Session pattern. Most endpoint -methods use `v1`. `get_game` uses the `v1.1` live feed endpoint. The `v1` -adapter resolves and owns the shared client (library-created when the caller -passes none to `AsyncMlb`, otherwise the caller's own); the `v1.1` adapter -borrows that same client and never closes it directly. Retry eligibility -follows the shared client's ownership on both adapters, not which adapter -version issues a given request, matching `Mlb`'s single retry policy mounted -on the shared `Session`. +methods use `v1`. `get_game` uses the `v1.1` live feed endpoint. `AsyncMlb` +owns the shared client, exactly as `Mlb` owns the shared `Session`: it creates +one when the caller passes none, closes only a client it created, and hands +the same client to both adapters. + +Retries are a property of that client, not of either adapter. A +library-created client is built with the library retry transport mounted on +it, the way a library-created `Session` is built with the library retry +adapters mounted on it, so both API versions retry identically without either +adapter holding retry state. A caller-injected client keeps whatever transport +its caller mounted. ### Endpoint methods diff --git a/mlbstatsapi/_async_transport.py b/mlbstatsapi/_async_transport.py new file mode 100644 index 0000000..816e34e --- /dev/null +++ b/mlbstatsapi/_async_transport.py @@ -0,0 +1,165 @@ +"""Retry-aware HTTPX transport for the async client. + +The synchronous side does not implement retries. It *configures* them: ``Mlb`` +mounts an ``HTTPAdapter`` carrying the library ``Retry`` policy onto the +Session it creates, and from that point on every ``session.get()`` retries +without any caller — ``MlbDataAdapter`` included — knowing retries exist. + +HTTPX has the same seam. ``AsyncClient(transport=...)`` accepts any +``AsyncBaseTransport``, which is the position ``HTTPAdapter`` occupies in +Requests. Putting the retry loop there instead of inside +``AsyncMlbDataAdapter`` gives the async side the sync structure: + +* Adapters call ``client.get()`` and are unaware of retries. +* The retry policy travels with the client, so two adapters sharing one client + share one policy by construction. Neither adapter holds retry state, so + neither can disagree with the other about it. +* A caller-injected client keeps whatever transport its caller mounted, so + "the library does not touch an injected client" needs no flag to enforce. + +A caller who wants library retry behavior on a client they own mounts this +transport themselves, mirroring the documented sync recipe for +``create_retry_policy()``. +""" + +import asyncio + +from ._async_support import import_httpx +from .mlb_dataadapter import _build_user_agent, create_retry_policy + +httpx = import_httpx() + + +class MlbAsyncRetryTransport(httpx.AsyncBaseTransport): + """Wrap an HTTPX transport with the library's bounded retry policy. + + Failures spend the same retry budget the sync policy spends: + + ReadTimeout -> read budget + ConnectTimeout -> connect budget + ConnectError -> connect budget + other TimeoutException -> total budget + other RequestError -> total budget + retryable HTTP status -> status budget + + Exhausting a budget re-raises the underlying HTTPX exception. Translating + those into the library's public exception types stays with the adapter, so + this class satisfies the transport contract HTTPX documents: transports + raise HTTPX errors. + """ + + def __init__( + self, + inner: httpx.AsyncBaseTransport | None = None, + *, + retry_policy=None, + ): + self._inner = inner if inner is not None else httpx.AsyncHTTPTransport() + self._retry_policy = ( + retry_policy if retry_policy is not None else create_retry_policy() + ) + + async def handle_async_request(self, request: httpx.Request) -> httpx.Response: + policy = self._retry_policy + + attempt = 0 + while True: + attempt += 1 + try: + response = await self._inner.handle_async_request(request) + + except httpx.ReadTimeout: + if attempt > policy.read: + raise + await self._backoff(attempt=attempt, response=None) + continue + + except httpx.ConnectTimeout: + # Caught before httpx.TimeoutException: a connect timeout is a + # timeout for the caller, but it spends the connect budget so + # the retry accounting matches the sync policy. + if attempt > policy.connect: + raise + await self._backoff(attempt=attempt, response=None) + continue + + except httpx.ConnectError: + if attempt > policy.connect: + raise + await self._backoff(attempt=attempt, response=None) + continue + + except httpx.TimeoutException: + if attempt > policy.total: + raise + await self._backoff(attempt=attempt, response=None) + continue + + except httpx.RequestError: + if attempt > policy.total: + raise + await self._backoff(attempt=attempt, response=None) + continue + + if ( + response.status_code not in policy.status_forcelist + or attempt > policy.status + ): + return response + + # The response is discarded, so release it before another attempt + # rather than leaving a connection checked out of the pool. + delay = self._delay_for(attempt=attempt, response=response) + await response.aclose() + if delay > 0: + await asyncio.sleep(delay) + + async def _backoff( + self, + *, + attempt: int, + response: httpx.Response | None, + ) -> None: + delay = self._delay_for(attempt=attempt, response=response) + if delay > 0: + await asyncio.sleep(delay) + + def _delay_for( + self, + *, + attempt: int, + response: httpx.Response | None, + ) -> float: + policy = self._retry_policy + + if policy.respect_retry_after_header and response is not None: + retry_after = policy.get_retry_after(response) + if retry_after: + return retry_after + + # Mirrors urllib3's Retry.get_backoff_time(): no delay before the + # first retry, exponential thereafter, capped at backoff_max. + if attempt <= 1: + return 0.0 + + return min( + policy.backoff_factor * (2 ** (attempt - 1)), + policy.backoff_max, + ) + + async def aclose(self) -> None: + await self._inner.aclose() + + +def create_library_async_client() -> httpx.AsyncClient: + """Build the async client the library creates and owns. + + The counterpart of ``_configure_library_session()`` on the sync side: + library defaults are applied here, at creation, and only to clients the + library creates. Passing headers to the constructor replaces just the + User-Agent, so HTTPX's other default headers survive. + """ + return httpx.AsyncClient( + headers={"User-Agent": _build_user_agent()}, + transport=MlbAsyncRetryTransport(), + ) diff --git a/mlbstatsapi/async_mlb.py b/mlbstatsapi/async_mlb.py index 5ff4973..94a479e 100644 --- a/mlbstatsapi/async_mlb.py +++ b/mlbstatsapi/async_mlb.py @@ -5,6 +5,7 @@ import logging from typing import TYPE_CHECKING +from ._async_transport import create_library_async_client from ._helpers.id_lookup import find_ids_by_key from ._helpers.schedule import build_schedule_params from ._parsers.attendance import parse_attendance @@ -63,17 +64,23 @@ def __init__( ): self._logger = logger or logging.getLogger(__name__) - # One client is shared by the v1 and v1.1 adapters, mirroring Mlb's - # shared-Session pattern. The v1 adapter resolves and owns the client - # (library-created when the caller passes none); the v1.1 adapter - # borrows that same client and never closes it itself, but still - # retries exactly when the shared client is library-owned. + # One client is shared by the v1 and v1.1 adapters, and this client + # owns it, mirroring Mlb's shared-Session pattern. The library closes + # only clients it creates; caller-injected clients remain caller-owned. + # The versioned User-Agent and the retry transport are applied only to + # library-created clients. + self._owns_client = client is None + if client is None: + self._client = create_library_async_client() + else: + self._client = client + self._closed = False self._mlb_adapter_v1 = AsyncMlbDataAdapter( hostname=hostname, ver="v1", logger=self._logger, timeout=timeout, - client=client, + client=self._client, strict_http=strict_http, ) self._mlb_adapter_v1_1 = AsyncMlbDataAdapter( @@ -81,19 +88,18 @@ def __init__( ver="v1.1", logger=self._logger, timeout=timeout, - client=self._mlb_adapter_v1._client, + client=self._client, strict_http=strict_http, ) - # AsyncMlb, not either adapter, actually owns this shared transport, - # so it is the one that knows whether the client is library-owned. - # The v1.1 adapter received a non-None client above, so it would - # otherwise conclude it's using a caller-injected client and disable - # retries even when the client is really library-owned via v1. - self._mlb_adapter_v1_1._set_retries_enabled(self._mlb_adapter_v1._owns_client) async def aclose(self) -> None: - """Close library-owned async resources.""" - await self._mlb_adapter_v1.aclose() + """Close the HTTP client when this client owns it. + + Safe to call more than once. Caller-injected clients are left alone. + """ + if self._owns_client and not self._closed: + await self._client.aclose() + self._closed = True async def __aenter__(self) -> "AsyncMlb": return self diff --git a/mlbstatsapi/async_mlb_dataadapter.py b/mlbstatsapi/async_mlb_dataadapter.py index 8e18065..ec67631 100644 --- a/mlbstatsapi/async_mlb_dataadapter.py +++ b/mlbstatsapi/async_mlb_dataadapter.py @@ -1,9 +1,9 @@ -import asyncio import logging from typing import Dict from ._async_support import import_httpx +from ._async_transport import create_library_async_client from .exceptions import ( MlbDecodeError, MlbTimeoutError, @@ -13,8 +13,6 @@ DEFAULT_TIMEOUT, MlbResult, TimeoutType, - _build_user_agent, - create_retry_policy, ) from ._http import ( @@ -49,25 +47,16 @@ def __init__( self._timeout = timeout self._strict_http = strict_http self._owns_client = client is None - # Retry eligibility follows client ownership by default, like the - # sync adapter (retries are mounted on the Session, not per - # MlbDataAdapter version). This is not a constructor knob: a caller - # that owns this adapter's transport (AsyncMlb, for its v1.1 adapter - # sharing v1's client) may call _set_retries_enabled() after - # construction, since it — not this adapter — is the one that knows - # whether the shared client is actually library-owned. - self._retries_enabled = self._owns_client - self._retry_policy = create_retry_policy() if client is None: - # Only a library-owned client gets the package User-Agent. Passing - # it to the constructor replaces just that header, so httpx's other - # default headers (Accept, Accept-Encoding, Connection) survive. - self._client = httpx.AsyncClient( - headers={"User-Agent": _build_user_agent()}, - ) + # A library-created client carries the package User-Agent and the + # library retry transport. Retries are a property of the client, + # not of this adapter, exactly as they are a property of the + # Session on the sync side. + self._client = create_library_async_client() else: - # An injected client stays exactly as the caller configured it. + # An injected client stays exactly as the caller configured it, + # retry transport included or not. self._client = client self._closed = False @@ -100,8 +89,20 @@ async def get(self, endpoint: str, ep_params: Dict = None, data: Dict = None) -> ) ) - self._logger.debug(logline_post) - response = await self._request_with_retries(full_url, ep_params) + try: + self._logger.debug(logline_post) + response = await self._client.get( + url=full_url, + params=ep_params, + timeout=self._translate_timeout(self._timeout), + ) + + except httpx.TimeoutException as exc: + self._logger.error(msg=(str(exc))) + raise MlbTimeoutError("Request failed") from exc + except httpx.RequestError as exc: + self._logger.error(msg=(str(exc))) + raise MlbTransportError("Request failed") from exc status_code = response.status_code @@ -181,129 +182,6 @@ async def get(self, endpoint: str, ep_params: Dict = None, data: Dict = None) -> data=response_data, ) - async def _request_with_retries( - self, - full_url: str, - ep_params: Dict, - ) -> httpx.Response: - """Issue the GET call, retrying with bounded backoff when this - adapter owns its httpx.AsyncClient. - - An injected client is called exactly once; its retry behavior stays - under caller control, matching the sync adapter's session-ownership - rule. - - Failures spend the retry budget the sync policy would spend, and - surface the public exception the sync adapter raises: - - ReadTimeout -> read budget -> MlbTimeoutError - ConnectTimeout -> connect budget -> MlbTimeoutError - ConnectError -> connect budget -> MlbTransportError - other TimeoutException -> total budget -> MlbTimeoutError - other RequestError -> total budget -> MlbTransportError - retryable HTTP status -> status budget - """ - policy = self._retry_policy - - attempt = 0 - while True: - attempt += 1 - try: - response = await self._client.get( - url=full_url, - params=ep_params, - timeout=self._translate_timeout(self._timeout), - ) - - except httpx.ReadTimeout as exc: - max_attempts = policy.read + 1 if self._retries_enabled else 1 - - if attempt >= max_attempts: - self._logger.error(msg=str(exc)) - raise MlbTimeoutError("Request failed") from exc - - await self._sleep_before_retry(attempt=attempt, response=None) - continue - - except httpx.ConnectTimeout as exc: - # Caught before httpx.TimeoutException: a connect timeout is a - # timeout for the caller, but it spends the connect budget so - # the retry accounting matches the sync policy. - max_attempts = policy.connect + 1 if self._retries_enabled else 1 - - if attempt >= max_attempts: - self._logger.error(msg=str(exc)) - raise MlbTimeoutError("Request failed") from exc - - await self._sleep_before_retry(attempt=attempt, response=None) - continue - - except httpx.ConnectError as exc: - max_attempts = policy.connect + 1 if self._retries_enabled else 1 - - if attempt >= max_attempts: - self._logger.error(msg=str(exc)) - raise MlbTransportError("Request failed") from exc - - await self._sleep_before_retry( - attempt=attempt, - response=None, - ) - continue - - except httpx.TimeoutException as exc: - max_attempts = policy.total + 1 if self._retries_enabled else 1 - - if attempt >= max_attempts: - raise MlbTimeoutError("Request failed") from exc - - await self._sleep_before_retry( - attempt=attempt, - response=None, - ) - continue - - except httpx.RequestError as exc: - max_attempts = policy.total + 1 if self._retries_enabled else 1 - - if attempt >= max_attempts: - self._logger.error(msg=str(exc)) - raise MlbTransportError("Request failed") from exc - - await self._sleep_before_retry(attempt=attempt, response=None) - continue - - max_attempts = policy.status + 1 if self._retries_enabled else 1 - - if response.status_code not in policy.status_forcelist or attempt >= max_attempts: - return response - - await self._sleep_before_retry(attempt=attempt, response=response) - - async def _sleep_before_retry( - self, - *, - attempt: int, - response: httpx.Response | None, - ) -> None: - policy = self._retry_policy - - if policy.respect_retry_after_header and response is not None: - retry_after = policy.get_retry_after(response) - if retry_after: - await asyncio.sleep(retry_after) - return - - # Mirrors urllib3's Retry.get_backoff_time(): no delay before the - # first retry, exponential thereafter, capped at backoff_max. - delay = 0.0 if attempt <= 1 else min( - policy.backoff_factor * (2 ** (attempt - 1)), - policy.backoff_max, - ) - - if delay > 0: - await asyncio.sleep(delay) - @staticmethod def _translate_timeout(timeout: TimeoutType) -> httpx.Timeout: if isinstance(timeout, tuple): @@ -322,14 +200,3 @@ async def aclose(self) -> None: if self._owns_client and not self._closed: await self._client.aclose() self._closed = True - - def _set_retries_enabled(self, enabled: bool) -> None: - """Override retry eligibility for a borrowed, non-owned client. - - Internal coordination hook, not public API: only a caller that - actually owns this adapter's transport (AsyncMlb, wiring up its v1.1 - adapter to share the v1 adapter's client) should call this. Standalone - use never needs it; retry eligibility already follows client - ownership by default. - """ - self._retries_enabled = enabled diff --git a/tests/test_async_mlb.py b/tests/test_async_mlb.py index 08a98d2..afa87fe 100644 --- a/tests/test_async_mlb.py +++ b/tests/test_async_mlb.py @@ -36,6 +36,7 @@ httpx = pytest.importorskip("httpx", reason="requires the async extra (HTTPX)") from mlbstatsapi import Mlb # noqa: E402 +from mlbstatsapi._async_transport import MlbAsyncRetryTransport # noqa: E402 from mlbstatsapi.async_mlb import AsyncMlb # noqa: E402 from mlbstatsapi.mlb_dataadapter import MlbResult # noqa: E402 from mlbstatsapi.models.attendances import Attendance # noqa: E402 @@ -387,29 +388,23 @@ def request(self) -> httpx.Request: async def async_mlb(handler: _Handler): """Yield an AsyncMlb whose own client talks to ``handler``, then close it. - AsyncMlb builds its adapter and client through the production path; only - the transport is swapped. Teardown closes the adapter's client directly - rather than calling AsyncMlb.aclose(), so the lifecycle tests that replace - aclose with a mock still get their real client closed. + AsyncMlb builds its client, its retry transport and its adapters through + the production path; only the innermost network transport is swapped. + Teardown closes the client directly rather than calling AsyncMlb.aclose(), + so the lifecycle tests that replace aclose with a mock still get their real + client closed. """ - real_async_client = httpx.AsyncClient - - def mock_transport_client(**client_kwargs) -> httpx.AsyncClient: - return real_async_client( - transport=httpx.MockTransport(handler), **client_kwargs - ) - with pytest.MonkeyPatch.context() as monkeypatch: monkeypatch.setattr( - "mlbstatsapi.async_mlb_dataadapter.httpx.AsyncClient", - mock_transport_client, + "mlbstatsapi._async_transport.httpx.AsyncHTTPTransport", + lambda **kwargs: httpx.MockTransport(handler), ) mlb = AsyncMlb() try: yield mlb finally: - await mlb._mlb_adapter_v1._client.aclose() + await mlb._client.aclose() def sync_request_for(method: str, *args, **kwargs) -> tuple[str, dict, str]: @@ -495,7 +490,7 @@ async def scenario(): def test_context_exit_closes_the_owned_client(): async def scenario(): async with async_mlb(_Handler(_json(TEAM_PAYLOAD))) as mlb: - client = mlb._mlb_adapter_v1._client + client = mlb._client async with mlb: await mlb.get_team(133) @@ -508,7 +503,7 @@ async def scenario(): def test_context_exit_closes_the_owned_client_when_the_body_raises(): async def scenario(): async with async_mlb(_Handler(_json(TEAM_PAYLOAD))) as mlb: - client = mlb._mlb_adapter_v1._client + client = mlb._client with pytest.raises(ValueError, match="boom"): async with mlb: @@ -528,7 +523,7 @@ def test_cleanup_failure_does_not_replace_the_original_exception(): async def scenario(): async with async_mlb(_Handler(_json(TEAM_PAYLOAD))) as mlb: - mlb._mlb_adapter_v1.aclose = AsyncMock( + mlb.aclose = AsyncMock( side_effect=RuntimeError("cleanup failed") ) @@ -548,7 +543,7 @@ def test_cancellation_is_preserved_through_cleanup(): async def scenario(): async with async_mlb(_Handler(_json(TEAM_PAYLOAD))) as mlb: - client = mlb._mlb_adapter_v1._client + client = mlb._client async def worker(): async with mlb: @@ -583,42 +578,47 @@ async def scenario(): asyncio.run(scenario()) -def test_v1_and_v1_1_adapters_share_one_client(): - """One client is shared by both adapters, mirroring Mlb's shared Session.""" +def test_v1_and_v1_1_adapters_share_the_client_this_client_owns(): + """One client is shared by both adapters and owned by AsyncMlb itself, + mirroring Mlb's shared Session.""" async def scenario(): async with async_mlb(_Handler(_json(TEAM_PAYLOAD))) as mlb: - assert mlb._mlb_adapter_v1._client is mlb._mlb_adapter_v1_1._client - # Only v1 tracks close-ownership of the shared client; v1.1 must - # never double-close it. - assert mlb._mlb_adapter_v1._owns_client is True + assert mlb._mlb_adapter_v1._client is mlb._client + assert mlb._mlb_adapter_v1_1._client is mlb._client + # Close-ownership lives on AsyncMlb, so neither adapter can close + # the shared client out from under the other. + assert mlb._owns_client is True + assert mlb._mlb_adapter_v1._owns_client is False assert mlb._mlb_adapter_v1_1._owns_client is False asyncio.run(scenario()) -def test_v1_1_adapter_retries_when_the_shared_client_is_library_owned(): - """Retry eligibility follows the shared client's ownership, not which - adapter version issues the request (matching Mlb, which configures - retries once on the shared Session).""" +def test_both_api_versions_retry_because_the_shared_client_carries_the_policy(): + """Retries belong to the shared client's transport, not to an adapter, so + the two versions cannot disagree about them (matching Mlb, which mounts + one retry policy on the shared Session).""" async def scenario(): async with async_mlb(_Handler(_json(TEAM_PAYLOAD))) as mlb: - assert mlb._mlb_adapter_v1._retries_enabled is True - assert mlb._mlb_adapter_v1_1._retries_enabled is True + assert isinstance(mlb._client._transport, MlbAsyncRetryTransport) asyncio.run(scenario()) -def test_v1_1_adapter_does_not_retry_with_a_caller_injected_client(): +def test_caller_injected_client_keeps_its_own_transport(): + """The library mounts nothing on a client it did not create, so an + injected client retries exactly as much as its caller configured.""" handler = _Handler(_json(TEAM_PAYLOAD)) - client = httpx.AsyncClient(transport=httpx.MockTransport(handler)) + transport = httpx.MockTransport(handler) + client = httpx.AsyncClient(transport=transport) async def scenario(): try: async with AsyncMlb(client=client) as mlb: - assert mlb._mlb_adapter_v1._retries_enabled is False - assert mlb._mlb_adapter_v1_1._retries_enabled is False + assert mlb._client is client + assert mlb._client._transport is transport finally: await client.aclose() diff --git a/tests/test_async_mlb_dataadapter.py b/tests/test_async_mlb_dataadapter.py index f2ef0f5..9073732 100644 --- a/tests/test_async_mlb_dataadapter.py +++ b/tests/test_async_mlb_dataadapter.py @@ -41,6 +41,10 @@ MlbTimeoutError, MlbTransportError, ) +from mlbstatsapi._async_transport import ( # noqa: E402 + MlbAsyncRetryTransport, + create_library_async_client, +) from mlbstatsapi.async_mlb_dataadapter import AsyncMlbDataAdapter # noqa: E402 from mlbstatsapi.mlb_dataadapter import PACKAGE_DISTRIBUTION_NAME # noqa: E402 @@ -54,11 +58,13 @@ BASE_URL = "https://statsapi.mlb.com/api/v1/" -SLEEP_TARGET = "mlbstatsapi.async_mlb_dataadapter.asyncio.sleep" +SLEEP_TARGET = "mlbstatsapi._async_transport.asyncio.sleep" # Patched only while a test adapter is constructed, so the adapter creates its -# own library-owned client the way production does, over a MockTransport. -CLIENT_TARGET = "mlbstatsapi.async_mlb_dataadapter.httpx.AsyncClient" +# own library-owned client the way production does. Only the innermost network +# transport is swapped, so the library retry transport under test is the real +# one, wrapping a MockTransport instead of a socket. +INNER_TRANSPORT_TARGET = "mlbstatsapi._async_transport.httpx.AsyncHTTPTransport" # Matches tests/test_mlb_session.py, so both adapters assert the same contract. MOCKED_PACKAGE_VERSION = "9.8.7" @@ -117,26 +123,23 @@ def _owned_adapter(handler, **kwargs) -> AsyncMlbDataAdapter: """Build an adapter that owns its client, so retries are active. The adapter still builds its own client through the production path — only - the transport is swapped for a MockTransport — so ownership, headers, and - retry behavior are exactly what the library does at runtime, and no client - is constructed and then discarded. Call this from inside a run_async() - scenario; run_async() closes what it creates. + the innermost network transport is swapped for a MockTransport — so + ownership, headers, and retry behavior are exactly what the library does at + runtime, and no client is constructed and then discarded. Call this from + inside a run_async() scenario; run_async() closes what it creates. """ - real_async_client = httpx.AsyncClient - - def mock_transport_client(**client_kwargs) -> httpx.AsyncClient: - return real_async_client( - transport=httpx.MockTransport(handler), - **client_kwargs, - ) - - with patch(CLIENT_TARGET, mock_transport_client): + with patch(INNER_TRANSPORT_TARGET, lambda **kwargs: httpx.MockTransport(handler)): adapter = AsyncMlbDataAdapter(**kwargs) _ADAPTERS_TO_CLOSE.append(adapter) return adapter +def _retry_policy_of(adapter: AsyncMlbDataAdapter): + """Read the retry policy the adapter's client actually uses.""" + return adapter._client._transport._retry_policy + + def _injected_adapter(handler, **kwargs) -> AsyncMlbDataAdapter: """Build an adapter with a caller-supplied client, so retries are bypassed.""" client = httpx.AsyncClient(transport=httpx.MockTransport(handler)) @@ -145,34 +148,43 @@ def _injected_adapter(handler, **kwargs) -> AsyncMlbDataAdapter: def test_retry_policy_matches_library_default(): adapter = AsyncMlbDataAdapter() - assert_library_retry_policy(adapter._retry_policy) + assert_library_retry_policy(_retry_policy_of(adapter)) -def test_set_retries_enabled_does_not_change_client_ownership(): - """The private coordination hook AsyncMlb uses for its v1.1 adapter only - overrides retry eligibility; it must never grant close-ownership over a - client this adapter did not create.""" - handler = _ScriptedHandler(_response(200)) - adapter = _injected_adapter(handler) - assert adapter._retries_enabled is False +def test_library_created_client_mounts_the_retry_transport(): + """Retries are configured onto the client at creation, the way the sync + side mounts them onto a library-created Session.""" + client = create_library_async_client() + + assert isinstance(client._transport, MlbAsyncRetryTransport) - adapter._set_retries_enabled(True) - assert adapter._retries_enabled is True +def test_injected_client_transport_is_left_alone(): + """The library mounts nothing on a client it did not create, so an + injected client keeps exactly the retry behavior its caller gave it.""" + transport = httpx.MockTransport(_ScriptedHandler(_response(200))) + client = httpx.AsyncClient(transport=transport) + adapter = AsyncMlbDataAdapter(client=client) + + assert adapter._client._transport is transport assert adapter._owns_client is False -def test_set_retries_enabled_true_makes_an_injected_client_retry(): - """An injected client normally gets zero retries; overriding the flag - must actually change retry behavior, not just the stored value.""" +def test_mounting_the_retry_transport_makes_an_injected_client_retry(): + """The supported way for a caller to opt their own client into library + retry behavior, mirroring the sync create_retry_policy() recipe.""" handler = _ScriptedHandler(_response(503), _response(200)) async def scenario(): - adapter = _injected_adapter(handler) - adapter._set_retries_enabled(True) - - with patch(SLEEP_TARGET, new_callable=AsyncMock): - return await adapter.get(endpoint="sports") + client = httpx.AsyncClient( + transport=MlbAsyncRetryTransport(httpx.MockTransport(handler)), + ) + adapter = AsyncMlbDataAdapter(client=client) + try: + with patch(SLEEP_TARGET, new_callable=AsyncMock): + return await adapter.get(endpoint="sports") + finally: + await client.aclose() result = run_async(scenario()) assert result.status_code == 200 @@ -765,7 +777,7 @@ def test_connect_timeout_spends_the_connect_retry_budget(): async def scenario(): adapter = _owned_adapter(handler) - adapter._retry_policy.connect = 1 + _retry_policy_of(adapter).connect = 1 with patch(SLEEP_TARGET, new_callable=AsyncMock): with pytest.raises(MlbTimeoutError): await adapter.get(endpoint="sports") @@ -825,7 +837,7 @@ def test_generic_failures_spend_the_total_retry_budget(failure, expected_excepti async def scenario(): adapter = _owned_adapter(handler) - adapter._retry_policy.total = 1 + _retry_policy_of(adapter).total = 1 with patch(SLEEP_TARGET, new_callable=AsyncMock): with pytest.raises(expected_exception) as exc_info: await adapter.get(endpoint="sports") @@ -849,7 +861,7 @@ def test_connect_error_spends_the_connect_retry_budget(): async def scenario(): adapter = _owned_adapter(handler) - adapter._retry_policy.connect = 1 + _retry_policy_of(adapter).connect = 1 with patch(SLEEP_TARGET, new_callable=AsyncMock): with pytest.raises(MlbTransportError): await adapter.get(endpoint="sports") @@ -864,7 +876,7 @@ def test_retryable_status_spends_the_status_retry_budget(): async def scenario(): adapter = _owned_adapter(handler) - adapter._retry_policy.status = 1 + _retry_policy_of(adapter).status = 1 with patch(SLEEP_TARGET, new_callable=AsyncMock): with pytest.raises(MlbHttpError) as exc_info: await adapter.get(endpoint="sports") @@ -921,9 +933,9 @@ async def scenario(): def test_retry_sleep_is_async_and_non_blocking(): """A real (unmocked) backoff wait must yield the event loop. - If _sleep_before_retry ever used a blocking call (e.g. time.sleep) - instead of `await asyncio.sleep(...)`, the whole event loop would - freeze for the wait's duration and the concurrently running marker + If the retry transport's backoff ever used a blocking call (e.g. + time.sleep) instead of `await asyncio.sleep(...)`, the whole event loop + would freeze for the wait's duration and the concurrently running marker task below would make zero progress during it. """ handler = _ScriptedHandler(_response(500), _response(500), _response(200)) @@ -931,7 +943,7 @@ def test_retry_sleep_is_async_and_non_blocking(): async def scenario(): adapter = _owned_adapter(handler) # Small but real backoff so the test stays fast without mocking sleep. - adapter._retry_policy.backoff_factor = 0.05 + _retry_policy_of(adapter).backoff_factor = 0.05 marker_ticks = 0