Doron Gold opened SPR-15724 and commented

It would be very helpful to have a way to set custom error handling for WebClient responses. This could be done by providing methods that allow developers to map each desired response code to an error handling Function.

Code that uses such functionality could look similar to the following:

Mono<ExampleResponse> exampleResponse = webClient.get()
                .uri("http://spring.io/example")
                .exchange()
                .onStatusCode(HttpStatus.FORBIDDEN, (ClientResponse clientResponse) -> {
                    return new CustomException("Access Forbidden");
                })
                .on4xx((ClientResponse clientResponse) -> {
                    return new CustomException("Client Error");
                })
                .on5xx((ClientResponse clientResponse) -> {
                    return new CustomException("Server Error");
                })
                .flatMap(clientResponse -> clientResponse.bodyToMono(ExampleResponse.class));

Mono<ExampleResponse2> exampleResponse2 = webClient.get()
                .uri("http://spring.io/example2")
                .exchange()
                .onStatusCode(HttpStatus.FORBIDDEN, (ClientResponse clientResponse) -> {
                    return new CustomException("Access Forbidden");
                })
                .onStatusCode(HttpStatus.INTERNAL_SERVER_ERROR, (ClientResponse clientResponse) -> {
                    return new CustomException("Internal Error");
                })
                .on4xx5xx((ClientResponse clientResponse) -> {
                    return new CustomException("General Error");
                })
                .flatMap(clientResponse -> clientResponse.bodyToMono(ExampleResponse2.class));

In the example above the onStatusCode method receives a status code and a Function. The specified status code maps to the specified Function. The Function is of the following type: Function<ClientResponse, ? extends Throwable> In case the returned response code matches, the appropriate Function is applied and an error Mono that wraps the Throwable returned by the Function is emitted.

The variants on4xx, on5xx, and on4xx5xx provide a "catch all" ability. They map a range of status codes to a Function.

Affects: 5.0 RC2

Issue Links: - #20379 WebClientException should allow access to status code of the response

3 votes, 5 watchers

Comment From: spring-projects-issues

Arjen Poutsma commented

The exchange method returns a Mono<ClientResponse>>, and because Mono is a generic type used in a lot of places, it is impossible to add methods like onStatusCode on it. One way to solve this would be for exchange to return a custom type that extends from Mono. But as Mono already has plenty of methods of its own, I don't think adding any additional ones is going to benefit the usability here, as the additional methods will just get lost in the crowd.

As an alternative, it would be quite easy and fitting to implement this feature as a ExchangeFilterFunction, which you can register as the web client is built. We could add a method with the following signature to the utility class ExchangeFilterFunctions:

public static ExchangeFilterFunction statusHandler(Predicate<HttpStatus> statusPredicate,
            Supplier<? extends Throwable> exceptionSupplier)

note the use of predicates, allowing for more flexibility.

You can then use this filter when building your web client:

WebClient client = WebClient.builder()
    .filter(ExchangeFilterFunctions.statusHandler(HttpStatus::is5xxServerError, MyException::new))
    .build();

What do you think about this alternative?

Comment From: spring-projects-issues

Boaz commented

Hi Arjen,

What you've describe is an approach that can work yet it's really verbose. The thing is, the reasoning behind rejecting Doron's example is the ordering in which it was described, meaning the presence of the exchange invocation before the error handlers. I fully agree, having those handlers available as part of the Mono class will be undesirable, but the example Doron provided could be altered slightly to demonstrate a better approach:

webClient.get()
                .uri("http://spring.io/example2")
                .onStatusCode(HttpStatus.FORBIDDEN, (ClientResponse clientResponse) -> {
                    return new CustomException("Access Forbidden");
                })
                .onStatusCode(HttpStatus.INTERNAL_SERVER_ERROR, (ClientResponse clientResponse) -> {
                    return new CustomException("Internal Error");
                })
                .on4xx5xx((ClientResponse clientResponse) -> {
                    return new CustomException("General Error");
                })
                .exchange()
                .flatMap(clientResponse -> clientResponse.bodyToMono(ExampleResponse2.class));

