fix: reject invalid order parameter values DHIS2-16935 (#16673)

* fix: reject invalid order parameter values

Rename OrderCriteria factory to valueOf so its used by Springs conversion
system. The OrderCriteriaParamEditor is not needed. In fact its even causing the
issue that "order=createdAt:desc,event:asc" is not split by Spring and passed
as is to the PropertyEditor. Whatever mechansim is used to convert Spring
suggests throwing an IllegalArgumentException in case conversion failed. We returned
null which lead to successful conversions with null elements

* chore: reject empty order fields

* 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

* test: TE workinglist separates order using ; instead of ,

* chore: UID does not need a PropertyEditor

as it has a contructor from String -> UID its automatically
picked up by Spring

* chore: cannot order by orgUnit using this implementation

we need to update this to the new tracker mechanism. Not sure if
these workinglists are also used by old tracker though

* test: UID binding

* test: fix

* test: clean

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<OrderCriteria> fromOrderString(String source) {
 
   private static List<OrderCriteria> 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);
   }
 }

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<OrderCriteria> order = OrderCriteria.fromOrderString("one:desc,, two:asc  ,three  ");
 
-    List<OrderCriteria> 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<OrderCriteria> order = OrderCriteria.fromOrderString(source);
 
-    List<OrderCriteria> 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<OrderCriteria> order = OrderCriteria.fromOrderString("one:");
 
-    List<OrderCriteria> 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 = OrderCriteria.fromOrderString(" ");
 
     assertNotNull(orderCriteria);
@@ -94,13 +81,13 @@ void fromOrderStringReturnsEmptyListGivenEmptyOrder() {
   }
 
   @Test
-  void fromOrderStringReturnsNullGivenOrderWithMoreThanTwoProperties() {
-
-    List<OrderCriteria> 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"));
   }
 }

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"
         ]
-    }
-}
+      }
+    ]
+  }
+}