[improvement][undertow] Add EndpointHandlerWrapper for filter-like features

[rubenfiszel]

Changes in this PR

This PR enables features that are similar to the concept of Jersey filters. Undertow is meant to be used by chaining HttpHandlers. The first HttpHandler applies logic and then pass it to the next handler. With this structure, handlers always have control over the control flow and can apply logic, before and after processing by the next handlers. This is similar to the desired behavior of jersey Filters.

After that PR, it is possible add arbitrary behaviours pre and post processing of the underlying resource by leveraging a concept similar to Undertow's HandlerWrapper: the EndpointHandlerWrapper. It wraps the handler of an endpoint, taking potentially advantage of all informations contained in an Endpoint.

Below an example of an EndpointHandlerWrapper with pre and post processing.

EndpointHandlerWrapper myHandlerWrapper = 
    endpoint -> 
      if (shouldWrap) {
        Optional.of(exchange -> {
          preProcessingLogic(endpoint, exchange);
          endpoint.handler().handleRequest(exchange);
          postLogic(endpoint, exchange)
          exchange.addCompletionListener(postCompletionLogic(endpoint))})
      } else {
        Optional.empty()
      }

• added EndpointHandlerWrapper as a functional interface of the form HttpHandler wrap(Endpoint endpoint). Warning EndpointHandlerWrappers are meant to be used only by library writers and webserver maintainers.
• added addEndpointHandlerWrapperBeforeBlocking(EndpointHandlerWrapper wrapper) to the ConjureHandler class to give the possibility of adding non-blocking EndpointHandlerWrapper to all registered UndertowServices.

The utilities to easily wrap an UndertowService with an EndpointHandlerWrapper have been purposefully not included as this is a behavior that we do not want to encourage.
One can still use the following snipper to achieve this behavior.

    public static UndertowService map(UndertowService service, EndpointHandlerWrapper wrapper) {
        return runtime ->
                ImmutableList.copyOf(
                        service.endpoints(runtime).stream()
                                .map(endpoint -> Endpoints.map(endpoint, wrapper))
                                .collect(ImmutableList.toImmutableList()));
    }

Possible downsides?

None

[rubenfiszel]

@carterkozak I probably have to add tests but what do you think of the refactor?

[carterkozak]

Needs tests for Endpoints and UndertowServices utility functions, especially since UndertowServices isn't otherwise used.

[carterkozak]

ConjureHandler is meant to be immutable, configuration should occur on the builder

Maybe call the builder method initialNonBlockingEndpointHandlerWrappers(EndpointHandlerWrapper) with strongly worded documentation warning against performing any blocking operations in this wrapper. We should also call out the order we expect them to be invoked in.

[rubenfiszel]

Added doc, but kept the name.

[markelliot]

would it be possible to get a top-line description of why we want this functionality, either on the PR, in docs or in code?

[rubenfiszel]

@markelliot updated the title and description

[markelliot]

@rubenfiszel thanks! The description sounds pretty great, but isn't quite what the code gets us right now. Could we do something like this?

interface EndpointHandlerWrapper<C> {
    C beforeExecution(Endpoint endpoint, HttpServerExchange exchange);
    C afterExecution(C context, Endpoint endpoint, HttpServerExchange exchange);
    C completion(C context, Endpoint endpoint);
}

Somewhat inspired by https://github.com/palantir/tritium/blob/develop/tritium-api/src/main/java/com/palantir/tritium/event/InvocationEventHandler.java.

The idea is that it makes it impossible to mess up/not call the original handler, still gives options to code an aspect.

If we're after filters and want the ability to stop processing, may want to have something that's more explicitly a predicate boolean shouldExecute(...).

[carterkozak]

@markelliot I'm somewhat skeptical of that approach because it doesn't allow us to try/finally around execution, which can lead to leaks.

While I agree that it may be cleaner for some functionality, it has drawbacks for others. I want to avoid over-engineering a new api unless it's a strict improvement or would solve problems we have encountered.

A few benefits of the current proposal:

1. It works similarly to other undertow constructs, allowing for simple integration with existing components.
2. We can choose to selectively wrap endpoints, leaving others completely unmodified.
3. The endpoint is provided to the wrapper, not necessarily to each request, this way implements are pushed to do setup once on initialization, not for every request.

Given that, I would propose that we build the aop extension in a way that it can produce a EndpointHandlerWrapper (HttpHandler wrap(Endpoint)). Our WC API may expose only the aop approach for witchcraft consumers to filter on, while leaving itself the more expressive and potentially more dangerous wrapper api.

[markelliot]

The try/finally semantics are exactly why one would do this — and it’s otherwise not possible to guarantee that all implementers will correctly perform that kind of wrapping unless we’re a little more structured.

The current proposal also seems way more likely to leak things like Endpoint (my sample was directional, drop the args you don’t want to have), and things like selective application can easily be solved with an additional predicate.

I’m not a fan of the big hammer of being able to wholesale replace an Endpoint, and don’t think this is the appropriate way to allow developers to add cross-cut functionality.

[rubenfiszel]

@markelliot I share the concerns of @carterkozak .The experience we have had with WC is that the Undertow model of HttpHandler/HandlerWrapper is pleasant to write in, concise/readable and give us sufficient flexibility to achieve a diverse set of features so far without having us to go through any escape hatch.

One of the goal of having your proposed interface is to make it "impossible to mess up/not call the original handler". However, in some cases, the control that you would want to apply is precisely to not call the original handler.

That being said, I agree that onSuccess, onFailure is a better try/catch replacement and that your proposal bring more safety and structure. I think in an ideal world, what we would want is to be able to use the PR's EndpointHandlerWrapper for WC's internals and core libraries, but to force other final consumers to go through your proposed interface of EndpointHandlerWrapper if they desire to implement their own filter-like logic.

It is possible to achieve some of this desired goal by taking advantage of the fact that your proposed EndpointHandlerWrapper can extend the PR's one and that we can strongly recommend final consumers to extend your more structured interface when implementing their custom filter logic.

[markelliot]

Okay, perhaps we could make clear the possible downsides of the approach proposed here:

• Implementors may fail to correctly call the delegate handler (docs presently fail to make this clear)
• Implementors will likely assume that the delegate handler will never throw
• Implementations of wrappers can wholesale replace the existing HttpHandler, and implementors can abuse this to modify the behavior of Conjure endpoints arbitrarily
• Implementations of wrappers are not guided to separate initialization logic from per-endpoint logic (docs presently fail to make this clear)
• Implementors of wrappers may not consider whether their implementation should always wrap or sometimes wrap (not only do the docs presently fail to make this clear, but @rubenfiszel recently submitted an internal proposal which failed to do this)
• We're exposing a very general purpose override, and once exposed, we will not be able to remove this override easily, even if it is used poorly or abusively

[rubenfiszel]

@markelliot @carterkozak @iamdanfox
Updated the PR and PR description, changed the EndpointHandlerWrapper to Endpoint -> HttpHandler. Removed the utilities from being usable by users. Made the doc more explicit about the use and dangers of EndpointHandlerWrapper.
Ready for re-review.

[carterkozak]

Mind adding a test for this?

[carterkozak]

Pushed commit 00820bb
Thoughts on that? I like having all of the wrappers in a single place.

[rubenfiszel]

@carterkozak LGTM. I did a small aesthetic change of using add(E ...)

[rubenfiszel]

I'm just worried that it's less clear in this structure that the pre-defined ConjureHandler EndpointHandlerWrappers are consts.

[carterkozak]

👍