Note the changed location of the exchange invocation. This is a superior approach as it limits the change to the WebClient interface (and it's various fluent return types), it provides a more declarative coding style that is on par with the rest of the interface and allows the exchange method (or any other terminal method invocation in the WebClient class) to handle the "catching" of the status code and passing it to the handler.

Comment From: spring-projects-issues

Igal commented

Hey,

Two thing worth mentioning: 1. The approach as suggested by Arjan is less optimal, since this way we will have to build a web client on each method, that's because each method has its own internal logic. Though it's probably not so expensive to create a web client on each method, it's better to create it once and to be able to declare those predicates as a fluent declarative style.

  1. Also Doron and Boaz suggestions have another advantage. in their solution the predicate function gets the clientResponse and thats allows us to have logic based on the response body for example. Thats actually something we do need.

Comment From: spring-projects-issues

Rossen Stoyanchev commented

Do you actually mean for this to be per-request error handling or global, onStatus handlers? In Arjen's example the filter is declared on the builder, and therefore only once, and you can shorten it further with a static import. It's not something that you type for every request.

If what you mean is per-request error handling why do you even need onStatusCode vs doing it inside the flatMap (as shown below)? After all this is one reason for using exchange + flatMap vs retrieve which is more streamlined but less control.

Mono<ExampleResponse> exampleResponse = webClient.get()
                .uri("http://spring.io/example")
                .exchange()
                .flatMap(clientResponse -> {
                     if (response.statusCode().is4xxClientError()) {
                         // ...
                     }
                     // ...
                    clientResponse.bodyToMono(ExampleResponse.class);
});

Comment From: spring-projects-issues

Igal commented

I totally agree with you, we are doing what you've suggested on per request basis. Doron only suggested an improvement which describes a declarative/functional way for mapping a status code to an exception.

Comment From: spring-projects-issues

Doron Gold commented

@Rossen Stoyanchev as @Igal mentioned above, we need to do error mapping per request. Not globally per WebClient. Also, We currently do exactly what you suggested. However, we find that we have to write error handling almost for each request in our code and having it inside a flatMap() with several if statements is quite verbose. Our client services end up with many methods, each looking like the following:

// ...
.flatMap(clientResponse -> {
    // build a custom error in case not found
    if (HttpsStatus.NOT_FOUND == response.statusCode().value()) {
         // ...
         return Mono.error(/* ... */);
    }
    // build a custom error in case forbidden
    if (HttpsStatus.FORBIDDEN == response.statusCode().value()) {
          // ...
         return Mono.error(/* ... */);
    })
    // catch all other errors that we may have missed and build a general error
    if (response.statusCode().is4xxClientError() || response.statusCode().is5xxServerError()) {
         // ...
         return Mono.error(/* ... */);
    })
    // continue in case there was no error
    clientResponse.bodyToMono(ExampleResponse.class);
}

In addition to what @Boaz suggested in his comment, I would suggest an alternative: Add the error handling methods to WebClient.ResponseSpec. That's the type returned from retrieve().

Comment From: spring-projects-issues

Rossen Stoyanchev commented

Ok yeah with exchange() we'd have to either extend Mono as Arjen explained or add onStatus [response] handlers to the RequestHeaders/BodySpec, neither of which is ideal. It's also hard to justify since the current option isn't that much worse and we're creating two ways of doing the same.

I think what this is really about though is per-request error handling in combination with retrieve(). Perhaps we could add such methods to the ResponseSpec indeed. Arjen Poutsma what do you think?

Comment From: spring-projects-issues

Arjen Poutsma commented

I think what this is really about though is per-request error handling in combination with retrieve(). Perhaps we could add such methods to the ResponseSpec indeed.

I agree that "local" error handling does not seem to add a lot of value when using exchange(), and does seem to make more sense when using retrieve() in combination with bodyToMono and bodyToFlux. In fact, it makes so much sense that we already have a basic version of such a mechanism in place. But, as the javadoc explains, this status checking mechanism is mostly there because - when using retrieve with bodyToMono or bodyToFlux - there is no way for the user to get access to the status code, and therefore no way to act upon it. And an erroneous response will typically have a different format than a regular response, so we probably cannot use the same decoding mechanism there.

All other methods, including retrieve() followed by toEntity(Class) and toEntityList(Class) do not throw exceptions for 4xx or 5xx status codes, as they provide access to the response status code directly. One of the lessons we learned from RestTemplate is that we cannot simply assume that a 4xx or 5xx error should always result in an exception, especially when the response object provides access to the status code (cf. RestTemplate.exchange).

So let's say we add such a mechanism to WebClient.ResponseSpec. Something like:

ResponseSpec onStatus(Predicate<HttpStatus>, Supplier<? extends Throwable>);

Edit: See comment below for an updated version that allows for access to the ClientResponse.

so that you can do:

Mono<MyPojo> result = this.webClient.get()
    .uri("/")
    .retrieve()
    .onStatus(HttpStatus::is4xxClientError, MyClientException::new)
    .onStatus(HttpStatus::is5xxServerError, MyServerException::new)
    .bodyToMono(MyPojo.class);

There are a couple of things we need to consider:

  1. What happens with the default error handling? We definitely need a default mechanism in place, for reasons explained above. But we don't want these defaults to interfere with user-provided error checking, provided through onStatus. So one way to solve this would be to only use the default mechanism when no user-provided error checking has been registered.

  2. As explained above, the ResponseSpec.toEntity and ResponseSpec.toEntityList methods currently do not use the status code error detection mechanism, and I think this continues to makes sense when we introduce onStatus. However, the resulting API will be quite confusing, as onStatus would only apply to the bodyTo methods. I think we can solve this by moving the toEntity and toEntityList methods to ClientResponse itself, so that the the typical use case to get a response entity becomes:

Mono<ResponseEntity<MyPojo>> result = this.webClient.get()
        .uri("/")
        .exchange()
        .flatMap(clientResponse -> clientResponse.toEntity(MyPojo.class));

Conceptually, such a move would also make sense: both ClientResponse and ResponseEntity represent the same data: status code, headers, and a body. The only difference is that ResponseEntity's body is decoded and "disconnected", whereas ClientResponse provides "live" access to the byte stream.

If these proposed solutions sound satisfactory, then I can start working on this feature. I also think that the ExchangeFilterFunction, as described in my comment above, is still useful to have, though not to resolve this particular use case.

Comment From: spring-projects-issues

Arjen Poutsma commented

On second read, it looks like you would like access to the ClientResponse in the status error handler, so let's change that proposed signature to:

ResponseSpec onStatus(Predicate<HttpStatus>, Function<ClientResponse, ? extends Throwable>);

Comment From: spring-projects-issues

Rossen Stoyanchev commented

I think replacing toEntity with onStatus handlers after retrieve() make sense since it's a little nicer to handle the status in a declarative way like vs taking the ResponseEntity and then using if-else.

Comment From: spring-projects-issues

Doron Gold commented

@Arjen Poutsma:

it looks like you would like access to the ClientResponse in the status error handler That's exactly right. Having what you suggested :

