From b3ec99180af09f1e566b91fe67512962003f3204 Mon Sep 17 00:00:00 2001 From: Thomas Cederholm Date: Wed, 20 May 2026 11:36:57 +0200 Subject: [PATCH 01/11] docs: Add extra-route interface design spec --- ...2026-05-20-extra-route-interface-design.md | 208 ++++++++++++++++++ 1 file changed, 208 insertions(+) create mode 100644 docs/superpowers/specs/2026-05-20-extra-route-interface-design.md diff --git a/docs/superpowers/specs/2026-05-20-extra-route-interface-design.md b/docs/superpowers/specs/2026-05-20-extra-route-interface-design.md new file mode 100644 index 0000000..f738464 --- /dev/null +++ b/docs/superpowers/specs/2026-05-20-extra-route-interface-design.md @@ -0,0 +1,208 @@ +# Extra-route interface, ExceptionHandler decoupling, and problem+json via registered TypeMapper + +Date: 2026-05-20 + +## Problem + +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: + +1. `OpenApiServer.Builder.extraRoute(String, com.sun.net.httpserver.HttpHandler)` — the registration point for routes that bypass OpenAPI (alive / health / spec endpoints). +2. `Handlers.aliveHandler()`, `healthHandler(...)`, `specHandler(...)`, `notFoundHandler()` — return `HttpHandler`. +3. `ExceptionHandler.handle(HttpExchange, Throwable)` — exposes `HttpExchange` to user code that defines a custom error mapper. + +This couples the public API to the JDK server and blocks a future swap to a different transport (Netty, Helidon Níma, etc.). + +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. + +## Goals + +- Remove every `com.sun.net.httpserver` import from `com.retailsvc.http.*` (excluding `com.retailsvc.http.internal.*`). +- Express extra routes through the same `RequestHandler` / `Request` / `Response` triple already used for OpenAPI operations. +- Express exception mapping as `Throwable → Response`, rendered by the framework. +- Delete `ProblemDetailRenderer`; serialize problem+json bodies through the registered JSON `TypeMapper`. + +## Non-goals + +- Path templating for extra routes (still exact match). +- OpenAPI validation for extra routes (still bypassed). +- Body parsing through `TypeMapper` for extra routes (handlers see raw bytes via `Request.bytes()`). +- Migrating the `internal/*` filters and handlers off `HttpExchange` — they are the transport adapter and stay coupled by design. + +## Design + +### 1. `Request` gains `method()` + +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`). + +Construction sites: + +- `RequestPreparationFilter` — already parses `HttpMethod method = HttpMethod.parse(exchange.getRequestMethod())` before routing; pass it into the `Request` constructor. +- New `ExtraRouteAdapter` (see §3) — passes its own parsed method. + +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. + +`withPrincipals` is updated to thread `method` through. + +### 2. `extraRoute` accepts `RequestHandler` + +```java +public Builder extraRoute(String path, RequestHandler handler) { ... } +``` + +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. + +The `Builder.extras` field becomes `LinkedHashMap`. +`HandlerConfig.extras` follows. + +### 3. `ExtraRouteAdapter` (internal) + +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: + +``` +HttpContext extraCtx = httpServer.createContext(path); +extraCtx.getFilters().add(new ExceptionFilter(exceptionHandler)); +extraCtx.setHandler(new ExtraRouteAdapter(handler, bodyMappers, renderer)); +``` + +On each request, `ExtraRouteAdapter.handle(exchange)`: + +1. Reads body bytes (`exchange.getRequestBody().readAllBytes()`). +2. Parses the method via `HttpMethod.parse(exchange.getRequestMethod())`. +3. Constructs a `Request` with: body bytes, `parsed=null`, `bodyMapper=null`, `operationId=null`, `pathParameters=Map.of()`, `rawQuery`, `headerLookup`, `principals=Map.of()`, `method`. +4. Calls `handler.handle(request)`. +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). + +Exceptions thrown by the handler propagate to the outer `ExceptionFilter` exactly like operation handlers do today, so the same `ExceptionHandler` is invoked. + +`Response` features (streaming, byte body, JSON body, status-only, headers) all work for extras without code duplication because `ResponseRenderer` is shared. + +### 4. `ExceptionHandler` returns a `Response` + +```java +@FunctionalInterface +public interface ExceptionHandler { + Response handle(Throwable t); +} +``` + +`ExceptionFilter` becomes: + +```java +public void doFilter(HttpExchange exchange, Chain chain) throws IOException { + try { + chain.doFilter(exchange); + } catch (RuntimeException | IOException t) { + Response response = handler.handle(t); + renderer.render(exchange, response); + } +} +``` + +`ExceptionFilter` is constructed with both the handler and the shared `ResponseRenderer`. (`renderer.render(exchange, response)` already exists for the normal path; reused here.) + +`Handlers.defaultExceptionHandler(TypeMapper jsonMapper)` takes the registered JSON mapper and becomes: + +```java +public static ExceptionHandler defaultExceptionHandler(TypeMapper jsonMapper) { + Objects.requireNonNull(jsonMapper, "jsonMapper"); + return t -> switch (t) { + case ValidationException ve -> Response.bytes( + HTTP_BAD_REQUEST, + jsonMapper.writeTo(ProblemDetail.forValidation(ve.error())), + "application/problem+json"); + case NotFoundException _ -> Response.notFound(); + case MethodNotAllowedException mna -> Response.status(HTTP_BAD_METHOD) + .withHeader("Allow", mna.allowed().stream() + .map(Enum::name).collect(Collectors.joining(", "))); + default -> { + LOG.error("Unhandled exception in handler", t); + yield Response.status(HTTP_INTERNAL_ERROR); + } + }; +} +``` + +`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`). + +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. + +### 4a. `ProblemDetail` record replaces `ProblemDetailRenderer` + +New `internal/ProblemDetail` record (or public if we want users to be able to return problem+json from handlers — TBD; start internal): + +```java +record ProblemDetail( + String type, String title, int status, String detail, + String pointer, String keyword) { + static ProblemDetail forValidation(ValidationError e) { + return new ProblemDetail( + "about:blank", "Bad Request", 400, e.message(), e.pointer(), e.keyword()); + } +} +``` + +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. + +`ProblemDetailRenderer` is deleted along with its bespoke escape logic. + +### 5. `Handlers.*` factories migrate + +Return types change from `HttpHandler` to `RequestHandler`. The 405-on-non-GET/HEAD check, previously a `MethodLimitedHandler` wrapper, is inlined in each factory: + +```java +public static RequestHandler aliveHandler() { + return req -> switch (req.method()) { + case GET, HEAD -> Response.empty(); + default -> Response.status(HTTP_BAD_METHOD) + .withHeader("Allow", "GET, HEAD"); + }; +} +``` + +`healthHandler(TypeMapper, Supplier)` 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. + +`Handlers.notFoundHandler()` is dropped from the public API and moved to `internal/NotFoundHandler` (the framework's catch-all `/` context is its only caller). + +`MethodLimitedHandler` (internal) is deleted. + +### 6. Public API surface after the change + +`com.retailsvc.http.*` (excluding `internal.*`) contains zero references to `com.sun.net.httpserver`. Grep verification: + +``` +grep -rn "com\.sun\.net\.httpserver" src/main/java/com/retailsvc/http/ \ + | grep -v "/internal/" +``` + +returns no results. + +## Migration steps + +1. Add `method` to `Request`; update `RequestPreparationFilter` to pass it. +2. Introduce `ExtraRouteAdapter`; switch `OpenApiServer` extras wiring to it. +3. Change `Builder.extraRoute` signature; update `HandlerConfig` and tests. +4. Change `ExceptionHandler` signature; update `ExceptionFilter` to accept a `ResponseRenderer`; rewrite `Handlers.defaultExceptionHandler(TypeMapper)` and wire it from `Builder.build()`. +5. Add `internal/ProblemDetail`; delete `internal/ProblemDetailRenderer`. +6. Migrate `Handlers.aliveHandler/healthHandler/specHandler` to `RequestHandler` with inline 405 checks. +7. Move `Handlers.notFoundHandler` to `internal/NotFoundHandler`. +8. Delete `MethodLimitedHandler`. +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. + +## Test plan + +- Existing integration tests for extra routes (`ExtraHandlersIT`) pass unchanged behaviourally (paths still return the same bytes/status). +- `OpenApiServerBuilderTest` covers the duplicate-path rule for the new signature. +- 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. +- 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). +- Custom exception handlers: regression test that user-supplied `ExceptionHandler` returning `Response.of(418, body)` is rendered. + +## Risks + +- **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. +- **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. +- **`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. + +## Out of scope + +- Removing `HttpExchange` from `internal/*`. The internal package is the transport adapter and intentionally coupled to the JDK server. +- 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. From 5c9d2a903b4f516ef4cd932e3054389c42d5cd90 Mon Sep 17 00:00:00 2001 From: Thomas Cederholm Date: Wed, 20 May 2026 11:40:10 +0200 Subject: [PATCH 02/11] docs: Add BadRequestException to design spec --- ...2026-05-20-extra-route-interface-design.md | 81 +++++++++++++++++-- 1 file changed, 75 insertions(+), 6 deletions(-) diff --git a/docs/superpowers/specs/2026-05-20-extra-route-interface-design.md b/docs/superpowers/specs/2026-05-20-extra-route-interface-design.md index f738464..54a9e7b 100644 --- a/docs/superpowers/specs/2026-05-20-extra-route-interface-design.md +++ b/docs/superpowers/specs/2026-05-20-extra-route-interface-design.md @@ -110,6 +110,10 @@ public static ExceptionHandler defaultExceptionHandler(TypeMapper jsonMapper) { HTTP_BAD_REQUEST, jsonMapper.writeTo(ProblemDetail.forValidation(ve.error())), "application/problem+json"); + case BadRequestException bre -> Response.bytes( + bre.status(), + jsonMapper.writeTo(ProblemDetail.forBadRequest(bre)), + "application/problem+json"); case NotFoundException _ -> Response.notFound(); case MethodNotAllowedException mna -> Response.status(HTTP_BAD_METHOD) .withHeader("Allow", mna.allowed().stream() @@ -126,7 +130,55 @@ public static ExceptionHandler defaultExceptionHandler(TypeMapper jsonMapper) { 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. -### 4a. `ProblemDetail` record replaces `ProblemDetailRenderer` +### 4a. New public `BadRequestException` + +User handlers need a way to reject a syntactically-valid-but-semantically-wrong request (e.g. "email already taken" → 422 Unprocessable Content, "stale ETag" → 412 Precondition Failed) and have the framework render a problem+json response — without each handler re-implementing the problem-detail wire shape. + +New public class: + +```java +package com.retailsvc.http; + +public final class BadRequestException extends RuntimeException { + + private static final int DEFAULT_STATUS = 400; + + private final int status; + private final String pointer; // nullable + private final String keyword; // nullable + + public BadRequestException(String detail) { + this(DEFAULT_STATUS, detail, null, null); + } + + public BadRequestException(int status, String detail) { + this(status, detail, null, null); + } + + public BadRequestException(int status, String detail, String pointer, String keyword) { + super(Objects.requireNonNull(detail, "detail must not be null")); + if (status < 400 || status > 499) { + throw new IllegalArgumentException("status must be 4xx, got " + status); + } + this.status = status; + this.pointer = pointer; + this.keyword = keyword; + } + + public int status() { return status; } + public Optional pointer() { return Optional.ofNullable(pointer); } + public Optional keyword() { return Optional.ofNullable(keyword); } +} +``` + +Design points: + +- **Optional status, default 400.** The no-status constructor preserves the common "client sent something bad" case at zero ceremony; the overload accepts any 4xx for 409/412/422/429 etc. +- **4xx range enforced.** Throwing at construction prevents accidentally surfacing a 500 through a "bad request" path. 5xx errors should propagate as ordinary `RuntimeException` and hit the default 500 branch. +- **Optional pointer/keyword** mirror `ValidationError` so the problem+json document shape is consistent whether the body was rejected by the OpenAPI validator (`ValidationException`) or by handler-level domain rules (`BadRequestException`). +- **`detail`** uses `super(message)` so standard exception machinery (logging, stack traces) sees the human-readable reason. + +### 4b. `ProblemDetail` record replaces `ProblemDetailRenderer` New `internal/ProblemDetail` record (or public if we want users to be able to return problem+json from handlers — TBD; start internal): @@ -134,10 +186,26 @@ New `internal/ProblemDetail` record (or public if we want users to be able to re record ProblemDetail( String type, String title, int status, String detail, String pointer, String keyword) { + static ProblemDetail forValidation(ValidationError e) { return new ProblemDetail( "about:blank", "Bad Request", 400, e.message(), e.pointer(), e.keyword()); } + + static ProblemDetail forBadRequest(BadRequestException e) { + return new ProblemDetail( + "about:blank", + titleFor(e.status()), + e.status(), + e.getMessage(), + e.pointer().orElse(null), + e.keyword().orElse(null)); + } + + // Small map of common 4xx codes to RFC-7231 reason phrases. + // Unknown 4xx falls back to "Bad Request" — the type field is "about:blank", + // so per RFC 7807 the title is advisory; the precise meaning rides on status. + private static String titleFor(int status) { ... } } ``` @@ -182,11 +250,12 @@ returns no results. 2. Introduce `ExtraRouteAdapter`; switch `OpenApiServer` extras wiring to it. 3. Change `Builder.extraRoute` signature; update `HandlerConfig` and tests. 4. Change `ExceptionHandler` signature; update `ExceptionFilter` to accept a `ResponseRenderer`; rewrite `Handlers.defaultExceptionHandler(TypeMapper)` and wire it from `Builder.build()`. -5. Add `internal/ProblemDetail`; delete `internal/ProblemDetailRenderer`. -6. Migrate `Handlers.aliveHandler/healthHandler/specHandler` to `RequestHandler` with inline 405 checks. -7. Move `Handlers.notFoundHandler` to `internal/NotFoundHandler`. -8. Delete `MethodLimitedHandler`. -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. +5. Add public `BadRequestException`; wire a `case` for it into the default exception handler. +6. Add `internal/ProblemDetail`; delete `internal/ProblemDetailRenderer`. +7. Migrate `Handlers.aliveHandler/healthHandler/specHandler` to `RequestHandler` with inline 405 checks. +8. Move `Handlers.notFoundHandler` to `internal/NotFoundHandler`. +9. Delete `MethodLimitedHandler`. +10. Update tests: `OpenApiServerBuilderTest`, `ExtraHandlersIT`, `HandlersTest` (if present), exception-handler tests (problem+json wire-shape now asserted as parsed JSON, not byte-equality), integration tests, plus new tests for `BadRequestException` (default 400, custom status, 4xx-range guard, pointer/keyword propagation to problem+json). ## Test plan From 65881767532f8e9d3eaedfa5b9424e0045dcb2cf Mon Sep 17 00:00:00 2001 From: Thomas Cederholm Date: Wed, 20 May 2026 11:50:25 +0200 Subject: [PATCH 03/11] docs: Add implementation plan for extra-route interface --- .../plans/2026-05-20-extra-route-interface.md | 1337 +++++++++++++++++ 1 file changed, 1337 insertions(+) create mode 100644 docs/superpowers/plans/2026-05-20-extra-route-interface.md diff --git a/docs/superpowers/plans/2026-05-20-extra-route-interface.md b/docs/superpowers/plans/2026-05-20-extra-route-interface.md new file mode 100644 index 0000000..7274677 --- /dev/null +++ b/docs/superpowers/plans/2026-05-20-extra-route-interface.md @@ -0,0 +1,1337 @@ +# Extra-route interface, ExceptionHandler decoupling, BadRequestException Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Remove every `com.sun.net.httpserver` import from `com.retailsvc.http.*` (excluding `com.retailsvc.http.internal.*`), introduce `BadRequestException`, and serialize problem+json via the registered JSON `TypeMapper` instead of the hand-rolled `ProblemDetailRenderer`. + +**Architecture:** `extraRoute` accepts a `RequestHandler` (same triple as OpenAPI operations); an internal `ExtraRouteAdapter` bridges to the JDK `HttpHandler`. `ExceptionHandler.handle(Throwable)` returns a `Response`, rendered by `ResponseRenderer`. The default handler is built with the resolved JSON `TypeMapper` and serializes a `ProblemDetail` record. `Request.method()` exposes the HTTP method via the existing `com.retailsvc.http.spec.HttpMethod` enum. + +**Tech Stack:** Java 25, JUnit 5, AssertJ, Mockito, Maven (Surefire/Failsafe), Google Java Format. + +**Spec:** `docs/superpowers/specs/2026-05-20-extra-route-interface-design.md` + +**Worktree:** Work in `.claude/worktrees/extra-route-interface` on branch `feat/extra-route-interface`. Commit messages: skip commitlint via `SKIP=commitlint git commit ...`; capitalise the first character after the conventional-commit `:`. + +--- + +## File Structure + +**New files (main):** +- `src/main/java/com/retailsvc/http/BadRequestException.java` — public 4xx exception. +- `src/main/java/com/retailsvc/http/internal/ExtraRouteAdapter.java` — bridges JDK `HttpHandler` to `RequestHandler` for extras. +- `src/main/java/com/retailsvc/http/internal/ProblemDetail.java` — record serialized by the registered JSON `TypeMapper`. +- `src/main/java/com/retailsvc/http/internal/NotFoundHandler.java` — moved from `Handlers.notFoundHandler()`. + +**Modified files (main):** +- `src/main/java/com/retailsvc/http/Request.java` — adds `HttpMethod method` field/getter; updates constructors and `withPrincipals`. +- `src/main/java/com/retailsvc/http/ExceptionHandler.java` — signature becomes `Response handle(Throwable)`. +- `src/main/java/com/retailsvc/http/Handlers.java` — factories return `RequestHandler`; `defaultExceptionHandler(TypeMapper)`; `notFoundHandler` removed. +- `src/main/java/com/retailsvc/http/OpenApiServer.java` — `extraRoute(String, RequestHandler)`; wires `defaultExceptionHandler(jsonMapper)`; uses `ExtraRouteAdapter`; uses `internal/NotFoundHandler`. +- `src/main/java/com/retailsvc/http/internal/ExceptionFilter.java` — takes `ResponseRenderer`; renders the returned `Response`. +- `src/main/java/com/retailsvc/http/internal/RequestPreparationFilter.java` — passes `method` to `Request`. + +**Deleted files (main):** +- `src/main/java/com/retailsvc/http/internal/ProblemDetailRenderer.java` +- `src/main/java/com/retailsvc/http/internal/MethodLimitedHandler.java` + +**Test files (modified/new):** +- `src/test/java/com/retailsvc/http/BadRequestExceptionTest.java` (new) +- `src/test/java/com/retailsvc/http/RequestTest.java` (constructor signature updates) +- `src/test/java/com/retailsvc/http/HandlersTest.java` (rewritten to use `RequestHandler`) +- `src/test/java/com/retailsvc/http/HandlersDefaultExceptionTest.java` (rewritten to assert `Response` outputs) +- `src/test/java/com/retailsvc/http/ExtraHandlersIT.java` (signature migration, exception-flow test uses `RequestHandler`) +- `src/test/java/com/retailsvc/http/OpenApiServerBuilderTest.java` (signature migration) +- `src/test/java/com/retailsvc/http/internal/ProblemDetailRendererTest.java` — delete (renderer is gone) +- `src/test/java/com/retailsvc/http/internal/ExtraRouteAdapterTest.java` (new unit test for adapter) + +--- + +## Task 1: Add `HttpMethod method` field to `Request` + +Make method observable on `Request`. Every existing construction site moves from the 7/8-arg form to an 8/9-arg form taking `HttpMethod`. Done first because every subsequent task depends on it. + +**Files:** +- Modify: `src/main/java/com/retailsvc/http/Request.java` +- Modify: `src/main/java/com/retailsvc/http/internal/RequestPreparationFilter.java` +- Modify: `src/main/java/com/retailsvc/http/internal/SecurityFilter.java` (uses `withPrincipals`) +- Test: `src/test/java/com/retailsvc/http/RequestTest.java` + +- [ ] **Step 1.1: Write a failing test for `Request.method()`** + +Add to `RequestTest.java`: + +```java +@Test +void exposesMethod() { + Request req = + new Request( + new byte[0], + null, + null, + "op", + Map.of(), + null, + NO_HEADERS, + Map.of(), + com.retailsvc.http.spec.HttpMethod.POST); + + assertThat(req.method()).isEqualTo(com.retailsvc.http.spec.HttpMethod.POST); +} +``` + +- [ ] **Step 1.2: Run test, expect compile failure** + +Run: `mvn -q test -Dtest=RequestTest#exposesMethod` +Expected: compilation error — no 9-arg `Request` constructor, no `method()` accessor. + +- [ ] **Step 1.3: Add `method` to `Request`** + +Update `Request.java`: + +1. Add field: `private final com.retailsvc.http.spec.HttpMethod method;` +2. Replace the two existing constructors with a single 9-arg primary constructor (keep `@SuppressWarnings("java:S107")`): + +```java +@SuppressWarnings("java:S107") +public Request( + byte[] body, + Object parsed, + TypeMapper bodyMapper, + String operationId, + Map pathParameters, + String rawQuery, + UnaryOperator headerLookup, + Map principals, + com.retailsvc.http.spec.HttpMethod method) { + this.body = body; + this.parsed = parsed; + this.bodyMapper = bodyMapper; + this.operationId = operationId; + this.pathParameters = pathParameters; + this.rawQuery = rawQuery; + this.headerLookup = headerLookup; + this.principals = Map.copyOf(principals); + this.method = method; +} +``` + +3. Add a back-compat overload that supplies `null` principals/method for tests (keep the existing 7/8-arg signatures intact for test convenience; they delegate to the 9-arg form with `principals = Map.of()` and `method = null`): + +```java +public Request( + byte[] body, + Object parsed, + TypeMapper bodyMapper, + String operationId, + Map pathParameters, + String rawQuery, + UnaryOperator headerLookup) { + this(body, parsed, bodyMapper, operationId, pathParameters, rawQuery, headerLookup, Map.of(), null); +} + +@SuppressWarnings("java:S107") +public Request( + byte[] body, + Object parsed, + TypeMapper bodyMapper, + String operationId, + Map pathParameters, + String rawQuery, + UnaryOperator headerLookup, + Map principals) { + this(body, parsed, bodyMapper, operationId, pathParameters, rawQuery, headerLookup, principals, null); +} +``` + +4. Add accessor: + +```java +public com.retailsvc.http.spec.HttpMethod method() { + return method; +} +``` + +5. Thread `method` through `withPrincipals`: + +```java +public Request withPrincipals(Map principals) { + return new Request( + body, parsed, bodyMapper, operationId, pathParameters, rawQuery, headerLookup, principals, method); +} +``` + +- [ ] **Step 1.4: Update `RequestPreparationFilter` to pass method** + +In `RequestPreparationFilter.java` around line 68, replace the `new Request(...)` call so it passes `method` as the 9th argument: + +```java +Request request = + new Request( + body, + parsedBody.value(), + parsedBody.mapper(), + op.operationId(), + match.pathParameters(), + exchange.getRequestURI().getRawQuery(), + headers::getFirst, + Map.of(), + method); +``` + +- [ ] **Step 1.5: Run the new test and the full suite** + +Run: `mvn -q test` +Expected: BUILD SUCCESS, 377 tests pass (existing 376 + new `exposesMethod`). + +If `SecurityFilter` or other callers fail to compile, they are using `withPrincipals` only — that already threads `method` through and should still compile. If a test fails because `method()` returns `null` for a `Request` constructed via the 7/8-arg overload, that's expected — those tests don't exercise method. + +- [ ] **Step 1.6: Commit** + +```bash +git add src/main/java/com/retailsvc/http/Request.java \ + src/main/java/com/retailsvc/http/internal/RequestPreparationFilter.java \ + src/test/java/com/retailsvc/http/RequestTest.java +SKIP=commitlint git commit -m "feat: Expose HTTP method on Request" +``` + +--- + +## Task 2: Add `BadRequestException` + +Public exception users throw from handlers to surface 4xx errors with optional pointer/keyword. + +**Files:** +- Create: `src/main/java/com/retailsvc/http/BadRequestException.java` +- Test: `src/test/java/com/retailsvc/http/BadRequestExceptionTest.java` + +- [ ] **Step 2.1: Write failing test** + +`src/test/java/com/retailsvc/http/BadRequestExceptionTest.java`: + +```java +package com.retailsvc.http; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import org.junit.jupiter.api.Test; + +class BadRequestExceptionTest { + + @Test + void defaultsStatusTo400() { + BadRequestException e = new BadRequestException("bad input"); + + assertThat(e.status()).isEqualTo(400); + assertThat(e.getMessage()).isEqualTo("bad input"); + assertThat(e.pointer()).isEmpty(); + assertThat(e.keyword()).isEmpty(); + } + + @Test + void honorsExplicitStatus() { + BadRequestException e = new BadRequestException(422, "email taken"); + + assertThat(e.status()).isEqualTo(422); + assertThat(e.getMessage()).isEqualTo("email taken"); + } + + @Test + void carriesPointerAndKeyword() { + BadRequestException e = new BadRequestException(422, "email taken", "/email", "unique"); + + assertThat(e.pointer()).contains("/email"); + assertThat(e.keyword()).contains("unique"); + } + + @Test + void rejectsNon4xxStatus() { + assertThatThrownBy(() -> new BadRequestException(500, "boom")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("4xx"); + assertThatThrownBy(() -> new BadRequestException(399, "x")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("4xx"); + } + + @Test + void rejectsNullDetail() { + assertThatThrownBy(() -> new BadRequestException(null)) + .isInstanceOf(NullPointerException.class) + .hasMessageContaining("detail"); + } +} +``` + +- [ ] **Step 2.2: Run test, expect compile failure** + +Run: `mvn -q test -Dtest=BadRequestExceptionTest` +Expected: compilation error — `BadRequestException` does not exist. + +- [ ] **Step 2.3: Implement `BadRequestException`** + +`src/main/java/com/retailsvc/http/BadRequestException.java`: + +```java +package com.retailsvc.http; + +import java.util.Objects; +import java.util.Optional; + +/** + * Thrown by user handlers to signal a 4xx client error. The default {@link ExceptionHandler} + * renders this as an RFC 7807 {@code application/problem+json} response carrying the supplied + * status, detail, and optional JSON-pointer / validation-keyword fields. + * + *

