Skip to content

Commit e5f49e8

Browse files
Fix/optionset preheat n1 master (#24850)
* feat: add valid-option-code storage to TrackerPreheat Adds per-option-set valid-code and resolved-set tracking so validators and the ASSIGN-action pipeline can check option codes without materializing the full OptionSet.options collection. * feat: add OptionValueSupplier, preheat only referenced option codes Queries optionvalue directly for the (option set, code) pairs actually referenced by the import payload, in chunks, instead of relying on OptionSetMapper to hydrate entire option collections. Cost is bounded by distinct codes referenced, not option set size. Includes the NOSONAR suppression for the two-phase collect/query loop (false positive: confirmed is populated by the query immediately before it's read). * feat: validate option codes against preheated OptionValueSupplier data Switches validateOptionSet() to check TrackerPreheat's valid-code cache instead of scanning OptionSet.getOptions(), with a diagnostic log.warn when an option set never got resolved during preheat. Updates all three call sites (tracked entity, enrollment, and event attribute/data value validators) and their tests accordingly. * perf: stop preheat from fully materializing OptionSet.options OptionSetMapper no longer maps the options collection, which was forcing a full lazy-collection load (and JSONB deserialization of every option) for every OptionSet touched during tracker preheat. Callers now go through OptionValueSupplier/TrackerPreheat instead. * fix: resolve option codes introduced by program rule ASSIGN actions ASSIGN actions can introduce data element/attribute values that were never part of the original import payload, so OptionValueSupplier's single preheat pass can't have seen their option codes. Runs the assigned values back through OptionValueSupplier as a synthetic TrackerObjects before validation, so option-set validation still sees them as preheated. Adds getter access to the executors' resolved data element/attribute UID and value, and asserts no errors in the previously-unchecked option-value ASSIGN warning test. * test: remove redundant public visibility modifier in AttributeValidatorTest SonarQube java:S5786 -- JUnit5 lifecycle methods don't need public visibility. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test: prove OptionValueSupplier can't validate a fabricated cross-option-set pair The two-column IN/IN query in queryChunk() can return rows whose optionsetid and code each satisfy one of the two independent IN-lists without the pair itself ever having been requested. Correctness depends entirely on the in-memory confirmed.contains(pair) check filtering those out before anything is marked valid. Add a regression test that mocks a query result covering that exact cross-contamination shape and confirms the fabricated pairs stay unvalidated while a genuine pair still passes; verified by temporarily weakening the check and watching this test fail. * fix: use unnest parallel-array join in OptionValueSupplier Switches the (optionsetid, code) lookup from IN (...) AND IN (...) to a join against unnest(?::bigint[], ?::text[]), per @teleivo/@ameenhere's review on #24850. The IN/IN form only sargs on the leading index column once cardinality grows, downgrading `code` to a post-scan Filter; the unnest join keeps both columns in the Index Cond via optionvalue_unique_optionsetid_and_code regardless of scale. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
1 parent af68eda commit e5f49e8

17 files changed

Lines changed: 842 additions & 74 deletions

File tree

dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/tracker/imports/programrule/ProgramRuleAssignActionTest.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2004-2022, University of Oslo
2+
* Copyright (c) 2004-2026, University of Oslo
33
* All rights reserved.
44
*
55
* Redistribution and use in source and binary forms, with or without
@@ -308,6 +308,8 @@ void shouldImportWithWarningWhenDataElementOfTypeOptionWithValidValueIsAssignedB
308308
assertHasOnlyWarnings(importReport, E1308);
309309
if (!hasValidValue) {
310310
assertHasOnlyErrors(importReport, E1125);
311+
} else {
312+
assertNoErrors(importReport);
311313
}
312314
}
313315

dhis-2/dhis-tracker/src/main/java/org/hisp/dhis/tracker/imports/bundle/DefaultTrackerBundleService.java

Lines changed: 102 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2004-2022, University of Oslo
2+
* Copyright (c) 2004-2026, University of Oslo
33
* All rights reserved.
44
*
55
* Redistribution and use in source and binary forms, with or without
@@ -34,25 +34,37 @@
3434
import com.google.common.collect.Lists;
3535
import java.util.Date;
3636
import java.util.List;
37+
import java.util.Objects;
38+
import java.util.Set;
39+
import java.util.stream.Collectors;
3740
import java.util.stream.Stream;
3841
import javax.annotation.Nonnull;
3942
import lombok.RequiredArgsConstructor;
4043
import org.hisp.dhis.common.UID;
44+
import org.hisp.dhis.dataelement.DataElement;
4145
import org.hisp.dhis.feedback.ForbiddenException;
4246
import org.hisp.dhis.feedback.NotFoundException;
4347
import org.hisp.dhis.program.UserInfoSnapshot;
48+
import org.hisp.dhis.trackedentity.TrackedEntityAttribute;
4449
import org.hisp.dhis.tracker.imports.ParamsConverter;
4550
import org.hisp.dhis.tracker.imports.TrackerImportParams;
4651
import org.hisp.dhis.tracker.imports.bundle.persister.CommitService;
4752
import org.hisp.dhis.tracker.imports.bundle.persister.PersistenceException;
4853
import org.hisp.dhis.tracker.imports.bundle.persister.TrackerObjectDeletionService;
4954
import org.hisp.dhis.tracker.imports.bundle.persister.TrackerPersister.PersistResult;
55+
import org.hisp.dhis.tracker.imports.domain.Attribute;
56+
import org.hisp.dhis.tracker.imports.domain.DataValue;
57+
import org.hisp.dhis.tracker.imports.domain.Enrollment;
5058
import org.hisp.dhis.tracker.imports.domain.TrackerDto;
59+
import org.hisp.dhis.tracker.imports.domain.TrackerEvent;
5160
import org.hisp.dhis.tracker.imports.domain.TrackerObjects;
5261
import org.hisp.dhis.tracker.imports.notification.EntityNotifications;
5362
import org.hisp.dhis.tracker.imports.preheat.TrackerPreheat;
5463
import org.hisp.dhis.tracker.imports.preheat.TrackerPreheatService;
64+
import org.hisp.dhis.tracker.imports.preheat.supplier.OptionValueSupplier;
5565
import org.hisp.dhis.tracker.imports.programrule.ProgramRuleService;
66+
import org.hisp.dhis.tracker.imports.programrule.executor.enrollment.AssignAttributeExecutor;
67+
import org.hisp.dhis.tracker.imports.programrule.executor.event.AssignDataValueExecutor;
5668
import org.hisp.dhis.tracker.imports.report.PersistenceReport;
5769
import org.hisp.dhis.tracker.imports.report.TrackerTypeReport;
5870
import org.hisp.dhis.user.UserDetails;
@@ -77,6 +89,8 @@ public class DefaultTrackerBundleService implements TrackerBundleService {
7789

7890
private final TrackerObjectDeletionService deletionService;
7991

92+
private final OptionValueSupplier optionValueSupplier;
93+
8094
private final ObjectMapper mapper;
8195

8296
@Nonnull
@@ -97,9 +111,96 @@ public TrackerBundle create(
97111
public TrackerBundle runRuleEngine(@Nonnull TrackerBundle trackerBundle) {
98112
programRuleService.calculateRuleEffects(trackerBundle, trackerBundle.getPreheat());
99113

114+
optionValueSupplier.preheatAdd(
115+
collectRuleAssignedValues(trackerBundle), trackerBundle.getPreheat());
116+
100117
return trackerBundle;
101118
}
102119

120+
/**
121+
* Collects the values {@code ASSIGN} rule actions are going to apply, shaped as a synthetic
122+
* {@link TrackerObjects} payload the {@link OptionValueSupplier} can resolve option codes from.
123+
*
124+
* <p>Rule engine validation rejects unknown option codes based on {@link
125+
* TrackerPreheat#isValidOptionCode(Long, String)}, but {@code ASSIGN} actions can add or
126+
* overwrite data values and attributes that were not in the original payload, so the codes they
127+
* introduce were never resolved during preheat and valid data would be rejected with E1125.
128+
*
129+
* <p>The values are already final here: they are the rule engine's evaluated output, captured in
130+
* the executors {@code calculateRuleEffects} just built, so nothing evaluated later can change
131+
* them.
132+
*
133+
* <p>All assigned values are gathered onto a single synthetic event and enrollment. The supplier
134+
* only looks at (data element, value) and (attribute, value) pairs, so which or how many real
135+
* entities the values belong to does not matter.
136+
*/
137+
private TrackerObjects collectRuleAssignedValues(TrackerBundle bundle) {
138+
TrackerPreheat preheat = bundle.getPreheat();
139+
140+
Set<DataValue> assignedDataValues =
141+
bundle.getEventRuleActionExecutors().values().stream()
142+
.flatMap(List::stream)
143+
.filter(AssignDataValueExecutor.class::isInstance)
144+
.map(AssignDataValueExecutor.class::cast)
145+
.map(executor -> toDataValue(preheat, executor))
146+
.filter(Objects::nonNull)
147+
.collect(Collectors.toSet());
148+
149+
List<Attribute> assignedAttributes =
150+
bundle.getEnrollmentRuleActionExecutors().values().stream()
151+
.flatMap(List::stream)
152+
.filter(AssignAttributeExecutor.class::isInstance)
153+
.map(AssignAttributeExecutor.class::cast)
154+
.map(executor -> toAttribute(preheat, executor))
155+
.filter(Objects::nonNull)
156+
.toList();
157+
158+
return TrackerObjects.builder()
159+
.events(
160+
List.of(
161+
TrackerEvent.builder()
162+
.event(UID.generate())
163+
.dataValues(assignedDataValues)
164+
.build()))
165+
.enrollments(
166+
List.of(
167+
Enrollment.builder()
168+
.enrollment(UID.generate())
169+
.attributes(assignedAttributes)
170+
.build()))
171+
.build();
172+
}
173+
174+
/**
175+
* Executors only know the target's UID, while the supplier looks metadata up by the payload's id
176+
* scheme, so the identifier has to be converted the same way the executors themselves do when
177+
* they apply the value.
178+
*/
179+
private DataValue toDataValue(TrackerPreheat preheat, AssignDataValueExecutor executor) {
180+
DataElement dataElement = preheat.getDataElement(executor.getDataElementUid().getValue());
181+
if (dataElement == null) {
182+
return null;
183+
}
184+
185+
return DataValue.builder()
186+
.dataElement(preheat.getIdSchemes().toMetadataIdentifier(dataElement))
187+
.value(executor.getValue())
188+
.build();
189+
}
190+
191+
private Attribute toAttribute(TrackerPreheat preheat, AssignAttributeExecutor executor) {
192+
TrackedEntityAttribute attribute =
193+
preheat.getTrackedEntityAttribute(executor.getAttributeUid().getValue());
194+
if (attribute == null) {
195+
return null;
196+
}
197+
198+
return Attribute.builder()
199+
.attribute(preheat.getIdSchemes().toMetadataIdentifier(attribute))
200+
.value(executor.getValue())
201+
.build();
202+
}
203+
103204
@Nonnull
104205
@Override
105206
@Transactional

dhis-2/dhis-tracker/src/main/java/org/hisp/dhis/tracker/imports/config/TrackerPreheatConfig.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2004-2022, University of Oslo
2+
* Copyright (c) 2004-2026, University of Oslo
33
* All rights reserved.
44
*
55
* Redistribution and use in source and binary forms, with or without
@@ -38,6 +38,7 @@
3838
import org.hisp.dhis.tracker.imports.preheat.supplier.EnrollmentsWithAtLeastOneEventSupplier;
3939
import org.hisp.dhis.tracker.imports.preheat.supplier.EventCategoryOptionComboSupplier;
4040
import org.hisp.dhis.tracker.imports.preheat.supplier.FileResourceSupplier;
41+
import org.hisp.dhis.tracker.imports.preheat.supplier.OptionValueSupplier;
4142
import org.hisp.dhis.tracker.imports.preheat.supplier.OrgUnitValueTypeSupplier;
4243
import org.hisp.dhis.tracker.imports.preheat.supplier.PreheatStrategyScanner;
4344
import org.hisp.dhis.tracker.imports.preheat.supplier.PreheatSupplier;
@@ -56,6 +57,7 @@ public class TrackerPreheatConfig {
5657
private final List<Class<? extends PreheatSupplier>> preheatOrder =
5758
List.of(
5859
ClassBasedSupplier.class,
60+
OptionValueSupplier.class,
5961
DefaultsSupplier.class,
6062
TrackedEntityEnrollmentSupplier.class,
6163
EnrollmentsWithAtLeastOneEventSupplier.class,

dhis-2/dhis-tracker/src/main/java/org/hisp/dhis/tracker/imports/preheat/TrackerPreheat.java

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2004-2022, University of Oslo
2+
* Copyright (c) 2004-2026, University of Oslo
33
* All rights reserved.
44
*
55
* Redistribution and use in source and binary forms, with or without
@@ -138,6 +138,42 @@ private Pair<String, Set<MetadataIdentifier>> toCategoryOptionComboCacheKey(
138138
return Pair.of(categoryCombo.getUid(), categoryOptions);
139139
}
140140

141+
/**
142+
* Set of (option set id, option code) pairs confirmed to exist, populated by {@code
143+
* OptionValueSupplier}. Only pairs actually referenced by the import payload are present here —
144+
* this is not the full set of options for any given option set.
145+
*
146+
* <p>Preheated {@link org.hisp.dhis.option.OptionSet} instances deliberately carry no populated
147+
* {@code options} collection (see {@code OptionSetMapper} for why). {@link
148+
* #isValidOptionCode(Long, String)} and {@link #addValidOptionCode(Long, String)} are the
149+
* supported way to check option code validity against preheated data. Do not call {@code
150+
* OptionSet#getOptions()} on a preheated option set expecting real data — it is always empty.
151+
*/
152+
private final Set<Pair<Long, String>> validOptionCodes = new HashSet<>();
153+
154+
/**
155+
* Option sets {@code OptionValueSupplier} attempted to resolve. Lets callers tell "this code was
156+
* checked against the database and does not exist" apart from "this option set was never resolved
157+
* at all", which would be an internal bug rather than user error.
158+
*/
159+
private final Set<Long> resolvedOptionSets = new HashSet<>();
160+
161+
public void addValidOptionCode(Long optionSetId, String code) {
162+
this.validOptionCodes.add(Pair.of(optionSetId, code));
163+
}
164+
165+
public boolean isValidOptionCode(Long optionSetId, String code) {
166+
return this.validOptionCodes.contains(Pair.of(optionSetId, code));
167+
}
168+
169+
public void addResolvedOptionSet(Long optionSetId) {
170+
this.resolvedOptionSets.add(optionSetId);
171+
}
172+
173+
public boolean isOptionSetResolved(Long optionSetId) {
174+
return this.resolvedOptionSets.contains(optionSetId);
175+
}
176+
141177
/**
142178
* Check if a category option combo for given category combo and category options has been stored
143179
* using {@link #putCategoryOptionCombo}. Returns true if null and a non-null category option

dhis-2/dhis-tracker/src/main/java/org/hisp/dhis/tracker/imports/preheat/mappers/OptionSetMapper.java

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2004-2022, University of Oslo
2+
* Copyright (c) 2004-2026, University of Oslo
33
* All rights reserved.
44
*
55
* Redistribution and use in source and binary forms, with or without
@@ -29,15 +29,19 @@
2929
*/
3030
package org.hisp.dhis.tracker.imports.preheat.mappers;
3131

32-
import java.util.List;
33-
import org.hisp.dhis.option.Option;
3432
import org.hisp.dhis.option.OptionSet;
3533
import org.mapstruct.BeanMapping;
3634
import org.mapstruct.Mapper;
3735
import org.mapstruct.Mapping;
38-
import org.mapstruct.Named;
3936
import org.mapstruct.factory.Mappers;
4037

38+
/**
39+
* {@code options} is deliberately left unmapped — mapping it would force Hibernate to fully
40+
* materialize the {@code OptionSet.options} collection (including JSONB attribute values) on every
41+
* preheat, regardless of how many option codes the import actually references. {@link
42+
* org.hisp.dhis.tracker.imports.preheat.supplier.OptionValueSupplier} preheats only the specific
43+
* {@code (option set, code)} pairs the payload references instead.
44+
*/
4145
@Mapper
4246
public interface OptionSetMapper extends PreheatMapper<OptionSet> {
4347
OptionSetMapper INSTANCE = Mappers.getMapper(OptionSetMapper.class);
@@ -47,9 +51,5 @@ public interface OptionSetMapper extends PreheatMapper<OptionSet> {
4751
@Mapping(target = "uid")
4852
@Mapping(target = "name")
4953
@Mapping(target = "code")
50-
@Mapping(target = "options", qualifiedByName = "options")
5154
OptionSet map(OptionSet optionSet);
52-
53-
@Named("options")
54-
List<Option> mapOptionValues(List<Option> options);
5555
}

0 commit comments

Comments
 (0)