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()); } }