Code Review: Generic Service Class for Calling External REST APIs with RestTemplate

[Simon Ritchie]

Hi folks,

There’s a third-party API with four endpoints that I’ve written a service class to interact with.  For the purposes of the example let’s say the endpoints are

/api/v1/dogs
/api/v1/cats
/api/v1/ducks
/api/v1/crocodiles

Each of the four endpoints are called using a RestTemplate and I’ve written a separate “wrapper” POJO to interpret the response from each endpoint.  Each of these four POJOs extends a base wrapper class that has three properties - a HttpStatus variable, a String variable that contains potential HTTP response error descriptions and a List<> of custom POJOs that I’m mapping errors to.  So my wrapper classes look like this

public class BaseWrapper {

	private HttpStatus httpStatus;
	private String statusText;
	private List<MyErrorDTO> errors;

	//Constructors, getters, setters

}

public class DogWrapper extends BaseWrapper {

	private String dogName;
	//Some other fields from the Dog endpoint
	//Constructors, getters, setters

}

public class CatWrapper extends BaseWrapper {

	private String catName;
	//Some other fields from the Cat endpoint
	//Constructors, getters, setters

}

public class DuckWrapper extends BaseWrapper {

	private String duckName;
	//Some other fields from the Duck endpoint
	//Constructors, getters, setters

}

public class CrocodileWrapper extends BaseWrapper {

	private String crocodileName;
	//Some other fields from the Crocodile endpoint
	//Constructors, getters, setters

}

The third-party APIs are written in such a way that if anything other than a 2xx response is sent, they will respond with a JSON array of error objects, so I’ve also created a wrapper for this response too

public class ErrorWrapper extends BaseWrapper {

	private List<ErrorDTO> errors;
	//Constructors, getters, setters

}

Here is my service class that does all of the heavy lifting of calling these external endpoints.  I’ve left out the import statements and the MyCustomException code as I think it’s probably enough to understand how the code works without seeing those

@Service
public class ClientService {

	private static final String DOGS_API = "https://www.remote.com/api/v1/dogs";
	private static final String CATS_API = "https://www.remote.com/api/v1/cats";
	private static final String DUCKS_API = "https://www.remote.com/api/v1/ducks";
	private static final String CROCODILES_API = "https://www.remote.com/api/v1/crocodiles";
	private static final Integer UNLIMITED = 0;
	private static final String BEARER = "Bearer";
	private static final String BODY = "body";
	
	@Autowired
	private RestTemplate restTemplate;

	/**
	 * Call the Dogs API
	 */
	public DogWrapper getDogs(final Integer limit) throws MyCustomException {
		try {
			final URI dogsUri = buildDogsUri(limit, DOGS_API);

			ResponseEntity<DogWrapper> responseEntity = getResponse(dogsUri, DogWrapper.class);

			return getDogWrapper(responseEntity);
		} catch (HttpClientErrorException exception) {
			log.error(exception.getMessage());
			return new DogWrapper(HttpStatus.valueOf(exception.getRawStatusCode()), exception.getStatusText(), getErrors(exception.getResponseBodyAsString()));
		}
	}

	/**
	 * Call the Cats API 
	 */
	public CatWrapper getCats(final Map<String, Object> parameters) throws MyCustomException {
		try {
			final URI catsUri = buildCatsUri(parameters, CATS_API);

			ResponseEntity<CatWrapper> responseEntity = getResponse(catsUri, CatWrapper.class);

			return getCatWrapper(responseEntity);
		} catch (HttpClientErrorException exception) {
			log.error(exception.getMessage());
			return new CatWrapper(HttpStatus.valueOf(exception.getRawStatusCode()), exception.getStatusText(), getErrors(exception.getResponseBodyAsString()));
		}
	}

	/**
	 * Call the Ducks API
	 */
	public DuckWrapper getDucks(final Map<String, Object> parameters) throws MyCustomException {
		try {
			final URI ducksUri = buildDucksUri(parameters, DUCKS_API);

			ResponseEntity<DuckWrapper> responseEntity = getResponse(ducksUri, DuckWrapper.class);

			return getDuckWrapper(responseEntity);
		} catch (HttpClientErrorException exception) {
			log.error(exception.getMessage());
			return new DuckWrapper(HttpStatus.valueOf(exception.getRawStatusCode()), exception.getStatusText(), getErrors(exception.getResponseBodyAsString()));
		}
	}

