Revert "fix: endpoint api/options causes too many DB queries (#19114)" (

#19418)
dhis2 · Dec 10, 2024 · 99c00a5 · 99c00a5
1 parent f09da55
commit 99c00a5
Show file tree

Hide file tree

Showing 10 changed files with 27 additions and 121 deletions.
diff --git a/...-services/dhis-service-core/src/main/java/org/hisp/dhis/query/JpaCriteriaQueryEngine.java b/...-services/dhis-service-core/src/main/java/org/hisp/dhis/query/JpaCriteriaQueryEngine.java
@@ -221,15 +221,13 @@ private <E extends IdentifiableObject> InternalHibernateGenericStore<E> getStore
 
   private <Y> Predicate buildPredicates(CriteriaBuilder builder, Root<Y> root, Query query) {
     Predicate junction = builder.conjunction();
-    if (query.isEmpty()) return junction;
-    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;
   }
 
@@ -245,6 +243,7 @@ private <Y> Predicate getPredicate(
     if (restriction == null || restriction.getOperator() == null) {
       return null;
     }
+
     return restriction.getOperator().getPredicate(builder, root, restriction.getQueryPath());
   }
 
@@ -253,7 +252,8 @@ private <Y> void addPredicate(
       Root<Y> root,
       Predicate predicateJunction,
       org.hisp.dhis.query.Criterion criterion) {
-    if (criterion instanceof Restriction restriction) {
+    if (criterion instanceof Restriction) {
+      Restriction restriction = (Restriction) criterion;
       Predicate predicate = getPredicate(builder, root, restriction);
 
       if (predicate != null) {
@@ -281,7 +281,8 @@ private <Y> void addJunction(
       Root<Y> root,
       Predicate junction,
       org.hisp.dhis.query.Criterion criterion) {
-    if (criterion instanceof Restriction restriction) {
+    if (criterion instanceof Restriction) {
+      Restriction restriction = (Restriction) criterion;
       Predicate predicate = getPredicate(builder, root, restriction);
 
       if (predicate != null) {

diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/query/JpaQueryUtils.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/query/JpaQueryUtils.java
@@ -31,6 +31,7 @@
 
 import jakarta.persistence.criteria.CriteriaBuilder;
 import jakarta.persistence.criteria.Expression;
+import jakarta.persistence.criteria.Order;
 import jakarta.persistence.criteria.Path;
 import jakarta.persistence.criteria.Predicate;
 import jakarta.persistence.criteria.Root;
@@ -51,6 +52,12 @@
 public class JpaQueryUtils {
   public static final String HIBERNATE_CACHEABLE_HINT = "org.hibernate.cacheable";
 
+  public static Function<Root<?>, Order> getOrders(CriteriaBuilder builder, String field) {
+    Function<Root<?>, Order> order = root -> builder.asc(root.get(field));
+
+    return order;
+  }
+
   /**
    * Generate a String comparison Predicate base on input parameters.
    *

diff --git a/...services/dhis-service-core/src/main/java/org/hisp/dhis/query/operators/EqualOperator.java b/...services/dhis-service-core/src/main/java/org/hisp/dhis/query/operators/EqualOperator.java
@@ -28,7 +28,6 @@
 package org.hisp.dhis.query.operators;
 
 import jakarta.persistence.criteria.CriteriaBuilder;
-import jakarta.persistence.criteria.Join;
 import jakarta.persistence.criteria.Predicate;
 import jakarta.persistence.criteria.Root;
 import java.util.Collection;
@@ -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));
   }
 

diff --git a/...is-services/dhis-service-core/src/main/java/org/hisp/dhis/query/operators/InOperator.java b/...is-services/dhis-service-core/src/main/java/org/hisp/dhis/query/operators/InOperator.java
@@ -28,7 +28,6 @@
 package org.hisp.dhis.query.operators;
 
 import jakarta.persistence.criteria.CriteriaBuilder;
-import jakarta.persistence.criteria.Join;
 import jakarta.persistence.criteria.Predicate;
 import jakarta.persistence.criteria.Root;
 import java.util.Collection;
@@ -79,13 +78,7 @@ public <Y> Predicate getPredicate(CriteriaBuilder builder, Root<Y> root, QueryPa
                   queryPath.getProperty().getItemKlass(),
                   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));
   }
 

diff --git a/...ices/dhis-service-core/src/main/java/org/hisp/dhis/query/planner/DefaultQueryPlanner.java b/...ices/dhis-service-core/src/main/java/org/hisp/dhis/query/planner/DefaultQueryPlanner.java
@@ -217,12 +217,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()));
-          }
         }
       }
     }
@@ -264,15 +266,14 @@ 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) {
           throw new RuntimeException(
               "Path "
@@ -299,14 +300,4 @@ private boolean isFilterByAttributeId(Property curProperty, String propertyName)
   private void setQueryPathLocale(Restriction restriction) {
     restriction.getQueryPath().setLocale(UserSettings.getCurrentSettings().getUserDbLocale());
   }
-
-  /**
-   * 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());
-  }
 }
diff --git a/...ort/dhis-support-external/src/main/java/org/hisp/dhis/external/conf/ConfigurationKey.java b/...ort/dhis-support-external/src/main/java/org/hisp/dhis/external/conf/ConfigurationKey.java
@@ -136,12 +136,6 @@ public enum ConfigurationKey {
   /** Sets 'hibernate.cache.use_query_cache'. (default: true) */
   USE_QUERY_CACHE("hibernate.cache.use_query_cache", "true", false),
 
-  /**
-   * Show SQL statements generated by hibernate in the log. Can be 'true', 'false'. (default: false)
-   * This will also enable hibernate.format_sql and hibernate.highlight_sql
-   */
-  HIBERNATE_SHOW_SQL("hibernate.show_sql", "false", false),
-
   /**
    * Sets 'hibernate.hbm2ddl.auto' (default: validate). This can be overridden by the same property
    * loaded by any class implementing {@link DhisConfigurationProvider} like {@link

diff --git a/...is-support/dhis-support-hibernate/src/main/java/org/hisp/dhis/config/HibernateConfig.java b/...is-support/dhis-support-hibernate/src/main/java/org/hisp/dhis/config/HibernateConfig.java
@@ -183,14 +183,6 @@ private Properties getAdditionalProperties(DhisConfigurationProvider dhisConfig)
 
     properties.put(AvailableSettings.HBM2DDL_AUTO, getHibernateSchemaAction(dhisConfig));
 
-    properties.put(
-        AvailableSettings.SHOW_SQL, dhisConfig.getProperty(ConfigurationKey.HIBERNATE_SHOW_SQL));
-    properties.put(
-        AvailableSettings.FORMAT_SQL, dhisConfig.getProperty(ConfigurationKey.HIBERNATE_SHOW_SQL));
-    properties.put(
-        AvailableSettings.HIGHLIGHT_SQL,
-        dhisConfig.getProperty(ConfigurationKey.HIBERNATE_SHOW_SQL));
-
     // TODO: this is anti-pattern and should be turn off
     properties.put("hibernate.allow_update_outside_transaction", "true");
 

diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/query/QueryServiceTest.java b/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;
@@ -597,22 +595,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

diff --git a/dhis-2/dhis-test-web-api/pom.xml b/dhis-2/dhis-test-web-api/pom.xml
@@ -157,16 +157,6 @@
       <artifactId>spring-webmvc</artifactId>
       <scope>test</scope>
     </dependency>
-    <dependency>
-      <groupId>jakarta.persistence</groupId>
-      <artifactId>jakarta.persistence-api</artifactId>
-      <scope>test</scope>
-    </dependency>
-    <dependency>
-      <groupId>org.hibernate</groupId>
-      <artifactId>hibernate-core-jakarta</artifactId>
-      <scope>test</scope>
-    </dependency>
     <dependency>
       <groupId>org.slf4j</groupId>
       <artifactId>slf4j-api</artifactId>

diff --git a/...dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/OptionControllerTest.java b/...dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/OptionControllerTest.java
@@ -31,8 +31,6 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
 import java.util.List;
-import org.hibernate.Session;
-import org.hibernate.stat.Statistics;
 import org.hisp.dhis.http.HttpStatus;
 import org.hisp.dhis.jsontree.JsonObject;
 import org.hisp.dhis.test.webapi.H2ControllerIntegrationTestBase;
@@ -142,40 +140,4 @@ void testOptionSetsWithDescription() {
         List.of("this-is-a", "this-is-b"),
         set.getOptions().toList(JsonIdentifiableObject::getDescription));
   }
-
-  @Test
-  void testQueryOptionsByOptionSetIds() {
-    Session session = entityManager.unwrap(Session.class);
-
-    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, statistics.getQueryExecutionCount());
-    assertEquals(2, set.getOptions().size());
-    statistics.setStatisticsEnabled(false);
-  }
 }