Skip to content

Commit ad736ef

Browse files
nabeelamirchnabeelamirch
authored
Merge pull request #15 from companieshouse/PCI-715-global-exceptions
Added global exception handler to give nicer error messages
2 parents a2f6561 + fa3c173 commit ad736ef

File tree

5 files changed

+292
-6
lines changed

5 files changed

+292
-6
lines changed

‎src/main/java/uk/gov/companieshouse/orders/api/controller/GlobalExceptionHandler.java‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
package uk.gov.companieshouse.orders.api.controller;
2+
3+
import com.fasterxml.jackson.core.JsonProcessingException;
4+
import org.springframework.http.HttpHeaders;
5+
import org.springframework.http.HttpStatus;
6+
import org.springframework.http.ResponseEntity;
7+
import org.springframework.http.converter.HttpMessageNotReadableException;
8+
import org.springframework.validation.FieldError;
9+
import org.springframework.validation.ObjectError;
10+
import org.springframework.web.bind.MethodArgumentNotValidException;
11+
import org.springframework.web.bind.annotation.ControllerAdvice;
12+
import org.springframework.web.context.request.WebRequest;
13+
import org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler;
14+
import uk.gov.companieshouse.orders.api.model.ApiError;
15+
import uk.gov.companieshouse.orders.api.util.FieldNameConverter;
16+
17+
import java.util.ArrayList;
18+
import java.util.List;
19+
20+
import static java.util.Collections.singletonList;
21+
22+
@ControllerAdvice
23+
public class GlobalExceptionHandler extends ResponseEntityExceptionHandler {
24+
25+
private final FieldNameConverter converter;
26+
27+
public GlobalExceptionHandler(FieldNameConverter converter) {
28+
this.converter = converter;
29+
}
30+
31+
@Override
32+
protected ResponseEntity<Object> handleMethodArgumentNotValid(
33+
final MethodArgumentNotValidException ex,
34+
final HttpHeaders headers,
35+
final HttpStatus status,
36+
final WebRequest request) {
37+
final ApiError apiError = buildBadRequestApiError(ex);
38+
return handleExceptionInternal(ex, apiError, headers, apiError.getStatus(), request);
39+
}
40+
41+
@Override
42+
protected ResponseEntity<Object> handleHttpMessageNotReadable(
43+
final HttpMessageNotReadableException ex,
44+
final HttpHeaders headers,
45+
final HttpStatus status,
46+
final WebRequest request) {
47+
48+
if (ex.getCause() instanceof JsonProcessingException) {
49+
final ApiError apiError = buildBadRequestApiError((JsonProcessingException) ex.getCause());
50+
return handleExceptionInternal(ex, apiError, headers, apiError.getStatus(), request);
51+
}
52+
53+
return super.handleHttpMessageNotReadable(ex, headers, status, request);
54+
}
55+
56+
/**
57+
* Utility to build ApiError from MethodArgumentNotValidException.
58+
*
59+
* @param ex the MethodArgumentNotValidException handled
60+
* @return the resulting ApiError
61+
*/
62+
ApiError buildBadRequestApiError(final MethodArgumentNotValidException ex) {
63+
final List<String> errors = new ArrayList<>();
64+
65+
for (final FieldError error : ex.getBindingResult().getFieldErrors()) {
66+
System.out.println("HIII");
67+
System.out.println(error.getObjectName());
68+
69+
errors.add(converter.toSnakeCase(error.getField()) + ": " + error.getDefaultMessage());
70+
}
71+
for (final ObjectError error : ex.getBindingResult().getGlobalErrors()) {
72+
errors.add(error.getObjectName() + ": " + error.getDefaultMessage());
73+
}
74+
75+
return new ApiError(HttpStatus.BAD_REQUEST, errors);
76+
}
77+
78+
/**
79+
* Utility to build ApiError from JsonProcessingException.
80+
*
81+
* @param jpe the JsonProcessingException handled
82+
* @return the resulting ApiError
83+
*/
84+
ApiError buildBadRequestApiError(final JsonProcessingException jpe) {
85+
final String errorMessage = jpe.getOriginalMessage();
86+
return new ApiError(HttpStatus.BAD_REQUEST, singletonList(errorMessage));
87+
}
88+
89+
}

‎src/main/java/uk/gov/companieshouse/orders/api/util/FieldNameConverter.java‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
package uk.gov.companieshouse.orders.api.util;
2+
3+
import org.springframework.stereotype.Component;
4+
5+
@Component
6+
public class FieldNameConverter {
7+
8+
/**
9+
* Converts the field name provided to its corresponding snake case representation.
10+
* It takes into account letters and numbers
11+
* @param fieldName the name of the field to be converted (typically camel case)
12+
* @return the field name's snake case representation minus any <code>is_</code>
13+
* string, assumed to be a prefix.
14+
*/
15+
public String toSnakeCase(final String fieldName) {
16+
String[] splitString = fieldName.split("(?=[A-Z0-9']+)");
17+
return String.join("_", splitString).toLowerCase().replace("is_", "");
18+
}
19+
20+
}

‎src/test/java/uk/gov/companieshouse/orders/api/controller/BasketControllerIntegrationTest.java‎

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import org.springframework.boot.test.mock.mockito.SpyBean;
1212
import org.springframework.http.MediaType;
1313
import org.springframework.test.web.servlet.MockMvc;
14+
import org.springframework.test.web.servlet.result.MockMvcResultHandlers;
1415
import uk.gov.companieshouse.orders.api.dto.AddDeliveryDetailsRequestDTO;
1516
import uk.gov.companieshouse.orders.api.dto.AddToBasketRequestDTO;
1617
import uk.gov.companieshouse.orders.api.dto.BasketPaymentRequestDTO;
@@ -27,14 +28,17 @@
2728
import java.util.Arrays;
2829
import java.util.Optional;
2930

31+
import static java.util.Arrays.asList;
3032
import static org.hamcrest.MatcherAssert.assertThat;
3133
import static org.hamcrest.core.Is.is;
3234
import static org.junit.Assert.*;
3335
import static org.mockito.ArgumentMatchers.any;
3436
import static org.mockito.Mockito.doAnswer;
3537
import static org.mockito.Mockito.when;
38+
import static org.springframework.http.HttpStatus.BAD_REQUEST;
3639
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.patch;
3740
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post;
41+
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content;
3842
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
3943
import static uk.gov.companieshouse.orders.api.util.TestConstants.ERIC_IDENTITY_HEADER_NAME;
4044
import static uk.gov.companieshouse.orders.api.util.TestConstants.ERIC_IDENTITY_VALUE;
@@ -346,16 +350,25 @@ public void addDeliveryDetailsFailsDueToFailedValidation() throws Exception {
346350
DeliveryDetailsDTO deliveryDetailsDTO = new DeliveryDetailsDTO();
347351
deliveryDetailsDTO.setAddressLine1("");
348352
deliveryDetailsDTO.setAddressLine2(ADDRESS_LINE_2);
349-
deliveryDetailsDTO.setCountry("");
353+
deliveryDetailsDTO.setCountry(COUNTRY);
354+
deliveryDetailsDTO.setPremises(PREMISES);
355+
deliveryDetailsDTO.setSurname(SURNAME);
350356
deliveryDetailsDTO.setForename(FORENAME);
357+
deliveryDetailsDTO.setLocality(LOCALITY);
351358
addDeliveryDetailsRequestDTO.setDeliveryDetails(deliveryDetailsDTO);
352359

360+
final ApiError expectedValidationError =
361+
new ApiError(BAD_REQUEST,
362+
asList("delivery_details.address_line_1: must not be blank"));
363+
353364
mockMvc.perform(patch("/basket")
354-
.header(REQUEST_ID_HEADER_NAME, TOKEN_REQUEST_ID_VALUE)
355-
.header(ERIC_IDENTITY_HEADER_NAME, ERIC_IDENTITY_VALUE)
356-
.contentType(MediaType.APPLICATION_JSON)
357-
.content(mapper.writeValueAsString(addDeliveryDetailsRequestDTO)))
358-
.andExpect(status().isBadRequest());
365+
.header(REQUEST_ID_HEADER_NAME, TOKEN_REQUEST_ID_VALUE)
366+
.header(ERIC_IDENTITY_HEADER_NAME, ERIC_IDENTITY_VALUE)
367+
.contentType(MediaType.APPLICATION_JSON)
368+
.content(mapper.writeValueAsString(addDeliveryDetailsRequestDTO)))
369+
.andExpect(status().isBadRequest())
370+
.andExpect(content().json(mapper.writeValueAsString(expectedValidationError)))
371+
.andDo(MockMvcResultHandlers.print());
359372
}
360373

