feat(httputil): add Bind/BindEmpty request binding from path and query; v1.6.0

Bind and BindEmpty fill Req from path/query/json struct tags and validate
once, extending the typed decode->validate->call->encode pipeline to routes
with identifiers and filters. Conversion via builtins + encoding.TextUnmarshaler
(uuid.UUID, time.Time); malformed value -> 400 naming the parameter; default:
applies only when absent; repeated query -> slice; mis-tagged struct panics at
wiring. Purely additive; existing adapters unchanged. Coordinated lockstep v1.6.0.
This commit is contained in:
2026-08-13 23:11:23 -06:00
parent bcccfef443
commit a778227fc2
10 changed files with 920 additions and 14 deletions
+109
View File
@@ -0,0 +1,109 @@
# ADR-001 — `httputil` request binding: `Bind` and `BindEmpty`
**Status:** Accepted
**Date:** 2026-08-13
**Module:** `web` (`httputil`)
**Shipped:** v1.6.0
## Context
The typed adapters (`Handle`, `HandleNoBody`, `HandleEmpty`) keep
`http.ResponseWriter` and `*http.Request` out of business handlers so that decode,
validation, encoding, status selection and error mapping happen once inside the
framework. But they define a handler's input as **the JSON body and nothing else**.
An HTTP request carries four input channels; only the body was reachable from a
typed handler:
| Channel | Before v1.6.0 |
|---|---|
| JSON body | decoded into `Req`, validated |
| Path parameter | only via `chi.URLParamFromCtx(ctx, …)` — untyped `string`, unvalidated, off the handler signature |
| Query parameter | unreachable — the adapters never pass `r.URL` through |
| Header | out of scope by design (middleware's concern) |
So the two most ordinary REST shapes — `GET /roles/{id}` and
`GET /roles?page=2&per_page=50` — had to abandon the typed adapters for
`HandlerFunc` and hand-write the decode, the validation call, the encoding and the
status. This is a **correctness** problem, not only ergonomics: `HandlerFunc` is the
single path by which a handler reaches production without `v.Struct(req)` ever
running. Two failure modes followed, both observed in a consumer (`kch-core-svc`):
1. **Unvalidated bounds** — a list endpoint that forgets to clamp answers
`?per_page=99999`; the validator that would refuse it is not in the code path.
2. **Client mistakes reported as server faults** — a hand-written
`strconv.Atoi(...)` whose error is wrapped as internal answers **500** for a
plain **400**, misclassifying a client error as an outage.
A third, smaller trap: `HandleEmpty` decodes a body unconditionally, so a bodiless
`DELETE` fails on `io.EOF` before the handler runs.
## Decision
Add a fourth adapter family, `Bind` and `BindEmpty`, that fills `Req` from path,
query **and** body — each field declaring its source with a struct tag — and
validates the assembled struct once with the `valid.Validator` already in scope.
**The handler signature does not change**; only what `Req` may be filled from does.
`JSON`, `NoContent`, `Error` and `WithStatus` are reused unmodified.
### Binding rules
1. **One source per field.** A field carries at most one of `path:`/`query:`/`json:`.
Two source tags is a programming error, detected when the type is first reflected
over and reported as a **wiring failure at startup**, not per request.
2. **No body is not an error.** An empty body (`GET`, `DELETE`, `Content-Length: 0`)
decodes to `io.EOF`, which is treated as "no body" — retiring the `HandleEmpty`
bodiless trap.
3. **Conversion** covers `string`, the sized integer/unsigned/float types, `bool`,
and anything whose pointer implements `encoding.TextUnmarshaler`. That one
interface is the whole extensibility story: `uuid.UUID` and `time.Time` bind with
no special-casing and no new dependency in `web`.
4. **A conversion failure is `ErrInvalidInput`, naming the parameter** — never
`ErrInternal`. This turns failure mode 2 from a 500 into the 400 it always was.
5. **`default:` applies only when a parameter is absent** — never when present and
empty, because `?q=` is a caller deliberately clearing a filter. (Pair `default:`
with a plain `min=1`, not `omitempty,min=1`: the default guarantees presence, and
`omitempty` would let an explicit `?page=0` skip the bound.)
6. **Repeated query parameters bind to a slice.** A comma inside a single value is
**not** split — one syntax, so a value legitimately containing a comma survives.
7. **Metadata is parsed once per type and cached**, as `encoding/json` does. A
startup benchmark keeps the per-request cost flat in the number of tagged fields.
### Out of scope: header binding
There is no `header:` tag, in this version or a later one. Headers are middleware's
concern (authentication, request identity, tenancy). A `header:` tag would make one
specific mistake ergonomic — filling a tenant/actor identifier from a value the
client fully controls — which `kch-core-svc`'s own ADR-007 forbids. Reducing that
mistake to one word in a struct tag would make it likely rather than merely possible.
## Options considered
- **Fourth adapter family** *(chosen)* — additive, one concept, existing call sites
untouched.
- **Extend the existing three** — smallest diff, but `HandleNoBody` would need a
`Req` type parameter it does not have: a breaking signature change to the
most-used adapter, for the benefit of routes that could equally call something new.
- **One adapter per channel combination** (`HandleQuery`, `HandlePathBody`, …) —
eight exported functions expressing one idea; the caller must pick correctly each
time.
- **An `Option`** (`Handle(v, logger, fn, WithBinding())`) — leaves two ways to
express one thing, and `Option` currently means "adjust the response", not "change
how the request is read".
- **Leave it to `HandlerFunc`** — the status quo, and the only route to production
without validation. REST resources with identifiers are not an edge case.
## Consequences
- **Purely additive.** `Handle`, `HandleNoBody`, `HandleEmpty` and `HandlerFunc` are
behaviourally identical to v1.5.0; adoption is per route and per service.
- **`HandlerFunc`'s doc comment is amended** — it stops advertising itself for path
parameters and remains the answer for genuinely custom responses (streaming, file
downloads, non-JSON content types).
- **Routing coupling is acknowledged, not abstracted.** Path binding asks chi for a
named parameter, so `httputil` imports `chi/v5` directly (it was already a `web`
module dependency). `web/server` is chi and does not pretend to be swappable; an
indirection layer nothing else uses would cost more than it buys.
- **Reflection enters `httputil`** (a package that previously did none), mitigated by
the per-type cache and kept honest by the benchmark.
- **A new failure mode at startup, by design** — a mis-tagged struct fails the
service at boot rather than on the first request that exercises it.
+7
View File
@@ -9,6 +9,12 @@ No module-level ADRs for v1.0.0 — all design decisions were consistent with
existing framework principles (ADR-001 through ADR-003 from `core`, framework
ADRs 001004 from `docs`). No contested choices required a record.
Module ADRs:
| ADR | Title | Shipped |
|---|---|---|
| [ADR-001](ADR-001-request-binding.md) | `httputil` request binding — `Bind` / `BindEmpty` | v1.6.0 |
Decisions worth noting (not ADR-worthy individually):
| Decision | Outcome | Rationale |
@@ -21,3 +27,4 @@ Decisions worth noting (not ADR-worthy individually):
| Background goroutine for in-memory eviction | `time.Ticker` goroutine | Avoids `worker` module dependency; in-memory store is self-contained |
| `mw.RequestIDFrom` resolver sees the request (v1.4.0) | New entry point takes `func(*http.Request) string`; `RequestID` becomes a request-ignoring wrapper over it | An inbound correlation id must be able to survive this boundary, but acceptability is per-service — a typed audit column rejects what an opaque log accepts. The framework provides plumbing only (context + header, once per request); the app owns policy (which header, validation, generation fallback). An empty resolver result attaches nothing rather than a silently-empty value |
| `httputil` success status is configurable (v1.5.0) | `Handle`/`HandleNoBody`/`HandleEmpty` take `opts ...Option`; `WithStatus(code)` overrides the default (200 / 200 / 204) | 201 Created / 202 Accepted are common and were only reachable by hand-rolling the handler (losing decode+validate+error-mapping). Variadic options are non-breaking and future-extensible (headers, etc.). `WithStatus` is success-only (2xx) and panics at wiring on a non-2xx code — a wrong status is a routing mistake that should fail to boot, not surface at runtime; error status stays separate, resolved from the xerror by `Error` |
| `httputil` request binding (v1.6.0) | `Bind`/`BindEmpty` fill `Req` from `path:`/`query:`/`json:` tags and validate once; see [ADR-001](ADR-001-request-binding.md) | Path/query were only reachable via `HandlerFunc`, the one route to production with no validation (unvalidated bounds; client mistakes as 500s). `encoding.TextUnmarshaler` is the whole extensibility story (uuid/time, no new dep). Mis-tagged struct panics at wiring. Purely additive |