Skip to content

Commit 2d446ef

Browse files
fix: avoid OptionSet.getOptions() N+1 in analytics metadata resolution [DHIS2-21975]
QueryItemHelper.getItemOptions and its helpers called gridHeader.getOptionSetObject().getOptions() directly to resolve option codes for grid metadata. That collection is L2-cached by ID list only; reassembling entities under a cold entity-level cache falls back to one SELECT per option (profiled: 6,713 individual optionvalue selects, 1.38s of a 2s /analytics/events/query response). Thread an injectable options resolver through QueryItemHelper instead, wired at the three real callers to OptionService.findOptionsByNamePattern, which fetches the option set in one query and bypasses the broken collection-cache path entirely. Matching semantics are unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 0fe439e commit 2d446ef

10 files changed

Lines changed: 140 additions & 35 deletions

File tree

dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/common/processing/MetadataDimensionsHandler.java

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@
5252
import org.hisp.dhis.common.MetadataItem;
5353
import org.hisp.dhis.common.QueryItem;
5454
import org.hisp.dhis.option.Option;
55+
import org.hisp.dhis.option.OptionService;
5556
import org.hisp.dhis.period.PeriodType;
5657

5758
/**
@@ -66,6 +67,12 @@
6667
* the risk of bugs.
6768
*/
6869
public class MetadataDimensionsHandler {
70+
private final OptionService optionService;
71+
72+
MetadataDimensionsHandler(OptionService optionService) {
73+
this.optionService = optionService;
74+
}
75+
6976
/**
7077
* Handles all required logic/rules in order to return a map of metadata item identifiers.
7178
*
@@ -147,7 +154,11 @@ private void putFilterItemsIntoMap(
147154
*/
148155
private void putQueryItemsIntoMap(
149156
Map<String, List<String>> dimensionItems, List<QueryItem> items, Grid grid) {
150-
Map<String, List<Option>> optionsPresentInGrid = getItemOptions(grid, items);
157+
Map<String, List<Option>> optionsPresentInGrid =
158+
getItemOptions(
159+
grid,
160+
items,
161+
optionSet -> optionService.findOptionsByNamePattern(optionSet.getUid(), null, null));
151162

152163
for (QueryItem item : items) {
153164
String itemUid = getItemUid(item);

dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/common/processing/MetadataItemsHandler.java

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@
6464
import org.hisp.dhis.dataelement.DataElement;
6565
import org.hisp.dhis.legend.Legend;
6666
import org.hisp.dhis.option.Option;
67+
import org.hisp.dhis.option.OptionService;
6768
import org.hisp.dhis.option.OptionSet;
6869
import org.hisp.dhis.organisationunit.OrganisationUnit;
6970
import org.hisp.dhis.period.PeriodDimension;
@@ -83,6 +84,12 @@
8384
* the risk of bugs.
8485
*/
8586
public class MetadataItemsHandler {
87+
private final OptionService optionService;
88+
89+
public MetadataItemsHandler(OptionService optionService) {
90+
this.optionService = optionService;
91+
}
92+
8693
/**
8794
* Handles all required logic/rules in order to return a map of metadata item identifiers and its
8895
* respective {@link MetadataItem}.
@@ -103,7 +110,11 @@ Map<String, MetadataItem> handle(
103110
delegator.getItemsOptions().stream()
104111
.filter(o -> isInOriginalRequest(o.getUid(), commonRequestParams))
105112
.collect(toSet());
106-
Map<String, List<Option>> optionsPresentInGrid = getItemOptions(grid, items);
113+
Map<String, List<Option>> optionsPresentInGrid =
114+
getItemOptions(
115+
grid,
116+
items,
117+
optionSet -> optionService.findOptionsByNamePattern(optionSet.getUid(), null, null));
107118
Set<Option> optionItems = getOptionItems(grid, itemOptions, items, optionsPresentInGrid);
108119
List<DimensionalObject> allDimensionalObjects = delegator.getAllDimensionalObjects();
109120
List<DimensionItemKeywords.Keyword> periodKeywords = delegator.getPeriodKeywords();

dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/common/processing/MetadataParamsHandler.java

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@
6565
import org.hisp.dhis.common.Grid;
6666
import org.hisp.dhis.common.MetadataItem;
6767
import org.hisp.dhis.common.QueryItem;
68+
import org.hisp.dhis.option.OptionService;
6869
import org.hisp.dhis.organisationunit.OrganisationUnit;
6970
import org.hisp.dhis.user.User;
7071
import org.springframework.stereotype.Component;
@@ -83,6 +84,12 @@ public class MetadataParamsHandler {
8384
private static final String DOT = ".";
8485
private static final String ORG_UNIT_DIM = "ou";
8586

87+
private final OptionService optionService;
88+
89+
public MetadataParamsHandler(OptionService optionService) {
90+
this.optionService = optionService;
91+
}
92+
8693
/**
8794
* Appends the metadata to the given {@link Grid} based on the given arguments.
8895
*
@@ -103,7 +110,8 @@ public void handle(
103110
List<AnalyticsMetaDataKey> userOrgUnitMetaDataKeys =
104111
getUserOrgUnitsMetadataKeys(commonParsed);
105112
Map<String, Object> items =
106-
new HashMap<>(new MetadataItemsHandler().handle(grid, commonParsed, commonRequest));
113+
new HashMap<>(
114+
new MetadataItemsHandler(optionService).handle(grid, commonParsed, commonRequest));
107115

108116
commonParsed
109117
.getAllDimensionIdentifiers()
@@ -116,7 +124,8 @@ public void handle(
116124
metadataInfo.put(ITEMS.getKey(), items);
117125

118126
metadataInfo.put(
119-
DIMENSIONS.getKey(), new MetadataDimensionsHandler().handle(grid, commonParsed));
127+
DIMENSIONS.getKey(),
128+
new MetadataDimensionsHandler(optionService).handle(grid, commonParsed));
120129

121130
// Org. Units.
122131
boolean hierarchyMeta = commonRequest.isHierarchyMeta();

dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/event/data/QueryItemHelper.java

Lines changed: 34 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
import java.util.Objects;
4949
import java.util.Optional;
5050
import java.util.Set;
51+
import java.util.function.Function;
5152
import java.util.stream.Collectors;
5253
import lombok.extern.slf4j.Slf4j;
5354
import org.apache.commons.lang3.StringUtils;
@@ -174,18 +175,23 @@ private static Option getOptionByCode(OptionSet set, String code) {
174175
*
175176
* @param grid the {@link Grid}.
176177
* @param queryItems the list of {@link QueryItem}.
178+
* @param optionsResolver resolves the {@link Option}s belonging to an {@link OptionSet}. Callers
179+
* backed by a real persistence context should supply a resolver that fetches the options
180+
* directly (e.g. via a service call), rather than {@link OptionSet#getOptions()}, since that
181+
* lazy collection can degrade into one query per option under a cold second-level cache.
177182
* @return a map of list of options.
178183
*/
179-
public static Map<String, List<Option>> getItemOptions(Grid grid, List<QueryItem> queryItems) {
184+
public static Map<String, List<Option>> getItemOptions(
185+
Grid grid, List<QueryItem> queryItems, Function<OptionSet, List<Option>> optionsResolver) {
180186
Map<String, List<Option>> options = new HashMap<>();
181187

182188
for (int i = 0; i < grid.getHeaders().size(); ++i) {
183189
GridHeader gridHeader = grid.getHeaders().get(i);
184190

185191
if (gridHeader.hasOptionSet() && isNotEmpty(grid.getRows())) {
186-
options.put(gridHeader.getName(), getItemOptionsThatMatchesRows(grid, i));
192+
options.put(gridHeader.getName(), getItemOptionsThatMatchesRows(grid, i, optionsResolver));
187193
} else if (gridHeader.hasOptionSet() && isEmpty(grid.getRows())) {
188-
options.put(gridHeader.getName(), getItemOptionsForEmptyRows(queryItems));
194+
options.put(gridHeader.getName(), getItemOptionsForEmptyRows(queryItems, optionsResolver));
189195
}
190196
}
191197

@@ -255,18 +261,14 @@ private static boolean filtersContainOption(Option option, List<QueryFilter> que
255261
* @param queryItems the {@link EventQueryParams}.
256262
* @return the options for empty rows.
257263
*/
258-
private static List<Option> getItemOptionsForEmptyRows(List<QueryItem> queryItems) {
264+
private static List<Option> getItemOptionsForEmptyRows(
265+
List<QueryItem> queryItems, Function<OptionSet, List<Option>> optionsResolver) {
259266
List<Option> options = new ArrayList<>();
260267

261268
if (isNotEmpty(queryItems)) {
262-
List<QueryItem> items = queryItems;
263-
264-
for (QueryItem item : items) {
265-
boolean hasOptions =
266-
item.getOptionSet() != null && isNotEmpty(item.getOptionSet().getOptions());
267-
268-
if (hasOptions && isNotEmpty(item.getFilters())) {
269-
options.addAll(getItemOptionsForFilter(item));
269+
for (QueryItem item : queryItems) {
270+
if (item.getOptionSet() != null && isNotEmpty(item.getFilters())) {
271+
options.addAll(getItemOptionsForFilter(item, optionsResolver));
270272
}
271273
}
272274
}
@@ -281,9 +283,12 @@ private static List<Option> getItemOptionsForEmptyRows(List<QueryItem> queryItem
281283
*
282284
* @param grid the {@link Grid}.
283285
* @param columnIndex the column index.
286+
* @param optionsResolver resolves the {@link Option}s belonging to an {@link OptionSet} without
287+
* going through {@link OptionSet#getOptions()}.
284288
* @return a list of matching options.
285289
*/
286-
private static List<Option> getItemOptionsThatMatchesRows(Grid grid, int columnIndex) {
290+
private static List<Option> getItemOptionsThatMatchesRows(
291+
Grid grid, int columnIndex, Function<OptionSet, List<Option>> optionsResolver) {
287292
if (grid == null || grid.getHeaders() == null || columnIndex < 0) {
288293
return Collections.emptyList();
289294
}
@@ -292,15 +297,11 @@ private static List<Option> getItemOptionsThatMatchesRows(Grid grid, int columnI
292297
}
293298

294299
GridHeader gridHeader = grid.getHeaders().get(columnIndex);
295-
if (gridHeader == null
296-
|| gridHeader.getOptionSetObject() == null
297-
|| gridHeader.getOptionSetObject().getOptions() == null) {
300+
if (gridHeader == null || gridHeader.getOptionSetObject() == null) {
298301
return Collections.emptyList();
299302
}
300303

301-
List<Option> allOptions = gridHeader.getOptionSetObject().getOptions();
302-
// Check if there are no options to filter or no rows to check against
303-
if (allOptions.isEmpty() || grid.getRows() == null || grid.getRows().isEmpty()) {
304+
if (grid.getRows() == null || grid.getRows().isEmpty()) {
304305
return Collections.emptyList();
305306
}
306307

@@ -315,11 +316,16 @@ private static List<Option> getItemOptionsThatMatchesRows(Grid grid, int columnI
315316
.collect(Collectors.toSet());
316317

317318
// If there are no distinct, non-null values in the rows for this column, no option can possibly
318-
// match.
319+
// match, so there is no need to resolve any options at all.
319320
if (distinctRowCellValues.isEmpty()) {
320321
return Collections.emptyList();
321322
}
322323

324+
List<Option> allOptions = optionsResolver.apply(gridHeader.getOptionSetObject());
325+
if (allOptions == null || allOptions.isEmpty()) {
326+
return Collections.emptyList();
327+
}
328+
323329
// Filter the options
324330
// For each option, check if its code matches any of the pre-collected distinct row cell values.
325331
return allOptions.stream()
@@ -400,10 +406,16 @@ public static boolean isItemOptionEqualToRowContent(String code, Object rowConte
400406
* @param item the {@link QueryItem}.
401407
* @return a list of options found in the filter.
402408
*/
403-
private static List<Option> getItemOptionsForFilter(QueryItem item) {
409+
private static List<Option> getItemOptionsForFilter(
410+
QueryItem item, Function<OptionSet, List<Option>> optionsResolver) {
404411
List<Option> options = new ArrayList<>();
412+
List<Option> resolvedOptions = optionsResolver.apply(item.getOptionSet());
413+
414+
if (resolvedOptions == null) {
415+
return options;
416+
}
405417

406-
for (Option option : item.getOptionSet().getOptions()) {
418+
for (Option option : resolvedOptions) {
407419
for (QueryFilter filter : item.getFilters()) {
408420
List<String> filterSplit =
409421
Arrays.stream(trimToEmpty(filter.getFilter()).split(";")).collect(toList());

dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/tracker/MetadataItemsHandler.java

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,8 @@
9191
import org.hisp.dhis.i18n.I18nFormat;
9292
import org.hisp.dhis.i18n.I18nManager;
9393
import org.hisp.dhis.option.Option;
94+
import org.hisp.dhis.option.OptionService;
95+
import org.hisp.dhis.option.OptionSet;
9496
import org.hisp.dhis.organisationunit.OrganisationUnit;
9597
import org.hisp.dhis.period.PeriodDimension;
9698
import org.hisp.dhis.period.PeriodType;
@@ -112,6 +114,8 @@ public class MetadataItemsHandler {
112114

113115
private final I18nManager i18nManager;
114116

117+
private final OptionService optionService;
118+
115119
/**
116120
* Adds meta-data values to the given grid based on the given data query parameters.
117121
*
@@ -187,7 +191,8 @@ private void addUserOrgUnitItems(Map<String, Object> items, EventQueryParams par
187191
*/
188192
private Set<Option> collectOptionItems(Grid grid, EventQueryParams params) {
189193
Set<Option> optionItems = new LinkedHashSet<>();
190-
Map<String, List<Option>> optionsPresentInGrid = getItemOptions(grid, params.getItems());
194+
Map<String, List<Option>> optionsPresentInGrid =
195+
getItemOptions(grid, params.getItems(), this::resolveOptions);
191196

192197
if (isNotEmpty(grid.getRows())) {
193198
optionItems.addAll(
@@ -208,12 +213,25 @@ private Set<Option> collectOptionItems(Grid grid, EventQueryParams params) {
208213
*/
209214
private Map<String, List<String>> buildDimensionItems(Grid grid, EventQueryParams params) {
210215
if (params.isComingFromQuery()) {
211-
Map<String, List<Option>> optionsPresentInGrid = getItemOptions(grid, params.getItems());
216+
Map<String, List<Option>> optionsPresentInGrid =
217+
getItemOptions(grid, params.getItems(), this::resolveOptions);
212218
return getDimensionItems(params, Optional.of(optionsPresentInGrid));
213219
}
214220
return getDimensionItems(params, empty());
215221
}
216222

223+
/**
224+
* Resolves the {@link Option}s belonging to the given {@link OptionSet} directly, instead of
225+
* through {@link OptionSet#getOptions()} - that lazy collection can fall back to one query per
226+
* option under a cold second-level cache.
227+
*
228+
* @param optionSet the {@link OptionSet}.
229+
* @return the list of {@link Option}.
230+
*/
231+
private List<Option> resolveOptions(OptionSet optionSet) {
232+
return optionService.findOptionsByNamePattern(optionSet.getUid(), null, null);
233+
}
234+
217235
/**
218236
* Returns a unified map of metadata item identifiers and {@link MetadataItem}. This method
219237
* handles both query and non-query scenarios.
@@ -337,7 +355,7 @@ private void addOptionMetadataForQuery(
337355
includeDetails ? option.getUid() : null,
338356
option.getCode())));
339357

340-
new org.hisp.dhis.analytics.common.processing.MetadataItemsHandler()
358+
new org.hisp.dhis.analytics.common.processing.MetadataItemsHandler(optionService)
341359
.addOptionsSetIntoMap(metadataItemMap, itemOptions);
342360
}
343361

dhis-2/dhis-services/dhis-service-analytics/src/test/java/org/hisp/dhis/analytics/common/GridAdaptorTest.java

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@
7272
import org.hisp.dhis.common.Grid;
7373
import org.hisp.dhis.i18n.I18n;
7474
import org.hisp.dhis.i18n.I18nManager;
75+
import org.hisp.dhis.option.OptionService;
7576
import org.hisp.dhis.program.Program;
7677
import org.hisp.dhis.program.ProgramStage;
7778
import org.hisp.dhis.test.TestBase;
@@ -96,6 +97,8 @@ class GridAdaptorTest extends TestBase {
9697

9798
@Mock private I18n i18n;
9899

100+
@Mock private OptionService optionService;
101+
99102
private GridAdaptor gridAdaptor;
100103

101104
private HeaderParamsHandler headerParamsHandler;
@@ -107,7 +110,7 @@ class GridAdaptorTest extends TestBase {
107110
@BeforeEach
108111
void setUp() {
109112
headerParamsHandler = new HeaderParamsHandler();
110-
metadataDetailsHandler = new MetadataParamsHandler();
113+
metadataDetailsHandler = new MetadataParamsHandler(optionService);
111114
schemeIdResponseMapper = new SchemeIdResponseMapper(i18nManager);
112115
gridAdaptor =
113116
new GridAdaptor(headerParamsHandler, metadataDetailsHandler, schemeIdResponseMapper);

dhis-2/dhis-services/dhis-service-analytics/src/test/java/org/hisp/dhis/analytics/common/processing/MetadataParamsHandlerTest.java

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@
5151
import org.hisp.dhis.common.QueryItem;
5252
import org.hisp.dhis.common.ValueType;
5353
import org.hisp.dhis.dataelement.DataElement;
54+
import org.hisp.dhis.option.OptionService;
5455
import org.hisp.dhis.program.Program;
5556
import org.hisp.dhis.program.ProgramStage;
5657
import org.hisp.dhis.system.grid.ListGrid;
@@ -59,16 +60,19 @@
5960
import org.junit.jupiter.api.Nested;
6061
import org.junit.jupiter.api.Test;
6162
import org.junit.jupiter.api.extension.ExtendWith;
63+
import org.mockito.Mock;
6264
import org.mockito.junit.jupiter.MockitoExtension;
6365

6466
@ExtendWith(MockitoExtension.class)
6567
class MetadataParamsHandlerTest {
6668

69+
@Mock private OptionService optionService;
70+
6771
private MetadataParamsHandler metadataParamsHandler;
6872

6973
@BeforeEach
7074
void setUp() {
71-
metadataParamsHandler = new MetadataParamsHandler();
75+
metadataParamsHandler = new MetadataParamsHandler(optionService);
7276
}
7377

7478
@Nested

0 commit comments

Comments
 (0)