Skip to content

chore: Refactor relationship validations [DHIS2-20158] #23866

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
muilpp merged 3 commits into from
May 13, 2026
Merged

chore: Refactor relationship validations [DHIS2-20158] #23866

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
da08b08
chore: Refactor relationship validations [DHIS2-20158]
muilpp May 11, 2026
d4b625d
chore: Address PR comments [DHIS2-20158]
muilpp May 12, 2026
d9f83ee
chore: Fix error messages typos [DHIS2-20158]
muilpp May 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions dhis-2/dhis-api/src/main/java/org/hisp/dhis/tracker/imports/validation/ValidationCode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,8 @@ public enum ValidationCode {
E1029("Event OrganisationUnit: `{0}` and Program: `{1}`, do not match."),
E1030("Event: `{0}` already exists."),
E1031("Event occurredAt date is missing."),
E1032("Event: `{0}` do not exist."),
E1033("Event: `{0}` Enrollment value is null."),
E1032("Event: `{0}` does not exist."),
E1033("Event: `{0}`, Enrollment value is null."),
E1039("ProgramStage: `{0}` is not repeatable and an Event already exists."),
E1041("Enrollment OrganisationUnit: `{0}` and Program: `{1}` do not match."),
E1043("Event: `{0}` completeness date has expired, not allowed to make changes to this Event."),
Expand Down Expand Up @@ -153,7 +153,7 @@ public enum ValidationCode {
E1316("Event cannot transition from status `{0}` to status `{1}`."),
E1317("Generated by ProgramRule (`{0}`) - Attribute `{1}` is mandatory and cannot be deleted."),
E1318("Status `{0}` is not applicable for single events."),
E1319("Generated by program rule (`{0}`) - Date `{1}` is not valid"),
E1319("Generated by program rule (`{0}`) - Date `{1}` is not valid."),
E1320("Generated by program rule (`{0}`) - Event (`{1}`) was automatically scheduled at `{2}`."),
E1321(
"Generated by program rule (`{0}`) - User (`{1}`) does not have write access to ProgramStage `{2}`, where an event was auto-scheduled using SCHEDULEEVENT rule action."),
Expand All @@ -172,10 +172,11 @@ public enum ValidationCode {
E4012("Could not find `{0}`: `{1}`, linked to relationship."),
E4014("RelationshipType `{0}` constraint requires TrackedEntityType `{1}` but `{2}` was found."),
E4015("Relationship: `{0}` already exists."),
E4016("Relationship: `{0}` do not exist."),
E4016("Relationship: `{0}` does not exist."),
E4017("Relationship: `{0}` is already deleted and cannot be modified."),
E4018("Relationship: `{0}` linking {1}: `{2}` to {3}: `{4}` already exists."),
E4020("User: `{0}` has no write access to relationship: `{1}`."),
E4019("User: `{0}` has no data read access to RelationshipType: `{1}`."),
E4020("User: `{0}` has no data write access to RelationshipType: `{1}`."),
E5000("{0} `{1}` cannot be created because {2} `{3}` referenced by it could not be created."),
E9999("N/A.");

Expand Down
1 change: 0 additions & 1 deletion ...ntegration/src/test/java/org/hisp/dhis/tracker/imports/bundle/RelationshipImportTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,6 @@ void shouldFailWhenUserNotAuthorizedToCreateRelationship() throws IOException {
params, testSetup.fromJson("tracker/relationships.json"));

assertHasError(importReport, ValidationCode.E4020);
assertThat(importReport.getStats().getIgnored(), is(2));
}

@Test
Expand Down
3 changes: 2 additions & 1 deletion ...rc/test/java/org/hisp/dhis/tracker/imports/bundle/ReportSummaryDeleteIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import static org.hisp.dhis.tracker.Assertions.assertHasOnlyErrors;
import static org.hisp.dhis.tracker.Assertions.assertNoErrors;
import static org.hisp.dhis.tracker.imports.validation.Users.USER_10;
import static org.hisp.dhis.tracker.imports.validation.ValidationCode.E1324;
import static org.hisp.dhis.tracker.imports.validation.ValidationCode.E4016;
import static org.hisp.dhis.tracker.imports.validation.ValidationCode.E4020;
import static org.junit.jupiter.api.Assertions.assertEquals;
Expand Down Expand Up @@ -162,7 +163,7 @@ void shouldSuccessfullyDeleteTrackedEntityAndNotAccessibleRelationshipLinkedToIt
ImportReport importReport = trackerImportService.importTracker(params, trackerObjects);

assertDeletedObjects(1, importReport.getPersistenceReport(), TrackerType.TRACKED_ENTITY);
assertHasOnlyErrors(importReport, E4020);
assertHasOnlyErrors(importReport, E4020, E1324);
}

@Test
Expand Down
26 changes: 15 additions & 11 deletions ...va/org/hisp/dhis/tracker/imports/validation/RelationshipSecurityImportValidationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@
import static org.hisp.dhis.tracker.Assertions.assertNoErrors;
import static org.hisp.dhis.tracker.imports.validation.Users.USER_11;
import static org.hisp.dhis.tracker.imports.validation.Users.USER_12;
import static org.hisp.dhis.tracker.imports.validation.ValidationCode.E1001;
import static org.hisp.dhis.tracker.imports.validation.ValidationCode.E1323;
import static org.hisp.dhis.tracker.imports.validation.ValidationCode.E1324;
import static org.hisp.dhis.tracker.imports.validation.ValidationCode.E1325;
import static org.hisp.dhis.tracker.imports.validation.ValidationCode.E4020;

import java.io.IOException;
Expand Down Expand Up @@ -128,12 +132,12 @@ void shouldCreateWhenUserHasAccessToRelationshipTypeAndWriteAccessToBidirectiona

ImportReport importReport = trackerImportService.importTracker(params, trackerObjects);

assertHasOnlyErrors(importReport, E4020);
assertHasOnlyErrors(importReport, E1323);
Comment thread
teleivo marked this conversation as resolved.
}

@Test
void
shouldFailToCreateWhenUserHasAccessToRelationshipTypeAndNoWriteAccessToBidirectionalBidirectionalRelationshipTo()
shouldFailToCreateWhenUserHasAccessToRelationshipTypeAndNoWriteAccessToBidirectionalRelationshipTo()
throws IOException {
TrackerObjects trackerObjects =
testSetup.fromJson("tracker/validations/relationship_siblings.json");
Expand All @@ -146,7 +150,7 @@ void shouldCreateWhenUserHasAccessToRelationshipTypeAndWriteAccessToBidirectiona

ImportReport importReport = trackerImportService.importTracker(params, trackerObjects);

assertHasOnlyErrors(importReport, E4020);
assertHasOnlyErrors(importReport, E1323);
}

@Test
Expand Down Expand Up @@ -200,12 +204,12 @@ void shouldDeleteWhenUserHasAccessToRelationshipTypeAndWriteAccessToBidirectiona
Program program = manager.get(Program.class, "E8o1E9tAppy");
program.getSharing().getUsers().get(USER_11).setAccess("r-r-----");
manager.update(program);
injectSecurityContextUser(userService.getUser(USER_12));
injectSecurityContextUser(userService.getUser(USER_11));

params.setImportStrategy(TrackerImportStrategy.DELETE);
importReport = trackerImportService.importTracker(params, trackerObjects);

assertHasOnlyErrors(importReport, E4020);
assertHasOnlyErrors(importReport, E1323);
}

@Test
Expand All @@ -227,7 +231,7 @@ void shouldDeleteWhenUserHasAccessToRelationshipTypeAndWriteAccessToBidirectiona
params.setImportStrategy(TrackerImportStrategy.DELETE);
importReport = trackerImportService.importTracker(params, trackerObjects);

assertHasOnlyErrors(importReport, E4020);
assertHasOnlyErrors(importReport, E1323);
}

@Test
Expand All @@ -253,7 +257,7 @@ void shouldFailToCreateWhenUserHasNoAccessToRelationshipTypeAndWriteAccessToRela

ImportReport importReport = trackerImportService.importTracker(params, trackerObjects);

assertHasOnlyErrors(importReport, E4020);
assertHasOnlyErrors(importReport, E4020, E1001);
}

@Test
Expand All @@ -269,7 +273,7 @@ void shouldFailToCreateWhenUserHasAccessToRelationshipTypeAndNoWriteAccessToRela

ImportReport importReport = trackerImportService.importTracker(params, trackerObjects);

assertHasOnlyErrors(importReport, E4020);
assertHasOnlyErrors(importReport, E1001, E1323);
}

@Test
Expand All @@ -285,7 +289,7 @@ void shouldFailToCreateWhenUserHasAccessToRelationshipTypeAndNoWriteAccessToRela

ImportReport importReport = trackerImportService.importTracker(params, trackerObjects);

assertHasOnlyErrors(importReport, E4020);
assertHasOnlyErrors(importReport, E1001, E1325, E1324);
}

@Test
Expand Down Expand Up @@ -323,7 +327,7 @@ void shouldFailToDeleteWhenUserHasNoAccessToRelationshipTypeAndWriteAccessToRela
params.setImportStrategy(TrackerImportStrategy.DELETE);
importReport = trackerImportService.importTracker(params, trackerObjects);

assertHasOnlyErrors(importReport, E4020);
assertHasOnlyErrors(importReport, E4020, E1001);
}

@Test
Expand All @@ -343,6 +347,6 @@ void shouldFailToDeleteWhenUserHasAccessToRelationshipTypeAndNoWriteAccessToRela
params.setImportStrategy(TrackerImportStrategy.DELETE);
importReport = trackerImportService.importTracker(params, trackerObjects);

assertHasOnlyErrors(importReport, E4020);
assertHasOnlyErrors(importReport, E1001, E1323);
}
}
101 changes: 45 additions & 56 deletions dhis-2/dhis-tracker/src/main/java/org/hisp/dhis/tracker/acl/DefaultTrackerAccessManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@
import static org.hisp.dhis.tracker.imports.validation.ValidationCode.E1105;
import static org.hisp.dhis.tracker.imports.validation.ValidationCode.E1324;
import static org.hisp.dhis.tracker.imports.validation.ValidationCode.E1325;
import static org.hisp.dhis.tracker.imports.validation.ValidationCode.E4019;
import static org.hisp.dhis.tracker.imports.validation.ValidationCode.E4020;

import java.util.ArrayList;
import java.util.List;
Expand All @@ -60,7 +62,6 @@
import org.hisp.dhis.tracker.model.TrackerEvent;
import org.hisp.dhis.user.UserDetails;
import org.springframework.stereotype.Component;
import org.springframework.transaction.annotation.Transactional;

/**
* @author Morten Olav Hansen <mortenoh@gmail.com>
Expand Down Expand Up @@ -401,15 +402,17 @@ private List<ErrorMessage> validateSingleEventAccess(
}

@Override
public List<String> canRead(@Nonnull UserDetails user, Relationship relationship) {
if (user.isSuper() || relationship == null) {
public List<ErrorMessage> canRead(@Nonnull UserDetails user, @Nonnull Relationship relationship) {
if (user.isSuper()) {
return List.of();
}

RelationshipType relationshipType = relationship.getRelationshipType();
List<String> errors = new ArrayList<>();
List<ErrorMessage> errors = new ArrayList<>();
if (!aclService.canDataRead(user, relationshipType)) {
errors.add("User has no data read access to relationshipType: " + relationshipType.getUid());
errors.add(
new ErrorMessage(
E4019, user.getUid(), List.of(user.getUid(), relationshipType.getUid())));
}

RelationshipItem from = relationship.getFrom();
Expand All @@ -422,85 +425,71 @@ public List<String> canRead(@Nonnull UserDetails user, Relationship relationship
}

@Override
@Transactional(readOnly = true)
public List<String> canCreate(UserDetails user, Relationship relationship) {
if (user.isSuper() || relationship == null) return List.of();
List<String> errors = new ArrayList<>(canWriteRelationship(user, relationship));
public List<ErrorMessage> canCreate(UserDetails user, @Nonnull Relationship relationship) {
if (user.isSuper()) return List.of();
List<ErrorMessage> errors = new ArrayList<>(canWrite(user, relationship));
if (!relationship.getRelationshipType().isBidirectional()) {
Comment thread
enricocolasante marked this conversation as resolved.
errors.addAll(canRead(user, relationship.getTo()));
}
return errors;
}

@Override
@Transactional(readOnly = true)
public List<String> canDelete(UserDetails user, @Nonnull Relationship relationship) {
public List<ErrorMessage> canDelete(UserDetails user, @Nonnull Relationship relationship) {
if (user.isSuper()) return List.of();
return canWriteRelationship(user, relationship);
return canWrite(user, relationship);
}

private List<String> canWriteRelationship(UserDetails user, Relationship relationship) {
RelationshipType type = relationship.getRelationshipType();
List<String> errors = new ArrayList<>();
if (!aclService.canDataWrite(user, type)) {
errors.add("User has no data write access to relationshipType: " + type.getUid());
private List<ErrorMessage> canWrite(UserDetails user, Relationship relationship) {
RelationshipType relationshipType = relationship.getRelationshipType();
List<ErrorMessage> errors = new ArrayList<>();
if (!aclService.canDataWrite(user, relationshipType)) {
errors.add(
new ErrorMessage(
E4020, user.getUid(), List.of(user.getUid(), relationshipType.getUid())));
}
errors.addAll(canWrite(user, relationship.getFrom()));
if (type.isBidirectional()) {
if (relationshipType.isBidirectional()) {
errors.addAll(canWrite(user, relationship.getTo()));
}
return errors;
}

private List<String> canRead(@Nonnull UserDetails user, RelationshipItem item) {
if (item.getTrackedEntity() != null)
return canRead(user, item.getTrackedEntity()).stream()
.map(em -> em.validationCode().getMessage())
.toList();
if (item.getEnrollment() != null)
return canRead(user, item.getEnrollment()).stream()
.map(em -> em.validationCode().getMessage())
.toList();
if (item.getTrackerEvent() != null)
return canRead(user, item.getTrackerEvent()).stream()
.map(em -> em.validationCode().getMessage())
.toList();
if (item.getSingleEvent() != null)
return canRead(user, item.getSingleEvent()).stream()
.map(em -> em.validationCode().getMessage())
.toList();
private List<ErrorMessage> canRead(@Nonnull UserDetails user, RelationshipItem item) {
if (item.getTrackedEntity() != null) return canRead(user, item.getTrackedEntity());
if (item.getEnrollment() != null) return canRead(user, item.getEnrollment());
if (item.getTrackerEvent() != null) return canRead(user, item.getTrackerEvent());
if (item.getSingleEvent() != null) return canRead(user, item.getSingleEvent());
return List.of();
}

private List<String> canWrite(@Nonnull UserDetails user, RelationshipItem item) {
private List<ErrorMessage> canWrite(@Nonnull UserDetails user, RelationshipItem item) {
if (item.getTrackedEntity() != null) {
TrackedEntity te = item.getTrackedEntity();
return canUpdate(user, te, te.getOrganisationUnit()).stream()
.map(em -> em.validationCode().getMessage())
.toList();
return canUpdate(user, te, te.getOrganisationUnit());
}
if (item.getEnrollment() != null) {
Enrollment enrollment = item.getEnrollment();
return canUpdate(
user,
enrollment,
enrollment.getOrganisationUnit(),
enrollment.getAttributeOptionCombo())
.stream()
.map(em -> em.validationCode().getMessage())
.toList();
user, enrollment, enrollment.getOrganisationUnit(), enrollment.getAttributeOptionCombo());
}
if (item.getTrackerEvent() != null) {
TrackerEvent event = item.getTrackerEvent();
return canUpdate(user, event, event.getOrganisationUnit(), event.getAttributeOptionCombo())
.stream()
.map(em -> em.validationCode().getMessage())
.toList();
}
if (item.getSingleEvent() != null)
return canCreate(user, item.getSingleEvent()).stream()
.map(em -> em.validationCode().getMessage())
.toList();
TrackerEvent trackerEvent = item.getTrackerEvent();
return canUpdate(
user,
trackerEvent,
trackerEvent.getOrganisationUnit(),
trackerEvent.getAttributeOptionCombo());
}
if (item.getSingleEvent() != null) {
SingleEvent singleEvent = item.getSingleEvent();
return canUpdate(
user,
singleEvent,
singleEvent.getOrganisationUnit(),
singleEvent.getAttributeOptionCombo());
}

return List.of();
}

Expand Down
Loading
Loading