361374
@Test

‎src/test/java/uk/gov/companieshouse/orders/api/controller/GlobalExceptionHandlerTest.java‎

Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,138 @@
1+
package uk.gov.companieshouse.orders.api.controller;
2+
3+
import com.fasterxml.jackson.core.JsonProcessingException;
4+
import org.junit.jupiter.api.Test;
5+
import org.junit.jupiter.api.extension.ExtendWith;
6+
import org.mockito.InjectMocks;
7+
import org.mockito.Mock;
8+
import org.mockito.junit.jupiter.MockitoExtension;
9+
import org.springframework.http.HttpHeaders;
10+
import org.springframework.http.HttpStatus;
11+
import org.springframework.http.ResponseEntity;
12+
import org.springframework.http.converter.HttpMessageNotReadableException;
13+
import org.springframework.validation.BindingResult;
14+
import org.springframework.validation.FieldError;
15+
import org.springframework.validation.ObjectError;
16+
import org.springframework.web.bind.MethodArgumentNotValidException;
17+
import org.springframework.web.context.request.WebRequest;
18+
import uk.gov.companieshouse.orders.api.model.ApiError;
19+
import uk.gov.companieshouse.orders.api.util.FieldNameConverter;
20+
21+
import java.util.Collections;
22+
23+
import static org.hamcrest.MatcherAssert.assertThat;
24+
import static org.hamcrest.Matchers.*;
25+
import static org.mockito.Mockito.when;
26+
import static org.springframework.http.HttpStatus.MULTI_STATUS;
27+
28+
@ExtendWith(MockitoExtension.class)
29+
public class GlobalExceptionHandlerTest {
30+
31+
private static final String OBJECT1 = "object1";
32+
private static final String OBJECT2 = "object2";
33+
private static final String FIELD1 = "field1";
34+
private static final String MESSAGE1 = "message1";
35+
private static final String MESSAGE2 = "message2";
36+
private static final String ORIGINAL_MESSAGE = "original";
37+
private static final HttpStatus ORIGINAL_STATUS = MULTI_STATUS;
38+
39+
/**
40+
* Extends {@link GlobalExceptionHandler} to facilitate its unit testing.
41+
*/
42+
private static final class TestGlobalExceptionHandler extends GlobalExceptionHandler {
43+
44+
public TestGlobalExceptionHandler(FieldNameConverter converter) {
45+
super(converter);
46+
}
47+
48+
@Override
49+
protected ResponseEntity<Object> handleExceptionInternal(final Exception ex,
50+
final Object body,
51+
final HttpHeaders headers,
52+
final HttpStatus status,
53+
final WebRequest request) {
54+
return new ResponseEntity<>(body, status);
55+
}
56+
}
57+
58+
@InjectMocks
59+
private TestGlobalExceptionHandler handlerUnderTest;
60+
61+
@Mock
62+
private MethodArgumentNotValidException mex;
63+
64+
@Mock
65+
private HttpMessageNotReadableException hex;
66+
67+
@Mock
68+
private JsonProcessingException jpe;
69+
70+
@Mock
71+
private BindingResult result;
72+
73+
@Mock
74+
private FieldNameConverter converter;
75+
76+
@Mock
77+
private HttpHeaders headers;
78+
79+
@Mock
80+
private WebRequest request;
81+
82+
@Test
83+
void buildsApiErrorFromMethodArgumentNotValidException() {
84+
85+
// Given
86+
when(mex.getBindingResult()).thenReturn(result);
87+
when(result.getFieldErrors()).thenReturn(Collections.singletonList(new FieldError(OBJECT1, FIELD1, MESSAGE1)));
88+
when(result.getGlobalErrors()).thenReturn(Collections.singletonList(new ObjectError(OBJECT2, MESSAGE2)));
89+
when(converter.toSnakeCase(FIELD1)).thenReturn(FIELD1);
90+
91+
// When
92+
final ResponseEntity<Object> response = handlerUnderTest.handleMethodArgumentNotValid(mex, headers, ORIGINAL_STATUS, request);
93+
94+
// Then
95+
final ApiError error = (ApiError) response.getBody();
96+
assertThat(error, is(notNullValue()));
97+
assertThat(error.getStatus(), is(HttpStatus.BAD_REQUEST));
98+
assertThat(error.getErrors().stream()
99+
.anyMatch(o -> o.equals(FIELD1 + ": " + MESSAGE1)), is(true));
100+
assertThat(error.getErrors().stream()
101+
.anyMatch(o -> o.equals(OBJECT2 + ": " + MESSAGE2)), is(true));
102+
}
103+
104+
@Test
105+
void buildsApiErrorFromJsonProcessingException() {
106+
107+
// Given
108+
when(hex.getCause()).thenReturn(jpe);
109+
when(jpe.getOriginalMessage()).thenReturn(ORIGINAL_MESSAGE);
110+
111+
// When
112+
final ResponseEntity<Object> response =
113+
handlerUnderTest.handleHttpMessageNotReadable(hex, headers, ORIGINAL_STATUS, request);
114+
115+
// Then
116+
final ApiError error = (ApiError) response.getBody();
117+
assertThat(error, is(notNullValue()));
118+
assertThat(error.getStatus(), is(HttpStatus.BAD_REQUEST));
119+
assertThat(error.getErrors().get(0), is(ORIGINAL_MESSAGE));
120+
}
121+
122+
@Test
123+
void delegatesHandlingOfNonJsonProcessingExceptionsToSpring() {
124+
125+
// Given
126+
when(hex.getCause()).thenReturn(hex);
127+
128+
// When
129+
final ResponseEntity<Object> response =
130+
handlerUnderTest.handleHttpMessageNotReadable(hex, headers, ORIGINAL_STATUS, request);
131+
132+
// Then
133+
// Note these assertions are testing behaviour implemented in the Spring framework.
134+
assertThat(response.getStatusCode(), is(ORIGINAL_STATUS));
135+
assertThat(response.getBody(), is(nullValue()));
136+
}
137+
138+
}

