fix: endpoint api/options causes too many DB queries (2.39) (#19218)

dhis-2/dhis-api/src/main/java/org/hisp/dhis/attribute/Attribute.java

@@ -173,6 +173,10 @@ public static ObjectType valueOf(Class<?> type) {
       return stream(values()).filter(t -> t.type == type).findFirst().orElse(null);
     }
 
+    public static boolean isValidType(String type) {
+      return stream(values()).anyMatch(t -> t.getPropertyName().equals(type));
+    }
+
     public String getPropertyName() {
       return CaseFormat.UPPER_UNDERSCORE.converterTo(CaseFormat.LOWER_CAMEL).convert(name())
           + "Attribute";

dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/query/JpaCriteriaQueryEngine.java

@@ -244,13 +244,14 @@ private InternalHibernateGenericStore<?> getStore(Class<? extends IdentifiableOb
 
   private <Y> Predicate buildPredicates(CriteriaBuilder builder, Root<Y> root, Query query) {
     Predicate junction = builder.conjunction();
+    query.getAliases().forEach(alias -> root.join(alias).alias(alias));
     if (!query.getCriterions().isEmpty()) {
       junction = getJpaJunction(builder, query.getRootJunctionType());
       for (org.hisp.dhis.query.Criterion criterion : query.getCriterions()) {
         addPredicate(builder, root, junction, criterion);
       }
     }
-    query.getAliases().forEach(alias -> root.get(alias).alias(alias));
+
     return junction;
   }
 

dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/query/operators/EqualOperator.java

@@ -30,6 +30,7 @@
 import java.util.Collection;
 import java.util.Date;
 import javax.persistence.criteria.CriteriaBuilder;
+import javax.persistence.criteria.Join;
 import javax.persistence.criteria.Predicate;
 import javax.persistence.criteria.Root;
 import org.hibernate.criterion.Criterion;
@@ -85,6 +86,13 @@ public <Y> Predicate getPredicate(CriteriaBuilder builder, Root<Y> root, QueryPa
 
       return builder.equal(builder.size(root.get(queryPath.getPath())), value);
     }
+    if (queryPath.haveAlias()) {
+      for (Join<Y, ?> join : root.getJoins()) {
+        if (join.getAlias().equals(queryPath.getAlias()[0])) {
+          return builder.equal(join.get(queryPath.getProperty().getFieldName()), args.get(0));
+        }
+      }
+    }
     return builder.equal(root.get(queryPath.getPath()), args.get(0));
   }
 

dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/query/operators/InOperator.java

@@ -30,6 +30,7 @@
 import java.util.Collection;
 import java.util.Date;
 import javax.persistence.criteria.CriteriaBuilder;
+import javax.persistence.criteria.Join;
 import javax.persistence.criteria.Predicate;
 import javax.persistence.criteria.Root;
 import org.hibernate.criterion.Criterion;
@@ -79,6 +80,13 @@ public <Y> Predicate getPredicate(CriteriaBuilder builder, Root<Y> root, QueryPa
                   getCollectionArgs().get(0)));
     }
 
+    if (queryPath.haveAlias()) {
+      for (Join<Y, ?> join : root.getJoins()) {
+        if (join.getAlias().equals(queryPath.getAlias()[0])) {
+          return join.get(queryPath.getProperty().getFieldName()).in(getCollectionArgs().get(0));
+        }
+      }
+    }
     return root.get(queryPath.getPath()).in(getCollectionArgs().get(0));
   }
 

dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/query/operators/NotEqualOperator.java

@@ -28,6 +28,7 @@
 package org.hisp.dhis.query.operators;
 
 import javax.persistence.criteria.CriteriaBuilder;
+import javax.persistence.criteria.Join;
 import javax.persistence.criteria.Predicate;
 import javax.persistence.criteria.Root;
 import org.hibernate.criterion.Criterion;
