diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/sql/QueryBuilder.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/sql/QueryBuilder.java index 99d4a82f0f48..30b153faf5aa 100644 --- a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/sql/QueryBuilder.java +++ b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/sql/QueryBuilder.java @@ -87,7 +87,8 @@ public final class QueryBuilder { private static final Pattern WHERE_AND = Pattern.compile("([\n\t ]+)WHERE[\n\t ]+(?:1=1)?[\n\t ]+AND[\n\t ]+"); - private static final Pattern WITH_START = Pattern.compile("^\\s*[a-z_]{1,30}\\s+AS\\s*\\(\\s*$"); + private static final Pattern WITH_START = + Pattern.compile("^\\s*[a-z_]{1,30}\\s+AS(?:\\s+MATERIALIZED)?\\s*\\(\\s*$"); private static final Pattern WITH_END_COMMA = Pattern.compile("^\\s*\\)\\s*,\\s*$"); private static final Pattern WITH_END_SELECT = Pattern.compile("^\\s*\\)\\s*$"); private static final Pattern WITH_END_COMMA_SELECT = diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/datavalue/hibernate/HibernateDataExportStore.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/datavalue/hibernate/HibernateDataExportStore.java index 73e322f1ae7c..f4929bd7c455 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/datavalue/hibernate/HibernateDataExportStore.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/datavalue/hibernate/HibernateDataExportStore.java @@ -31,6 +31,7 @@ import static java.lang.System.currentTimeMillis; import static java.util.function.Function.identity; +import static org.apache.commons.collections4.CollectionUtils.isEmpty; import static org.hisp.dhis.query.JpaQueryUtils.generateSQlQueryForSharingCheck; import static org.hisp.dhis.security.acl.AclService.LIKE_READ_DATA; import static org.hisp.dhis.user.CurrentUserUtil.getCurrentUserDetails; @@ -131,6 +132,12 @@ public Stream exportValues(@Nonnull DataExportParams params) { static QueryBuilder createExportQuery( DataExportParams params, SQL.QueryAPI api, UserDetails currentUser) { + String aocAclSql = null; + boolean isSuper = currentUser.isSuper(); + // explicit AOCs mean they are already sharing checked + if ((params.getAttributeOptionCombos() == null || params.getAttributeOptionCombos().isEmpty()) + && !isSuper) + aocAclSql = generateSQlQueryForSharingCheck("co.sharing", currentUser, LIKE_READ_DATA); String sql = """ WITH @@ -177,7 +184,14 @@ ou_with_descendants_ids AS ( FROM ou_ids JOIN organisationunit root USING (organisationunitid) JOIN organisationunit ou ON ou.path LIKE root.path || '%' - ) + ), + aoc_access AS MATERIALIZED ( + SELECT aoc.categoryoptioncomboid, aoc.uid + FROM categoryoptioncombo aoc + WHERE NOT EXISTS (SELECT 1 FROM categoryoptioncombos_categoryoptions coc_co + JOIN categoryoption co ON coc_co.categoryoptionid = co.categoryoptionid + WHERE coc_co.categoryoptioncomboid = aoc.categoryoptioncomboid AND NOT (:aocAccess)) + ), SELECT de.uid AS deid, pe.iso, @@ -201,31 +215,22 @@ JOIN organisationunit root USING (organisationunitid) JOIN period pe ON dv.periodid = pe.periodid JOIN organisationunit ou ON dv.sourceid = ou.organisationunitid JOIN categoryoptioncombo coc ON dv.categoryoptioncomboid = coc.categoryoptioncomboid + JOIN aoc_access ON dv.attributeoptioncomboid = aoc_access.categoryoptioncomboid JOIN categoryoptioncombo aoc ON dv.attributeoptioncomboid = aoc.categoryoptioncomboid WHERE 1=1 AND coc.uid = ANY(:coc) AND aoc.uid = ANY(:aoc) AND dv.lastupdated >= :lastUpdated AND dv.deleted = :deleted - AND ou.hierarchylevel = :level - -- access check below must be 1 line for erasure - AND NOT EXISTS (SELECT 1 FROM categoryoptioncombos_categoryoptions coc_co \ - JOIN categoryoption co ON coc_co.categoryoptionid = co.categoryoptionid \ - WHERE coc_co.categoryoptioncomboid = aoc.categoryoptioncomboid AND NOT (:aocAccess))"""; + AND ou.hierarchylevel = :level"""; + Date lastUpdated = params.getLastUpdated(); if (lastUpdated == null && params.getLastUpdatedDuration() != null) lastUpdated = new Date(currentTimeMillis() - params.getLastUpdatedDuration().toMillis()); - String aocAclSql = null; - boolean isSuper = currentUser.isSuper(); - // explicit AOCs mean they are already sharing checked - if ((params.getAttributeOptionCombos() == null || params.getAttributeOptionCombos().isEmpty()) - && !isSuper) - aocAclSql = generateSQlQueryForSharingCheck("co.sharing", currentUser, LIKE_READ_DATA); - boolean descendants = params.isIncludeDescendants(); List orders = params.getOrders(); - if (orders == null || orders.isEmpty()) orders = List.of(Order.PE, Order.CREATED, Order.DE); + if (isEmpty(orders)) orders = List.of(Order.PE, Order.CREATED, Order.DE); List oug = params.getOrganisationUnitGroups(); Set ouCapture = currentUser.getUserOrgUnitIds(); @@ -251,6 +256,7 @@ AND NOT EXISTS (SELECT 1 FROM categoryoptioncombos_categoryoptions coc_co \ .setDynamicClause("aocAccess", aocAclSql) .eraseNullParameterLines() // keep params below even when null + .eraseJoinLine("aoc_access", aocAclSql == null) .eraseJoinLine("de_ids", !params.hasDataElementFilters()) .eraseJoinLine("pe_ids", !params.hasPeriodFilters()) .eraseJoinLine("ou_with_descendants_ids", !descendants || !params.hasOrgUnitFilters()) diff --git a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/datavalue/hibernate/DataExportQueryBuilderTest.java b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/datavalue/hibernate/DataExportQueryBuilderTest.java index 56322cefa350..4b0ab7ac553c 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/datavalue/hibernate/DataExportQueryBuilderTest.java +++ b/dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/datavalue/hibernate/DataExportQueryBuilderTest.java @@ -32,10 +32,13 @@ import static org.hisp.dhis.datavalue.DataExportParams.Order.*; import static org.hisp.dhis.datavalue.hibernate.HibernateDataExportStore.createExportQuery; import static org.hisp.dhis.period.Period.of; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.Date; import java.util.List; import java.util.Set; +import java.util.concurrent.atomic.AtomicReference; import org.hisp.dhis.common.UID; import org.hisp.dhis.datavalue.DataExportParams; import org.hisp.dhis.sql.AbstractQueryBuilderTest; @@ -436,6 +439,13 @@ ou_ids AS ( UNION (SELECT ougm.organisationunitid FROM orgunitgroupmembers ougm JOIN orgunitgroup oug ON ougm.orgunitgroupid = oug.orgunitgroupid JOIN organisationunit ou ON ougm.organisationunitid = ou.organisationunitid WHERE oug.uid = ANY(:oug) AND ou.uid = ANY(:capture)) ) ou_all WHERE organisationunitid IS NOT NULL + ), + aoc_access AS MATERIALIZED ( + SELECT aoc.categoryoptioncomboid, aoc.uid + FROM categoryoptioncombo aoc + WHERE NOT EXISTS (SELECT 1 FROM categoryoptioncombos_categoryoptions coc_co + JOIN categoryoption co ON coc_co.categoryoptionid = co.categoryoptionid + WHERE coc_co.categoryoptioncomboid = aoc.categoryoptioncomboid AND NOT ( ( co.sharing->>'owner' is null or co.sharing->>'owner' = 'null') or co.sharing->>'public' like '__r_____' or co.sharing->>'public' is null or (jsonb_has_user_id( co.sharing, 'null') = true and jsonb_check_user_access( co.sharing, 'null', '__r_____' ) = true ) )) ) SELECT de.uid AS deid, @@ -457,9 +467,9 @@ ou_ids AS ( JOIN period pe ON dv.periodid = pe.periodid JOIN organisationunit ou ON dv.sourceid = ou.organisationunitid JOIN categoryoptioncombo coc ON dv.categoryoptioncomboid = coc.categoryoptioncomboid + JOIN aoc_access ON dv.attributeoptioncomboid = aoc_access.categoryoptioncomboid JOIN categoryoptioncombo aoc ON dv.attributeoptioncomboid = aoc.categoryoptioncomboid WHERE dv.deleted = :deleted - AND NOT EXISTS (SELECT 1 FROM categoryoptioncombos_categoryoptions coc_co JOIN categoryoption co ON coc_co.categoryoptionid = co.categoryoptionid WHERE coc_co.categoryoptioncomboid = aoc.categoryoptioncomboid AND NOT ( ( co.sharing->>'owner' is null or co.sharing->>'owner' = 'null') or co.sharing->>'public' like '__r_____' or co.sharing->>'public' is null or (jsonb_has_user_id( co.sharing, 'null') = true and jsonb_check_user_access( co.sharing, 'null', '__r_____' ) = true ) )) ORDER BY pe.startdate, pe.enddate, dv.created, deid""", Set.of("oug", "capture", "deleted"), createExportQuery(params, createSpyQuery(), currentUser)); @@ -500,4 +510,36 @@ void testFilter_LastUpdated() { Set.of("lastUpdated"), createExportQuery(params, createSpyQuery(), new SystemUser())); } + + @Test + void testAocAccess_nonSuperuser_usesMaterializedCte() { + UserDetails user = UserDetails.empty().build(); + DataExportParams params = DataExportParams.builder().includeDeleted(true).build(); + AtomicReference captured = new AtomicReference<>(); + SQL.QueryAPI spy = SQL.spy(captured::set, (name, param) -> {}); + createExportQuery(params, spy, user).stream(); + String sql = captured.get(); + assertTrue(sql.contains("aoc_access AS MATERIALIZED"), "expected MATERIALIZED CTE: " + sql); + assertTrue(sql.contains("JOIN aoc_access ON"), "expected join to aoc_access CTE: " + sql); + assertTrue( + sql.contains("JOIN categoryoptioncombo aoc ON"), + "expected direct categoryoptioncombo join for aoc alias: " + sql); + assertFalse( + sql.contains("AND NOT EXISTS"), + "NOT EXISTS should not appear inline in WHERE clause: " + sql); + } + + @Test + void testAocAccess_superuser_usesDirectJoin() { + DataExportParams params = DataExportParams.builder().includeDeleted(true).build(); + AtomicReference captured = new AtomicReference<>(); + SQL.QueryAPI spy = SQL.spy(captured::set, (name, param) -> {}); + createExportQuery(params, spy, new SystemUser()).stream(); + String sql = captured.get(); + assertFalse(sql.contains("aoc_access"), "expected no aoc_access CTE for superuser: " + sql); + assertTrue( + sql.contains("JOIN categoryoptioncombo aoc ON"), + "expected direct categoryoptioncombo join: " + sql); + assertFalse(sql.contains("NOT EXISTS"), "expected no NOT EXISTS correlated subquery: " + sql); + } } diff --git a/dhis-2/dhis-test-performance/src/test/java/org/hisp/dhis/test/platform/AggregateDataExportPerformanceTest.java b/dhis-2/dhis-test-performance/src/test/java/org/hisp/dhis/test/platform/AggregateDataExportPerformanceTest.java new file mode 100644 index 000000000000..16ba8c0f7a49 --- /dev/null +++ b/dhis-2/dhis-test-performance/src/test/java/org/hisp/dhis/test/platform/AggregateDataExportPerformanceTest.java @@ -0,0 +1,104 @@ +/* + * Copyright (c) 2004-2025, University of Oslo + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * 3. Neither the name of the copyright holder nor the names of its contributors + * may be used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR + * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package org.hisp.dhis.test.platform; + +import static io.gatling.javaapi.core.CoreDsl.*; +import static io.gatling.javaapi.http.HttpDsl.*; + +import io.gatling.javaapi.core.ClosedInjectionStep; +import io.gatling.javaapi.core.ScenarioBuilder; +import io.gatling.javaapi.core.Simulation; +import io.gatling.javaapi.http.HttpProtocolBuilder; +import java.time.LocalDate; +import java.time.format.DateTimeFormatter; + +/** + * Performance test for data value set export with org unit descendants (DHIS2-21490). + * + *

Exercises {@code GET /api/dataValueSets/} with {@code children=true}, which activates the + * {@code ou_with_descendants_ids} CTE in {@code HibernateDataExportStore}. The original query used + * an O(n²) self-join across the entire org unit table; the fix drives from the selected roots and + * uses the {@code varchar_pattern_ops} path index to find descendants in O(roots × log n). + * + *

Assumes a Sierra Leone demo database at {@code localhost:8080}. UIDs are stable across demo + * database versions: + * + *

    + *
  • Sierra Leone root org unit: {@code ImspTQPwCqd} + *
  • ART monthly summary dataset: {@code lyLU2wR22tC} + *
+ */ +public class AggregateDataExportPerformanceTest extends Simulation { + + private static final String BASE_URL = "http://localhost:8080"; + private static final String EXPORT_REQUEST = "GET dataValueSets - root OU with descendants"; + + @Override + public void before() { + MetadataImporter.importJsonFile("platform/superuser-data-sl-db.json", "admin", "district"); + } + + public AggregateDataExportPerformanceTest() { + DateTimeFormatter fmt = DateTimeFormatter.ISO_LOCAL_DATE; + String endDate = LocalDate.now().format(fmt); + String startDate = LocalDate.now().minusYears(2).format(fmt); + + HttpProtocolBuilder httpProtocol = + http.baseUrl(BASE_URL) + .acceptHeader("application/json") + .warmUp(BASE_URL + "/api/ping") + .disableCaching() + .basicAuth("admin", "district"); + + ScenarioBuilder exportScenario = + scenario(EXPORT_REQUEST) + .group(EXPORT_REQUEST) + .on( + repeat(1) + .on( + exec(http(EXPORT_REQUEST) + .get("/api/dataValueSets/") + .queryParam("orgUnit", "ImspTQPwCqd") + .queryParam("children", "true") + .queryParam("dataSet", "lyLU2wR22tC") + .queryParam("startDate", startDate) + .queryParam("endDate", endDate)) + .pause(1))); + + ClosedInjectionStep closedInjection = rampConcurrentUsers(0).to(5).during(10); + + setUp(exportScenario.injectClosed(closedInjection)) + .protocols(httpProtocol) + .assertions( + details(EXPORT_REQUEST).responseTime().percentile(95).lt(300), + details(EXPORT_REQUEST).responseTime().max().lt(500), + details(EXPORT_REQUEST).successfulRequests().percent().is(100D)); + } +}