Handle op:add for same path in patch (#2370)

* handle add for same path in patch

* clean up naming

engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGenerator.kt

@@ -1,5 +1,5 @@
 /*
- * Copyright 2023 Google LLC
+ * Copyright 2023-2024 Google LLC
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -18,11 +18,10 @@ package com.google.android.fhir.sync.upload.patch
 
 import com.fasterxml.jackson.databind.JsonNode
 import com.fasterxml.jackson.databind.ObjectMapper
+import com.github.fge.jackson.JsonLoader
 import com.github.fge.jsonpatch.JsonPatch
 import com.google.android.fhir.LocalChange
 import com.google.android.fhir.LocalChange.Type
-import org.json.JSONArray
-import org.json.JSONObject
 
 /**
  * Generates a [Patch] for all [LocalChange]es made to a single FHIR resource.
@@ -103,24 +102,40 @@ internal object PerResourcePatchGenerator : PatchGenerator {
     return patchJson.apply(resourceJson).toString()
   }
 
-  /** Merge two JSON patch strings by concatenating their elements into a new JSON array. */
+  /**
+   * Merges two JSON patches represented as strings.
+   *
+   * This function combines operations from two JSON patch arrays into a single patch array. The
+   * merging rules are as follows:
+   * - "replace" and "remove" operations from the second patch will overwrite any existing
+   *   operations for the same path.
+   * - "add" operations from the second patch will be added to the list of operations for that path,
+   *   even if operations already exist for that path.
+   * - The function does not handle other operation types like "move", "copy", or "test".
+   */
   private fun mergePatches(firstPatch: String, secondPatch: String): String {
     // TODO: validate patches are RFC 6902 compliant JSON patches
-    val firstMap = JSONArray(firstPatch).patchMergeMap()
-    val secondMap = JSONArray(secondPatch).patchMergeMap()
-    firstMap.putAll(secondMap)
-    return JSONArray(firstMap.values).toString()
-  }
+    val objectMapper = ObjectMapper()
+    val firstPatchNode: JsonNode = JsonLoader.fromString(firstPatch)
+    val secondPatchNode: JsonNode = JsonLoader.fromString(secondPatch)
+    val mergedOperations = hashMapOf<String, MutableList<JsonNode>>()
 
-  /**
-   * Creates a mutable map from operation type (e.g. add/remove) + property path to the entire
-   * operation containing the updated value. Two such maps can be merged using `Map.putAll()` to
-   * yield a minimal set of operations equivalent to individual patches.
-   */
-  private fun JSONArray.patchMergeMap(): MutableMap<Pair<String, String>, JSONObject> {
-    return (0 until this.length())
-      .map { this.optJSONObject(it) }
-      .associateBy { it.optString("op") to it.optString("path") }
-      .toMutableMap()
+    firstPatchNode.forEach { patchNode ->
+      val path = patchNode.get("path").asText()
+      mergedOperations.getOrPut(path) { mutableListOf() }.add(patchNode)
+    }
+
+    secondPatchNode.forEach { patchNode ->
+      val path = patchNode.get("path").asText()
+      val opType = patchNode.get("op").asText()
+      when (opType) {
+        "replace",
+        "remove", -> mergedOperations[path] = mutableListOf(patchNode)
+        "add" -> mergedOperations.getOrPut(path) { mutableListOf() }.add(patchNode)
+      }
+    }
+    val mergedNode = objectMapper.createArrayNode()
+    mergedOperations.values.flatten().forEach(mergedNode::add)
+    return objectMapper.writeValueAsString(mergedNode)
   }
 }

engine/src/test/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGeneratorTest.kt

