diff --git a/dhis-2/dhis-tracker/src/main/java/org/hisp/dhis/tracker/imports/validation/PersistablesFilter.java b/dhis-2/dhis-tracker/src/main/java/org/hisp/dhis/tracker/imports/validation/PersistablesFilter.java index f0eb30a6fa34..53617f5bdc66 100644 --- a/dhis-2/dhis-tracker/src/main/java/org/hisp/dhis/tracker/imports/validation/PersistablesFilter.java +++ b/dhis-2/dhis-tracker/src/main/java/org/hisp/dhis/tracker/imports/validation/PersistablesFilter.java @@ -260,21 +260,22 @@ private boolean hasInvalidParents( private Predicate parentConditions( List> parents) { - final Predicate parentCondition = - parent -> isMarked(parent) || this.preheat.exists(parent); - return parents.stream() .map( p -> (Predicate) t -> - parentCondition.test( + isPersistableParent( p.apply(t))) // children of invalid parents can only be persisted under // certain conditions .reduce(Predicate::and) .orElse(t -> true); // predicate always returning true for entities without parents } + private boolean isPersistableParent(TrackerDto parent) { + return isMarked(parent) || this.preheat.exists(parent); + } + /** * Add error for valid child entity with invalid parent as a reason. If a child is invalid that is * enough information for a user to know why it could not be persisted. @@ -288,7 +289,7 @@ private void addErrorsForChildren( List errors = parents.stream() .map(p -> p.apply(entity)) - .filter(this::isNotValid) // remove valid parents + .filter(p -> !isPersistableParent(p)) .map(p -> error(ValidationCode.E5000, entity, p)) .toList(); this.result.errors.addAll(errors); diff --git a/dhis-2/dhis-tracker/src/test/java/org/hisp/dhis/tracker/imports/validation/PersistablesFilterTest.java b/dhis-2/dhis-tracker/src/test/java/org/hisp/dhis/tracker/imports/validation/PersistablesFilterTest.java index 702ad58569a7..224d259d22a6 100644 --- a/dhis-2/dhis-tracker/src/test/java/org/hisp/dhis/tracker/imports/validation/PersistablesFilterTest.java +++ b/dhis-2/dhis-tracker/src/test/java/org/hisp/dhis/tracker/imports/validation/PersistablesFilterTest.java @@ -225,6 +225,32 @@ void testCreateAndUpdateValidEventOfInvalidEnrollmentCannotBeCreatedIfEnrollment persistable, EVENT, "Qck4PQ7TMun", E5000, "because enrollment `t1zaUjKgT3p`")); } + @Test + void testCreateAndUpdateValidEventOfValidEnrollmentWithInvalidTrackedEntityCannotBeCreated() { + Setup setup = + new Setup.Builder() + .trackedEntity("xK7H53f4Hc2") + .isNotValid() + .enrollment("t1zaUjKgT3p") + .event("Qck4PQ7TMun") + .build(); + + PersistablesFilter.Result persistable = + filter(setup.bundle, setup.invalidEntities, TrackerImportStrategy.CREATE_AND_UPDATE); + + assertAll( + () -> assertIsEmpty(persistable.get(TrackedEntity.class)), + () -> assertIsEmpty(persistable.get(Enrollment.class)), + () -> assertIsEmpty(persistable.get(TrackerEvent.class)), + () -> assertIsEmpty(persistable.get(SingleEvent.class)), + () -> + assertHasError( + persistable, ENROLLMENT, "t1zaUjKgT3p", E5000, "trackedEntity `xK7H53f4Hc2`"), + () -> + assertHasError( + persistable, EVENT, "Qck4PQ7TMun", E5000, "because enrollment `t1zaUjKgT3p`")); + } + @Test void testCreateAndUpdateInvalidEventOfValidEnrollmentCannotBePersisted() { Setup setup = diff --git a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/tracker/sync/BaseDataSynchronizationWithPaging.java b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/tracker/sync/BaseDataSynchronizationWithPaging.java index c1f625d115ae..6423e4d9ee5e 100644 --- a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/tracker/sync/BaseDataSynchronizationWithPaging.java +++ b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/tracker/sync/BaseDataSynchronizationWithPaging.java @@ -36,7 +36,8 @@ import static org.hisp.dhis.tracker.imports.TrackerImportStrategy.DELETE; import static org.hisp.dhis.webapi.controller.tracker.export.MappingErrors.ensureNoMappingErrors; import static org.hisp.dhis.webapi.controller.tracker.sync.TrackerSyncReportUtils.alreadyDeletedOrSucceededUids; -import static org.hisp.dhis.webapi.controller.tracker.sync.TrackerSyncReportUtils.blockingFailedUids; +import static org.hisp.dhis.webapi.controller.tracker.sync.TrackerSyncReportUtils.blockingFailedItems; +import static org.hisp.dhis.webapi.controller.tracker.sync.TrackerSyncReportUtils.failedItems; import static org.hisp.dhis.webapi.controller.tracker.sync.TrackerSyncReportUtils.failedUids; import static org.hisp.dhis.webapi.controller.tracker.sync.TrackerSyncReportUtils.formatFailedUids; import static org.hisp.dhis.webapi.controller.tracker.sync.TrackerSyncReportUtils.sendTrackerRequest; @@ -72,6 +73,7 @@ import org.hisp.dhis.tracker.TrackerType; import org.hisp.dhis.tracker.imports.report.ImportReport; import org.hisp.dhis.webapi.controller.tracker.export.MappingErrors; +import org.hisp.dhis.webapi.controller.tracker.sync.TrackerSyncReportUtils.FailedItem; import org.hisp.dhis.webapi.controller.tracker.view.Relationship; import org.springframework.web.client.RestTemplate; @@ -101,6 +103,12 @@ abstract class BaseDataSynchronizationWithPaging implements DataSynchronizationWithPaging { + /** + * Synthetic error code used in place of a real one when an entity is excluded from sync only + * because one of its children failed, not because of any error of its own. + */ + static final String CHILD_FAILED_ERROR_CODE = "CHILD_FAILED"; + private final RenderService renderService; private final RestTemplate restTemplate; private final SystemSettingsService systemSettingsService; @@ -379,7 +387,7 @@ private DeleteSyncResult syncDeleted( // An entity whose own delete came back "already deleted" achieved its goal, so it is treated // as synced here too. Set syncedTopLevelUids = alreadyDeletedOrSucceededUids(report, getTrackerType()); - Set failedTopLevelUids = blockingFailedUids(report, getTrackerType()); + List failedTopLevelItems = blockingFailedItems(report, getTrackerType()); Set blockingFailedChildUids = new HashSet<>(); StringBuilder childSummary = new StringBuilder(); @@ -387,12 +395,12 @@ private DeleteSyncResult syncDeleted( TrackerType childType = entry.getKey(); int total = entry.getValue(); Set syncedChild = alreadyDeletedOrSucceededUids(report, childType); - Set failedChild = blockingFailedUids(report, childType); - blockingFailedChildUids.addAll(failedChild); + List failedChildItems = blockingFailedItems(report, childType); + failedChildItems.forEach(item -> blockingFailedChildUids.add(item.uid())); childSummary.append( format( ", %s=%d/%d synced%s", - childType, syncedChild.size(), total, formatFailedUids(failedChild))); + childType, syncedChild.size(), total, formatFailedUids(failedChildItems))); } log.info( @@ -401,7 +409,7 @@ private DeleteSyncResult syncDeleted( getTrackerType(), syncedTopLevelUids.size(), deletedTopLevelDtos.size(), - formatFailedUids(failedTopLevelUids), + formatFailedUids(failedTopLevelItems), childSummary); return new DeleteSyncResult(syncedTopLevelUids, blockingFailedChildUids); @@ -426,22 +434,38 @@ private Set syncActive(List entities, SystemInstance instance, SystemSet Set succeededTopLevelUids = successfullyProcessedUids(report, getTrackerType()); Set syncedUids = filterByFailedChildren(succeededTopLevelUids, entities, report); - Set failedTopLevelUids = - entities.stream() - .map(this::getUid) - .filter(uid -> !syncedUids.contains(uid)) - .collect(Collectors.toCollection(HashSet::new)); - log.info( "{} create/update sync: {}/{} synced{}", getEntityName(), syncedUids.size(), entities.size(), - formatFailedUids(failedTopLevelUids)); + formatFailedUids(explainUnsyncedEntities(succeededTopLevelUids, syncedUids, report))); return syncedUids; } + /** + * Adds the default error code {@link #CHILD_FAILED_ERROR_CODE} to all top entities that are + * successfully processed but are not synchronized and don't have an error code yet. + * + * @return the list of (entity, errorCode) pairs + */ + List explainUnsyncedEntities( + Set succeededTopLevelUids, Set syncedUids, ImportReport report) { + List failures = new ArrayList<>(failedItems(report, getTrackerType())); + Set explainedUids = + failures.stream().map(FailedItem::uid).collect(Collectors.toCollection(HashSet::new)); + + for (UID uid : succeededTopLevelUids) { + if (!syncedUids.contains(uid) && !explainedUids.contains(uid)) { + explainedUids.add(uid); + failures.add(new FailedItem(uid, CHILD_FAILED_ERROR_CODE)); + } + } + + return failures; + } + private Set filterByFailedChildren( Set succeededTopLevelUids, List candidates, ImportReport report) { if (succeededTopLevelUids.isEmpty()) { diff --git a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/tracker/sync/TrackerSyncReportUtils.java b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/tracker/sync/TrackerSyncReportUtils.java index 81f3ba5af861..17da9cc17848 100644 --- a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/tracker/sync/TrackerSyncReportUtils.java +++ b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/tracker/sync/TrackerSyncReportUtils.java @@ -37,7 +37,12 @@ import static org.hisp.dhis.tracker.imports.validation.ValidationCode.E4017; import java.io.IOException; +import java.util.ArrayList; +import java.util.Comparator; import java.util.HashSet; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; +import java.util.List; import java.util.Map; import java.util.Set; import java.util.stream.Collectors; @@ -110,8 +115,105 @@ private static RequestCallback createRequestCallback( }; } - static String formatFailedUids(Set failedUids) { - return failedUids.isEmpty() ? "" : format(" (failed: %s)", failedUids); + record FailedItem(UID uid, String errorCode) {} + + private static final int MAX_UIDS_PER_REASON = 5; + + /** Formats failures grouped by error code, so a large batch of failures stays readable */ + static String formatFailedUids(List failed) { + if (failed.isEmpty()) { + return ""; + } + + Map> uidsByErrorCode = + failed.stream() + .collect( + Collectors.groupingBy( + FailedItem::errorCode, + LinkedHashMap::new, + Collectors.collectingAndThen( + Collectors.mapping( + FailedItem::uid, Collectors.toCollection(LinkedHashSet::new)), + ArrayList::new))); + + String reasons = + uidsByErrorCode.entrySet().stream() + .sorted( + Comparator.>>comparingInt(e -> e.getValue().size()) + .reversed()) + .map(e -> formatReasonGroup(e.getKey(), e.getValue())) + .collect(Collectors.joining(", ")); + + return format(" (failed: %s)", reasons); + } + + private static String formatReasonGroup(String errorCode, List uids) { + List shown = + uids.size() > MAX_UIDS_PER_REASON ? uids.subList(0, MAX_UIDS_PER_REASON) : uids; + String uidList = shown.stream().map(UID::getValue).collect(Collectors.joining(", ")); + String more = + uids.size() > MAX_UIDS_PER_REASON + ? format(", +%d more", uids.size() - MAX_UIDS_PER_REASON) + : ""; + return format("%s x%d [%s%s]", errorCode, uids.size(), uidList, more); + } + + /** Like {@link #failedUids}, but keeps the error code behind each failure for logging. */ + static List failedItems(ImportReport report, TrackerType type) { + List failed = new ArrayList<>(); + + if (report.getValidationReport() != null) { + report.getValidationReport().getErrors().stream() + .filter(e -> type.name().equals(e.getTrackerType())) + .forEach(e -> failed.add(new FailedItem(e.getUid(), e.getErrorCode()))); + } + + if (report.getPersistenceReport() != null) { + TrackerTypeReport typeReport = report.getPersistenceReport().getTypeReportMap().get(type); + if (typeReport != null) { + typeReport + .getEntityReport() + .forEach( + entity -> + entity + .getErrorReports() + .forEach( + e -> failed.add(new FailedItem(entity.getUid(), e.getErrorCode())))); + } + } + + return failed; + } + + /** + * Like {@link #blockingFailedUids}, but keeps the error code behind each failure for logging. + * Only meaningful against a DELETE report. + */ + static List blockingFailedItems(ImportReport report, TrackerType type) { + List blocking = new ArrayList<>(); + + if (report.getValidationReport() != null) { + report.getValidationReport().getErrors().stream() + .filter(e -> type.name().equals(e.getTrackerType())) + .filter(e -> !ALREADY_DELETED_CODES.contains(e.getErrorCode())) + .forEach(e -> blocking.add(new FailedItem(e.getUid(), e.getErrorCode()))); + } + + if (report.getPersistenceReport() != null) { + TrackerTypeReport typeReport = report.getPersistenceReport().getTypeReportMap().get(type); + if (typeReport != null) { + typeReport + .getEntityReport() + .forEach( + entity -> + entity.getErrorReports().stream() + .filter(e -> !ALREADY_DELETED_CODES.contains(e.getErrorCode())) + .forEach( + e -> blocking.add(new FailedItem(entity.getUid(), e.getErrorCode())))); + } + } + + return blocking; } static Set successfullyProcessedUids(ImportReport report, TrackerType type) { diff --git a/dhis-2/dhis-web-api/src/test/java/org/hisp/dhis/webapi/controller/tracker/sync/TrackerDataSynchronizationServiceTest.java b/dhis-2/dhis-web-api/src/test/java/org/hisp/dhis/webapi/controller/tracker/sync/TrackerDataSynchronizationServiceTest.java index 25ab8b814aa6..fd408ce6e7d8 100644 --- a/dhis-2/dhis-web-api/src/test/java/org/hisp/dhis/webapi/controller/tracker/sync/TrackerDataSynchronizationServiceTest.java +++ b/dhis-2/dhis-web-api/src/test/java/org/hisp/dhis/webapi/controller/tracker/sync/TrackerDataSynchronizationServiceTest.java @@ -329,6 +329,28 @@ void shouldNotStampSyncTimestampWhenActiveChildRelationshipFailedRemotely() thro verify(trackedEntityService, never()).updateTrackedEntitiesSyncTimestamp(any(), any()); } + @Test + void shouldExplainExclusionWhenEntityItselfSucceededButChildFailed() { + UID teUid = UID.of("TrackedEnt1"); + UID relationshipUid = UID.of("Rel00000001"); + ImportReport report = + reportWith( + successEntity(TrackerType.TRACKED_ENTITY, teUid), + failedEntity(TrackerType.RELATIONSHIP, relationshipUid, "E4009", "validation failed")); + + // teUid succeeded at its own type (present in succeededTopLevelUids) but is absent from + // syncedUids, i.e. filterByFailedChildren actually excluded it over the failed relationship — + // a verified child failure, not a guess. + List failures = + service.explainUnsyncedEntities(Set.of(teUid), Set.of(), report); + + assertEquals( + List.of( + new TrackerSyncReportUtils.FailedItem( + teUid, BaseDataSynchronizationWithPaging.CHILD_FAILED_ERROR_CODE)), + failures); + } + @Test void shouldNotStampSyncTimestampWhenDeletedRelationshipFailedForRealReason() throws Exception { UID teUid = UID.of("TrackedEnt1"); diff --git a/dhis-2/dhis-web-api/src/test/java/org/hisp/dhis/webapi/controller/tracker/sync/TrackerSyncReportUtilsTest.java b/dhis-2/dhis-web-api/src/test/java/org/hisp/dhis/webapi/controller/tracker/sync/TrackerSyncReportUtilsTest.java index 92e4fe33df5e..ebaedb64fdf5 100644 --- a/dhis-2/dhis-web-api/src/test/java/org/hisp/dhis/webapi/controller/tracker/sync/TrackerSyncReportUtilsTest.java +++ b/dhis-2/dhis-web-api/src/test/java/org/hisp/dhis/webapi/controller/tracker/sync/TrackerSyncReportUtilsTest.java @@ -32,6 +32,7 @@ import static org.hisp.dhis.webapi.controller.tracker.sync.TrackerSyncReportUtils.alreadyDeletedOrSucceededUids; import static org.hisp.dhis.webapi.controller.tracker.sync.TrackerSyncReportUtils.blockingFailedUids; import static org.hisp.dhis.webapi.controller.tracker.sync.TrackerSyncReportUtils.failedUids; +import static org.hisp.dhis.webapi.controller.tracker.sync.TrackerSyncReportUtils.formatFailedUids; import static org.hisp.dhis.webapi.controller.tracker.sync.TrackerSyncReportUtils.successfullyProcessedUids; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -46,6 +47,7 @@ import org.hisp.dhis.tracker.imports.report.PersistenceReport; import org.hisp.dhis.tracker.imports.report.Status; import org.hisp.dhis.tracker.imports.report.TrackerTypeReport; +import org.hisp.dhis.webapi.controller.tracker.sync.TrackerSyncReportUtils.FailedItem; import org.junit.jupiter.api.Test; class TrackerSyncReportUtilsTest { @@ -125,4 +127,59 @@ private ImportReport deleteReportFor(TrackerType type) { return ImportReport.builder().status(Status.ERROR).persistenceReport(persistenceReport).build(); } + + @Test + void shouldReturnEmptyStringWhenNoFailures() { + assertEquals("", formatFailedUids(List.of())); + } + + @Test + void shouldListAllUidsWhenFailuresAreAtOrBelowTheCap() { + List failed = List.of(item("Uid00000001", "E1032"), item("Uid00000002", "E1032")); + + assertEquals(" (failed: E1032 x2 [Uid00000001, Uid00000002])", formatFailedUids(failed)); + } + + @Test + void shouldCapUidsPerReasonAndReportHowManyMoreThereAre() { + List failed = + List.of( + item("Uid00000001", "E1032"), + item("Uid00000002", "E1032"), + item("Uid00000003", "E1032"), + item("Uid00000004", "E1032"), + item("Uid00000005", "E1032"), + item("Uid00000006", "E1032"), + item("Uid00000007", "E1032")); + + assertEquals( + " (failed: E1032 x7 [Uid00000001, Uid00000002, Uid00000003, Uid00000004, Uid00000005," + + " +2 more])", + formatFailedUids(failed)); + } + + @Test + void shouldGroupByErrorCodeAndOrderReasonsByDescendingFailureCount() { + List failed = + List.of( + item("Uid00000001", "E1082"), + item("Uid00000002", "E1032"), + item("Uid00000003", "E1032"), + item("Uid00000004", "E1032")); + + assertEquals( + " (failed: E1032 x3 [Uid00000002, Uid00000003, Uid00000004], E1082 x1 [Uid00000001])", + formatFailedUids(failed)); + } + + @Test + void shouldCountAFailedEntityOnceEvenWhenItHasMultipleErrorsUnderTheSameCode() { + List failed = List.of(item("Uid00000001", "E5000"), item("Uid00000001", "E5000")); + + assertEquals(" (failed: E5000 x1 [Uid00000001])", formatFailedUids(failed)); + } + + private static FailedItem item(String uid, String errorCode) { + return new FailedItem(UID.of(uid), errorCode); + } }