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); + } }