From 905ccd02568589d309d8d081a3a25c8fb955d68a Mon Sep 17 00:00:00 2001 From: Jason Pickering Date: Wed, 13 May 2026 09:43:46 +0200 Subject: [PATCH 01/10] fix: support MATERIALIZED keyword in QueryBuilder CTE erasure [DHIS2-21497] Co-Authored-By: Claude Sonnet 4.6 --- .../dhis-api/src/main/java/org/hisp/dhis/sql/QueryBuilder.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 = From 7ba0e058e154069cf10dd9919ae41b53fc27e25d Mon Sep 17 00:00:00 2001 From: Jason Pickering Date: Wed, 13 May 2026 10:06:31 +0200 Subject: [PATCH 02/10] test: add failing tests for AOC MATERIALIZED CTE paths [DHIS2-21497] Co-Authored-By: Claude Sonnet 4.6 --- .../hibernate/DataExportQueryBuilderTest.java | 43 ++++++++++++++++++- 1 file changed, 41 insertions(+), 2 deletions(-) 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..13d441883d03 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,11 @@ 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_ids 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 +465,8 @@ 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 categoryoptioncombo aoc ON dv.attributeoptioncomboid = aoc.categoryoptioncomboid + JOIN aoc_ids 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 +507,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_ids AS MATERIALIZED"), "expected MATERIALIZED CTE: " + sql); + assertTrue(sql.contains("JOIN aoc_ids aoc ON"), "expected join to aoc_ids CTE: " + sql); + assertFalse( + sql.contains("JOIN categoryoptioncombo aoc ON"), + "expected no direct categoryoptioncombo join: " + 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_ids"), "expected no aoc_ids 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); + } } From 9a9d6936047271f06ddb2d3991a633475ea9badb Mon Sep 17 00:00:00 2001 From: Jason Pickering Date: Wed, 13 May 2026 10:15:18 +0200 Subject: [PATCH 03/10] perf: add aoc_ids MATERIALIZED CTE to data value export SQL [DHIS2-21497] --- .../hibernate/HibernateDataExportStore.java | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) 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..2bef6e936745 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 @@ -177,6 +177,13 @@ ou_with_descendants_ids AS ( FROM ou_ids JOIN organisationunit root USING (organisationunitid) JOIN organisationunit ou ON ou.path LIKE root.path || '%' + ), + aoc_ids 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, @@ -202,16 +209,13 @@ JOIN organisationunit root USING (organisationunitid) JOIN organisationunit ou ON dv.sourceid = ou.organisationunitid JOIN categoryoptioncombo coc ON dv.categoryoptioncomboid = coc.categoryoptioncomboid JOIN categoryoptioncombo aoc ON dv.attributeoptioncomboid = aoc.categoryoptioncomboid + JOIN aoc_ids 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()); From 8d2a35a2d36955b90f30b830340ecb0d1caf25bf Mon Sep 17 00:00:00 2001 From: Jason Pickering Date: Wed, 13 May 2026 10:17:32 +0200 Subject: [PATCH 04/10] perf: route aoc ACL check through MATERIALIZED CTE via eraseJoinLine [DHIS2-21497] Co-Authored-By: Claude Sonnet 4.6 --- .../java/org/hisp/dhis/sql/QueryBuilder.java | 19 +++++++++++++++++++ .../hibernate/HibernateDataExportStore.java | 5 +++++ 2 files changed, 24 insertions(+) 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 30b153faf5aa..118e42a74018 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 @@ -38,6 +38,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashMap; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Objects; @@ -107,6 +108,7 @@ public final class QueryBuilder { private final Set nullParams = new HashSet<>(); private final Set erasedParams = new HashSet<>(); private final Set eqParams = new HashSet<>(); + private final Set erasedFragments = new LinkedHashSet<>(); private Integer limit; private Integer offset; @@ -282,6 +284,15 @@ public QueryBuilder eraseJoinLine(String alias, boolean condition) { return this; } + /** + * Erases all lines containing {@code fragment}. To also erase an orphaned CTE, pair with {@link + * #eraseJoinLine}. + */ + public QueryBuilder eraseLineContaining(String fragment, boolean condition) { + if (condition) erasedFragments.add(fragment); + return this; + } + /** * For each of the given named parameters a SQL {@code IN(:name)} or {@code ANY(:name)} is * replaced with {@code = :name} if the current value for {@code name} is a single value. @@ -349,6 +360,7 @@ private String toSQL(boolean forCount) { String sql = eraseNullParams(this.sql); sql = eraseNullClauses(sql); sql = eraseNullJoins(sql); + sql = eraseFragmentLines(sql); sql = eraseUnusedWith(sql); sql = eraseOrders(sql, forCount); sql = eraseComments(sql); @@ -374,6 +386,13 @@ private String eraseAllOrders(String sql) { return sql.lines().filter(not(this::isOrderBy)).collect(joining("\n")); } + private String eraseFragmentLines(String sql) { + if (erasedFragments.isEmpty()) return sql; + return sql.lines() + .filter(line -> erasedFragments.stream().noneMatch(line::contains)) + .collect(joining("\n")); + } + private String eraseNullJoins(String sql) { if (erasedJoins.isEmpty() || erasedParams.isEmpty()) return sql; return sql.lines().filter(not(this::containsErasedJoin)).collect(joining("\n")); 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 2bef6e936745..8924c0e3ade6 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 @@ -259,6 +259,11 @@ WHERE NOT EXISTS (SELECT 1 FROM categoryoptioncombos_categoryoptions coc_co \ .eraseJoinLine("pe_ids", !params.hasPeriodFilters()) .eraseJoinLine("ou_with_descendants_ids", !descendants || !params.hasOrgUnitFilters()) .eraseJoinLine("ou_ids", descendants || !params.hasOrgUnitFilters()) + .eraseLineContaining("JOIN aoc_ids aoc", aocAclSql == null) + .eraseJoinLine( + "aoc_ids", + aocAclSql == null) // registers "aoc_ids" for CTE block erasure by eraseUnusedWith + .eraseLineContaining("categoryoptioncombo aoc ON", aocAclSql != null) .useEqualsOverInForParameters("de", "pe", "pt", "ou", "path", "coc", "aoc") .setLimit(params.getLimit()) .setOffset(params.getOffset()) From 596b710627ae90a81a6d1a5080d229043f8b4658 Mon Sep 17 00:00:00 2001 From: Jason Pickering Date: Wed, 13 May 2026 12:56:12 +0200 Subject: [PATCH 05/10] refactor: replace eraseLineContaining with conditional SQL construction for AOC CTE [DHIS2-21497] Instead of maintaining a dual-JOIN template and erasing the wrong line at runtime, determine the AOC path (CTE vs direct join) upfront and build the SQL string with String.replace() before handing it to QueryBuilder. Removes the eraseLineContaining / erasedFragments machinery from QueryBuilder that was added specifically to work around the alias-extraction limitation (both JOIN lines extract alias "aoc", making eraseJoinLine unable to distinguish them). The simpler approach: if useAocCte, inject the MATERIALIZED CTE block and JOIN aoc_ids; otherwise inject the direct categoryoptioncombo join. No erasure of AOC-related lines needed at all. Co-Authored-By: Claude Sonnet 4.6 --- .../java/org/hisp/dhis/sql/QueryBuilder.java | 19 ------- .../hibernate/HibernateDataExportStore.java | 50 ++++++++++--------- 2 files changed, 27 insertions(+), 42 deletions(-) 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 118e42a74018..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 @@ -38,7 +38,6 @@ import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashMap; -import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Objects; @@ -108,7 +107,6 @@ public final class QueryBuilder { private final Set nullParams = new HashSet<>(); private final Set erasedParams = new HashSet<>(); private final Set eqParams = new HashSet<>(); - private final Set erasedFragments = new LinkedHashSet<>(); private Integer limit; private Integer offset; @@ -284,15 +282,6 @@ public QueryBuilder eraseJoinLine(String alias, boolean condition) { return this; } - /** - * Erases all lines containing {@code fragment}. To also erase an orphaned CTE, pair with {@link - * #eraseJoinLine}. - */ - public QueryBuilder eraseLineContaining(String fragment, boolean condition) { - if (condition) erasedFragments.add(fragment); - return this; - } - /** * For each of the given named parameters a SQL {@code IN(:name)} or {@code ANY(:name)} is * replaced with {@code = :name} if the current value for {@code name} is a single value. @@ -360,7 +349,6 @@ private String toSQL(boolean forCount) { String sql = eraseNullParams(this.sql); sql = eraseNullClauses(sql); sql = eraseNullJoins(sql); - sql = eraseFragmentLines(sql); sql = eraseUnusedWith(sql); sql = eraseOrders(sql, forCount); sql = eraseComments(sql); @@ -386,13 +374,6 @@ private String eraseAllOrders(String sql) { return sql.lines().filter(not(this::isOrderBy)).collect(joining("\n")); } - private String eraseFragmentLines(String sql) { - if (erasedFragments.isEmpty()) return sql; - return sql.lines() - .filter(line -> erasedFragments.stream().noneMatch(line::contains)) - .collect(joining("\n")); - } - private String eraseNullJoins(String sql) { if (erasedJoins.isEmpty() || erasedParams.isEmpty()) return sql; return sql.lines().filter(not(this::containsErasedJoin)).collect(joining("\n")); 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 8924c0e3ade6..ca673c4bd36f 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 @@ -131,6 +131,14 @@ 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); + boolean useAocCte = aocAclSql != null; + String sql = """ WITH @@ -177,14 +185,7 @@ ou_with_descendants_ids AS ( FROM ou_ids JOIN organisationunit root USING (organisationunitid) JOIN organisationunit ou ON ou.path LIKE root.path || '%' - ), - aoc_ids 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)) - ) + )${aocCte} SELECT de.uid AS deid, pe.iso, @@ -208,25 +209,33 @@ WHERE NOT EXISTS (SELECT 1 FROM categoryoptioncombos_categoryoptions coc_co \ 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 categoryoptioncombo aoc ON dv.attributeoptioncomboid = aoc.categoryoptioncomboid - JOIN aoc_ids aoc ON dv.attributeoptioncomboid = aoc.categoryoptioncomboid + ${aocJoin} 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"""; + AND ou.hierarchylevel = :level""" + .replace( + "${aocCte}", + useAocCte + ? ",\naoc_ids AS MATERIALIZED (\n" + + " SELECT aoc.categoryoptioncomboid, aoc.uid\n" + + " FROM categoryoptioncombo aoc\n" + + " 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))\n" + + ")" + : "") + .replace( + "${aocJoin}", + useAocCte + ? "JOIN aoc_ids aoc ON dv.attributeoptioncomboid = aoc.categoryoptioncomboid" + : "JOIN categoryoptioncombo aoc ON dv.attributeoptioncomboid = aoc.categoryoptioncomboid"); 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); @@ -259,11 +268,6 @@ WHERE NOT EXISTS (SELECT 1 FROM categoryoptioncombos_categoryoptions coc_co \ .eraseJoinLine("pe_ids", !params.hasPeriodFilters()) .eraseJoinLine("ou_with_descendants_ids", !descendants || !params.hasOrgUnitFilters()) .eraseJoinLine("ou_ids", descendants || !params.hasOrgUnitFilters()) - .eraseLineContaining("JOIN aoc_ids aoc", aocAclSql == null) - .eraseJoinLine( - "aoc_ids", - aocAclSql == null) // registers "aoc_ids" for CTE block erasure by eraseUnusedWith - .eraseLineContaining("categoryoptioncombo aoc ON", aocAclSql != null) .useEqualsOverInForParameters("de", "pe", "pt", "ou", "path", "coc", "aoc") .setLimit(params.getLimit()) .setOffset(params.getOffset()) From 59aef0bd82bfc1e19a794fc3544d4af0f97e8cc6 Mon Sep 17 00:00:00 2001 From: Jason Pickering Date: Wed, 13 May 2026 13:30:09 +0200 Subject: [PATCH 06/10] style: replace AOC CTE string concatenation with text block constant Extracts the multi-line AOC sharing CTE string into a `private static final String AOC_CTE_BLOCK` text block, and uses `CollectionUtils.isEmpty` for the orders null-or-empty check to reduce cognitive complexity from 16 to 15 (Sonar S1764 / S5663). Co-Authored-By: Claude Sonnet 4.6 --- .../hibernate/HibernateDataExportStore.java | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) 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 ca673c4bd36f..a9fe5a29cfc6 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; @@ -68,6 +69,15 @@ @RequiredArgsConstructor public class HibernateDataExportStore implements DataExportStore { + private static final String AOC_CTE_BLOCK = + """ + , + aoc_ids 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)) + )"""; + private final EntityManager entityManager; @Override @@ -216,29 +226,20 @@ JOIN organisationunit root USING (organisationunitid) AND dv.lastupdated >= :lastUpdated AND dv.deleted = :deleted AND ou.hierarchylevel = :level""" - .replace( - "${aocCte}", - useAocCte - ? ",\naoc_ids AS MATERIALIZED (\n" - + " SELECT aoc.categoryoptioncomboid, aoc.uid\n" - + " FROM categoryoptioncombo aoc\n" - + " 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))\n" - + ")" - : "") + .replace("${aocCte}", useAocCte ? AOC_CTE_BLOCK : "") .replace( "${aocJoin}", useAocCte ? "JOIN aoc_ids aoc ON dv.attributeoptioncomboid = aoc.categoryoptioncomboid" : "JOIN categoryoptioncombo aoc ON dv.attributeoptioncomboid = aoc.categoryoptioncomboid"); + Date lastUpdated = params.getLastUpdated(); if (lastUpdated == null && params.getLastUpdatedDuration() != null) lastUpdated = new Date(currentTimeMillis() - params.getLastUpdatedDuration().toMillis()); 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(); From 201917e70a18e5c196533acdb5535ddc96c62332 Mon Sep 17 00:00:00 2001 From: Jason Pickering Date: Wed, 13 May 2026 13:43:12 +0200 Subject: [PATCH 07/10] refactor: extract getSql helper to separate SQL building from query params Moves the SQL template and AOC CTE block construction into a private static `getSql(String aocAclSql)` helper, keeping `createExportQuery` focused on parameter binding and query builder configuration. The WHERE NOT EXISTS subquery is now spread across readable lines inside the text block (test expectation updated to match). Co-Authored-By: Claude Sonnet 4.6 --- .../hibernate/HibernateDataExportStore.java | 133 +++++++++--------- .../hibernate/DataExportQueryBuilderTest.java | 4 +- 2 files changed, 72 insertions(+), 65 deletions(-) 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 a9fe5a29cfc6..3fe0470adf4e 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 @@ -69,15 +69,6 @@ @RequiredArgsConstructor public class HibernateDataExportStore implements DataExportStore { - private static final String AOC_CTE_BLOCK = - """ - , - aoc_ids 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)) - )"""; - private final EntityManager entityManager; @Override @@ -147,10 +138,72 @@ static QueryBuilder createExportQuery( if ((params.getAttributeOptionCombos() == null || params.getAttributeOptionCombos().isEmpty()) && !isSuper) aocAclSql = generateSQlQueryForSharingCheck("co.sharing", currentUser, LIKE_READ_DATA); + String sql = getSql(aocAclSql); + + Date lastUpdated = params.getLastUpdated(); + if (lastUpdated == null && params.getLastUpdatedDuration() != null) + lastUpdated = new Date(currentTimeMillis() - params.getLastUpdatedDuration().toMillis()); + + boolean descendants = params.isIncludeDescendants(); + List orders = params.getOrders(); + if (isEmpty(orders)) orders = List.of(Order.PE, Order.CREATED, Order.DE); + + List oug = params.getOrganisationUnitGroups(); + Set ouCapture = currentUser.getUserOrgUnitIds(); + if (oug == null || oug.isEmpty() || isSuper) ouCapture = Set.of(); + return SQL.of(sql, api) + .setParameter("ds", params.getDataSets()) + .setParameter("de", params.getDataElements()) + .setParameter("deg", params.getDataElementGroups()) + .setParameter("pe", params.getPeriods(), Period::getIsoDate) + .setParameter("pt", params.getPeriodTypes(), PeriodType::getName) + .setParameter("start", params.getStartDate()) + .setParameter("end", params.getEndDate()) + .setParameter("includedDate", params.getIncludedDate()) + .setParameter("ou", params.getOrganisationUnits()) + .setParameter("oug", isSuper ? List.of() : oug) + .setParameter("capture", ouCapture, identity()) + .setParameter("ougSuper", isSuper ? oug : List.of()) + .setParameter("level", params.getOrgUnitLevel()) + .setParameter("coc", params.getCategoryOptionCombos()) + .setParameter("aoc", params.getAttributeOptionCombos()) + .setParameter("lastUpdated", lastUpdated) + .setParameter("deleted", params.isIncludeDeleted() ? null : false) + .setDynamicClause("aocAccess", aocAclSql) + .eraseNullParameterLines() + // keep params below even when null + .eraseJoinLine("de_ids", !params.hasDataElementFilters()) + .eraseJoinLine("pe_ids", !params.hasPeriodFilters()) + .eraseJoinLine("ou_with_descendants_ids", !descendants || !params.hasOrgUnitFilters()) + .eraseJoinLine("ou_ids", descendants || !params.hasOrgUnitFilters()) + .useEqualsOverInForParameters("de", "pe", "pt", "ou", "path", "coc", "aoc") + .setLimit(params.getLimit()) + .setOffset(params.getOffset()) + .setOrders( + orders, + Map.ofEntries( + Map.entry(Order.OU, "ou.path"), + Map.entry(Order.PE, "pe.startdate, pe.enddate"), + Map.entry(Order.CREATED, "dv.created"), + Map.entry(Order.DE, "deid"), + Map.entry(Order.AOC, "aocid"))); + } + + private static String getSql(String aocAclSql) { boolean useAocCte = aocAclSql != null; - String sql = + String aocCTEBlock = """ + , + aoc_ids 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)) + )"""; + + return """ WITH de_ids AS ( SELECT dataelementid @@ -226,60 +279,12 @@ JOIN organisationunit root USING (organisationunitid) AND dv.lastupdated >= :lastUpdated AND dv.deleted = :deleted AND ou.hierarchylevel = :level""" - .replace("${aocCte}", useAocCte ? AOC_CTE_BLOCK : "") - .replace( - "${aocJoin}", - useAocCte - ? "JOIN aoc_ids aoc ON dv.attributeoptioncomboid = aoc.categoryoptioncomboid" - : "JOIN categoryoptioncombo aoc ON dv.attributeoptioncomboid = aoc.categoryoptioncomboid"); - - Date lastUpdated = params.getLastUpdated(); - if (lastUpdated == null && params.getLastUpdatedDuration() != null) - lastUpdated = new Date(currentTimeMillis() - params.getLastUpdatedDuration().toMillis()); - - boolean descendants = params.isIncludeDescendants(); - List orders = params.getOrders(); - if (isEmpty(orders)) orders = List.of(Order.PE, Order.CREATED, Order.DE); - - List oug = params.getOrganisationUnitGroups(); - Set ouCapture = currentUser.getUserOrgUnitIds(); - if (oug == null || oug.isEmpty() || isSuper) ouCapture = Set.of(); - return SQL.of(sql, api) - .setParameter("ds", params.getDataSets()) - .setParameter("de", params.getDataElements()) - .setParameter("deg", params.getDataElementGroups()) - .setParameter("pe", params.getPeriods(), Period::getIsoDate) - .setParameter("pt", params.getPeriodTypes(), PeriodType::getName) - .setParameter("start", params.getStartDate()) - .setParameter("end", params.getEndDate()) - .setParameter("includedDate", params.getIncludedDate()) - .setParameter("ou", params.getOrganisationUnits()) - .setParameter("oug", isSuper ? List.of() : oug) - .setParameter("capture", ouCapture, identity()) - .setParameter("ougSuper", isSuper ? oug : List.of()) - .setParameter("level", params.getOrgUnitLevel()) - .setParameter("coc", params.getCategoryOptionCombos()) - .setParameter("aoc", params.getAttributeOptionCombos()) - .setParameter("lastUpdated", lastUpdated) - .setParameter("deleted", params.isIncludeDeleted() ? null : false) - .setDynamicClause("aocAccess", aocAclSql) - .eraseNullParameterLines() - // keep params below even when null - .eraseJoinLine("de_ids", !params.hasDataElementFilters()) - .eraseJoinLine("pe_ids", !params.hasPeriodFilters()) - .eraseJoinLine("ou_with_descendants_ids", !descendants || !params.hasOrgUnitFilters()) - .eraseJoinLine("ou_ids", descendants || !params.hasOrgUnitFilters()) - .useEqualsOverInForParameters("de", "pe", "pt", "ou", "path", "coc", "aoc") - .setLimit(params.getLimit()) - .setOffset(params.getOffset()) - .setOrders( - orders, - Map.ofEntries( - Map.entry(Order.OU, "ou.path"), - Map.entry(Order.PE, "pe.startdate, pe.enddate"), - Map.entry(Order.CREATED, "dv.created"), - Map.entry(Order.DE, "deid"), - Map.entry(Order.AOC, "aocid"))); + .replace("${aocCte}", useAocCte ? aocCTEBlock : "") + .replace( + "${aocJoin}", + useAocCte + ? "JOIN aoc_ids aoc ON dv.attributeoptioncomboid = aoc.categoryoptioncomboid" + : "JOIN categoryoptioncombo aoc ON dv.attributeoptioncomboid = aoc.categoryoptioncomboid"); } @CheckForNull 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 13d441883d03..4b7da0119e28 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 @@ -443,7 +443,9 @@ ou_ids AS ( aoc_ids 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 ) )) + 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, From 489edd6d86c1b7b58b163d1a79f2efbb03f7f1d3 Mon Sep 17 00:00:00 2001 From: Jason Pickering Date: Wed, 13 May 2026 14:09:30 +0200 Subject: [PATCH 08/10] refactor: inline SQL template back into createExportQuery [DHIS2-21497] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per reviewer feedback: the QueryBuilder DSL is designed for one SQL template and one parameter/condition block — splitting the SQL into a helper method works against this design. Inline everything back into createExportQuery. Co-Authored-By: Claude Sonnet 4.6 --- .../hibernate/HibernateDataExportStore.java | 117 +++++++++--------- 1 file changed, 56 insertions(+), 61 deletions(-) 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 3fe0470adf4e..91cf7560c0e2 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 @@ -138,60 +138,7 @@ static QueryBuilder createExportQuery( if ((params.getAttributeOptionCombos() == null || params.getAttributeOptionCombos().isEmpty()) && !isSuper) aocAclSql = generateSQlQueryForSharingCheck("co.sharing", currentUser, LIKE_READ_DATA); - String sql = getSql(aocAclSql); - - Date lastUpdated = params.getLastUpdated(); - if (lastUpdated == null && params.getLastUpdatedDuration() != null) - lastUpdated = new Date(currentTimeMillis() - params.getLastUpdatedDuration().toMillis()); - - boolean descendants = params.isIncludeDescendants(); - List orders = params.getOrders(); - if (isEmpty(orders)) orders = List.of(Order.PE, Order.CREATED, Order.DE); - - List oug = params.getOrganisationUnitGroups(); - Set ouCapture = currentUser.getUserOrgUnitIds(); - if (oug == null || oug.isEmpty() || isSuper) ouCapture = Set.of(); - return SQL.of(sql, api) - .setParameter("ds", params.getDataSets()) - .setParameter("de", params.getDataElements()) - .setParameter("deg", params.getDataElementGroups()) - .setParameter("pe", params.getPeriods(), Period::getIsoDate) - .setParameter("pt", params.getPeriodTypes(), PeriodType::getName) - .setParameter("start", params.getStartDate()) - .setParameter("end", params.getEndDate()) - .setParameter("includedDate", params.getIncludedDate()) - .setParameter("ou", params.getOrganisationUnits()) - .setParameter("oug", isSuper ? List.of() : oug) - .setParameter("capture", ouCapture, identity()) - .setParameter("ougSuper", isSuper ? oug : List.of()) - .setParameter("level", params.getOrgUnitLevel()) - .setParameter("coc", params.getCategoryOptionCombos()) - .setParameter("aoc", params.getAttributeOptionCombos()) - .setParameter("lastUpdated", lastUpdated) - .setParameter("deleted", params.isIncludeDeleted() ? null : false) - .setDynamicClause("aocAccess", aocAclSql) - .eraseNullParameterLines() - // keep params below even when null - .eraseJoinLine("de_ids", !params.hasDataElementFilters()) - .eraseJoinLine("pe_ids", !params.hasPeriodFilters()) - .eraseJoinLine("ou_with_descendants_ids", !descendants || !params.hasOrgUnitFilters()) - .eraseJoinLine("ou_ids", descendants || !params.hasOrgUnitFilters()) - .useEqualsOverInForParameters("de", "pe", "pt", "ou", "path", "coc", "aoc") - .setLimit(params.getLimit()) - .setOffset(params.getOffset()) - .setOrders( - orders, - Map.ofEntries( - Map.entry(Order.OU, "ou.path"), - Map.entry(Order.PE, "pe.startdate, pe.enddate"), - Map.entry(Order.CREATED, "dv.created"), - Map.entry(Order.DE, "deid"), - Map.entry(Order.AOC, "aocid"))); - } - - private static String getSql(String aocAclSql) { boolean useAocCte = aocAclSql != null; - String aocCTEBlock = """ , @@ -202,8 +149,8 @@ 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)) )"""; - - return """ + String sql = + """ WITH de_ids AS ( SELECT dataelementid @@ -279,12 +226,60 @@ JOIN organisationunit root USING (organisationunitid) AND dv.lastupdated >= :lastUpdated AND dv.deleted = :deleted AND ou.hierarchylevel = :level""" - .replace("${aocCte}", useAocCte ? aocCTEBlock : "") - .replace( - "${aocJoin}", - useAocCte - ? "JOIN aoc_ids aoc ON dv.attributeoptioncomboid = aoc.categoryoptioncomboid" - : "JOIN categoryoptioncombo aoc ON dv.attributeoptioncomboid = aoc.categoryoptioncomboid"); + .replace("${aocCte}", useAocCte ? aocCTEBlock : "") + .replace( + "${aocJoin}", + useAocCte + ? "JOIN aoc_ids aoc ON dv.attributeoptioncomboid = aoc.categoryoptioncomboid" + : "JOIN categoryoptioncombo aoc ON dv.attributeoptioncomboid = aoc.categoryoptioncomboid"); + + Date lastUpdated = params.getLastUpdated(); + if (lastUpdated == null && params.getLastUpdatedDuration() != null) + lastUpdated = new Date(currentTimeMillis() - params.getLastUpdatedDuration().toMillis()); + + boolean descendants = params.isIncludeDescendants(); + List orders = params.getOrders(); + if (isEmpty(orders)) orders = List.of(Order.PE, Order.CREATED, Order.DE); + + List oug = params.getOrganisationUnitGroups(); + Set ouCapture = currentUser.getUserOrgUnitIds(); + if (oug == null || oug.isEmpty() || isSuper) ouCapture = Set.of(); + return SQL.of(sql, api) + .setParameter("ds", params.getDataSets()) + .setParameter("de", params.getDataElements()) + .setParameter("deg", params.getDataElementGroups()) + .setParameter("pe", params.getPeriods(), Period::getIsoDate) + .setParameter("pt", params.getPeriodTypes(), PeriodType::getName) + .setParameter("start", params.getStartDate()) + .setParameter("end", params.getEndDate()) + .setParameter("includedDate", params.getIncludedDate()) + .setParameter("ou", params.getOrganisationUnits()) + .setParameter("oug", isSuper ? List.of() : oug) + .setParameter("capture", ouCapture, identity()) + .setParameter("ougSuper", isSuper ? oug : List.of()) + .setParameter("level", params.getOrgUnitLevel()) + .setParameter("coc", params.getCategoryOptionCombos()) + .setParameter("aoc", params.getAttributeOptionCombos()) + .setParameter("lastUpdated", lastUpdated) + .setParameter("deleted", params.isIncludeDeleted() ? null : false) + .setDynamicClause("aocAccess", aocAclSql) + .eraseNullParameterLines() + // keep params below even when null + .eraseJoinLine("de_ids", !params.hasDataElementFilters()) + .eraseJoinLine("pe_ids", !params.hasPeriodFilters()) + .eraseJoinLine("ou_with_descendants_ids", !descendants || !params.hasOrgUnitFilters()) + .eraseJoinLine("ou_ids", descendants || !params.hasOrgUnitFilters()) + .useEqualsOverInForParameters("de", "pe", "pt", "ou", "path", "coc", "aoc") + .setLimit(params.getLimit()) + .setOffset(params.getOffset()) + .setOrders( + orders, + Map.ofEntries( + Map.entry(Order.OU, "ou.path"), + Map.entry(Order.PE, "pe.startdate, pe.enddate"), + Map.entry(Order.CREATED, "dv.created"), + Map.entry(Order.DE, "deid"), + Map.entry(Order.AOC, "aocid"))); } @CheckForNull From e83f2b356cea5a281eb87d354b4af01aa0d22bc0 Mon Sep 17 00:00:00 2001 From: Jason Pickering Date: Wed, 13 May 2026 16:44:10 +0200 Subject: [PATCH 09/10] Fixes --- .../hibernate/HibernateDataExportStore.java | 32 +++++++------------ .../hibernate/DataExportQueryBuilderTest.java | 16 ++++++---- 2 files changed, 21 insertions(+), 27 deletions(-) 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 91cf7560c0e2..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 @@ -138,17 +138,6 @@ static QueryBuilder createExportQuery( if ((params.getAttributeOptionCombos() == null || params.getAttributeOptionCombos().isEmpty()) && !isSuper) aocAclSql = generateSQlQueryForSharingCheck("co.sharing", currentUser, LIKE_READ_DATA); - boolean useAocCte = aocAclSql != null; - String aocCTEBlock = - """ - , - aoc_ids 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)) - )"""; String sql = """ WITH @@ -195,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 || '%' - )${aocCte} + ), + 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, @@ -219,19 +215,14 @@ 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 - ${aocJoin} + 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""" - .replace("${aocCte}", useAocCte ? aocCTEBlock : "") - .replace( - "${aocJoin}", - useAocCte - ? "JOIN aoc_ids aoc ON dv.attributeoptioncomboid = aoc.categoryoptioncomboid" - : "JOIN categoryoptioncombo aoc ON dv.attributeoptioncomboid = aoc.categoryoptioncomboid"); + AND ou.hierarchylevel = :level"""; Date lastUpdated = params.getLastUpdated(); if (lastUpdated == null && params.getLastUpdatedDuration() != null) @@ -265,6 +256,7 @@ JOIN organisationunit root USING (organisationunitid) .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 4b7da0119e28..25602a6d5128 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 @@ -440,7 +440,7 @@ ou_ids AS ( ) ou_all WHERE organisationunitid IS NOT NULL ), - aoc_ids AS MATERIALIZED ( + aoc_access AS MATERIALIZED ( SELECT aoc.categoryoptioncomboid, aoc.uid FROM categoryoptioncombo aoc WHERE NOT EXISTS (SELECT 1 FROM categoryoptioncombos_categoryoptions coc_co @@ -467,7 +467,8 @@ WHERE NOT EXISTS (SELECT 1 FROM categoryoptioncombos_categoryoptions coc_co 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_ids aoc ON dv.attributeoptioncomboid = aoc.categoryoptioncomboid + JOIN aoc_access ON dv.attributeoptioncomboid = aoc_access.categoryoptioncomboid + JOIN categoryoptioncombo aoc ON dv.attributeoptioncomboid = aoc.categoryoptioncomboid WHERE dv.deleted = :deleted ORDER BY pe.startdate, pe.enddate, dv.created, deid""", Set.of("oug", "capture", "deleted"), @@ -518,11 +519,12 @@ void testAocAccess_nonSuperuser_usesMaterializedCte() { SQL.QueryAPI spy = SQL.spy(captured::set, (name, param) -> {}); createExportQuery(params, spy, user).stream(); String sql = captured.get(); - assertTrue(sql.contains("aoc_ids AS MATERIALIZED"), "expected MATERIALIZED CTE: " + sql); - assertTrue(sql.contains("JOIN aoc_ids aoc ON"), "expected join to aoc_ids CTE: " + sql); - assertFalse( + 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 no direct categoryoptioncombo join: " + sql); + "expected direct categoryoptioncombo join for aoc alias: " + sql); assertFalse( sql.contains("AND NOT EXISTS"), "NOT EXISTS should not appear inline in WHERE clause: " + sql); @@ -535,7 +537,7 @@ void testAocAccess_superuser_usesDirectJoin() { SQL.QueryAPI spy = SQL.spy(captured::set, (name, param) -> {}); createExportQuery(params, spy, new SystemUser()).stream(); String sql = captured.get(); - assertFalse(sql.contains("aoc_ids"), "expected no aoc_ids CTE for superuser: " + sql); + 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); From 1d99210aada8b2eb146e64e09456f18963f1f21b Mon Sep 17 00:00:00 2001 From: Jason Pickering Date: Wed, 13 May 2026 16:51:04 +0200 Subject: [PATCH 10/10] Linting --- .../dhis/datavalue/hibernate/DataExportQueryBuilderTest.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 25602a6d5128..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 @@ -520,8 +520,7 @@ void testAocAccess_nonSuperuser_usesMaterializedCte() { 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 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);