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 225eeecddc25..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 @@ -46,9 +46,9 @@ @Value @AllArgsConstructor(staticName = "of") public class OrderCriteria { - private final String field; + String field; - private final SortDirection direction; + SortDirection direction; public OrderParam toOrderParam() { return new OrderParam(field, direction); @@ -64,18 +64,36 @@ public static List fromOrderString(String source) { private static List toOrderCriterias(String s) { return Arrays.stream(s.split(",")) - .map(OrderCriteria::toOrderCriteria) + .filter(StringUtils::isNotBlank) + .map(OrderCriteria::valueOf) .collect(Collectors.toList()); } - public static OrderCriteria toOrderCriteria(String s1) { - String[] props = s1.split(":"); - if (props.length == 2) { - return OrderCriteria.of(props[0], SortDirection.of(props[1].trim())); + /** + * Create an {@link OrderCriteria} from a string in the format of "field:direction". Valid + * directions are defined by {@link SortDirection}. + * + * @throws IllegalArgumentException if the input is not in the correct format. + */ + public static OrderCriteria valueOf(String input) { + String[] props = input.split(":"); + if (props.length > 2) { + throw new IllegalArgumentException( + "Invalid order property: '" + + input + + "'. Valid formats are 'field:direction' or 'field'. Valid directions are 'asc' or 'desc'. Direction defaults to 'asc'."); } - if (props.length == 1) { - return OrderCriteria.of(props[0], SortDirection.ASC); + + String field = props[0].trim(); + if (StringUtils.isEmpty(field)) { + throw new IllegalArgumentException( + "Missing field name in order property: '" + + input + + "'. Valid formats are 'field:direction' or 'field'. Valid directions are 'asc' or 'desc'. Direction defaults to 'asc'."); } - return null; + + SortDirection direction = + props.length == 1 ? SortDirection.ASC : SortDirection.of(props[1].trim()); + return OrderCriteria.of(field, direction); } } diff --git a/dhis-2/dhis-api/src/test/java/org/hisp/dhis/webapi/controller/event/webrequest/OrderCriteriaTest.java b/dhis-2/dhis-api/src/test/java/org/hisp/dhis/webapi/controller/event/webrequest/OrderCriteriaTest.java index bead3fb09dfc..ee4c9862d947 100644 --- a/dhis-2/dhis-api/src/test/java/org/hisp/dhis/webapi/controller/event/webrequest/OrderCriteriaTest.java +++ b/dhis-2/dhis-api/src/test/java/org/hisp/dhis/webapi/controller/event/webrequest/OrderCriteriaTest.java @@ -29,7 +29,6 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import java.util.List; @@ -42,50 +41,38 @@ class OrderCriteriaTest { @Test void fromOrderString() { + List order = OrderCriteria.fromOrderString("one:desc,, two:asc ,three "); - List orderCriteria = - OrderCriteria.fromOrderString("one:desc,, two:asc ,three "); - - assertNotNull(orderCriteria); assertEquals( - 4, orderCriteria.size(), String.format("Expected 4 item, instead got %s", orderCriteria)); - assertEquals(OrderCriteria.of("one", SortDirection.DESC), orderCriteria.get(0)); - assertEquals(OrderCriteria.of("", SortDirection.ASC), orderCriteria.get(1)); - assertEquals(OrderCriteria.of(" two", SortDirection.ASC), orderCriteria.get(2)); - assertEquals(OrderCriteria.of("three", SortDirection.ASC), orderCriteria.get(3)); + List.of( + OrderCriteria.of("one", SortDirection.DESC), + OrderCriteria.of("two", SortDirection.ASC), + OrderCriteria.of("three", SortDirection.ASC)), + order); } @ValueSource(strings = {"one:desc", "one:Desc", "one:DESC"}) @ParameterizedTest void fromOrderStringSortDirectionParsingIsCaseInsensitive(String source) { + List order = OrderCriteria.fromOrderString(source); - List orderCriteria = OrderCriteria.fromOrderString(source); - - assertNotNull(orderCriteria); - assertEquals( - 1, orderCriteria.size(), String.format("Expected 1 item, instead got %s", orderCriteria)); - assertEquals(OrderCriteria.of("one", SortDirection.DESC), orderCriteria.get(0)); + assertEquals(List.of(OrderCriteria.of("one", SortDirection.DESC)), order); } @Test void fromOrderStringDefaultsToAscGivenFieldAndColon() { + List order = OrderCriteria.fromOrderString("one:"); - List orderCriteria = OrderCriteria.fromOrderString("one:"); - - assertNotNull(orderCriteria); - assertEquals( - 1, orderCriteria.size(), String.format("Expected 1 item, instead got %s", orderCriteria)); - assertEquals(OrderCriteria.of("one", SortDirection.ASC), orderCriteria.get(0)); + assertEquals(List.of(OrderCriteria.of("one", SortDirection.ASC)), order); } @Test - void fromOrderStringDefaultsSortDirectionToAscGivenAnUnknownSortDirection() { + void failGivenAnUnknownSortDirection() { assertThrows(IllegalArgumentException.class, () -> OrderCriteria.fromOrderString("one:wrong")); } @Test void fromOrderStringReturnsEmptyListGivenEmptyOrder() { - List orderCriteria = OrderCriteria.fromOrderString(" "); assertNotNull(orderCriteria); @@ -94,13 +81,13 @@ void fromOrderStringReturnsEmptyListGivenEmptyOrder() { } @Test - void fromOrderStringReturnsNullGivenOrderWithMoreThanTwoProperties() { - - List orderCriteria = OrderCriteria.fromOrderString("one:desc:wrong"); + void failGivenMoreThanTwoColons() { + assertThrows( + IllegalArgumentException.class, () -> OrderCriteria.fromOrderString("one:desc:wrong")); + } - assertNotNull(orderCriteria); - assertEquals( - 1, orderCriteria.size(), String.format("Expected 1 item, instead got %s", orderCriteria)); - assertNull(orderCriteria.get(0)); + @Test + void failGivenEmptyField() { + assertThrows(IllegalArgumentException.class, () -> OrderCriteria.valueOf(" :desc")); } } diff --git a/dhis-2/dhis-test-e2e/src/test/resources/tracker/workinglists/trackedEntityFilters.json b/dhis-2/dhis-test-e2e/src/test/resources/tracker/workinglists/trackedEntityFilters.json index 5ce7817f73a7..0a3c468f3aee 100644 --- a/dhis-2/dhis-test-e2e/src/test/resources/tracker/workinglists/trackedEntityFilters.json +++ b/dhis-2/dhis-test-e2e/src/test/resources/tracker/workinglists/trackedEntityFilters.json @@ -1,75 +1,83 @@ { - "id": "ctZIHTIIlYS", - "name": "TA Tracker program filter", - "program": { - "id": "f1AyMswryyQ" + "id": "ctZIHTIIlYS", + "name": "TA Tracker program filter", + "program": { + "id": "f1AyMswryyQ" + }, + "sharing": { + "public": "rw------", + "userGroups": { + "OPVIvvXzNTw": { + "access": "rw------", + "id": "OPVIvvXzNTw" + } + } + }, + "entityQueryCriteria": { + "enrollmentStatus": "COMPLETED", + "followUp": true, + "ouMode": "ACCESSIBLE", + "displayColumnOrder": [ + "eventDate", + "dueDate", + "program", + "invalid" + ], + "assignedUserMode": "ANY", + "programStage": "a3kGcGDCuk6", + "trackedEntityType": "Q9GufDoplCL", + "lastUpdatedDate": { + "period": "TODAY", + "startBuffer": -5, + "endBuffer": 5, + "type": "RELATIVE" }, - "sharing": { - "public": "rw------", - "userGroups": { - "OPVIvvXzNTw": { - "access": "rw------", - "id": "OPVIvvXzNTw" - } - } + "enrollmentCreatedDate": { + "period": "TODAY", + "startBuffer": -5, + "endBuffer": 5, + "type": "RELATIVE" + }, + "enrollmentIncidentDate": { + "endDate": "2019-03-20T00:00:00.000", + "type": "ABSOLUTE", + "startDate": "2014-05-01T00:00:00.000" }, - "entityQueryCriteria": { - "enrollmentStatus": "COMPLETED", - "followUp": true, - "ouMode": "ACCESSIBLE", - "displayColumnOrder": [ "eventDate", "dueDate", "program", "invalid"], - "assignedUserMode": "ANY", - "programStage":"a3kGcGDCuk6", - "trackedEntityType":"Q9GufDoplCL", - "lastUpdatedDate": { - "period": "TODAY", - "startBuffer": -5, - "endBuffer": 5, - "type": "RELATIVE" - }, - "enrollmentCreatedDate": { - "period": "TODAY", - "startBuffer": -5, - "endBuffer": 5, - "type": "RELATIVE" - }, - "enrollmentIncidentDate": { - "endDate": "2019-03-20T00:00:00.000", - "type": "ABSOLUTE", - "startDate": "2014-05-01T00:00:00.000" - }, - "trackedEntityInstances": ["a3kGcGDCuk7", "a3kGcGDCuk8"], - "order": "createdAt:desc;orgUnit:asc", - "attributeValueFilters": [ - { - "attribute": "dIVt4l5vIOa", - "sw": "a", - "ew": "e" - }, - { - "attribute": "kZeSYCgaHTk", - "like": "abc" - }, - { - "attribute": "x5yfLot5VCM", - "dateFilter": { - "startDate": "2014-05-01T00:00:00.000", - "endDate": "2019-03-20T00:00:00.000", - "type": "ABSOLUTE" - } - }, - { - "attribute": "ypGAwVRNtVY", - "le": "20", - "ge": "10" - }, - { - "attribute": "aIga5mPOFOJ", - "in": [ - "MALE", - "FEMALE" - ] - } + "trackedEntityInstances": [ + "a3kGcGDCuk7", + "a3kGcGDCuk8" + ], + "order": "createdAt:desc", + "attributeValueFilters": [ + { + "attribute": "dIVt4l5vIOa", + "sw": "a", + "ew": "e" + }, + { + "attribute": "kZeSYCgaHTk", + "like": "abc" + }, + { + "attribute": "x5yfLot5VCM", + "dateFilter": { + "startDate": "2014-05-01T00:00:00.000", + "endDate": "2019-03-20T00:00:00.000", + "type": "ABSOLUTE" + } + }, + { + "attribute": "ypGAwVRNtVY", + "le": "20", + "ge": "10" + }, + { + "attribute": "aIga5mPOFOJ", + "in": [ + "MALE", + "FEMALE" ] - } -} \ No newline at end of file + } + ] + } +} diff --git a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/common/OrderCriteriaParamEditor.java b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/common/OrderCriteriaParamEditor.java deleted file mode 100644 index e3eb8cd6bf3f..000000000000 --- a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/common/OrderCriteriaParamEditor.java +++ /dev/null @@ -1,38 +0,0 @@ -/* - * Copyright (c) 2004-2023, University of Oslo - * All rights reserved. - * - * Redistribution and use in source and binary forms, with or without - * modification, are permitted provided that the following conditions are met: - * Redistributions of source code must retain the above copyright notice, this - * list of conditions and the following disclaimer. - * - * Redistributions in binary form must reproduce the above copyright notice, - * this list of conditions and the following disclaimer in the documentation - * and/or other materials provided with the distribution. - * Neither the name of the HISP project nor the names of its contributors may - * be used to endorse or promote products derived from this software without - * specific prior written permission. - * - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND - * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED - * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE - * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR - * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES - * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; - * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON - * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT - * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS - * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - */ -package org.hisp.dhis.webapi.common; - -import java.beans.PropertyEditorSupport; -import org.hisp.dhis.webapi.controller.event.webrequest.OrderCriteria; - -public class OrderCriteriaParamEditor extends PropertyEditorSupport { - @Override - public void setAsText(String source) { - setValue(OrderCriteria.toOrderCriteria(source)); - } -} diff --git a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/common/UIDParamEditor.java b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/common/UIDParamEditor.java deleted file mode 100644 index ea84d67a44ee..000000000000 --- a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/common/UIDParamEditor.java +++ /dev/null @@ -1,38 +0,0 @@ -/* - * Copyright (c) 2004-2023, University of Oslo - * All rights reserved. - * - * Redistribution and use in source and binary forms, with or without - * modification, are permitted provided that the following conditions are met: - * Redistributions of source code must retain the above copyright notice, this - * list of conditions and the following disclaimer. - * - * Redistributions in binary form must reproduce the above copyright notice, - * this list of conditions and the following disclaimer in the documentation - * and/or other materials provided with the distribution. - * Neither the name of the HISP project nor the names of its contributors may - * be used to endorse or promote products derived from this software without - * specific prior written permission. - * - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND - * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED - * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE - * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR - * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES - * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; - * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON - * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT - * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS - * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - */ -package org.hisp.dhis.webapi.common; - -import java.beans.PropertyEditorSupport; -import org.hisp.dhis.common.UID; - -public class UIDParamEditor extends PropertyEditorSupport { - @Override - public void setAsText(String source) { - setValue(UID.of(source)); - } -} 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 de8ff1a2d752..fcb92eb50085 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 @@ -79,9 +79,6 @@ import org.hisp.dhis.security.spring2fa.TwoFactorAuthenticationException; import org.hisp.dhis.tracker.imports.TrackerIdSchemeParam; import org.hisp.dhis.util.DateUtils; -import org.hisp.dhis.webapi.common.OrderCriteriaParamEditor; -import org.hisp.dhis.webapi.common.UIDParamEditor; -import org.hisp.dhis.webapi.controller.event.webrequest.OrderCriteria; import org.hisp.dhis.webapi.controller.exception.MetadataImportConflictException; import org.hisp.dhis.webapi.controller.exception.MetadataSyncException; import org.hisp.dhis.webapi.controller.exception.MetadataVersionException; @@ -90,6 +87,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; @@ -151,8 +150,6 @@ protected void initBinder(WebDataBinder binder) { IdentifiableProperty.class, new FromTextPropertyEditor(String::toUpperCase)); this.enumClasses.forEach(c -> binder.registerCustomEditor(c, new ConvertEnum(c))); binder.registerCustomEditor(TrackerIdSchemeParam.class, new IdSchemeParamEditor()); - binder.registerCustomEditor(OrderCriteria.class, new OrderCriteriaParamEditor()); - binder.registerCustomEditor(UID.class, new UIDParamEditor()); } @ExceptionHandler @@ -217,8 +214,10 @@ public WebMessage methodArgumentTypeMismatchException(MethodArgumentTypeMismatch Class requiredType = ex.getRequiredType(); PathVariable pathVariableAnnotation = ex.getParameter().getParameterAnnotation(PathVariable.class); - String notValidValueMessage = - getNotValidValueMessage(ex.getValue(), ex.getName(), pathVariableAnnotation != null); + String field = ex.getName(); + Object value = ex.getValue(); + boolean isPathVariable = pathVariableAnnotation != null; + String notValidValueMessage = getNotValidValueMessage(value, field, isPathVariable); String customErrorMessage; if (requiredType == null) { @@ -229,6 +228,10 @@ 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, isPathVariable); + customErrorMessage = ""; } else { customErrorMessage = getGenericFieldErrorMessage(requiredType.getSimpleName()); } @@ -240,7 +243,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) { @@ -251,6 +256,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, false); + customErrorMessage = ""; } else { customErrorMessage = getGenericFieldErrorMessage(requiredType.getSimpleName()); } @@ -272,11 +280,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); } @@ -291,9 +300,29 @@ 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, boolean isPathVariable) { + 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, isPathVariable) + + " " + + 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/common/UIDBindingTest.java b/dhis-2/dhis-web-api/src/test/java/org/hisp/dhis/webapi/common/UIDBindingTest.java index 1dd0cb4f42e1..c640defa4aff 100644 --- a/dhis-2/dhis-web-api/src/test/java/org/hisp/dhis/webapi/common/UIDBindingTest.java +++ b/dhis-2/dhis-web-api/src/test/java/org/hisp/dhis/webapi/common/UIDBindingTest.java @@ -27,15 +27,21 @@ */ package org.hisp.dhis.webapi.common; -import static org.hamcrest.Matchers.containsString; +import static org.hisp.dhis.dxf2.webmessage.WebMessageUtils.ok; +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 org.hisp.dhis.common.UID; +import org.hisp.dhis.dxf2.webmessage.WebMessage; +import org.hisp.dhis.jsontree.JsonMixed; import org.hisp.dhis.webapi.controller.CrudControllerAdvice; +import org.hisp.dhis.webapi.json.domain.JsonWebMessage; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +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; @@ -46,7 +52,8 @@ class UIDBindingTest { - private static final String VALID_UID = "bRNvL6NMQXb"; + private static final String VALID_UID_STRING = "bRNvL6NMQXb"; + private static final UID VALID_UID = UID.of("bRNvL6NMQXb"); private static final String INVALID_UID = "invalidUid"; @@ -57,10 +64,13 @@ class UIDBindingTest { "Value 'invalidUid' is not valid for path parameter uid. UID must be an alphanumeric string of 11 characters"; private static final String BINDING_OBJECT_ERROR_MESSAGE = - "Value 'invalidUid' is not valid for parameter uidInRequestParams. UID must be an alphanumeric string of 11 characters"; + "Value 'invalidUid' is not valid for parameter uid. UID must be an alphanumeric string of 11 characters"; private static final String ENDPOINT = "/uid"; + private UID actual; + private List actualCollection; + private MockMvc mockMvc; @BeforeEach @@ -73,66 +83,130 @@ public void setUp() { @Test void shouldReturnUIDValueWhenPassingValidUidAsPathVariable() throws Exception { - mockMvc - .perform(get(ENDPOINT + "/" + VALID_UID)) - .andExpect(status().isOk()) - .andExpect(content().string(VALID_UID)); + mockMvc.perform(get(ENDPOINT + "/" + VALID_UID)).andExpect(status().isOk()); + + assertEquals(VALID_UID, actual); } @Test void shouldReturnUIDValueWhenPassingValidUidAsRequestParam() throws Exception { - mockMvc - .perform(get(ENDPOINT).param("uid", VALID_UID)) - .andExpect(status().isOk()) - .andExpect(content().string(VALID_UID)); + mockMvc.perform(get(ENDPOINT).param("uid", VALID_UID_STRING)).andExpect(status().isOk()); + + assertEquals(VALID_UID, actual); } @Test void shouldReturnUIDValueWhenPassingValidUidAsRequestParamObject() throws Exception { mockMvc - .perform(get(ENDPOINT + "/params").param("uidInRequestParams", VALID_UID)) - .andExpect(status().isOk()) - .andExpect(content().string(VALID_UID)); + .perform(get(ENDPOINT + "/params").param("uid", VALID_UID_STRING)) + .andExpect(status().isOk()); + + assertEquals(VALID_UID, actual); } @Test void shouldReturnBadRequestResponseWhenPassingInvalidUidAsPathVariable() throws Exception { - mockMvc - .perform(get(ENDPOINT + "/" + INVALID_UID)) - .andExpect(content().string(containsString(ERROR_MESSAGE_FOR_PATH_PARAM))); + MockHttpServletResponse response = + mockMvc + .perform(get(ENDPOINT + "/" + INVALID_UID).accept("application/json")) + .andReturn() + .getResponse(); + + JsonWebMessage message = JsonMixed.of(response.getContentAsString()).as(JsonWebMessage.class); + assertEquals(400, message.getHttpStatusCode()); + assertStartsWith(ERROR_MESSAGE_FOR_PATH_PARAM, message.getMessage()); } @Test void shouldReturnBadRequestResponseWhenPassingInvalidUidAsRequestParam() throws Exception { - mockMvc - .perform(get(ENDPOINT).param("uid", INVALID_UID)) - .andExpect(content().string(containsString(ERROR_MESSAGE))); + MockHttpServletResponse response = + mockMvc + .perform(get(ENDPOINT).accept("application/json").param("uid", INVALID_UID)) + .andReturn() + .getResponse(); + + JsonWebMessage message = JsonMixed.of(response.getContentAsString()).as(JsonWebMessage.class); + assertEquals(400, message.getHttpStatusCode()); + assertStartsWith(ERROR_MESSAGE, message.getMessage()); } @Test void shouldReturnBadRequestResponseWhenPassingInvalidUidAsRequestParamObject() throws Exception { + MockHttpServletResponse response = + mockMvc + .perform(get(ENDPOINT + "/params").accept("application/json").param("uid", INVALID_UID)) + .andReturn() + .getResponse(); + + JsonWebMessage message = JsonMixed.of(response.getContentAsString()).as(JsonWebMessage.class); + assertEquals(400, message.getHttpStatusCode()); + assertStartsWith(BINDING_OBJECT_ERROR_MESSAGE, message.getMessage()); + } + + @Test + void shouldHandleMultipleOrderComponentsGivenViaOneParameter() throws Exception { mockMvc - .perform(get(ENDPOINT + "/params").param("uidInRequestParams", INVALID_UID)) - .andExpect(content().string(containsString(BINDING_OBJECT_ERROR_MESSAGE))); + .perform(get(ENDPOINT + "/collection").param("uids", "PHB3x6n374I,Sq3QLSHij67")) + .andExpect(status().isOk()); + + assertEquals(List.of(UID.of("PHB3x6n374I"), UID.of("Sq3QLSHij67")), actualCollection); + } + + @Test + void shouldHandleMultipleOrderComponentsGivenViaMultipleParameters() throws Exception { + mockMvc + .perform( + get(ENDPOINT + "/collection").param("uids", "PHB3x6n374I").param("uids", "Sq3QLSHij67")) + .andExpect(status().isOk()); + + assertEquals(List.of(UID.of("PHB3x6n374I"), UID.of("Sq3QLSHij67")), actualCollection); + } + + @Test + void shouldReturnABadRequestWhenMixingRepeatedParameterAndCommaSeparatedValues() + throws Exception { + MockHttpServletResponse response = + mockMvc + .perform( + get(ENDPOINT + "/collection") + .accept("application/json") + .param("uids", "PHB3x6n374I") + .param("uids", "Sq3QLSHij67,jEVhr5j6EaA")) + .andReturn() + .getResponse(); + + JsonWebMessage message = JsonMixed.of(response.getContentAsString()).as(JsonWebMessage.class); + assertEquals(400, message.getHttpStatusCode()); + assertStartsWith("You likely repeated request parameter 'uids' and used", message.getMessage()); } @Controller - private static class UIDController extends CrudControllerAdvice { + private class UIDController extends CrudControllerAdvice { @GetMapping(value = ENDPOINT + "/{uid}") - public @ResponseBody String getUIDValueFromPath(@PathVariable UID uid) { - return uid.getValue(); + public @ResponseBody WebMessage getUIDValueFromPath(@PathVariable UID uid) { + actual = uid; + return ok(); } @GetMapping(value = ENDPOINT) - public @ResponseBody String getUIDValueFromRequestParam(@RequestParam UID uid) { - return uid.getValue(); + public @ResponseBody WebMessage getUIDValueFromRequestParam(@RequestParam UID uid) { + actual = uid; + return ok(); + } + + @GetMapping(value = ENDPOINT + "/collection") + public @ResponseBody WebMessage getUIDValueFromRequestParamCollection( + @RequestParam List uids) { + actualCollection = uids; + return ok(); } @GetMapping(value = ENDPOINT + "/params") - public @ResponseBody String getUIDValueFromRequestObject(UIDRequestParams uidRequestParams) { - return uidRequestParams.uidInRequestParams().getValue(); + public @ResponseBody WebMessage getUIDValueFromRequestObject(UIDRequestParams requestParams) { + actual = requestParams.uid; + return ok(); } } - private record UIDRequestParams(UID uidInRequestParams) {} + private record UIDRequestParams(UID uid) {} } 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..ff3d43a1ec16 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()); + assertContains( + "'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(); } } }