|
| 1 | +--- |
| 2 | +summary: Share the decoders' memoizing get-or-build cache pattern (4 duplicated sites across pydantic.py/msgspec.py) via one generic _get_or_build function, and drop decode()'s now-dead TypeError fallback. |
| 3 | +--- |
| 4 | + |
| 5 | +# Design: Share the decoders' memoizing cache pattern |
| 6 | + |
| 7 | +## Summary |
| 8 | + |
| 9 | +`src/httpware/decoders/pydantic.py` and `decoders/msgspec.py` each |
| 10 | +implement the identical "check a `dict[type, V]` cache, build-and-memoize |
| 11 | +on miss, unhashable models bypass the cache entirely" pattern at two sites |
| 12 | +per file: the object cache (`_get_adapter`/`_get_msgspec_decoder`) and the |
| 13 | +verdict cache (`can_decode`). This change extracts one generic |
| 14 | +`_get_or_build` function into a new `decoders/_caching.py`, used at all |
| 15 | +four sites, and simplifies `decode()` in both classes by dropping a |
| 16 | +try/except that becomes dead code once the cache handles the unhashable |
| 17 | +case internally. Two existing tests that mocked the wrong layer are |
| 18 | +corrected to test the real mechanism. **One intentional, incidental |
| 19 | +behavior fix**: `can_decode` now agrees with `decode` for genuinely |
| 20 | +unhashable, schema-buildable models (see Motivation) — everything else is |
| 21 | +behavior-preserving. |
| 22 | + |
| 23 | +## Motivation |
| 24 | + |
| 25 | +- **Four duplicated cache sites, not two.** `PydanticDecoder._get_adapter` |
| 26 | + (`:45-50`) and `MsgspecDecoder._get_msgspec_decoder` (`:67-72`) are |
| 27 | + structurally identical (differing only in what gets built and which |
| 28 | + dict holds it). `PydanticDecoder.can_decode` (`:52-67`) and |
| 29 | + `MsgspecDecoder.can_decode` (`:74-89`) are byte-identical (modulo |
| 30 | + docstring wording) — same `try: cache.get(model) / except TypeError: |
| 31 | + probe fresh`, same `if cached is not None: return cached`, same |
| 32 | + probe-store-return. |
| 33 | +- **The sentinel trick is shared too.** Both cache flavors — `dict[type, |
| 34 | + bool]` (verdicts) and `dict[type, TypeAdapter | Decoder]` (built |
| 35 | + objects) — rely on the same fact: a real stored value (`True`/`False`, |
| 36 | + or a built object) is never `None`, so `dict.get(model)` returning |
| 37 | + `None` unambiguously means "not cached." One generic function serves |
| 38 | + both flavors. |
| 39 | +- **Corroborating signal:** `tests/test_decoders_pydantic.py` and |
| 40 | + `tests/test_decoders_msgspec.py` (241 lines each) independently |
| 41 | + re-verify the same caching contract via `# noqa: SLF001` pokes at |
| 42 | + `_adapters`/`_can_decode_results` and `_msgspec_decoders`/ |
| 43 | + `_can_decode_results` — the memoization concern isn't exercisable |
| 44 | + through the public `ResponseDecoder` protocol alone, so tests reach |
| 45 | + through the seam rather than past it. |
| 46 | +- **Deletion test:** delete `_get_or_build` and the identical |
| 47 | + get-or-build logic reappears at all four call sites — real, load-bearing |
| 48 | + logic, not a pass-through. |
| 49 | +- **A second, independent finding surfaced while re-reading both files |
| 50 | + closely:** `decode()` in both classes has its own try/except `TypeError` |
| 51 | + fallback (falls back to a fresh, uncached build) that exists solely to |
| 52 | + handle the same unhashable-model case `_get_or_build` will now handle |
| 53 | + internally. Traced through Seam B's contract |
| 54 | + (`decoders/_resolver.py`'s `_DecoderResolver.resolve`): `decode()` only |
| 55 | + ever runs on a model that already passed `can_decode`'s probe in the |
| 56 | + same request, so `TypeAdapter(model)`/`msgspec.json.Decoder(model)` |
| 57 | + construction cannot newly fail at `decode()` time — the only thing the |
| 58 | + existing fallback ever guarded against was the cache-lookup |
| 59 | + `TypeError`. Once that's handled inside `_get_or_build`, the fallback in |
| 60 | + `decode()` becomes genuinely unreachable and is removed. |
| 61 | +- **Discovered during task review, verified empirically: a pre-existing |
| 62 | + inconsistency between `can_decode` and `decode` for genuinely |
| 63 | + unhashable models, which this refactor incidentally fixes.** Today, |
| 64 | + `_probe_can_decode` calls `self._get_adapter(model)` inside a broad |
| 65 | + `except Exception: return False`. For a genuinely unhashable model |
| 66 | + (e.g. `typing.Annotated[int, []]` — confirmed unhashable, and confirmed |
| 67 | + buildable by both pydantic and msgspec), the *old* `_get_adapter`/ |
| 68 | + `_get_msgspec_decoder` propagate the cache-lookup `TypeError` uncaught, |
| 69 | + which that `except Exception` catches — so `can_decode` returns `False` |
| 70 | + for *any* unhashable model today, regardless of whether the underlying |
| 71 | + library could actually build a schema for it. Meanwhile `decode()` |
| 72 | + already works fine for the same model (it has its own separate |
| 73 | + fallback) — meaning today, `MissingDecoderError` can incorrectly fire |
| 74 | + for a model `decode()` would have handled correctly. Once |
| 75 | + `_get_or_build` swallows that same `TypeError` internally (which it |
| 76 | + must, to keep `decode()` working for unhashable models — see the point |
| 77 | + above), `_probe_can_decode`'s `try` no longer sees an exception for |
| 78 | + this case, so `can_decode` now returns `True` — consistent with |
| 79 | + `decode`'s actual capability. This is accepted as an intentional |
| 80 | + incidental fix (see Non-goals and Testing). |
| 81 | + |
| 82 | +## Non-goals |
| 83 | + |
| 84 | +- No *unintentional* behavior change from the caller's perspective. One |
| 85 | + intentional exception, accepted as a bonus fix rather than reverted: |
| 86 | + `can_decode` now returns `True` (not `False`) for a genuinely unhashable |
| 87 | + model that the underlying library can actually build a schema for — |
| 88 | + matching what `decode` already does for the same model, closing a |
| 89 | + pre-existing inconsistency. Every other input/output pair for |
| 90 | + `can_decode`/`decode` is unchanged, including unhashable models handled |
| 91 | + via a fresh, uncached build (still true — that mechanism is unchanged, |
| 92 | + only the *verdict* changes for the specific unhashable-but-buildable |
| 93 | + case). |
| 94 | +- Not extracting the tiny `if not is_X_installed: raise |
| 95 | + ImportError(MESSAGE)` check duplicated in both `__init__`s (2 lines, |
| 96 | + different flag and message per call — a shared helper would need both |
| 97 | + passed as parameters, more ceremony than it replaces). Same judgment |
| 98 | + already applied to `_internal/status.py`'s 3-line predicate functions |
| 99 | + in the original architecture review. |
| 100 | +- Not touching `_probe_can_decode` (genuinely different per decoder — the |
| 101 | + actual varying logic this refactor is careful to leave alone) or |
| 102 | + `_contains_custom_type` (msgspec-specific, unrelated concern). |
| 103 | +- Not touching the `ResponseDecoder` protocol, `_DecoderResolver`, or |
| 104 | + `_BoundDecoder` (Seam B's orchestration, already deep, unaffected). |
| 105 | + |
| 106 | +## Design |
| 107 | + |
| 108 | +### 1. `decoders/_caching.py` — new module |
| 109 | + |
| 110 | +```python |
| 111 | +"""Shared memoizing get-or-build cache for the decoder adapters (Seam C). |
| 112 | +
|
| 113 | +Imports neither pydantic nor msgspec — safe for both decoder modules to |
| 114 | +import without crossing Seam C's per-extra import isolation. |
| 115 | +""" |
| 116 | + |
| 117 | +import typing |
| 118 | + |
| 119 | + |
| 120 | +V = typing.TypeVar("V") |
| 121 | + |
| 122 | + |
| 123 | +def _get_or_build(cache: dict[type, V], model: type, build: typing.Callable[[], V]) -> V: |
| 124 | + """Return cache[model], building and memoizing it via build() on miss. |
| 125 | +
|
| 126 | + Unhashable models bypass the cache entirely: dict.get(model) raises |
| 127 | + TypeError, in which case this returns a fresh, uncached build() every |
| 128 | + call rather than raising. A real cached value (bool verdict or a |
| 129 | + built adapter/decoder) is never None, so a None result unambiguously |
| 130 | + means "not cached yet" — callers must not store None as a value. |
| 131 | + """ |
| 132 | + try: |
| 133 | + cached = cache.get(model) |
| 134 | + except TypeError: |
| 135 | + return build() |
| 136 | + if cached is not None: |
| 137 | + return cached |
| 138 | + result = build() |
| 139 | + cache[model] = result |
| 140 | + return result |
| 141 | +``` |
| 142 | + |
| 143 | +Placed alongside `decoders/_resolver.py` — both are private, cross-file |
| 144 | +modules inside `decoders/` shared by the seam's adapters, not moved to |
| 145 | +`_internal/` (which is for logic shared across otherwise-unrelated parts |
| 146 | +of the whole package, not decoders-specific plumbing). |
| 147 | + |
| 148 | +### 2. `PydanticDecoder` — four call sites become two, `decode()` simplifies |
| 149 | + |
| 150 | +```python |
| 151 | +from httpware.decoders._caching import _get_or_build |
| 152 | + |
| 153 | +# ... |
| 154 | + |
| 155 | +def _get_adapter(self, model: type[T]) -> "TypeAdapter[T]": |
| 156 | + return _get_or_build(self._adapters, model, lambda: TypeAdapter(model)) |
| 157 | + |
| 158 | +def can_decode(self, model: type) -> bool: |
| 159 | + """Return True iff pydantic can build a schema for `model`. |
| 160 | +
|
| 161 | + The verdict is memoized per `model` so a rejection (which costs a |
| 162 | + `PydanticSchemaGenerationError` round-trip) is not re-probed on every |
| 163 | + dispatch. Unhashable models skip the cache and probe fresh. |
| 164 | + """ |
| 165 | + return _get_or_build(self._can_decode_results, model, lambda: self._probe_can_decode(model)) |
| 166 | + |
| 167 | +def decode(self, content: bytes, model: type[T]) -> T: |
| 168 | + """Validate `content` as JSON against `model` in a single parse pass.""" |
| 169 | + adapter = self._get_adapter(model) |
| 170 | + return adapter.validate_json(content) |
| 171 | +``` |
| 172 | + |
| 173 | +`_probe_can_decode` is untouched. |
| 174 | + |
| 175 | +### 3. `MsgspecDecoder` — the same shape |
| 176 | + |
| 177 | +```python |
| 178 | +from httpware.decoders._caching import _get_or_build |
| 179 | + |
| 180 | +# ... |
| 181 | + |
| 182 | +def _get_msgspec_decoder(self, model: type[T]) -> "msgspec.json.Decoder[T]": |
| 183 | + return _get_or_build(self._msgspec_decoders, model, lambda: msgspec.json.Decoder(model)) |
| 184 | + |
| 185 | +def can_decode(self, model: type) -> bool: |
| 186 | + """Return True iff msgspec natively understands `model` end-to-end. |
| 187 | +
|
| 188 | + The verdict is memoized per `model`: the probe below (an uncached |
| 189 | + `type_info` call plus a recursive tree walk) runs once per type, not |
| 190 | + on every dispatch. Unhashable models skip the cache and probe fresh. |
| 191 | + """ |
| 192 | + return _get_or_build(self._can_decode_results, model, lambda: self._probe_can_decode(model)) |
| 193 | + |
| 194 | +def decode(self, content: bytes, model: type[T]) -> T: |
| 195 | + """Validate `content` as JSON against `model` in a single parse pass.""" |
| 196 | + decoder = self._get_msgspec_decoder(model) |
| 197 | + return decoder.decode(content) |
| 198 | +``` |
| 199 | + |
| 200 | +`_probe_can_decode` and `_contains_custom_type` are untouched. |
| 201 | + |
| 202 | +### 4. Test corrections (necessary, not optional) |
| 203 | + |
| 204 | +Two existing tests mock the wrong layer and must change, or they break |
| 205 | +once `decode()`'s fallback is removed: |
| 206 | + |
| 207 | +- `tests/test_decoders_pydantic.py::test_unhashable_model_falls_back_to_uncached_adapter` |
| 208 | + (currently `:134-150`) mocks `PydanticDecoder._get_adapter` itself with |
| 209 | + `side_effect=TypeError(...)` to exercise `decode()`'s fallback. Once |
| 210 | + `_get_adapter` delegates to `_get_or_build`, it can no longer raise |
| 211 | + `TypeError` for this reason, so the mock describes a scenario the real |
| 212 | + method can't produce. Rewrite to mock the cache dict instead |
| 213 | + (`decoder._adapters = MagicMock(); decoder._adapters.get.side_effect = |
| 214 | + TypeError(...)`), mirroring the pattern the existing |
| 215 | + `test_pydantic_can_decode_unhashable_model_does_not_raise` (`:236-241`) |
| 216 | + already uses for the verdict cache. Same assertions |
| 217 | + (`decoder.decode(b"42", int) == 42`, then a genuinely malformed payload |
| 218 | + still raises `pydantic.ValidationError`). |
| 219 | +- `tests/test_decoders_msgspec.py::test_unhashable_model_falls_back_to_uncached_decoder` |
| 220 | + (currently `:140-154`) — identical fix, mocking `decoder._msgspec_decoders` |
| 221 | + instead of `MsgspecDecoder._get_msgspec_decoder`. |
| 222 | + |
| 223 | +Once fixed this way, both tests directly exercise `_get_or_build`'s |
| 224 | +`TypeError`-bypass path for the object-cache flavor, complementing the |
| 225 | +existing `can_decode`-unhashable tests (verdict-cache flavor) — decent |
| 226 | +direct coverage of the new shared function without a dedicated test |
| 227 | +file. |
| 228 | + |
| 229 | +**Everything else in both test files is unaffected**, verified by reading |
| 230 | +every test in both files: |
| 231 | +- `test_can_decode_returns_false_when_decoder_build_raises` (msgspec |
| 232 | + `:130-137`) mocks `_get_msgspec_decoder` to test `_probe_can_decode`'s |
| 233 | + own broad `except Exception: return False` — untouched, since |
| 234 | + `_probe_can_decode` still calls `self._get_msgspec_decoder(model)` as a |
| 235 | + method regardless of its internal implementation. |
| 236 | +- The `patch.object(decoder, "_get_adapter", wraps=decoder._get_adapter)` |
| 237 | + spy tests (pydantic `:219`, `:229`) wrap the real method rather than |
| 238 | + replacing it — call-count assertions hold unchanged since the number of |
| 239 | + calls to `_get_adapter`/`_probe_can_decode` doesn't change. |
| 240 | +- `test_cache_invariance_*` (pydantic `:87-131`) patch the module-level |
| 241 | + `TypeAdapter` name directly — independent of internal call structure. |
| 242 | +- `test_pydantic_can_decode_uses_cache`/`test_msgspec_can_decode_uses_cache` |
| 243 | + check dict contents (`len(...) == 1`, membership) — unaffected, the |
| 244 | + dicts are populated the same way. |
| 245 | + |
| 246 | +### 5. Minor comment accuracy fix |
| 247 | + |
| 248 | +`test_cache_invariance_concurrent_first_calls_threadpool`'s comment |
| 249 | +(pydantic `:127-130`) says "the get→set sequence in `_get_adapter` is |
| 250 | +not [atomic]" — the get→set sequence now lives in `_get_or_build`. Update |
| 251 | +the comment to name the new location. |
| 252 | + |
| 253 | +## Testing |
| 254 | + |
| 255 | +- **Parity net:** every other existing test in both files stays green |
| 256 | + unchanged (see the itemized check above). |
| 257 | +- **Corrected tests:** the two rewritten unhashable-fallback tests, as |
| 258 | + specified in Design §4. |
| 259 | +- No new dedicated test file for `_get_or_build` — the corrected tests |
| 260 | + plus the existing `can_decode`-unhashable tests already exercise both |
| 261 | + value-type flavors (bool, built object) of its `TypeError`-bypass path, |
| 262 | + and every cache-hit/cache-miss branch is already exercised by the |
| 263 | + existing cache-content and call-count assertions. |
| 264 | +- **New regression tests for the intentional behavior fix** (added after |
| 265 | + task review surfaced the gap empirically): one test per decoder, using |
| 266 | + a genuinely unhashable model (`typing.Annotated[int, []]` — confirmed |
| 267 | + unhashable via `hash()`, confirmed buildable by both pydantic and |
| 268 | + msgspec), asserting `can_decode(model) is True` and that `decode(...)` |
| 269 | + on the same model succeeds and agrees — proving the two are consistent |
| 270 | + post-fix. No existing test used a genuinely unhashable model through |
| 271 | + the real (non-mocked) code path; the two corrected tests from Design §4 |
| 272 | + mock the cache dict with an otherwise-hashable model, which does not |
| 273 | + exercise this interaction. |
| 274 | +- `just lint && just test` both clean; 100% coverage maintained. |
| 275 | + |
| 276 | +## Risk |
| 277 | + |
| 278 | +- **Silently breaking the two rewritten tests' original intent** |
| 279 | + (unlikely × medium): the fix must preserve the same observable |
| 280 | + contract (unhashable model still decodes correctly via a fresh, |
| 281 | + uncached build) even though the mock target moves. *Mitigation:* keep |
| 282 | + the same assertions, only change what's mocked and how. |
| 283 | +- **Generic typing friction** (`_get_or_build`'s `V` vs. `T` narrowing at |
| 284 | + each call site) (unlikely × low): `self._adapters`/`self._msgspec_decoders` |
| 285 | + are already typed `dict[type, X[typing.Any]]` today, so the existing |
| 286 | + code already widens through `Any` in `_get_adapter`'s own return |
| 287 | + statement before this change — no new widening is introduced. |
| 288 | + *Mitigation:* `ty check` in the verification gate catches any real |
| 289 | + regression. |
0 commit comments