Skip to content

Commit 02d4b54

Browse files
lesnik512claude
andcommitted
fix(story-1.2): apply review fixes
- response.py: Response.text falls back to UTF-8 when the declared charset is unknown (wraps decode in try/except LookupError); class docstring now documents `elapsed` as wall-clock seconds. - tests: add quote-stripping coverage (charset="latin-1" / charset='latin-1'), add UTF-8 fallback test for unknown charset, and tighten equality tests with !=-against-variants for both Request and Response. - Story 1.2 doc: AC8 typo ("frozen on all 4 dataclasses" → "all 5"); status → done. - docs/deferred-work.md: log 10 items deferred from the review (charset parser robustness, header/URL/with_* validation, Timeout/Limits guards, multi-valued params, streaming bodies, json() charset, @Final). Post-patch: just lint-ci clean, 27 tests pass (+3 new), 100% coverage retained. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 502adcb commit 02d4b54

5 files changed

Lines changed: 81 additions & 5 deletions

File tree

‎docs/deferred-work.md‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,19 @@
22

33
Items raised in reviews that are real but not actionable now.
44

5+
## Deferred from: code review of story-1-2 (2026-05-13)
6+
7+
- **Charset parser robustness** — quoted whitespace, mismatched quotes, multi-`charset=` directives, substring false-positives (e.g. `boundary` containing `charset=`). (`src/httpware/response.py:21-26`)
8+
- **Header name/value validation** — `with_header` accepts CR/LF (injection), `None`, empty string. Lands with header-handling story (2.3 or later). (`src/httpware/request.py:21-23`)
9+
- **URL validation** — `with_url("")` accepts empty; `base_url` has no trailing-slash normalization. (`src/httpware/request.py:25-27`, `src/httpware/config.py:27-33`)
10+
- **`with_query(None)` handling** — currently accepted and breaks downstream iteration. (`src/httpware/request.py:33-35`)
11+
- **`Timeout` / `Limits` negative-value validation** — no `__post_init__` guard; nonsensical values silently accepted. (`src/httpware/config.py:10-22`)
12+
- **Multi-valued query params** — `Mapping[str, str]` cannot express `?tag=a&tag=b`. Type widening needed. (`src/httpware/request.py:8`)
13+
- **Streaming / async-iterable request bodies** — `body: bytes | None` only. Revisit in transport stories. (`src/httpware/request.py:11`)
14+
- **`with_headers` / `with_cookie` / `with_extension` merge helpers** — only `with_header` (single) and `with_query` (replace) exist. Story 2.3 will fill this in. (`src/httpware/request.py:20-35`)
15+
- **`Response.json()` honor declared charset** — `json.loads(bytes)` auto-detects only UTF-8/16/32. Real APIs vary. (`src/httpware/response.py:44-45`)
16+
- **`@final` to prevent subclassing** — frozen+slots subclassing is fragile. No current subclasser; defer until needed. (`src/httpware/request.py`, `response.py`, `config.py`)
17+
518
## Deferred from: code review of story-1-1 (2026-05-13)
619

720
- **Codecov upload fails on fork PRs** — fork PRs cannot access `CODECOV_TOKEN`; matches modern-di pattern, accepted tradeoff. (`.github/workflows/ci.yml:46-52`)

