From 96b3f0ecef7fae5bf2388bccc1cf601fb854dbb3 Mon Sep 17 00:00:00 2001 From: Viet Nguyen Date: Tue, 10 Dec 2024 05:25:24 -0600 Subject: [PATCH] fix: revert endpoint api/options causes too many DB queries (#19419) This reverts commit e8b586f6a96b4934d58c5adb23605cd59cfabd10. --- .../dhis/query/JpaCriteriaQueryEngine.java | 3 +- .../dhis/query/operators/EqualOperator.java | 8 --- .../hisp/dhis/query/operators/InOperator.java | 8 --- .../query/planner/DefaultQueryPlanner.java | 32 +++++------- .../org/hisp/dhis/query/QueryServiceTest.java | 18 +------ dhis-2/dhis-test-web-api/pom.xml | 5 -- .../controller/OptionControllerTest.java | 50 ------------------- 7 files changed, 14 insertions(+), 110 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 e58c0d82616d..2d75b86e31f2 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 @@ -262,14 +262,13 @@ private IdentifiableObjectStore getStore(Class private Predicate buildPredicates(CriteriaBuilder builder, Root 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; } 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 cfcc657b90e3..dfab2be5887f 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 @@ -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 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 33ebef282f58..79b98fd9ed88 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 @@ -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 Predicate getPredicate(CriteriaBuilder builder, Root root, QueryPa 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 d0bb8afa7482..70a21d6affbd 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 @@ -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()); - } } 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 ef56cef7eeb8..ced6088804ca 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; @@ -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 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 fc6342bf3fc4..3a112764d815 100644 --- a/dhis-2/dhis-test-web-api/pom.xml +++ b/dhis-2/dhis-test-web-api/pom.xml @@ -231,11 +231,6 @@ spring-tx test - - org.hibernate - hibernate-core - test - com.google.code.gson gson 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 e817b8d048ee..e7572364da13 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,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); - } }