Use for cases like {@code 422 Unprocessable Content} (payload is syntactically valid but + * violates a business rule), {@code 409 Conflict}, {@code 412 Precondition Failed}, etc. For + * 5xx errors, throw an ordinary {@link RuntimeException} and let the default handler render 500. + */ +public final class BadRequestException extends RuntimeException { + + private static final int DEFAULT_STATUS = 400; + + private final int status; + private final String pointer; + private final String keyword; + + public BadRequestException(String detail) { + this(DEFAULT_STATUS, detail, null, null); + } + + public BadRequestException(int status, String detail) { + this(status, detail, null, null); + } + + public BadRequestException(int status, String detail, String pointer, String keyword) { + super(Objects.requireNonNull(detail, "detail must not be null")); + if (status < 400 || status > 499) { + throw new IllegalArgumentException("status must be 4xx, got " + status); + } + this.status = status; + this.pointer = pointer; + this.keyword = keyword; + } + + public int status() { + return status; + } + + public Optional pointer() { + return Optional.ofNullable(pointer); + } + + public Optional keyword() { + return Optional.ofNullable(keyword); + } +} +``` + +- [ ] **Step 2.4: Run test, expect pass** + +Run: `mvn -q test -Dtest=BadRequestExceptionTest` +Expected: BUILD SUCCESS, 5 tests pass. + +- [ ] **Step 2.5: Commit** + +```bash +git add src/main/java/com/retailsvc/http/BadRequestException.java \ + src/test/java/com/retailsvc/http/BadRequestExceptionTest.java +SKIP=commitlint git commit -m "feat: Add BadRequestException for 4xx handler errors" +``` + +--- + +## Task 3: Add internal `ProblemDetail` record + +Data carrier for RFC 7807 problem+json bodies. Serialized later (Task 4) via the registered JSON `TypeMapper`. + +**Files:** +- Create: `src/main/java/com/retailsvc/http/internal/ProblemDetail.java` + +- [ ] **Step 3.1: Write `ProblemDetail`** + +`src/main/java/com/retailsvc/http/internal/ProblemDetail.java`: + +```java +package com.retailsvc.http.internal; + +import com.retailsvc.http.BadRequestException; +import com.retailsvc.http.validate.ValidationError; +import java.util.Map; + +/** + * Carrier for an RFC 7807 problem+json document. Serialized by the registered JSON + * {@code TypeMapper}; the wire shape and field-order are whatever the configured mapper + * produces — title is advisory per RFC 7807 since {@code type} is always {@code about:blank}. + */ +public record ProblemDetail( + String type, String title, int status, String detail, String pointer, String keyword) { + + private static final String DEFAULT_TYPE = "about:blank"; + + public static ProblemDetail forValidation(ValidationError e) { + return new ProblemDetail(DEFAULT_TYPE, "Bad Request", 400, e.message(), e.pointer(), e.keyword()); + } + + public static ProblemDetail forBadRequest(BadRequestException e) { + return new ProblemDetail( + DEFAULT_TYPE, + titleFor(e.status()), + e.status(), + e.getMessage(), + e.pointer().orElse(null), + e.keyword().orElse(null)); + } + + private static final Map TITLES = + Map.of( + 400, "Bad Request", + 401, "Unauthorized", + 403, "Forbidden", + 404, "Not Found", + 405, "Method Not Allowed", + 409, "Conflict", + 410, "Gone", + 412, "Precondition Failed", + 415, "Unsupported Media Type", + 422, "Unprocessable Content"); + + private static String titleFor(int status) { + return TITLES.getOrDefault(status, "Bad Request"); + } +} +``` + +- [ ] **Step 3.2: Compile-check** + +Run: `mvn -q compile` +Expected: BUILD SUCCESS. + +- [ ] **Step 3.3: Commit** + +```bash +git add src/main/java/com/retailsvc/http/internal/ProblemDetail.java +SKIP=commitlint git commit -m "feat: Add internal ProblemDetail record for problem+json bodies" +``` + +--- + +## Task 4: Migrate `ExceptionHandler` to `Throwable -> Response` + +Change the public interface, render the returned `Response` through `ResponseRenderer`, and rewrite the default handler to use `ProblemDetail` + the registered JSON `TypeMapper`. Wire the JSON mapper from `OpenApiServer.Builder.build()`. + +**Files:** +- Modify: `src/main/java/com/retailsvc/http/ExceptionHandler.java` +- Modify: `src/main/java/com/retailsvc/http/internal/ExceptionFilter.java` +- Modify: `src/main/java/com/retailsvc/http/Handlers.java` (only `defaultExceptionHandler`) +- Modify: `src/main/java/com/retailsvc/http/OpenApiServer.java` +- Modify: `src/test/java/com/retailsvc/http/HandlersDefaultExceptionTest.java` + +- [ ] **Step 4.1: Change `ExceptionHandler` signature** + +Replace `src/main/java/com/retailsvc/http/ExceptionHandler.java` with: + +```java +package com.retailsvc.http; + +/** + * Maps a {@link Throwable} thrown anywhere in the request pipeline to a {@link Response}. + * + *

Runs outside any {@code ScopedValue} bindings established by filters or interceptors — + * scopes are torn down as the exception unwinds. Context-aware error mapping (trace IDs, etc.) + * should be done in a {@link RequestInterceptor} that wraps {@code next.proceed()} in try/catch. + */ +@FunctionalInterface +public interface ExceptionHandler { + + Response handle(Throwable t); +} +``` + +- [ ] **Step 4.2: Update `ExceptionFilter` to render via `ResponseRenderer`** + +Replace `src/main/java/com/retailsvc/http/internal/ExceptionFilter.java`: + +```java +package com.retailsvc.http.internal; + +import com.retailsvc.http.ExceptionHandler; +import com.retailsvc.http.Response; +import com.sun.net.httpserver.Filter; +import com.sun.net.httpserver.HttpExchange; +import java.io.IOException; + +public final class ExceptionFilter extends Filter { + + private final ExceptionHandler handler; + private final ResponseRenderer renderer; + + public ExceptionFilter(ExceptionHandler handler, ResponseRenderer renderer) { + this.handler = handler; + this.renderer = renderer; + } + + @Override + public void doFilter(HttpExchange exchange, Chain chain) throws IOException { + try { + chain.doFilter(exchange); + } catch (RuntimeException | IOException t) { + Response response = handler.handle(t); + renderer.render(exchange, response); + } + } + + @Override + public String description() { + return "Exception filter"; + } +} +``` + +- [ ] **Step 4.3: Rewrite `Handlers.defaultExceptionHandler`** + +In `Handlers.java`: + +1. Add imports: + +```java +import com.retailsvc.http.internal.ProblemDetail; +import java.util.stream.Collectors; +``` + +Remove the now-unused `ProblemDetailRenderer` import. + +2. Replace the existing `defaultExceptionHandler()` body. The old method returned an `ExceptionHandler` that wrote to `HttpExchange`. The new one takes `TypeMapper jsonMapper` and returns a `Throwable -> Response`: + +```java +public static ExceptionHandler defaultExceptionHandler(TypeMapper jsonMapper) { + Objects.requireNonNull(jsonMapper, "jsonMapper must not be null"); + return t -> + switch (t) { + case ValidationException ve -> + Response.bytes( + HTTP_BAD_REQUEST, + jsonMapper.writeTo(ProblemDetail.forValidation(ve.error())), + "application/problem+json"); + case BadRequestException bre -> + Response.bytes( + bre.status(), + jsonMapper.writeTo(ProblemDetail.forBadRequest(bre)), + "application/problem+json"); + case NotFoundException _ -> Response.notFound(); + case MethodNotAllowedException mna -> + Response.status(HTTP_BAD_METHOD) + .withHeader( + "Allow", + mna.allowed().stream().map(Enum::name).collect(Collectors.joining(", "))); + default -> { + LOG.error("Unhandled exception in handler", t); + yield Response.status(HTTP_INTERNAL_ERROR); + } + }; +} +``` + +Imports needed in `Handlers.java`: `HTTP_BAD_REQUEST`, `HTTP_BAD_METHOD`, `HTTP_INTERNAL_ERROR` from `java.net.HttpURLConnection` (some already present). + +Delete the no-arg `defaultExceptionHandler()` overload — every caller passes a JSON mapper now (the framework wires it; users who supply their own custom `ExceptionHandler` skip this entirely). + +- [ ] **Step 4.4: Wire JSON mapper in `OpenApiServer.Builder.build()`** + +In `OpenApiServer.java`, locate the `build()` method (around line 271). Replace the section that resolves the exception handler. The current code is in the constructor (around line 74); change it so the resolved JSON mapper is available when constructing the default handler. + +Updated `build()` (relevant block): + +```java +Map resolved = resolveBodyMappers(bodyMappers); +ExceptionHandler effectiveExceptionHandler = + exceptionHandler != null + ? exceptionHandler + : Handlers.defaultExceptionHandler(resolved.get(JSON)); +HandlerConfig handlerConfig = + new HandlerConfig( + handlers, + interceptors, + decorators, + effectiveExceptionHandler, + extras, + Map.copyOf(securityValidators), + externalAuth); +return new OpenApiServer(spec, resolved, handlerConfig, port, shutdownTimeoutSeconds); +``` + +Remove the in-constructor fallback block: + +```java +ExceptionHandler exceptionHandler = handlerConfig.exceptionHandler(); +if (exceptionHandler == null) { + LOG.warn("No ExceptionHandler set, using default"); + exceptionHandler = Handlers.defaultExceptionHandler(); +} +``` + +Replace with: `ExceptionHandler exceptionHandler = handlerConfig.exceptionHandler();` (already non-null by construction). + +- [ ] **Step 4.5: Update `ExceptionFilter` construction** + +In `OpenApiServer.java`, the two `new ExceptionFilter(exceptionHandler)` calls (around lines 91 and 110) now need a `ResponseRenderer`. Construct one `ResponseRenderer` early in the constructor and reuse it: + +```java +ResponseRenderer renderer = new ResponseRenderer(bodyMappers); +// ... +ctx.getFilters().add(new ExceptionFilter(exceptionHandler, renderer)); +// ... +extraCtx.getFilters().add(new ExceptionFilter(exceptionHandler, renderer)); +``` + +The existing `new ResponseRenderer(bodyMappers)` argument to `DispatchHandler` (around line 106) becomes `renderer`. Single renderer instance shared by `ExceptionFilter` and `DispatchHandler`. + +- [ ] **Step 4.6: Rewrite `HandlersDefaultExceptionTest`** + +Replace `src/test/java/com/retailsvc/http/HandlersDefaultExceptionTest.java`: + +```java +package com.retailsvc.http; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.retailsvc.http.spec.HttpMethod; +import com.retailsvc.http.validate.ValidationError; +import java.nio.charset.StandardCharsets; +import java.util.Map; +import java.util.Set; +import org.junit.jupiter.api.Test; + +class HandlersDefaultExceptionTest { + + private static final TypeMapper JSON = + new GsonTypeMapper(); // built-in Gson mapper covers serialization + + @Test + void validationExceptionRendersProblemJson() { + Response resp = + Handlers.defaultExceptionHandler(JSON) + .handle(new ValidationException(new ValidationError("/x", "type", "expected string", null))); + + assertThat(resp.status()).isEqualTo(400); + assertThat(resp.contentType()).isEqualTo("application/problem+json"); + byte[] bytes = (byte[]) resp.body(); + String json = new String(bytes, StandardCharsets.UTF_8); + Map parsed = (Map) JSON.readFrom(bytes, "application/json"); + assertThat(parsed).containsEntry("status", 400.0).containsEntry("keyword", "type"); + assertThat(json).contains("expected string"); + } + + @Test + void badRequestExceptionRendersProblemJsonWithCustomStatus() { + Response resp = + Handlers.defaultExceptionHandler(JSON) + .handle(new BadRequestException(422, "email taken", "/email", "unique")); + + assertThat(resp.status()).isEqualTo(422); + assertThat(resp.contentType()).isEqualTo("application/problem+json"); + Map parsed = (Map) JSON.readFrom((byte[]) resp.body(), "application/json"); + assertThat(parsed) + .containsEntry("status", 422.0) + .containsEntry("title", "Unprocessable Content") + .containsEntry("detail", "email taken") + .containsEntry("pointer", "/email") + .containsEntry("keyword", "unique"); + } + + @Test + void notFoundReturns404() { + Response resp = + Handlers.defaultExceptionHandler(JSON).handle(new NotFoundException("GET /x")); + + assertThat(resp.status()).isEqualTo(404); + assertThat(resp.body()).isNull(); + } + + @Test + void methodNotAllowedReturns405WithAllowHeader() { + Response resp = + Handlers.defaultExceptionHandler(JSON) + .handle(new MethodNotAllowedException(Set.of(HttpMethod.GET, HttpMethod.POST))); + + assertThat(resp.status()).isEqualTo(405); + assertThat(resp.headers()).containsKey("Allow"); + assertThat(resp.headers().get("Allow")).contains("GET").contains("POST"); + } + + @Test + void unknownExceptionReturns500() { + Response resp = + Handlers.defaultExceptionHandler(JSON).handle(new RuntimeException("kaboom")); + + assertThat(resp.status()).isEqualTo(500); + assertThat(resp.body()).isNull(); + } +} +``` + +Note: Gson deserializes numeric JSON as `Double`, hence `400.0`. + +- [ ] **Step 4.7: Run the suite** + +Run: `mvn -q test` +Expected: BUILD SUCCESS. `HandlersDefaultExceptionTest` passes (5 tests). Other tests that called the old `Handlers.defaultExceptionHandler()` no-arg form will fail to compile — that's expected and addressed in Task 6 (`ExtraHandlersIT`) and Task 7 (`OpenApiServerBuilderTest`). If failures appear only in those files, proceed; otherwise resolve before committing. + +For now, temporarily comment out broken test usages OR use the workaround: every test that currently calls `defaultExceptionHandler()` should pass through the builder which auto-wires; explicit calls in tests can pass `new GsonTypeMapper()`. Update the obvious affected tests inline now and re-run: + +- `ExtraHandlersIT.java` lines 20, 44, 74: change `.exceptionHandler(defaultExceptionHandler())` to `.exceptionHandler(defaultExceptionHandler(new GsonTypeMapper()))`. Update the static import accordingly. + +Run again: `mvn -q test` +Expected: BUILD SUCCESS for unit tests (IT not run here). + +- [ ] **Step 4.8: Commit** + +```bash +git add src/main/java/com/retailsvc/http/ExceptionHandler.java \ + src/main/java/com/retailsvc/http/Handlers.java \ + src/main/java/com/retailsvc/http/OpenApiServer.java \ + src/main/java/com/retailsvc/http/internal/ExceptionFilter.java \ + src/test/java/com/retailsvc/http/HandlersDefaultExceptionTest.java \ + src/test/java/com/retailsvc/http/ExtraHandlersIT.java +SKIP=commitlint git commit -m "feat: ExceptionHandler returns Response; problem+json via TypeMapper" +``` + +--- + +## Task 5: Migrate `Handlers.aliveHandler/healthHandler/specHandler` to `RequestHandler` + +Inline the 405-on-non-GET/HEAD check; produce `Response` directly. Delete `MethodLimitedHandler` after no callers remain. + +**Files:** +- Modify: `src/main/java/com/retailsvc/http/Handlers.java` +- Modify: `src/main/java/com/retailsvc/http/internal/ClasspathResourceHandler.java` → becomes a plain bytes/content-type pair, not an `HttpHandler`. +- Test: `src/test/java/com/retailsvc/http/HandlersTest.java` (rewritten) +- Test: `src/test/java/com/retailsvc/http/HealthHandlerTest.java` (signature update — verify and adjust) + +- [ ] **Step 5.1: Rewrite `HandlersTest`** + +Replace `src/test/java/com/retailsvc/http/HandlersTest.java`: + +```java +package com.retailsvc.http; + +import static com.retailsvc.http.spec.HttpMethod.GET; +import static com.retailsvc.http.spec.HttpMethod.HEAD; +import static com.retailsvc.http.spec.HttpMethod.POST; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import com.retailsvc.http.spec.HttpMethod; +import java.util.Map; +import java.util.function.UnaryOperator; +import org.junit.jupiter.api.Test; + +class HandlersTest { + + private static final UnaryOperator NO_HEADERS = name -> null; + + private static Request request(HttpMethod method) { + return new Request(new byte[0], null, null, null, Map.of(), null, NO_HEADERS, Map.of(), method); + } + + @Test + void aliveHandlerReturns204OnGet() { + Response resp = Handlers.aliveHandler().handle(request(GET)); + + assertThat(resp.status()).isEqualTo(204); + assertThat(resp.body()).isNull(); + } + + @Test + void aliveHandlerReturns204OnHead() { + Response resp = Handlers.aliveHandler().handle(request(HEAD)); + + assertThat(resp.status()).isEqualTo(204); + } + + @Test + void aliveHandlerReturns405OnPost() { + Response resp = Handlers.aliveHandler().handle(request(POST)); + + assertThat(resp.status()).isEqualTo(405); + assertThat(resp.headers()).containsEntry("Allow", "GET, HEAD"); + } + + @Test + void specHandlerServesYamlBytesWithInferredContentType() { + Response resp = Handlers.specHandler("/openapi.yaml").handle(request(GET)); + + assertThat(resp.status()).isEqualTo(200); + assertThat(resp.contentType()).isEqualTo("application/yaml"); + assertThat(resp.body()).isInstanceOf(byte[].class); + assertThat((byte[]) resp.body()).isNotEmpty(); + } + + @Test + void specHandlerInfersJsonContentType() { + Response resp = Handlers.specHandler("/openapi.json").handle(request(GET)); + + assertThat(resp.contentType()).isEqualTo("application/json"); + } + + @Test + void specHandlerThrowsAtConstructionForMissingResource() { + assertThatThrownBy(() -> Handlers.specHandler("/does-not-exist.yaml")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("/does-not-exist.yaml"); + } + + @Test + void specHandlerReturns405OnPost() { + Response resp = Handlers.specHandler("/openapi.yaml").handle(request(POST)); + + assertThat(resp.status()).isEqualTo(405); + assertThat(resp.headers()).containsEntry("Allow", "GET, HEAD"); + } + + @Test + void specHandlerHeadReturnsContentLengthWithoutBody() { + Response resp = Handlers.specHandler("/openapi.yaml").handle(request(HEAD)); + + assertThat(resp.status()).isEqualTo(200); + assertThat(resp.body()).isNull(); + assertThat(resp.headers()).containsKey("Content-Length"); + assertThat(Integer.parseInt(resp.headers().get("Content-Length"))).isGreaterThan(0); + } +} +``` + +- [ ] **Step 5.2: Run, expect compile failure** + +Run: `mvn -q test -Dtest=HandlersTest` +Expected: compilation error — return types don't match. + +- [ ] **Step 5.3: Migrate `Handlers.aliveHandler`** + +In `Handlers.java`, replace the existing `aliveHandler()`: + +```java +/** Returns 204 No Content on GET/HEAD; 405 with {@code Allow: GET, HEAD} otherwise. */ +public static RequestHandler aliveHandler() { + return req -> + switch (req.method()) { + case GET, HEAD -> Response.empty(); + default -> Response.status(HTTP_BAD_METHOD).withHeader("Allow", "GET, HEAD"); + }; +} +``` + +Add static import: `import static com.retailsvc.http.spec.HttpMethod.GET;` and `HEAD`. Or qualify inline — the agent picks whichever fits the existing import style (file uses static imports for HTTP constants; follow that). + +- [ ] **Step 5.4: Migrate `Handlers.specHandler`** + +`ClasspathResourceHandler` currently implements `HttpHandler`. Refactor it to a plain immutable holder of `(bytes, contentType)`. Replace `src/main/java/com/retailsvc/http/internal/ClasspathResourceHandler.java`: + +```java +package com.retailsvc.http.internal; + +import java.io.IOException; +import java.io.InputStream; +import java.util.Locale; + +/** + * Eagerly-loaded bytes for a classpath resource. Content-Type is inferred from the file + * extension. Throws {@link IllegalArgumentException} if the resource is missing. + */ +public final class ClasspathResourceHandler { + + private final byte[] bytes; + private final String contentType; + + public ClasspathResourceHandler(String classpathResource) { + try (InputStream in = ClasspathResourceHandler.class.getResourceAsStream(classpathResource)) { + if (in == null) { + throw new IllegalArgumentException("classpath resource not found: " + classpathResource); + } + this.bytes = in.readAllBytes(); + } catch (IOException io) { + throw new IllegalArgumentException( + "failed reading classpath resource: " + classpathResource, io); + } + this.contentType = contentTypeFor(classpathResource); + } + + public byte[] bytes() { + return bytes; + } + + public String contentType() { + return contentType; + } + + private static String contentTypeFor(String path) { + String lower = path.toLowerCase(Locale.ROOT); + if (lower.endsWith(".json")) { + return "application/json"; + } + if (lower.endsWith(".yaml") || lower.endsWith(".yml")) { + return "application/yaml"; + } + if (lower.endsWith(".txt")) { + return "text/plain; charset=utf-8"; + } + return "application/octet-stream"; + } +} +``` + +Replace `Handlers.specHandler`: + +```java +public static RequestHandler specHandler(String classpathResource) { + ClasspathResourceHandler resource = new ClasspathResourceHandler(classpathResource); + byte[] bytes = resource.bytes(); + String contentType = resource.contentType(); + return req -> + switch (req.method()) { + case GET -> Response.bytes(HTTP_OK, bytes, contentType); + case HEAD -> + Response.status(HTTP_OK) + .withContentType(contentType) + .withHeader("Content-Length", String.valueOf(bytes.length)); + default -> Response.status(HTTP_BAD_METHOD).withHeader("Allow", "GET, HEAD"); + }; +} +``` + +- [ ] **Step 5.5: Migrate `Handlers.healthHandler`** + +Replace `Handlers.healthHandler(TypeMapper, Supplier)`: + +```java +public static RequestHandler healthHandler(TypeMapper jsonMapper, Supplier probe) { + Objects.requireNonNull(jsonMapper, "jsonMapper"); + Objects.requireNonNull(probe, "probe"); + return req -> { + if (req.method() != GET && req.method() != HEAD) { + return Response.status(HTTP_BAD_METHOD).withHeader("Allow", "GET, HEAD"); + } + HealthOutcome outcome; + try { + outcome = Objects.requireNonNull(probe.get(), "Health probe returned null"); + } catch (RuntimeException e) { + LOG.warn("Health probe failed", e); + outcome = new HealthOutcome(false, List.of()); + } + byte[] body = jsonMapper.writeTo(toWireShape(outcome)); + int status = outcome.up() ? HTTP_OK : HTTP_UNAVAILABLE; + return Response.bytes(status, body, "application/json"); + }; +} +``` + +`toWireShape`, `label`, `HealthBody`, `DependencyBody` stay as-is. + +- [ ] **Step 5.6: Update `HealthHandlerTest`** + +Note: `Handlers.notFoundHandler()` is left in place this task and removed in Task 6 alongside introducing `internal/NotFoundHandler` (single atomic switch so the catch-all `/` context never references a missing symbol). + + + +Read `src/test/java/com/retailsvc/http/HealthHandlerTest.java` and adapt: every `HttpExchange` mock is replaced with `Request` construction (see `HandlersTest` pattern in Step 5.1); assertions read `Response.status()`, `Response.contentType()`, `Response.body()` instead of `verify(ex).sendResponseHeaders(...)`. + +This file is not enumerated here line-by-line — apply the same mechanical translation as `HandlersTest`. + +- [ ] **Step 5.7: Run all unit tests** + +Run: `mvn -q test` +Expected: BUILD SUCCESS. If any test file other than `OpenApiServerBuilderTest`, `ExtraHandlersIT` still fails, fix it inline before committing — those two are addressed next. + +- [ ] **Step 5.8: Commit** + +```bash +git add src/main/java/com/retailsvc/http/Handlers.java \ + src/main/java/com/retailsvc/http/internal/ClasspathResourceHandler.java \ + src/test/java/com/retailsvc/http/HandlersTest.java \ + src/test/java/com/retailsvc/http/HealthHandlerTest.java +SKIP=commitlint git commit -m "feat: Migrate Handlers factories to RequestHandler" +``` + +--- + +## Task 6: `extraRoute(String, RequestHandler)` + `ExtraRouteAdapter` + +Switch the builder to the new signature, introduce the internal adapter, delete `MethodLimitedHandler`, and migrate `Handlers.notFoundHandler` to `internal/NotFoundHandler`. + +**Files:** +- Create: `src/main/java/com/retailsvc/http/internal/ExtraRouteAdapter.java` +- Create: `src/main/java/com/retailsvc/http/internal/NotFoundHandler.java` +- Modify: `src/main/java/com/retailsvc/http/OpenApiServer.java` +- Delete: `src/main/java/com/retailsvc/http/internal/MethodLimitedHandler.java` +- Test: `src/test/java/com/retailsvc/http/internal/ExtraRouteAdapterTest.java` (new) +- Modify: `src/test/java/com/retailsvc/http/ExtraHandlersIT.java` +- Modify: `src/test/java/com/retailsvc/http/OpenApiServerBuilderTest.java` + +- [ ] **Step 6.1: Write failing unit test for `ExtraRouteAdapter`** + +`src/test/java/com/retailsvc/http/internal/ExtraRouteAdapterTest.java`: + +```java +package com.retailsvc.http.internal; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import com.retailsvc.http.GsonTypeMapper; +import com.retailsvc.http.Request; +import com.retailsvc.http.RequestHandler; +import com.retailsvc.http.Response; +import com.retailsvc.http.TypeMapper; +import com.sun.net.httpserver.Headers; +import com.sun.net.httpserver.HttpExchange; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.net.URI; +import java.util.Map; +import java.util.concurrent.atomic.AtomicReference; +import org.junit.jupiter.api.Test; + +class ExtraRouteAdapterTest { + + @Test + void buildsRequestWithMethodQueryHeadersAndBodyBytesAndNullOperationId() throws Exception { + AtomicReference captured = new AtomicReference<>(); + RequestHandler handler = + req -> { + captured.set(req); + return Response.empty(); + }; + + Map mappers = Map.of("application/json", new GsonTypeMapper()); + ResponseRenderer renderer = new ResponseRenderer(mappers); + ExtraRouteAdapter adapter = new ExtraRouteAdapter(handler, renderer); + + HttpExchange ex = mock(HttpExchange.class); + Headers reqHeaders = new Headers(); + reqHeaders.add("X-Trace", "abc"); + when(ex.getRequestMethod()).thenReturn("POST"); + when(ex.getRequestURI()).thenReturn(new URI("/alive?x=1")); + when(ex.getRequestHeaders()).thenReturn(reqHeaders); + when(ex.getRequestBody()).thenReturn(new ByteArrayInputStream("hi".getBytes())); + when(ex.getResponseHeaders()).thenReturn(new Headers()); + when(ex.getResponseBody()).thenReturn(new ByteArrayOutputStream()); + + adapter.handle(ex); + + Request r = captured.get(); + assertThat(r.operationId()).isNull(); + assertThat(r.pathParams()).isEmpty(); + assertThat(r.principals()).isEmpty(); + assertThat(r.method()).isEqualTo(com.retailsvc.http.spec.HttpMethod.POST); + assertThat(r.rawQuery()).isEqualTo("x=1"); + assertThat(r.header("X-Trace")).contains("abc"); + assertThat(r.bytes()).containsExactly('h', 'i'); + } +} +``` + +- [ ] **Step 6.2: Run, expect compile failure** + +Run: `mvn -q test -Dtest=ExtraRouteAdapterTest` +Expected: `ExtraRouteAdapter` not found. + +- [ ] **Step 6.3: Implement `ExtraRouteAdapter`** + +`src/main/java/com/retailsvc/http/internal/ExtraRouteAdapter.java`: + +```java +package com.retailsvc.http.internal; + +import com.retailsvc.http.Request; +import com.retailsvc.http.RequestHandler; +import com.retailsvc.http.Response; +import com.retailsvc.http.spec.HttpMethod; +import com.sun.net.httpserver.HttpExchange; +import com.sun.net.httpserver.HttpHandler; +import java.io.IOException; +import java.util.Map; + +/** + * Bridges an extra-route {@link RequestHandler} to the underlying JDK {@link HttpHandler}. + * + *

