[http-client] Add client filter scopes#1662
Conversation
| HttpResponse(HttpStatus.SwitchingProtocols).addField("body", result) | ||
| } | ||
| }.asInstanceOf[HttpResponse["body" ~ A] < (Async & Abort[HttpException | HttpResponse.Halt])] | ||
| ).map(_.fields.body).asInstanceOf[A < (S & Async & Abort[HttpException])] |
There was a problem hiding this comment.
why are these casts necessary? are they safe?
There was a problem hiding this comment.
Necessary given the current HttpFilter.Passthrough shape, and safe. The continuation type is fixed: next: HttpRequest[In] => HttpResponse[Out] < (Async & Abort[E2 | Halt]). It has no slot for the WebSocket user-handler effect row S (the WS callback is f: HttpWebSocket => A < S with arbitrary S), so to thread the session through a filter we erase S to match the continuation type and restore it after. I pulled that into two named helpers (eraseWebSocketUserEffects / handleWebSocketFilterResult) with comments so the intent is explicit.
Why it is safe: filters only forward the continuation result; they never inspect or handle S, and the suspended S effects are preserved unchanged at the runtime level across the erase/restore. This mirrors the existing non-WS pattern in poolWith (from #1518), which does the same erase-into-continuation + restore-result cast. The WS path is actually stricter on Halt: instead of casting Halt away like the non-WS path, it runs Abort.run[Halt] and converts a filter Halt into an HttpStatusException.
One residual assumption worth naming: this is sound as long as S is disjoint from what filters handle (Async, Abort[Halt]). The only realistic collision would be a user S containing Abort[HttpResponse.Halt], which is an http-internal type and not something a WS handler row would carry.
If you would prefer the casts gone entirely, the clean route is to scope the filter to the upgrade handshake only and serve the session outside the filter (where S flows naturally). That needs splitting WebSocketCodec.requestUpgradeWith so the handshake and the f(wsStream) session are no longer in one resource bracket. Happy to do that as a follow-up if you want it.
There was a problem hiding this comment.
I don't think it's safe. Can you introduce tests that use effects like Var in the provided function? It seems we need an S type param in HttpFilter.apply or restrict the provided function to not have free S effects
|
|
||
| /** Applies multiple client filters for all `HttpClient` calls within the given computation. */ | ||
| def withFilters[A, S](filters: Seq[HttpFilter.Passthrough[Nothing]])(v: A < S)(using Frame): A < S = | ||
| withFilter(filters.foldLeft(HttpFilter.noop: HttpFilter.Passthrough[Nothing])(_.andThen(_)))(v) |
There was a problem hiding this comment.
Reasoning about the filters running in a computation becomes more difficult with the new features. Can we have methods to inspect the current enabled filters + to disable them in nested scopes?
There was a problem hiding this comment.
Added both. Inspect: HttpClient.useConfig(config => ...) and HttpClient.useFilter(filter => ...) expose the active config and composed client filter. Disable in nested scope: HttpClient.withoutFilters { ... } (backed by HttpClientConfig.clearFilters) clears the config/scoped client filters for that computation while leaving ServiceLoader and route filters in place. Tests cover the nested-disable + outer-scope-restore case.
One limitation to flag: useFilter/withoutFilters only see and clear the programmatic config.clientFilter; the ServiceLoader-discovered filters (Factory.composedClient) are not included, so a caller cannot inspect or fully disable those. Happy to widen the scope if you want those covered too.
There was a problem hiding this comment.
Followed up on the ServiceLoader-filter limitation I flagged above with a read-only blast-radius exploration. Sketch of how we could cover the auto-discovered filters too, if you want it:
Proposal (additive, non-breaking): add disableAutoFilters: Boolean = false to HttpClientConfig, plus a dedicated HttpClient.useAutoFilter accessor (exposes the composed Factory.composedClient) and a HttpClient.withoutAutoFilters { ... } scope. Backend composition becomes (if config.disableAutoFilters then noop else Factory.composedClient).andThen(config.clientFilter) at the two client sites (poolWith, runWsSessionWith). This keeps Factory.composedClient as a memoized lazy val and preserves the eq noop fast path exactly.
Why not just fold the auto filters into config.clientFilter: it would force ServiceLoader discovery at every HttpClientConfig() construction (including the Local.init default and every doctest), defeat the lazy-val memoization, and break the no-filter hot path.
Footgun to avoid: I would NOT make the existing withoutFilters clear auto filters. Today the only auto client filter is OTLP W3C trace-context propagation; having withoutFilters silently drop it would break distributed traces with no compile error. Hence a separate, explicit withoutAutoFilters rather than overloading withoutFilters.
Open question for you: scope. Do you want server-side parity in the same change? The server path composes composedServer.andThen(route.filter) in HttpHandler, which would have the same inspect/disable gap. Happy to do client-only now and server as a follow-up, or both together. Size is roughly M client-only (field + conditional + accessor + scope + tests + README/scaladoc), M/L if server parity is included.
Can leave the current limitation as-is for this PR and land the auto-filter support separately, or fold it in here. Your call.
There was a problem hiding this comment.
I like the proposal to handle auto filters separately. We should also document the terminology and behavior. About parity, it's always ideal.
8fd0e67 to
e8194f6
Compare
Summary
This PR adds programmatic and scoped client filter configuration for
kyo-http, building on the existingHttpFiltermodel and the existingHttpFilter.FactorySPI.The goal is to make cross-cutting client concerns first-class without requiring every typed route or convenience call to repeat the same filter setup. Supported scenarios include auth headers, request IDs, logging, tracing, metrics, and other request/response middleware that should apply consistently across outgoing HTTP requests.
Closes #1660
What changed
clientFiltertoHttpClientConfig, defaulting toHttpFilter.noop.HttpClientConfig.filter(...)andHttpClientConfig.filters(...)builder methods that append filters in order.HttpClient.withFilter(...)andHttpClient.withFilters(...)for dynamic, scoped filter configuration.HttpFilter.FactoryServiceLoader filtersHttpClientConfigfiltersHttpClient.withFilterfiltersHost,Upgrade,Connection,Sec-WebSocket-Version, andSec-WebSocket-Key.kyo-httpREADME with the new client filter configuration options and composition order.Why
kyo-httpalready has a solid filter abstraction and an SPI path throughHttpFilter.Factory, but the client side had two important gaps:This PR keeps the existing route-level filter support, keeps SPI support for library-provided filters such as tracing, and adds config plus scoped layers for application-controlled middleware.
Behavior notes
HttpClientConfigapply to outgoing HTTP requests and WebSocket upgrade handshakes.HttpClient.withFilterorHttpClient.withFiltersblock.HttpClientConfigremains a case class. Its equality now includes theclientFilterfield by reference, similar to the existing function-valuedretryOnfield.Pros
HttpFilter.Passthroughabstraction instead of introducing a separate middleware type.Tradeoffs
HttpClientConfigequality now includes the filter reference, so two configs with behaviorally equivalent but separately allocated filters will not compare equal.Validation
sbt 'kyo-http/doctest': passed, 64 doctest blocks, 0 failuressbt 'kyo-http/test': passed, 2,184 tests, 0 failed, 30 pendinggit diff --check: passedRelated issue