Provide simple way to create ClientResponse [SPR-16553]

[spring-projects-issues]

Rob Winch opened SPR-16553 and commented

When working with an ExchangeFilterFunction it expects that a Mono<ClientResponse> is provided as a return type. Similar to ClientRequest.Builder for ClientRequest, it would be nice if there were a simple way to create a ClientResponse. This would make it easier to use the ExchangeFilterFunction API to provide custom ClientResponse. For example, consider an ExchangeFilterFunction that provides a default value on an error:

ClientResponse fallbackResponse = null; // no builder for this
ExchangeFilterFunction fallback = (r,n) -> n
		.exchange(r)
		.onErrorResume(Exception.class, e -> Mono.just(fallbackResponse));

A few use cases that would be valuable to support are:

• Creating a ClientResponse from a WebClient.ResponseSpec
• Creating a ClientResponse from an existing ClientResponse

---

Affects: 5.0.4

Issue Links:

• Provide simple way to create ServerRequest [SPR-16706] #21247 Provide simple way to create ServerRequest

Referenced from: commits 04c2a29

0 votes, 5 watchers

[spring-projects-issues]

Arjen Poutsma commented

Can you elaborate what you mean with "Creating a ClientResponse from a WebClient.ResponseSpec"?

ResponseSpec is just a convenient grouping of methods that bypasses ClientResponse to get access to the response body; it is not meant as an object that is to be passed around.

[spring-projects-issues]

Arjen Poutsma commented

robwinch, Rossen Stoyanchev, Josh Long, can you take a look at this branch and let me know what you think? It's still a WIP, but I wanted to see if my current approach fits the bill before I continue to work on it.

There are two main classes to look at:

• ClientResponseWrapper, which is aimed at wrapping an existing response and overriding one or two properties to change the wrapped delegate.
• ClientResponseBuilder, which is aimed at building a response from scratch.

In ClientResponseBuilder, you specify the body by registering a Publisher that is to be returned from ClientResponse.bodyToXXX. This approach seemed the easiest way to set a response body without having to deal with data buffers (and potential memory leaks). It does, however, mean that the built response can only handle types that are assignable from the ones that were registered in the builder (for instance: if you register a CharSequence body in the builder, you can get a String out of it, but you cannot get a byte[]). The alternative to this approach would be for users have to set a Flux<DataBuffer> in the builder and extract the desired type in bodyToXXX using a body extractor, similar to MockClientHttpResponse. Thoughts? (see my comment below)

Note that ClientResponseBuilder lives in the support package (next to ClientResponseWrapper, and is not created through a static method of ClientResponse, but through a normal constructor (unlike ClientRequest). The reason for this is that building a response from scratch is an advanced use case that is typically only done in filters. Adding a static method to ClientResponse add more noise to that type, and cause potential confusion.

[spring-projects-issues]

Arjen Poutsma commented

After giving it some more thought, the approach for setting bodies in ClientResponseBuilder described above is never going to work, as it assumes that all consumers of the built response are known upfront. This would rule out, for instance, a general purpose filter, for instance in one of the portfolio projects, that uses the builder.

So I guess it is going to have to be data buffer based after all, much like MockClientResponse. I'll give that a shot tomorrow.

[spring-projects-issues]

Rossen Stoyanchev commented

I see your point about ClientRequest vs ClientResponse but in practice, since requests are built through the WebClient, the most likely case for ClientRequest.Builder is still the advanced case of filters. This is why we scaled back on the options in the builder at some point. By contrast, the response only needs one static factory method and that alone should be a good differentiation + perhaps the name could be something more explicit like mutate? Given that an ExchangeFilterFunction may work with both, I think the symmetry would be highly desirable.

About potential memory leaks, I can't quite see what a good approach could be? If an alternative body is specified, how to ensure the original one is consumed and released?

[spring-projects-issues]

Rob Winch commented

Can you elaborate what you mean with "Creating a ClientResponse from a WebClient.ResponseSpec

My thought was that it might be nice if a ClientResponse needed to be enriched using a WebClient to get a ResponseSpec. Given the builder is pretty simple and that users can already process multiple ClientRequest I don't think this is really necessary though. What's more is we could easily add a method later if we felt it was necessary (probably isn't).

I second Rossen Stoyanchev sentiment about having factory methods on ClientResponse. They seem very related and I think the symmetry would be ideal. Rather than a method named mutate I'd suggest that we should use Builder ClientResponse from(ClientResponse other) which aligns with ClientRequest.

With that in mind I'm not sure if we really need to have a ClientResponseWrapper. Are there advantages to this vs a builder method that initializes the state from an existing ClientResponse? I'd personally prefer from(ClientResponse other since I do not need to extend a class and I can interact with it in the same way I do that I create any other ClientResponse.

[spring-projects-issues]

Arjen Poutsma commented

Here is my second attempt: https://github.com/poutsma/spring-framework/tree/SPR-16553.

I've left in the ClientResponseWrapper. I can see that it's not as useful as the builder, but we have the ServerRequestWrapper on the server side, so this is mostly for symmetry and consistency than anything. And it is in a separate support package, so it doesn't get in the way.

The builder is now part of ClientResponse, see https://github.com/poutsma/spring-framework/blob/3f06a3eabeaaf11bb4e618f9fd9b6906e6a42224/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/ClientResponse.java#L239

You can create a response from an existing response (using ClientResponse.from(ClientResponse)), or from scratch, with just a status code an a list of message readers. I thought about using the default message readers by default, but shunned away from that as it would have unexpected result when they have been customised.

If you change the body of the response in the builder, the old response will be released.

I think this satisfies everybody's demands, but let me know if I missed anything.

[spring-projects-issues]

Rob Winch commented

Thanks for the updates.

What is the proper way of creating a ClientResponse.create(HttpStatus, ExchangeStrategies) within a ExchangeFilterFunction? From that place in the code, there is no way I can see to obtain a reference to an existing DataBufferFactory. Is it safe to just create a new DefaultDataBufferFactory()? For example, I have the following:

Flux<DataBuffer> fallbackBody = Flux.just("Default")
			.map(s -> s.getBytes(StandardCharsets.UTF_8))
			.map(new DefaultDataBufferFactory()::wrap);

ClientResponse fallbackResponse = ClientResponse.create(HttpStatus.OK, ExchangeStrategies.withDefaults())
		.body(fallbackBody)
		.build();

ExchangeFilterFunction fallback = (r,n) -> n
		.exchange(r)
		.onErrorResume(Exception.class, e -> Mono.just(fallbackResponse));

[spring-projects-issues]

Arjen Poutsma commented

Yes, using a heap-based DefaultDataBufferFactory for bodies in a fallback response is fine. You'd typically use the one provided by the received response, but the whole point of a fallback response is that there is no received response.

[spring-projects-issues]

Rob Winch commented

Thanks for the fast response!

Any chance there could be an additional builder method that provides a convenience method for setting the body that encapsulates this logic?

[spring-projects-issues]

Arjen Poutsma commented

Your wish is my command: poutsma@6d4f5bc

[spring-projects-issues]

Arjen Poutsma commented

I am going to merge this now, unless there are any more comments?

[spring-projects-issues]

Rob Winch commented

Thanks! +1 for merging

[spring-projects-issues]

Rob Winch commented

For reference, after this is merged you can handle errors in an ExchangeFilterFunction using something like:

ExchangeFilterFunction oops = ExchangeFilterFunction
		.ofRequestProcessor(r -> Mono.error(new RuntimeException("Ooops")));

ClientResponse fallbackResponse = ClientResponse.create(HttpStatus.OK)
		.body("Default")
		.build();

ExchangeFilterFunction fallback = (r,n) -> n
		.exchange(r)
		.onErrorResume(Exception.class, e -> Mono.just(fallbackResponse));

WebClient client = WebClient.builder()
		.filter(fallback)
		.filter(oops)
		.build();

String body = client.get().uri("https://www.google.com/")
		.retrieve()
		.bodyToMono(String.class)
		.block();

assertEquals("Default", body);

[spring-projects-issues]

Arjen Poutsma commented

That +1 is good enough for me, as I'd like to finish this up before the weekend ;). We can always change it before 5.0.5 comes out.

[spring-projects-issues]

Arjen Poutsma commented

Done, see 04c2a29