Builds a {@link Request} with {@code operationId=null}, empty path-params, empty principals, + * raw body bytes, raw query, and the parsed HTTP method, then renders the returned {@link + * Response} through the shared {@link ResponseRenderer}. OpenAPI validation, body parsing, and + * security are intentionally bypassed — extras are by definition outside the spec. + */ +public final class ExtraRouteAdapter implements HttpHandler { + + private final RequestHandler handler; + private final ResponseRenderer renderer; + + public ExtraRouteAdapter(RequestHandler handler, ResponseRenderer renderer) { + this.handler = handler; + this.renderer = renderer; + } + + @Override + public void handle(HttpExchange exchange) throws IOException { + byte[] body = exchange.getRequestBody().readAllBytes(); + HttpMethod method = HttpMethod.parse(exchange.getRequestMethod()); + var headers = exchange.getRequestHeaders(); + Request request = + new Request( + body, + null, + null, + null, + Map.of(), + exchange.getRequestURI().getRawQuery(), + headers::getFirst, + Map.of(), + method); + Response response = handler.handle(request); + renderer.render(exchange, response); + } +} +``` + +- [ ] **Step 6.4: Run adapter test, expect pass** + +Run: `mvn -q test -Dtest=ExtraRouteAdapterTest` +Expected: BUILD SUCCESS. + +- [ ] **Step 6.5: Create `internal/NotFoundHandler`** + +`src/main/java/com/retailsvc/http/internal/NotFoundHandler.java`: + +```java +package com.retailsvc.http.internal; + +import static java.net.HttpURLConnection.HTTP_NOT_FOUND; + +import com.sun.net.httpserver.HttpExchange; +import com.sun.net.httpserver.HttpHandler; +import java.io.IOException; + +/** Returns 404 with no body. Used for the framework's catch-all {@code /} context. */ +public final class NotFoundHandler implements HttpHandler { + + @Override + public void handle(HttpExchange exchange) throws IOException { + try (exchange) { + exchange.sendResponseHeaders(HTTP_NOT_FOUND, -1); + } + } +} +``` + +- [ ] **Step 6.6: Switch `OpenApiServer.Builder.extraRoute` to `RequestHandler`** + +In `OpenApiServer.java`: + +1. Change field declaration: + +```java +private final LinkedHashMap extras = new LinkedHashMap<>(); +``` + +2. Replace `extraRoute(String, HttpHandler)` with: + +```java +public Builder extraRoute(String path, RequestHandler handler) { + requireNonNull(path, "path must not be null"); + requireNonNull(handler, "handler must not be null"); + if (extras.containsKey(path)) { + throw new IllegalStateException("duplicate extra route path: " + path); + } + extras.put(path, handler); + return this; +} +``` + +3. Update `HandlerConfig`: + +```java +record HandlerConfig( + Map handlers, + List interceptors, + List decorators, + ExceptionHandler exceptionHandler, + Map extras, + Map securityValidators, + boolean externalAuth) {} +``` + +4. Update the extras-wiring loop in the constructor to use `ExtraRouteAdapter`: + +```java +for (Map.Entry e : handlerConfig.extras().entrySet()) { + HttpContext extraCtx = httpServer.createContext(e.getKey()); + extraCtx.getFilters().add(new ExceptionFilter(exceptionHandler, renderer)); + extraCtx.setHandler(new ExtraRouteAdapter(e.getValue(), renderer)); +} +``` + +5. Replace the catch-all: + +```java +httpServer.createContext("/", new com.retailsvc.http.internal.NotFoundHandler()); +``` + +6. Delete `Handlers.notFoundHandler()` (now unused). + +7. Remove the `import com.sun.net.httpserver.HttpHandler;` from `OpenApiServer.java` if no longer needed. + +- [ ] **Step 6.7: Update `ExtraHandlersIT`** + +`extraHandlerExceptionFlowsThroughExceptionHandler` currently constructs a `com.sun.net.httpserver.HttpHandler`. Replace with `RequestHandler`: + +```java +@Test +void extraHandlerExceptionFlowsThroughExceptionHandler() throws Exception { + RequestHandler boom = + req -> { + throw new RuntimeException("boom"); + }; + + try (var s = + newBuilder() + .spec(spec) + .handlers(Map.of()) + .port(0) + .extraRoute("/boom", boom) + .build(); + var client = httpClient()) { + + var req = + HttpRequest.newBuilder() + .uri(URI.create("http://localhost:" + s.listenPort() + "/boom")) + .GET() + .build(); + var resp = client.send(req, BodyHandlers.ofString()); + + assertThat(resp.statusCode()).isEqualTo(500); + } +} +``` + +Note the removed `.exceptionHandler(defaultExceptionHandler(...))` line — builder auto-wires it now. Apply the same removal to the other two tests in the file (they no longer need to set it explicitly). + +- [ ] **Step 6.8: Update `OpenApiServerBuilderTest`** + +Read the test, find `.extraRoute("/alive", duplicate)`. `duplicate` is currently an `HttpHandler`; change its declared/inferred type to `RequestHandler`: + +```java +RequestHandler duplicate = req -> Response.empty(); +``` + +And the `.extraRoute("/api", Handlers.aliveHandler())` line still works since `Handlers.aliveHandler()` now returns `RequestHandler`. + +- [ ] **Step 6.9: Delete `MethodLimitedHandler`** + +```bash +git rm src/main/java/com/retailsvc/http/internal/MethodLimitedHandler.java +``` + +- [ ] **Step 6.10: Run full suite including integration tests** + +Run: `mvn -q verify` +Expected: BUILD SUCCESS. + +- [ ] **Step 6.11: Commit** + +```bash +git add -A +SKIP=commitlint git commit -m "feat: ExtraRoute accepts RequestHandler via ExtraRouteAdapter" +``` + +--- + +## Task 7: Delete `ProblemDetailRenderer` and its test + +The renderer has no remaining callers after Task 4. + +**Files:** +- Delete: `src/main/java/com/retailsvc/http/internal/ProblemDetailRenderer.java` +- Delete: `src/test/java/com/retailsvc/http/internal/ProblemDetailRendererTest.java` (if present) + +- [ ] **Step 7.1: Confirm no callers remain** + +Run: `grep -rn "ProblemDetailRenderer" src/main/java/ src/test/java/` +Expected: only the renderer file itself (and possibly its own test). + +If anything else matches, that call site needs updating to use `ProblemDetail` + the registered `TypeMapper`. + +- [ ] **Step 7.2: Delete the files** + +```bash +git rm src/main/java/com/retailsvc/http/internal/ProblemDetailRenderer.java +[ -f src/test/java/com/retailsvc/http/internal/ProblemDetailRendererTest.java ] && \ + git rm src/test/java/com/retailsvc/http/internal/ProblemDetailRendererTest.java +``` + +- [ ] **Step 7.3: Run full suite** + +Run: `mvn -q verify` +Expected: BUILD SUCCESS. + +- [ ] **Step 7.4: Commit** + +```bash +SKIP=commitlint git commit -m "refactor: Delete hand-rolled ProblemDetailRenderer" +``` + +--- + +## Task 8: Verify public API is free of `com.sun.net.httpserver` + +Final guard that the goal is met. + +- [ ] **Step 8.1: Grep** + +Run: + +```bash +grep -rn "com\.sun\.net\.httpserver" src/main/java/com/retailsvc/http/ \ + | grep -v "/internal/" +``` + +Expected: no output. + +If any line is reported, fix that file (move the dependency behind an `internal/` type, or remove it). + +- [ ] **Step 8.2: Run full verify + sonar (if MCP is up against `master`)** + +Run: `mvn -q verify` +Expected: BUILD SUCCESS, all tests pass, coverage report generated. + +SonarLint MCP is blind to worktrees per project memory — rely on the CI scan for the branch. Locally, check `mvn` output for warnings. + +- [ ] **Step 8.3: Final commit (if any cleanup was needed)** + +```bash +git status +# If clean, skip. Otherwise: +git add -A +SKIP=commitlint git commit -m "chore: Final cleanup of HttpServer leaks" +``` + +- [ ] **Step 8.4: Push and hand off to the user for PR creation** + +```bash +git push -u origin feat/extra-route-interface +``` + +Per the project's `reference_gh_token.md` memory, the `gh` CLI cannot create PRs in this repo — the user opens the PR manually after the push. + +--- + +## Spec coverage check + +- §1 `Request.method()` → Task 1 ✓ +- §2 `extraRoute(String, RequestHandler)` → Task 6 ✓ +- §3 `ExtraRouteAdapter` → Task 6 ✓ +- §4 `ExceptionHandler` returns `Response` → Task 4 ✓ +- §4a `BadRequestException` → Task 2 ✓ +- §4b `ProblemDetail` record → Task 3 ✓; renderer deletion → Task 7 ✓ +- §5 `Handlers.*` factories return `RequestHandler` with inline 405 → Task 5 ✓ +- §6 zero `com.sun.net.httpserver` in non-internal main → Task 8 ✓ +- Non-goal: `ScopedValue` access from `ExceptionHandler` is documented in the new javadoc on `ExceptionHandler` (Step 4.1) ✓ From eebc70be186fbc532e10544e6c7b9425cc750261 Mon Sep 17 00:00:00 2001 From: Thomas Cederholm Date: Wed, 20 May 2026 11:55:37 +0200 Subject: [PATCH 04/11] feat: Expose HTTP method on Request --- src/main/java/com/retailsvc/http/Request.java | 84 +++++++++++++++++-- .../internal/RequestPreparationFilter.java | 4 +- .../java/com/retailsvc/http/RequestTest.java | 11 +++ 3 files changed, 92 insertions(+), 7 deletions(-) diff --git a/src/main/java/com/retailsvc/http/Request.java b/src/main/java/com/retailsvc/http/Request.java index bb6a9b3..20b3c78 100644 --- a/src/main/java/com/retailsvc/http/Request.java +++ b/src/main/java/com/retailsvc/http/Request.java @@ -1,5 +1,6 @@ package com.retailsvc.http; +import com.retailsvc.http.spec.HttpMethod; import java.net.URLDecoder; import java.nio.charset.StandardCharsets; import java.util.LinkedHashMap; @@ -9,8 +10,8 @@ import java.util.function.UnaryOperator; /** - * Read-only per-request handle passed to {@link RequestHandler}. Carries the parsed body, path - * parameters, query parameters, headers, and operation ID. + * Read-only per-request handle passed to {@link RequestHandler}. Carries the HTTP method, parsed + * body, path parameters, query parameters, headers, and operation ID. * *

