|
| 1 | +# Decorator runs inside interceptor ScopedValue scope |
| 2 | + |
| 3 | +## Problem |
| 4 | + |
| 5 | +`README.md` ("Combining the two") promises that a `ResponseDecorator` |
| 6 | +sees `ScopedValue` bindings established by a `RequestInterceptor`: |
| 7 | + |
| 8 | +> Decorators run inside the interceptor's `ScopedValue` binding (the |
| 9 | +> decorator transforms the `Response` returned by `next.proceed()`, |
| 10 | +> which is still on the call stack), so `CORRELATION_ID.get()` / |
| 11 | +> `TENANT_ID.get()` see the bound values. |
| 12 | +
|
| 13 | +The implementation contradicts this contract. |
| 14 | +`DispatchHandler.handle` runs the entire interceptor chain to |
| 15 | +completion, then loops decorators after the chain has unwound: |
| 16 | + |
| 17 | +```java |
| 18 | +Response response = invoke(0, request, handler); // pops all |
| 19 | + // ScopedValue |
| 20 | + // frames |
| 21 | +for (ResponseDecorator decorator : decorators) { |
| 22 | + response = decorator.decorate(request, response); // no bindings |
| 23 | +} |
| 24 | +``` |
| 25 | + |
| 26 | +A decorator that calls `CORRELATION_ID.get()` throws |
| 27 | +`NoSuchElementException`. |
| 28 | + |
| 29 | +## Fix |
| 30 | + |
| 31 | +Move the decorator loop into the base case of `invoke()`, so |
| 32 | +decorators run after `handler.handle(request)` and before the call |
| 33 | +unwinds through the interceptor frames. |
| 34 | + |
| 35 | +`DispatchHandler.handle`: |
| 36 | + |
| 37 | +```java |
| 38 | +public void handle(HttpExchange exchange) { |
| 39 | + Request request = CURRENT.get(); |
| 40 | + RequestHandler handler = handlers.get(request.operationId()); |
| 41 | + Response response = invoke(0, request, handler); |
| 42 | + exchange.setAttribute(RESPONSE_ATTR, response); |
| 43 | + renderer.render(exchange, response); |
| 44 | +} |
| 45 | + |
| 46 | +private Response invoke(int idx, Request req, RequestHandler h) { |
| 47 | + if (idx == interceptors.size()) { |
| 48 | + Response response = h.handle(req); |
| 49 | + for (ResponseDecorator d : decorators) { |
| 50 | + response = d.decorate(req, response); |
| 51 | + } |
| 52 | + return response; |
| 53 | + } |
| 54 | + return interceptors.get(idx) |
| 55 | + .around(req, () -> invoke(idx + 1, req, h)); |
| 56 | +} |
| 57 | +``` |
| 58 | + |
| 59 | +## Consequences |
| 60 | + |
| 61 | +- Decorators see all interceptor `ScopedValue` bindings. README |
| 62 | + contract honored. |
| 63 | +- Interceptors observe the decorated `Response` on the way back up. |
| 64 | + An interceptor wrapping `next.proceed()` in `try`/log can record |
| 65 | + the final status, headers, and body produced by the handler + |
| 66 | + decorators. README already implies this. |
| 67 | +- If a decorator throws, the exception propagates through the |
| 68 | + interceptor chain. Previously it skipped interceptors and went |
| 69 | + straight to `ExceptionFilter`. Interceptors that wrap |
| 70 | + `next.proceed()` in `try`/`catch` now observe decorator failures. |
| 71 | + Worth a release note. |
| 72 | + |
| 73 | +## Out of scope |
| 74 | + |
| 75 | +`AfterResponseHook` runs from `RequestPreparationFilter` after the |
| 76 | +interceptor chain has unwound and therefore does not see interceptor |
| 77 | +`ScopedValue` bindings. That is a separate gap with a different |
| 78 | +fix (the hook would need to fire from inside the interceptor frame, |
| 79 | +which changes the documented "interceptor wraps the handler" |
| 80 | +contract). Users who need per-request context in an after-callback |
| 81 | +can register a closure via `request.afterResponse(Runnable)` from |
| 82 | +inside the interceptor — the runnable captures the resolved values. |
| 83 | + |
| 84 | +## Tests |
| 85 | + |
| 86 | +Add to `DispatchHandlerTest` (or create one): |
| 87 | + |
| 88 | +1. **Decorator sees interceptor-bound `ScopedValue`.** Register an interceptor that binds a `ScopedValue<String>` and a decorator that reads it and stamps a header. Assert the header is present on the rendered response. Without the fix this throws `NoSuchElementException`. |
| 89 | +2. **Interceptor observes decorated response.** Register an interceptor that captures the `Response` returned by `next.proceed()`. Register a decorator that adds a header. Assert the captured response carries the decorator-added header. |
| 90 | +3. **Decorator failure is visible to interceptors.** Register an interceptor with `try`/`catch` around `next.proceed()` and a decorator that throws. Assert the interceptor's catch block ran. |
| 91 | + |
| 92 | +## Files touched |
| 93 | + |
| 94 | +- `src/main/java/com/retailsvc/http/internal/DispatchHandler.java` |
| 95 | +- `src/test/java/com/retailsvc/http/internal/DispatchHandlerTest.java` |
| 96 | + (extend existing or add) |
| 97 | + |
| 98 | +## README |
| 99 | + |
| 100 | +No change. The current wording in "Combining the two" is correct; |
| 101 | +the implementation is being brought into compliance. |
0 commit comments