[REQ][Java][resttemplate] Being able to override response handling for a better error management

[astik]

Is your feature request related to a problem? Please describe.

restTemplate generated ApiClient handles response this way :

        if (responseEntity.getStatusCode().is2xxSuccessful()) {
            return responseEntity;
        } else {
            // The error handler built into the RestTemplate should handle 400 and 500 series errors.
            throw new RestClientException("API returned " + responseEntity.getStatusCode() + " and it wasn't handled by the RestTemplate error handler");
        }

It means, either we have a success response which is return with the right type, either we get a generic response with interesting information hidden in its message.

Describe the solution you'd like

Some methods from ApiClient are defined as private and others as protected which means it is meant to be extended. It would be great to be able to override the invokeAPI method but, some method used in it are private and can't be used from child class. I don't want to copy/paste most of the class, i'm just interested in the response handling.

One solution: we could move response handling to a protected method :

protected <T> ResponseEntity<T> handleResponse(ResponseEntity<T> responseEntity) {
	if (responseEntity.getStatusCode().is2xxSuccessful()) {
            return responseEntity;
        } else {
            // The error handler built into the RestTemplate should handle 400 and 500 series errors.
            throw new RestClientException("API returned " + responseEntity.getStatusCode() + " and it wasn't handled by the RestTemplate error handler");
        }
}

