Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: revert endpoint api/options causes too many DB queries #19418

Merged
merged 1 commit into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -245,6 +243,7 @@ private <Y> Predicate getPredicate(
if (restriction == null || restriction.getOperator() == null) {
return null;
}

return restriction.getOperator().getPredicate(builder, root, restriction.getQueryPath());
}

Expand All @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
}
}
}
}
Expand Down Expand Up @@ -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 "
Expand All @@ -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());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
10 changes: 0 additions & 10 deletions dhis-2/dhis-test-web-api/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}
Loading