	/**
	 * Call the Crocodiles API
	 */
	public CrocodileWrapper getCrocodiles(final Map<String, Object> parameters) throws MyCustomException {
		try {
			final URI crocodilesUri = buildCrocodilesUri(parameters, CROCODILES_API);

			ResponseEntity<CrocodileWrapper> responseEntity = getResponse(crocodilesUri, CrocodileWrapper.class);

			return getCrocodileWrapper(responseEntity);
		} catch (HttpClientErrorException exception) {
			log.error(exception.getMessage());
			return new CrocodileWrapper(HttpStatus.valueOf(exception.getRawStatusCode()), exception.getStatusText(), getErrors(exception.getResponseBodyAsString()));
		}
	}

	/**
	 * Build the URI for Dogs - it has an optional parameter called "limit"
	 */
	private URI buildDogsUri(final Integer limit, final String dogsUrl) {
		UriComponentsBuilder uriComponentsBuilder = UriComponentsBuilder.fromUriString(dogsUrl);

		if (limit > UNLIMITED) {
			Map<String, Object> params = new HashMap<>();
			params.put(LIMIT, limit);
			uriComponentsBuilder.buildAndExpand(params);
		}

		return uriComponentsBuilder.build().toUri();
	}

	/**
	 * Build the URI for Cats - it has two optional parameters called FIRST_PARAM and SECOND_PARAM
	 */
	private URI buildCatsUri(final Map<String, Object> parameters, final String catsUrl) {
		UriComponentsBuilder uriComponentsBuilder = UriComponentsBuilder.fromUriString(catsUrl);

		if (ObjectUtils.isNotEmpty(parameters.get("FIRST_PARAM"))) {
			String firstParam = (String) parameters.get("FIRST_PARAM");
			uriComponentsBuilder.queryParam("FIRST_PARAM", firstParam);
		}

		if (ObjectUtils.isNotEmpty(parameters.get("SECOND_PARAM"))) {
			String secondParam = (String) parameters.get("SECOND_PARAM");
			uriComponentsBuilder.queryParam("SECOND_PARAM", secondParam);
		}

		return uriComponentsBuilder.build().toUri();
	}

