Skip to content

Commit b3ec991

Browse files
thcedthced
committed
docs: Add extra-route interface design spec
1 parent f24ea84 commit b3ec991

1 file changed

Lines changed: 208 additions & 0 deletions

File tree

‎docs/superpowers/specs/2026-05-20-extra-route-interface-design.md‎

Lines changed: 208 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,208 @@
1+
# Extra-route interface, ExceptionHandler decoupling, and problem+json via registered TypeMapper
2+
3+
Date: 2026-05-20
4+
5+
## Problem
6+
7+
The library's stated goal is to wrap `com.sun.net.httpserver.HttpServer` behind transport-neutral abstractions (`Request`, `Response`, `RequestHandler`). Today three public API surfaces still leak the JDK types:
8+
9+
1. `OpenApiServer.Builder.extraRoute(String, com.sun.net.httpserver.HttpHandler)` — the registration point for routes that bypass OpenAPI (alive / health / spec endpoints).
10+
2. `Handlers.aliveHandler()`, `healthHandler(...)`, `specHandler(...)`, `notFoundHandler()` — return `HttpHandler`.
11+
3. `ExceptionHandler.handle(HttpExchange, Throwable)` — exposes `HttpExchange` to user code that defines a custom error mapper.
12+
13+
This couples the public API to the JDK server and blocks a future swap to a different transport (Netty, Helidon Níma, etc.).
14+
15+
Separately, `internal/ProblemDetailRenderer` hand-rolls JSON for RFC 7807 problem+json bodies, with bespoke escape logic for quotes, backslashes, control characters, etc. The comment justifies this with "Hand-rolled to avoid pulling in a JSON library", but a JSON `TypeMapper` is now mandatory on every `OpenApiServer` (user-supplied, or auto-detected via the Gson fallback), so the hand-rolled path is no longer justified — and it's an unmaintained second escaper that can drift from the real one.
16+
17+
## Goals
18+
19+
- Remove every `com.sun.net.httpserver` import from `com.retailsvc.http.*` (excluding `com.retailsvc.http.internal.*`).
20+
- Express extra routes through the same `RequestHandler` / `Request` / `Response` triple already used for OpenAPI operations.
21+
- Express exception mapping as `Throwable → Response`, rendered by the framework.
22+
- Delete `ProblemDetailRenderer`; serialize problem+json bodies through the registered JSON `TypeMapper`.
23+
24+
## Non-goals
25+
26+
- Path templating for extra routes (still exact match).
27+
- OpenAPI validation for extra routes (still bypassed).
28+
- Body parsing through `TypeMapper` for extra routes (handlers see raw bytes via `Request.bytes()`).
29+
- Migrating the `internal/*` filters and handlers off `HttpExchange` — they are the transport adapter and stay coupled by design.
30+
31+
## Design
32+
33+
### 1. `Request` gains `method()`
34+
35+
Add an `HttpMethod method` field and accessor to `com.retailsvc.http.Request`. The existing `com.retailsvc.http.spec.HttpMethod` enum is reused (already public, already covers GET/POST/PUT/DELETE/PATCH/HEAD/OPTIONS/TRACE/CONNECT, and is used by `Router` and `MethodNotAllowedException`).
36+
37+
Construction sites:
38+
39+
- `RequestPreparationFilter` — already parses `HttpMethod method = HttpMethod.parse(exchange.getRequestMethod())` before routing; pass it into the `Request` constructor.
40+
- New `ExtraRouteAdapter` (see §3) — passes its own parsed method.
41+
42+
The 8-arg `Request` constructor grows to 9-arg. The existing `@SuppressWarnings("java:S107")` annotation already documents that this parameter list is intentionally flat at the adapter boundary; that justification still applies.
43+
44+
`withPrincipals` is updated to thread `method` through.
45+
46+
### 2. `extraRoute` accepts `RequestHandler`
47+
48+
```java
49+
public Builder extraRoute(String path, RequestHandler handler) { ... }
50+
```
51+
52+
The old `HttpHandler` overload is removed (not deprecated). Pre-1.0; only internal callers (the `Handlers.*` factories) and tests use it, and they all migrate in the same PR.
53+
54+
The `Builder.extras` field becomes `LinkedHashMap<String, RequestHandler>`.
55+
`HandlerConfig.extras` follows.
56+
57+
### 3. `ExtraRouteAdapter` (internal)
58+
59+
New `internal/ExtraRouteAdapter implements HttpHandler`. Bridges between the JDK transport and the user's `RequestHandler` for an extra path. For each registered extra, `OpenApiServer` wires:
60+
61+
```
62+
HttpContext extraCtx = httpServer.createContext(path);
63+
extraCtx.getFilters().add(new ExceptionFilter(exceptionHandler));
64+
extraCtx.setHandler(new ExtraRouteAdapter(handler, bodyMappers, renderer));
65+
```
66+
67+
On each request, `ExtraRouteAdapter.handle(exchange)`:
68+
69+
1. Reads body bytes (`exchange.getRequestBody().readAllBytes()`).
70+
2. Parses the method via `HttpMethod.parse(exchange.getRequestMethod())`.
71+
3. Constructs a `Request` with: body bytes, `parsed=null`, `bodyMapper=null`, `operationId=null`, `pathParameters=Map.of()`, `rawQuery`, `headerLookup`, `principals=Map.of()`, `method`.
72+
4. Calls `handler.handle(request)`.
73+
5. Renders the returned `Response` via the existing `ResponseRenderer` (the same renderer instance used for OpenAPI operations is reused — no behaviour drift between extras and operations).
74+
75+
Exceptions thrown by the handler propagate to the outer `ExceptionFilter` exactly like operation handlers do today, so the same `ExceptionHandler` is invoked.
76+
77+
`Response` features (streaming, byte body, JSON body, status-only, headers) all work for extras without code duplication because `ResponseRenderer` is shared.
78+
79+
### 4. `ExceptionHandler` returns a `Response`
80+
81+
```java
82+
@FunctionalInterface
83+
public interface ExceptionHandler {
84+
Response handle(Throwable t);
85+
}
86+
```
87+
88+
`ExceptionFilter` becomes:
89+
90+
```java
91+
public void doFilter(HttpExchange exchange, Chain chain) throws IOException {
92+
try {
93+
chain.doFilter(exchange);
94+
} catch (RuntimeException | IOException t) {
95+
Response response = handler.handle(t);
96+
renderer.render(exchange, response);
97+
}
98+
}
99+
```
100+
101+
`ExceptionFilter` is constructed with both the handler and the shared `ResponseRenderer`. (`renderer.render(exchange, response)` already exists for the normal path; reused here.)
102+
103+
`Handlers.defaultExceptionHandler(TypeMapper jsonMapper)` takes the registered JSON mapper and becomes:
104+
105+
```java
106+
public static ExceptionHandler defaultExceptionHandler(TypeMapper jsonMapper) {
107+
Objects.requireNonNull(jsonMapper, "jsonMapper");
108+
return t -> switch (t) {
109+
case ValidationException ve -> Response.bytes(
110+
HTTP_BAD_REQUEST,
111+
jsonMapper.writeTo(ProblemDetail.forValidation(ve.error())),
112+
"application/problem+json");
113+
case NotFoundException _ -> Response.notFound();
114+
case MethodNotAllowedException mna -> Response.status(HTTP_BAD_METHOD)
115+
.withHeader("Allow", mna.allowed().stream()
116+
.map(Enum::name).collect(Collectors.joining(", ")));
117+
default -> {
118+
LOG.error("Unhandled exception in handler", t);
119+
yield Response.status(HTTP_INTERNAL_ERROR);
120+
}
121+
};
122+
}
123+
```
124+
125+
`OpenApiServer.Builder.build()` supplies the resolved JSON mapper when no custom `ExceptionHandler` is set (the JSON mapper has already been resolved at that point via `resolveBodyMappers`).
126+
127+
Rationale: the exception path runs before a `Request` is necessarily built (e.g. a malformed URI in `RequestPreparationFilter` itself), so the handler signature cannot take `Request` — the simplest pre-`Request` signature is the right one.
128+
129+
### 4a. `ProblemDetail` record replaces `ProblemDetailRenderer`
130+
131+
New `internal/ProblemDetail` record (or public if we want users to be able to return problem+json from handlers — TBD; start internal):
132+
133+
```java
134+
record ProblemDetail(
135+
String type, String title, int status, String detail,
136+
String pointer, String keyword) {
137+
static ProblemDetail forValidation(ValidationError e) {
138+
return new ProblemDetail(
139+
"about:blank", "Bad Request", 400, e.message(), e.pointer(), e.keyword());
140+
}
141+
}
142+
```
143+
144+
Serialization runs through whichever JSON `TypeMapper` the server is configured with (Gson by default, or Jackson if the user registered one). Field order and `null` handling are whatever the mapper produces — Gson and Jackson both emit fields in declaration order and skip nulls when configured; field-presence is asserted in tests against the actual configured mapper rather than a hand-rolled string.
145+
146+
`ProblemDetailRenderer` is deleted along with its bespoke escape logic.
147+
148+
### 5. `Handlers.*` factories migrate
149+
150+
Return types change from `HttpHandler` to `RequestHandler`. The 405-on-non-GET/HEAD check, previously a `MethodLimitedHandler` wrapper, is inlined in each factory:
151+
152+
```java
153+
public static RequestHandler aliveHandler() {
154+
return req -> switch (req.method()) {
155+
case GET, HEAD -> Response.empty();
156+
default -> Response.status(HTTP_BAD_METHOD)
157+
.withHeader("Allow", "GET, HEAD");
158+
};
159+
}
160+
```
161+
162+
`healthHandler(TypeMapper, Supplier<HealthOutcome>)` and `specHandler(String)` follow the same shape. For `specHandler`, `HEAD` returns `Response.bytes(200, new byte[0], contentType).withHeader("Content-Length", String.valueOf(bytes.length))` to preserve the existing HEAD-omits-body behaviour.
163+
164+
`Handlers.notFoundHandler()` is dropped from the public API and moved to `internal/NotFoundHandler` (the framework's catch-all `/` context is its only caller).
165+
166+
`MethodLimitedHandler` (internal) is deleted.
167+
168+
### 6. Public API surface after the change
169+
170+
`com.retailsvc.http.*` (excluding `internal.*`) contains zero references to `com.sun.net.httpserver`. Grep verification:
171+
172+
```
173+
grep -rn "com\.sun\.net\.httpserver" src/main/java/com/retailsvc/http/ \
174+
| grep -v "/internal/"
175+
```
176+
177+
returns no results.
178+
179+
## Migration steps
180+
181+
1. Add `method` to `Request`; update `RequestPreparationFilter` to pass it.
182+
2. Introduce `ExtraRouteAdapter`; switch `OpenApiServer` extras wiring to it.
183+
3. Change `Builder.extraRoute` signature; update `HandlerConfig` and tests.
184+
4. Change `ExceptionHandler` signature; update `ExceptionFilter` to accept a `ResponseRenderer`; rewrite `Handlers.defaultExceptionHandler(TypeMapper)` and wire it from `Builder.build()`.
185+
5. Add `internal/ProblemDetail`; delete `internal/ProblemDetailRenderer`.
186+
6. Migrate `Handlers.aliveHandler/healthHandler/specHandler` to `RequestHandler` with inline 405 checks.
187+
7. Move `Handlers.notFoundHandler` to `internal/NotFoundHandler`.
188+
8. Delete `MethodLimitedHandler`.
189+
9. Update tests: `OpenApiServerBuilderTest`, `ExtraHandlersIT`, `HandlersTest` (if present), exception-handler tests (problem+json wire-shape now asserted as parsed JSON, not byte-equality), integration tests.
190+
191+
## Test plan
192+
193+
- Existing integration tests for extra routes (`ExtraHandlersIT`) pass unchanged behaviourally (paths still return the same bytes/status).
194+
- `OpenApiServerBuilderTest` covers the duplicate-path rule for the new signature.
195+
- New unit test: `ExtraRouteAdapter` constructs a `Request` with `operationId=null`, empty `pathParams`, empty `principals`, correct method, raw query, and body bytes; invokes the user handler; renders the response.
196+
- Default exception handler produces the same wire output for the four known exception classes as the current implementation (byte-for-byte for the problem+json case).
197+
- Custom exception handlers: regression test that user-supplied `ExceptionHandler` returning `Response.of(418, body)` is rendered.
198+
199+
## Risks
200+
201+
- **Behavioural drift in exception rendering.** Today the default handler writes headers and body directly via hand-rolled JSON. Routing through `Response`/`ResponseRenderer` and the registered JSON mapper changes both the code path and the exact bytes of the problem+json document (different mappers may differ in whitespace, null-omission, or field order). Mitigation: assert wire shape by parsing the response with the same mapper and comparing field-by-field, not byte-by-byte. Document that the exact byte output of problem+json depends on the registered JSON mapper.
202+
- **Pre-`Request` exceptions losing context.** The new `ExceptionHandler` signature has no `Request`. User code that wanted to log the request path on error must use logging in the handler itself. Acceptable: the default handler already does not use request context, and user custom handlers that need it can attach an `OpenApiServer.builder().interceptor(...)` to capture request info into an MDC.
203+
- **`Request.method()` non-null for OpenAPI handlers.** Since the OpenAPI router already dispatches by method, this is redundant for those handlers but consistent and cheap.
204+
205+
## Out of scope
206+
207+
- Removing `HttpExchange` from `internal/*`. The internal package is the transport adapter and intentionally coupled to the JDK server.
208+
- A swap-the-transport SPI. Even with this change shipped, swapping transports still requires reworking `internal/*` filters; the API surface, however, no longer blocks that work.

0 commit comments

Comments
 (0)