Skip to content

feat: Add enrollment AOC to exporter [DHIS-20659]#22959

Open
muilpp wants to merge 2 commits intomasterfrom
DHIS2-20659
Open

feat: Add enrollment AOC to exporter [DHIS-20659]#22959
muilpp wants to merge 2 commits intomasterfrom
DHIS2-20659

Conversation

@muilpp
Copy link
Contributor

@muilpp muilpp commented Feb 13, 2026

Issues

1. getCurrentUserDetails() called twice in addCategoryOptionComboJoin()

JdbcEnrollmentStore.java:193,197 -- getCurrentUserDetails() is called twice: once for the isNotSuperUser check and again inside the having clause. The event store (JdbcTrackerEventStore:1087) receives UserDetails user as a parameter instead. Should follow the same pattern -- accept user as a parameter or store it in a local variable.

2. OpenAPI annotation references CategoryCombo instead of CategoryOptionCombo

EnrollmentRequestParams.java:135 -- @OpenApi.Property({UID.class, CategoryCombo.class}) should be CategoryOptionCombo.class. The parameter is attributeOptionCombo, not a category combo.

3. Error message inconsistency when moving validateAttributeOptionCombo

The event version said "User has no access to attribute category option combo: " but the new shared version in OperationsParamsValidator:340 says "User has no access to attribute option combo: ". The word "category" was dropped. This is a behavioral change for the event exporter -- any client parsing or matching on this error message will break.

4. No null check on getAttributeOptionCombo(UID) result

CategoryOptionComboService:101 -- getAttributeOptionCombo(UID uid) can return null if the UID doesn't match any COC. The caller in EnrollmentOperationParamsMapper:78 passes this potentially null value to validateAttributeOptionCombo, which only checks access but doesn't validate existence. Compare with how events resolve it via getAttributeOptionCombo(String cc, Set<String> options, boolean skipFallback) which throws IllegalQueryException when not found.

Minor

  • The getEnrollment -> addRequestedFields refactoring removes the defensive copy pattern (previously created a new Enrollment and copied fields). The new version mutates the input directly. Fine if the enrollment from the store is never reused, but it's a semantic change.
  • Test renames SingleEventExporterTest -> SingleEventServiceTest and TrackerEventExporterTest -> TrackerEventServiceTest are unrelated cleanup that could be a separate commit.
  • shouldNotFailWhenUserHasAccessToMapAttributeOptionCombo -- test name has a stray "Map".

Performance

Smoke test shows "Get all enrollments in program" going from 17ms to 25ms (47% increase). Expected
given the new inner join with the COC subquery. Small absolute difference for a smoke test, but
the relative increase is notable.

COC subquery analysis

The COC join is added unconditionally to every enrollment query via addCategoryOptionComboJoin():

inner join (
  select coc.categoryoptioncomboid as id, coc.uid
  from categoryoptioncombo coc
  inner join categoryoptioncombos_categoryoptions cocco
    on coc.categoryoptioncomboid = cocco.categoryoptioncomboid
  inner join categoryoption co
    on cocco.categoryoptionid = co.categoryoptionid
  group by coc.categoryoptioncomboid
  having bool_and(case when <sharing_check> then true else false end) = true
) as coc on coc.id = e.attributeoptioncomboid

This is the same pattern used in JdbcTrackerEventStore for events. The subquery itself is fast
(0.5ms, 225 COCs) and joined against a single attributeoptioncomboid per enrollment. In Sierra
Leone, all ~11M enrollments use the default COC (HllvX50cXC0).

Impact on existing queries: The join is unconditional -- it runs even when attributeOptionCombo
is not requested as a filter or field. This means every enrollment query pays the cost of the COC
subquery + group by + having (for non-super users). With 225 COCs the absolute cost is small (~0.5ms
for the subquery itself), but the join can affect the planner's choice of join strategy on the main
query.

Measured baseline vs this PR (Sierra Leone, ~11M enrollments, pgbench 1 client 30s):

  • Base query (admin): 8ms -> ~12ms (estimated from smoke test ratio)
  • Order by enrolledAt: already 25-39s baseline, COC join adds negligible overhead relative to the
    sort

Suggestion: make the COC join conditional. Only add it when attributeOptionCombo is requested
as a filter parameter or when the attributeOptionCombo field is included in the response. When
neither is requested, skip the join entirely and don't select coc.uid. This matches how the
attributes lateral join is conditional on isIncludeAttributes().

Count query impact

The COC join is also included in the count query path (getCountQuery -> addEnrollmentFromItem ->
addInnerJoins). The count query already has its own issues (49s baseline due to unnecessary notes
lateral join, trackedentity/trackedentitytype/en_ou joins). Adding the COC subquery to the count
path adds more unnecessary work.

The count query only needs: enrollment e, trackedentityprogramowner po,
organisationunit ou, and optionally the COC filter (only when attributeOptionCombo is provided
as a filter). The COC join for access control is not needed in the count -- access was already
validated in the mapper via validateAttributeOptionCombo().

Interaction with existing performance issues

The enrollment query already has several performance bottlenecks (see sql/ideas.md):

Issue Baseline Notes
Order by enrolledAt/createdAt/completedAt 25-39s Full table sort, no composite index
Count query (totalPages=true) 49s Unnecessary joins (notes, TE, TET, en_ou)
orgUnitMode=CHILDREN 42s Path + hierarchylevel scan

The COC join does not make these worse in absolute terms (they're already dominated by table scans),
but it does add complexity to an already heavy query. If the enrollment store is refactored to
address the above issues (program join elimination + composite indexes), the COC join overhead would
become relatively more significant.

@sonarqubecloud
Copy link

@muilpp muilpp marked this pull request as ready for review February 13, 2026 12:10
@muilpp muilpp requested review from ameenhere and zubaira February 13, 2026 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants