From 5a62d496bfe297d9748e2589e8059e87ade9a964 Mon Sep 17 00:00:00 2001 From: Jan Bernitt Date: Thu, 2 Jan 2025 16:42:00 +0100 Subject: [PATCH 1/2] fix: mandatory attributes cannot have empty value [DHIS2-18365] --- .../dhis/attribute/LazyAttributeValues.java | 15 ++++- .../DefaultMetadataImportService.java | 3 +- .../validation/MandatoryAttributesCheck.java | 57 ++++++++----------- 3 files changed, 39 insertions(+), 36 deletions(-) diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/attribute/LazyAttributeValues.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/attribute/LazyAttributeValues.java index e70b9130254a..a4a4de09fe07 100644 --- a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/attribute/LazyAttributeValues.java +++ b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/attribute/LazyAttributeValues.java @@ -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; @@ -148,7 +149,7 @@ private static TreeMap 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)); } @@ -160,11 +161,21 @@ private static TreeMap 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 from) { keys = new String[from.size()]; values = new String[keys.length]; diff --git a/dhis-2/dhis-services/dhis-service-dxf2/src/main/java/org/hisp/dhis/dxf2/metadata/DefaultMetadataImportService.java b/dhis-2/dhis-services/dhis-service-dxf2/src/main/java/org/hisp/dhis/dxf2/metadata/DefaultMetadataImportService.java index 6f7d7ffaa0b8..5187a64e66e7 100644 --- a/dhis-2/dhis-services/dhis-service-dxf2/src/main/java/org/hisp/dhis/dxf2/metadata/DefaultMetadataImportService.java +++ b/dhis-2/dhis-services/dhis-service-dxf2/src/main/java/org/hisp/dhis/dxf2/metadata/DefaultMetadataImportService.java @@ -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; @@ -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 diff --git a/dhis-2/dhis-services/dhis-service-dxf2/src/main/java/org/hisp/dhis/dxf2/metadata/objectbundle/validation/MandatoryAttributesCheck.java b/dhis-2/dhis-services/dhis-service-dxf2/src/main/java/org/hisp/dhis/dxf2/metadata/objectbundle/validation/MandatoryAttributesCheck.java index ff12ed78b8ba..c82747031c33 100644 --- a/dhis-2/dhis-services/dhis-service-dxf2/src/main/java/org/hisp/dhis/dxf2/metadata/objectbundle/validation/MandatoryAttributesCheck.java +++ b/dhis-2/dhis-services/dhis-service-dxf2/src/main/java/org/hisp/dhis/dxf2/metadata/objectbundle/validation/MandatoryAttributesCheck.java @@ -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; @@ -64,41 +62,34 @@ public void check( List objects = selectObjectsBasedOnImportStrategy(persistedObjects, nonPersistedObjects, importStrategy); - if (objects.isEmpty() || !schema.hasPersistedProperty("attributeValues")) { - return; - } + if (objects.isEmpty() || !schema.hasAttributeValues()) return; - for (T object : objects) { - List errorReports = checkMandatoryAttributes(klass, object, bundle.getPreheat()); + Preheat preheat = bundle.getPreheat(); + Set 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 checkMandatoryAttributes( - Class klass, IdentifiableObject object, Preheat preheat) { - if (object == null - || preheat.isDefault(object) - || !preheat.getMandatoryAttributes().containsKey(klass)) { - return emptyList(); - } - - Set mandatoryAttributes = preheat.getMandatoryAttributes().get(klass); - if (mandatoryAttributes.isEmpty()) { - return emptyList(); - } - Set 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(); } } From 32d96a72ba5b2906653b41b62d9c1d0130ac59d9 Mon Sep 17 00:00:00 2001 From: Jan Bernitt Date: Mon, 6 Jan 2025 11:40:09 +0100 Subject: [PATCH 2/2] test: adds a test for user without mandatory attribute creation [DHIS2-18365] --- .../AbstractCrudControllerTest.java | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/AbstractCrudControllerTest.java b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/AbstractCrudControllerTest.java index 97a596c5e3ed..7b94aae3a774 100644 --- a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/AbstractCrudControllerTest.java +++ b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/AbstractCrudControllerTest.java @@ -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; @@ -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 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();