fix: Sorting validation and prevent NPE [DHIS2-16369] (#16151)

dhis2 · Jan 12, 2024 · f25e152 · f25e152
1 parent 3a58d32
commit f25e152
Show file tree

Hide file tree

Showing 3 changed files with 48 additions and 4 deletions.
diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/common/DimensionalObject.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/common/DimensionalObject.java
@@ -27,6 +27,8 @@
  */
 package org.hisp.dhis.common;
 
+import static org.apache.commons.collections4.CollectionUtils.isNotEmpty;
+
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -164,7 +166,7 @@ public interface DimensionalObject extends NameableObject, GroupableItem {
 
   /** Indicates whether this dimension has any dimension items. */
   default boolean hasItems() {
-    return !getItems().isEmpty();
+    return isNotEmpty(getItems());
   }
 
   /** Gets the legend set. */

diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/visualization/Visualization.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/visualization/Visualization.java
@@ -32,6 +32,7 @@
 import static java.util.Arrays.asList;
 import static java.util.stream.Collectors.toList;
 import static org.apache.commons.collections4.CollectionUtils.isEmpty;
+import static org.apache.commons.lang3.ObjectUtils.defaultIfNull;
 import static org.apache.commons.lang3.StringUtils.containsAny;
 import static org.apache.commons.lang3.StringUtils.defaultIfEmpty;
 import static org.apache.commons.lang3.StringUtils.isBlank;
@@ -828,15 +829,22 @@ private List<DimensionalItemObject> getDimensionalItemObjects(String dimension)
 
   /** Validates the state of the current list of {@link Sorting} objects (if one is defined). */
   public void validateSortingState() {
-    List<String> columns = getColumnDimensions();
+    List<String> columns = defaultIfNull(getColumnDimensions(), List.of());
     List<Sorting> sortingList = getSorting();
+    List<DimensionalItemObject> items =
+        getColumns().stream()
+            .filter(c -> c.hasItems())
+            .flatMap(c -> c.getItems().stream())
+            .toList();
 
     sortingList.forEach(
         s -> {
           if (isBlank(s.getDimension()) || s.getDirection() == null) {
             throw new IllegalArgumentException("Sorting is not valid");
-          } else if (columns.stream()
-              .noneMatch(c -> containsAny(s.getDimension(), c.split("\\.")))) {
+          } else if (columns.stream().noneMatch(c -> containsAny(s.getDimension(), c.split("\\.")))
+              && items.stream()
+                  .noneMatch(
+                      c -> containsAny(s.getDimension(), c.getDimensionItem().split("\\.")))) {
             throw new IllegalStateException(s.getDimension());
           }
         });

diff --git a/...st-web-api/src/test/java/org/hisp/dhis/webapi/controller/VisualizationControllerTest.java b/...st-web-api/src/test/java/org/hisp/dhis/webapi/controller/VisualizationControllerTest.java
@@ -36,6 +36,8 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
 import org.hisp.dhis.common.IdentifiableObjectManager;
+import org.hisp.dhis.indicator.Indicator;
+import org.hisp.dhis.indicator.IndicatorType;
 import org.hisp.dhis.jsontree.JsonList;
 import org.hisp.dhis.jsontree.JsonMixed;
 import org.hisp.dhis.jsontree.JsonObject;
@@ -156,6 +158,38 @@ void testPostSortingObject() {
     assertThat(response.get("sorting").toString(), containsString("ASC"));
   }
 
+  @Test
+  void testPostSortingObjectForColumnItems() {
+    // Given
+    IndicatorType indicatorType = createIndicatorType('A');
+    manager.save(indicatorType);
+
+    Indicator mockIndicator = createIndicator('A', indicatorType);
+    manager.save(mockIndicator);
+
+    String sorting =
+        "'sorting': [{'dimension': '" + mockIndicator.getUid() + "', 'direction':'ASC'}]";
+    String body =
+        "{'name': 'Name Test', 'type': 'STACKED_COLUMN', 'program': {'id':'"
+            + mockProgram.getUid()
+            + "'}, 'columns': [{'dimension': 'dx',"
+            + "'items': [{'id': '"
+            + mockIndicator.getUid()
+            + "'}]}],"
+            + sorting
+            + "}";
+
+    // When
+    String uid = assertStatus(CREATED, POST("/visualizations/", body));
+
+    // Then
+    String getParams = "?fields=:all,columns[:all,items,sorting]";
+    JsonObject response = GET("/visualizations/" + uid + getParams).content();
+
+    assertThat(response.get("sorting").toString(), containsString(mockIndicator.getUid()));
+    assertThat(response.get("sorting").toString(), containsString("ASC"));
+  }
+
   @Test
   void testPostMultipleSortingObject() {
     // Given