‎docs/stories/1-2-core-data-types.md‎

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ story_key: 1-2-core-data-types
33
epic: 1
44
story: 2
55
title: Core data types
6-
status: ready-for-dev
6+
status: done
77
created: 2026-05-13
88
input_documents:
99
- docs/prd.md
@@ -197,7 +197,7 @@ Followed TDD per module: write failing tests → confirm collection failure (RED
197197
- **AC5** ✓ `Timeout`, `Limits`, `ClientConfig` in `src/httpware/config.py` are all `@dataclass(frozen=True, slots=True)`. Defaults exactly match the spec. `ClientConfig.timeout` and `.limits` use `field(default_factory=Timeout)` / `field(default_factory=Limits)` — never bare instances. `Redactor` is intentionally not present (Story 5.3).
198198
- **AC6** ✓ `src/httpware/__init__.py` re-exports all five via explicit absolute imports; `__all__` is the sorted list `["ClientConfig", "Limits", "Request", "Response", "Timeout"]`. Smoke import succeeds.
199199
- **AC7** ✓ `just lint-ci` reports zero diagnostics across `eof-fixer`, `ruff format --check`, `ruff check --no-fix`, `ty check`.
200-
- **AC8** ✓ 24 tests cover every documented case: frozen on all 4 dataclasses, `with_*` immutability + new-instance identity + replace-semantics on `with_header`, equality, independent default mappings, charset case-insensitivity (parametrized over three casings), UTF-8 / latin-1 / missing-charset paths, JSON round-trip, default constructor returns documented defaults. `pytest` exits 0; coverage 100%.
200+
- **AC8** ✓ 24 tests cover every documented case: frozen on all 5 dataclasses, `with_*` immutability + new-instance identity + replace-semantics on `with_header`, equality, independent default mappings, charset case-insensitivity (parametrized over three casings), UTF-8 / latin-1 / missing-charset paths, JSON round-trip, default constructor returns documented defaults. `pytest` exits 0; coverage 100%.
201201

202202
**Deviations from the story spec (worth flagging for reviewer):**
203203

@@ -229,4 +229,26 @@ Followed TDD per module: write failing tests → confirm collection failure (RED
229229

230230
## Status
231231

232-
`review`
232+
`done`
233+
234+
### Review Findings
235+
236+
_Code review run: 2026-05-13. Reviewers: Blind Hunter, Edge Case Hunter, Acceptance Auditor. Acceptance Auditor reported **no AC violations**. 5 patches applied, 11 deferred, 36 dismissed as noise. Post-patch verification: `just lint-ci` clean, 27 tests pass (+3 new), 100% coverage retained._
237+
238+
- [x] [Review][Patch] `Response.text` now wraps `bytes.decode` in `try/except LookupError` and falls back to UTF-8 when the declared charset is unknown. [`src/httpware/response.py:42-46`] — applied
239+
- [x] [Review][Patch] `Response` class docstring now documents `elapsed` as wall-clock seconds from request send to response receipt. [`src/httpware/response.py:29-32`] — applied
240+
- [x] [Review][Patch] Added `test_response_text_strips_quotes_around_charset` (parametrized over `"latin-1"` and `'latin-1'`) covering the quote-stripping branch. [`tests/test_response.py`] — applied
241+
- [x] [Review][Patch] Equality tests now also assert `!=` against per-field variants (Request and Response). [`tests/test_request.py`, `tests/test_response.py`] — applied
242+
- [x] [Review][Patch] Completion Notes AC8 — "frozen on all 4 dataclasses" → "all 5". [`docs/stories/1-2-core-data-types.md:200`] — applied
243+
244+
- [x] [Review][Defer] Charset parser robustness — whitespace inside quotes, mismatched quotes, multiple `charset=` directives, `charset=` substring inside a quoted boundary param. [`src/httpware/response.py:21-26`] — deferred, pragmatic v0; revisit when transport tests reveal real-world breakage
245+
- [x] [Review][Defer] Header name/value validation (CR/LF injection, `None`, empty string) on `Request.with_header`. [`src/httpware/request.py:21-23`] — deferred to header-handling story (2.3 or later)
246+
- [x] [Review][Defer] URL validation — `with_url("")` accepts empty, `base_url` has no trailing-slash normalization. [`src/httpware/request.py:25-27`, `src/httpware/config.py:27-33`] — deferred, lands when transport composes URLs
247+
- [x] [Review][Defer] `with_query(None)` is currently accepted and breaks downstream iteration. [`src/httpware/request.py:33-35`] — deferred, type system already says `Mapping[str, str]`
248+
- [x] [Review][Defer] `Timeout` / `Limits` accept negative or zero values silently (no `__post_init__`). [`src/httpware/config.py:10-22`] — deferred, validation lands with transport integration
249+
- [x] [Review][Defer] `params: Mapping[str, str]` cannot express multi-valued query strings (`?tag=a&tag=b`). [`src/httpware/request.py:8`] — deferred, type widening tracked separately
250+
- [x] [Review][Defer] `body: bytes | None` precludes streaming / async-iterable bodies. [`src/httpware/request.py:11`] — deferred, intentional minimal scope; revisit in transport stories
251+
- [x] [Review][Defer] No `with_headers` / `with_cookie` / `with_extension` merge helpers; only `with_header` (single) and `with_query` (replace). [`src/httpware/request.py:20-35`] — deferred to Story 2.3 (merge/case-insensitive helpers)
252+
- [x] [Review][Defer] `Response.json()` ignores declared charset; `json.loads(bytes)` auto-detects only UTF-8/16/32 by BOM. [`src/httpware/response.py:44-45`] — deferred, most APIs are UTF-8; revisit if a real backend breaks
253+
- [x] [Review][Defer] `Request.body` / `Response.content` (bytes) render verbatim in the default dataclass `__repr__`, leaking large payloads / secrets into logs. [`src/httpware/request.py`, `src/httpware/response.py`] — deferred to Story 5.3 (`Redactor`)
254+
- [x] [Review][Defer] No `@final` decorator — frozen+slots subclassing is fragile and the spec doesn't bless it; an explicit `@final` would prevent the footgun. [`src/httpware/request.py`, `src/httpware/response.py`, `src/httpware/config.py`] — deferred, no current subclasser

‎src/httpware/response.py‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,10 @@ def _parse_charset(content_type: str) -> str | None:
2626

2727
@dataclass(frozen=True, slots=True)
2828
class Response:
29-
"""Immutable HTTP response value type."""
29+
"""Immutable HTTP response value type.
30+
31+
`elapsed` is wall-clock seconds from request send to response receipt.
32+
"""
3033

3134
status: int
3235
headers: Mapping[str, str]
@@ -38,7 +41,10 @@ class Response:
3841
def text(self) -> str:
3942
"""Decode `content` using the response's declared charset (default UTF-8)."""
4043
charset = _parse_charset(_get_content_type(self.headers)) or "utf-8"
41-
return self.content.decode(charset)
44+
try:
45+
return self.content.decode(charset)
46+
except LookupError:
47+
return self.content.decode("utf-8")
4248

4349
def json(self) -> Any: # noqa: ANN401
4450
"""Parse `content` as JSON."""

‎tests/test_request.py‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,9 @@ def test_request_equality_on_identical_fields() -> None:
2828
r1 = Request(method="GET", url="/x", headers={"a": "1"})
2929
r2 = Request(method="GET", url="/x", headers={"a": "1"})
3030
assert r1 == r2
31+
assert r1 != Request(method="POST", url="/x", headers={"a": "1"})
32+
assert r1 != Request(method="GET", url="/y", headers={"a": "1"})
33+
assert r1 != Request(method="GET", url="/x", headers={"a": "2"})
3134

3235

3336
def test_with_header_adds_when_absent() -> None:

‎tests/test_response.py‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,36 @@ def test_response_text_falls_back_to_utf8_on_missing_charset() -> None:
4848
assert resp.text == '{"x": 1}'
4949

5050

51+
@pytest.mark.parametrize(
52+
"content_type",
53+
[
54+
'text/plain; charset="latin-1"',
55+
"text/plain; charset='latin-1'",
56+
],
57+
)
58+
def test_response_text_strips_quotes_around_charset(content_type: str) -> None:
59+
body = "café".encode("latin-1")
60+
resp = Response(
61+
status=200,
62+
headers={"content-type": content_type},
63+
content=body,
64+
url="/",
65+
elapsed=0.0,
66+
)
67+
assert resp.text == "café"
68+
69+
70+
def test_response_text_falls_back_to_utf8_on_unknown_charset() -> None:
71+
resp = Response(
72+
status=200,
73+
headers={"content-type": "text/plain; charset=not-a-real-codec"},
74+
content=b"hello",
75+
url="/",
76+
elapsed=0.0,
77+
)
78+
assert resp.text == "hello"
79+
80+
5181
def test_response_json_parses_body() -> None:
5282
resp = Response(status=200, headers={}, content=b'{"a": 1, "b": [2, 3]}', url="/", elapsed=0.0)
5383
assert resp.json() == {"a": 1, "b": [2, 3]}
@@ -57,3 +87,5 @@ def test_response_equality_on_identical_fields() -> None:
5787
r1 = Response(status=200, headers={"a": "1"}, content=b"x", url="/", elapsed=0.5)
5888
r2 = Response(status=200, headers={"a": "1"}, content=b"x", url="/", elapsed=0.5)
5989
assert r1 == r2
90+
assert r1 != Response(status=200, headers={"a": "1"}, content=b"x", url="/", elapsed=0.6)
91+
assert r1 != Response(status=201, headers={"a": "1"}, content=b"x", url="/", elapsed=0.5)

0 commit comments

Comments
 (0)