feat: Add enrollment AOC to exporter [DHIS-20659]#22959
Open
feat: Add enrollment AOC to exporter [DHIS-20659]#22959
Conversation
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Issues
1.
getCurrentUserDetails()called twice inaddCategoryOptionComboJoin()JdbcEnrollmentStore.java:193,197--getCurrentUserDetails()is called twice: once for theisNotSuperUsercheck and again inside thehavingclause. The event store (JdbcTrackerEventStore:1087) receivesUserDetails useras a parameter instead. Should follow the same pattern -- accept user as a parameter or store it in a local variable.2. OpenAPI annotation references
CategoryComboinstead ofCategoryOptionComboEnrollmentRequestParams.java:135--@OpenApi.Property({UID.class, CategoryCombo.class})should beCategoryOptionCombo.class. The parameter isattributeOptionCombo, not a category combo.3. Error message inconsistency when moving
validateAttributeOptionComboThe event version said
"User has no access to attribute category option combo: "but the new shared version inOperationsParamsValidator:340says"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)resultCategoryOptionComboService:101--getAttributeOptionCombo(UID uid)can returnnullif the UID doesn't match any COC. The caller inEnrollmentOperationParamsMapper:78passes this potentially null value tovalidateAttributeOptionCombo, which only checks access but doesn't validate existence. Compare with how events resolve it viagetAttributeOptionCombo(String cc, Set<String> options, boolean skipFallback)which throwsIllegalQueryExceptionwhen not found.Minor
getEnrollment->addRequestedFieldsrefactoring removes the defensive copy pattern (previously created a newEnrollmentand 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.SingleEventExporterTest->SingleEventServiceTestandTrackerEventExporterTest->TrackerEventServiceTestare 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 joinwith the COC subquery. Small absolute difference for a smoke test, butthe relative increase is notable.
COC subquery analysis
The COC join is added unconditionally to every enrollment query via
addCategoryOptionComboJoin():This is the same pattern used in
JdbcTrackerEventStorefor events. The subquery itself is fast(0.5ms, 225 COCs) and joined against a single
attributeoptioncomboidper enrollment. In SierraLeone, all ~11M enrollments use the default COC (
HllvX50cXC0).Impact on existing queries: The join is unconditional -- it runs even when
attributeOptionCombois 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):
sort
Suggestion: make the COC join conditional. Only add it when
attributeOptionCombois requestedas a filter parameter or when the
attributeOptionCombofield is included in the response. Whenneither is requested, skip the join entirely and don't select
coc.uid. This matches how theattributes 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 noteslateral 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 whenattributeOptionCombois providedas 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):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.