From ed11497d02be24175650793df7d1b5cee4d1f12c Mon Sep 17 00:00:00 2001 From: teleivo Date: Mon, 8 Jan 2024 15:17:02 +0100 Subject: [PATCH] fix: /tracker/relationships?order=createdAt (#16100) In https://dhis2.atlassian.net/browse/TECH-1619 we harmonized ordering relationships in 2.41. This now allows ordering by createdAt as the only field. Any other field will be rejected with a 400 like in 2.41. 2.41 also added ordering by createdAtClient which will not be backported. --- .../PagingAndSortingCriteriaAdapter.java | 9 ++ .../org/hisp/dhis/webapi/MvcTestConfig.java | 2 + .../controller/tracker/JsonRelationship.java | 4 + ...ckerRelationshipsExportControllerTest.java | 12 ++ .../export/RequestParamsValidator.java | 131 ++++++++++++++++++ .../TrackerRelationshipsExportController.java | 38 +++++ .../StringToOrderCriteriaListConverter.java | 2 +- .../export/RequestParamsValidatorTest.java | 115 +++++++++++++++ 8 files changed, 312 insertions(+), 1 deletion(-) create mode 100644 dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/tracker/export/RequestParamsValidator.java create mode 100644 dhis-2/dhis-web-api/src/test/java/org/hisp/dhis/webapi/controller/tracker/export/RequestParamsValidatorTest.java diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/webapi/controller/event/webrequest/PagingAndSortingCriteriaAdapter.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/webapi/controller/event/webrequest/PagingAndSortingCriteriaAdapter.java index c0e484271d55..1f4f0ec175fa 100644 --- a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/webapi/controller/event/webrequest/PagingAndSortingCriteriaAdapter.java +++ b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/webapi/controller/event/webrequest/PagingAndSortingCriteriaAdapter.java @@ -107,6 +107,15 @@ public List getOrder() { return dtoNameToDatabaseNameTranslator.apply(orderCriteriaPartitionedByAllowance.get(true)); } + /** + * Get the raw {@link #order} field, without any field name translation. Added the method to + * validate the user provided order fields without affecting code relying on the existing behavior + * of {@link #getOrder()}. + */ + public List getRawOrder() { + return order; + } + private boolean isAllowed(OrderCriteria orderCriteria) { return getAllowedOrderingFields().contains(orderCriteria.getField()); } diff --git a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/MvcTestConfig.java b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/MvcTestConfig.java index 02f490aef67a..528a917fa6aa 100644 --- a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/MvcTestConfig.java +++ b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/MvcTestConfig.java @@ -52,6 +52,7 @@ import org.hisp.dhis.webapi.mvc.messageconverter.JsonMessageConverter; import org.hisp.dhis.webapi.mvc.messageconverter.XmlDeprecationNoticeHttpMessageConverter; import org.hisp.dhis.webapi.mvc.messageconverter.XmlMessageConverter; +import org.hisp.dhis.webapi.security.config.StringToOrderCriteriaListConverter; import org.hisp.dhis.webapi.view.CustomPathExtensionContentNegotiationStrategy; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Qualifier; @@ -201,6 +202,7 @@ public void configureMessageConverters(List> converters) @Override public void addFormatters(FormatterRegistry registry) { + registry.addConverter(new StringToOrderCriteriaListConverter()); registry.addConverter(new FieldPathConverter()); } diff --git a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/tracker/JsonRelationship.java b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/tracker/JsonRelationship.java index 62e78a86cb52..fec226fe39b4 100644 --- a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/tracker/JsonRelationship.java +++ b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/tracker/JsonRelationship.java @@ -43,6 +43,10 @@ default String getRelationshipType() { return getString("relationshipType").string(); } + default String getCreatedAt() { + return getString("createdAt").string(); + } + default JsonRelationshipItem getFrom() { return get("from").as(JsonRelationshipItem.class); } diff --git a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/tracker/export/TrackerRelationshipsExportControllerTest.java b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/tracker/export/TrackerRelationshipsExportControllerTest.java index d3500d766e68..5103d1b41f01 100644 --- a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/tracker/export/TrackerRelationshipsExportControllerTest.java +++ b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/tracker/export/TrackerRelationshipsExportControllerTest.java @@ -473,6 +473,18 @@ void getRelationshipsByTrackedEntityWithEnrollments() { assertEquals(to.getUid(), enrollment.getTrackedEntity()); } + @Test + void getRelationshipsByTrackedEntityWithEnrollmentsOrderByIllegalField() { + TrackedEntityInstance to = trackedEntityInstance(); + ProgramInstance from = programInstance(to); + relationship(from, to); + + GET( + "/tracker/relationships?trackedEntity={tei}&order=created&fields=relationship,createdAt", + to.getUid()) + .content(HttpStatus.BAD_REQUEST); + } + @Test void getRelationshipsByTrackedEntityWithAttributes() { TrackedEntityInstance to = trackedEntityInstance(orgUnit); diff --git a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/tracker/export/RequestParamsValidator.java b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/tracker/export/RequestParamsValidator.java new file mode 100644 index 000000000000..a31856cc1a65 --- /dev/null +++ b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/tracker/export/RequestParamsValidator.java @@ -0,0 +1,131 @@ +/* + * Copyright (c) 2004-2022, 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.controller.tracker.export; + +import java.util.List; +import java.util.Map.Entry; +import java.util.Set; +import java.util.function.Function; +import java.util.stream.Collectors; +import org.hisp.dhis.common.CodeGenerator; +import org.hisp.dhis.webapi.controller.event.webrequest.OrderCriteria; +import org.hisp.dhis.webapi.controller.exception.BadRequestException; + +/** + * RequestParamUtils are functions used to parse and transform tracker request parameters. This + * class is intended to only house functions without any dependencies on services or components. + */ +class RequestParamsValidator { + private RequestParamsValidator() { + throw new IllegalStateException("Utility class"); + } + + /** + * Validate the {@code order} request parameter in tracker exporters. Allowed order values are + * {@code supportedFieldNames} and UIDs which represent {@code uidMeaning}. Every field name or + * UID can be specified at most once. + */ + public static void validateOrderParams( + List order, Set supportedFieldNames, String uidMeaning) + throws BadRequestException { + if (order == null || order.isEmpty()) { + return; + } + + Set invalidOrderComponents = + order.stream().map(OrderCriteria::getField).collect(Collectors.toSet()); + invalidOrderComponents.removeAll(supportedFieldNames); + Set uids = + invalidOrderComponents.stream() + .filter(CodeGenerator::isValidUid) + .collect(Collectors.toSet()); + invalidOrderComponents.removeAll(uids); + + String errorSuffix = + String.format( + "Supported are %s UIDs and fields '%s'. All of which can at most be specified once.", + uidMeaning, supportedFieldNames.stream().sorted().collect(Collectors.joining(", "))); + if (!invalidOrderComponents.isEmpty()) { + throw new BadRequestException( + String.format( + "order parameter is invalid. Cannot order by '%s'. %s", + String.join(", ", invalidOrderComponents), errorSuffix)); + } + + validateOrderParamsContainNoDuplicates(order, errorSuffix); + } + + private static void validateOrderParamsContainNoDuplicates( + List order, String errorSuffix) throws BadRequestException { + Set duplicateOrderComponents = + order.stream() + .map(OrderCriteria::getField) + .collect(Collectors.groupingBy(Function.identity(), Collectors.counting())) + .entrySet() + .stream() + .filter(e -> e.getValue() > 1) + .map(Entry::getKey) + .collect(Collectors.toSet()); + + if (!duplicateOrderComponents.isEmpty()) { + throw new BadRequestException( + String.format( + "order parameter is invalid. '%s' are repeated. %s", + String.join(", ", duplicateOrderComponents), errorSuffix)); + } + } + + /** + * Validate the {@code order} request parameter in tracker exporters. Allowed order values are + * {@code supportedFieldNames}. Every field name can be specified at most once. If the endpoint + * supports field names and UIDs use {@link #validateOrderParams(List, Set, String)}. + */ + public static void validateOrderParams(List order, Set supportedFieldNames) + throws BadRequestException { + if (order == null || order.isEmpty()) { + return; + } + + Set invalidOrderComponents = + order.stream().map(OrderCriteria::getField).collect(Collectors.toSet()); + invalidOrderComponents.removeAll(supportedFieldNames); + + String errorSuffix = + String.format( + "Supported fields are '%s'. All of which can at most be specified once.", + supportedFieldNames.stream().sorted().collect(Collectors.joining(", "))); + if (!invalidOrderComponents.isEmpty()) { + throw new BadRequestException( + String.format( + "order parameter is invalid. Cannot order by '%s'. %s", + String.join(", ", invalidOrderComponents), errorSuffix)); + } + + validateOrderParamsContainNoDuplicates(order, errorSuffix); + } +} diff --git a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/tracker/export/TrackerRelationshipsExportController.java b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/tracker/export/TrackerRelationshipsExportController.java index 5805b7592ac2..8d27d639d22b 100644 --- a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/tracker/export/TrackerRelationshipsExportController.java +++ b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/tracker/export/TrackerRelationshipsExportController.java @@ -27,16 +27,20 @@ */ package org.hisp.dhis.webapi.controller.tracker.export; +import static java.util.Map.entry; import static org.hisp.dhis.dxf2.webmessage.WebMessageUtils.notFound; import static org.hisp.dhis.webapi.controller.tracker.TrackerControllerSupport.RESOURCE_PATH; +import static org.hisp.dhis.webapi.controller.tracker.export.RequestParamsValidator.validateOrderParams; import static org.springframework.http.MediaType.APPLICATION_JSON_VALUE; import com.fasterxml.jackson.databind.node.ObjectNode; import com.google.common.collect.ImmutableMap; +import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Set; import java.util.function.BiFunction; import java.util.function.Function; import java.util.function.Supplier; @@ -58,6 +62,7 @@ import org.hisp.dhis.program.ProgramStageInstanceService; import org.hisp.dhis.trackedentity.TrackedEntityInstance; import org.hisp.dhis.trackedentity.TrackedEntityInstanceService; +import org.hisp.dhis.webapi.controller.event.webrequest.OrderCriteria; import org.hisp.dhis.webapi.controller.event.webrequest.PagingAndSortingCriteriaAdapter; import org.hisp.dhis.webapi.controller.event.webrequest.PagingWrapper; import org.hisp.dhis.webapi.controller.event.webrequest.tracker.TrackerRelationshipCriteria; @@ -80,6 +85,15 @@ public class TrackerRelationshipsExportController { protected static final String RELATIONSHIPS = "relationships"; + /** + * Relationships can be ordered by given fields which correspond to fields on {@link + * org.hisp.dhis.relationship.Relationship}. + */ + private static final Map ORDERABLE_FIELDS = + Map.ofEntries(entry("createdAt", "created")); + + private static final Set ORDERABLE_FIELD_NAMES = ORDERABLE_FIELDS.keySet(); + private static final String DEFAULT_FIELDS_PARAM = "relationship,relationshipType,from[trackedEntity[trackedEntity],enrollment[enrollment],event[event]],to[trackedEntity[trackedEntity],enrollment[enrollment],event[event]]"; @@ -191,6 +205,16 @@ private List tryGetRelationshipFrom( Class type, Supplier notFoundMessageSupplier, PagingAndSortingCriteriaAdapter pagingAndSortingCriteria) { + + if (pagingAndSortingCriteria != null) { + // validate and translate from user facing field names to internal field names users can order + // by avoiding field translation by PagingAndSortingCriteriaAdapter to validate user order + // fields and to not affect old tracker. + validateOrderParams(pagingAndSortingCriteria.getRawOrder(), ORDERABLE_FIELD_NAMES); + List orders = mapOrderParam(pagingAndSortingCriteria.getRawOrder()); + pagingAndSortingCriteria.setOrder(orders); + } + if (identifier != null) { Object object = getObjectRetriever(type).apply(identifier); if (object != null) { @@ -219,4 +243,18 @@ private List tryGetRelationshipFrom( .orElseThrow( () -> new IllegalArgumentException("Unable to detect object retriever from " + type)); } + + private static List mapOrderParam(List orders) { + if (orders == null || orders.isEmpty()) { + return List.of(); + } + + List result = new ArrayList<>(); + for (OrderCriteria order : orders) { + if (ORDERABLE_FIELDS.containsKey(order.getField())) { + result.add(OrderCriteria.of(ORDERABLE_FIELDS.get(order.getField()), order.getDirection())); + } + } + return result; + } } diff --git a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/security/config/StringToOrderCriteriaListConverter.java b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/security/config/StringToOrderCriteriaListConverter.java index 1f1bccb01dd9..8c0191aad41c 100644 --- a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/security/config/StringToOrderCriteriaListConverter.java +++ b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/security/config/StringToOrderCriteriaListConverter.java @@ -36,7 +36,7 @@ * * @author Giuseppe Nespolino */ -class StringToOrderCriteriaListConverter implements Converter> { +public class StringToOrderCriteriaListConverter implements Converter> { @Override public List convert(String source) { return OrderCriteria.fromOrderString(source); diff --git a/dhis-2/dhis-web-api/src/test/java/org/hisp/dhis/webapi/controller/tracker/export/RequestParamsValidatorTest.java b/dhis-2/dhis-web-api/src/test/java/org/hisp/dhis/webapi/controller/tracker/export/RequestParamsValidatorTest.java new file mode 100644 index 000000000000..969a4d8d99c5 --- /dev/null +++ b/dhis-2/dhis-web-api/src/test/java/org/hisp/dhis/webapi/controller/tracker/export/RequestParamsValidatorTest.java @@ -0,0 +1,115 @@ +/* + * Copyright (c) 2004-2022, 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.controller.tracker.export; + +import static org.hisp.dhis.utils.Assertions.assertContains; +import static org.hisp.dhis.utils.Assertions.assertStartsWith; +import static org.hisp.dhis.webapi.controller.event.webrequest.OrderCriteria.fromOrderString; +import static org.hisp.dhis.webapi.controller.tracker.export.RequestParamsValidator.validateOrderParams; +import static org.junit.jupiter.api.Assertions.assertAll; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.Set; +import org.hisp.dhis.common.CodeGenerator; +import org.hisp.dhis.webapi.controller.exception.BadRequestException; +import org.junit.jupiter.api.Test; + +/** Tests {@link RequestParamsValidator}. */ +class RequestParamsValidatorTest { + + @Test + void shouldPassOrderParamsValidationWhenGivenOrderIsOrderable() throws BadRequestException { + Set supportedFieldNames = Set.of("createdAt", "scheduledAt"); + + validateOrderParams(fromOrderString("createdAt:asc,scheduledAt:asc"), supportedFieldNames, ""); + } + + @Test + void shouldFailOrderParamsValidationWhenGivenInvalidOrderComponents() { + Set supportedFieldNames = Set.of("enrolledAt"); + String invalidUID = "Cogn34Del"; + assertFalse(CodeGenerator.isValidUid(invalidUID)); + + Exception exception = + assertThrows( + BadRequestException.class, + () -> + validateOrderParams( + fromOrderString( + "unsupportedProperty1:asc,enrolledAt:asc," + + invalidUID + + ",unsupportedProperty2:desc"), + supportedFieldNames, + "data element and attribute")); + assertAll( + () -> assertStartsWith("order parameter is invalid", exception.getMessage()), + () -> + assertContains( + "Supported are data element and attribute UIDs and fields", exception.getMessage()), + // order of fields might not always be the same; therefore using contains + () -> assertContains(invalidUID, exception.getMessage()), + () -> assertContains("unsupportedProperty1", exception.getMessage()), + () -> assertContains("unsupportedProperty2", exception.getMessage())); + } + + @Test + void shouldPassOrderParamsValidationWhenGivenInvalidOrderNameWhichIsAValidUID() + throws BadRequestException { + Set supportedFieldNames = Set.of("enrolledAt"); + // This test case shows that some field names are valid UIDs. We can thus not rule out all + // invalid field names and UIDs at this stage as we do not have access to data element/attribute + // services. Such invalid order values will be caught in the service (mapper). + assertTrue(CodeGenerator.isValidUid("lastUpdated")); + + validateOrderParams(fromOrderString("lastUpdated:desc"), supportedFieldNames, ""); + } + + @Test + void shouldFailOrderParamsValidationWhenGivenRepeatedOrderComponents() { + Set supportedFieldNames = Set.of("createdAt", "enrolledAt"); + + Exception exception = + assertThrows( + BadRequestException.class, + () -> + validateOrderParams( + fromOrderString( + "zGlzbfreTOH,createdAt:asc,enrolledAt:asc,enrolledAt,zGlzbfreTOH"), + supportedFieldNames, + "")); + + assertAll( + () -> assertStartsWith("order parameter is invalid", exception.getMessage()), + // order of fields might not always be the same; therefore using contains + () -> assertContains("repeated", exception.getMessage()), + () -> assertContains("enrolledAt", exception.getMessage()), + () -> assertContains("zGlzbfreTOH", exception.getMessage())); + } +}