From 98f5ac263f850d6c9467687d402c959e4c9484e8 Mon Sep 17 00:00:00 2001 From: teleivo Date: Fri, 1 Mar 2024 09:18:53 +0100 Subject: [PATCH] test: asssert on actual types and their order instead of strings. Add tests for passing comma separated values in one order param. Add test for the mixed case of comma separated values in one order param with a repeated param --- .../controller/CrudControllerAdvice.java | 20 +++++ .../webapi/controller/OrderBindingTest.java | 79 ++++++++++++++----- 2 files changed, 81 insertions(+), 18 deletions(-) diff --git a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/CrudControllerAdvice.java b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/CrudControllerAdvice.java index 6d4c48c3e673..a126787a316d 100644 --- a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/CrudControllerAdvice.java +++ b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/CrudControllerAdvice.java @@ -88,6 +88,8 @@ import org.hisp.dhis.webapi.security.apikey.ApiTokenAuthenticationException; import org.hisp.dhis.webapi.security.apikey.ApiTokenError; import org.springframework.beans.TypeMismatchException; +import org.springframework.core.convert.ConversionFailedException; +import org.springframework.core.convert.TypeDescriptor; import org.springframework.dao.DataAccessResourceFailureException; import org.springframework.http.HttpStatus; import org.springframework.http.converter.HttpMessageNotReadableException; @@ -226,6 +228,8 @@ public WebMessage methodArgumentTypeMismatchException(MethodArgumentTypeMismatch customErrorMessage = getGenericFieldErrorMessage(requiredType.getSimpleName()); } else if (ex.getCause() instanceof IllegalArgumentException) { customErrorMessage = ex.getCause().getMessage(); + } else if (ex.getCause() instanceof ConversionFailedException conversionException) { + customErrorMessage = getConversionErrorMessage(ex, conversionException); } else { customErrorMessage = getGenericFieldErrorMessage(requiredType.getSimpleName()); } @@ -291,6 +295,22 @@ private String getFormattedBadRequestMessage(String fieldErrorMessage, String cu return fieldErrorMessage + " " + customMessage; } + private static String getConversionErrorMessage( + MethodArgumentTypeMismatchException ex, ConversionFailedException conversionException) { + String customErrorMessage; + if (TypeDescriptor.valueOf(String.class) == conversionException.getSourceType() + && ((String) conversionException.getValue()).contains(",") + && ex.getValue() != null + && ex.getValue().getClass().isArray()) { + customErrorMessage = + "You might have repeated a request parameter and used multiple comma-separated values within at least one of the parameters. Choose one of both possible approaches. " + + ex.getCause().getMessage(); + } else { + customErrorMessage = ex.getCause().getMessage(); + } + return customErrorMessage; + } + /** * A BindException wraps possible errors happened trying to bind all the request parameters to a * binding object. Errors could be simple conversion failures (Trying to convert a 'TEXT' to an diff --git a/dhis-2/dhis-web-api/src/test/java/org/hisp/dhis/webapi/controller/OrderBindingTest.java b/dhis-2/dhis-web-api/src/test/java/org/hisp/dhis/webapi/controller/OrderBindingTest.java index dc671a25c4c8..3505f3d59715 100644 --- a/dhis-2/dhis-web-api/src/test/java/org/hisp/dhis/webapi/controller/OrderBindingTest.java +++ b/dhis-2/dhis-web-api/src/test/java/org/hisp/dhis/webapi/controller/OrderBindingTest.java @@ -29,10 +29,13 @@ import static org.hamcrest.core.StringContains.containsString; import static org.hisp.dhis.dxf2.webmessage.WebMessageUtils.ok; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; import java.util.List; +import org.hisp.dhis.common.SortDirection; import org.hisp.dhis.dxf2.webmessage.WebMessage; import org.hisp.dhis.webapi.controller.event.webrequest.OrderCriteria; import org.junit.jupiter.api.BeforeEach; @@ -46,8 +49,8 @@ class OrderBindingTest { private static final String ENDPOINT = "/ordering"; - private MockMvc mockMvc; + private List actual; @BeforeEach public void setUp() { @@ -59,35 +62,70 @@ public void setUp() { @Test void shouldReturnDefaultSortDirectionWhenNoSortDirectionIsPassedAsParameter() throws Exception { - mockMvc - .perform(get(ENDPOINT).param("order", "field")) - .andExpect(content().string(containsString("OK"))) - .andExpect(content().string(containsString("field"))) - .andExpect(content().string(containsString("ASC"))); + mockMvc.perform(get(ENDPOINT).param("order", "field")).andExpect(status().isOk()); + + assertEquals(List.of(OrderCriteria.of("field", SortDirection.ASC)), actual); } @Test void shouldReturnAscSortDirectionWhenAscSortDirectionIsPassedAsParameter() throws Exception { - mockMvc - .perform(get(ENDPOINT).param("order", "field:asc")) - .andExpect(content().string(containsString("OK"))) - .andExpect(content().string(containsString("field"))) - .andExpect(content().string(containsString("ASC"))); + mockMvc.perform(get(ENDPOINT).param("order", "field:asc")).andExpect(status().isOk()); + + assertEquals(List.of(OrderCriteria.of("field", SortDirection.ASC)), actual); } @Test void shouldReturnDescSortDirectionWhenDescSortDirectionIsPassedAsParameter() throws Exception { + mockMvc.perform(get(ENDPOINT).param("order", "field:desc")).andExpect(status().isOk()); + + assertEquals(List.of(OrderCriteria.of("field", SortDirection.DESC)), actual); + } + + @Test + void shouldHandleMultipleOrderComponentsGivenViaOneParameter() throws Exception { mockMvc - .perform(get(ENDPOINT).param("order", "field:desc")) - .andExpect(content().string(containsString("OK"))) - .andExpect(content().string(containsString("field"))) - .andExpect(content().string(containsString("DESC"))); + .perform(get(ENDPOINT).param("order", "field1:desc,field2:asc")) + .andExpect(status().isOk()); + + assertEquals( + List.of( + OrderCriteria.of("field1", SortDirection.DESC), + OrderCriteria.of("field2", SortDirection.ASC)), + actual); + } + + @Test + void shouldHandleMultipleOrderComponentsGivenViaMultipleParameters() throws Exception { + mockMvc + .perform(get(ENDPOINT).param("order", "field1:desc").param("order", "field2:asc")) + .andExpect(status().isOk()); + + assertEquals( + List.of( + OrderCriteria.of("field1", SortDirection.DESC), + OrderCriteria.of("field2", SortDirection.ASC)), + actual); + } + + @Test + void shouldReturnABadRequestWhenMixingRepeatedParameterAndCommaSeparatedValues() + throws Exception { + mockMvc + .perform( + get(ENDPOINT) + .accept("application/json") + .param("order", "field1:desc") + .param("order", "field2:asc,field3:desc")) + .andExpect(content().string(containsString("Bad Request"))) + .andExpect( + content() + .string(containsString("You might have repeated a request parameter and used"))); } @Test void shouldReturnABadRequestWhenInvalidSortDirectionIsPassedAsParameter() throws Exception { mockMvc - .perform(get(ENDPOINT).param("order", "field:wrong")) + .perform(get(ENDPOINT).accept("application/json").param("order", "field:wrong")) .andExpect(content().string(containsString("Bad Request"))) .andExpect( content() @@ -100,7 +138,11 @@ void shouldReturnABadRequestWhenInvalidSortDirectionIsPassedAsParameter() throws void shouldReturnABadRequestWhenInvalidSortDirectionIsPassedAsParameterInAListOfOrders() throws Exception { mockMvc - .perform(get(ENDPOINT).param("order", "field1:wrong").param("order", "field2:asc")) + .perform( + get(ENDPOINT) + .accept("application/json") + .param("order", "field1:wrong") + .param("order", "field2:asc")) .andExpect(content().string(containsString("Bad Request"))) .andExpect( content() @@ -110,9 +152,10 @@ void shouldReturnABadRequestWhenInvalidSortDirectionIsPassedAsParameterInAListOf } @Controller - private static class OrderingController extends CrudControllerAdvice { + private class OrderingController extends CrudControllerAdvice { @GetMapping(value = ENDPOINT) public @ResponseBody WebMessage getOrder(@RequestParam List order) { + actual = order; return ok(order.toString()); } }