chore: Validate sort direction and remove unsupported options [TECH-1…

…622]

dhis-2/dhis-api/src/main/java/org/hisp/dhis/webapi/controller/event/mapper/SortDirection.java

@@ -34,29 +34,26 @@
 @Getter
 @AllArgsConstructor
 public enum SortDirection {
-  ASC("asc", false),
-  DESC("desc", false),
-  IASC("iasc", true),
-  IDESC("idesc", true);
-
-  private static final SortDirection DEFAULT_SORTING_DIRECTION = ASC;
+  ASC("asc"),
+  DESC("desc");
 
   private final String value;
 
-  private final boolean ignoreCase;
-
   public static SortDirection of(String value) {
-    return of(value, DEFAULT_SORTING_DIRECTION);
-  }
-
-  public static SortDirection of(String value, SortDirection defaultSortingDirection) {
     return Arrays.stream(values())
         .filter(sortDirection -> sortDirection.getValue().equalsIgnoreCase(value))
         .findFirst()
-        .orElse(defaultSortingDirection);
+        .orElseThrow(
+            () ->
+                new IllegalArgumentException(
+                    "'"
+                        + value
+                        + "' is not a valid sort direction. Valid values are: "
+                        + Arrays.toString(SortDirection.values())
+                        + "."));
   }
 
   public boolean isAscending() {
-    return this == ASC || this == IASC;
+    return this == ASC;
   }
 }

dhis-2/dhis-api/src/main/java/org/hisp/dhis/webapi/controller/event/webrequest/OrderCriteria.java

@@ -68,10 +68,10 @@ private static List<OrderCriteria> toOrderCriterias(String s) {
         .collect(Collectors.toList());
   }
 
-  private static OrderCriteria toOrderCriteria(String s1) {
+  public static OrderCriteria toOrderCriteria(String s1) {
     String[] props = s1.split(":");
     if (props.length == 2) {
-      return OrderCriteria.of(props[0], SortDirection.of(props[1]));
+      return OrderCriteria.of(props[0], SortDirection.of(props[1].trim()));
     }
     if (props.length == 1) {
       return OrderCriteria.of(props[0], SortDirection.ASC);

dhis-2/dhis-api/src/test/java/org/hisp/dhis/webapi/controller/event/webrequest/OrderCriteriaTest.java

@@ -30,6 +30,7 @@
 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;
 import org.hisp.dhis.webapi.controller.event.mapper.SortDirection;
@@ -79,13 +80,7 @@ void fromOrderStringDefaultsToAscGivenFieldAndColon() {
 
   @Test
   void fromOrderStringDefaultsSortDirectionToAscGivenAnUnknownSortDirection() {
-
-    List<OrderCriteria> orderCriteria = OrderCriteria.fromOrderString("one:wrong");
-
-    assertNotNull(orderCriteria);
-    assertEquals(
-        1, orderCriteria.size(), String.format("Expected 1 item, instead got %s", orderCriteria));
-    assertEquals(OrderCriteria.of("one", SortDirection.ASC), orderCriteria.get(0));
+    assertThrows(IllegalArgumentException.class, () -> OrderCriteria.fromOrderString("one:wrong"));
   }
 
   @Test

dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/security/config/StringToOrderCriteriaListConverter.java → dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/common/OrderCriteriaParamEditor.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2004-2022, University of Oslo
+ * Copyright (c) 2004-2023, University of Oslo
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -25,20 +25,14 @@
  * (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.security.config;
+package org.hisp.dhis.webapi.common;
 
-import java.util.List;
+import java.beans.PropertyEditorSupport;
 import org.hisp.dhis.webapi.controller.event.webrequest.OrderCriteria;
-import org.springframework.core.convert.converter.Converter;
 
-/**
- * String to Order Criterias converter for MVC requests
- *
- * @author Giuseppe Nespolino <[email protected]>
- */
-class StringToOrderCriteriaListConverter implements Converter<String, List<OrderCriteria>> {
+public class OrderCriteriaParamEditor extends PropertyEditorSupport {
   @Override
-  public List<OrderCriteria> convert(String source) {
-    return OrderCriteria.fromOrderString(source);
+  public void setAsText(String source) {
+    setValue(OrderCriteria.toOrderCriteria(source));
   }
 }

dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/CrudControllerAdvice.java

@@ -77,7 +77,9 @@
 import org.hisp.dhis.schema.SchemaPathException;
 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;
@@ -145,6 +147,7 @@ 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());
   }
 

dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/security/config/WebMvcConfig.java

@@ -219,7 +219,6 @@ public void configureMessageConverters(List<HttpMessageConverter<?>> converters)
 
   @Override
   protected void addFormatters(FormatterRegistry registry) {
-    registry.addConverter(new StringToOrderCriteriaListConverter());
     registry.addConverter(new FieldPathConverter());
   }
 

dhis-2/dhis-web-api/src/test/java/org/hisp/dhis/webapi/controller/OrderBindingTest.java

@@ -0,0 +1,119 @@
+/*
+ * 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;
+
+import static org.hamcrest.core.StringContains.containsString;
+import static org.hisp.dhis.dxf2.webmessage.WebMessageUtils.ok;
+import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
+import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content;
+
+import java.util.List;
+import org.hisp.dhis.dxf2.webmessage.WebMessage;
+import org.hisp.dhis.webapi.controller.event.webrequest.OrderCriteria;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.springframework.stereotype.Controller;
+import org.springframework.test.web.servlet.MockMvc;
+import org.springframework.test.web.servlet.setup.MockMvcBuilders;
+import org.springframework.web.bind.annotation.GetMapping;
+import org.springframework.web.bind.annotation.RequestParam;
+import org.springframework.web.bind.annotation.ResponseBody;
+
+class OrderBindingTest {
+  private static final String ENDPOINT = "/ordering";
+
+  private MockMvc mockMvc;
+
+  @BeforeEach
+  public void setUp() {
+    mockMvc =
+        MockMvcBuilders.standaloneSetup(new OrderingController())
+            .setControllerAdvice(new CrudControllerAdvice())
+            .build();
+  }
+
+  @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")));
+  }
+
+  @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")));
+  }
+
+  @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")));
+  }
+
+  @Test
+  void shouldReturnABadRequestWhenInvalidSortDirectionIsPassedAsParameter() 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]")));
+  }
+
+  @Test
+  void shouldReturnABadRequestWhenInvalidSortDirectionIsPassedAsParameterInAListOfOrders()
+      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]")));
+  }
+
+  @Controller
+  private static class OrderingController extends CrudControllerAdvice {
+    @GetMapping(value = ENDPOINT)
+    public @ResponseBody WebMessage getOrder(@RequestParam List<OrderCriteria> order) {
+      return ok(order.toString());
+    }
+  }
+}