fix: revert endpoint api/options causes too many DB queries (#19419)

This reverts commit e8b586f.

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

@@ -262,14 +262,13 @@ private IdentifiableObjectStore<?> getStore(Class<? extends IdentifiableObject>
 
   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

@@ -31,7 +31,6 @@
 import java.util.Date;
 import java.util.Map;
 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;
@@ -87,13 +86,6 @@ 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,7 +30,6 @@
 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;
@@ -80,13 +79,6 @@ 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/planner/DefaultQueryPlanner.java

@@ -219,12 +219,14 @@ private Query getQuery(Query query, boolean persistedOnly) {
           setQueryPathLocale(restriction);
         }
 
-        if (restriction.getQueryPath().isPersisted() && !isAttributeFilter(query, restriction)) {
+        if (restriction.getQueryPath().isPersisted()
+            && !restriction.getQueryPath().haveAlias()
+            && !Attribute.ObjectType.isValidType(restriction.getQueryPath().getPath())) {
+          pQuery
+              .getAliases()
+              .addAll(Arrays.asList(((Restriction) criterion).getQueryPath().getAlias()));
           pQuery.getCriterions().add(criterion);
           iterator.remove();
-          if (restriction.getQueryPath().haveAlias()) {
-            pQuery.getAliases().addAll(Arrays.asList(restriction.getQueryPath().getAlias()));
-          }
         }
       }
     }
@@ -266,12 +268,12 @@ private Junction handleJunction(Query query, Junction queryJunction, boolean per
           setQueryPathLocale(restriction);
         }
 
-        if (restriction.getQueryPath().isPersisted() && !isAttributeFilter(query, restriction)) {
-          if (restriction.getQueryPath().haveAlias()) {
-            criteriaJunction
-                .getAliases()
-                .addAll(Arrays.asList(restriction.getQueryPath().getAlias()));
-          }
+        if (restriction.getQueryPath().isPersisted()
+            && !restriction.getQueryPath().haveAlias(1)
+            && !Attribute.ObjectType.isValidType(restriction.getQueryPath().getPath())) {
+          criteriaJunction
+              .getAliases()
+              .addAll(Arrays.asList(((Restriction) criterion).getQueryPath().getAlias()));
           criteriaJunction.getCriterions().add(criterion);
           iterator.remove();
         } else if (persistedOnly) {
@@ -306,14 +308,4 @@ private void setQueryPathLocale(Restriction restriction) {
                 systemSettingManager.getSystemSetting(
                     SettingKey.DB_LOCALE, LocaleManager.DEFAULT_LOCALE)));
   }
-
-  /**
-   * 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());
-  }
 }

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

@@ -39,8 +39,6 @@
 import java.util.Date;
 import java.util.List;
 import java.util.stream.Collectors;
-import org.hibernate.Session;
-import org.hibernate.stat.Statistics;
 import org.hisp.dhis.common.IdentifiableObject;
 import org.hisp.dhis.common.IdentifiableObjectManager;
 import org.hisp.dhis.common.ValueType;
@@ -628,22 +626,8 @@ void testCriteriaAndRootJunctionDEG() {
     query.add(Restrictions.eq("dataElements.id", "deabcdefghD"));
     query.add(Restrictions.eq("dataElements.id", "deabcdefghE"));
     query.add(Restrictions.eq("dataElements.id", "deabcdefghF"));
-    Session session = entityManager.unwrap(Session.class);
-    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);
+    List<? extends IdentifiableObject> objects = queryService.query(query);
     assertTrue(objects.isEmpty());
-    statistics.setStatisticsEnabled(false);
   }
 
   @Test

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

@@ -231,11 +231,6 @@
       <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

@@ -31,21 +31,14 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
 import java.util.List;
-import org.hibernate.Session;
-import org.hibernate.SessionFactory;
-import org.hibernate.stat.Statistics;
 import org.hisp.dhis.jsontree.JsonObject;
 import org.hisp.dhis.web.HttpStatus;
 import org.hisp.dhis.webapi.DhisControllerConvenienceTest;
 import org.hisp.dhis.webapi.json.domain.JsonIdentifiableObject;
 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
@@ -145,47 +138,4 @@ void testOptionSetsWithDescription() {
         List.of("this-is-a", "this-is-b"),
         set.getOptions().toList(JsonIdentifiableObject::getDescription));
   }
-
-  @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);
-  }
 }