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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

Author: jason-p-pickering

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<UID> 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<Order> 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<UID> resolveAttributeOptionCombos(
+      IdentifiableProperty anyIn, DataExportParams.Input params) {
+    List<UID> 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<Order> resolveOrders(DataExportParams.Input params) {
+    List<Order> 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<UID> decodeIds(
       IdCoder.ObjectType type, @CheckForNull IdentifiableProperty from, Collection<String> ids) {

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));
+  }
+}