Ordering of patches with cycles during upload

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

@@ -20,7 +20,7 @@ import com.google.android.fhir.LocalChange
 import com.google.android.fhir.db.Database
 
 /**
- * Generates [Patch]es from [LocalChange]s and output [List<[PatchMapping]>] to keep a mapping of
+ * Generates [Patch]es from [LocalChange]s and output [List<[OrderedMapping]>] to keep a mapping of
  * the [LocalChange]s to their corresponding generated [Patch]
  *
  * INTERNAL ONLY. This interface should NEVER been exposed as an external API because it works
@@ -35,7 +35,7 @@ internal interface PatchGenerator {
    * NOTE: different implementations may have requirements on the size of [localChanges] and output
    * certain numbers of [Patch]es.
    */
-  suspend fun generate(localChanges: List<LocalChange>): List<PatchMapping>
+  suspend fun generate(localChanges: List<LocalChange>): List<OrderedMapping>
 }
 
 internal object PatchGeneratorFactory {
@@ -67,3 +67,20 @@ internal data class PatchMapping(
   val localChanges: List<LocalChange>,
   val generatedPatch: Patch,
 )
+
+/** Structure to help describe the cyclic nature of ordered [PatchMapping]. */
+internal sealed interface OrderedMapping {
+
+  /**
+   * [IndividualMapping] doesn't have a cyclic dependency on other [IndividualMapping] /
+   * [PatchMapping]. It may however have an acyclic dependency on other [IndividualMapping]s /
+   * [PatchMapping]s.
+   */
+  data class IndividualMapping(val patchMapping: PatchMapping) : OrderedMapping
+
+  /**
+   * [CombinedMapping] contains weakly connected [PatchMapping]s where one or more [PatchMapping]s
+   * have a cyclic dependency between each other.
+   */
+  data class CombinedMapping(val patchMappings: List<PatchMapping>) : OrderedMapping
+}

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

@@ -19,8 +19,11 @@ package com.google.android.fhir.sync.upload.patch
 import androidx.annotation.VisibleForTesting
 import com.google.android.fhir.db.Database
 import com.google.android.fhir.db.LocalChangeResourceReference
+import kotlin.math.min
 
-private typealias Node = String
+typealias Node = String
+
+typealias Graph = Map<Node, List<Node>>
 
 /**
  * Orders the [PatchMapping]s to maintain referential integrity during upload.
@@ -53,15 +56,16 @@ internal object PatchOrdering {
    * {D} (UPDATE), then B,C needs to go before the resource A so that referential integrity is
    * retained. Order of D shouldn't matter for the purpose of referential integrity.
    *
-   * @return - A ordered list of the [PatchMapping]s based on the references to other [PatchMapping]
-   *   if the mappings are acyclic
-   * - throws [IllegalStateException] otherwise
+   * @return A ordered list of the [OrderedMapping] containing:
+   * - [OrderedMapping.IndividualMapping] for the [PatchMapping] based on the references to other
+   *   [PatchMapping] if the mappings are acyclic
+   * - [OrderedMapping.CombinedMapping] for [PatchMapping]s based on the references to other
+   *   [PatchMapping]s if the mappings are cyclic.
    */
   suspend fun List<PatchMapping>.orderByReferences(
     database: Database,
-  ): List<PatchMapping> {
+  ): List<OrderedMapping> {
     val resourceIdToPatchMapping = associateBy { patchMapping -> patchMapping.resourceTypeAndId }
-
     /* Get LocalChangeResourceReferences for all the local changes. A single LocalChange may have
     multiple LocalChangeResourceReference, one for each resource reference in the
     LocalChange.payload.*/
@@ -71,7 +75,20 @@ internal object PatchOrdering {
         .groupBy { it.localChangeId }
 
     val adjacencyList = createAdjacencyListForCreateReferences(localChangeIdToResourceReferenceMap)
-    return createTopologicalOrderedList(adjacencyList).mapNotNull { resourceIdToPatchMapping[it] }
+    val stronglyConnected: List<List<Node>> =
+      stronglyConnectedComponents(adjacencyList, resourceIdToPatchMapping.size)
+    val mapping = reduceToSuperNode(stronglyConnected)
+    val updatedGraph = transformGraph(adjacencyList, mapping)
+    val orderedNodes = createTopologicalOrderedList(updatedGraph)
+    val orderedStronglyConnected = superNodeToSSC(orderedNodes, mapping.first)
+    return orderedStronglyConnected.map {
+      val mappings = it.mapNotNull { resourceIdToPatchMapping[it] }
+      if (mappings.size == 1) {
+        OrderedMapping.IndividualMapping(mappings.first())
+      } else {
+        OrderedMapping.CombinedMapping(mappings)
+      }
+    }
   }
 
   /**
@@ -139,4 +156,138 @@ internal object PatchOrdering {
     adjacencyList.keys.forEach { dfs(it) }
     return stack.reversed()
   }
+
+  private fun stronglyConnectedComponents(diGraph: Graph, nodesCount: Int): List<List<Node>> {
+    return sscTarzan(diGraph, nodesCount)
+  }
+
+  private fun reduceToSuperNode(
+    ssc: List<List<Node>>,
+  ): Pair<Map<Node, List<Node>>, Map<Node, Node>> {
+    val superNodesMap = mutableMapOf<Node, List<Node>>()
+    val nodeToSuperNode = mutableMapOf<Node, String>()
+
+    var counter = 0
+    ssc.forEach {
+      superNodesMap[(++counter).toString()] = it
+      it.forEach { nodeToSuperNode[it] = (counter).toString() }
+    }
+
+    return superNodesMap to nodeToSuperNode
+  }
+
+  private fun transformGraph(
+    oldGraph: Graph,
+    nodeMapping: Pair<Map<Node, List<Node>>, Map<Node, Node>>,
+  ): Graph {
+    val newGraph = mutableMapOf<Node, List<Node>>()
+    val (superNodeToNodes, nodeToSuperNode) = nodeMapping
+    // Remove any cyclic dependency from connected components.
+    // reduce them to a single node
+    // replace the ssc nodes with super node
+    oldGraph.forEach {
+      //      println(it.key)
+      val superNode = nodeToSuperNode[it.key]!!
+
+      if (newGraph.containsKey(superNode)) {
+        newGraph[superNode] =
+          (newGraph[superNode]!! + it.value.mapNotNull { nodeToSuperNode[it] })
+            .distinct()
+            .filterNot { superNode == it }
+      } else {
+        newGraph[superNode] = it.value.mapNotNull { nodeToSuperNode[it] }
+      }
+    }
+
+    return newGraph
+  }
+
+  private fun superNodeToSSC(
+    orderedNodes: List<Node>,
+    mapping: Map<Node, List<Node>>,
+  ): List<List<Node>> {
+    return orderedNodes.mapNotNull { mapping[it] }
+  }
+
+  private fun sscTarzan(diGraph: Graph, nodesCount: Int): List<List<Node>> {
+    // Code inspired from
+    // https://github.com/williamfiset/Algorithms/blob/master/src/main/java/com/williamfiset/algorithms/graphtheory/TarjanSccSolverAdjacencyList.java
+    var counter = -1
+    val intToNode = mutableMapOf<Int, String>()
+    val nodeToInt = mutableMapOf<String, Int>()
+
+    val graph = MutableList<MutableList<Int>>(nodesCount) { mutableListOf() }
+
+    diGraph.forEach { (key, value) ->
+      val intForKey =
+        nodeToInt[key]
+          ?: let {
+            intToNode[++counter] = key
+            nodeToInt[key] = counter
+            counter
+          }
+
+      value.forEach { node ->
+        val intForValue =
+          nodeToInt[node]
+            ?: let {
+              intToNode[++counter] = node
+              nodeToInt[node] = counter
+              counter
+            }
+        graph[intForKey].add(intForValue)
+      }
+    }
+
+    var id = 0
+    var sccCount = 0
+    val visited = BooleanArray(graph.size)
+    val ids = IntArray(graph.size) { -1 }
+    val low = IntArray(graph.size)
+    val sccs = IntArray(graph.size)
+    val stack = ArrayDeque<Int>()
+
+    fun dfs(at: Int) {
+      low[at] = id++
+      ids[at] = low[at]
+      stack.addFirst(at)
+      visited[at] = true
+      for (to in graph[at]) {
+        if (ids[to] == -1) {
+          dfs(to)
+        }
+        if (visited[to]) {
+          low[at] = min(low[at], low[to])
+        }
+      }
+
+      // On recursive callback, if we're at the root node (start of SCC)
+      // empty the seen stack until back to root.
+      if (ids[at] == low[at]) {
+        var node: Int = stack.removeFirst()
+        while (true) {
+          visited[node] = false
+          sccs[node] = sccCount
+          if (node == at) break
+          node = stack.removeFirst()
+        }
+        sccCount++
+      }
+    }
+
+    for (i in 0 until nodesCount) {
+      if (ids[i] == -1) {
+        dfs(i)
+      }
+    }
+
+    val lowLinkToConnectedMap = mutableMapOf<Int, MutableList<Int>>()
+    for (i in 0 until nodesCount) {
+      if (!lowLinkToConnectedMap.containsKey(sccs[i])) {
+        lowLinkToConnectedMap[sccs[i]] = mutableListOf()
+      }
+      lowLinkToConnectedMap[sccs[i]]!!.add(i)
+    }
+    return lowLinkToConnectedMap.values.map { it.mapNotNull { intToNode[it] } }
+  }
 }

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

@@ -25,19 +25,21 @@ import com.google.android.fhir.LocalChange
  * maintain an audit trail.
  */
 internal object PerChangePatchGenerator : PatchGenerator {
-  override suspend fun generate(localChanges: List<LocalChange>): List<PatchMapping> =
-    localChanges.map {
-      PatchMapping(
-        localChanges = listOf(it),
-        generatedPatch =
-          Patch(
-            resourceType = it.resourceType,
-            resourceId = it.resourceId,
-            versionId = it.versionId,
-            timestamp = it.timestamp,
-            type = it.type.toPatchType(),
-            payload = it.payload,
-          ),
-      )
-    }
+  override suspend fun generate(localChanges: List<LocalChange>): List<OrderedMapping> =
+    localChanges
+      .map {
+        PatchMapping(
+          localChanges = listOf(it),
+          generatedPatch =
+            Patch(
+              resourceType = it.resourceType,
+              resourceId = it.resourceId,
+              versionId = it.versionId,
+              timestamp = it.timestamp,
+              type = it.type.toPatchType(),
+              payload = it.payload,
+            ),
+        )
+      }
+      .map { OrderedMapping.IndividualMapping(it) }
 }

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

@@ -35,7 +35,7 @@ import com.google.android.fhir.sync.upload.patch.PatchOrdering.orderByReferences
 internal class PerResourcePatchGenerator private constructor(val database: Database) :
   PatchGenerator {
 
-  override suspend fun generate(localChanges: List<LocalChange>): List<PatchMapping> {
+  override suspend fun generate(localChanges: List<LocalChange>): List<OrderedMapping> {
     return generateSquashedChangesMapping(localChanges).orderByReferences(database)
   }
 

engine/src/main/java/com/google/android/fhir/sync/upload/request/TransactionBundleGenerator.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.
@@ -17,6 +17,7 @@
 package com.google.android.fhir.sync.upload.request
 
 import com.google.android.fhir.LocalChange
+import com.google.android.fhir.sync.upload.patch.OrderedMapping
 import com.google.android.fhir.sync.upload.patch.Patch
 import com.google.android.fhir.sync.upload.patch.PatchMapping
 import org.hl7.fhir.r4.model.Bundle
@@ -29,10 +30,52 @@ internal class TransactionBundleGenerator(
     (patch: Patch, useETagForUpload: Boolean) -> BundleEntryComponentGenerator,
 ) : UploadRequestGenerator {
 
+  /**
+   * In order to accommodate cyclic dependencies between [PatchMapping]s and maintain referential
+   * integrity on the server, the [PatchMapping]s in a [OrderedMapping.CombinedMapping] are all put
+   * in a single [BundleUploadRequestMapping]. Based on the [generatedBundleSize], the remaining
+   * space of the [BundleUploadRequestMapping] maybe filled with other
+   * [OrderedMapping.CombinedMapping] or [OrderedMapping.IndividualMapping] mappings.
+   *
+   * In case a single [OrderedMapping.CombinedMapping] has more [PatchMapping]s than the
+   * [generatedBundleSize], [generatedBundleSize] will be ignored so that all of the dependent
+   * mappings in [OrderedMapping.CombinedMapping] can be sent in a single [Bundle].
+   *
+   * **NOTE: The order of the [OrderedMapping.IndividualMapping] is always maintained and the order
+   * of [OrderedMapping.CombinedMapping] doesn't matter since it contain all the required
+   * [PatchMapping] inside the same [Bundle].**
+   */
   override fun generateUploadRequests(
-    mappedPatches: List<PatchMapping>,
+    mappedPatches: List<OrderedMapping>,
   ): List<BundleUploadRequestMapping> {
-    return mappedPatches.chunked(generatedBundleSize).map { patchList ->
+    val mappingsPerBundle = mutableListOf<List<PatchMapping>>()
+
+    var bundle = mutableListOf<PatchMapping>()
+    mappedPatches.forEach {
+      when (it) {
+        is OrderedMapping.IndividualMapping -> {
+          if (bundle.size < generatedBundleSize) {
+            bundle.add(it.patchMapping)
+          } else {
+            mappingsPerBundle.add(bundle)
+            bundle = mutableListOf(it.patchMapping)
+          }
+        }
+        is OrderedMapping.CombinedMapping -> {
+          if ((bundle.size + it.patchMappings.size) <= generatedBundleSize) {
+            bundle.addAll(it.patchMappings)
+          } else {
+            mappingsPerBundle.add(bundle)
+            bundle = mutableListOf()
+            bundle.addAll(it.patchMappings)
+          }
+        }
+      }
+    }
+
+    if (bundle.isNotEmpty()) mappingsPerBundle.add(bundle)
+
+    return mappingsPerBundle.map { patchList ->
       generateBundleRequest(patchList).let { mappedBundleRequest ->
         BundleUploadRequestMapping(
           splitLocalChanges = mappedBundleRequest.first,

engine/src/main/java/com/google/android/fhir/sync/upload/request/UploadRequestGenerator.kt

@@ -17,21 +17,22 @@
 package com.google.android.fhir.sync.upload.request
 
 import com.google.android.fhir.LocalChange
+import com.google.android.fhir.sync.upload.patch.OrderedMapping
 import com.google.android.fhir.sync.upload.patch.Patch
 import com.google.android.fhir.sync.upload.patch.PatchMapping
 import org.hl7.fhir.r4.model.Bundle
 import org.hl7.fhir.r4.model.codesystems.HttpVerb
 
 /**
  * Generator that generates [UploadRequest]s from the [Patch]es present in the
- * [List<[PatchMapping]>]. Any implementation of this generator is expected to output
+ * [List<[OrderedMapping]>]. Any implementation of this generator is expected to output
  * [List<[UploadRequestMapping]>] which maps [UploadRequest] to the corresponding [LocalChange]s it
  * was generated from.
  */
 internal interface UploadRequestGenerator {
   /** Generates a list of [UploadRequestMapping] from the [PatchMapping]s */
   fun generateUploadRequests(
-    mappedPatches: List<PatchMapping>,
+    mappedPatches: List<OrderedMapping>,
   ): List<UploadRequestMapping>
 }