	/**
	 * Build the URI for Ducks API - it has two mandatory parameters DUCK_FIRST_NAME and DUCK_SECOND_NAME
	 */
	private URI buildDucksUri(final Map<String, Object> parameters, final String ducksUrl) throws MyCustomException {
		if (ObjectUtils.isEmpty(parameters.get("DUCK_FIRST_NAME")) {
			log.error("No first name provided");
			throw new MyCustomException();
		}

		if (ObjectUtils.isEmpty(parameters.get("DUCK_SECOND_NAME")) {
			log.error("No second name ID provided");
			throw new MyCustomException();
		}

		Map<String, Object> pathParameters = new HashMap<>();
		String firstName = (String) parameters.get("DUCK_FIRST_NAME");
		pathParameters.put("DUCK_FIRST_NAME", firstName);
		String secondName = (String) parameters.get("DUCK_SECOND_NAME");
		pathParameters.put("DUCK_SECOND_NAME", secondName);
		return UriComponentsBuilder.fromUriString(ducksUrl).buildAndExpand(pathParameters).toUri();
	}

	/**
	 * Build the headers for the request
	 */
	private HttpEntity<String> buildHttpEntity(final String token) {
		HttpHeaders httpHeaders = new HttpHeaders();
		httpHeaders.add(HttpHeaders.AUTHORIZATION, BEARER + StringUtils.SPACE + token);
		httpHeaders.setAccept(Collections.singletonList(MediaType.APPLICATION_JSON));
		return new HttpEntity<>(BODY, httpHeaders);
	}

	/**
	 * Generic method to perform a GET request to the external APIs
	 * If no response is received or if a problem is encountered, throw an exception
	 */
	private <T> ResponseEntity<T> getResponse(final URI uri, final Class<T> clazz) throws MyCustomException {
		final String token = //Get the token from another API - this part is working

		final HttpEntity<String> entity = buildHttpEntity(token);

		final ResponseEntity<T> responseEntity = restTemplate.exchange(uri, HttpMethod.GET, entity, clazz);

		ensureSuccessfulRequest(uri, responseEntity);

		ensureResponseBodyPresent(uri, responseEntity);

		return responseEntity;
	}

	/**
	 * Ensure that the response from the API was 2xx.  If not then throw an exception
	 */
	private <T> void ensureSuccessfulRequest(final URI uri, final ResponseEntity<T> responseEntity) throws MyCustomException {
		if (!responseEntity.getStatusCode().is2xxSuccessful()) {
			HttpStatus httpStatus = responseEntity.getStatusCode();
			String reasonPhrase = httpStatus.getReasonPhrase();
			throw new MyCustomException(httpStatus, reasonPhrase);
		}
	}

	/**
	 * Ensure that the response body is not null.  Otherwise throw an exception
	 */
	private <T> void ensureResponseBodyPresent(URI crocodilesUri, ResponseEntity<T> responseEntity) throws MyCustomException {
		if (ObjectUtils.isEmpty(responseEntity.getBody())) {
			throw new MyCustomException();
		}
	}

	/**
	 * Get the list of errors that were received in response to the request
	 */
	private List<ErrorDTO> getErrors(final String responseBody) {
		try {
			ObjectMapper objectMapper = new ObjectMapper();
			ErrorWrapper errorWrapper = objectMapper.readValue(responseBody, ErrorWrapper.class);
			return errorWrapper.getErrors();
		} catch (JsonProcessingException exception) {
			log.error(exception.getMessage());
			return Collections.emptyList();
		}
	}

	/**
	 * Get the response wrapper for the Dogs API
	 */
	private DogWrapper getDogWrapper(ResponseEntity<DogWrapper> responseEntity) throws MyCustomException {
		DogWrapper dogWrapper = responseEntity.getBody();
		dogWrapper.setHttpStatus(HttpStatus.OK);  //IntelliJ complains that this could potentially produce a NullPointerException despite the fact that the ensureResponseBodyPresent() method would have thrown an exception before now if the response.getBody() was empty - why is this?
		return dogWrapper;
	}

	/**
	 * Get the response wrapper for the Cats API
	 */
	private CatWrapper getCatWrapper(ResponseEntity<CatWrapper> responseEntity) throws MyCustomException {
		CatWrapper catWrapper = responseEntity.getBody();
		catWrapper.setHttpStatus(HttpStatus.OK);  //IntelliJ complains that this could potentially produce a NullPointerException despite the fact that the ensureResponseBodyPresent() method would have thrown an exception before now if the response.getBody() was empty - why is this?
		return catWrapper;
	}

	/**
	 * Get the response wrapper for the Ducks API
	 */
	private DuckWrapper getDuckWrapper(ResponseEntity<DuckWrapper> responseEntity) throws MyCustomException {
		DuckWrapper duckWrapper = responseEntity.getBody();
		duckWrapper.setHttpStatus(HttpStatus.OK);  //IntelliJ complains that this could potentially produce a NullPointerException despite the fact that the ensureResponseBodyPresent() method would have thrown an exception before now if the response.getBody() was empty - why is this?
		return duckWrapper;
	}

	/**
	 * Get the response wrapper for the Crocodiles API
	 */
	private CrocodileWrapper getCrocodileWrapper(ResponseEntity<CrocodileWrapper> responseEntity) throws MyCustomException {
		CrocodileWrapper crocodileWrapper = responseEntity.getBody();
		crocodileWrapper.setHttpStatus(HttpStatus.OK);  //IntelliJ complains that this could potentially produce a NullPointerException despite the fact that the ensureResponseBodyPresent() method would have thrown an exception before now if the response.getBody() was empty - why is this?
		return crocodileWrapper;
	}
}

Despite the fact that this is the best I could do, I’m not happy in the slightest.  There are a number of problems.  First, I tried to make the class as generic as possible, which is why I delegated all the RestTemplate calls to a single method called getResponse() that returns a generic ResponseEntity object.  But this has had the effect of making all of the methods getDogs, getCats, getDucks, getCrocodiles look the same if not very similar, which could confuse another developer.

Secondly, I don’t think I’m using generics very well here.  Can someone enlighten me and tell me if it could be improved on?

Thirdly, I’m not too sure that using two methods ensure…() to throw exceptions is clean code.  For a start, despite the fact that if the responseEntity.getBody() method returns null then a MyCustomException will be thrown but my IDE still says that setting the HttpStatus variable in the get…Wrapper() methods might still produce a NullPointerException.  Why is that?

There are probably dozens of things wrong with this class but I really want to learn and want to know what is the best way to write a service that calls an external REST API.  Any help would be welcome.

[Liutauras Vilda]

Cowgratulations, your post has been published in CodeRanch's August 2020 journal.

[Simon Ritchie]

Thanks!

[Junilu Lacar]

What's definitely smelly to me are all these classes that extend BaseWrapper. What are you "wrapping" with these classes anyway? What do you gain from making all these classes extend BaseWrapper? How can a DogWrapper be in the same class hierarchy as an ErrorWrapper? This hierarchy seems like an improper use of inheritance to me and I suspect there are many instances of violating the Liskov Substitution Principle

Also, what exactly is your understanding of "clean code"? For me, the basic qualities of clean code is that it makes sense, that it's intent is clear, and that it's easy to work with. At the very least, the clarity of intent quality is lacking in this code.

[Simon Ritchie]

Junilu Lacar wrote:What's definitely smelly to me are all these classes that extend BaseWrapper. What are you "wrapping" with these classes anyway?

I'm wrapping the responses from a third party API.

Junilu Lacar wrote:What do you gain from making all these classes extend BaseWrapper?

Each of the response objects from the four different APIs I'm calling are different.  But they will all have a HttpStatus, a text field containing a description of the HttpStatus (i.e. "404 Not Found") and then, if the there was an error on the client side, a list of MyErrorDTO objects.  So rather than have those fields duplicated in the wrappers for the Dog, Cat, Crocodile APIs I thought they could all extend a base wrapper class to avoid duplication.

Junilu Lacar wrote:How can a DogWrapper be in the same class hierarchy as an ErrorWrapper?

Yes, that's a good point.

Junilu Lacar wrote:Also, what exactly is your understanding of "clean code"? For me, the basic qualities of clean code is that it makes sense, that it's intent is clear, and that it's easy to work with. At the very least, the clarity of intent quality is lacking in this code.

My understanding would be the same as yours in that clean code should first of all be easy to understand, should avoid unnecessary duplication and be easy to maintain.  I acknowledge that this code I've written is not very clear, which is why I decided to put it up for review.  I had toyed with the idea of writing four different methods, one for each client API call, but I thought this would lead to duplication of code where I was calling the RestTemplate.  I thought "Why not just have a generic method that calls the RestTemplate and then convert the response data to the relevant wrapper?"

[Junilu Lacar]

I think the wrapper concept is not appropriate here. It's abstracting at the wrong level. The third party API is an external concern and calling out to it via a common  getResponse() method is more of an infrastructure concern than a domain concern. What you end up with are two different kinds of ideas (domain: Dogs, Cats, Crocodiles, etc. versus infrastructure) more tightly coupled. What happens when you need to support additional domain types like Chickens or Pigs? It would be pretty intrusive to this code, wouldn't it? It violates the Single Responsibility and the Open-Closed Principles.

In similiar situations, I would try to figure out a design where I only need to add new classes instead of adding new methods to an existing class to support additional domain types. The design would also either allow configuration-based registration of the new domain classes or have some way of discovering domain classes as they are added to the system. If you go the discovery route, it would also adapt the system when types have been removed or obsoleted.

[Junilu Lacar]

One other thing about clean code: it's easy to test. Think about what kind of challenges the current design poses with regard to testing. Then make the design so that testing becomes easier to do.

[Simon Ritchie]

That's a thoughtful response, thank you.  I've some further questions, if that's alright.

Junilu Lacar wrote:I think the wrapper concept is not appropriate here. It's abstracting at the wrong level.

What would you recommend I use instead?  I thought I only had two choices when it comes to using a RestTemplate -
1) creating a wrapper class to contain the response
2) mapping the response to a Map<String, Object>

