test: ResponseChangingFilterSpec #3983

[lightoze]

This is an attempt to add a comprehensive test for filter-controller interaction, more specifically response modification. It uncovers a few more problems in addition to #3983 and represents my understanding of how filters should work. If some of the cases work as desired or aren't supported, it might be worth documenting them.

[jameskleeh]

@lightoze Can you merge 2.2.x into your branch?

[lightoze]

@jameskleeh Rebase done. Most of the failures are due to the fact that RoutingInBoundHandler assumes that response processing mode (in terms of publisher/stream handling) should be always as defined for the route, but when HttpServerFilter replaces or modifies the response this cannot be true. Imagine some streaming response controller being intercepted by authentication filter and returning static login page instead, or exception handling filter kicking in and responding with JSON body instead of initially expected byte chunk stream. More extreme case would be if static controller response is being replaced with a stream by a filter.

SessionCreationSpec, on the other hand, is currently processed so that RoutingInBoundHandler.buildResultEmitter returns Flowable(MutableHttpResponse(body=Flowable(HttpResponse.ok()))) which should not happen, and it breaks things.

[lightoze]

I'm very glad to see this is being taken into account for 3.0. I fixed compilation error because of moved @Inject class so the test is usable again.

I have also marked Publisher<HttpResponse> reactive() controller method as a single result and it improved test outcome. It is quite counter-intuitive, though, that you have to do that. Publisher<HttpResponse> seem to always imply a single result, and even if you attempt to return multiple responses only the first one is used.

All the issues seem to arise from how the response publisher processing mode is being derived from the controller method, even when the response is being entirely replaced by a filter. With 3.0, have you considered moving these controls to MutableHttpResponse, with defaults deriving from controller signature before the filter chain is invoked? Basically, copy all necessary info into the response object so that route/controller metadata is not used at later response encoding anymore.

[jameskleeh]

Publisher<HttpResponse> seem to always imply a single result

Not for HTTP/2

have you considered moving these controls to MutableHttpResponse, with defaults deriving from controller signature before the filter chain is invoked? Basically, copy all necessary info into the response object so that route/controller metadata is not used at later response encoding anymore.

Can you give an example of what info you're referring to?

[lightoze]

Not for HTTP/2

Good point )

Can you give an example of what info you're referring to?

Prime example is to create a "single vs streaming result" attribute for MutableHttpResponse to replace current "isSingle" logic.

The current processing chain looks like the following:

• Call the controller method and wrap result into MutableHttpResponse (createExecuteRoutePublisher, emitRouteResponse methods)
• Apply filter chain (filterPublisher method)
• If body is reactive type, do some processing (executeRoute method):
  • When route is marked as "single", replaces response body with such single value
  • As a special case, when body is HttpResponse, replaces the whole response discarding outer response object entirely. This is the reason why e.g. session creation does not work on Publisher<HitpResponse> reactive() method without single = true - cookie headers are being added to outer response object and then discarded.
  • For streaming (not "single") responses it serializes each chunk to HttpContent object.
• Resulting response is sent to the client (handleRouteMatch, encodeHttpResponse methods)

What I was suggesting is to:

• Call the controller method and wrap result into MutableHttpResponse. Handle Publisher<HttpResponse> case here, so it is not possible to end up with MutableHttpResponse<Publisher<HttpResponse>>.
• Apply route/controller metadata to the response object, unless it is already explicitly set. If body is reactive type, it will be marked as "single" if necessary, response media type will be applied, etc. MutableHttpResponse API can be expanded with methods like bodySingle(boolean) to set single body marker and bodySingle(Publisher<?>) to set reactive body alongside with the marker. I would personally prefer to assume single body by default in this API and call bodyStream(Publisher<?>) when I want the opposite. Having streaming response content type would also default it to expect multiple results from a publisher, of course. But changing defaults may be too much of a breaking change, so whatever.
• Apply fitter chain
• Do the rest of processing as before, but instead of RouteMatch object all necessary information should already be in MutableHttpResponse object

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

:x: lightoze
:x: jameskleeh
_{You have signed the CLA already but the status is still pending? Let us recheck it.}