From b636189ed13721b99b82b3cf17bdc85efc2473bc Mon Sep 17 00:00:00 2001 From: Viet Nguyen Date: Wed, 13 Dec 2023 02:56:21 +0700 Subject: [PATCH] fix: json patch throw error when remove null property --- .../hisp/dhis/jsonpatch/JsonPatchManager.java | 16 +++++ .../jsonpatch/operations/RemoveOperation.java | 2 +- .../commons/jackson/RemoveOperationTest.java | 13 ---- .../commons/jackson/ReplaceOperationTest.java | 16 ----- .../dhis/jsonpatch/JsonPatchManagerTest.java | 68 +++++++++++++++++++ 5 files changed, 85 insertions(+), 30 deletions(-) diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/jsonpatch/JsonPatchManager.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/jsonpatch/JsonPatchManager.java index 5bdc3114a20b..3255c7cab4dd 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/jsonpatch/JsonPatchManager.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/jsonpatch/JsonPatchManager.java @@ -39,6 +39,7 @@ import org.hisp.dhis.commons.collection.CollectionUtils; import org.hisp.dhis.commons.jackson.jsonpatch.JsonPatch; import org.hisp.dhis.commons.jackson.jsonpatch.JsonPatchException; +import org.hisp.dhis.commons.jackson.jsonpatch.JsonPatchOperation; import org.hisp.dhis.schema.Property; import org.hisp.dhis.schema.Schema; import org.hisp.dhis.schema.SchemaService; @@ -87,6 +88,8 @@ public T apply(JsonPatch patch, T object) throws JsonPatchException { // correctly made into json nodes. handleCollectionUpdates(object, schema, (ObjectNode) node); + validatePatchPath(patch, schema); + node = patch.apply(node); return (T) jsonToObject( @@ -137,4 +140,17 @@ private BaseIdentifiableObject shallowCopyIdentifiableObject(BaseIdentifiableObj clone.setUid(source.getUid()); return clone; } + + /** Check if all patch paths are valid for the given schema. */ + private void validatePatchPath(JsonPatch patch, Schema schema) throws JsonPatchException { + for (JsonPatchOperation op : patch.getOperations()) { + if (!schema.hasProperty(op.getPath().head().toString())) { + throw new JsonPatchException( + "Property " + + op.getPath().head() + + " does not exist on " + + schema.getClass().getSimpleName()); + } + } + } } diff --git a/dhis-2/dhis-support/dhis-support-commons/src/main/java/org/hisp/dhis/commons/jackson/jsonpatch/operations/RemoveOperation.java b/dhis-2/dhis-support/dhis-support-commons/src/main/java/org/hisp/dhis/commons/jackson/jsonpatch/operations/RemoveOperation.java index 0b685ef5c68d..f5b2dd34a7e6 100644 --- a/dhis-2/dhis-support/dhis-support-commons/src/main/java/org/hisp/dhis/commons/jackson/jsonpatch/operations/RemoveOperation.java +++ b/dhis-2/dhis-support/dhis-support-commons/src/main/java/org/hisp/dhis/commons/jackson/jsonpatch/operations/RemoveOperation.java @@ -55,7 +55,7 @@ public JsonNode apply(JsonNode node) throws JsonPatchException { } if (!nodePathExists(node)) { - throw new JsonPatchException(String.format("Invalid path %s", path)); + return node; } final JsonNode parentNode = node.at(path.head()); diff --git a/dhis-2/dhis-support/dhis-support-commons/src/test/java/org/hisp/dhis/commons/jackson/RemoveOperationTest.java b/dhis-2/dhis-support/dhis-support-commons/src/test/java/org/hisp/dhis/commons/jackson/RemoveOperationTest.java index 7fb09c444d89..53b86a00f489 100644 --- a/dhis-2/dhis-support/dhis-support-commons/src/test/java/org/hisp/dhis/commons/jackson/RemoveOperationTest.java +++ b/dhis-2/dhis-support/dhis-support-commons/src/test/java/org/hisp/dhis/commons/jackson/RemoveOperationTest.java @@ -30,11 +30,9 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import com.fasterxml.jackson.core.JsonProcessingException; -import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.node.ArrayNode; import com.fasterxml.jackson.databind.node.ObjectNode; @@ -52,17 +50,6 @@ class RemoveOperationTest { private final ObjectMapper jsonMapper = JacksonObjectMapperConfig.staticJsonMapper(); - @Test - void testRemoveInvalidKeyShouldThrowException() throws JsonProcessingException { - JsonPatch patch = - jsonMapper.readValue( - "[" + "{\"op\": \"remove\", \"path\": \"/aaa\"}" + "]", JsonPatch.class); - assertNotNull(patch); - JsonNode root = jsonMapper.createObjectNode(); - assertFalse(root.has("aaa")); - assertThrows(JsonPatchException.class, () -> patch.apply(root)); - } - @Test void testRemoveProperty() throws JsonProcessingException, JsonPatchException { JsonPatch patch = diff --git a/dhis-2/dhis-support/dhis-support-commons/src/test/java/org/hisp/dhis/commons/jackson/ReplaceOperationTest.java b/dhis-2/dhis-support/dhis-support-commons/src/test/java/org/hisp/dhis/commons/jackson/ReplaceOperationTest.java index c740e8dbd71d..5299b3918e75 100644 --- a/dhis-2/dhis-support/dhis-support-commons/src/test/java/org/hisp/dhis/commons/jackson/ReplaceOperationTest.java +++ b/dhis-2/dhis-support/dhis-support-commons/src/test/java/org/hisp/dhis/commons/jackson/ReplaceOperationTest.java @@ -29,7 +29,6 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import com.fasterxml.jackson.core.JsonProcessingException; @@ -65,21 +64,6 @@ void testBasicPropertyReplacement() throws JsonProcessingException, JsonPatchExc assertEquals("bbb", root.get("aaa").asText()); } - @Test - void testBasicReplaceNotExistProperty() throws JsonProcessingException { - - JsonPatch patch = - jsonMapper.readValue( - "[" + "{\"op\": \"replace\", \"path\": \"/notExist\", \"value\": \"bbb\"}" + "]", - JsonPatch.class); - assertNotNull(patch); - ObjectNode root = jsonMapper.createObjectNode(); - root.set("aaa", TextNode.valueOf("aaa")); - assertTrue(root.has("aaa")); - assertEquals("aaa", root.get("aaa").asText()); - assertThrows(JsonPatchException.class, () -> patch.apply(root)); - } - @Test void testBasicTextToArray() throws JsonProcessingException, JsonPatchException { JsonPatch patch = diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/jsonpatch/JsonPatchManagerTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/jsonpatch/JsonPatchManagerTest.java index 85ce5f192fa9..e88204ebf3ea 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/jsonpatch/JsonPatchManagerTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/jsonpatch/JsonPatchManagerTest.java @@ -29,6 +29,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; @@ -42,6 +43,7 @@ import org.hisp.dhis.dataelement.DataElementGroup; import org.hisp.dhis.test.integration.IntegrationTestBase; import org.hisp.dhis.user.User; +import org.hisp.dhis.user.UserRole; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; @@ -198,4 +200,70 @@ void testAddAndRemoveSharingUser() throws JsonProcessingException, JsonPatchExce DataElement removedPatchedDE = jsonPatchManager.apply(removePatch, patchedDE); assertEquals(0, removedPatchedDE.getSharing().getUsers().size()); } + + @Test + void testReplaceNullProperty() throws JsonProcessingException, JsonPatchException { + UserRole userRole = createUserRole("test", "ALL"); + userRole.setCode(null); + manager.save(userRole); + JsonPatch patch = + jsonMapper.readValue( + "[" + "{\"op\": \"replace\", \"path\": \"/code\", \"value\": \"updated\"}" + "]", + JsonPatch.class); + assertNotNull(patch); + UserRole patchedUserRole = jsonPatchManager.apply(patch, userRole); + assertEquals("updated", patchedUserRole.getCode()); + } + + @Test + void testReplaceNotExistProperty() throws JsonProcessingException { + UserRole userRole = createUserRole("test", "ALL"); + userRole.setCode(null); + manager.save(userRole); + JsonPatch patch = + jsonMapper.readValue( + "[" + "{\"op\": \"replace\", \"path\": \"/notexist\", \"value\": \"updated\"}" + "]", + JsonPatch.class); + assertNotNull(patch); + assertThrows(JsonPatchException.class, () -> jsonPatchManager.apply(patch, userRole)); + } + + @Test + void testAddNotExistProperty() throws JsonProcessingException { + UserRole userRole = createUserRole("test", "ALL"); + userRole.setCode(null); + manager.save(userRole); + JsonPatch patch = + jsonMapper.readValue( + "[" + "{\"op\": \"add\", \"path\": \"/notexist\", \"value\": \"updated\"}" + "]", + JsonPatch.class); + assertNotNull(patch); + assertThrows(JsonPatchException.class, () -> jsonPatchManager.apply(patch, userRole)); + } + + @Test + void testRemoveNotExistProperty() throws JsonProcessingException { + UserRole userRole = createUserRole("test", "ALL"); + userRole.setCode(null); + manager.save(userRole); + JsonPatch patch = + jsonMapper.readValue( + "[" + "{\"op\": \"remove\", \"path\": \"/notexist\", \"value\": \"updated\"}" + "]", + JsonPatch.class); + assertNotNull(patch); + assertThrows(JsonPatchException.class, () -> jsonPatchManager.apply(patch, userRole)); + } + + @Test + void testRemoveByIdNotExistProperty() throws JsonProcessingException { + UserRole userRole = createUserRole("test", "ALL"); + userRole.setCode(null); + manager.save(userRole); + JsonPatch patch = + jsonMapper.readValue( + "[" + "{\"op\": \"remove\", \"path\": \"/notexist\", \"id\": \"uid\"}" + "]", + JsonPatch.class); + assertNotNull(patch); + assertThrows(JsonPatchException.class, () -> jsonPatchManager.apply(patch, userRole)); + } }