@@ -64,6 +65,13 @@ public <Y> Predicate getPredicate(CriteriaBuilder builder, Root<Y> root, QueryPa
 
       return builder.notEqual(builder.size(root.get(queryPath.getPath())), value);
     }
+    if (queryPath.haveAlias()) {
+      for (Join<Y, ?> join : root.getJoins()) {
+        if (join.getAlias().equals(queryPath.getAlias()[0])) {
+          return builder.equal(join.get(queryPath.getProperty().getFieldName()), args.get(0));
+        }
+      }
+    }
     return builder.notEqual(root.get(queryPath.getPath()), args.get(0));
   }
 

dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/query/planner/DefaultQueryPlanner.java

@@ -34,6 +34,7 @@
 import javax.persistence.criteria.Path;
 import javax.persistence.criteria.Root;
 import lombok.AllArgsConstructor;
+import org.hisp.dhis.attribute.Attribute;
 import org.hisp.dhis.common.CodeGenerator;
 import org.hisp.dhis.query.Conjunction;
 import org.hisp.dhis.query.Criterion;
@@ -180,33 +181,13 @@ public Path<?> getQueryPath(Root<?> root, Schema schema, String path) {
    */
   private Query getQuery(Query query, boolean persistedOnly) {
     Query pQuery = Query.from(query.getSchema(), query.getRootJunctionType());
-    Iterator<Criterion> iterator = query.getCriterions().iterator();
+    Iterator<Criterion> criterions = query.getCriterions().iterator();
 
-    while (iterator.hasNext()) {
-      Criterion criterion = iterator.next();
-
-      if (criterion instanceof Junction) {
-        Junction junction = handleJunction(pQuery, (Junction) criterion, persistedOnly);
-
-        if (!junction.getCriterions().isEmpty()) {
-          pQuery.getAliases().addAll(junction.getAliases());
-          pQuery.add(junction);
-        }
-
-        if (((Junction) criterion).getCriterions().isEmpty()) {
-          iterator.remove();
-        }
-      } else if (criterion instanceof Restriction) {
-        Restriction restriction = (Restriction) criterion;
-        restriction.setQueryPath(getQueryPath(query.getSchema(), restriction.getPath()));
-
-        if (restriction.getQueryPath().isPersisted() && !restriction.getQueryPath().haveAlias()) {
-          pQuery
-              .getAliases()
-              .addAll(Arrays.asList(((Restriction) criterion).getQueryPath().getAlias()));
-          pQuery.getCriterions().add(criterion);
-          iterator.remove();
-        }
+    while (criterions.hasNext()) {
+      Criterion criterion = criterions.next();
+      if (addJunction(criterion, pQuery, persistedOnly)
+          || addRestriction(query, criterion, pQuery)) {
+        criterions.remove();
       }
     }
 
@@ -245,10 +226,12 @@ private Junction handleJunction(Query query, Junction queryJunction, boolean per
         Restriction restriction = (Restriction) criterion;
         restriction.setQueryPath(getQueryPath(query.getSchema(), restriction.getPath()));
 
-        if (restriction.getQueryPath().isPersisted() && !restriction.getQueryPath().haveAlias(1)) {
-          criteriaJunction
-              .getAliases()
-              .addAll(Arrays.asList(((Restriction) criterion).getQueryPath().getAlias()));
+        if (restriction.getQueryPath().isPersisted() && !isAttributeFilter(query, restriction)) {
+          if (restriction.getQueryPath().haveAlias()) {
+            criteriaJunction
+                .getAliases()
+                .addAll(Arrays.asList(restriction.getQueryPath().getAlias()));
+          }
           criteriaJunction.getCriterions().add(criterion);
           iterator.remove();
         } else if (persistedOnly) {
@@ -266,4 +249,58 @@ private Junction handleJunction(Query query, Junction queryJunction, boolean per
   private boolean isFilterByAttributeId(Property curProperty, String propertyName) {
     return curProperty == null && CodeGenerator.isValidUid(propertyName);
   }
+
+  /**
+   * Handle attribute filter such as /attributes?fields=id,name&filter=userAttribute:eq:true
+   *
+   * @return true if attribute filter
+   */
+  private boolean isAttributeFilter(Query query, Restriction restriction) {
+    return query.getSchema().getKlass().isAssignableFrom(Attribute.class)
+        && Attribute.ObjectType.isValidType(restriction.getQueryPath().getPath());
+  }
+
+  /**
+   * Add given criterion which is an instance of {@link Junction} to given {@link Query} pQuery. If
+   * successfully added, return TRUE indicating that the given {@link Criterion} criterion should be
+   * removed.
+   */
+  private boolean addJunction(Criterion criterion, Query pQuery, boolean persistedOnly) {
+    if (!(criterion instanceof Junction)) {
+      return false;
+    }
+    boolean shouldRemove = false;
+    Junction junction = handleJunction(pQuery, (Junction) criterion, persistedOnly);
+    if (!junction.getCriterions().isEmpty()) {
+      pQuery.getAliases().addAll(junction.getAliases());
+      pQuery.add(junction);
+    }
+    if (((Junction) criterion).getCriterions().isEmpty()) {
+      shouldRemove = true;
+    }
+    return shouldRemove;
+  }
+
+  /**
+   * Add given criterion which is an instance of {@link Restriction} to given {@link Query} pQuery.
+   * If successfully added, return TRUE indicating that the given {@link Criterion} criterion should
+   * be removed.
+   */
+  private boolean addRestriction(Query query, Criterion criterion, Query pQuery) {
+    if (!(criterion instanceof Restriction)) {
+      return false;
+    }
+    boolean shouldRemove = false;
+    Restriction restriction = (Restriction) criterion;
+    restriction.setQueryPath(getQueryPath(query.getSchema(), restriction.getPath()));
+
+    if (restriction.getQueryPath().isPersisted() && !isAttributeFilter(query, restriction)) {
+      pQuery.getCriterions().add(criterion);
+      shouldRemove = true;
+      if (restriction.getQueryPath().haveAlias()) {
+        pQuery.getAliases().addAll(Arrays.asList(restriction.getQueryPath().getAlias()));
+      }
+    }
+    return shouldRemove;
+  }
 }

dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/query/QueryServiceTest.java

@@ -39,6 +39,9 @@
 import java.util.Date;
 import java.util.List;
 import java.util.stream.Collectors;
+import org.hibernate.Session;
+import org.hibernate.SessionFactory;
+import org.hibernate.stat.Statistics;
 import org.hisp.dhis.common.IdentifiableObject;
 import org.hisp.dhis.common.IdentifiableObjectManager;
 import org.hisp.dhis.common.ValueType;
@@ -66,6 +69,8 @@ class QueryServiceTest extends SingleSetupIntegrationTestBase {
 
   @Autowired private IdentifiableObjectManager identifiableObjectManager;
 
+  @Autowired private SessionFactory sessionFactory;
+
   @BeforeEach
   void createDataElements() {
     DataElement dataElementA = createDataElement('A');
@@ -545,8 +550,22 @@ void testCriteriaAndRootJunctionDEG() {
     query.add(Restrictions.eq("dataElements.id", "deabcdefghD"));
     query.add(Restrictions.eq("dataElements.id", "deabcdefghE"));
     query.add(Restrictions.eq("dataElements.id", "deabcdefghF"));
-    List<? extends IdentifiableObject> objects = queryService.query(query);
+    Session session = sessionFactory.getCurrentSession();
+    Statistics statistics = session.getSessionFactory().getStatistics();
+    statistics.setStatisticsEnabled(true);
+    List<? extends IdentifiableObject> objects = queryService.query(query);
+    String[] queries = statistics.getQueries();
+    boolean findQuery = false;
+    for (String q : queries) {
+      if (q.equals(
+          "select generatedAlias0 from DataElementGroup as generatedAlias0 inner join generatedAlias0.members as members where ( members.uid=:param0 ) and ( members.uid=:param1 ) and ( members.uid=:param2 ) and ( members.uid=:param3 ) and ( members.uid=:param4 ) and ( members.uid=:param5 )")) {
+        findQuery = true;
+        break;
+      }
+    }
+    assertTrue(findQuery);
     assertTrue(objects.isEmpty());
+    statistics.setStatisticsEnabled(false);
   }
 
   @Test

dhis-2/dhis-test-web-api/pom.xml

@@ -251,6 +251,11 @@
       <artifactId>spring-tx</artifactId>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>org.hibernate</groupId>
+      <artifactId>hibernate-core</artifactId>
+      <scope>test</scope>
+    </dependency>
     <dependency>
       <groupId>com.google.code.gson</groupId>
       <artifactId>gson</artifactId>

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

@@ -27,14 +27,23 @@
  */
 package org.hisp.dhis.webapi.controller;
 
+import static org.hisp.dhis.web.WebClientUtils.assertStatus;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
+import org.hibernate.Session;
+import org.hibernate.SessionFactory;
+import org.hibernate.stat.Statistics;
 import org.hisp.dhis.jsontree.JsonResponse;
 import org.hisp.dhis.web.HttpStatus;
 import org.hisp.dhis.webapi.DhisControllerConvenienceTest;
+import org.hisp.dhis.webapi.json.domain.JsonOptionSet;
 import org.junit.jupiter.api.Test;
+import org.springframework.beans.factory.annotation.Autowired;
 
 class OptionControllerTest extends DhisControllerConvenienceTest {
+
+  @Autowired private SessionFactory sessionFactory;
+
   @Test
   void testUpdateOptionWithSortOrderGap() {
     // Create OptionSet with two Options
@@ -69,4 +78,48 @@ void testUpdateOptionWithSortOrderGap() {
     assertEquals("Uh4HvjK6zg3", response.getString("options[1].id").string());
     assertEquals(2, response.getNumber("options[1].sortOrder").intValue());
   }
+
+  @Test
+  void testQueryOptionsByOptionSetIds() {
+    Session session = sessionFactory.getCurrentSession();
+    String id =
+        assertStatus(
+            HttpStatus.CREATED,
+            POST(
+                "/optionSets/",
+                "{'name': 'test', 'version': 2, 'valueType': 'TEXT', 'description':'desc' }"));
+    assertStatus(
+        HttpStatus.CREATED,
+        POST(
+            "/options/",
+            "{'optionSet': { 'id':'"
+                + id
+                + "'}, 'id':'Uh4HvjK6zg3', 'code': 'A', 'name': 'Anna', 'description': 'this-is-a'}"));
+    assertStatus(
+        HttpStatus.CREATED,
+        POST(
+            "/options/",
+            "{'optionSet': { 'id':'"
+                + id
+                + "'},'id':'BQMei56UBl6','code': 'B', 'name': 'Betta', 'description': 'this-is-b'}"));
+    Statistics statistics = session.getSessionFactory().getStatistics();
+    statistics.setStatisticsEnabled(true);
+    assertEquals(0, statistics.getQueryExecutionCount());
+    JsonOptionSet set =
+        GET(String.format("/options?filter=optionSet.id:in:[%s,%s]", id, "TESTUIDA"))
+            .content()
+            .as(JsonOptionSet.class);
+    assertEquals(2, set.getOptions().size());
+
+    // Verify that there is no n+1 queries to Option table
+    int count = 0;
+    for (String q : statistics.getQueries()) {
+      if (q.contains("from Option")) {
+        count++;
+      }
+    }
+    assertEquals(2, count);
+
+    statistics.setStatisticsEnabled(false);
+  }
 }