fix: option filter by optionSetid should go to database

dhis2 · Oct 28, 2024 · 36b3a46 · 36b3a46
1 parent 112bd6f
commit 36b3a46
Show file tree

Hide file tree

Showing 5 changed files with 79 additions and 14 deletions.
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
@@ -78,6 +78,11 @@ public <Y> Predicate getPredicate(CriteriaBuilder builder, Root<Y> root, QueryPa
                   queryPath.getProperty().getItemKlass(),
                   getCollectionArgs().get(0)));
     }
+    if (queryPath.haveAlias()) {
+      return root.join(queryPath.getAlias()[0])
+          .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,11 @@ private Query getQuery(Query query, boolean persistedOnly) {
           setQueryPathLocale(restriction);
         }
 
-        if (restriction.getQueryPath().isPersisted()
-            && !restriction.getQueryPath().haveAlias()
-            && !Attribute.ObjectType.isValidType(restriction.getQueryPath().getPath())) {
-          pQuery
-              .getAliases()
-              .addAll(Arrays.asList(((Restriction) criterion).getQueryPath().getAlias()));
+        if (restriction.getQueryPath().haveAlias() || !isAttributeFilter(query, restriction)) {
+          pQuery.getAliases().addAll(Arrays.asList(restriction.getQueryPath().getAlias()));
+        }
+
+        if (restriction.getQueryPath().isPersisted()) {
           pQuery.getCriterions().add(criterion);
           iterator.remove();
         }
@@ -266,19 +265,18 @@ private Junction handleJunction(Query query, Junction queryJunction, boolean per
           setQueryPathLocale(restriction);
         }
 
-        if (restriction.getQueryPath().isPersisted()
-            && !restriction.getQueryPath().haveAlias(1)
-            && !Attribute.ObjectType.isValidType(restriction.getQueryPath().getPath())) {
-          criteriaJunction
-              .getAliases()
-              .addAll(Arrays.asList(((Restriction) criterion).getQueryPath().getAlias()));
+        if (restriction.getQueryPath().haveAlias() || !isAttributeFilter(query, restriction)) {
+          criteriaJunction.getAliases().addAll(Arrays.asList(restriction.getQueryPath().getAlias()));
+        }
+
+        if (restriction.getQueryPath().isPersisted()) {
           criteriaJunction.getCriterions().add(criterion);
           iterator.remove();
         } else if (persistedOnly) {
           throw new RuntimeException(
               "Path "
-                  + restriction.getQueryPath().getPath()
-                  + " is not fully persisted, unable to build persisted only query plan.");
+              + restriction.getQueryPath().getPath()
+              + " is not fully persisted, unable to build persisted only query plan.");
         }
       }
     }
@@ -300,4 +298,14 @@ 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
@@ -127,6 +127,12 @@ 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
@@ -179,6 +179,14 @@ private Properties getAdditionalProperties(DhisConfigurationProvider dhisConfig)
           MissingCacheStrategy.CREATE.getExternalRepresentation());
     }
 
+    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-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,6 +31,8 @@
 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;
@@ -140,4 +142,40 @@ 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);
+  }
 }