Skip to content
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

fix: mandatory attributes cannot have empty value [DHIS2-18365] #19586

Merged
merged 2 commits into from
Jan 6, 2025
+87 −36
Merged

fix: mandatory attributes cannot have empty value [DHIS2-18365] #19586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5a62d49
fix: mandatory attributes cannot have empty value [DHIS2-18365]
jbee Jan 2, 2025
32d96a7
test: adds a test for user without mandatory attribute creation [DHIS…
jbee Jan 6, 2025
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
15 changes: 13 additions & 2 deletions dhis-2/dhis-api/src/main/java/org/hisp/dhis/attribute/LazyAttributeValues.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
import org.hisp.dhis.jsontree.JsonArray;
import org.hisp.dhis.jsontree.JsonMixed;
import org.hisp.dhis.jsontree.JsonObject;
import org.hisp.dhis.jsontree.JsonString;
import org.hisp.dhis.jsontree.JsonValue;
import org.intellij.lang.annotations.Language;

Expand Down Expand Up @@ -148,7 +149,7 @@ private static TreeMap<String, String> parseObjectJson(JsonObject map) {
.collect(
Collectors.toMap(
Map.Entry::getKey,
e -> e.getValue().asObject().getString("value").string(),
e -> parseValue(e.getValue().asObject().get("value")),
(a, b) -> a,
TreeMap::new));
}
Expand All @@ -160,11 +161,21 @@ private static TreeMap<String, String> parseArrayJson(JsonArray arr) {
.collect(
Collectors.toMap(
obj -> obj.getObject("attribute").getString("id").string(),
obj -> obj.getString("value").string(),
obj -> parseValue(obj.get("value")),
(a, b) -> a,
TreeMap::new));
}

@Nonnull
private static String parseValue(JsonValue value) {
if (value.isUndefined()) return "";
return switch (value.type()) {
case NULL -> "";
case STRING -> value.as(JsonString.class).string();
default -> value.toJson();
};
}

private void init(TreeMap<String, String> from) {
keys = new String[from.size()];
values = new String[keys.length];
Expand Down
3 changes: 2 additions & 1 deletion ...-service-dxf2/src/main/java/org/hisp/dhis/dxf2/metadata/DefaultMetadataImportService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
import org.hisp.dhis.preheat.PreheatIdentifier;
import org.hisp.dhis.preheat.PreheatMode;
import org.hisp.dhis.scheduling.JobProgress;
import org.hisp.dhis.scheduling.RecordingJobProgress;
import org.hisp.dhis.security.acl.AclService;
import org.hisp.dhis.user.CurrentUserUtil;
import org.hisp.dhis.user.User;
Expand All @@ -80,7 +81,7 @@ public class DefaultMetadataImportService implements MetadataImportService {
@Transactional
public ImportReport importMetadata(
@Nonnull MetadataImportParams params, @Nonnull MetadataObjects objects) {
return importMetadata(params, objects, JobProgress.noop());
return importMetadata(params, objects, RecordingJobProgress.transitory());
}

@Override
Expand Down
57 changes: 24 additions & 33 deletions ...in/java/org/hisp/dhis/dxf2/metadata/objectbundle/validation/MandatoryAttributesCheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,13 @@
*/
package org.hisp.dhis.dxf2.metadata.objectbundle.validation;

import static java.util.Collections.emptyList;
import static org.hisp.dhis.dxf2.metadata.objectbundle.validation.ValidationUtils.createObjectReport;

import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.function.Consumer;
import java.util.stream.Collectors;
import org.hisp.dhis.attribute.Attribute;
import org.hisp.dhis.attribute.AttributeValues;
import org.hisp.dhis.common.IdentifiableObject;
import org.hisp.dhis.dxf2.metadata.objectbundle.ObjectBundle;
import org.hisp.dhis.feedback.ErrorCode;
Expand Down Expand Up @@ -64,41 +62,34 @@ public <T extends IdentifiableObject> void check(
List<T> objects =
selectObjectsBasedOnImportStrategy(persistedObjects, nonPersistedObjects, importStrategy);

if (objects.isEmpty() || !schema.hasPersistedProperty("attributeValues")) {
return;
}
if (objects.isEmpty() || !schema.hasAttributeValues()) return;

for (T object : objects) {
List<ErrorReport> errorReports = checkMandatoryAttributes(klass, object, bundle.getPreheat());
Preheat preheat = bundle.getPreheat();
Set<String> mandatoryAttributes = preheat.getMandatoryAttributes().get(klass);
if (mandatoryAttributes == null || mandatoryAttributes.isEmpty()) return;

if (!errorReports.isEmpty()) {
addReports.accept(createObjectReport(errorReports, object, bundle));
ctx.markForRemoval(object);
for (T object : objects) {
if (object != null && !preheat.isDefault(object)) {
AttributeValues attributeValues = object.getAttributeValues();
mandatoryAttributes.stream()
.filter(attrId -> !isDefined(attrId, attributeValues))
.forEach(
attrId -> {
addReports.accept(
createObjectReport(
new ErrorReport(Attribute.class, ErrorCode.E4011, attrId)
.setMainId(attrId)
.setErrorProperty("value"),
object,
bundle));
ctx.markForRemoval(object);
});
}
}
}

private List<ErrorReport> checkMandatoryAttributes(
Class<? extends IdentifiableObject> klass, IdentifiableObject object, Preheat preheat) {
if (object == null
|| preheat.isDefault(object)
|| !preheat.getMandatoryAttributes().containsKey(klass)) {
return emptyList();
}

Set<String> mandatoryAttributes = preheat.getMandatoryAttributes().get(klass);
if (mandatoryAttributes.isEmpty()) {
return emptyList();
}
Set<String> missingMandatoryAttributes = new HashSet<>(mandatoryAttributes);
missingMandatoryAttributes.removeAll(object.getAttributeValues().keys());

return missingMandatoryAttributes.stream()
.map(
att ->
new ErrorReport(Attribute.class, ErrorCode.E4011, att)
.setMainId(att)
.setErrorProperty("value"))
.collect(Collectors.toList());
private static boolean isDefined(String attrId, AttributeValues values) {
String value = values.get(attrId);
return value != null && !value.isEmpty();
}
}
48 changes: 48 additions & 0 deletions ...est-web-api/src/test/java/org/hisp/dhis/webapi/controller/AbstractCrudControllerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;

import java.util.List;
import java.util.Map;
import java.util.Set;
import org.hisp.dhis.attribute.Attribute;
Expand Down Expand Up @@ -1246,6 +1247,53 @@ void testFilterSharingLt() {
assertEquals(1, programs.get("programs").as(JsonArray.class).size());
}

@Test
void testPostObject_MandatoryAttributeNoValue() {
String attr =
"{'name':'USER', 'valueType':'TRUE_ONLY', 'userAttribute':true, 'mandatory':true}";
String attrId = assertStatus(HttpStatus.CREATED, POST("/attributes", attr));
// language=JSON5
String user =
"""
{
"username": "testMandatoryAttribute",
"password": "-hu@_ka9$P",
"firstName": "testMandatoryAttribute",
"surname": "tester",
"userRoles":[{ "id": "yrB6vc5Ip3r" }],
"attributeValues": [{ "attribute": { "id": "%s" }, "value": "" } ]
}
""";
assertErrorMandatoryAttributeRequired(attrId, POST("/users", user.formatted(attrId)));
}

@Test
void testPostObject_MandatoryAttributeNoAttribute() {
String attr =
"{'name':'USER', 'valueType':'TRUE_ONLY', 'userAttribute':true, 'mandatory':true}";
String attrId = assertStatus(HttpStatus.CREATED, POST("/attributes", attr));
String user =
"""
{
"username": "testMandatoryAttribute",
"password": "-hu@_ka9$P",
"firstName": "testMandatoryAttribute",
"surname": "tester",
"userRoles":[{ "id": "yrB6vc5Ip3r" }]
}
""";
assertErrorMandatoryAttributeRequired(attrId, POST("/users", user));
}

private void assertErrorMandatoryAttributeRequired(String attrId, HttpResponse response) {
JsonError msg = response.content(HttpStatus.CONFLICT).as(JsonError.class);
JsonList<JsonErrorReport> errorReports = msg.getTypeReport().getErrorReports();
assertEquals(1, errorReports.size());
JsonErrorReport error = errorReports.get(0);
assertEquals(ErrorCode.E4011, error.getErrorCode());
assertEquals(List.of(attrId), error.getErrorProperties());
}

private void assertUserGroupHasOnlyUser(String groupId, String userId) {
manager.flush();
manager.clear();
Expand Down
Loading