‎src/test/java/uk/gov/companieshouse/orders/api/util/FieldNameConverterTest.java‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
package uk.gov.companieshouse.orders.api.util;
2+
3+
import org.junit.jupiter.api.Test;
4+
import org.springframework.beans.factory.annotation.Autowired;
5+
import org.springframework.beans.factory.annotation.Qualifier;
6+
import org.springframework.boot.test.context.SpringBootTest;
7+
8+
import static org.hamcrest.MatcherAssert.assertThat;
9+
import static org.hamcrest.Matchers.is;
10+
11+
@SpringBootTest
12+
class FieldNameConverterTest {
13+
14+
@Qualifier("fieldNameConverter")
15+
@Autowired
16+
private FieldNameConverter converterUnderTest;
17+
18+
@Test
19+
void toSnakeCaseWorksAsExpected() {
20+
assertThat(converterUnderTest.toSnakeCase("itemCosts"), is("item_costs"));
21+
assertThat(converterUnderTest.toSnakeCase("item"), is("item"));
22+
assertThat(converterUnderTest.toSnakeCase("certIncConLast"), is("cert_inc_con_last"));
23+
assertThat(converterUnderTest.toSnakeCase("isPostalDelivery"), is("postal_delivery"));
24+
assertThat(converterUnderTest.toSnakeCase("addressLine1"), is("address_line_1"));
25+
}
26+
}

0 commit comments

Comments
 (0)