From 36b3a4682c04910c98f7171bfe5babd9e14fcdab Mon Sep 17 00:00:00 2001 From: Viet Nguyen Date: Thu, 24 Oct 2024 15:33:17 +0700 Subject: [PATCH] fix: option filter by optionSetid should go to database --- .../hisp/dhis/query/operators/InOperator.java | 5 +++ .../query/planner/DefaultQueryPlanner.java | 36 +++++++++++------- .../dhis/external/conf/ConfigurationKey.java | 6 +++ .../org/hisp/dhis/config/HibernateConfig.java | 8 ++++ .../controller/OptionControllerTest.java | 38 +++++++++++++++++++ 5 files changed, 79 insertions(+), 14 deletions(-) diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/query/operators/InOperator.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/query/operators/InOperator.java index 3670033e5978..10b87723139a 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/query/operators/InOperator.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/query/operators/InOperator.java @@ -78,6 +78,11 @@ public Predicate getPredicate(CriteriaBuilder builder, Root 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/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/query/planner/DefaultQueryPlanner.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/query/planner/DefaultQueryPlanner.java index 1757a0c573b9..d7ddca5535a2 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/query/planner/DefaultQueryPlanner.java +++ b/dhis-2/dhis-services/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/dhis-2/dhis-support/dhis-support-external/src/main/java/org/hisp/dhis/external/conf/ConfigurationKey.java b/dhis-2/dhis-support/dhis-support-external/src/main/java/org/hisp/dhis/external/conf/ConfigurationKey.java index 2ae21e7515a0..caae8b168242 100644 --- a/dhis-2/dhis-support/dhis-support-external/src/main/java/org/hisp/dhis/external/conf/ConfigurationKey.java +++ b/dhis-2/dhis-support/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/dhis-2/dhis-support/dhis-support-hibernate/src/main/java/org/hisp/dhis/config/HibernateConfig.java b/dhis-2/dhis-support/dhis-support-hibernate/src/main/java/org/hisp/dhis/config/HibernateConfig.java index c26fb5e58ef9..1536fc88ac80 100644 --- a/dhis-2/dhis-support/dhis-support-hibernate/src/main/java/org/hisp/dhis/config/HibernateConfig.java +++ b/dhis-2/dhis-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-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/OptionControllerTest.java b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/OptionControllerTest.java index 116770a383d2..49fd8b49213d 100644 --- a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/OptionControllerTest.java +++ b/dhis-2/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); + } }