Document that NoOpResponseErrorHandler is to be used with the RestTemplate

[pelepelin]

Affects: spring-web-6.1.11

Code

Let's pretend I don't want to handle status codes in the status handlers.

                RestClient.builder()
                    .defaultStatusHandler(new NoOpResponseErrorHandler())
                    .build()
                    .get()
                    .uri("http://httpbin.org/status/400")
                    .retrieve()
                    .toEntity(String.class)
                    .getBody()

Expected

Default status handler is overridden, there should be no status code handling, no exception thrown.

Actual

org.springframework.web.client.HttpClientErrorException$BadRequest: 400 Bad Request: [no body]
	at org.springframework.web.client.HttpClientErrorException.create(HttpClientErrorException.java:103) ~[spring-web-6.1.11.jar:6.1.11]
	at org.springframework.web.client.StatusHandler.lambda$defaultHandler$3(StatusHandler.java:86) ~[spring-web-6.1.11.jar:6.1.11]
	at org.springframework.web.client.StatusHandler.handle(StatusHandler.java:146) ~[spring-web-6.1.11.jar:6.1.11]
	at org.springframework.web.client.DefaultRestClient$DefaultResponseSpec.applyStatusHandlers(DefaultRestClient.java:698) ~[spring-web-6.1.11.jar:6.1.11]
	at org.springframework.web.client.DefaultRestClient.readWithMessageConverters(DefaultRestClient.java:200) ~[spring-web-6.1.11.jar:6.1.11]
	at org.springframework.web.client.DefaultRestClient$DefaultResponseSpec.readBody(DefaultRestClient.java:685) ~[spring-web-6.1.11.jar:6.1.11]
	at org.springframework.web.client.DefaultRestClient$DefaultResponseSpec.toEntityInternal(DefaultRestClient.java:655) ~[spring-web-6.1.11.jar:6.1.11]
	at org.springframework.web.client.DefaultRestClient$DefaultResponseSpec.toEntity(DefaultRestClient.java:644) ~[spring-web-6.1.11.jar:6.1.11]

Reason: while the built RestClient contains only a single NoOpResponseErrorHandler, a retrieve() call creates an org.springframework.web.client.DefaultRestClient.DefaultResponseSpec which puts an additional default StatusHandler at the end of the handlers list.

spring-framework/spring-web/src/main/java/org/springframework/web/client/DefaultRestClient.java

Lines 651 to 652 in fc28926

	this.statusHandlers.addAll(DefaultRestClient.this.defaultStatusHandlers);
	this.statusHandlers.add(StatusHandler.defaultHandler(DefaultRestClient.this.messageConverters));

and the handlers are tried sequentially

spring-framework/spring-web/src/main/java/org/springframework/web/client/DefaultRestClient.java

Lines 747 to 752 in fc28926

	for (StatusHandler handler : this.statusHandlers) {
	if (handler.test(response)) {
	handler.handle(this.clientRequest, response);
	return;
	}
	}

So, it's only possible to override the default error handler but not possible to replace it.

Workaround

Implement a handler so it returns true from hasError but does nothing in the handler. This looks more like abuse than following a contract.

                RestClient.builder()
                    .defaultStatusHandler(new ResponseErrorHandler() {
                        @Override
                        public boolean hasError(ClientHttpResponse response) throws IOException {
                            return true;
                        }

                        @Override
                        public void handleError(ClientHttpResponse response) {
                            // do nothing
                        }
                    })
                    .build()
                    .get()
                    .uri("http://httpbin.org/status/400")
                    .retrieve()
                    .toEntity(String.class)
                    .getBody()

[pelepelin]

Expectation is actually incorrect, because defaultStatusHandler is default in the sense "applied to all exchanges" and not in the sense "overrides previous default".

However, the bottom line is the same: I see no proper method to remove default handling of 4xx/5xx statuses.

[snicoll]

Thanks for the report. I agree that it's a little odd that the only way to disable the behavior is to implement an error handler that claims there is an error for every request. Looking at WebClient and its statusHandlers however, I can see that the same thing happens in DefaultResponseSpec:

spring-framework/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultWebClient.java

Lines 549 to 550 in fc28926

	this.statusHandlers.addAll(defaultStatusHandlers);
	this.statusHandlers.add(DEFAULT_STATUS_HANDLER);

I think the difference here is that the status handler uses a predicate based on the status code and you can indeed disable everything by using a predicate that always match. Perhaps NoOpResponseErrorHandler is named poorly, or we should change its implementation?

Wondering what the rest of the team thinks.

[rstoyanchev]

ResponseErrorHandler was the error mechanism of the RestTemplate. You could register one of these. The main error mechanism in RestClient is ResponseSpec.ErrorHandler, and you can register any number of these each with a Predicate<HttpStatusCode> that helps to decide which status handler to use. ResponseErrorHandler is adapted to a status handler where the hasError method used as the predicate.

That means a ResponseErrorHandler has to return true for the responses it wants to handle. If it wants to handle all error responses, then it should return true for status 400 and above, which is what the default status handler at the end of the chain does. Ideally though register a statusHandler with a predicate and a ResponseSpec.ErrorHandler. Maybe we can update the Javadoc to make all this more clear.

[pelepelin]

Sounds good. Probably the linked reference page could have an explicit example that in order to disable status handling one needs configuration like this (I have not checked)

RestClient restClient = RestClient.builder()
		.defaultStatusHandler((statusCode) -> true, (request, response) -> {})
		.build();

[snicoll]

Thanks for the suggestion. I think the main goal here is to clarify that NoOpResponseErrorHandler is meant to be used with the RestTemplate and that it can be used with the RestClient only to ease the migration of the former to the latter. With the Javadoc in place, I don't think there is a need of more content than that.