Document that WebClient defaultStatusHandler do not apply to exchange* methods

[rubasace]

As the title states, the behavior of WebClient.Builder.defaultStatusHandler() seems inconsistent. If I'm setting a default status handler my expectation is that it handles the status regardless of how the call is done by the client. This becomes particularly important when customizing the builder from a common library, as I need to enforce that the default handling always takes place; Prior to Spring Framework 6 and Spring Boot 3, this could be done via an ExchangeFilterFunction. Now, things get more complicated as exceptions thrown from the exchange filter functions affect the actuator observations, forcing me to override quite some methods just to be able to not lose the response HTTP status on them.

As for documentation, I couldn't find anywhere where this is documented either and, on StackOverflow answers, everything seems to indicate that retrieve() is just a simplification of the exchangeTo* methods when we don't care about more granular control.

Taking all the above into consideration, I'm wondering if there's a good reason why defaultStatusHandler() does only apply to retrieve() calls. Even if that were the case, I think this should be somehow stated in the documentation.

[rstoyanchev]

Status handlers can only be declared under retrieve() and defaultStatusHandlers likewise were only ever meant to apply to retrieve. The idea is that with retrieve() one doesn't see the response, while exchange and exchangeTo* methods have direct access to it. So status handlers for transparently handled responses via retrieve. Existing exchangeTo* methods may well deal with the response status already and applying a status handler on top of that would be a problem. We can improve the Javadoc for defaultStatusHandler to be more clear about this.

That said I'm interested in your comment related to Spring Framework 6 and actuator observations, could you provide more details? It may not be an intended consequence that we should consider and possibly address.

[rubasace]

Sure @rstoyanchev :)

Let me give a bit of context on why this is an issue for us: we have a custom framework extending Spring Boot and adding a lot of business-specific defaults like standardized logging format and error handling (among many others). Our framework allows us to opt-out from the default error handler but we still want to put it in place by default, regardless of the client calling retrieve() or exchange(). This is because in many scenarios we are calling exchange() to work with the response headers. I know since 5.2 we can get the ResponseEntity from the retrieve() method but we cannot guarantee everybody uses it (across 200+ applications) as on the online examples (even in places like Baeldung) it seems to be the standard to use exchange() when you care about something else that isn't the body: example).

Now the observation issue: before the introduction of the Observation API, metrics were calculated by an implementation of the ExchangeFilterFunction interface (MetricsWebClientFilterFunction). This class delegated to a tag provider that would decide the status tag based on the HTTP status. So, in order to guarantee that the error handling was taking place we implemented another ExchangeFilterFunction that would be applied after the metrics one, throwing a custom exception in case of an invalid HTTP status without affecting the outcome of the metrics.

Our problem now is that, by moving the metrics measurement away from the ExchangeFilterFunction implementation, we are left with no clean solution to place a default error handler anywhere. This is because any thrown exception from the filters will be processed as an error by the observation, losing all information about the HTTP status in the process.

As a temporary solution, we just overwrote the DefaultClientRequestObservationConvention so it can add special cases for our exception (which holds the HTTP status) but it feels like a bit of an overkill for something as simple as a default error handling mechanism.

It feels a bit weird given that on RestTemplate we can have a default error handler and, as far as I know, it's respected regardless of the call you make.

[bclozel]

@rubasace I'm looking at the observation instrumentation aspect of this issue. About this particular section:

Our problem now is that, by moving the metrics measurement away from the ExchangeFilterFunction implementation, we are left with no clean solution to place a default error handler anywhere. This is because any thrown exception from the filters will be processed as an error by the observation, losing all information about the HTTP status in the process.
As a temporary solution, we just overwrote the DefaultClientRequestObservationConvention so it can add special cases for our exception (which holds the HTTP status) but it feels like a bit of an overkill for something as simple as a default error handling mechanism.

If I understand correctly, the application is setting up the client with an ExhangeFilterFunction that can throw exceptions under some circumstances, like:

WebClient.builder().filter(new ExchangeFilterFunction() {
	@Override
	public Mono<ClientResponse> filter(ClientRequest request, ExchangeFunction next) {
		return next.exchange(request).flatMap(response -> {
			if (response.headers().header("X-Custom-Header").size() == 0) {
				return Mono.error(new IllegalStateException());
			}
			return Mono.just(response);
		});
	}
}).build();

And in this case, the observation is recorded as an error and the HTTP status and other information are missing, since the error signal prevented the actual response from flowing to the instrumentation.

I've got a change lined up to fix this, but want to make sure about the following: the observation should still record an error, even if it has been thrown by a filter. In my opinion, it still makes sense to mark the observation as an error because the client code won't be able to consume the response. Do you agree with this behavior?

[rubasace]

Hi @bclozel ,

actually, the interceptors are throwing the exceptions based on the HTTP status. The reason why is that we cannot add a generic status handler that takes place regardless of the developers using .exchange() or .retrieve() as I mentioned above. The main issue here is the lack of a mechanism to add a default status code handler regardless of how the calls are executed.

the observation should still record an error, even if it has been thrown by a filter

I understand the thought process but the issue is that this is the behavior that leaves us with no good solution for a global status code handler. It would be ok if somehow the same could be achieved without an exchangeFilter but that's not the case at the moment.

PS: this exchangeFilter is not set by the application but by a common company framework aligning 200+ microservices, that's why having a way of configuring this is key for us

[bclozel]

I understand @rubasace , but this point has been answered already by @rstoyanchev in #30059 (comment) - we can't make the default status handler work for .exchange() calls as it doesn't fit the programming model. Changing that behavior would be a breaking change for a lot of applications, including for companies that have large deployment like yourself.

I'm repurposing this issue as a documentation to make that clearer in the Javadoc. I've also opened #30247 to fix the observability side of things - this should improve the situation in your case.

[rubasace]

That's ok @bclozel , the above-mentioned issue will definitely help.

The only thing I'd like to mention is that I understand the point from @rstoyanchev:

Existing exchangeTo* methods may well deal with the response status already and applying a status handler on top of that would be a problem.

But reality is that same idea applies to RestTemplate exchange() method and I believe, in that case, the status handlers are still respected. I'm happy with the documentation improvement though :)