fix: reject invalid order parameter values DHIS2-16935

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