so that, child class could be able to handle response in the way they want (store response status code in a handy way, even go further by handling error response body to extract error key and then throw specific business exception.

Describe alternatives you've considered

Other solution: from the existing private invokeApi, throw a newly defined error exception extending RestClientException, that contains status code and response body (like what is done with other generators, like jersey for example)

[astik]

Here is an example of an overrided APIClient:

package org.acme.client;

import java.io.UnsupportedEncodingException;
import java.net.URI;
import java.net.URISyntaxException;
import java.net.URLEncoder;
import java.util.List;

import org.springframework.core.ParameterizedTypeReference;
import org.springframework.http.HttpHeaders;
import org.springframework.http.HttpMethod;
import org.springframework.http.MediaType;
import org.springframework.http.RequestEntity;
import org.springframework.http.RequestEntity.BodyBuilder;
import org.springframework.http.ResponseEntity;
import org.springframework.util.MultiValueMap;
import org.springframework.web.client.RestClientException;
import org.springframework.web.client.RestTemplate;
import org.springframework.web.util.UriComponentsBuilder;

public class ApiClient extends org.acme.generated.client.ApiClient {
	private RestTemplate restTemplate;

	public ApiClient(RestTemplate restTemplate) {
		super(restTemplate);
		this.restTemplate = restTemplate;
	}

	@Override
	public <T> ResponseEntity<T> invokeAPI(
			String path,
			HttpMethod method,
			MultiValueMap<String, String> queryParams,
			Object body,
			HttpHeaders headerParams,
			MultiValueMap<String, String> cookieParams,
			MultiValueMap<String, Object> formParams,
			List<MediaType> accept,
			MediaType contentType,
			String[] authNames,
			ParameterizedTypeReference<T> returnType
	) throws RestClientException {
		final UriComponentsBuilder builder = UriComponentsBuilder.fromHttpUrl(getBasePath()).path(path);
		if (queryParams != null) {
			// encode the query parameters in case they contain unsafe characters
			for (List<String> values : queryParams.values()) {
				if (values != null) {
					for (int i = 0; i < values.size(); i++) {
						try {
							values.set(i, URLEncoder.encode(values.get(i), "utf8"));
						} catch (UnsupportedEncodingException e) {

						}
					}
				}
			}
			builder.queryParams(queryParams);
		}
		URI uri;
		try {
			uri = new URI(builder.build().toUriString());
		} catch (URISyntaxException ex) {
			throw new RestClientException("Could not build URL: " + builder.toUriString(), ex);
		}
		final BodyBuilder requestBuilder = RequestEntity.method(method, uri);
		if (accept != null) {
			requestBuilder.accept(accept.toArray(new MediaType[accept.size()]));
		}
		if (contentType != null) {
			requestBuilder.contentType(contentType);
		}
		addHeadersToRequest(headerParams, requestBuilder);
		addCookiesToRequest(cookieParams, requestBuilder);
		RequestEntity<Object> requestEntity = requestBuilder.body(selectBody(body, formParams, contentType));
		ResponseEntity<T> responseEntity = restTemplate.exchange(requestEntity, returnType);
		return handleResponse(responseEntity);
	}

	private <T> ResponseEntity<T> handleResponse(ResponseEntity<T> responseEntity) {
		if (responseEntity.getStatusCode().is2xxSuccessful()) {
			return responseEntity;
		} else {
			// The error handler built into the RestTemplate should handle 400 and 500 series errors.
			throw new RestClientException(
					"API returned " + responseEntity.getStatusCode() + " and it wasn't handled by the RestTemplate error handler");
		}
	}
}

Notice that i removed updateParamsForAuth as i managed it through RestTemplate directly. I also remove defaultHeaders and defaultCookies as it is not needed for my use case. As those 3 elements are not protected, if i would have needed them, it would have needed more copy / paste ='(

Also, if i want debugging, i need to copy paste setDebugging method and ApiClientHttpRequestInterceptor class ='(

[astik]

One other solution that doesn't need overriding ApiClient is to add error management to ResponseErrorHandler:

	private class RestTemplateResponseErrorHandler implements ResponseErrorHandler {
		@Override
		public boolean hasError(ClientHttpResponse httpResponse) throws IOException {
			return (httpResponse.getStatusCode().series() == Series.CLIENT_ERROR
					|| httpResponse.getStatusCode().series() == Series.SERVER_ERROR);
		}

		@Override
		public void handleError(ClientHttpResponse httpResponse) throws IOException {
			if (httpResponse.getStatusCode().series() == Series.SERVER_ERROR) {
				// handle SERVER_ERROR
			} else if (httpResponse.getStatusCode().series() == Series.CLIENT_ERROR) {
				// handle CLIENT_ERROR
				if (httpResponse.getStatusCode() == HttpStatus.NOT_FOUND) {
					throw new MyNotFoundException();
				}
				if (httpResponse.getStatusCode() == HttpStatus.BAD_REQUEST) {
					throw new MyBadRequestException();
				}
				if (httpResponse.getStatusCode() == HttpStatus.CONFLICT) {
					throw new MyConflictException();
				}
			}
			throw new MyApiException(httpResponse.getStatusCode());
		}
	}

then, set it to the RestTemplate (you'll see the authentication management as well):

	private RestTemplate getRestTemplate() {
		RestTemplate restTemplate = new RestTemplate();
		// This allows us to read the response more than once - Necessary for debugging.
		restTemplate.setRequestFactory(new BufferingClientHttpRequestFactory(restTemplate.getRequestFactory()));
		restTemplate.setErrorHandler(new RestTemplateResponseErrorHandler());
		restTemplate.getInterceptors().add(new BasicAuthorizationInterceptor(username, password));
		return restTemplate;
	}

and finally the ApiClient:

	@Bean("apiClient")
	public ApiClient apiClient() {
		LOGGER.trace("Creating new ApiClient");
		ApiClient apiClient = new ApiClient(getRestTemplate());
		apiClient.setBasePath(basePath);
		apiClient.setDebugging(debug);
		return apiClient;
	}

So you can do pretty much what you want once you've got the RestTemplate instance. Therefore, this feature could be closed without anything else.

Still, current ApiClient with RestTemplate feels a little weird to work with and might need some make over. Another possibility, i missed completely how ApiClient should be used, but i haven't find any documentation about it ='(

[Erikmolin]

Hi @astik! I've recently run into the same issue, and I tend to agree that the current solution is a little weird. My thinking has been that simply removing the status check from the response should be enough. Since the DefaultResponseErrorHandler throws exceptions on 4xx and 5xx any way, the default behaviour would be almost the same. This would however allow us to implement way more flexible error handlers (for example, we can actually return responses with 4xx-statuses). Would this solve your problem, or cause more problems for you?

i.e replace

if (responseEntity.getStatusCode().is2xxSuccessful()) {
            return responseEntity;
        } else {
            // The error handler built into the RestTemplate should handle 400 and 500 series errors.
            throw new RestClientException("API returned " + responseEntity.getStatusCode() + " and it wasn't handled by the RestTemplate error handler");
        }

with just

            return responseEntity;

[astik]

My main problem is that handleResponse obfuscates error in a generic one without details. If we remove that and we get access to the response directly, i guess i should be ok.

From my point of view, the less i write client boilerplate code, the happier i am =)

[benn-end]

Yes, the same thing in our project. In 99% of cases, we need to catch exceptions from HTTP 4xx responses. In any case that is bad practice because the client has the response from the server so the caller should handle it. Exceptions should be thrown only in cases when the response is not returned.

[pcomp96]

I have open an issue for this problem https://github.com/OpenAPITools/openapi-generator/issues/18700, i hope that the community help us 👍🏻

[djechelon]

Additionally, RestTemplate behaviour differs between ResponseEntity<T> ___ForEntity and T ___forObject.

When you want the Body as T, a status other than success throws an exception. But if you look for the ResponseEntity an exception is never thrown and you can/have to parse the status code as any standard HTTP client would