fix: json patch throw error when remove null property [DHIS2-16299] (…

…39) (#15923) * fix: json patch throw error when remove null property * fix backport conflict
dhis2 · Dec 13, 2023 · 9653394 · 9653394
1 parent bef33eb
commit 9653394
Show file tree

Hide file tree

Showing 5 changed files with 85 additions and 30 deletions.
diff --git a/...is-services/dhis-service-core/src/main/java/org/hisp/dhis/jsonpatch/JsonPatchManager.java b/...is-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> 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.haveProperty(op.getPath().getMatchingProperty())) {
+        throw new JsonPatchException(
+            "Property "
+                + op.getPath().getMatchingProperty()
+                + " does not exist on "
+                + schema.getClass().getSimpleName());
+      }
+    }
+  }
 }
diff --git a/...ons/src/main/java/org/hisp/dhis/commons/jackson/jsonpatch/operations/RemoveOperation.java b/...ons/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-support-commons/src/test/java/org/hisp/dhis/commons/jackson/RemoveOperationTest.java b/...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/...his-support-commons/src/test/java/org/hisp/dhis/commons/jackson/ReplaceOperationTest.java b/...his-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
@@ -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));
+  }
 }