@@ -1,5 +1,5 @@
 /*
- * Copyright 2023 Google LLC
+ * Copyright 2023-2024 Google LLC
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -314,6 +314,43 @@ class PerResourcePatchGeneratorTest {
     }
   }
 
+  @Test
+  fun `should generate a single update patch with three elements of two adds and one remove`() {
+    val expectedPatch = readJsonArrayFromFile("/update_careplan_patch.json")
+    val updatePatch1 = readJsonArrayFromFile("/update_careplan_patch_1.json")
+    val updatePatch2 = readJsonArrayFromFile("/update_careplan_patch_2.json")
+
+    val updatedLocalChange1 =
+      LocalChange(
+        resourceType = "CarePlan",
+        resourceId = "131b5257-a8b3-435a-8cb3-4cb1296be24a",
+        type = LocalChange.Type.UPDATE,
+        payload = updatePatch1.toString(),
+        timestamp = Instant.now(),
+        token = LocalChangeToken(listOf(1)),
+      )
+
+    val updatedLocalChange2 =
+      LocalChange(
+        resourceType = "CarePlan",
+        resourceId = "131b5257-a8b3-435a-8cb3-4cb1296be24a",
+        type = LocalChange.Type.UPDATE,
+        payload = updatePatch2.toString(),
+        timestamp = Instant.now(),
+        token = LocalChangeToken(listOf(1)),
+      )
+
+    val patches =
+      PerResourcePatchGenerator.generate(listOf(updatedLocalChange1, updatedLocalChange2))
+
+    with(patches.single().generatedPatch) {
+      assertThat(type).isEqualTo(Patch.Type.UPDATE)
+      assertThat(resourceId).isEqualTo("131b5257-a8b3-435a-8cb3-4cb1296be24a")
+      assertThat(resourceType).isEqualTo("CarePlan")
+      assertJsonArrayEqualsIgnoringOrder(JSONArray(payload), expectedPatch)
+    }
+  }
+
   @Test
   fun `should generate a single delete patch if the resource is updated and deleted`() {
     val remoteMeta =

engine/test-data/update_careplan_patch.json

@@ -0,0 +1,77 @@
+[
+  {
+    "op": "replace",
+    "path": "/activity/6/detail/status",
+    "value": "in-progress"
+  },
+  {
+    "op": "add",
+    "path": "/activity/-",
+    "value": {
+      "outcomeReference": [
+        {
+          "reference": "Task/4f26da79-4571-4442-911a-c3a707e3e662",
+          "display": "Viral Load Collection"
+        }
+      ],
+      "detail": {
+        "kind": "Task",
+        "code": {
+          "coding": [
+            {
+              "system": "https://d-tree.org/",
+              "code": "art-client-viral-load-collection"
+            }
+          ]
+        },
+        "status": "in-progress",
+        "scheduledPeriod": {
+          "start": "2023-11-15T11:45:40+02:00",
+          "end": "2023-12-14T11:10:34.227+03:00"
+        },
+        "performer": [
+          {
+            "reference": "Practitioner/311015",
+            "display": "John Doe"
+          }
+        ],
+        "description": "Viral Load Collection"
+      }
+    }
+  },
+  {
+    "op": "add",
+    "path": "/activity/-",
+    "value": {
+      "outcomeReference": [
+        {
+          "reference": "Task/6f6e6672-ef2d-4e57-9854-0a04cd0fb001",
+          "display": "Index Case Testing"
+        }
+      ],
+      "detail": {
+        "kind": "Task",
+        "code": {
+          "coding": [
+            {
+              "system": "https://d-tree.org/",
+              "code": "art-client-index-case-testing"
+            }
+          ]
+        },
+        "status": "in-progress",
+        "scheduledPeriod": {
+          "start": "2023-11-15T11:45:40+02:00",
+          "end": "2023-12-14T11:10:34.227+03:00"
+        },
+        "performer": [
+          {
+            "reference": "Practitioner/311015",
+            "display": "John Doe"
+          }
+        ],
+        "description": "Index Case Testing"
+      }
+    }
+  }
+]

engine/test-data/update_careplan_patch_1.json

@@ -0,0 +1,42 @@
+[
+  {
+    "op": "replace",
+    "path": "/activity/6/detail/status",
+    "value": "in-progress"
+  },
+  {
+    "op": "add",
+    "path": "/activity/-",
+    "value": {
+      "outcomeReference": [
+        {
+          "reference": "Task/4f26da79-4571-4442-911a-c3a707e3e662",
+          "display": "Viral Load Collection"
+        }
+      ],
+      "detail": {
+        "kind": "Task",
+        "code": {
+          "coding": [
+            {
+              "system": "https://d-tree.org/",
+              "code": "art-client-viral-load-collection"
+            }
+          ]
+        },
+        "status": "in-progress",
+        "scheduledPeriod": {
+          "start": "2023-11-15T11:45:40+02:00",
+          "end": "2023-12-14T11:10:34.227+03:00"
+        },
+        "performer": [
+          {
+            "reference": "Practitioner/311015",
+            "display": "John Doe"
+          }
+        ],
+        "description": "Viral Load Collection"
+      }
+    }
+  }
+]

engine/test-data/update_careplan_patch_2.json

@@ -0,0 +1,37 @@
+[
+  {
+    "op": "add",
+    "path": "/activity/-",
+    "value": {
+      "outcomeReference": [
+        {
+          "reference": "Task/6f6e6672-ef2d-4e57-9854-0a04cd0fb001",
+          "display": "Index Case Testing"
+        }
+      ],
+      "detail": {
+        "kind": "Task",
+        "code": {
+          "coding": [
+            {
+              "system": "https://d-tree.org/",
+              "code": "art-client-index-case-testing"
+            }
+          ]
+        },
+        "status": "in-progress",
+        "scheduledPeriod": {
+          "start": "2023-11-15T11:45:40+02:00",
+          "end": "2023-12-14T11:10:34.227+03:00"
+        },
+        "performer": [
+          {
+            "reference": "Practitioner/311015",
+            "display": "John Doe"
+          }
+        ],
+        "description": "Index Case Testing"
+      }
+    }
+  }
+]