fix: /tracker/events?order=<attributeUID> filters out events without attribute [ DHIS2-15679 ] [2.39]

dhis-2/dhis-services/dhis-service-dxf2/src/main/java/org/hisp/dhis/dxf2/events/event/EventQueryParams.java

@@ -33,6 +33,7 @@
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
+import java.util.stream.Collectors;
 import org.hisp.dhis.category.CategoryOptionCombo;
 import org.hisp.dhis.common.AssignedUserSelectionMode;
 import org.hisp.dhis.common.IdSchemes;
@@ -615,10 +616,6 @@ public EventQueryParams addFilters(List<QueryItem> items) {
     return this;
   }
 
-  public List<QueryItem> getFilterAttributes() {
-    return Collections.unmodifiableList(this.filterAttributes);
-  }
-
   public EventQueryParams addFilterAttributes(List<QueryItem> item) {
     this.filterAttributes.addAll(item);
     return this;
@@ -629,6 +626,29 @@ public EventQueryParams addFilterAttributes(QueryItem item) {
     return this;
   }
 
+  /** Returns attributes that are only used as filter. */
+  public List<QueryItem> getFilterAttributes() {
+    Set<String> attrOrderUids =
+        getAttributeOrders().stream().map(OrderParam::getField).collect(Collectors.toSet());
+    return filterAttributes.stream()
+        .filter(af -> !attrOrderUids.contains(af.getItem().getUid()))
+        .collect(Collectors.toList());
+  }
+
+  /** Returns attributes that are only ordered by and not present in any filter. */
+  public Set<QueryItem> leftJoinAttributes() {
+    Set<QueryItem> attributesWithEmptyFilter =
+        this.filterAttributes.stream()
+            .filter(fa -> fa.getFilters().isEmpty())
+            .collect(Collectors.toSet());
+    Set<String> attributeOrderUids =
+        getAttributeOrders().stream().map(OrderParam::getField).collect(Collectors.toSet());
+
+    return attributesWithEmptyFilter.stream()
+        .filter(a -> attributeOrderUids.contains(a.getItem().getUid()))
+        .collect(Collectors.toSet());
+  }
+
   public EventQueryParams setIncludeDeleted(boolean includeDeleted) {
     this.includeDeleted = includeDeleted;
     return this;

dhis-2/dhis-services/dhis-service-dxf2/src/main/java/org/hisp/dhis/dxf2/events/event/JdbcEventStore.java

@@ -946,12 +946,12 @@ private String buildSql(
    * Generates a single INNER JOIN for each attribute we are searching on. We can search by a range
    * of operators. All searching is using lower() since attribute values are case-insensitive.
    *
-   * @param attributes
-   * @param filterItems
+   * @param params
    */
-  private void joinAttributeValueWithoutQueryParameter(
-      StringBuilder attributes, List<QueryItem> filterItems) {
-    for (QueryItem queryItem : filterItems) {
+  private String joinAttributeValue(EventQueryParams params) {
+    StringBuilder attributes = new StringBuilder();
+
+    for (QueryItem queryItem : params.getFilterAttributes()) {
       String teaValueCol = statementBuilder.columnQuote(queryItem.getItemId());
       String teaCol = statementBuilder.columnQuote(queryItem.getItemId() + "ATT");
 
@@ -974,6 +974,36 @@ private void joinAttributeValueWithoutQueryParameter(
 
       attributes.append(getAttributeFilterQuery(queryItem, teaCol, teaValueCol));
     }
+    return attributes.toString();
+  }
+
+  /**
+   * Generates the LEFT JOINs used for attributes we are ordering by (If any). We use LEFT JOIN to
+   * avoid removing any rows if there is no value for a given attribute and te. The result of this
+   * LEFT JOIN is used in the sub-query projection, and ordering in the sub-query and main query.
+   *
+   * @return a SQL LEFT JOIN for attributes used for ordering, or empty string if no attributes is
+   *     used in order.
+   */
+  private String getFromSubQueryJoinOrderByAttributes(EventQueryParams params) {
+    StringBuilder joinOrderAttributes = new StringBuilder();
+
+    for (QueryItem orderAttribute : params.leftJoinAttributes()) {
+
+      joinOrderAttributes
+          .append(" LEFT JOIN trackedentityattributevalue AS ")
+          .append(statementBuilder.columnQuote(orderAttribute.getItem().getUid()))
+          .append(" ON ")
+          .append(statementBuilder.columnQuote(orderAttribute.getItem().getUid()))
+          .append(".trackedentityinstanceid = TEI.trackedentityinstanceid ")
+          .append("AND ")
+          .append(statementBuilder.columnQuote(orderAttribute.getItem().getUid()))
+          .append(".trackedentityattributeid = ")
+          .append(orderAttribute.getItem().getId())
+          .append(SPACE);
+    }
+
+    return joinOrderAttributes.toString();
   }
 
   private String getAttributeFilterQuery(QueryItem queryItem, String teaCol, String teaValueCol) {
@@ -1126,9 +1156,11 @@ private StringBuilder getFromWhereClause(
             "left join organisationunit teiou on (tei.organisationunitid=teiou.organisationunitid) ")
         .append("left join userinfo au on (psi.assigneduserid=au.userinfoid) ");
 
-    if (!params.getFilterAttributes().isEmpty()) {
-      joinAttributeValueWithoutQueryParameter(fromBuilder, params.getFilterAttributes());
-    }
+    // JOIN attributes we need to filter on.
+    fromBuilder.append(joinAttributeValue(params));
+
+    // LEFT JOIN not filterable attributes we need to sort on.
+    fromBuilder.append(getFromSubQueryJoinOrderByAttributes(params));
 
     fromBuilder.append(getCategoryOptionComboQuery(user));
 

dhis-2/dhis-test-e2e/src/test/java/org/hisp/dhis/tracker/importer/TrackerExportTests.java

@@ -413,7 +413,9 @@ public void shouldReturnFilteredEvent() {
 
   @Test
   public void shouldReturnDescOrderedEventByTEIAttribute() {
-    ApiResponse response = trackerActions.get("events?order=dIVt4l5vIOa:desc");
+    ApiResponse response =
+        trackerActions.get("events?order=dIVt4l5vIOa:desc&event=olfXZzSGacW;ZwwuwNp6gVd");
+
     response.validate().statusCode(200).body("instances", hasSize(equalTo(2)));
     List<String> events = response.extractList("instances.event.flatten()");
     assertEquals(
@@ -422,7 +424,9 @@ public void shouldReturnDescOrderedEventByTEIAttribute() {
 
   @Test
   public void shouldReturnAscOrderedEventByTEIAttribute() {
-    ApiResponse response = trackerActions.get("events?order=dIVt4l5vIOa:asc");
+    ApiResponse response =
+        trackerActions.get("events?order=dIVt4l5vIOa:asc&event=olfXZzSGacW;ZwwuwNp6gVd");
+
     response.validate().statusCode(200).body("instances", hasSize(equalTo(2)));
     List<String> events = response.extractList("instances.event.flatten()");
     assertEquals(

dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/tracker/EventExporterTest.java

@@ -48,6 +48,7 @@
 import java.util.Arrays;
 import java.util.Date;
 import java.util.List;
+import java.util.Objects;
 import java.util.Set;
 import java.util.function.Function;
 import java.util.stream.Collectors;
@@ -1118,11 +1119,13 @@ void testEnrollmentFilterAttributesWithMultipleFiltersOnTheSameAttribute() {
 
   @Test
   void testOrderEventsOnAttributeAsc() {
+    TrackedEntityAttribute tea = get(TrackedEntityAttribute.class, "toUpdate000");
+
     EventQueryParams params = new EventQueryParams();
     params.setOrgUnitSelectionMode(SELECTED);
     params.setOrgUnit(orgUnit);
-    params.addFilterAttributes(queryItem("toUpdate000"));
-    params.addAttributeOrders(List.of(new OrderParam("toUpdate000", OrderParam.SortDirection.ASC)));
+    params.addFilterAttributes(queryItem(tea));
+    params.addAttributeOrders(List.of(new OrderParam(tea.getUid(), SortDirection.ASC)));
     params.addOrders(params.getAttributeOrders());
 
     List<String> trackedEntities =
@@ -1135,12 +1138,13 @@ void testOrderEventsOnAttributeAsc() {
 
   @Test
   void testOrderEventsOnAttributeDesc() {
+    TrackedEntityAttribute tea = get(TrackedEntityAttribute.class, "toUpdate000");
+
     EventQueryParams params = new EventQueryParams();
     params.setOrgUnitSelectionMode(SELECTED);
     params.setOrgUnit(orgUnit);
-    params.addFilterAttributes(queryItem("toUpdate000"));
-    params.addAttributeOrders(
-        List.of(new OrderParam("toUpdate000", OrderParam.SortDirection.DESC)));
+    params.addFilterAttributes(queryItem(tea));
+    params.addAttributeOrders(List.of(new OrderParam(tea.getUid(), SortDirection.DESC)));
     params.addOrders(params.getAttributeOrders());
 
     List<String> trackedEntities =
@@ -1153,14 +1157,17 @@ void testOrderEventsOnAttributeDesc() {
 
   @Test
   void testOrderEventsOnMultipleAttributesDesc() {
+    TrackedEntityAttribute tea = get(TrackedEntityAttribute.class, "toUpdate000");
+    TrackedEntityAttribute tea1 = get(TrackedEntityAttribute.class, "toDelete000");
+
     EventQueryParams params = new EventQueryParams();
     params.setOrgUnitSelectionMode(SELECTED);
     params.setOrgUnit(orgUnit);
-    params.addFilterAttributes(List.of(queryItem("toUpdate000"), queryItem("toDelete000")));
+    params.addFilterAttributes(List.of(queryItem(tea), queryItem(tea1)));
     params.addAttributeOrders(
         List.of(
-            new OrderParam("toDelete000", OrderParam.SortDirection.DESC),
-            new OrderParam("toUpdate000", OrderParam.SortDirection.DESC)));
+            new OrderParam(tea1.getUid(), SortDirection.DESC),
+            new OrderParam(tea.getUid(), SortDirection.DESC)));
     params.addOrders(params.getAttributeOrders());
 
     List<String> trackedEntities =
@@ -1173,14 +1180,17 @@ void testOrderEventsOnMultipleAttributesDesc() {
 
   @Test
   void testOrderEventsOnMultipleAttributesAsc() {
+    TrackedEntityAttribute tea = get(TrackedEntityAttribute.class, "toUpdate000");
+    TrackedEntityAttribute tea1 = get(TrackedEntityAttribute.class, "toDelete000");
+
     EventQueryParams params = new EventQueryParams();
     params.setOrgUnitSelectionMode(SELECTED);
     params.setOrgUnit(orgUnit);
-    params.addFilterAttributes(List.of(queryItem("toUpdate000"), queryItem("toDelete000")));
+    params.addFilterAttributes(List.of(queryItem(tea), queryItem(tea1)));
     params.addAttributeOrders(
         List.of(
-            new OrderParam("toDelete000", OrderParam.SortDirection.DESC),
-            new OrderParam("toUpdate000", OrderParam.SortDirection.ASC)));
+            new OrderParam(tea1.getUid(), SortDirection.DESC),
+            new OrderParam(tea.getUid(), SortDirection.ASC)));
     params.addOrders(params.getAttributeOrders());
 
     Events events = eventService.getEvents(params);
@@ -1196,15 +1206,19 @@ void testOrderEventsOnMultipleAttributesAsc() {
 
   @Test
   void shouldOrderEventsByMultipleAttributesAndPaginateWhenGivenNonDefaultPageSize() {
+    TrackedEntityAttribute tea = get(TrackedEntityAttribute.class, "toUpdate000");
+    TrackedEntityAttribute tea1 = get(TrackedEntityAttribute.class, "toDelete000");
+
     EventQueryParams params = new EventQueryParams();
     params.setOrgUnit(orgUnit);
+    params.addFilterAttributes(List.of(queryItem(tea), queryItem(tea1)));
     params.setOrgUnitSelectionMode(SELECTED);
-    params.addFilterAttributes(List.of(queryItem("toUpdate000"), queryItem("toDelete000")));
     params.addAttributeOrders(
         List.of(
-            new OrderParam("toDelete000", SortDirection.DESC),
-            new OrderParam("toUpdate000", SortDirection.ASC)));
+            new OrderParam(tea1.getUid(), SortDirection.DESC),
+            new OrderParam(tea.getUid(), SortDirection.ASC)));
     params.addOrders(params.getAttributeOrders());
+    params.setEvents(Set.of("D9PbzJY8bJM", "pTzf9KYMk72"));
 
     params.setPage(1);
     params.setPageSize(1);
@@ -1351,15 +1365,17 @@ void shouldReturnEventsWhenParamEndDueDateLaterThanEventsDueDate() {
 
   @Test
   void shouldSortEntitiesRespectingOrderWhenAttributeOrderSuppliedBeforeOrderParam() {
+    TrackedEntityAttribute tea = get(TrackedEntityAttribute.class, "toUpdate000");
+
     EventQueryParams params = new EventQueryParams();
     params.setOrgUnitSelectionMode(SELECTED);
     params.setOrgUnit(orgUnit);
-    params.addFilterAttributes(List.of(queryItem("toUpdate000")));
-    params.addAttributeOrders(List.of(new OrderParam("toUpdate000", OrderParam.SortDirection.ASC)));
+    params.addFilterAttributes(List.of(queryItem(tea)));
+    params.addAttributeOrders(List.of(new OrderParam("toUpdate000", SortDirection.ASC)));
     params.addOrders(
         List.of(
-            new OrderParam("toUpdate000", OrderParam.SortDirection.ASC),
-            new OrderParam("enrolledAt", OrderParam.SortDirection.ASC)));
+            new OrderParam(tea.getUid(), SortDirection.ASC),
+            new OrderParam("enrolledAt", SortDirection.ASC)));
 
     List<String> trackedEntities =
         eventService.getEvents(params).getEvents().stream()
@@ -1371,16 +1387,17 @@ void shouldSortEntitiesRespectingOrderWhenAttributeOrderSuppliedBeforeOrderParam
 
   @Test
   void shouldSortEntitiesRespectingOrderWhenOrderParamSuppliedBeforeAttributeOrder() {
+    TrackedEntityAttribute tea = get(TrackedEntityAttribute.class, "toUpdate000");
+
     EventQueryParams params = new EventQueryParams();
     params.setOrgUnitSelectionMode(SELECTED);
     params.setOrgUnit(orgUnit);
-    params.addFilterAttributes(List.of(queryItem("toUpdate000")));
-    params.addAttributeOrders(
-        List.of(new OrderParam("toUpdate000", OrderParam.SortDirection.DESC)));
+    params.addFilterAttributes(List.of(queryItem(tea)));
+    params.addAttributeOrders(List.of(new OrderParam(tea.getUid(), SortDirection.DESC)));
     params.addOrders(
         List.of(
-            new OrderParam("enrolledAt", OrderParam.SortDirection.DESC),
-            new OrderParam("toUpdate000", OrderParam.SortDirection.DESC)));
+            new OrderParam("enrolledAt", SortDirection.DESC),
+            new OrderParam(tea.getUid(), SortDirection.DESC)));
 
     List<String> trackedEntities =
         eventService.getEvents(params).getEvents().stream()
@@ -1479,6 +1496,38 @@ void testExportEventsWithProgramStageOrder() {
         "Program Stage are not in the correct order");
   }
 
+  @Test
+  void shouldNotFilterOutEventsWithoutATrackedEntityAttributeWhenAttributeIsOnlyOrdering() {
+    TrackedEntityAttribute tea =
+        get(
+            TrackedEntityAttribute.class,
+            "toUpdate000"); // "QS6w44flWAf", "dUE514NMOlo" will be at the end of the result set as
+    // they do not have "numericAttr" and Postgres put null first when
+    // ordering
+    TrackedEntityAttribute tea1 = get(TrackedEntityAttribute.class, "numericAttr");
+
+    EventQueryParams params = new EventQueryParams();
+
+    params.setOrgUnit(orgUnit);
+    params.addFilterAttributes(List.of(queryItem(tea), queryItem(tea1)));
+
+    params.addAttributeOrders(
+        List.of(
+            new OrderParam(tea.getUid(), SortDirection.DESC),
+            new OrderParam(tea1.getUid(), SortDirection.DESC)));
+    params.addOrders(params.getAttributeOrders());
+    params.setEvents(Set.of("jxgFyJEMUPf", "pTzf9KYMk72", "D9PbzJY8bJM", "JaRDIvcEcEx"));
+
+    List<String> trackedEntities =
+        eventService.getEvents(params).getEvents().stream()
+            .map(Event::getTrackedEntityInstance)
+            .filter(Objects::nonNull) // exclude event with no teis
+            .collect(Collectors.toList());
+
+    assertEquals(
+        List.of("mHWCacsGYYn", "QesgJkTyTCk", "QS6w44flWAf", "dUE514NMOlo"), trackedEntities);
+  }
+
   private void assertNote(User expectedLastUpdatedBy, String expectedNote, Note actual) {
     assertEquals(expectedNote, actual.getValue());
     UserInfoSnapshot lastUpdatedBy = actual.getLastUpdatedBy();
@@ -1490,6 +1539,16 @@ private DataElement dataElement(String uid) {
     return dataElementService.getDataElement(uid);
   }
 
+  private static QueryItem queryItem(TrackedEntityAttribute tea) {
+    return new QueryItem(
+        tea,
+        null,
+        tea.getValueType(),
+        tea.getAggregationType(),
+        tea.getOptionSet(),
+        tea.isUnique());
+  }
+
   private static QueryItem queryItem(String teaUid, QueryOperator operator, String filter) {
     QueryItem item = queryItem(teaUid);
     item.addFilter(new QueryFilter(operator, filter));

dhis-2/dhis-test-integration/src/test/resources/tracker/event_and_enrollment.json

@@ -178,7 +178,7 @@
             "idScheme": "UID",
             "identifier": "numericAttr"
           },
-          "value": 88
+          "value": 87
         }
       ],
       "enrollments": []
@@ -206,7 +206,7 @@
             "idScheme": "UID",
             "identifier": "numericAttr"
           },
-          "value": 88
+          "value": 86
         }
       ],
       "enrollments": []

dhis-2/dhis-web-api/src/test/java/org/hisp/dhis/webapi/controller/tracker/export/TrackerEventCriteriaMapperTest.java

@@ -371,8 +371,7 @@ void testMappingAttributeOrdering() throws BadRequestException {
         () ->
             assertContainsOnly(
                 params.getAttributeOrders(),
-                List.of(new OrderParam(TEA_1_UID, OrderParam.SortDirection.ASC))),
-        () -> assertContainsOnly(params.getFilterAttributes(), List.of(new QueryItem(tea1))));
+                List.of(new OrderParam(TEA_1_UID, OrderParam.SortDirection.ASC))));
   }
 
   @Test