fix: IN filter now works with org units data elements [DHIS2-18845] (#…

…19727) (#19744) * fix: quotes ouIds for IN filter, resolve ou uids [DHIS2-18845] Signed-off-by: Giuseppe Nespolino <g.nespolino@gmail.com> * test: added e2e test [DHIS2-18845] Signed-off-by: Giuseppe Nespolino <g.nespolino@gmail.com> * fix: quotes ouIds for IN filter, resolve ou uids [DHIS2-18845] Signed-off-by: Giuseppe Nespolino <g.nespolino@gmail.com> * test: added e2e test [DHIS2-18845] Signed-off-by: Giuseppe Nespolino <g.nespolino@gmail.com> * test: added e2e test [DHIS2-18845] Signed-off-by: Giuseppe Nespolino <g.nespolino@gmail.com> --------- Signed-off-by: Giuseppe Nespolino <g.nespolino@gmail.com> (cherry picked from commit d2c87f6) Co-authored-by: Maikel Arabori <51713408+maikelarabori@users.noreply.github.com>
dhis2 · Jan 24, 2025 · ff60784 · ff60784
1 parent 124d845
commit ff60784
Show file tree

Hide file tree

Showing 9 changed files with 165 additions and 13 deletions.
diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/common/InQueryFilter.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/common/InQueryFilter.java
@@ -46,19 +46,19 @@
 public class InQueryFilter extends QueryFilter {
   private final String field;
 
-  private final boolean isText;
+  private final boolean shouldQuote;
 
   /**
    * Construct a InQueryFilter using field name and the original {@link QueryFilter}
    *
    * @param field the field on which to construct the InQueryFilter
    * @param encodedFilter The original encodedFilter in {@link QueryFilter}
-   * @param isText whether this filter contains text or numeric values
+   * @param shouldQuote whether this filter contains text or numeric values
    */
-  public InQueryFilter(String field, String encodedFilter, boolean isText) {
+  public InQueryFilter(String field, String encodedFilter, boolean shouldQuote) {
     super(IN, encodedFilter);
     this.field = field;
-    this.isText = isText;
+    this.shouldQuote = shouldQuote;
   }
 
   /**
@@ -100,7 +100,7 @@ public String getSqlFilter() {
   }
 
   private String quoteIfNecessary(String item) {
-    return isText ? quote(item) : item;
+    return shouldQuote ? quote(item) : item;
   }
 
   private boolean hasMissingValue(List<String> filterItems) {

diff --git a/dhis-2/dhis-api/src/test/java/org/hisp/dhis/common/InQueryFilterTest.java b/dhis-2/dhis-api/src/test/java/org/hisp/dhis/common/InQueryFilterTest.java
@@ -64,11 +64,11 @@ void verifyNestedSqlStmtInFieldWithNullOnly() {
     executeTest(field, "NV", true, "(" + field + " is null and exists((select * from xy))) ");
   }
 
-  private void executeTest(String filterValue, boolean isText, String expected) {
-    executeTest("aField", filterValue, isText, expected);
+  private void executeTest(String filterValue, boolean shouldQuote, String expected) {
+    executeTest("aField", filterValue, shouldQuote, expected);
   }
 
-  private void executeTest(String field, String filterValue, boolean isText, String expected) {
-    assertEquals(new InQueryFilter(field, filterValue, isText).getSqlFilter(), expected);
+  private void executeTest(String field, String filterValue, boolean shouldQuote, String expected) {
+    assertEquals(new InQueryFilter(field, filterValue, shouldQuote).getSqlFilter(), expected);
   }
 }
diff --git a/...rvices/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/DataQueryService.java b/...rvices/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/DataQueryService.java
@@ -173,4 +173,15 @@ DimensionalObject getDimension(
    * @return a list of organisation units.
    */
   List<OrganisationUnit> getUserOrgUnits(DataQueryParams params, String userOrgUnit);
+
+  /**
+   * Based on the given parameters, this method will return a list of {@link OrganisationUnit} UIDs
+   * based on the given items and user organisation units.
+   *
+   * @param items the list of items that might be included into the resulting organisation unit and
+   *     its keywords.
+   * @param userOrgUnits the list of organisation units associated with the current user.
+   * @return a list of {@link OrganisationUnit} UIDs.
+   */
+  List<String> getOrgUnitDimensionUid(List<String> items, List<OrganisationUnit> userOrgUnits);
 }
diff --git a/...service-analytics/src/main/java/org/hisp/dhis/analytics/data/DefaultDataQueryService.java b/...service-analytics/src/main/java/org/hisp/dhis/analytics/data/DefaultDataQueryService.java
@@ -341,6 +341,21 @@ private List<DimensionalObject> getDimensionalObjects(DataQueryRequest request)
     return list;
   }
 
+  /**
+   * Based on the given parameters, this method will return a list of {@link OrganisationUnit} UIDs
+   * based on the given items and user organisation units.
+   *
+   * @param items the list of items that might be included into the resulting organisation unit and
+   *     its keywords.
+   * @param userOrgUnits the list of organisation units associated with the current user.
+   * @return a list of {@link OrganisationUnit} UIDs.
+   */
+  @Override
+  public List<String> getOrgUnitDimensionUid(
+      List<String> items, List<OrganisationUnit> userOrgUnits) {
+    return dimensionalObjectProducer.getOrgUnitDimensionUid(items, userOrgUnits);
+  }
+
   /**
    * Returns a {@link DimensionalObject}.
    *

diff --git a/...rvice-analytics/src/main/java/org/hisp/dhis/analytics/data/DimensionalObjectProducer.java b/...rvice-analytics/src/main/java/org/hisp/dhis/analytics/data/DimensionalObjectProducer.java
@@ -422,6 +422,24 @@ public BaseDimensionalObject getOrgUnitDimension(
         dimensionalKeywords);
   }
 
+  /**
+   * This method will return a list of {@link OrganisationUnit} UIDs based on the given items and
+   * user organisation units.
+   *
+   * @param items the list of items that might be included into the resulting organisation unit and
+   *     its keywords.
+   * @param userOrgUnits the list of organisation units associated with the current user.
+   * @return a list of {@link OrganisationUnit} UIDs.
+   */
+  public List<String> getOrgUnitDimensionUid(
+      List<String> items, List<OrganisationUnit> userOrgUnits) {
+    return getOrgUnitDimensionItems(
+            items, userOrgUnits, IdScheme.UID, new ArrayList<>(), new ArrayList<>())
+        .stream()
+        .map(DimensionalItemObject::getUid)
+        .toList();
+  }
+
   /**
    * Based on the given parameters, this method will return a list of {@link DimensionalItemObject}
    * of type {@link OrganisationUnit}. It also adds to the given levels and groups if certain

diff --git a/...s/src/main/java/org/hisp/dhis/analytics/event/data/AbstractJdbcEventAnalyticsManager.java b/...s/src/main/java/org/hisp/dhis/analytics/event/data/AbstractJdbcEventAnalyticsManager.java
@@ -1294,21 +1294,22 @@ private static class IdentifiableSql {
   }
 
   /**
-   * Creates a SQL statement for a single filter inside a query item.
+   * Creates a SQL statement for a single filter inside a query item. Made public for testing
+   * purposes.
    *
    * @param item the {@link QueryItem}.
    * @param filter the {@link QueryFilter}.
    * @param params the {@link EventQueryParams}.
    */
-  private String toSql(QueryItem item, QueryFilter filter, EventQueryParams params) {
+  public String toSql(QueryItem item, QueryFilter filter, EventQueryParams params) {
     String field =
         item.hasAggregationType()
             ? getSelectSql(filter, item, params)
             : getSelectSql(filter, item, params.getEarliestStartDate(), params.getLatestEndDate());
 
     if (IN.equals(filter.getOperator())) {
       InQueryFilter inQueryFilter =
-          new InQueryFilter(field, sqlBuilder.escape(filter.getFilter()), item.isText());
+          new InQueryFilter(field, sqlBuilder.escape(filter.getFilter()), !item.isNumeric());
 
       return inQueryFilter.getSqlFilter();
     } else {

diff --git a/...lytics/src/main/java/org/hisp/dhis/analytics/event/data/DefaultEventDataQueryService.java b/...lytics/src/main/java/org/hisp/dhis/analytics/event/data/DefaultEventDataQueryService.java
@@ -37,13 +37,16 @@
 import static org.hisp.dhis.analytics.util.AnalyticsUtils.illegalQueryExSupplier;
 import static org.hisp.dhis.analytics.util.AnalyticsUtils.throwIllegalQueryEx;
 import static org.hisp.dhis.common.DimensionalObject.DIMENSION_NAME_SEP;
+import static org.hisp.dhis.common.DimensionalObject.OPTION_SEP;
 import static org.hisp.dhis.common.DimensionalObject.PERIOD_DIM_ID;
 import static org.hisp.dhis.common.DimensionalObjectUtils.getDimensionFromParam;
 import static org.hisp.dhis.common.DimensionalObjectUtils.getDimensionItemsFromParam;
 import static org.hisp.dhis.common.DimensionalObjectUtils.getDimensionalItemIds;
+import static org.hisp.dhis.common.ValueType.ORGANISATION_UNIT;
 
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.Date;
 import java.util.List;
 import java.util.Locale;
@@ -54,6 +57,7 @@
 import java.util.stream.Collectors;
 import lombok.Getter;
 import lombok.RequiredArgsConstructor;
+import lombok.SneakyThrows;
 import org.apache.commons.lang3.StringUtils;
 import org.hisp.dhis.analytics.AnalyticsAggregationType;
 import org.hisp.dhis.analytics.DataQueryService;
@@ -331,7 +335,7 @@ private void addDimensionsToParams(
           if (groupableItem != null) {
             params.addDimension((DimensionalObject) groupableItem);
           } else {
-            groupableItem = getQueryItem(dim, pr, request.getOutputType());
+            groupableItem = getQueryItem(dim, pr, request.getOutputType(), userOrgUnits);
             params.addItem((QueryItem) groupableItem);
           }
 
@@ -498,6 +502,14 @@ private QueryItem getQueryItem(
 
   @Override
   public QueryItem getQueryItem(String dimensionString, Program program, EventOutputType type) {
+    return getQueryItem(dimensionString, program, type, Collections.emptyList());
+  }
+
+  private QueryItem getQueryItem(
+      String dimensionString,
+      Program program,
+      EventOutputType type,
+      List<OrganisationUnit> userOrgUnits) {
     String[] split = dimensionString.split(DIMENSION_NAME_SEP);
 
     if (split.length % 2 != 1) {
@@ -523,13 +535,25 @@ public QueryItem getQueryItem(String dimensionString, Program program, EventOutp
         // FE uses HH.MM time format instead of HH:MM. This is not
         // compatible with db table/cell values
         modifyFilterWhenTimeQueryItem(queryItem, filter);
+        resolveOrgUnitDimensionIfNecessary(queryItem, filter, userOrgUnits);
         queryItem.addFilter(filter);
       }
     }
 
     return queryItem;
   }
 
+  @SneakyThrows
+  private void resolveOrgUnitDimensionIfNecessary(
+      QueryItem queryItem, QueryFilter queryFilter, List<OrganisationUnit> userOrgUnits) {
+    if (queryItem.getValueType().equals(ORGANISATION_UNIT)) {
+      List<String> filterItem = QueryFilter.getFilterItems(queryFilter.getFilter());
+      List<String> orgUnitDimensionUid =
+          dataQueryService.getOrgUnitDimensionUid(filterItem, userOrgUnits);
+      queryFilter.setFilter(String.join(OPTION_SEP, orgUnitDimensionUid));
+    }
+  }
+
   private static void modifyFilterWhenTimeQueryItem(QueryItem queryItem, QueryFilter filter) {
     if (queryItem.getItem() instanceof DataElement
         && ((DataElement) queryItem.getItem()).getValueType() == ValueType.TIME) {

diff --git a/...c/test/java/org/hisp/dhis/analytics/event/data/AbstractJdbcEventAnalyticsManagerTest.java b/...c/test/java/org/hisp/dhis/analytics/event/data/AbstractJdbcEventAnalyticsManagerTest.java
@@ -42,6 +42,7 @@
 import static org.hisp.dhis.analytics.AnalyticsAggregationType.fromAggregationType;
 import static org.hisp.dhis.analytics.DataType.NUMERIC;
 import static org.hisp.dhis.common.QueryOperator.EQ;
+import static org.hisp.dhis.common.QueryOperator.IN;
 import static org.hisp.dhis.common.QueryOperator.NE;
 import static org.hisp.dhis.common.QueryOperator.NEQ;
 import static org.hisp.dhis.common.QueryOperator.NIEQ;
@@ -860,6 +861,23 @@ void testGetSelectClauseForAggregatedEnrollments() {
     assertEquals("select pi,Yearly ", select);
   }
 
+  @Test
+  void testItemsInFilterAreQuotedForOrganisationUnit() {
+    // Given
+    QueryItem queryItem = mock(QueryItem.class);
+    QueryFilter queryFilter = new QueryFilter(IN, "A;B;C");
+    EventQueryParams params =
+        new EventQueryParams.Builder().withStartDate(new Date()).withEndDate(new Date()).build();
+    when(queryItem.getItemName()).thenReturn("anyItem");
+    when(queryItem.getValueType()).thenReturn(ValueType.ORGANISATION_UNIT);
+
+    // When
+    String sql = eventSubject.toSql(queryItem, queryFilter, params).trim();
+
+    // Then
+    assertEquals("ax.\"anyItem\" in ('A','B','C')", sql);
+  }
+
   @Test
   void testGetSelectClauseForQueryEnrollments() {
     // Given

diff --git a/dhis-2/dhis-test-e2e/src/test/java/org/hisp/dhis/analytics/event/query/EventQueryTest.java b/dhis-2/dhis-test-e2e/src/test/java/org/hisp/dhis/analytics/event/query/EventQueryTest.java
@@ -997,4 +997,69 @@ public void queryMetadataInfoForOptionSetAndOptionsWhenTenRows() throws JSONExce
     // Assert rows.
     validateRow(response, 0, List.of("Ngelehun CHC", "1"));
   }
+
+  @Test
+  public void queryWithOrgUnitDataElement() throws JSONException {
+    // Given
+    String dimensionItems =
+        String.join(
+            ";",
+            "DiszpKrYNg8",
+            "g8upMTyEZGZ",
+            "LEVEL-H1KlN4QIauv",
+            "OU_GROUP-nlX2VoouN63",
+            "USER_ORGUNIT",
+            "USER_ORGUNIT_CHILDREN",
+            "USER_ORGUNIT_GRANDCHILDREN");
+
+    String dimensionOrgUnitDataElement = "Ge7Eo3FNnbl.rypjN8CV02V:IN:" + dimensionItems;
+    String dimensionOrgUnit = "ou:USER_ORGUNIT";
+
+    String dimension = dimensionOrgUnitDataElement + "," + dimensionOrgUnit;
+
+    QueryParamsBuilder params =
+        new QueryParamsBuilder()
+            .add("dimension=" + dimension)
+            .add("headers=ouname,Ge7Eo3FNnbl.rypjN8CV02V")
+            .add("totalPages=false")
+            .add("displayProperty=NAME")
+            .add("pageSize=100")
+            .add("page=1")
+            .add("includeMetadataDetails=true")
+            .add("outputType=EVENT");
+
+    // When
+    ApiResponse response = analyticsEventActions.query().get("MoUd5BTQ3lY", params);
+
+    // Then
+    response
+        .validate()
+        .statusCode(200)
+        .body("headers", hasSize(equalTo(2)))
+        .body("rows", hasSize(equalTo(0)))
+        .body("height", equalTo(0))
+        .body("width", equalTo(0))
+        .body("headerWidth", equalTo(2));
+
+    // Assert metaData.
+    String expectedMetaData =
+        "{\"pager\":{\"isLastPage\":true,\"pageSize\":100,\"page\":1},\"items\":{\"ImspTQPwCqd\":{\"uid\":\"ImspTQPwCqd\",\"code\":\"OU_525\",\"valueType\":\"TEXT\",\"name\":\"Sierra Leone\",\"dimensionItemType\":\"ORGANISATION_UNIT\",\"totalAggregationType\":\"SUM\"},\"USER_ORGUNIT\":{\"organisationUnits\":[\"ImspTQPwCqd\"]},\"ou\":{\"uid\":\"ou\",\"dimensionType\":\"ORGANISATION_UNIT\",\"name\":\"Organisation unit\"},\"Ge7Eo3FNnbl\":{\"uid\":\"Ge7Eo3FNnbl\",\"name\":\"XX MAL RDT - Case Registration\"},\"Ge7Eo3FNnbl.rypjN8CV02V\":{\"uid\":\"rypjN8CV02V\",\"aggregationType\":\"SUM\",\"valueType\":\"TEXT\",\"name\":\"XX MAL RDT TRK - Village of Residence\",\"style\":{\"icon\":\"nullapi\\/icons\\/star_medium_positive\\/icon.svg\"},\"dimensionItemType\":\"DATA_ELEMENT\",\"totalAggregationType\":\"SUM\"},\"MoUd5BTQ3lY\":{\"uid\":\"MoUd5BTQ3lY\",\"name\":\"XX MAL RDT - Case Registration\"},\"USER_ORGUNIT_CHILDREN\":{\"organisationUnits\":[\"at6UHUQatSo\",\"TEQlaapDQoK\",\"PMa2VCrupOd\",\"qhqAxPSTUXp\",\"kJq2mPyFEHo\",\"jmIPBj66vD6\",\"Vth0fbpFcsO\",\"jUb8gELQApl\",\"fdc6uOvgoji\",\"eIQbndfxQMb\",\"O6uvpzGd5pu\",\"lc3eMKXaEfw\",\"bL4ooGhyHRQ\"]},\"rypjN8CV02V\":{\"uid\":\"rypjN8CV02V\",\"aggregationType\":\"SUM\",\"valueType\":\"TEXT\",\"name\":\"XX MAL RDT TRK - Village of Residence\",\"style\":{\"icon\":\"nullapi\\/icons\\/star_medium_positive\\/icon.svg\"},\"dimensionItemType\":\"DATA_ELEMENT\",\"totalAggregationType\":\"SUM\"},\"LAST_12_MONTHS\":{\"name\":\"Last 12 months\"}},\"dimensions\":{\"pe\":[],\"ou\":[\"ImspTQPwCqd\"],\"Ge7Eo3FNnbl.rypjN8CV02V\":[]}}";
+    String actualMetaData = new JSONObject((Map) response.extract("metaData")).toString();
+    assertEquals(expectedMetaData, actualMetaData, false);
+
+    // Assert headers.
+    validateHeader(
+        response, 0, "ouname", "Organisation unit name", "TEXT", "java.lang.String", false, true);
+    validateHeader(
+        response,
+        1,
+        "Ge7Eo3FNnbl.rypjN8CV02V",
+        "XX MAL RDT TRK - Village of Residence",
+        "ORGANISATION_UNIT",
+        "org.hisp.dhis.organisationunit.OrganisationUnit",
+        false,
+        true);
+
+    // no rows to assert
+  }
 }