Skip to content

Commit

Permalink
fix: Remove unused check on delete persistables [DHIS2-17402] (#17784)
Browse files Browse the repository at this point in the history 
* fix: Remove unused check on delete persistables [DHIS2-17402]

* Code review comments

* Code review comments
  • Loading branch information
@enricocolasante
enricocolasante committed Jun 24, 2024
1 parent 24769c8 commit c3e8fd9
Show file tree
Hide file tree
Showing 10 changed files with 272 additions and 236 deletions.
5 changes: 3 additions & 2 deletions ...vice-tracker/src/main/java/org/hisp/dhis/tracker/preprocess/EventProgramPreProcessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@

import java.util.List;
import java.util.Optional;
import java.util.stream.Collectors;
import org.hisp.dhis.program.Program;
import org.hisp.dhis.program.ProgramStage;
import org.hisp.dhis.tracker.TrackerIdSchemeParams;
Expand All @@ -57,7 +58,7 @@ public void process(TrackerBundle bundle) {
|| e.getProgram().isBlank()
|| e.getProgramStage() == null
|| e.getProgramStage().isBlank())
.toList();
.collect(Collectors.toList());

for (Event event : eventsToPreprocess) {
// Extract program from program stage
Expand Down Expand Up @@ -114,7 +115,7 @@ private void setAttributeOptionCombo(TrackerBundle bundle) {
e.getAttributeOptionCombo().isBlank()
&& !e.getAttributeCategoryOptions().isEmpty())
.filter(e -> preheat.getProgram(e.getProgram()) != null)
.toList();
.collect(Collectors.toList());

for (Event e : events) {
Program program = preheat.getProgram(e.getProgram());
Expand Down
2 changes: 0 additions & 2 deletions ...ces/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/report/TrackerErrorCode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,8 +181,6 @@ public enum TrackerErrorCode {
E4020("User: `{0}`, has no write access to relationship: `{1}`."),
E5000(
"\"{0}\" `{1}` cannot be persisted because \"{2}\" `{3}` referenced by it cannot be persisted."),
E5001(
"\"{0}\" `{1}` cannot be deleted because \"{2}\" `{3}` referenced by it cannot be deleted."),
E9999("N/A");

private final String message;
Expand Down
75 changes: 19 additions & 56 deletions ...is-service-tracker/src/main/java/org/hisp/dhis/tracker/validation/PersistablesFilter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,9 @@
* TrackerBundle} is not mutated!
*
* <p>Errors are only added to {@link Result#errors} if they add information. For example a valid
* entity with invalid children cannot be deleted because of its children. Since the valid parent
* did not already have an error added during validation on will be added here. For more information
* see {@link #addErrorsForParents(List, TrackerDto)} and {@link #addErrorsForChildren(List,
* TrackerDto)}.
* entity with invalid parent cannot be created because of its parent. Since the valid children did
* not already have an error added during validation on will be added here. For more information see
* {@link #addErrorsForChildren(List, TrackerDto)}.
*
* <p>This filter relies on preprocessing and all {@link
* org.hisp.dhis.tracker.validation.TrackerValidationHook}s having run beforehand. The following are
Expand All @@ -84,17 +83,18 @@
* <li>This does not validate whether the {@link TrackerImportStrategy} matches the state of a
* given entity. In case a non existing entity is updated it is expected to already be flagged
* in the {@code invalidEntities}.
* <li>Existence is only checked in relation to a parent or child. During {@link
* TrackerImportStrategy#UPDATE} a valid enrollment can be updated if its parent the TEI is
* invalid but exists. Same applies to {@link TrackerImportStrategy#DELETE} as you cannot
* delete an entity that does not yet exist.
* <li>Existence is only checked in relation to a parent. During {@link
* TrackerImportStrategy#UPDATE} a valid enrollment can be updated if its parent the TE is
* invalid but exists.
* <li>An {@link Event} in an event program does not have an {@link Event#getEnrollment()}
* (parent) set in the payload. This expects one to be set during preprocessing. So events in
* program with or without registration do not get any special treatment.
* <li>{@link TrackerImportStrategy#DELETE} with {@link
* org.hisp.dhis.security.Authorities#F_TEI_CASCADE_DELETE} is not treated differently than a
* delete without such authority. Validation should have already flagged entities in {@code
* invalidEntities} if they have children that cannot be deleted by that user.
* <li>It is not possible to have a valid parent with invalid children in {@link
* TrackerImportStrategy#DELETE} as an entity can be invalid only in 2 cases:
* <ul>
* <li>if it doesn't exist there are no children to check
* <li>if user has no access to it, then it will not have access to its children
* </ul>
* </ul>
*/
class PersistablesFilter {
Expand Down Expand Up @@ -160,11 +160,12 @@ private PersistablesFilter(
private void filter() {
if (onDelete()) {
// bottom-up
collectDeletables(Relationship.class, RELATIONSHIP_PARENTS, bundle.getRelationships());
collectDeletables(Event.class, EVENT_PARENTS, bundle.getEvents());
collectDeletables(Enrollment.class, ENROLLMENT_PARENTS, bundle.getEnrollments());
collectDeletables(TrackedEntity.class, TRACKED_ENTITY_PARENTS, bundle.getTrackedEntities());
collectDeletables(Relationship.class, bundle.getRelationships());
collectDeletables(Event.class, bundle.getEvents());
collectDeletables(Enrollment.class, bundle.getEnrollments());
collectDeletables(TrackedEntity.class, bundle.getTrackedEntities());
} else {

// top-down
collectPersistables(TrackedEntity.class, TRACKED_ENTITY_PARENTS, bundle.getTrackedEntities());
collectPersistables(Enrollment.class, ENROLLMENT_PARENTS, bundle.getEnrollments());
Expand All @@ -173,16 +174,11 @@ private void filter() {
}
}

private <T extends TrackerDto> void collectDeletables(
Class<T> type, List<Function<T, TrackerDto>> parents, List<T> entities) {
private <T extends TrackerDto> void collectDeletables(Class<T> type, List<T> entities) {
for (T entity : entities) {
if (isValid(entity) && isDeletable(entity)) {
if (isValid(entity)) {
collectPersistable(type, entity);
continue;
}

List<TrackerErrorReport> errors = addErrorsForParents(parents, entity);
markAsNonDeletable(errors);
}
}

Expand Down Expand Up @@ -211,10 +207,6 @@ private boolean isContained(EnumMap<TrackerType, Set<String>> map, TrackerDto en
return map.get(entity.getTrackerType()).contains(entity.getUid());
}

private <T extends TrackerDto> boolean isDeletable(T entity) {
return !isMarked(entity);
}

/**
* Collect given entity in the persistables result. This entity can be created, updated or deleted
* (depending on context and {@link TrackerImportStrategy}.
Expand All @@ -240,22 +232,10 @@ private <T extends TrackerDto> void mark(T entity) {
this.markedEntities.get(entity.getTrackerType()).add(entity.getUid());
}

private void mark(TrackerErrorReport error) {
this.markedEntities.get(error.getTrackerType()).add(error.getUid());
}

private <T extends TrackerDto> boolean isMarked(T entity) {
return isContained(this.markedEntities, entity);
}

/**
* Mark parents as non-deletable for potential children. For example an invalid relationship
* (child) referencing a valid tracked entity (parent).
*/
private void markAsNonDeletable(List<TrackerErrorReport> errors) {
errors.forEach(this::mark);
}

/**
* Mark as persistable for later children. For example a tracked entity (parent) referenced by an
* enrollment (child).
Expand Down Expand Up @@ -291,23 +271,6 @@ private <T extends TrackerDto> Predicate<T> parentConditions(
// entities without parents
}

/**
* Add error for valid parents in the payload with invalid child as the reason. So users know why
* a valid entity could not be deleted.
*/
private <T extends TrackerDto> List<TrackerErrorReport> addErrorsForParents(
List<Function<T, TrackerDto>> parents, T entity) {
List<TrackerErrorReport> errors =
parents.stream()
.map(p -> p.apply(entity))
.filter(this::isValid) // remove invalid parents
.filter(this.bundle::exists) // remove parents not in payload
.map(p -> error(TrackerErrorCode.E5001, p, entity))
.collect(Collectors.toList());
this.result.errors.addAll(errors);
return errors;
}

/**
* 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.
Expand Down
175 changes: 2 additions & 173 deletions ...ervice-tracker/src/test/java/org/hisp/dhis/tracker/validation/PersistablesFilterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@
import static org.hisp.dhis.tracker.TrackerType.RELATIONSHIP;
import static org.hisp.dhis.tracker.TrackerType.TRACKED_ENTITY;
import static org.hisp.dhis.tracker.report.TrackerErrorCode.E5000;
import static org.hisp.dhis.tracker.report.TrackerErrorCode.E5001;
import static org.hisp.dhis.tracker.validation.PersistablesFilter.filter;
import static org.hisp.dhis.tracker.validation.ValidationCode.E5000;
import static org.hisp.dhis.tracker.validation.hooks.AssertTrackerValidationReport.assertHasError;
import static org.hisp.dhis.utils.Assertions.assertIsEmpty;
import static org.junit.jupiter.api.Assertions.assertAll;
Expand Down Expand Up @@ -374,7 +374,7 @@ void testDeleteValidEntitiesCanBeDeleted() {
.enrollment("Ok4Fe5moc3N")
.event("Ox1qBWsnVwE")
.event("jNyGqnwryNi")
.relationship("Te3IC6TpnBB", trackedEntity("xK7H53f4Hc2"), trackedEntity("QxGbKYwChDM"))
.relationship("Te3IC6TpnBB", null, null)
.build();

PersistablesFilter.Result persistable =
Expand All @@ -390,177 +390,6 @@ void testDeleteValidEntitiesCanBeDeleted() {
() -> assertIsEmpty(persistable.getErrors()));
}

@Test
void testDeleteInvalidTrackedEntityCannotBeDeletedButItsChildrenCan() {
Setup setup =
new Setup.Builder()
.trackedEntity("xK7H53f4Hc2")
.isNotValid()
.enrollment("t1zaUjKgT3p")
.event("Qck4PQ7TMun")
.event("Ox1qBWsnVwE")
.build();

PersistablesFilter.Result persistable =
filter(setup.bundle, setup.invalidEntities, TrackerImportStrategy.DELETE);

assertAll(
() -> assertIsEmpty(persistable.get(TrackedEntity.class)),
() -> assertContainsOnly(persistable, Enrollment.class, "t1zaUjKgT3p"),
() -> assertContainsOnly(persistable, Event.class, "Qck4PQ7TMun", "Ox1qBWsnVwE"),
() -> assertIsEmpty(persistable.getErrors()));
}

@Test
void testDeleteOnlyReportErrorsIfItAddsNewInformation() {
// If entities are found to be invalid during the validation an error for the entity will
// already be in the
// validation report. Only add errors if it would not be clear why an entity cannot be
// persisted.

Setup setup =
new Setup.Builder()
.trackedEntity("xK7H53f4Hc2")
.isNotValid()
.enrollment("t1zaUjKgT3p")
.isNotValid()
.relationship("Te3IC6TpnBB", trackedEntity("xK7H53f4Hc2"), enrollment("t1zaUjKgT3p"))
.isNotValid()
.build();

PersistablesFilter.Result persistable =
filter(setup.bundle, setup.invalidEntities, TrackerImportStrategy.DELETE);

assertAll(
() -> assertIsEmpty(persistable.get(TrackedEntity.class)),
() -> assertIsEmpty(persistable.get(Enrollment.class)),
() -> assertIsEmpty(persistable.get(Relationship.class)),
() -> assertIsEmpty(persistable.getErrors()));
}

@Test
void testDeleteInvalidRelationshipPreventsDeletionOfTrackedEntityAndEvent() {
Setup setup =
new Setup.Builder()
.trackedEntity("xK7H53f4Hc2")
.enrollment("t1zaUjKgT3p")
.event("Qck4PQ7TMun")
.trackedEntity("QxGbKYwChDM")
.enrollment("Ok4Fe5moc3N")
.relationship("Te3IC6TpnBB", trackedEntity("xK7H53f4Hc2"), event("Qck4PQ7TMun"))
.isNotValid()
.build();

PersistablesFilter.Result persistable =
filter(setup.bundle, setup.invalidEntities, TrackerImportStrategy.DELETE);

assertAll(
() -> assertContainsOnly(persistable, TrackedEntity.class, "QxGbKYwChDM"),
() -> assertContainsOnly(persistable, Enrollment.class, "Ok4Fe5moc3N"),
() -> assertIsEmpty(persistable.get(Event.class)),
() -> assertIsEmpty(persistable.get(Relationship.class)),
() ->
assertError(
persistable,
E5001,
TRACKED_ENTITY,
"xK7H53f4Hc2",
"because \"relationship\" `Te3IC6TpnBB`"),
() ->
assertError(
persistable, E5001, ENROLLMENT, "t1zaUjKgT3p", "because \"event\" `Qck4PQ7TMun`"),
() ->
assertError(
persistable,
E5001,
EVENT,
"Qck4PQ7TMun",
"because \"relationship\" `Te3IC6TpnBB`"));
}

@Test
void testDeleteInvalidRelationshipPreventsDeletionOfEnrollmentAndEvent() {
Setup setup =
new Setup.Builder()
.trackedEntity("xK7H53f4Hc2")
.enrollment("t1zaUjKgT3p")
.event("QxGbKYwChDM")
.event("Ox1qBWsnVwE")
.enrollment("Ok4Fe5moc3N")
.relationship("Te3IC6TpnBB", event("QxGbKYwChDM"), enrollment("t1zaUjKgT3p"))
.isNotValid()
.build();

PersistablesFilter.Result persistable =
filter(setup.bundle, setup.invalidEntities, TrackerImportStrategy.DELETE);

assertAll(
() -> assertIsEmpty(persistable.get(TrackedEntity.class)),
() -> assertContainsOnly(persistable, Enrollment.class, "Ok4Fe5moc3N"),
() -> assertContainsOnly(persistable, Event.class, "Ox1qBWsnVwE"),
() -> assertIsEmpty(persistable.get(Relationship.class)),
() ->
assertError(
persistable,
E5001,
ENROLLMENT,
"t1zaUjKgT3p",
"because \"relationship\" `Te3IC6TpnBB`"),
() ->
assertError(
persistable,
E5001,
EVENT,
"QxGbKYwChDM",
"because \"relationship\" `Te3IC6TpnBB`"));
}

@Test
void testDeleteValidRelationshipWithInvalidFrom() {
Setup setup =
new Setup.Builder()
.trackedEntity("QxGbKYwChDM")
.trackedEntity("xK7H53f4Hc2")
.enrollment("t1zaUjKgT3p")
.isNotValid()
.relationship("Te3IC6TpnBB", enrollment("t1zaUjKgT3p"), trackedEntity("QxGbKYwChDM"))
.build();

PersistablesFilter.Result persistable =
filter(setup.bundle, setup.invalidEntities, TrackerImportStrategy.DELETE);

assertAll(
() -> assertContainsOnly(persistable, TrackedEntity.class, "QxGbKYwChDM"),
() -> assertIsEmpty(persistable.get(Enrollment.class)),
() -> assertContainsOnly(persistable, Relationship.class, "Te3IC6TpnBB"));
}

@Test
void testDeleteValidEntitiesReferencingParentsNotInPayload() {
Setup setup =
new Setup.Builder()
.trackedEntity("xK7H53f4Hc2")
.isInDB()
.isNotInPayload()
.enrollment("t1zaUjKgT3p")
.enrollment("Ok4Fe5moc3N")
.isInDB()
.isNotInPayload()
.event("Ox1qBWsnVwE")
.relationship("Te3IC6TpnBB", trackedEntity("xK7H53f4Hc2"), enrollment("Ok4Fe5moc3N"))
.build();

PersistablesFilter.Result persistable =
filter(setup.bundle, setup.invalidEntities, TrackerImportStrategy.DELETE);

assertAll(
() -> assertIsEmpty(persistable.get(TrackedEntity.class)),
() -> assertContainsOnly(persistable, Enrollment.class, "t1zaUjKgT3p"),
() -> assertContainsOnly(persistable, Event.class, "Ox1qBWsnVwE"),
() -> assertContainsOnly(persistable, Relationship.class, "Te3IC6TpnBB"),
() -> assertIsEmpty(persistable.getErrors()));
}

@RequiredArgsConstructor
private static class Entity<T extends TrackerDto> {
private boolean valid = true;
Expand Down
Loading

0 comments on commit c3e8fd9

Please sign in to comment.