From 8d56d25df5d94cd2d44ec9f853b6a0536c3c302b 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 --- .../event/webrequest/OrderCriteria.java | 2 +- .../controller/CrudControllerAdvice.java | 39 ++++- .../webapi/controller/OrderBindingTest.java | 160 +++++++++++++----- 3 files changed, 158 insertions(+), 43 deletions(-) diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/webapi/controller/event/webrequest/OrderCriteria.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/webapi/controller/event/webrequest/OrderCriteria.java index 8e73604e8f44..f60d1de5bd01 100644 --- a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/webapi/controller/event/webrequest/OrderCriteria.java +++ b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/webapi/controller/event/webrequest/OrderCriteria.java @@ -87,7 +87,7 @@ public static OrderCriteria valueOf(String input) { String field = props[0].trim(); if (StringUtils.isEmpty(field)) { throw new IllegalArgumentException( - "Invalid order property: '" + "Missing field name in order property: '" + input + "'. Valid formats are 'field:direction' or 'field'. Valid directions are 'asc' or 'desc'. Direction defaults to 'asc'."); } 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..371cca900fe1 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; @@ -214,8 +216,10 @@ public WebMessage methodArgumentTypeMismatchException(MethodArgumentTypeMismatch Class requiredType = ex.getRequiredType(); PathVariable pathVariableAnnotation = ex.getParameter().getParameterAnnotation(PathVariable.class); + String field = ex.getName(); + Object value = ex.getValue(); String notValidValueMessage = - getNotValidValueMessage(ex.getValue(), ex.getName(), pathVariableAnnotation != null); + getNotValidValueMessage(value, field, pathVariableAnnotation != null); String customErrorMessage; if (requiredType == null) { @@ -226,6 +230,9 @@ 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) { + notValidValueMessage = getConversionErrorMessage(value, field, conversionException); + customErrorMessage = ""; } else { customErrorMessage = getGenericFieldErrorMessage(requiredType.getSimpleName()); } @@ -237,7 +244,9 @@ public WebMessage methodArgumentTypeMismatchException(MethodArgumentTypeMismatch @ResponseBody public WebMessage handleTypeMismatchException(TypeMismatchException ex) { Class requiredType = ex.getRequiredType(); - String notValidValueMessage = getNotValidValueMessage(ex.getValue(), ex.getPropertyName()); + String field = ex.getPropertyName(); + Object value = ex.getValue(); + String notValidValueMessage = getNotValidValueMessage(value, field); String customErrorMessage; if (requiredType == null) { @@ -248,6 +257,9 @@ public WebMessage handleTypeMismatchException(TypeMismatchException ex) { customErrorMessage = getGenericFieldErrorMessage(requiredType.getSimpleName()); } else if (ex.getCause() instanceof IllegalArgumentException) { customErrorMessage = ex.getCause().getMessage(); + } else if (ex.getCause() instanceof ConversionFailedException conversionException) { + notValidValueMessage = getConversionErrorMessage(value, field, conversionException); + customErrorMessage = ""; } else { customErrorMessage = getGenericFieldErrorMessage(requiredType.getSimpleName()); } @@ -269,11 +281,12 @@ private String getGenericFieldErrorMessage(String fieldType) { return MessageFormat.format("It should be of type {0}", fieldType); } - private String getNotValidValueMessage(Object value, String field) { + private static String getNotValidValueMessage(Object value, String field) { return getNotValidValueMessage(value, field, false); } - private String getNotValidValueMessage(Object value, String field, boolean isPathVariable) { + private static String getNotValidValueMessage( + Object value, String field, boolean isPathVariable) { if (value == null || (value instanceof String stringValue && stringValue.isEmpty())) { return MessageFormat.format("{0} cannot be empty.", field); } @@ -288,9 +301,27 @@ private String getFormattedBadRequestMessage(Object value, String field, String } private String getFormattedBadRequestMessage(String fieldErrorMessage, String customMessage) { + if (StringUtils.isEmpty(customMessage)) { + return fieldErrorMessage; + } return fieldErrorMessage + " " + customMessage; } + private static String getConversionErrorMessage( + Object rootValue, String field, ConversionFailedException ex) { + Object invalidValue = ex.getValue(); + if (TypeDescriptor.valueOf(String.class).equals(ex.getSourceType()) + && (invalidValue != null && ((String) invalidValue).contains(",")) + && (rootValue != null && rootValue.getClass().isArray())) { + return "You likely repeated request parameter '" + + field + + "' and used multiple comma-separated values within at least one of its values. Choose one of these approaches. " + + ex.getCause().getMessage(); + } + + return getNotValidValueMessage(invalidValue, field) + " " + ex.getCause().getMessage(); + } + /** * 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..f00307a05f0a 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 @@ -27,16 +27,25 @@ */ package org.hisp.dhis.webapi.controller; -import static org.hamcrest.core.StringContains.containsString; import static org.hisp.dhis.dxf2.webmessage.WebMessageUtils.ok; +import static org.hisp.dhis.utils.Assertions.assertContains; +import static org.hisp.dhis.utils.Assertions.assertStartsWith; +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 lombok.Data; +import org.hisp.dhis.common.SortDirection; import org.hisp.dhis.dxf2.webmessage.WebMessage; +import org.hisp.dhis.jsontree.JsonMixed; import org.hisp.dhis.webapi.controller.event.webrequest.OrderCriteria; +import org.hisp.dhis.webapi.json.domain.JsonWebMessage; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.stereotype.Controller; import org.springframework.test.web.servlet.MockMvc; import org.springframework.test.web.servlet.setup.MockMvcBuilders; @@ -45,9 +54,10 @@ import org.springframework.web.bind.annotation.ResponseBody; class OrderBindingTest { - private static final String ENDPOINT = "/ordering"; - + private static final String METHOD_PARAMETER_ENDPOINT = "/order/methodParameter"; + private static final String CLASS_PARAMETER_ENDPOINT = "/order/classParameter"; private MockMvc mockMvc; + private List actual; @BeforeEach public void setUp() { @@ -60,60 +70,134 @@ 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"))); + .perform(get(METHOD_PARAMETER_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"))); + .perform(get(METHOD_PARAMETER_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(content().string(containsString("OK"))) - .andExpect(content().string(containsString("field"))) - .andExpect(content().string(containsString("DESC"))); + .perform(get(METHOD_PARAMETER_ENDPOINT).param("order", "field:desc")) + .andExpect(status().isOk()); + + assertEquals(List.of(OrderCriteria.of("field", SortDirection.DESC)), actual); } - @Test - void shouldReturnABadRequestWhenInvalidSortDirectionIsPassedAsParameter() throws Exception { + @ParameterizedTest + @ValueSource(strings = {METHOD_PARAMETER_ENDPOINT, CLASS_PARAMETER_ENDPOINT}) + void shouldHandleMultipleOrderComponentsGivenViaOneParameter(String endpoint) throws Exception { mockMvc - .perform(get(ENDPOINT).param("order", "field:wrong")) - .andExpect(content().string(containsString("Bad Request"))) - .andExpect( - content() - .string( - containsString( - "'wrong' is not a valid sort direction. Valid values are: [ASC, 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 shouldReturnABadRequestWhenInvalidSortDirectionIsPassedAsParameterInAListOfOrders() + @ParameterizedTest + @ValueSource(strings = {METHOD_PARAMETER_ENDPOINT, CLASS_PARAMETER_ENDPOINT}) + void shouldHandleMultipleOrderComponentsGivenViaMultipleParameters(String endpoint) throws Exception { mockMvc - .perform(get(ENDPOINT).param("order", "field1:wrong").param("order", "field2:asc")) - .andExpect(content().string(containsString("Bad Request"))) - .andExpect( - content() - .string( - containsString( - "'wrong' is not a valid sort direction. Valid values are: [ASC, DESC]"))); + .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); + } + + @ParameterizedTest + @ValueSource(strings = {METHOD_PARAMETER_ENDPOINT, CLASS_PARAMETER_ENDPOINT}) + void shouldReturnABadRequestWhenMixingRepeatedParameterAndCommaSeparatedValues(String endpoint) + throws Exception { + MockHttpServletResponse response = + mockMvc + .perform( + get(endpoint) + .accept("application/json") + .param("order", "field1:desc") + .param("order", "field2:asc,field3:desc")) + .andReturn() + .getResponse(); + + JsonWebMessage message = JsonMixed.of(response.getContentAsString()).as(JsonWebMessage.class); + assertEquals(400, message.getHttpStatusCode()); + assertStartsWith( + "You likely repeated request parameter 'order' and used", message.getMessage()); + } + + @ParameterizedTest + @ValueSource(strings = {METHOD_PARAMETER_ENDPOINT, CLASS_PARAMETER_ENDPOINT}) + void shouldReturnABadRequestWhenInvalidSortDirectionIsPassedAsParameter(String endpoint) + throws Exception { + MockHttpServletResponse response = + mockMvc + .perform(get(endpoint).accept("application/json").param("order", "field:wrong")) + .andReturn() + .getResponse(); + + JsonWebMessage message = JsonMixed.of(response.getContentAsString()).as(JsonWebMessage.class); + assertEquals(400, message.getHttpStatusCode()); + assertStartsWith( + "'wrong' is not a valid sort direction. Valid values are: [ASC,", message.getMessage()); + } + + @ParameterizedTest + @ValueSource(strings = {METHOD_PARAMETER_ENDPOINT, CLASS_PARAMETER_ENDPOINT}) + void shouldReturnABadRequestWhenInvalidSortDirectionIsPassedAsParameterInAListOfOrders( + String endpoint) throws Exception { + MockHttpServletResponse response = + mockMvc + .perform( + get(endpoint) + .accept("application/json") + .param("order", "field1:wrong") + .param("order", "field2:asc")) + .andReturn() + .getResponse(); + + JsonWebMessage message = JsonMixed.of(response.getContentAsString()).as(JsonWebMessage.class); + assertEquals(400, message.getHttpStatusCode()); + assertStartsWith("Value 'field1:wrong' is not valid for parameter order", message.getMessage()); + assertContains( + "'wrong' is not a valid sort direction. Valid values are: [ASC,", message.getMessage()); } @Controller - private static class OrderingController extends CrudControllerAdvice { - @GetMapping(value = ENDPOINT) - public @ResponseBody WebMessage getOrder(@RequestParam List order) { - return ok(order.toString()); + private class OrderingController extends CrudControllerAdvice { + @Data + static class RequestParams { + List order; + } + + @GetMapping(value = CLASS_PARAMETER_ENDPOINT) + public @ResponseBody WebMessage getOrderViaClassParameter(RequestParams params) { + actual = params.order; + return ok(); + } + + @GetMapping(value = METHOD_PARAMETER_ENDPOINT) + public @ResponseBody WebMessage getOrderViaMethodParameter( + @RequestParam List order) { + actual = order; + return ok(); } } }