New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Convenience API for custom error handling on WebClient [SPR-15724] #20280
Comments
Arjen Poutsma commented The As an alternative, it would be quite easy and fitting to implement this feature as a 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? |
Boaz commented Hi Arjen, What you've describe is an approach that can work yet it's really verbose. 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. |
Igal commented Hey, Two thing worth mentioning:
|
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 Mono<ExampleResponse> exampleResponse = webClient.get()
.uri("http://spring.io/example")
.exchange()
.flatMap(clientResponse -> {
if (response.statusCode().is4xxClientError()) {
// ...
}
// ...
clientResponse.bodyToMono(ExampleResponse.class);
}); |
Igal commented I totally agree with you, we are doing what you've suggested on per request basis. |
Doron Gold commented
// ...
.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 |
Rossen Stoyanchev commented Ok yeah with I think what this is really about though is per-request error handling in combination with |
Arjen Poutsma commented
I agree that "local" error handling does not seem to add a lot of value when using All other methods, including So let's say we add such a mechanism to ResponseSpec onStatus(Predicate<HttpStatus>, Supplier<? extends Throwable>); Edit: See comment below for an updated version that allows for access to the 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:
Mono<ResponseEntity<MyPojo>> result = this.webClient.get()
.uri("/")
.exchange()
.flatMap(clientResponse -> clientResponse.toEntity(MyPojo.class)); Conceptually, such a move would also make sense: both If these proposed solutions sound satisfactory, then I can start working on this feature. I also think that the |
Arjen Poutsma commented On second read, it looks like you would like access to the ResponseSpec onStatus(Predicate<HttpStatus>, Function<ClientResponse, ? extends Throwable>); |
Rossen Stoyanchev commented I think replacing |
Doron Gold commented
ResponseSpec onStatus(Predicate<HttpStatus>, Function<ClientResponse, ? extends Throwable> Would be perfect. 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 All that goes to say that |
Arjen Poutsma commented
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. |
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 I did not add the convenience 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. |
Doron Gold commented After upgrading to Spring 5.0 RC3, we refactored all our error handling to use this new feature. However, we encountered a use-case, which might become very common, that we somehow missed in the discussion above: Our use-case: |
Doron Gold commented In order to be able to examine the body of the response before building the 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); |
Rossen Stoyanchev commented This was done under #20379 and is available in post-RC3 snapshots. |
Rossen Stoyanchev commented I should rather point to the commit since the actual ticket didn't ask for the body (see 5394cc). |
Arjen Poutsma commented Note that we could not have both the former method signature |
Doron Gold commented
|
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:
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
, andon4xx5xx
provide a "catch all" ability. They map a range of status codes to a Function.Affects: 5.0 RC2
Issue Links:
3 votes, 5 watchers
The text was updated successfully, but these errors were encountered: