chore: reject empty order fields

dhis2 · Mar 1, 2024 · d245c75 · d245c75
1 parent e4d1b38
commit d245c75
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 25 deletions.
diff --git a/...his-api/src/main/java/org/hisp/dhis/webapi/controller/event/webrequest/OrderCriteria.java b/...his-api/src/main/java/org/hisp/dhis/webapi/controller/event/webrequest/OrderCriteria.java
@@ -77,16 +77,23 @@ private static List<OrderCriteria> toOrderCriterias(String s) {
    */
   public static OrderCriteria valueOf(String input) {
     String[] props = input.split(":");
-    if (props.length == 2) {
-      return OrderCriteria.of(props[0].trim(), SortDirection.of(props[1].trim()));
+    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].trim(), SortDirection.ASC);
+
+    String field = props[0].trim();
+    if (StringUtils.isEmpty(field)) {
+      throw new IllegalArgumentException(
+          "Invalid order property: '"
+              + input
+              + "'. Valid formats are 'field:direction' or 'field'. Valid directions are 'asc' or 'desc'. Direction defaults to 'asc'.");
     }
 
-    throw new IllegalArgumentException(
-        "Invalid order property: '"
-            + input
-            + "'. Valid formats are 'field:direction' or 'field'. Valid directions are 'asc' or 'desc'. Direction defaults to 'asc'.");
+    SortDirection direction =
+        props.length == 1 ? SortDirection.ASC : SortDirection.of(props[1].trim());
+    return OrderCriteria.of(field, direction);
   }
 }
diff --git a/...api/src/test/java/org/hisp/dhis/webapi/controller/event/webrequest/OrderCriteriaTest.java b/...api/src/test/java/org/hisp/dhis/webapi/controller/event/webrequest/OrderCriteriaTest.java
@@ -41,36 +41,29 @@ class OrderCriteriaTest {
 
   @Test
   void fromOrderString() {
-    List<OrderCriteria> orderCriteria =
-        OrderCriteria.fromOrderString("one:desc,, two:asc  ,three  ");
+    List<OrderCriteria> order = OrderCriteria.fromOrderString("one:desc,, two:asc  ,three  ");
 
-    assertNotNull(orderCriteria);
     assertEquals(
-        3, orderCriteria.size(), String.format("Expected 3 item, instead got %s", orderCriteria));
-    assertEquals(OrderCriteria.of("one", SortDirection.DESC), orderCriteria.get(0));
-    assertEquals(OrderCriteria.of("two", SortDirection.ASC), orderCriteria.get(1));
-    assertEquals(OrderCriteria.of("three", SortDirection.ASC), orderCriteria.get(2));
+        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> orderCriteria = OrderCriteria.fromOrderString(source);
+    List<OrderCriteria> order = 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> orderCriteria = OrderCriteria.fromOrderString("one:");
+    List<OrderCriteria> order = 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
@@ -92,4 +85,9 @@ void failGivenMoreThanTwoColons() {
     assertThrows(
         IllegalArgumentException.class, () -> OrderCriteria.fromOrderString("one:desc:wrong"));
   }
+
+  @Test
+  void failGivenEmptyField() {
+    assertThrows(IllegalArgumentException.class, () -> OrderCriteria.valueOf(" :desc"));
+  }
 }