I went with the wrapper approach because I thought the alternative (using a Map) would lead to lots of code where I would have to pick through the map looking for properties with specific names and then try and map those properties to DTOs or POJOs, etc.  

Junilu Lacar wrote:In similiar situations, I would try to figure out a design where I only need to add new classes instead of adding new methods to an existing class to support additional domain types.

Do you think then that a better design would be to have a separate class (with its own RestTemplate) for each external API call instead of cramming them all into one service as I've done here?  It makes sense but it leads to more classes...

Junilu Lacar wrote:The design would also either allow configuration-based registration of the new domain classes or have some way of discovering domain classes as they are added to the system. If you go the discovery route, it would also adapt the system when types have been removed or obsoleted.

Oh, that's interesting.  So basically, design it in such a way that on application startup the code should get the format of the responses from a configuration file?

[Junilu Lacar]

Simon Ritchie wrote:Oh, that's interesting.  So basically, design it in such a way that on application startup the code should get the format of the responses from a configuration file?

That's not what I was thinking. More along the lines of a custom annotation that would tag a class as being one of these, for lack of a better term, "wrapper" classes. On startup, the application will scan for classes that are marked with that annotation. That would probably be the end goal because there are simpler ways that you can implement fairly quickly. I wouldn't go the "get the format fo the responses from a configuration file" route though. That seems more complicated than it needs to be. I assume the format of each response is stable and known so you can just code that into the wrapper class itself. I still don't think "wrapper" is the appropriate term for these things.