From be77a5965833cadbde55882d765e677b9c3fc013 Mon Sep 17 00:00:00 2001 From: Viet Nguyen Date: Tue, 10 Dec 2024 04:29:56 -0600 Subject: [PATCH] Revert "fix: endpoint api/options causes too many DB queries (#19114)" This reverts commit 2f4111bbdaee89293529b4f4dc23c6e0ef0d5436. --- .../dhis/query/JpaCriteriaQueryEngine.java | 11 +++--- .../org/hisp/dhis/query/JpaQueryUtils.java | 7 ++++ .../dhis/query/operators/EqualOperator.java | 8 ---- .../hisp/dhis/query/operators/InOperator.java | 9 +---- .../query/planner/DefaultQueryPlanner.java | 33 ++++++---------- .../dhis/external/conf/ConfigurationKey.java | 6 --- .../org/hisp/dhis/query/QueryServiceTest.java | 18 +-------- dhis-2/dhis-test-web-api/pom.xml | 10 ----- .../controller/OptionControllerTest.java | 38 ------------------- 9 files changed, 27 insertions(+), 113 deletions(-) diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/query/JpaCriteriaQueryEngine.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/query/JpaCriteriaQueryEngine.java index 9f9a02b251c7..8a3176f33286 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/query/JpaCriteriaQueryEngine.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/query/JpaCriteriaQueryEngine.java @@ -221,15 +221,13 @@ private InternalHibernateGenericStore getStore private Predicate buildPredicates(CriteriaBuilder builder, Root 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 Predicate getPredicate( if (restriction == null || restriction.getOperator() == null) { return null; } + return restriction.getOperator().getPredicate(builder, root, restriction.getQueryPath()); } @@ -253,7 +252,8 @@ private void addPredicate( Root 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 void addJunction( Root 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 index 58768be647bf..fd63fdf1366a 100644 --- 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, Order> getOrders(CriteriaBuilder builder, String field) { + Function, Order> order = root -> builder.asc(root.get(field)); + + return order; + } + /** * Generate a String comparison Predicate base on input parameters. * diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/query/operators/EqualOperator.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/query/operators/EqualOperator.java index 4a521ca46d42..eca23c6e0e50 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/query/operators/EqualOperator.java +++ b/dhis-2/dhis-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 Predicate getPredicate(CriteriaBuilder builder, Root root, QueryPa return builder.equal(builder.size(root.get(queryPath.getPath())), value); } - if (queryPath.haveAlias()) { - for (Join 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/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 2156d37fb07f..3670033e5978 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 @@ -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 Predicate getPredicate(CriteriaBuilder builder, Root root, QueryPa queryPath.getProperty().getItemKlass(), getCollectionArgs().get(0))); } - if (queryPath.haveAlias()) { - for (Join 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/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 cf5e5757b1d7..1757a0c573b9 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,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/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 15d0c5154408..e0923fb6f83a 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 @@ -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/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 index 8a401834d99c..d6a5715a8f86 100644 --- 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 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 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 index ac54aae39485..8589e77aff19 100644 --- a/dhis-2/dhis-test-web-api/pom.xml +++ b/dhis-2/dhis-test-web-api/pom.xml @@ -157,16 +157,6 @@ spring-webmvc test - - jakarta.persistence - jakarta.persistence-api - test - - - org.hibernate - hibernate-core-jakarta - test - org.slf4j slf4j-api 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 49fd8b49213d..116770a383d2 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,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); - } }