ResponseSpec onStatus(Predicate<HttpStatus>, Function<ClientResponse, ? extends Throwable>

Would be perfect. Also, it would be nice to have additional convenience "catch all" methods, such as the following:

ResponseSpec onStatus4xxClientError(Function<ClientResponse, ? extends Throwable>);
ResponseSpec onStatus5xxServerError(Function<ClientResponse, ? extends Throwable>);
ResponseSpec onStatus4xx5xx(Function<ClientResponse, ? extends Throwable>);

The last one is for handling any status code in the range 400-599.

Regarding ExchangeFilterFunction, it is indeed very useful to have and we do use it when we build our WebClients. Basically each "client service" in our system is responsible to communicate with one other micro-service. So for example a BankClient is a Spring bean with methods for doing various operations against a Bank's API. The constructor of BankClient instantiates a WebClient with the base URL for the Bank API and with an ExchangeFilterFunction that inserts an authentication token to each request. If a request fails, the token is refreshed and the request is retried. All this authentication logic sits inside the ExchangeFilterFunction. Some of our WebClients even have several chained ExchangeFilterFunctions, each responsible for a different cross-cutting concern. Another example is Service Discovery: we use an ExchangeFilterFunction to discover the up-to-date host and port of the service that the current client needs to communicate with. The result is cached and if a request fails it is refreshed by inquiring our service-discovery once again.

All that goes to say that ExchangeFilterFunction is very useful for concerns that are global to the WebClient and orthogonal to specific API operations. Having a convenient way to do error handling per API operation is yet another useful feature.

Comment From: spring-projects-issues

Arjen Poutsma commented

Regarding ExchangeFilterFunction, it is indeed very useful to have and we do use it when we build our WebClients.

Of course. I am sorry, I guess I wasn't clear: I meant having a status-based error filter would still be useful, not to resolve this particular use case, but to achieve the same effect globally.

Comment From: spring-projects-issues

Arjen Poutsma commented

This is now in master. See this commit for the WebClient changes, this commit for the status-based error filter, and this commit for the toEntity(List) method move.

I did not add the convenience onStatus variants, as I didn't see the added value of them. In IntelliJ, typing ctrl-space after onStatus( will already suggest a list of HttpStatus methods that return boolean, and method references make the use of a Predicate here as convenient as it's going to get.

Moreover, we have to consider the signal-to-noise ratio as people are ctrl-spacing their way through the fluent API that WebClient exposes. Most users probably do not care about local status-based error handling, so those additional convenience methods would stand in their way as they are searching for the bodyTo methods, or other methods that we might add in the future.

I did, however, add HttpStatus.isError(), so you can check for both 4xx and 5xx status codes with one method reference. Let me know if there are any other HttpStatus methods you would like to see.

Thanks for reporting this, and helping us understand the desired functionality.

Comment From: spring-projects-issues

Doron Gold commented

After upgrading to Spring 5.0 RC3, we refactored all our error handling to use this new feature. Our code is much cleaner and more readable now.

However, we encountered a use-case, which might become very common, that we somehow missed in the discussion above: It is not possible to examine the body of the error response. The 2nd argument of onStatus() is Function<ClientResponse, ? extends Throwable>. So we can get statusCode and headers from ClientResponse and build our Throwable accordingly, but we cannot examine the body and build the Throwable accordingly. ClientResponse does have bodyToMono and toEntity, but they return a Mono, this is unusable because we need to return Throwable

Our use-case: when we call another microservice it may return an error response with a body that holds some info about the error. The error response always has the same structure (we use a special type: ServiceErrorResponse). When an error is received, we have to build a ServiceException which contains data that should be taken from ServiceErrorResponse.

Comment From: spring-projects-issues

Doron Gold commented

In order to be able to examine the body of the response before building the Throwable, maybe it is possible to add another signature to onStatus that builds a Mono instead of a Throwable. The purpose of that Mono is to hold the Throwable. The new signature can look like this:

ResponseSpec onStatus(Predicate<HttpStatus> statusPredicate, Function<ClientResponse, Mono<?> exceptionFunction);

With the above method in place, application code could do something like the following:

Mono<ExampleResponse> exampleResponse = webClient.get()
                .uri("http://spring.io/example")
                .retrieve()
                .onStatus(HttpStatus.NOT_FOUND::equals,
                        clientResponse -> new CustomException("Not Found"))
                .onStatus(HttpStatus::isError,
                        clientResponse -> clientResponse.bodyToMono(ServiceException.class)
                                    .flatMap(serviceException - > Mono.error(serviceException)))
                .bodyToMono(ExampleResponse.class);

Comment From: spring-projects-issues

Rossen Stoyanchev commented

This was done under #20379 and is available in post-RC3 snapshots.

Comment From: spring-projects-issues

Rossen Stoyanchev commented

I should rather point to the commit since the actual ticket didn't ask for the body (see 5394cc).

Comment From: spring-projects-issues

Arjen Poutsma commented

Note that we could not have both the former method signature onStatus(Predicate<HttpStatus>, Function<ClientResponse, Throwable>) as well as the new method signature onStatus(Predicate<HttpStatus>, Function<ClientResponse, Mono<Throwable>>) because of type erasure: both signatures reduce to onStatus(Preficate, Function) at runtime.

Comment From: spring-projects-issues

Doron Gold commented

@Aryen Poutsma Thanks for the fix! It's true that it's not possible to have both signatures. I pulled the snapshot build and checked how this works in our project. Had to refactor all uses of onStatus to use the new signature. It still looks well and it's good to have the possibility to read the body of the response.

Comment From: rakshith32

As explained above, similar to

Mono> result = this.webClient.get() .uri("/") .exchange() .flatMap(clientResponse -> clientResponse.toEntity(MyPojo.class));

below is the approach I am using for capturing the response body from vendor response in all cases of 200, 4xx, 5xx, along with its http status.

**Mono result = this.webClient.post() .uri("/") .body(BodyInserter.fromValue(jsonPayloadObject)) .exchange();

//returns response like this Mono> return result.flatMap(clientResponse -> clientResponse.toEntity(MyPojo.class));**

this is the only approach helping me to capture the "Response body & http status" from vendor,

the response body will be something like below Success example (http status code 200)

{ code: "200", type: "information" message: "deal info $number" }

failure example (http status code 500) { code: "500", type: "error" message: "duplicate deal etc" }

failure example (http status code 400) { code: "400", type: "error" message: "something in request is invalid" }

So question is, here is there anything wrong in my approach, is there any other way i can handle error scenarios to capture response body and status by returning Mono>. can anyone please suggest, or at least redirect this question where i can get the answer.