refactor(client): extract shared request-assembly functions

planning/changes/2026-07-13.02-client-request-assembly-extraction.md

@@ -0,0 +1,251 @@
+---
+summary: Extract the non-I/O construction-validation and kwargs-assembly logic hand-duplicated between Client and AsyncClient into shared module-level functions in client.py.
+---
+
+# Design: Extract shared request-assembly functions from `Client`/`AsyncClient`
+
+## Summary
+
+`AsyncClient` (`client.py:60-1031`, ~972 lines) and `Client` (`client.py:1032-1999`,
+~968 lines) are structurally identical, byte-for-byte duplicated except for
+`async`/`await` and the sync-vs-async I/O calls. This change extracts the
+purely non-I/O logic — `__init__`'s `httpx2_client=...` conflict validation,
+`__init__`'s httpx2-client-constructor kwargs assembly, and the per-request
+kwargs assembly shared by `_prepare_request` and `stream()` — into three
+module-level private functions in `client.py`, called by both classes. The
+public per-verb surface (`get`/`post`/`put`/.../`request` and their
+`_with_response` siblings, each `async`/sync and separately overloaded) and
+the actual I/O call sites (`_terminal`, `_prepare_request`'s
+`build_request` call, `stream()`'s `self._httpx2_client.stream(...)` call)
+stay exactly as they are — mirroring httpx2's own per-method API is a
+deliberate design choice (see the `# noqa: PLR0913 — mirrors httpx2
+per-method signatures` comments throughout), and the `await`/non-`await`
+split at the I/O boundary is irreducible without source-to-source codegen,
+which this change deliberately does not introduce. No behavior change.
+
+## Motivation
+
+- The package already solved this exact duplication shape twice:
+  `middleware/resilience/circuit_breaker.py`'s `_CircuitBreakerState` and
+  `middleware/resilience/retry.py`'s `_RetryPolicy` are both stateless/
+  lock-free decision logic shared by a sync and an async wrapper that
+  otherwise differ only in `await`/locking. `client.py` never received the
+  same treatment — its non-I/O logic is hand-copied instead of shared.
+- **Depth:** three blocks of pure, input-to-output logic are duplicated
+  verbatim: the `httpx2_client=...` conflict check (~11 lines ×2), the
+  httpx2-client-constructor kwargs dict-building (~14 lines ×2), and the
+  per-request kwargs dict-building (~19 lines — duplicated not just across
+  `AsyncClient`/`Client` but a second time within each class, between
+  `_prepare_request` and `stream()`, for 4 call sites total). None of this
+  logic touches `httpx2.Client` vs `httpx2.AsyncClient` differently.
+- **Deletion test:** delete the three shared functions after this change
+  and the same four dict-building/validation blocks reappear at their
+  six call sites — real, load-bearing logic, not a pass-through.
+- **Scope discipline:** the per-verb methods (`get`/`post`/etc.) are
+  call-site boilerplate whose complexity is the sheer *count* of methods
+  (8 verbs × 2 overloads + impl + `_with_response` sibling × 2 classes),
+  not hidden logic — each body is already just "forward these named kwargs
+  to `_request_with_body`". There is nothing further to extract there
+  without codegen, and the type overloads are part of the public interface
+  (caller ergonomics), not internal duplication. This change does not touch
+  them.
+
+## Non-goals
+
+- No behavior change. Every existing test must pass with the same
+  assertions; only the internal call path changes.
+- Not collapsing the per-verb methods, `_request_with_body`/
+  `_request_with_body_with_response`, `_terminal`, or `stream()`'s I/O
+  call — these stay duplicated (thin as they already are), because the
+  `await`/non-`await` split there is structural, not incidental.
+- Not introducing codegen (unasync-style generation of `Client` from
+  `AsyncClient`). Considered and explicitly rejected for this change: no
+  precedent in this codebase for that kind of build/lint tooling, and the
+  bulk of the actual duplication cost is concentrated in the non-I/O logic
+  this change already collapses.
+- Not moving the new functions to `_internal/`. Precedent
+  (`_CircuitBreakerState` in `circuit_breaker.py`, `_RetryPolicy` in
+  `retry.py`) keeps logic private to one file's two classes in that same
+  file; `_internal/` is reserved for logic shared *across* otherwise-
+  unrelated modules (`status.py`, `exception_mapping.py`).
+- Not a class. `_CircuitBreakerState`/`_RetryPolicy` are classes because
+  they hold config plus evolving/shared state reused across many calls.
+  The three functions extracted here hold no state between calls — each is
+  a pure function of its arguments — so a class would be a namespace with
+  no benefit.
+
+## Design
+
+### 1. `_validate_httpx2_client_conflict`
+
+```python
+def _validate_httpx2_client_conflict(
+    *,
+    base_url: str,
+    headers: dict[str, str] | None,
+    params: dict[str, str] | None,
+    cookies: dict[str, str] | None,
+    timeout: httpx2.Timeout | float | None,
+    limits: httpx2.Limits | None,
+    auth: httpx2.Auth | None,
+) -> None:
+    """Raise TypeError if httpx2_client=... is combined with a forwarded kwarg."""
+    forwarded = {
+        "base_url": base_url,
+        "headers": headers,
+        "params": params,
+        "cookies": cookies,
+        "timeout": timeout,
+        "limits": limits,
+        "auth": auth,
+    }
+    if any(value not in (None, "") for value in forwarded.values()):
+        raise TypeError(_HTTPX2_CLIENT_CONFLICT_MESSAGE)
+```
+
+Replaces `client.py:86-97` (async) and the equivalent sync block verbatim.
+Called only from inside the `if httpx2_client is not None:` branch of both
+`__init__`s, immediately before `self._httpx2_client = httpx2_client`.
+
+### 2. `_assemble_httpx2_client_kwargs`
+
+```python
+def _assemble_httpx2_client_kwargs(
+    *,
+    base_url: str,
+    headers: dict[str, str] | None,
+    params: dict[str, str] | None,
+    cookies: dict[str, str] | None,
+    timeout: httpx2.Timeout | float | None,
+    limits: httpx2.Limits | None,
+    auth: httpx2.Auth | None,
+) -> dict[str, typing.Any]:
+    """Build the kwargs dict for constructing the owned httpx2 client."""
+    kwargs: dict[str, typing.Any] = {}
+    if base_url:
+        kwargs["base_url"] = base_url
+    if headers is not None:
+        kwargs["headers"] = headers
+    if params is not None:
+        kwargs["params"] = params
+    if cookies is not None:
+        kwargs["cookies"] = cookies
+    if timeout is not None:
+        kwargs["timeout"] = timeout
+    if limits is not None:
+        kwargs["limits"] = limits
+    if auth is not None:
+        kwargs["auth"] = auth
+    return kwargs
+```
+
+Replaces `client.py:101-115` (async) and the equivalent sync block. Each
+`__init__`'s `else:` branch becomes:
+
+```python
+kwargs = _assemble_httpx2_client_kwargs(
+    base_url=base_url, headers=headers, params=params, cookies=cookies,
+    timeout=timeout, limits=limits, auth=auth,
+)
+self._httpx2_client = httpx2.AsyncClient(**kwargs)  # or httpx2.Client(**kwargs)
+self._owns_client = True
+```
+
+Uses `timeout is not None` — client-construction semantics. Distinct from
+function 3's `timeout is not httpx2.USE_CLIENT_DEFAULT` check; these are
+different semantics for a same-named parameter and must stay two separate
+functions (confirmed during design — a merged function would have silently
+conflated them).
+
+### 3. `_assemble_request_kwargs`
+
+```python
+def _assemble_request_kwargs(
+    *,
+    params: typing.Any | None,
+    headers: typing.Any | None,
+    cookies: typing.Any | None,
+    timeout: typing.Any,
+    extensions: typing.Any | None,
+    json: typing.Any | None,
+    content: typing.Any | None,
+    data: typing.Any | None,
+    files: typing.Any | None,
+) -> dict[str, typing.Any]:
+    """Build the kwargs dict for a per-request httpx2 call (build_request/stream)."""
+    kwargs: dict[str, typing.Any] = {}
+    if params is not None:
+        kwargs["params"] = params
+    if headers is not None:
+        kwargs["headers"] = headers
+    if cookies is not None:
+        kwargs["cookies"] = cookies
+    if timeout is not httpx2.USE_CLIENT_DEFAULT:
+        kwargs["timeout"] = timeout
+    if extensions is not None:
+        kwargs["extensions"] = extensions
+    if json is not None:
+        kwargs["json"] = json
+    if content is not None:
+        kwargs["content"] = content
+    if data is not None:
+        kwargs["data"] = data
+    if files is not None:
+        kwargs["files"] = files
+    return kwargs
+```
+
+Replaces `client.py:202-220` (async `_prepare_request`) and the
+byte-identical block at `client.py:976-994` (async `stream()`), plus both
+sync equivalents — **4 call sites total**, not 2. `_prepare_request` keeps
+its own `build_request(...)` call and the streaming-body-marker check
+(`_is_streaming_body_async`/`_sync` — this predicate genuinely differs
+between worlds and stays inline); `stream()` keeps its own
+`self._httpx2_client.stream(...)` call. Only the kwargs-dict construction
+moves.
+
+### 4. Placement
+
+All three functions go in `client.py`, module level, alongside the
+existing `_build_default_decoders` (before `class AsyncClient:`) —
+matching where `_CircuitBreakerState`/`_RetryPolicy` sit relative to their
+wrapper classes: private to the file, not moved to `_internal/`.
+
+## Testing
+
+- **Parity net:** all existing suites stay green unchanged —
+  `test_client_construction.py`, `test_client_sync.py`,
+  `test_client_dispatch.py`, `test_client_methods.py`,
+  `test_client_stream.py`, `test_client_stream_sync.py`, and the rest.
+  Byte-identical behavior is the bar.
+- **New seam tests:** `tests/test_request_assembly.py` drives
+  `_assemble_httpx2_client_kwargs` and `_assemble_request_kwargs` directly
+  (no client, no `MockTransport`) — which keys appear/are omitted for
+  `None`/falsy/`USE_CLIENT_DEFAULT` inputs, and that a fully-populated call
+  includes every key. `_validate_httpx2_client_conflict` gets no separate
+  direct test: it's a single boolean check already directly asserted via
+  `TypeError` through the public constructor in
+  `test_client_construction.py`, and a duplicate direct test would add no
+  leverage.
+- `just lint && just test` both clean; 100% coverage maintained.
+
+## Risk
+
+- **Behavioral drift during extraction** (unlikely × medium): a subtle
+  reordering changes which kwarg wins or silently drops a falsy-but-valid
+  value (e.g. `base_url=""` vs `base_url` unset — the existing code already
+  treats empty string as "unset" via the truthy check, and this must be
+  preserved exactly). *Mitigation:* extract verbatim under the existing
+  green suites, which construct clients with every combination of
+  set/unset kwargs; do not edit those suites in this change.
+- **Conflating the two `timeout` semantics** (unlikely × high, if it
+  happened): function 2 must keep `is not None`, function 3 must keep
+  `is not httpx2.USE_CLIENT_DEFAULT` — merging them into one function was
+  considered and rejected specifically to avoid this. *Mitigation:* keep
+  them as two functions per this design; `test_request_assembly.py`
+  asserts each function's actual sentinel/None behavior directly.
+- **Per-verb layer accidentally touched** (unlikely × low): scope creep
+  into the per-verb methods would reopen the codegen-vs-hand-duplication
+  question this change deliberately defers. *Mitigation:* this design's
+  Non-goals section is explicit; the task brief will name the exact
+  functions and line ranges to touch and nothing else.