From 49c0b2f74a5de1e886ba86043e1894d4bb94b092 Mon Sep 17 00:00:00 2001 From: Jason Pickering Date: Fri, 31 Jul 2026 13:42:11 +0200 Subject: [PATCH] fix: reject invalid/negative lastUpdatedDuration on dataValueSets export instead of silently ignoring it (#24470) * fix: reject invalid/negative lastUpdatedDuration on dataValueSets export instead of silently ignoring it DateUtils.getDuration() returns null on any parse failure (its regex requires unsigned digits, so negative durations like "-5d" never match), and DefaultDataExportService.decodeParams() called it unchecked, so a malformed or negative lastUpdatedDuration was silently discarded - indistinguishable from "not provided". If it was the client's only time filter, this surfaced as the unrelated E2002 error instead of pointing at the actual problem; otherwise it was dropped with no error at all. ErrorCode.E2005 ("Duration is not valid: `{0}`") already exists for exactly this and is used by the sibling DefaultCompleteDataSetRegistrationExchangeService for its createdDuration parameter, but was never wired up here. decodeParams() now throws ConflictException(E2005, rawValue) when lastUpdatedDuration is non-blank but fails to parse. lastUpdatedDuration=0d is deliberately left valid - it parses successfully (Duration.ZERO) and is a well-defined (if narrow) "as of right now" filter, analogous to startDate == endDate being a valid zero-width date range. Verified via mutation testing: temporarily disabled the new guard, confirmed only the two negative-case tests failed (the 0d-is-valid test stayed green), then restored. No regressions in DataExportServiceExportTest, DataValueServiceTest, or DataValueSetControllerTest. Related to DHIS2-21821. Co-Authored-By: Claude Sonnet 5 * fix: reduce decodeParams cognitive complexity and cover blank duration branch Extracts attribute-option-combo, order, and lastUpdatedDuration resolution into their own methods so decodeParams stays under the complexity limit, and adds a test for the blank (non-null) lastUpdatedDuration case that the isBlank() guard exists to handle. Co-Authored-By: Claude Sonnet 5 --------- Co-authored-by: Claude Sonnet 5 --- .../DefaultDataExportService.java | 61 +++++---- .../DefaultDataExportServiceTest.java | 118 ++++++++++++++++++ 2 files changed, 157 insertions(+), 22 deletions(-) create mode 100644 dhis-2/dhis-services/dhis-service-dxf2/src/test/java/org/hisp/dhis/dxf2/datavalueset/DefaultDataExportServiceTest.java diff --git a/dhis-2/dhis-services/dhis-service-dxf2/src/main/java/org/hisp/dhis/dxf2/datavalueset/DefaultDataExportService.java b/dhis-2/dhis-services/dhis-service-dxf2/src/main/java/org/hisp/dhis/dxf2/datavalueset/DefaultDataExportService.java index 6f8814a9e058..531f00aa597d 100644 --- a/dhis-2/dhis-services/dhis-service-dxf2/src/main/java/org/hisp/dhis/dxf2/datavalueset/DefaultDataExportService.java +++ b/dhis-2/dhis-services/dhis-service-dxf2/src/main/java/org/hisp/dhis/dxf2/datavalueset/DefaultDataExportService.java @@ -39,6 +39,7 @@ import static org.hisp.dhis.user.CurrentUserUtil.getCurrentUserDetails; import static org.hisp.dhis.util.DateUtils.toLongGmtDate; +import java.time.Duration; import java.util.ArrayList; import java.util.Collection; import java.util.Iterator; @@ -345,7 +346,7 @@ private static String emptyAsNull(String value) { return value == null || value.isEmpty() ? null : value; } - private DataExportParams decodeParams(DataExportParams.Input params) { + private DataExportParams decodeParams(DataExportParams.Input params) throws ConflictException { boolean codeFallback = Boolean.TRUE.equals(params.getInputUseCodeFallback()); IdentifiableProperty anyIn = params.getInputIdScheme(); IdentifiableProperty dsIn = params.getInputDataSetIdScheme(); @@ -358,24 +359,6 @@ private DataExportParams decodeParams(DataExportParams.Input params) { if (ouIn == null) ouIn = anyIn; if (degIn == null) degIn = anyIn; - List attributeOptionCombos = decodeIds(COC, anyIn, params.getAttributeOptionCombo()); - if (attributeOptionCombos.isEmpty() && params.getAttributeOptions() != null) { - UID aoc = - store.getAttributeOptionCombo( - params.getAttributeCombo(), params.getAttributeOptions().stream()); - if (aoc != null) attributeOptionCombos = List.of(aoc); - } - - List orders = params.getOrder(); - if (params.isOrderByPeriod()) { - if (orders == null) { - orders = List.of(Order.PE); - } else if (!orders.contains(Order.PE)) { - orders = new ArrayList<>(orders); - orders.add(0, Order.PE); - } - } - return DataExportParams.builder() .dataSets(decodeIds(DS, dsIn, params.getDataSet())) .dataElementGroups(decodeIds(DEG, degIn, params.getDataElementGroup())) @@ -384,20 +367,54 @@ private DataExportParams decodeParams(DataExportParams.Input params) { .organisationUnitGroups(decodeIds(OUG, anyIn, params.getOrgUnitGroup())) .orgUnitLevel(params.getLevel()) .categoryOptionCombos(decodeIds(COC, anyIn, params.getCategoryOptionCombo())) - .attributeOptionCombos(attributeOptionCombos) + .attributeOptionCombos(resolveAttributeOptionCombos(anyIn, params)) .periods(decodePeriods(params.getPeriod())) .startDate(params.getStartDate()) .endDate(params.getEndDate()) .includeDescendants(params.isChildren()) .includeDeleted(params.isIncludeDeleted()) .lastUpdated(params.getLastUpdated()) - .lastUpdatedDuration(DateUtils.getDuration(params.getLastUpdatedDuration())) + .lastUpdatedDuration(resolveLastUpdatedDuration(params)) .limit(params.getLimit()) .offset(params.getOffset()) - .orders(orders) + .orders(resolveOrders(params)) .build(); } + @Nonnull + private List resolveAttributeOptionCombos( + IdentifiableProperty anyIn, DataExportParams.Input params) { + List attributeOptionCombos = decodeIds(COC, anyIn, params.getAttributeOptionCombo()); + if (!attributeOptionCombos.isEmpty() || params.getAttributeOptions() == null) { + return attributeOptionCombos; + } + UID aoc = + store.getAttributeOptionCombo( + params.getAttributeCombo(), params.getAttributeOptions().stream()); + return aoc == null ? attributeOptionCombos : List.of(aoc); + } + + private List resolveOrders(DataExportParams.Input params) { + List orders = params.getOrder(); + if (!params.isOrderByPeriod()) return orders; + if (orders == null) return List.of(Order.PE); + if (orders.contains(Order.PE)) return orders; + orders = new ArrayList<>(orders); + orders.add(0, Order.PE); + return orders; + } + + private Duration resolveLastUpdatedDuration(DataExportParams.Input params) + throws ConflictException { + String lastUpdatedDurationIn = params.getLastUpdatedDuration(); + Duration lastUpdatedDuration = DateUtils.getDuration(lastUpdatedDurationIn); + if (lastUpdatedDurationIn != null + && !lastUpdatedDurationIn.isBlank() + && lastUpdatedDuration == null) + throw new ConflictException(ErrorCode.E2005, lastUpdatedDurationIn); + return lastUpdatedDuration; + } + @Nonnull private List decodeIds( IdCoder.ObjectType type, @CheckForNull IdentifiableProperty from, Collection ids) { diff --git a/dhis-2/dhis-services/dhis-service-dxf2/src/test/java/org/hisp/dhis/dxf2/datavalueset/DefaultDataExportServiceTest.java b/dhis-2/dhis-services/dhis-service-dxf2/src/test/java/org/hisp/dhis/dxf2/datavalueset/DefaultDataExportServiceTest.java new file mode 100644 index 000000000000..a75966062a47 --- /dev/null +++ b/dhis-2/dhis-services/dhis-service-dxf2/src/test/java/org/hisp/dhis/dxf2/datavalueset/DefaultDataExportServiceTest.java @@ -0,0 +1,118 @@ +/* + * Copyright (c) 2004-2022, University of Oslo + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * 3. Neither the name of the copyright holder nor the names of its contributors + * may be used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR + * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package org.hisp.dhis.dxf2.datavalueset; + +import static org.hisp.dhis.test.TestBase.clearSecurityContext; +import static org.hisp.dhis.test.TestBase.injectSecurityContextNoSettings; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.util.Set; +import org.hisp.dhis.common.IdCoder; +import org.hisp.dhis.common.UID; +import org.hisp.dhis.datavalue.DataExportParams; +import org.hisp.dhis.datavalue.DataExportStore; +import org.hisp.dhis.feedback.ConflictException; +import org.hisp.dhis.feedback.ErrorCode; +import org.hisp.dhis.user.SystemUser; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) +class DefaultDataExportServiceTest { + + @Mock private DataExportStore store; + + @Mock private IdCoder idCoder; + + @BeforeEach + void setUp() { + injectSecurityContextNoSettings(new SystemUser()); + } + + @AfterEach + void tearDown() { + clearSecurityContext(); + } + + private DataExportParams.Input.InputBuilder validFiltersBuilder() { + return DataExportParams.Input.builder() + .dataSet(Set.of(UID.generate().getValue())) + .orgUnit(Set.of(UID.generate().getValue())) + .period(Set.of("202201")); + } + + private DefaultDataExportService service() { + return new DefaultDataExportService(store, idCoder); + } + + @Test + void testExportValues_InvalidLastUpdatedDurationThrowsE2005() { + DataExportParams.Input params = validFiltersBuilder().lastUpdatedDuration("-5d").build(); + + ConflictException ex = + assertThrows(ConflictException.class, () -> service().exportValues(params)); + + assertEquals(ErrorCode.E2005, ex.getCode()); + } + + @Test + void testExportValues_MalformedLastUpdatedDurationThrowsE2005() { + DataExportParams.Input params = validFiltersBuilder().lastUpdatedDuration("abc").build(); + + ConflictException ex = + assertThrows(ConflictException.class, () -> service().exportValues(params)); + + assertEquals(ErrorCode.E2005, ex.getCode()); + } + + @Test + void testExportValues_ZeroDurationIsValid() { + // 0d parses successfully (Duration.ZERO); it's a degenerate "as of right now" filter, + // not a parse failure, so it must not be rejected the same way "-5d"/"abc" are. + DataExportParams.Input params = validFiltersBuilder().lastUpdatedDuration("0d").build(); + + assertDoesNotThrow(() -> service().exportValues(params)); + } + + @Test + void testExportValues_BlankLastUpdatedDurationIsIgnored() { + // a blank (but non-null) value never reaches the parser, so it must not be + // treated as unparseable and rejected like "-5d"/"abc" are. + DataExportParams.Input params = validFiltersBuilder().lastUpdatedDuration(" ").build(); + + assertDoesNotThrow(() -> service().exportValues(params)); + } +}