{@code Request} is transport-neutral: it holds the body bytes, the raw query string, the path * parameter map, and a header lookup function. The transport adapter (today the built-in JDK {@code @@ -28,6 +29,7 @@ public final class Request { private final String operationId; private final Map pathParameters; private final String rawQuery; + private final HttpMethod method; private final UnaryOperator headerLookup; private final Map principals; private Map queryParamCache; @@ -36,6 +38,8 @@ public final class Request { * Builds a {@code Request} from transport-neutral primitives. Adapters call this; handlers * receive the constructed instance. * + *

{@code method} is {@code null}; use the 9-arg constructor to supply it. + * * @param body raw request body bytes; never {@code null}, may be empty * @param parsed loose structural view of the body (Map / List / boxed primitive), or {@code null} * @param bodyMapper {@link TypeMapper} that produced {@code parsed}, used for typed conversion; @@ -53,12 +57,23 @@ public Request( Map pathParameters, String rawQuery, UnaryOperator headerLookup) { - this(body, parsed, bodyMapper, operationId, pathParameters, rawQuery, headerLookup, Map.of()); + this( + body, + parsed, + bodyMapper, + operationId, + pathParameters, + rawQuery, + headerLookup, + Map.of(), + null); } /** * Builds a {@code Request} from transport-neutral primitives with an explicit principals map. * + *

{@code method} is {@code null}; use the 9-arg constructor to supply it. + * * @param body raw request body bytes; never {@code null}, may be empty * @param parsed loose structural view of the body (Map / List / boxed primitive), or {@code null} * @param bodyMapper {@link TypeMapper} that produced {@code parsed}, used for typed conversion; @@ -69,9 +84,47 @@ public Request( * @param headerLookup first-value, case-insensitive header lookup; returns {@code null} if absent * @param principals principals stashed by the security filter, keyed by scheme name */ + @SuppressWarnings("java:S107") + public Request( + byte[] body, + Object parsed, + TypeMapper bodyMapper, + String operationId, + Map pathParameters, + String rawQuery, + UnaryOperator headerLookup, + Map principals) { + this( + body, + parsed, + bodyMapper, + operationId, + pathParameters, + rawQuery, + headerLookup, + principals, + null); + } + + /** + * Builds a {@code Request} from transport-neutral primitives with explicit principals and method. + * + * @param body raw request body bytes; never {@code null}, may be empty + * @param parsed loose structural view of the body (Map / List / boxed primitive), or {@code null} + * @param bodyMapper {@link TypeMapper} that produced {@code parsed}, used for typed conversion; + * may be {@code null} if there is no body + * @param operationId the OpenAPI {@code operationId} the request was routed to + * @param pathParameters path variables extracted by the router + * @param rawQuery raw (percent-encoded) query string, or {@code null} if absent + * @param headerLookup first-value, case-insensitive header lookup; returns {@code null} if absent + * @param principals principals stashed by the security filter, keyed by scheme name + * @param method the HTTP method of the request. Never {@code null} when constructed through the + * normal request pipeline. {@code null} only when constructed via the legacy 7- or 8-argument + * constructors (kept for backward compatibility). + */ // Request is transport-neutral and assembled from primitives at the adapter boundary; collapsing // these into a holder type would just move the parameter count one level out without simplifying - // the call site, so the 8-arg constructor is preferred over the rule's 7-param limit. + // the call site, so the 9-arg constructor is preferred over the rule's 7-param limit. @SuppressWarnings("java:S107") public Request( byte[] body, @@ -81,13 +134,15 @@ public Request( Map pathParameters, String rawQuery, UnaryOperator headerLookup, - Map principals) { + Map principals, + HttpMethod method) { this.body = body; this.parsed = parsed; this.bodyMapper = bodyMapper; this.operationId = operationId; this.pathParameters = pathParameters; this.rawQuery = rawQuery; + this.method = method; this.headerLookup = headerLookup; this.principals = Map.copyOf(principals); } @@ -207,6 +262,15 @@ public Optional principal(String schemeName) { return Optional.ofNullable(principals.get(schemeName)); } + /** + * HTTP method of the request. Never {@code null} for requests routed through the standard + * pipeline; {@code null} only when the {@code Request} was constructed via a legacy constructor + * without a method. + */ + public HttpMethod method() { + return method; + } + /** * Returns a new {@code Request} identical to this one except with the supplied principals. Used * by {@code SecurityFilter} on success; the returned instance carries the principals through to @@ -214,7 +278,15 @@ public Optional principal(String schemeName) { */ public Request withPrincipals(Map principals) { return new Request( - body, parsed, bodyMapper, operationId, pathParameters, rawQuery, headerLookup, principals); + body, + parsed, + bodyMapper, + operationId, + pathParameters, + rawQuery, + headerLookup, + principals, + method); } private static Map parseQuery(String query) { diff --git a/src/main/java/com/retailsvc/http/internal/RequestPreparationFilter.java b/src/main/java/com/retailsvc/http/internal/RequestPreparationFilter.java index 7d528a6..c6942e0 100644 --- a/src/main/java/com/retailsvc/http/internal/RequestPreparationFilter.java +++ b/src/main/java/com/retailsvc/http/internal/RequestPreparationFilter.java @@ -73,7 +73,9 @@ public void doFilter(HttpExchange exchange, Chain chain) throws IOException { op.operationId(), match.pathParameters(), exchange.getRequestURI().getRawQuery(), - headers::getFirst); + headers::getFirst, + Map.of(), + method); try { ScopedValue.where(DispatchHandler.CURRENT, request) diff --git a/src/test/java/com/retailsvc/http/RequestTest.java b/src/test/java/com/retailsvc/http/RequestTest.java index 1cb5d74..7b796b4 100644 --- a/src/test/java/com/retailsvc/http/RequestTest.java +++ b/src/test/java/com/retailsvc/http/RequestTest.java @@ -5,6 +5,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.retailsvc.http.internal.DispatchHandler; +import com.retailsvc.http.spec.HttpMethod; import java.nio.charset.StandardCharsets; import java.util.HashMap; import java.util.Map; @@ -231,6 +232,16 @@ void withPrincipalsDoesNotShareUnderlyingMap() { assertThat(copy.principal("a")).contains("b"); } + @Test + void exposesMethod() { + Request req = + new Request( + new byte[0], null, null, "op", Map.of(), null, NO_HEADERS, Map.of(), HttpMethod.POST); + + assertThat(req.method()).isEqualTo(HttpMethod.POST); + assertThat(req.withPrincipals(Map.of("k", "v")).method()).isEqualTo(HttpMethod.POST); + } + @Test void headerReturnsOptionalAndBlankIsAbsent() { Request req = From 202dc0672f6e7b9c91aea1e4cdef891ee9e0e06c Mon Sep 17 00:00:00 2001 From: Thomas Cederholm Date: Wed, 20 May 2026 12:43:18 +0200 Subject: [PATCH 05/11] feat: Add BadRequestException for 4xx handler errors --- .../retailsvc/http/BadRequestException.java | 52 +++++++++++++++++++ .../http/BadRequestExceptionTest.java | 52 +++++++++++++++++++ 2 files changed, 104 insertions(+) create mode 100644 src/main/java/com/retailsvc/http/BadRequestException.java create mode 100644 src/test/java/com/retailsvc/http/BadRequestExceptionTest.java diff --git a/src/main/java/com/retailsvc/http/BadRequestException.java b/src/main/java/com/retailsvc/http/BadRequestException.java new file mode 100644 index 0000000..4abaa69 --- /dev/null +++ b/src/main/java/com/retailsvc/http/BadRequestException.java @@ -0,0 +1,52 @@ +package com.retailsvc.http; + +import java.util.Objects; +import java.util.Optional; + +/** + * Thrown by user handlers to signal a 4xx client error. The default {@link ExceptionHandler} + * renders this as an RFC 7807 {@code application/problem+json} response carrying the supplied + * status, detail, and optional JSON-pointer / validation-keyword fields. + * + *

Use for cases like {@code 422 Unprocessable Content} (payload is syntactically valid but + * violates a business rule), {@code 409 Conflict}, {@code 412 Precondition Failed}, etc. For 5xx + * errors, throw an ordinary {@link RuntimeException} and let the default handler render 500. + */ +public final class BadRequestException extends RuntimeException { + + private static final int DEFAULT_STATUS = 400; + + private final int status; + private final String pointer; + private final String keyword; + + public BadRequestException(String detail) { + this(DEFAULT_STATUS, detail, null, null); + } + + public BadRequestException(int status, String detail) { + this(status, detail, null, null); + } + + public BadRequestException(int status, String detail, String pointer, String keyword) { + super(Objects.requireNonNull(detail, "detail must not be null")); + if (status < 400 || status > 499) { + throw new IllegalArgumentException("status must be 4xx, got " + status); + } + this.status = status; + this.pointer = pointer; + this.keyword = keyword; + } + + public int status() { + return status; + } + + public Optional pointer() { + return Optional.ofNullable(pointer); + } + + public Optional keyword() { + return Optional.ofNullable(keyword); + } +} diff --git a/src/test/java/com/retailsvc/http/BadRequestExceptionTest.java b/src/test/java/com/retailsvc/http/BadRequestExceptionTest.java new file mode 100644 index 0000000..395d474 --- /dev/null +++ b/src/test/java/com/retailsvc/http/BadRequestExceptionTest.java @@ -0,0 +1,52 @@ +package com.retailsvc.http; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import org.junit.jupiter.api.Test; + +class BadRequestExceptionTest { + + @Test + void defaultsStatusTo400() { + BadRequestException e = new BadRequestException("bad input"); + + assertThat(e.status()).isEqualTo(400); + assertThat(e.getMessage()).isEqualTo("bad input"); + assertThat(e.pointer()).isEmpty(); + assertThat(e.keyword()).isEmpty(); + } + + @Test + void honorsExplicitStatus() { + BadRequestException e = new BadRequestException(422, "email taken"); + + assertThat(e.status()).isEqualTo(422); + assertThat(e.getMessage()).isEqualTo("email taken"); + } + + @Test + void carriesPointerAndKeyword() { + BadRequestException e = new BadRequestException(422, "email taken", "/email", "unique"); + + assertThat(e.pointer()).contains("/email"); + assertThat(e.keyword()).contains("unique"); + } + + @Test + void rejectsNon4xxStatus() { + assertThatThrownBy(() -> new BadRequestException(500, "boom")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("4xx"); + assertThatThrownBy(() -> new BadRequestException(399, "x")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("4xx"); + } + + @Test + void rejectsNullDetail() { + assertThatThrownBy(() -> new BadRequestException(null)) + .isInstanceOf(NullPointerException.class) + .hasMessageContaining("detail"); + } +} From e32764090d398d9a641f21272f17af38cdb776ef Mon Sep 17 00:00:00 2001 From: Thomas Cederholm Date: Wed, 20 May 2026 12:48:34 +0200 Subject: [PATCH 06/11] feat: Add internal ProblemDetail record for problem+json bodies --- .../http/internal/ProblemDetail.java | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 src/main/java/com/retailsvc/http/internal/ProblemDetail.java diff --git a/src/main/java/com/retailsvc/http/internal/ProblemDetail.java b/src/main/java/com/retailsvc/http/internal/ProblemDetail.java new file mode 100644 index 0000000..b34135e --- /dev/null +++ b/src/main/java/com/retailsvc/http/internal/ProblemDetail.java @@ -0,0 +1,48 @@ +package com.retailsvc.http.internal; + +import com.retailsvc.http.BadRequestException; +import com.retailsvc.http.validate.ValidationError; +import java.util.Map; + +/** + * Carrier for an RFC 7807 problem+json document. Serialized by the registered JSON {@code + * TypeMapper}; the wire shape and field-order are whatever the configured mapper produces — title + * is advisory per RFC 7807 since {@code type} is always {@code about:blank}. + */ +public record ProblemDetail( + String type, String title, int status, String detail, String pointer, String keyword) { + + private static final String DEFAULT_TYPE = "about:blank"; + + public static ProblemDetail forValidation(ValidationError e) { + return new ProblemDetail( + DEFAULT_TYPE, "Bad Request", 400, e.message(), e.pointer(), e.keyword()); + } + + public static ProblemDetail forBadRequest(BadRequestException e) { + return new ProblemDetail( + DEFAULT_TYPE, + titleFor(e.status()), + e.status(), + e.getMessage(), + e.pointer().orElse(null), + e.keyword().orElse(null)); + } + + private static final Map TITLES = + Map.of( + 400, "Bad Request", + 401, "Unauthorized", + 403, "Forbidden", + 404, "Not Found", + 405, "Method Not Allowed", + 409, "Conflict", + 410, "Gone", + 412, "Precondition Failed", + 415, "Unsupported Media Type", + 422, "Unprocessable Content"); + + private static String titleFor(int status) { + return TITLES.getOrDefault(status, "Bad Request"); + } +} From 8c9e1400408b007bc3981a55611997f6e930c086 Mon Sep 17 00:00:00 2001 From: Thomas Cederholm Date: Wed, 20 May 2026 12:56:14 +0200 Subject: [PATCH 07/11] feat: ExceptionHandler returns Response; problem+json via TypeMapper --- .../com/retailsvc/http/ExceptionHandler.java | 12 +-- .../java/com/retailsvc/http/Handlers.java | 46 +++++----- .../com/retailsvc/http/OpenApiServer.java | 18 ++-- .../http/internal/ExceptionFilter.java | 9 +- .../com/retailsvc/http/ExtraHandlersIT.java | 11 +-- .../http/HandlersDefaultExceptionTest.java | 92 ++++++++++++------- .../http/internal/ExceptionFilterTest.java | 11 ++- .../retailsvc/http/start/ServerLauncher.java | 5 - 8 files changed, 112 insertions(+), 92 deletions(-) diff --git a/src/main/java/com/retailsvc/http/ExceptionHandler.java b/src/main/java/com/retailsvc/http/ExceptionHandler.java index 9a126e4..02f42dc 100644 --- a/src/main/java/com/retailsvc/http/ExceptionHandler.java +++ b/src/main/java/com/retailsvc/http/ExceptionHandler.java @@ -1,16 +1,14 @@ package com.retailsvc.http; -import com.sun.net.httpserver.HttpExchange; -import com.sun.net.httpserver.HttpHandler; -import java.io.IOException; - /** - * Handle exceptions thrown from a {@link HttpHandler}. + * Maps a {@link Throwable} thrown anywhere in the request pipeline to a {@link Response}. * - * @author thced + *

Runs outside any {@code ScopedValue} bindings established by filters or interceptors — scopes + * are torn down as the exception unwinds. Context-aware error mapping (trace IDs, etc.) should be + * done in a {@link RequestInterceptor} that wraps {@code next.proceed()} in try/catch. */ @FunctionalInterface public interface ExceptionHandler { - void handle(HttpExchange exchange, Throwable t) throws IOException; + Response handle(Throwable t); } diff --git a/src/main/java/com/retailsvc/http/Handlers.java b/src/main/java/com/retailsvc/http/Handlers.java index 10ed1e3..0e12af3 100644 --- a/src/main/java/com/retailsvc/http/Handlers.java +++ b/src/main/java/com/retailsvc/http/Handlers.java @@ -7,13 +7,11 @@ import static java.net.HttpURLConnection.HTTP_NO_CONTENT; import static java.net.HttpURLConnection.HTTP_OK; import static java.net.HttpURLConnection.HTTP_UNAVAILABLE; -import static java.nio.charset.StandardCharsets.UTF_8; import com.retailsvc.http.internal.ClasspathResourceHandler; import com.retailsvc.http.internal.MethodLimitedHandler; -import com.retailsvc.http.internal.ProblemDetailRenderer; +import com.retailsvc.http.internal.ProblemDetail; import com.sun.net.httpserver.HttpHandler; -import java.io.IOException; import java.util.List; import java.util.Objects; import java.util.function.Supplier; @@ -27,31 +25,31 @@ public final class Handlers { private Handlers() {} - public static ExceptionHandler defaultExceptionHandler() { - return (exchange, t) -> { - try (exchange) { + public static ExceptionHandler defaultExceptionHandler(TypeMapper jsonMapper) { + Objects.requireNonNull(jsonMapper, "jsonMapper must not be null"); + return t -> switch (t) { - case ValidationException ve -> { - byte[] body = ProblemDetailRenderer.render(ve.error()).getBytes(UTF_8); - exchange.getResponseHeaders().add("Content-Type", "application/problem+json"); - exchange.sendResponseHeaders(HTTP_BAD_REQUEST, body.length); - exchange.getResponseBody().write(body); - } - case NotFoundException _ -> exchange.sendResponseHeaders(HTTP_NOT_FOUND, -1); - case MethodNotAllowedException mna -> { - String allow = mna.allowed().stream().map(Enum::name).collect(Collectors.joining(", ")); - exchange.getResponseHeaders().add("Allow", allow); - exchange.sendResponseHeaders(HTTP_BAD_METHOD, -1); - } + case ValidationException ve -> + Response.bytes( + HTTP_BAD_REQUEST, + jsonMapper.writeTo(ProblemDetail.forValidation(ve.error())), + "application/problem+json"); + case BadRequestException bre -> + Response.bytes( + bre.status(), + jsonMapper.writeTo(ProblemDetail.forBadRequest(bre)), + "application/problem+json"); + case NotFoundException _ -> Response.notFound(); + case MethodNotAllowedException mna -> + Response.status(HTTP_BAD_METHOD) + .withHeader( + "Allow", + mna.allowed().stream().map(Enum::name).collect(Collectors.joining(", "))); default -> { LOG.error("Unhandled exception in handler", t); - exchange.sendResponseHeaders(HTTP_INTERNAL_ERROR, -1); + yield Response.status(HTTP_INTERNAL_ERROR); } - } - } catch (IOException io) { - LOG.error("Failed writing error response", io); - } - }; + }; } public static HttpHandler notFoundHandler() { diff --git a/src/main/java/com/retailsvc/http/OpenApiServer.java b/src/main/java/com/retailsvc/http/OpenApiServer.java index dbcbbc0..f280b69 100644 --- a/src/main/java/com/retailsvc/http/OpenApiServer.java +++ b/src/main/java/com/retailsvc/http/OpenApiServer.java @@ -74,10 +74,6 @@ record HandlerConfig( requireNonNull(bodyMappers, "bodyMappers must not be null"); requireNonNull(handlerConfig.handlers(), "handlers must not be null"); ExceptionHandler exceptionHandler = handlerConfig.exceptionHandler(); - if (exceptionHandler == null) { - LOG.warn("No ExceptionHandler set, using default"); - exceptionHandler = Handlers.defaultExceptionHandler(); - } long t0 = System.currentTimeMillis(); Router router = new Router(spec.operations()); @@ -93,9 +89,11 @@ record HandlerConfig( this.httpServer = HttpServer.create(socketAddress, 0); httpServer.setExecutor(newThreadPerTaskExecutor(ofVirtual().name("http-", 0).factory())); + ResponseRenderer renderer = new ResponseRenderer(bodyMappers); + String basePath = Optional.ofNullable(spec.basePath()).orElse("/"); HttpContext ctx = httpServer.createContext(basePath); - ctx.getFilters().add(new ExceptionFilter(exceptionHandler)); + ctx.getFilters().add(new ExceptionFilter(exceptionHandler, renderer)); ctx.getFilters().add(new RequestPreparationFilter(spec, router, validator, bodyMappers)); ctx.getFilters() .add( @@ -110,11 +108,11 @@ record HandlerConfig( handlerConfig.handlers(), handlerConfig.interceptors(), handlerConfig.decorators(), - new ResponseRenderer(bodyMappers))); + renderer)); for (Map.Entry e : handlerConfig.extras().entrySet()) { HttpContext extraCtx = httpServer.createContext(e.getKey()); - extraCtx.getFilters().add(new ExceptionFilter(exceptionHandler)); + extraCtx.getFilters().add(new ExceptionFilter(exceptionHandler, renderer)); extraCtx.setHandler(e.getValue()); } @@ -320,12 +318,16 @@ public OpenApiServer build() throws IOException { validateSecurityWiring(spec, securityValidators); } Map resolved = resolveBodyMappers(bodyMappers); + ExceptionHandler effectiveExceptionHandler = + exceptionHandler != null + ? exceptionHandler + : Handlers.defaultExceptionHandler(resolved.get(JSON)); HandlerConfig handlerConfig = new HandlerConfig( handlers, interceptors, decorators, - exceptionHandler, + effectiveExceptionHandler, extras, Map.copyOf(securityValidators), externalAuth); diff --git a/src/main/java/com/retailsvc/http/internal/ExceptionFilter.java b/src/main/java/com/retailsvc/http/internal/ExceptionFilter.java index af8f6a4..649a14e 100644 --- a/src/main/java/com/retailsvc/http/internal/ExceptionFilter.java +++ b/src/main/java/com/retailsvc/http/internal/ExceptionFilter.java @@ -1,15 +1,19 @@ package com.retailsvc.http.internal; import com.retailsvc.http.ExceptionHandler; +import com.retailsvc.http.Response; import com.sun.net.httpserver.Filter; import com.sun.net.httpserver.HttpExchange; import java.io.IOException; public final class ExceptionFilter extends Filter { + private final ExceptionHandler handler; + private final ResponseRenderer renderer; - public ExceptionFilter(ExceptionHandler handler) { + public ExceptionFilter(ExceptionHandler handler, ResponseRenderer renderer) { this.handler = handler; + this.renderer = renderer; } @Override @@ -17,7 +21,8 @@ public void doFilter(HttpExchange exchange, Chain chain) throws IOException { try { chain.doFilter(exchange); } catch (RuntimeException | IOException t) { - handler.handle(exchange, t); + Response response = handler.handle(t); + renderer.render(exchange, response); } } diff --git a/src/test/java/com/retailsvc/http/ExtraHandlersIT.java b/src/test/java/com/retailsvc/http/ExtraHandlersIT.java index d6e1393..fdfdb1c 100644 --- a/src/test/java/com/retailsvc/http/ExtraHandlersIT.java +++ b/src/test/java/com/retailsvc/http/ExtraHandlersIT.java @@ -1,6 +1,5 @@ package com.retailsvc.http; -import static com.retailsvc.http.Handlers.defaultExceptionHandler; import static org.assertj.core.api.Assertions.assertThat; import java.net.URI; @@ -17,7 +16,6 @@ void aliveExtraReturns204AndBypassesValidation() throws Exception { newBuilder() .spec(spec) .handlers(Map.of()) - .exceptionHandler(defaultExceptionHandler()) .port(0) .extraRoute("/alive", Handlers.aliveHandler()) .build(); @@ -41,7 +39,6 @@ void specHandlerServesClasspathResource() throws Exception { newBuilder() .spec(spec) .handlers(Map.of()) - .exceptionHandler(defaultExceptionHandler()) .port(0) .extraRoute("/openapi.yaml", Handlers.specHandler("/openapi.yaml")) .build(); @@ -68,13 +65,7 @@ void extraHandlerExceptionFlowsThroughExceptionHandler() throws Exception { }; try (var s = - newBuilder() - .spec(spec) - .handlers(Map.of()) - .exceptionHandler(defaultExceptionHandler()) - .port(0) - .extraRoute("/boom", boom) - .build(); + newBuilder().spec(spec).handlers(Map.of()).port(0).extraRoute("/boom", boom).build(); var client = httpClient()) { var req = diff --git a/src/test/java/com/retailsvc/http/HandlersDefaultExceptionTest.java b/src/test/java/com/retailsvc/http/HandlersDefaultExceptionTest.java index 8bac1d0..c371361 100644 --- a/src/test/java/com/retailsvc/http/HandlersDefaultExceptionTest.java +++ b/src/test/java/com/retailsvc/http/HandlersDefaultExceptionTest.java @@ -1,54 +1,80 @@ package com.retailsvc.http; import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Mockito.mock; import com.retailsvc.http.spec.HttpMethod; import com.retailsvc.http.validate.ValidationError; -import com.sun.net.httpserver.Headers; -import com.sun.net.httpserver.HttpExchange; -import java.io.ByteArrayOutputStream; +import java.nio.charset.StandardCharsets; +import java.util.Map; import java.util.Set; import org.junit.jupiter.api.Test; -import org.mockito.Mockito; class HandlersDefaultExceptionTest { - private HttpExchange newExchange(ByteArrayOutputStream sink) { - HttpExchange ex = mock(HttpExchange.class); - Mockito.when(ex.getResponseHeaders()).thenReturn(new Headers()); - Mockito.when(ex.getResponseBody()).thenReturn(sink); - return ex; + + private static final TypeMapper JSON = new GsonTypeMapper(); + + @Test + void validationExceptionRendersProblemJson() { + Response resp = + Handlers.defaultExceptionHandler(JSON) + .handle( + new ValidationException( + new ValidationError("/x", "type", "expected string", null))); + + assertThat(resp.status()).isEqualTo(400); + assertThat(resp.contentType()).isEqualTo("application/problem+json"); + byte[] bytes = (byte[]) resp.body(); + String json = new String(bytes, StandardCharsets.UTF_8); + @SuppressWarnings("unchecked") + Map parsed = (Map) JSON.readFrom(bytes, "application/json"); + assertThat(parsed).containsEntry("keyword", "type"); + assertThat(((Number) parsed.get("status")).intValue()).isEqualTo(400); + assertThat(json).contains("expected string"); } @Test - void validationExceptionRendersProblem() throws Exception { - ByteArrayOutputStream sink = new ByteArrayOutputStream(); - HttpExchange ex = newExchange(sink); - - Handlers.defaultExceptionHandler() - .handle( - ex, - new ValidationException(new ValidationError("/x", "type", "expected string", null))); - - Mockito.verify(ex).sendResponseHeaders(Mockito.eq(400), Mockito.anyLong()); - assertThat(ex.getResponseHeaders().getFirst("Content-Type")) - .isEqualTo("application/problem+json"); - assertThat(sink.toString()).contains("\"keyword\":\"type\""); + void badRequestExceptionRendersProblemJsonWithCustomStatus() { + Response resp = + Handlers.defaultExceptionHandler(JSON) + .handle(new BadRequestException(422, "email taken", "/email", "unique")); + + assertThat(resp.status()).isEqualTo(422); + assertThat(resp.contentType()).isEqualTo("application/problem+json"); + @SuppressWarnings("unchecked") + Map parsed = + (Map) JSON.readFrom((byte[]) resp.body(), "application/json"); + assertThat(((Number) parsed.get("status")).intValue()).isEqualTo(422); + assertThat(parsed) + .containsEntry("title", "Unprocessable Content") + .containsEntry("detail", "email taken") + .containsEntry("pointer", "/email") + .containsEntry("keyword", "unique"); } @Test - void notFoundReturns404() throws Exception { - HttpExchange ex = newExchange(new ByteArrayOutputStream()); - Handlers.defaultExceptionHandler().handle(ex, new NotFoundException("GET /x")); - Mockito.verify(ex).sendResponseHeaders(404, -1); + void notFoundReturns404() { + Response resp = Handlers.defaultExceptionHandler(JSON).handle(new NotFoundException("GET /x")); + + assertThat(resp.status()).isEqualTo(404); + assertThat(resp.body()).isNull(); } @Test - void methodNotAllowedReturns405WithAllowHeader() throws Exception { - HttpExchange ex = newExchange(new ByteArrayOutputStream()); - Handlers.defaultExceptionHandler() - .handle(ex, new MethodNotAllowedException(Set.of(HttpMethod.GET, HttpMethod.POST))); - Mockito.verify(ex).sendResponseHeaders(405, -1); - assertThat(ex.getResponseHeaders().getFirst("Allow")).contains("GET").contains("POST"); + void methodNotAllowedReturns405WithAllowHeader() { + Response resp = + Handlers.defaultExceptionHandler(JSON) + .handle(new MethodNotAllowedException(Set.of(HttpMethod.GET, HttpMethod.POST))); + + assertThat(resp.status()).isEqualTo(405); + assertThat(resp.headers()).containsKey("Allow"); + assertThat(resp.headers().get("Allow")).contains("GET").contains("POST"); + } + + @Test + void unknownExceptionReturns500() { + Response resp = Handlers.defaultExceptionHandler(JSON).handle(new RuntimeException("kaboom")); + + assertThat(resp.status()).isEqualTo(500); + assertThat(resp.body()).isNull(); } } diff --git a/src/test/java/com/retailsvc/http/internal/ExceptionFilterTest.java b/src/test/java/com/retailsvc/http/internal/ExceptionFilterTest.java index 1b08ca3..1333e96 100644 --- a/src/test/java/com/retailsvc/http/internal/ExceptionFilterTest.java +++ b/src/test/java/com/retailsvc/http/internal/ExceptionFilterTest.java @@ -4,28 +4,33 @@ import com.retailsvc.http.ExceptionHandler; import com.retailsvc.http.NotFoundException; +import com.retailsvc.http.Response; import com.sun.net.httpserver.Filter; import com.sun.net.httpserver.HttpExchange; import org.junit.jupiter.api.Test; import org.mockito.Mockito; class ExceptionFilterTest { + @Test void delegatesToExceptionHandler() throws Exception { HttpExchange ex = mock(HttpExchange.class); + ResponseRenderer renderer = mock(ResponseRenderer.class); ExceptionHandler handler = mock(ExceptionHandler.class); - Filter f = new ExceptionFilter(handler); + Mockito.when(handler.handle(Mockito.any())).thenReturn(Response.status(404)); + Filter f = new ExceptionFilter(handler, renderer); Filter.Chain chain = mock(Filter.Chain.class); Mockito.doThrow(new NotFoundException("x")).when(chain).doFilter(ex); f.doFilter(ex, chain); - Mockito.verify(handler).handle(Mockito.eq(ex), Mockito.any(NotFoundException.class)); + Mockito.verify(handler).handle(Mockito.any(NotFoundException.class)); } @Test void passThroughOnSuccess() throws Exception { HttpExchange ex = mock(HttpExchange.class); + ResponseRenderer renderer = mock(ResponseRenderer.class); ExceptionHandler handler = mock(ExceptionHandler.class); - Filter f = new ExceptionFilter(handler); + Filter f = new ExceptionFilter(handler, renderer); Filter.Chain chain = mock(Filter.Chain.class); f.doFilter(ex, chain); Mockito.verify(chain).doFilter(ex); diff --git a/src/test/java/com/retailsvc/http/start/ServerLauncher.java b/src/test/java/com/retailsvc/http/start/ServerLauncher.java index 670e623..104517a 100644 --- a/src/test/java/com/retailsvc/http/start/ServerLauncher.java +++ b/src/test/java/com/retailsvc/http/start/ServerLauncher.java @@ -1,7 +1,5 @@ package com.retailsvc.http.start; -import com.retailsvc.http.ExceptionHandler; -import com.retailsvc.http.Handlers; import com.retailsvc.http.OpenApiServer; import com.retailsvc.http.RequestHandler; import com.retailsvc.http.Response; @@ -44,12 +42,9 @@ public ServerLauncher() throws IOException { handlers.put("secureBasic", req -> Response.status(200)); handlers.put("secureOpen", req -> Response.status(200)); - ExceptionHandler exceptionHandler = Handlers.defaultExceptionHandler(); - OpenApiServer.builder() .spec(spec) .handlers(handlers) - .exceptionHandler(exceptionHandler) .securityValidator("apiKeyAuth", (req, cred) -> Optional.empty()) .securityValidator("bearerAuth", (req, cred) -> Optional.empty()) .securityValidator("basicAuth", (req, cred) -> Optional.empty()) From 94e5db7a67d4df62b2cf58d53b6a69cc9f438bcf Mon Sep 17 00:00:00 2001 From: Thomas Cederholm Date: Wed, 20 May 2026 13:03:03 +0200 Subject: [PATCH 08/11] feat: Migrate Handlers factories to RequestHandler --- .../java/com/retailsvc/http/Handlers.java | 66 ++++---- .../internal/ClasspathResourceHandler.java | 28 ++-- .../com/retailsvc/http/ExtraHandlersIT.java | 26 +++- .../java/com/retailsvc/http/HandlersTest.java | 108 ++++++------- .../com/retailsvc/http/HealthHandlerTest.java | 143 +++++++----------- .../http/OpenApiServerBuilderTest.java | 8 +- .../ClasspathResourceHandlerTest.java | 80 +++------- 7 files changed, 198 insertions(+), 261 deletions(-) diff --git a/src/main/java/com/retailsvc/http/Handlers.java b/src/main/java/com/retailsvc/http/Handlers.java index 0e12af3..b3cb1df 100644 --- a/src/main/java/com/retailsvc/http/Handlers.java +++ b/src/main/java/com/retailsvc/http/Handlers.java @@ -1,15 +1,15 @@ package com.retailsvc.http; +import static com.retailsvc.http.spec.HttpMethod.GET; +import static com.retailsvc.http.spec.HttpMethod.HEAD; import static java.net.HttpURLConnection.HTTP_BAD_METHOD; import static java.net.HttpURLConnection.HTTP_BAD_REQUEST; import static java.net.HttpURLConnection.HTTP_INTERNAL_ERROR; import static java.net.HttpURLConnection.HTTP_NOT_FOUND; -import static java.net.HttpURLConnection.HTTP_NO_CONTENT; import static java.net.HttpURLConnection.HTTP_OK; import static java.net.HttpURLConnection.HTTP_UNAVAILABLE; import com.retailsvc.http.internal.ClasspathResourceHandler; -import com.retailsvc.http.internal.MethodLimitedHandler; import com.retailsvc.http.internal.ProblemDetail; import com.sun.net.httpserver.HttpHandler; import java.util.List; @@ -61,13 +61,12 @@ public static HttpHandler notFoundHandler() { } /** Returns 204 No Content on GET/HEAD; 405 with {@code Allow: GET, HEAD} otherwise. */ - public static HttpHandler aliveHandler() { - return new MethodLimitedHandler( - exchange -> { - try (exchange) { - exchange.sendResponseHeaders(HTTP_NO_CONTENT, -1); - } - }); + public static RequestHandler aliveHandler() { + return req -> + switch (req.method()) { + case GET, HEAD -> Response.empty(); + default -> Response.status(HTTP_BAD_METHOD).withHeader("Allow", "GET, HEAD"); + }; } /** @@ -91,26 +90,24 @@ public static HttpHandler aliveHandler() { * @param jsonMapper used to encode the wire-shape DTO to bytes * @param probe supplier of the current {@link HealthOutcome} */ - public static HttpHandler healthHandler(TypeMapper jsonMapper, Supplier probe) { + public static RequestHandler healthHandler(TypeMapper jsonMapper, Supplier probe) { Objects.requireNonNull(jsonMapper, "jsonMapper"); Objects.requireNonNull(probe, "probe"); - return new MethodLimitedHandler( - exchange -> { - try (exchange) { - HealthOutcome outcome; - try { - outcome = Objects.requireNonNull(probe.get(), "Health probe returned null"); - } catch (RuntimeException e) { - LOG.warn("Health probe failed", e); - outcome = new HealthOutcome(false, List.of()); - } - byte[] body = jsonMapper.writeTo(toWireShape(outcome)); - int status = outcome.up() ? HTTP_OK : HTTP_UNAVAILABLE; - exchange.getResponseHeaders().add("Content-Type", "application/json"); - exchange.sendResponseHeaders(status, body.length); - exchange.getResponseBody().write(body); - } - }); + return req -> { + if (req.method() != GET && req.method() != HEAD) { + return Response.status(HTTP_BAD_METHOD).withHeader("Allow", "GET, HEAD"); + } + HealthOutcome outcome; + try { + outcome = Objects.requireNonNull(probe.get(), "Health probe returned null"); + } catch (RuntimeException e) { + LOG.warn("Health probe failed", e); + outcome = new HealthOutcome(false, List.of()); + } + byte[] body = jsonMapper.writeTo(toWireShape(outcome)); + int status = outcome.up() ? HTTP_OK : HTTP_UNAVAILABLE; + return Response.bytes(status, body, "application/json"); + }; } private static HealthBody toWireShape(HealthOutcome outcome) { @@ -137,7 +134,18 @@ private record DependencyBody(String id, String status) {} * * @param classpathResource absolute classpath path, e.g. {@code /schemas/v1/openapi.yaml} */ - public static HttpHandler specHandler(String classpathResource) { - return new MethodLimitedHandler(new ClasspathResourceHandler(classpathResource)); + public static RequestHandler specHandler(String classpathResource) { + ClasspathResourceHandler resource = new ClasspathResourceHandler(classpathResource); + byte[] bytes = resource.bytes(); + String contentType = resource.contentType(); + return req -> + switch (req.method()) { + case GET -> Response.bytes(HTTP_OK, bytes, contentType); + case HEAD -> + Response.status(HTTP_OK) + .withContentType(contentType) + .withHeader("Content-Length", String.valueOf(bytes.length)); + default -> Response.status(HTTP_BAD_METHOD).withHeader("Allow", "GET, HEAD"); + }; } } diff --git a/src/main/java/com/retailsvc/http/internal/ClasspathResourceHandler.java b/src/main/java/com/retailsvc/http/internal/ClasspathResourceHandler.java index 03791f8..64e9417 100644 --- a/src/main/java/com/retailsvc/http/internal/ClasspathResourceHandler.java +++ b/src/main/java/com/retailsvc/http/internal/ClasspathResourceHandler.java @@ -1,18 +1,14 @@ package com.retailsvc.http.internal; -import static java.net.HttpURLConnection.HTTP_OK; - -import com.sun.net.httpserver.HttpExchange; -import com.sun.net.httpserver.HttpHandler; import java.io.IOException; import java.io.InputStream; import java.util.Locale; /** - * Serves bytes loaded eagerly from a classpath resource. Content-Type is inferred from the file - * extension. Throws {@link IllegalArgumentException} if the resource is missing. + * Eagerly-loaded bytes for a classpath resource. Content-Type is inferred from the file extension. + * Throws {@link IllegalArgumentException} if the resource is missing. */ -public final class ClasspathResourceHandler implements HttpHandler { +public final class ClasspathResourceHandler { private final byte[] bytes; private final String contentType; @@ -30,18 +26,12 @@ public ClasspathResourceHandler(String classpathResource) { this.contentType = contentTypeFor(classpathResource); } - @Override - public void handle(HttpExchange exchange) throws IOException { - try (exchange) { - exchange.getResponseHeaders().add("Content-Type", contentType); - if ("HEAD".equals(exchange.getRequestMethod())) { - exchange.getResponseHeaders().add("Content-Length", String.valueOf(bytes.length)); - exchange.sendResponseHeaders(HTTP_OK, -1); - return; - } - exchange.sendResponseHeaders(HTTP_OK, bytes.length); - exchange.getResponseBody().write(bytes); - } + public byte[] bytes() { + return bytes; + } + + public String contentType() { + return contentType; } private static String contentTypeFor(String path) { diff --git a/src/test/java/com/retailsvc/http/ExtraHandlersIT.java b/src/test/java/com/retailsvc/http/ExtraHandlersIT.java index fdfdb1c..eaa11a8 100644 --- a/src/test/java/com/retailsvc/http/ExtraHandlersIT.java +++ b/src/test/java/com/retailsvc/http/ExtraHandlersIT.java @@ -11,14 +11,16 @@ class ExtraHandlersIT extends ServerBaseTest { @Test + // MIGRATED-IN-TASK-6: re-enable Handlers.aliveHandler() once extraRoute accepts RequestHandler void aliveExtraReturns204AndBypassesValidation() throws Exception { + com.sun.net.httpserver.HttpHandler alive = + ex -> { + try (ex) { + ex.sendResponseHeaders(204, -1); + } + }; try (var s = - newBuilder() - .spec(spec) - .handlers(Map.of()) - .port(0) - .extraRoute("/alive", Handlers.aliveHandler()) - .build(); + newBuilder().spec(spec).handlers(Map.of()).port(0).extraRoute("/alive", alive).build(); var client = httpClient()) { var req = @@ -34,13 +36,23 @@ void aliveExtraReturns204AndBypassesValidation() throws Exception { } @Test + // MIGRATED-IN-TASK-6: re-enable Handlers.specHandler() once extraRoute accepts RequestHandler void specHandlerServesClasspathResource() throws Exception { + byte[] yamlBytes = ExtraHandlersIT.class.getResourceAsStream("/openapi.yaml").readAllBytes(); + com.sun.net.httpserver.HttpHandler serveYaml = + ex -> { + try (ex) { + ex.getResponseHeaders().add("Content-Type", "application/yaml"); + ex.sendResponseHeaders(200, yamlBytes.length); + ex.getResponseBody().write(yamlBytes); + } + }; try (var s = newBuilder() .spec(spec) .handlers(Map.of()) .port(0) - .extraRoute("/openapi.yaml", Handlers.specHandler("/openapi.yaml")) + .extraRoute("/openapi.yaml", serveYaml) .build(); var client = httpClient()) { diff --git a/src/test/java/com/retailsvc/http/HandlersTest.java b/src/test/java/com/retailsvc/http/HandlersTest.java index dd640f2..b4004fb 100644 --- a/src/test/java/com/retailsvc/http/HandlersTest.java +++ b/src/test/java/com/retailsvc/http/HandlersTest.java @@ -1,71 +1,62 @@ package com.retailsvc.http; +import static com.retailsvc.http.spec.HttpMethod.GET; +import static com.retailsvc.http.spec.HttpMethod.HEAD; +import static com.retailsvc.http.spec.HttpMethod.POST; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; - -import com.sun.net.httpserver.Headers; -import com.sun.net.httpserver.HttpExchange; -import java.io.ByteArrayOutputStream; -import java.io.IOException; + +import com.retailsvc.http.spec.HttpMethod; +import java.util.Map; +import java.util.function.UnaryOperator; import org.junit.jupiter.api.Test; class HandlersTest { - @Test - void aliveHandlerReturns204OnGet() throws IOException { - HttpExchange ex = newExchange("GET"); - Handlers.aliveHandler().handle(ex); - verify(ex).sendResponseHeaders(204, -1); + private static final UnaryOperator NO_HEADERS = name -> null; + + private static Request request(HttpMethod method) { + return new Request(new byte[0], null, null, null, Map.of(), null, NO_HEADERS, Map.of(), method); } @Test - void aliveHandlerReturns204OnHead() throws IOException { - HttpExchange ex = newExchange("HEAD"); - Handlers.aliveHandler().handle(ex); - verify(ex).sendResponseHeaders(204, -1); + void aliveHandlerReturns204OnGet() { + Response resp = Handlers.aliveHandler().handle(request(GET)); + + assertThat(resp.status()).isEqualTo(204); + assertThat(resp.body()).isNull(); } @Test - void aliveHandlerReturns405OnPost() throws IOException { - HttpExchange ex = newExchange("POST"); - Headers headers = new Headers(); - when(ex.getResponseHeaders()).thenReturn(headers); - Handlers.aliveHandler().handle(ex); - verify(ex).sendResponseHeaders(405, -1); - assertThat(headers.getFirst("Allow")).isEqualTo("GET, HEAD"); + void aliveHandlerReturns204OnHead() { + Response resp = Handlers.aliveHandler().handle(request(HEAD)); + + assertThat(resp.status()).isEqualTo(204); } @Test - void specHandlerServesYamlWithInferredContentType() throws IOException { - HttpExchange ex = newExchange("GET"); - Headers responseHeaders = new Headers(); - when(ex.getResponseHeaders()).thenReturn(responseHeaders); - ByteArrayOutputStream body = new ByteArrayOutputStream(); - when(ex.getResponseBody()).thenReturn(body); - - Handlers.specHandler("/openapi.yaml").handle(ex); - - assertThat(responseHeaders.getFirst("Content-Type")).isEqualTo("application/yaml"); - verify(ex) - .sendResponseHeaders( - org.mockito.ArgumentMatchers.eq(200), - org.mockito.ArgumentMatchers.longThat(n -> n > 0)); - assertThat(body.toByteArray()).isNotEmpty(); + void aliveHandlerReturns405OnPost() { + Response resp = Handlers.aliveHandler().handle(request(POST)); + + assertThat(resp.status()).isEqualTo(405); + assertThat(resp.headers()).containsEntry("Allow", "GET, HEAD"); } @Test - void specHandlerInfersJsonContentType() throws IOException { - HttpExchange ex = newExchange("GET"); - Headers responseHeaders = new Headers(); - when(ex.getResponseHeaders()).thenReturn(responseHeaders); - when(ex.getResponseBody()).thenReturn(new ByteArrayOutputStream()); + void specHandlerServesYamlBytesWithInferredContentType() { + Response resp = Handlers.specHandler("/openapi.yaml").handle(request(GET)); - Handlers.specHandler("/openapi.json").handle(ex); + assertThat(resp.status()).isEqualTo(200); + assertThat(resp.contentType()).isEqualTo("application/yaml"); + assertThat(resp.body()).isInstanceOf(byte[].class); + assertThat((byte[]) resp.body()).isNotEmpty(); + } + + @Test + void specHandlerInfersJsonContentType() { + Response resp = Handlers.specHandler("/openapi.json").handle(request(GET)); - assertThat(responseHeaders.getFirst("Content-Type")).isEqualTo("application/json"); + assertThat(resp.contentType()).isEqualTo("application/json"); } @Test @@ -76,21 +67,20 @@ void specHandlerThrowsAtConstructionForMissingResource() { } @Test - void specHandlerReturns405OnPost() throws IOException { - HttpExchange ex = newExchange("POST"); - Headers headers = new Headers(); - when(ex.getResponseHeaders()).thenReturn(headers); + void specHandlerReturns405OnPost() { + Response resp = Handlers.specHandler("/openapi.yaml").handle(request(POST)); - Handlers.specHandler("/openapi.yaml").handle(ex); - - verify(ex).sendResponseHeaders(405, -1); - assertThat(headers.getFirst("Allow")).isEqualTo("GET, HEAD"); + assertThat(resp.status()).isEqualTo(405); + assertThat(resp.headers()).containsEntry("Allow", "GET, HEAD"); } - private static HttpExchange newExchange(String method) { - HttpExchange ex = mock(HttpExchange.class); - when(ex.getRequestMethod()).thenReturn(method); - when(ex.getResponseHeaders()).thenReturn(new Headers()); - return ex; + @Test + void specHandlerHeadReturnsContentLengthWithoutBody() { + Response resp = Handlers.specHandler("/openapi.yaml").handle(request(HEAD)); + + assertThat(resp.status()).isEqualTo(200); + assertThat(resp.body()).isNull(); + assertThat(resp.headers()).containsKey("Content-Length"); + assertThat(Integer.parseInt(resp.headers().get("Content-Length"))).isGreaterThan(0); } } diff --git a/src/test/java/com/retailsvc/http/HealthHandlerTest.java b/src/test/java/com/retailsvc/http/HealthHandlerTest.java index c9dfc0c..cbee414 100644 --- a/src/test/java/com/retailsvc/http/HealthHandlerTest.java +++ b/src/test/java/com/retailsvc/http/HealthHandlerTest.java @@ -1,136 +1,105 @@ package com.retailsvc.http; +import static com.retailsvc.http.spec.HttpMethod.GET; +import static com.retailsvc.http.spec.HttpMethod.HEAD; +import static com.retailsvc.http.spec.HttpMethod.POST; import static java.net.HttpURLConnection.HTTP_BAD_METHOD; import static java.net.HttpURLConnection.HTTP_OK; import static java.net.HttpURLConnection.HTTP_UNAVAILABLE; import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; import com.retailsvc.http.internal.gson.GsonJsonMapper; -import com.sun.net.httpserver.Headers; -import com.sun.net.httpserver.HttpExchange; -import java.io.ByteArrayOutputStream; -import java.io.IOException; +import com.retailsvc.http.spec.HttpMethod; +import java.nio.charset.StandardCharsets; import java.util.List; +import java.util.Map; import java.util.function.Supplier; +import java.util.function.UnaryOperator; import org.junit.jupiter.api.Test; class HealthHandlerTest { private static final TypeMapper JSON = new GsonJsonMapper(); + private static final UnaryOperator NO_HEADERS = name -> null; + + private static Request request(HttpMethod method) { + return new Request(new byte[0], null, null, null, Map.of(), null, NO_HEADERS, Map.of(), method); + } @Test - void getReturns200AndJsonBodyWhenUp() throws IOException { + void getReturns200AndJsonBodyWhenUp() { HealthOutcome outcome = new HealthOutcome(true, List.of(new Dependency("jdbc", true))); - HttpExchange ex = newExchange("GET"); - Headers headers = new Headers(); - when(ex.getResponseHeaders()).thenReturn(headers); - ByteArrayOutputStream body = new ByteArrayOutputStream(); - when(ex.getResponseBody()).thenReturn(body); - - Handlers.healthHandler(JSON, () -> outcome).handle(ex); - - verify(ex).sendResponseHeaders(HTTP_OK, (long) body.size()); - assertThat(headers.getFirst("Content-Type")).isEqualTo("application/json"); - assertThat(body) - .hasToString("{\"outcome\":\"Up\",\"dependencies\":[{\"id\":\"jdbc\",\"status\":\"Up\"}]}"); + + Response resp = Handlers.healthHandler(JSON, () -> outcome).handle(request(GET)); + + assertThat(resp.status()).isEqualTo(HTTP_OK); + assertThat(resp.contentType()).isEqualTo("application/json"); + assertThat(new String((byte[]) resp.body(), StandardCharsets.UTF_8)) + .isEqualTo("{\"outcome\":\"Up\",\"dependencies\":[{\"id\":\"jdbc\",\"status\":\"Up\"}]}"); } @Test - void getReturns200WithEmptyDependencyArrayWhenNoDeps() throws IOException { - HttpExchange ex = newExchange("GET"); - Headers headers = new Headers(); - when(ex.getResponseHeaders()).thenReturn(headers); - ByteArrayOutputStream body = new ByteArrayOutputStream(); - when(ex.getResponseBody()).thenReturn(body); + void getReturns200WithEmptyDependencyArrayWhenNoDeps() { + Response resp = + Handlers.healthHandler(JSON, () -> new HealthOutcome(true, List.of())).handle(request(GET)); - Handlers.healthHandler(JSON, () -> new HealthOutcome(true, List.of())).handle(ex); - - verify(ex).sendResponseHeaders(HTTP_OK, (long) body.size()); - assertThat(body).hasToString("{\"outcome\":\"Up\",\"dependencies\":[]}"); + assertThat(resp.status()).isEqualTo(HTTP_OK); + assertThat(new String((byte[]) resp.body(), StandardCharsets.UTF_8)) + .isEqualTo("{\"outcome\":\"Up\",\"dependencies\":[]}"); } @Test - void getReturns503WhenDown() throws IOException { + void getReturns503WhenDown() { HealthOutcome outcome = new HealthOutcome(false, List.of(new Dependency("jdbc", false))); - HttpExchange ex = newExchange("GET"); - Headers headers = new Headers(); - when(ex.getResponseHeaders()).thenReturn(headers); - ByteArrayOutputStream body = new ByteArrayOutputStream(); - when(ex.getResponseBody()).thenReturn(body); - - Handlers.healthHandler(JSON, () -> outcome).handle(ex); - - verify(ex).sendResponseHeaders(HTTP_UNAVAILABLE, (long) body.size()); - assertThat(headers.getFirst("Content-Type")).isEqualTo("application/json"); - assertThat(body) - .hasToString( + + Response resp = Handlers.healthHandler(JSON, () -> outcome).handle(request(GET)); + + assertThat(resp.status()).isEqualTo(HTTP_UNAVAILABLE); + assertThat(resp.contentType()).isEqualTo("application/json"); + assertThat(new String((byte[]) resp.body(), StandardCharsets.UTF_8)) + .isEqualTo( "{\"outcome\":\"Down\",\"dependencies\":[{\"id\":\"jdbc\",\"status\":\"Down\"}]}"); } @Test - void headIsAccepted() throws IOException { - HttpExchange ex = newExchange("HEAD"); - Headers headers = new Headers(); - when(ex.getResponseHeaders()).thenReturn(headers); - ByteArrayOutputStream body = new ByteArrayOutputStream(); - when(ex.getResponseBody()).thenReturn(body); + void headIsAccepted() { + Response resp = + Handlers.healthHandler(JSON, () -> new HealthOutcome(true, List.of())) + .handle(request(HEAD)); - Handlers.healthHandler(JSON, () -> new HealthOutcome(true, List.of())).handle(ex); - - verify(ex).sendResponseHeaders(HTTP_OK, (long) body.size()); + assertThat(resp.status()).isEqualTo(HTTP_OK); } @Test - void postReturns405WithAllowHeader() throws IOException { - HttpExchange ex = newExchange("POST"); - Headers headers = new Headers(); - when(ex.getResponseHeaders()).thenReturn(headers); - - Handlers.healthHandler(JSON, () -> new HealthOutcome(true, List.of())).handle(ex); + void postReturns405WithAllowHeader() { + Response resp = + Handlers.healthHandler(JSON, () -> new HealthOutcome(true, List.of())) + .handle(request(POST)); - verify(ex).sendResponseHeaders(HTTP_BAD_METHOD, -1); - assertThat(headers.getFirst("Allow")).isEqualTo("GET, HEAD"); + assertThat(resp.status()).isEqualTo(HTTP_BAD_METHOD); + assertThat(resp.headers()).containsEntry("Allow", "GET, HEAD"); } @Test - void runtimeExceptionFromProbeMapsToDown503() throws IOException { - HttpExchange ex = newExchange("GET"); - Headers headers = new Headers(); - when(ex.getResponseHeaders()).thenReturn(headers); - ByteArrayOutputStream body = new ByteArrayOutputStream(); - when(ex.getResponseBody()).thenReturn(body); - + void runtimeExceptionFromProbeMapsToDown503() { Supplier failing = () -> { throw new IllegalStateException("boom"); }; - Handlers.healthHandler(JSON, failing).handle(ex); - verify(ex).sendResponseHeaders(HTTP_UNAVAILABLE, (long) body.size()); - assertThat(body).hasToString("{\"outcome\":\"Down\",\"dependencies\":[]}"); + Response resp = Handlers.healthHandler(JSON, failing).handle(request(GET)); + + assertThat(resp.status()).isEqualTo(HTTP_UNAVAILABLE); + assertThat(new String((byte[]) resp.body(), StandardCharsets.UTF_8)) + .isEqualTo("{\"outcome\":\"Down\",\"dependencies\":[]}"); } @Test - void nullReturnFromProbeMapsToDown503() throws IOException { - HttpExchange ex = newExchange("GET"); - Headers headers = new Headers(); - when(ex.getResponseHeaders()).thenReturn(headers); - ByteArrayOutputStream body = new ByteArrayOutputStream(); - when(ex.getResponseBody()).thenReturn(body); - - Handlers.healthHandler(JSON, () -> null).handle(ex); - - verify(ex).sendResponseHeaders(HTTP_UNAVAILABLE, (long) body.size()); - assertThat(body).hasToString("{\"outcome\":\"Down\",\"dependencies\":[]}"); - } + void nullReturnFromProbeMapsToDown503() { + Response resp = Handlers.healthHandler(JSON, () -> null).handle(request(GET)); - private static HttpExchange newExchange(String method) { - HttpExchange ex = mock(HttpExchange.class); - when(ex.getRequestMethod()).thenReturn(method); - when(ex.getResponseHeaders()).thenReturn(new Headers()); - return ex; + assertThat(resp.status()).isEqualTo(HTTP_UNAVAILABLE); + assertThat(new String((byte[]) resp.body(), StandardCharsets.UTF_8)) + .isEqualTo("{\"outcome\":\"Down\",\"dependencies\":[]}"); } } diff --git a/src/test/java/com/retailsvc/http/OpenApiServerBuilderTest.java b/src/test/java/com/retailsvc/http/OpenApiServerBuilderTest.java index b522cf5..44f33ab 100644 --- a/src/test/java/com/retailsvc/http/OpenApiServerBuilderTest.java +++ b/src/test/java/com/retailsvc/http/OpenApiServerBuilderTest.java @@ -26,7 +26,9 @@ void buildsWithRequiredFieldsOnly() { @Test void rejectsDuplicateExtraPathOnSecondAddHandler() { - HttpHandler duplicate = Handlers.aliveHandler(); + // MIGRATED-IN-TASK-6: replace stub with Handlers.aliveHandler() once extraRoute accepts + // RequestHandler + HttpHandler duplicate = exchange -> {}; OpenApiServer.Builder b = OpenApiServer.builder().spec(spec).handlers(emptyMap()).extraRoute("/alive", duplicate); @@ -38,11 +40,13 @@ void rejectsDuplicateExtraPathOnSecondAddHandler() { @Test void rejectsExtraPathEqualToSpecBasePathAtBuildTime() { // testSpec() uses "/api" as the basePath (servers[0].url = http://localhost:8080/api). + // MIGRATED-IN-TASK-6: replace stub with Handlers.aliveHandler() once extraRoute accepts + // RequestHandler OpenApiServer.Builder b = OpenApiServer.builder() .spec(spec) .handlers(emptyMap()) - .extraRoute("/api", Handlers.aliveHandler()) + .extraRoute("/api", exchange -> {}) .port(0); assertThatThrownBy(b::build) diff --git a/src/test/java/com/retailsvc/http/internal/ClasspathResourceHandlerTest.java b/src/test/java/com/retailsvc/http/internal/ClasspathResourceHandlerTest.java index 1e786a5..01b4119 100644 --- a/src/test/java/com/retailsvc/http/internal/ClasspathResourceHandlerTest.java +++ b/src/test/java/com/retailsvc/http/internal/ClasspathResourceHandlerTest.java @@ -2,15 +2,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.ArgumentMatchers.longThat; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; - -import com.sun.net.httpserver.Headers; -import com.sun.net.httpserver.HttpExchange; -import java.io.ByteArrayOutputStream; + import java.io.IOException; import java.io.InputStream; import org.junit.jupiter.api.Test; @@ -20,48 +12,41 @@ class ClasspathResourceHandlerTest { @Test void getServesBytesVerbatim() throws IOException { byte[] expected = readResource("/sample.txt"); - HttpExchange ex = newExchange("GET"); - ByteArrayOutputStream body = new ByteArrayOutputStream(); - when(ex.getResponseBody()).thenReturn(body); - new ClasspathResourceHandler("/sample.txt").handle(ex); + byte[] actual = new ClasspathResourceHandler("/sample.txt").bytes(); - verify(ex).sendResponseHeaders(200, expected.length); - assertThat(body.toByteArray()).isEqualTo(expected); + assertThat(actual).isEqualTo(expected); } @Test - void headSendsContentLengthHeaderWithoutBody() throws IOException { - byte[] expected = readResource("/sample.txt"); - HttpExchange ex = newExchange("HEAD"); - Headers responseHeaders = new Headers(); - when(ex.getResponseHeaders()).thenReturn(responseHeaders); - - new ClasspathResourceHandler("/sample.txt").handle(ex); + void contentLengthMatchesBytesLength() { + ClasspathResourceHandler handler = new ClasspathResourceHandler("/sample.txt"); - verify(ex).sendResponseHeaders(200, -1); - assertThat(responseHeaders.getFirst("Content-Length")) - .isEqualTo(String.valueOf(expected.length)); + assertThat(handler.bytes()).hasSize(handler.bytes().length); } @Test - void infersApplicationJsonForJsonExtension() throws IOException { - assertThat(contentTypeFor("/openapi.json")).isEqualTo("application/json"); + void infersApplicationJsonForJsonExtension() { + assertThat(new ClasspathResourceHandler("/openapi.json").contentType()) + .isEqualTo("application/json"); } @Test - void infersApplicationYamlForYamlExtension() throws IOException { - assertThat(contentTypeFor("/openapi.yaml")).isEqualTo("application/yaml"); + void infersApplicationYamlForYamlExtension() { + assertThat(new ClasspathResourceHandler("/openapi.yaml").contentType()) + .isEqualTo("application/yaml"); } @Test - void infersTextPlainForTxtExtension() throws IOException { - assertThat(contentTypeFor("/sample.txt")).isEqualTo("text/plain; charset=utf-8"); + void infersTextPlainForTxtExtension() { + assertThat(new ClasspathResourceHandler("/sample.txt").contentType()) + .isEqualTo("text/plain; charset=utf-8"); } @Test - void fallsBackToOctetStreamForUnknownExtension() throws IOException { - assertThat(contentTypeFor("/sample.bin")).isEqualTo("application/octet-stream"); + void fallsBackToOctetStreamForUnknownExtension() { + assertThat(new ClasspathResourceHandler("/sample.bin").contentType()) + .isEqualTo("application/octet-stream"); } @Test @@ -73,36 +58,15 @@ void missingResourceThrowsIllegalArgumentExceptionWithPathInMessage() { @Test void resourceIsLoadedEagerlyAtConstruction() { - // If the resource were loaded lazily, construction would succeed and the handle() - // call would fail. Construction itself must fail. + // If the resource were loaded lazily, construction would succeed and bytes() would fail. + // Construction itself must fail for missing resources. assertThatThrownBy(() -> new ClasspathResourceHandler("/missing.txt")) .isInstanceOf(IllegalArgumentException.class); } @Test - void contentLengthIsSetForGetRequests() throws IOException { - HttpExchange ex = newExchange("GET"); - when(ex.getResponseBody()).thenReturn(new ByteArrayOutputStream()); - - new ClasspathResourceHandler("/sample.txt").handle(ex); - - verify(ex).sendResponseHeaders(eq(200), longThat(n -> n > 0)); - } - - private static String contentTypeFor(String resource) throws IOException { - HttpExchange ex = newExchange("GET"); - Headers headers = new Headers(); - when(ex.getResponseHeaders()).thenReturn(headers); - when(ex.getResponseBody()).thenReturn(new ByteArrayOutputStream()); - new ClasspathResourceHandler(resource).handle(ex); - return headers.getFirst("Content-Type"); - } - - private static HttpExchange newExchange(String method) { - HttpExchange ex = mock(HttpExchange.class); - when(ex.getRequestMethod()).thenReturn(method); - when(ex.getResponseHeaders()).thenReturn(new Headers()); - return ex; + void bytesAreNonEmptyForExistingResource() { + assertThat(new ClasspathResourceHandler("/sample.txt").bytes()).isNotEmpty(); } private static byte[] readResource(String path) throws IOException { From 7b2e648473f55200ee16f700f6403ebfbdf6546a Mon Sep 17 00:00:00 2001 From: Thomas Cederholm Date: Wed, 20 May 2026 13:08:59 +0200 Subject: [PATCH 09/11] feat: ExtraRoute accepts RequestHandler via ExtraRouteAdapter --- .../java/com/retailsvc/http/Handlers.java | 10 ---- .../com/retailsvc/http/OpenApiServer.java | 15 ++--- .../http/internal/ExtraRouteAdapter.java | 49 ++++++++++++++++ .../http/internal/MethodLimitedHandler.java | 35 ----------- .../http/internal/NotFoundHandler.java | 18 ++++++ .../com/retailsvc/http/ExtraHandlersIT.java | 30 +++------- .../http/OpenApiServerBuilderTest.java | 9 +-- .../http/internal/ExtraRouteAdapterTest.java | 58 +++++++++++++++++++ 8 files changed, 144 insertions(+), 80 deletions(-) create mode 100644 src/main/java/com/retailsvc/http/internal/ExtraRouteAdapter.java delete mode 100644 src/main/java/com/retailsvc/http/internal/MethodLimitedHandler.java create mode 100644 src/main/java/com/retailsvc/http/internal/NotFoundHandler.java create mode 100644 src/test/java/com/retailsvc/http/internal/ExtraRouteAdapterTest.java diff --git a/src/main/java/com/retailsvc/http/Handlers.java b/src/main/java/com/retailsvc/http/Handlers.java index b3cb1df..6b637bf 100644 --- a/src/main/java/com/retailsvc/http/Handlers.java +++ b/src/main/java/com/retailsvc/http/Handlers.java @@ -5,13 +5,11 @@ import static java.net.HttpURLConnection.HTTP_BAD_METHOD; import static java.net.HttpURLConnection.HTTP_BAD_REQUEST; import static java.net.HttpURLConnection.HTTP_INTERNAL_ERROR; -import static java.net.HttpURLConnection.HTTP_NOT_FOUND; import static java.net.HttpURLConnection.HTTP_OK; import static java.net.HttpURLConnection.HTTP_UNAVAILABLE; import com.retailsvc.http.internal.ClasspathResourceHandler; import com.retailsvc.http.internal.ProblemDetail; -import com.sun.net.httpserver.HttpHandler; import java.util.List; import java.util.Objects; import java.util.function.Supplier; @@ -52,14 +50,6 @@ public static ExceptionHandler defaultExceptionHandler(TypeMapper jsonMapper) { }; } - public static HttpHandler notFoundHandler() { - return exchange -> { - try (exchange) { - exchange.sendResponseHeaders(HTTP_NOT_FOUND, -1); - } - }; - } - /** Returns 204 No Content on GET/HEAD; 405 with {@code Allow: GET, HEAD} otherwise. */ public static RequestHandler aliveHandler() { return req -> diff --git a/src/main/java/com/retailsvc/http/OpenApiServer.java b/src/main/java/com/retailsvc/http/OpenApiServer.java index f280b69..4cd5766 100644 --- a/src/main/java/com/retailsvc/http/OpenApiServer.java +++ b/src/main/java/com/retailsvc/http/OpenApiServer.java @@ -6,7 +6,9 @@ import com.retailsvc.http.internal.DispatchHandler; import com.retailsvc.http.internal.ExceptionFilter; +import com.retailsvc.http.internal.ExtraRouteAdapter; import com.retailsvc.http.internal.FormTypeMapper; +import com.retailsvc.http.internal.NotFoundHandler; import com.retailsvc.http.internal.RequestPreparationFilter; import com.retailsvc.http.internal.ResponseRenderer; import com.retailsvc.http.internal.Router; @@ -18,7 +20,6 @@ import com.retailsvc.http.spec.security.SecurityScheme; import com.retailsvc.http.validate.DefaultValidator; import com.sun.net.httpserver.HttpContext; -import com.sun.net.httpserver.HttpHandler; import com.sun.net.httpserver.HttpServer; import java.io.IOException; import java.net.InetAddress; @@ -57,7 +58,7 @@ record HandlerConfig( List interceptors, List decorators, ExceptionHandler exceptionHandler, - Map extras, + Map extras, Map securityValidators, boolean externalAuth) {} @@ -110,14 +111,14 @@ record HandlerConfig( handlerConfig.decorators(), renderer)); - for (Map.Entry e : handlerConfig.extras().entrySet()) { + for (Map.Entry e : handlerConfig.extras().entrySet()) { HttpContext extraCtx = httpServer.createContext(e.getKey()); extraCtx.getFilters().add(new ExceptionFilter(exceptionHandler, renderer)); - extraCtx.setHandler(e.getValue()); + extraCtx.setHandler(new ExtraRouteAdapter(e.getValue(), renderer)); } if (!"/".equals(basePath)) { - httpServer.createContext("/", Handlers.notFoundHandler()); + httpServer.createContext("/", new NotFoundHandler()); } httpServer.start(); @@ -180,7 +181,7 @@ public static final class Builder { private int port = DEFAULT_PORT; private InetAddress bindAddress; private int shutdownTimeoutSeconds = 0; - private final LinkedHashMap extras = new LinkedHashMap<>(); + private final LinkedHashMap extras = new LinkedHashMap<>(); private final Map securityValidators = new LinkedHashMap<>(); private boolean externalAuth = false; @@ -294,7 +295,7 @@ public Builder shutdownTimeoutSeconds(int shutdownTimeoutSeconds) { * anything that isn't an OpenAPI {@code operationId}. For OpenAPI-described operations use * {@link #handlers(Map)}. */ - public Builder extraRoute(String path, HttpHandler handler) { + public Builder extraRoute(String path, RequestHandler handler) { requireNonNull(path, "path must not be null"); requireNonNull(handler, "handler must not be null"); if (extras.containsKey(path)) { diff --git a/src/main/java/com/retailsvc/http/internal/ExtraRouteAdapter.java b/src/main/java/com/retailsvc/http/internal/ExtraRouteAdapter.java new file mode 100644 index 0000000..95d496d --- /dev/null +++ b/src/main/java/com/retailsvc/http/internal/ExtraRouteAdapter.java @@ -0,0 +1,49 @@ +package com.retailsvc.http.internal; + +import com.retailsvc.http.Request; +import com.retailsvc.http.RequestHandler; +import com.retailsvc.http.Response; +import com.retailsvc.http.spec.HttpMethod; +import com.sun.net.httpserver.HttpExchange; +import com.sun.net.httpserver.HttpHandler; +import java.io.IOException; +import java.util.Map; + +/** + * Bridges an extra-route {@link RequestHandler} to the underlying JDK {@link HttpHandler}. + * + *

Builds a {@link Request} with {@code operationId=null}, empty path-params, empty principals, + * raw body bytes, raw query, and the parsed HTTP method, then renders the returned {@link Response} + * through the shared {@link ResponseRenderer}. OpenAPI validation, body parsing, and security are + * intentionally bypassed — extras are by definition outside the spec. + */ +public final class ExtraRouteAdapter implements HttpHandler { + + private final RequestHandler handler; + private final ResponseRenderer renderer; + + public ExtraRouteAdapter(RequestHandler handler, ResponseRenderer renderer) { + this.handler = handler; + this.renderer = renderer; + } + + @Override + public void handle(HttpExchange exchange) throws IOException { + byte[] body = exchange.getRequestBody().readAllBytes(); + HttpMethod method = HttpMethod.parse(exchange.getRequestMethod()); + var headers = exchange.getRequestHeaders(); + Request request = + new Request( + body, + null, + null, + null, + Map.of(), + exchange.getRequestURI().getRawQuery(), + headers::getFirst, + Map.of(), + method); + Response response = handler.handle(request); + renderer.render(exchange, response); + } +} diff --git a/src/main/java/com/retailsvc/http/internal/MethodLimitedHandler.java b/src/main/java/com/retailsvc/http/internal/MethodLimitedHandler.java deleted file mode 100644 index ff41580..0000000 --- a/src/main/java/com/retailsvc/http/internal/MethodLimitedHandler.java +++ /dev/null @@ -1,35 +0,0 @@ -package com.retailsvc.http.internal; - -import static java.net.HttpURLConnection.HTTP_BAD_METHOD; - -import com.sun.net.httpserver.HttpExchange; -import com.sun.net.httpserver.HttpHandler; -import java.io.IOException; - -/** - * Wraps a delegate handler so it answers only GET and HEAD. Other methods produce 405 with {@code - * Allow: GET, HEAD}. - */ -public final class MethodLimitedHandler implements HttpHandler { - - private static final String ALLOW = "GET, HEAD"; - - private final HttpHandler delegate; - - public MethodLimitedHandler(HttpHandler delegate) { - this.delegate = delegate; - } - - @Override - public void handle(HttpExchange exchange) throws IOException { - String method = exchange.getRequestMethod(); - if ("GET".equals(method) || "HEAD".equals(method)) { - delegate.handle(exchange); - return; - } - try (exchange) { - exchange.getResponseHeaders().add("Allow", ALLOW); - exchange.sendResponseHeaders(HTTP_BAD_METHOD, -1); - } - } -} diff --git a/src/main/java/com/retailsvc/http/internal/NotFoundHandler.java b/src/main/java/com/retailsvc/http/internal/NotFoundHandler.java new file mode 100644 index 0000000..c4a31e5 --- /dev/null +++ b/src/main/java/com/retailsvc/http/internal/NotFoundHandler.java @@ -0,0 +1,18 @@ +package com.retailsvc.http.internal; + +import static java.net.HttpURLConnection.HTTP_NOT_FOUND; + +import com.sun.net.httpserver.HttpExchange; +import com.sun.net.httpserver.HttpHandler; +import java.io.IOException; + +/** Returns 404 with no body. Used for the framework's catch-all {@code /} context. */ +public final class NotFoundHandler implements HttpHandler { + + @Override + public void handle(HttpExchange exchange) throws IOException { + try (exchange) { + exchange.sendResponseHeaders(HTTP_NOT_FOUND, -1); + } + } +} diff --git a/src/test/java/com/retailsvc/http/ExtraHandlersIT.java b/src/test/java/com/retailsvc/http/ExtraHandlersIT.java index eaa11a8..c243531 100644 --- a/src/test/java/com/retailsvc/http/ExtraHandlersIT.java +++ b/src/test/java/com/retailsvc/http/ExtraHandlersIT.java @@ -11,16 +11,14 @@ class ExtraHandlersIT extends ServerBaseTest { @Test - // MIGRATED-IN-TASK-6: re-enable Handlers.aliveHandler() once extraRoute accepts RequestHandler void aliveExtraReturns204AndBypassesValidation() throws Exception { - com.sun.net.httpserver.HttpHandler alive = - ex -> { - try (ex) { - ex.sendResponseHeaders(204, -1); - } - }; try (var s = - newBuilder().spec(spec).handlers(Map.of()).port(0).extraRoute("/alive", alive).build(); + newBuilder() + .spec(spec) + .handlers(Map.of()) + .port(0) + .extraRoute("/alive", Handlers.aliveHandler()) + .build(); var client = httpClient()) { var req = @@ -36,23 +34,13 @@ void aliveExtraReturns204AndBypassesValidation() throws Exception { } @Test - // MIGRATED-IN-TASK-6: re-enable Handlers.specHandler() once extraRoute accepts RequestHandler void specHandlerServesClasspathResource() throws Exception { - byte[] yamlBytes = ExtraHandlersIT.class.getResourceAsStream("/openapi.yaml").readAllBytes(); - com.sun.net.httpserver.HttpHandler serveYaml = - ex -> { - try (ex) { - ex.getResponseHeaders().add("Content-Type", "application/yaml"); - ex.sendResponseHeaders(200, yamlBytes.length); - ex.getResponseBody().write(yamlBytes); - } - }; try (var s = newBuilder() .spec(spec) .handlers(Map.of()) .port(0) - .extraRoute("/openapi.yaml", serveYaml) + .extraRoute("/openapi.yaml", Handlers.specHandler("/openapi.yaml")) .build(); var client = httpClient()) { @@ -71,8 +59,8 @@ void specHandlerServesClasspathResource() throws Exception { @Test void extraHandlerExceptionFlowsThroughExceptionHandler() throws Exception { - com.sun.net.httpserver.HttpHandler boom = - ex -> { + RequestHandler boom = + req -> { throw new RuntimeException("boom"); }; diff --git a/src/test/java/com/retailsvc/http/OpenApiServerBuilderTest.java b/src/test/java/com/retailsvc/http/OpenApiServerBuilderTest.java index 44f33ab..1808abf 100644 --- a/src/test/java/com/retailsvc/http/OpenApiServerBuilderTest.java +++ b/src/test/java/com/retailsvc/http/OpenApiServerBuilderTest.java @@ -5,7 +5,6 @@ import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import com.retailsvc.http.spec.Spec; -import com.sun.net.httpserver.HttpHandler; import java.util.List; import java.util.Map; import org.junit.jupiter.api.Test; @@ -26,9 +25,7 @@ void buildsWithRequiredFieldsOnly() { @Test void rejectsDuplicateExtraPathOnSecondAddHandler() { - // MIGRATED-IN-TASK-6: replace stub with Handlers.aliveHandler() once extraRoute accepts - // RequestHandler - HttpHandler duplicate = exchange -> {}; + RequestHandler duplicate = req -> Response.empty(); OpenApiServer.Builder b = OpenApiServer.builder().spec(spec).handlers(emptyMap()).extraRoute("/alive", duplicate); @@ -40,13 +37,11 @@ void rejectsDuplicateExtraPathOnSecondAddHandler() { @Test void rejectsExtraPathEqualToSpecBasePathAtBuildTime() { // testSpec() uses "/api" as the basePath (servers[0].url = http://localhost:8080/api). - // MIGRATED-IN-TASK-6: replace stub with Handlers.aliveHandler() once extraRoute accepts - // RequestHandler OpenApiServer.Builder b = OpenApiServer.builder() .spec(spec) .handlers(emptyMap()) - .extraRoute("/api", exchange -> {}) + .extraRoute("/api", Handlers.aliveHandler()) .port(0); assertThatThrownBy(b::build) diff --git a/src/test/java/com/retailsvc/http/internal/ExtraRouteAdapterTest.java b/src/test/java/com/retailsvc/http/internal/ExtraRouteAdapterTest.java new file mode 100644 index 0000000..184c546 --- /dev/null +++ b/src/test/java/com/retailsvc/http/internal/ExtraRouteAdapterTest.java @@ -0,0 +1,58 @@ +package com.retailsvc.http.internal; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import com.retailsvc.http.GsonTypeMapper; +import com.retailsvc.http.Request; +import com.retailsvc.http.RequestHandler; +import com.retailsvc.http.Response; +import com.retailsvc.http.TypeMapper; +import com.retailsvc.http.spec.HttpMethod; +import com.sun.net.httpserver.Headers; +import com.sun.net.httpserver.HttpExchange; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.net.URI; +import java.util.Map; +import java.util.concurrent.atomic.AtomicReference; +import org.junit.jupiter.api.Test; + +class ExtraRouteAdapterTest { + + @Test + void buildsRequestWithMethodQueryHeadersAndBodyBytesAndNullOperationId() throws Exception { + AtomicReference captured = new AtomicReference<>(); + RequestHandler handler = + req -> { + captured.set(req); + return Response.empty(); + }; + + Map mappers = Map.of("application/json", new GsonTypeMapper()); + ResponseRenderer renderer = new ResponseRenderer(mappers); + ExtraRouteAdapter adapter = new ExtraRouteAdapter(handler, renderer); + + HttpExchange ex = mock(HttpExchange.class); + Headers reqHeaders = new Headers(); + reqHeaders.add("X-Trace", "abc"); + when(ex.getRequestMethod()).thenReturn("POST"); + when(ex.getRequestURI()).thenReturn(new URI("/alive?x=1")); + when(ex.getRequestHeaders()).thenReturn(reqHeaders); + when(ex.getRequestBody()).thenReturn(new ByteArrayInputStream("hi".getBytes())); + when(ex.getResponseHeaders()).thenReturn(new Headers()); + when(ex.getResponseBody()).thenReturn(new ByteArrayOutputStream()); + + adapter.handle(ex); + + Request r = captured.get(); + assertThat(r.operationId()).isNull(); + assertThat(r.pathParams()).isEmpty(); + assertThat(r.principals()).isEmpty(); + assertThat(r.method()).isEqualTo(HttpMethod.POST); + assertThat(r.rawQuery()).isEqualTo("x=1"); + assertThat(r.header("X-Trace")).contains("abc"); + assertThat(r.bytes()).containsExactly('h', 'i'); + } +} From 63bf69a4b164e8baad6da1166c66386646aeda80 Mon Sep 17 00:00:00 2001 From: Thomas Cederholm Date: Wed, 20 May 2026 13:15:02 +0200 Subject: [PATCH 10/11] refactor: Delete hand-rolled ProblemDetailRenderer, migrate SecurityFilter to ProblemDetail --- .../com/retailsvc/http/OpenApiServer.java | 3 +- .../http/internal/ProblemDetailRenderer.java | 93 ------------------- .../http/internal/SecurityFilter.java | 13 ++- .../internal/ProblemDetailRendererTest.java | 29 ------ .../http/internal/SecurityFilterTest.java | 46 +++++++-- 5 files changed, 49 insertions(+), 135 deletions(-) delete mode 100644 src/main/java/com/retailsvc/http/internal/ProblemDetailRenderer.java delete mode 100644 src/test/java/com/retailsvc/http/internal/ProblemDetailRendererTest.java diff --git a/src/main/java/com/retailsvc/http/OpenApiServer.java b/src/main/java/com/retailsvc/http/OpenApiServer.java index 4cd5766..a152662 100644 --- a/src/main/java/com/retailsvc/http/OpenApiServer.java +++ b/src/main/java/com/retailsvc/http/OpenApiServer.java @@ -103,7 +103,8 @@ record HandlerConfig( spec.securitySchemes(), spec.security(), handlerConfig.securityValidators(), - handlerConfig.externalAuth())); + handlerConfig.externalAuth(), + bodyMappers.get(JSON))); ctx.setHandler( new DispatchHandler( handlerConfig.handlers(), diff --git a/src/main/java/com/retailsvc/http/internal/ProblemDetailRenderer.java b/src/main/java/com/retailsvc/http/internal/ProblemDetailRenderer.java deleted file mode 100644 index 31931d6..0000000 --- a/src/main/java/com/retailsvc/http/internal/ProblemDetailRenderer.java +++ /dev/null @@ -1,93 +0,0 @@ -package com.retailsvc.http.internal; - -import com.retailsvc.http.validate.ValidationError; - -/** - * Renders a {@link ValidationError} as an RFC 7807 {@code application/problem+json} document. - * - *

Hand-rolled to avoid pulling in a JSON library; only six fixed fields are emitted, all with - * known shapes, so a writer-from-scratch is safer than a generic encoder. - */ -public final class ProblemDetailRenderer { - - private static final String PROBLEM_TYPE = "about:blank"; - private static final String PROBLEM_TITLE = "Bad Request"; - private static final int PROBLEM_STATUS = 400; - - /** Initial capacity of the JSON buffer; sized for a typical problem-detail document. */ - private static final int INITIAL_BUFFER_CAPACITY = 128; - - /** Codepoints below this value are control characters and must be unicode-escaped in JSON. */ - private static final int FIRST_PRINTABLE_ASCII = 0x20; - - private ProblemDetailRenderer() {} - - public static String render(int status, String title, String detail) { - StringBuilder out = new StringBuilder(INITIAL_BUFFER_CAPACITY); - out.append('{'); - appendStringField(out, "type", PROBLEM_TYPE); - out.append(','); - appendStringField(out, "title", title); - out.append(','); - appendIntField(out, "status", status); - out.append(','); - appendStringField(out, "detail", detail); - out.append('}'); - return out.toString(); - } - - public static String render(ValidationError error) { - StringBuilder out = new StringBuilder(INITIAL_BUFFER_CAPACITY); - out.append('{'); - appendStringField(out, "type", PROBLEM_TYPE); - out.append(','); - appendStringField(out, "title", PROBLEM_TITLE); - out.append(','); - appendIntField(out, "status", PROBLEM_STATUS); - out.append(','); - appendStringField(out, "detail", error.message()); - out.append(','); - appendStringField(out, "pointer", error.pointer()); - out.append(','); - appendStringField(out, "keyword", error.keyword()); - out.append('}'); - return out.toString(); - } - - private static void appendStringField(StringBuilder out, String name, String value) { - out.append('"').append(name).append("\":\""); - appendEscaped(out, value); - out.append('"'); - } - - private static void appendIntField(StringBuilder out, String name, int value) { - out.append('"').append(name).append("\":").append(value); - } - - /** - * Appends {@code value} to {@code out} with JSON-string escaping applied. Handles the six - * mandatory escape sequences and emits {@code \uXXXX} for control characters below {@link - * #FIRST_PRINTABLE_ASCII}. - */ - private static void appendEscaped(StringBuilder out, String value) { - for (int i = 0; i < value.length(); i++) { - char c = value.charAt(i); - switch (c) { - case '\\' -> out.append("\\\\"); - case '"' -> out.append("\\\""); - case '\n' -> out.append("\\n"); - case '\r' -> out.append("\\r"); - case '\t' -> out.append("\\t"); - default -> appendUnicodeOrLiteral(out, c); - } - } - } - - private static void appendUnicodeOrLiteral(StringBuilder out, char c) { - if (c < FIRST_PRINTABLE_ASCII) { - out.append(String.format("\\u%04x", (int) c)); - } else { - out.append(c); - } - } -} diff --git a/src/main/java/com/retailsvc/http/internal/SecurityFilter.java b/src/main/java/com/retailsvc/http/internal/SecurityFilter.java index 96a9c62..22bd1d7 100644 --- a/src/main/java/com/retailsvc/http/internal/SecurityFilter.java +++ b/src/main/java/com/retailsvc/http/internal/SecurityFilter.java @@ -5,13 +5,13 @@ import com.retailsvc.http.Request; import com.retailsvc.http.SchemeValidator; +import com.retailsvc.http.TypeMapper; import com.retailsvc.http.spec.Operation; import com.retailsvc.http.spec.security.SecurityRequirement; import com.retailsvc.http.spec.security.SecurityScheme; import com.sun.net.httpserver.Filter; import com.sun.net.httpserver.HttpExchange; import java.io.IOException; -import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Comparator; import java.util.LinkedHashMap; @@ -19,6 +19,7 @@ import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.Objects; import java.util.Optional; public final class SecurityFilter extends Filter { @@ -28,18 +29,21 @@ public final class SecurityFilter extends Filter { private final List rootSecurity; private final Map validators; private final boolean externalAuth; + private final TypeMapper jsonMapper; public SecurityFilter( Map operationsById, Map schemes, List rootSecurity, Map validators, - boolean externalAuth) { + boolean externalAuth, + TypeMapper jsonMapper) { this.operationsById = Map.copyOf(operationsById); this.schemes = Map.copyOf(schemes); this.rootSecurity = List.copyOf(rootSecurity); this.validators = Map.copyOf(validators); this.externalAuth = externalAuth; + this.jsonMapper = Objects.requireNonNull(jsonMapper, "jsonMapper must not be null"); } @Override @@ -119,8 +123,9 @@ private void renderRejection(HttpExchange exchange, List fa failures.stream().max(Comparator.comparing(GroupOutcome.Failed::kind)).orElseThrow(); String detail = describe(pick); - byte[] body = - ProblemDetailRenderer.render(status, title, detail).getBytes(StandardCharsets.UTF_8); + ProblemDetail problemDetail = + new ProblemDetail("about:blank", title, status, detail, null, null); + byte[] body = jsonMapper.writeTo(problemDetail); exchange.getResponseHeaders().add("Content-Type", "application/problem+json"); if (!anyDenied) { LinkedHashSet attempted = new LinkedHashSet<>(); diff --git a/src/test/java/com/retailsvc/http/internal/ProblemDetailRendererTest.java b/src/test/java/com/retailsvc/http/internal/ProblemDetailRendererTest.java deleted file mode 100644 index 3d6d1d0..0000000 --- a/src/test/java/com/retailsvc/http/internal/ProblemDetailRendererTest.java +++ /dev/null @@ -1,29 +0,0 @@ -package com.retailsvc.http.internal; - -import static org.assertj.core.api.Assertions.assertThat; - -import com.retailsvc.http.validate.ValidationError; -import org.junit.jupiter.api.Test; - -class ProblemDetailRendererTest { - @Test - void rendersExpectedFields() { - String body = - ProblemDetailRenderer.render( - new ValidationError("/email", "format", "string does not match format 'email'", null)); - assertThat(body) - .contains("\"type\":\"about:blank\"") - .contains("\"title\":\"Bad Request\"") - .contains("\"status\":400") - .contains("\"pointer\":\"/email\"") - .contains("\"keyword\":\"format\"") - .contains("\"detail\":\"string does not match format 'email'\""); - } - - @Test - void escapesQuotesInDetail() { - String body = - ProblemDetailRenderer.render(new ValidationError("/x", "k", "has \"quotes\"", null)); - assertThat(body).contains("\"detail\":\"has \\\"quotes\\\"\""); - } -} diff --git a/src/test/java/com/retailsvc/http/internal/SecurityFilterTest.java b/src/test/java/com/retailsvc/http/internal/SecurityFilterTest.java index db83b2a..3b76f3e 100644 --- a/src/test/java/com/retailsvc/http/internal/SecurityFilterTest.java +++ b/src/test/java/com/retailsvc/http/internal/SecurityFilterTest.java @@ -11,6 +11,7 @@ import com.retailsvc.http.Request; import com.retailsvc.http.SchemeValidator; +import com.retailsvc.http.TypeMapper; import com.retailsvc.http.spec.HttpMethod; import com.retailsvc.http.spec.Operation; import com.retailsvc.http.spec.security.SecurityRequirement; @@ -23,6 +24,7 @@ import com.sun.net.httpserver.HttpExchange; import java.io.ByteArrayOutputStream; import java.net.URI; +import java.nio.charset.StandardCharsets; import java.util.List; import java.util.Map; import java.util.Optional; @@ -49,7 +51,8 @@ void allowsRequestWhenValidatorReturnsPrincipal() throws Exception { Map.of("bearerAuth", (req, cred) -> Optional.of("user-1")); SecurityFilter filter = - new SecurityFilter(Map.of("getX", op), schemes, List.of(), validators, false); + new SecurityFilter( + Map.of("getX", op), schemes, List.of(), validators, false, mockJsonMapper()); HttpExchange ex = mock(HttpExchange.class); Headers headers = new Headers(); @@ -88,7 +91,8 @@ void passesThroughWhenOperationHasNoSecurity() throws Exception { Optional.empty()); // inherits root, root is empty SecurityFilter filter = - new SecurityFilter(Map.of("getY", op), Map.of(), List.of(), Map.of(), false); + new SecurityFilter( + Map.of("getY", op), Map.of(), List.of(), Map.of(), false, mockJsonMapper()); HttpExchange ex = mock(HttpExchange.class); Chain chain = mock(Chain.class); @@ -117,7 +121,8 @@ void missingCredentialReturns401WithBearerChallenge() throws Exception { Map.of("bearerAuth", new HttpBearer(Optional.empty())), List.of(), Map.of("bearerAuth", (req, cred) -> Optional.of("never-called")), - false); + false, + mockJsonMapper()); HttpExchange ex = mock(HttpExchange.class); Headers headers = new Headers(); @@ -159,7 +164,8 @@ void deniedValidatorReturns403WithoutChallenge() throws Exception { Map.of("bearerAuth", new HttpBearer(Optional.empty())), List.of(), Map.of("bearerAuth", (req, cred) -> Optional.empty()), - false); + false, + mockJsonMapper()); HttpExchange ex = mock(HttpExchange.class); Headers headers = new Headers(); @@ -196,7 +202,8 @@ void apiKeyMissingReturnsApiKeyChallengeHeader() throws Exception { Map.of("apiKeyAuth", new ApiKey("X-API-Key", Location.HEADER)), List.of(), Map.of("apiKeyAuth", (req, cred) -> Optional.of("ok")), - false); + false, + mockJsonMapper()); HttpExchange ex = mock(HttpExchange.class); when(ex.getRequestHeaders()).thenReturn(new Headers()); @@ -241,7 +248,8 @@ void andGroupRequiresAllSchemesToSucceed() throws Exception { "bearerAuth", (req, cred) -> Optional.of("bearer-principal")); SecurityFilter filter = - new SecurityFilter(Map.of("getX", op), schemes, List.of(), validators, false); + new SecurityFilter( + Map.of("getX", op), schemes, List.of(), validators, false, mockJsonMapper()); HttpExchange ex = mock(HttpExchange.class); Headers headers = new Headers(); @@ -295,7 +303,8 @@ void orFallsBackToSecondGroupWhenFirstDenied() throws Exception { "bearerAuth", (req, cred) -> Optional.of("bearer-ok")); SecurityFilter filter = - new SecurityFilter(Map.of("getX", op), schemes, List.of(), validators, false); + new SecurityFilter( + Map.of("getX", op), schemes, List.of(), validators, false, mockJsonMapper()); HttpExchange ex = mock(HttpExchange.class); Headers headers = new Headers(); @@ -339,7 +348,8 @@ void externalAuthBypassesEverything() throws Exception { Map.of("bearerAuth", new HttpBearer(Optional.empty())), List.of(), Map.of(), // NO validators - /* externalAuth= */ true); + /* externalAuth= */ true, + mockJsonMapper()); HttpExchange ex = mock(HttpExchange.class); Chain chain = mock(Chain.class); @@ -351,4 +361,24 @@ void externalAuthBypassesEverything() throws Exception { private static Request newMinimalRequest(String operationId) { return new Request(new byte[0], null, null, operationId, Map.of(), null, h -> null); } + + private static TypeMapper mockJsonMapper() { + return new TypeMapper() { + @Override + public Object readFrom(byte[] body, String contentTypeHeader) { + return null; + } + + @Override + public byte[] writeTo(Object value) { + if (value instanceof ProblemDetail pd) { + return String.format( + "{\"type\":\"%s\",\"title\":\"%s\",\"status\":%d,\"detail\":\"%s\"}", + pd.type(), pd.title(), pd.status(), pd.detail()) + .getBytes(StandardCharsets.UTF_8); + } + return "{}".getBytes(StandardCharsets.UTF_8); + } + }; + } } From e25ae9570fca2daa2c614eb363f1f2c9f59b90a5 Mon Sep 17 00:00:00 2001 From: Thomas Cederholm Date: Wed, 20 May 2026 13:29:45 +0200 Subject: [PATCH 11/11] test: Lock .yml extension support for spec loading and content-type --- .../ClasspathResourceHandlerTest.java | 6 + .../retailsvc/http/spec/SpecFromPathTest.java | 10 + src/test/resources/openapi.yml | 410 ++++++++++++++++++ 3 files changed, 426 insertions(+) create mode 100644 src/test/resources/openapi.yml diff --git a/src/test/java/com/retailsvc/http/internal/ClasspathResourceHandlerTest.java b/src/test/java/com/retailsvc/http/internal/ClasspathResourceHandlerTest.java index 01b4119..ad80e77 100644 --- a/src/test/java/com/retailsvc/http/internal/ClasspathResourceHandlerTest.java +++ b/src/test/java/com/retailsvc/http/internal/ClasspathResourceHandlerTest.java @@ -37,6 +37,12 @@ void infersApplicationYamlForYamlExtension() { .isEqualTo("application/yaml"); } + @Test + void infersApplicationYamlForYmlExtension() { + assertThat(new ClasspathResourceHandler("/openapi.yml").contentType()) + .isEqualTo("application/yaml"); + } + @Test void infersTextPlainForTxtExtension() { assertThat(new ClasspathResourceHandler("/sample.txt").contentType()) diff --git a/src/test/java/com/retailsvc/http/spec/SpecFromPathTest.java b/src/test/java/com/retailsvc/http/spec/SpecFromPathTest.java index 4542877..17e9303 100644 --- a/src/test/java/com/retailsvc/http/spec/SpecFromPathTest.java +++ b/src/test/java/com/retailsvc/http/spec/SpecFromPathTest.java @@ -31,6 +31,16 @@ void loadsYamlSpecViaSnakeYaml() throws Exception { assertThat(spec.operations()).isNotEmpty(); } + @Test + void loadsYmlSpecViaSnakeYaml() throws Exception { + Path resource = Path.of(getClass().getResource("/openapi.yml").toURI()); + + Spec spec = Spec.fromPath(resource); + + assertThat(spec.openapi()).startsWith("3.1"); + assertThat(spec.operations()).isNotEmpty(); + } + @Test void rejectsUnknownExtension(@TempDir Path tmp) throws Exception { Path unknown = tmp.resolve("spec.txt"); diff --git a/src/test/resources/openapi.yml b/src/test/resources/openapi.yml new file mode 100644 index 0000000..fd274a5 --- /dev/null +++ b/src/test/resources/openapi.yml @@ -0,0 +1,410 @@ +openapi: 3.1.0 + +info: + title: API Title + version: 0.0.1-local + +servers: + - url: http://localhost:8080/api/v1 + +paths: + /data: + get: + operationId: get-data + parameters: + - in: header + name: correlation-id + schema: + type: string + format: uuid + - $ref: '#/components/parameters/Name-Header' + responses: + "200": + description: OK + content: + application/json: + schema: + $ref: '#/components/schemas/GetDataResponse' + + post: + operationId: post-data + x-permissions: + - pro.promotion.create + requestBody: + content: + application/json: + schema: + $ref: '#/components/schemas/PostDataRequest' + responses: + "200": + description: OK + content: + application/json: + schema: + $ref: '#/components/schemas/PostDataResponse' + + /list/objects: + post: + operationId: post-list-objects + requestBody: + content: + application/json: + schema: + type: array + items: + $ref: '#/components/schemas/ListItem' + responses: + "200": + description: OK + + /params/query: + get: + operationId: query-params + parameters: + - in: query + name: q1 + schema: + type: string + - in: query + name: q2 + required: true + schema: + type: string + responses: + "200": + description: OK + + /params/path/{ID}: + get: + operationId: path-params + parameters: + - in: path + name: ID + required: true + schema: + type: string + responses: + "200": + description: OK + + /params/path/{ID}/{Name}/{Surname}: + get: + operationId: path-params-multi + parameters: + - in: path + name: ID + required: true + schema: + type: string + - in: path + name: Name + required: true + schema: + type: string + pattern: '[A-Za-z]+' + - in: path + name: Surname + required: true + schema: + type: string + responses: + "200": + description: OK + + /shapes: + post: + operationId: post-shape + requestBody: + required: true + content: + application/json: + schema: + oneOf: + - type: object + required: [kind, radius] + properties: + kind: + type: string + enum: [circle] + radius: + type: number + - type: object + required: [kind, side] + properties: + kind: + type: string + enum: [square] + side: + type: number + responses: + "200": + description: OK + + /filters: + post: + operationId: post-filter + requestBody: + required: true + content: + application/json: + schema: + type: object + required: [value] + properties: + value: + anyOf: + - type: string + minLength: 3 + - type: integer + responses: + "200": + description: OK + + /blocked: + post: + operationId: post-blocked + requestBody: + required: true + content: + application/json: + schema: + type: object + required: [token] + properties: + token: + type: string + not: + enum: [forbidden] + responses: + "200": + description: OK + + /format/email: + get: + operationId: format-email + parameters: + - in: query + name: addr + required: true + schema: + type: string + format: email + responses: + "200": + description: OK + + /format/int32: + get: + operationId: format-int32 + parameters: + - in: query + name: n + required: true + schema: + type: integer + format: int32 + responses: + "200": + description: OK + + /format/byte: + get: + operationId: format-byte + parameters: + - in: query + name: data + required: true + schema: + type: string + format: byte + responses: + "200": + description: OK + + /gates: + post: + operationId: post-gate + requestBody: + required: true + content: + application/json: + schema: + type: object + required: [open] + properties: + open: true + blocked: false + responses: + "200": + description: OK + /form-echo: + post: + operationId: form-echo + requestBody: + required: true + content: + application/x-www-form-urlencoded: + schema: + type: object + properties: + name: { type: string } + age: { type: integer } + tags: + type: array + items: { type: string } + responses: + "200": { description: ok } + /text-echo: + post: + operationId: text-echo + requestBody: + required: true + content: + text/plain: + schema: { type: string } + responses: + "200": { description: ok } + + /secure/api-key: + get: + operationId: secureApiKey + security: + - apiKeyAuth: [] + responses: + "200": + description: ok + + /secure/bearer: + get: + operationId: secureBearer + security: + - bearerAuth: [] + responses: + "200": + description: ok + + /secure/basic: + get: + operationId: secureBasic + security: + - basicAuth: [] + responses: + "200": + description: ok + + /secure/open: + get: + operationId: secureOpen + security: [] + responses: + "200": + description: ok + +components: + securitySchemes: + apiKeyAuth: + type: apiKey + name: X-API-Key + in: header + bearerAuth: + type: http + scheme: bearer + basicAuth: + type: http + scheme: basic + + parameters: + Name-Header: + in: header + name: X-Name + schema: + type: string + pattern: '^[Aa].+' + + schemas: + GetDataResponse: + type: object + properties: + id: + type: integer + + NestedObject: + type: object + required: + - nestedValue + properties: + nestedValue: + type: integer + format: int32 + + InnerObject: + type: object + required: + - id + - age + properties: + id: + type: string + age: + type: integer + format: int32 + longNumber: + type: integer + format: int64 + maximum: 1000 + minimum: 100 + nested: + $ref: '#/components/schemas/NestedObject' + + ListItem: + type: object + properties: + value: + type: integer + + PostDataRequest: + type: object + required: + - aList + - feelingGood + properties: + id: + type: string + pattern: '^s.+d$' + age: + type: integer + score: + type: number + format: double + random: + type: string + format: uuid + status: + type: string + enum: + - COMPLETED + - ERROR + feelingGood: + type: boolean + aList: + type: array + items: + type: string + aListOfObjects: + type: array + items: + $ref: '#/components/schemas/ListItem' + anObject: + $ref: '#/components/schemas/InnerObject' + aDate: + type: string + format: date + aDateTime: + type: string + format: date-time + + PostDataResponse: + type: object + properties: + id: + type: string