From 05873b1f9edcf380de07b5f0edcce21aa6dc02aa Mon Sep 17 00:00:00 2001 From: Aditya Khajuria Date: Tue, 23 Apr 2024 12:15:40 +0530 Subject: [PATCH 01/16] Ordering of patches with cyclic during upload --- .../google/android/fhir/FhirEngineProvider.kt | 1 + .../fhir/sync/upload/patch/PatchGenerator.kt | 8 +- .../fhir/sync/upload/patch/PatchOrdering.kt | 116 +++++++++++++++++- .../upload/patch/PerChangePatchGenerator.kt | 32 ++--- .../upload/patch/PerResourcePatchGenerator.kt | 2 +- .../request/TransactionBundleGenerator.kt | 46 ++++++- .../upload/request/UploadRequestGenerator.kt | 3 +- .../upload/request/UrlRequestGenerator.kt | 34 +++-- .../sync/upload/patch/PatchOrderingTest.kt | 4 +- .../patch/PerResourcePatchGeneratorTest.kt | 38 ++++-- .../upload/request/IndividualGeneratorTest.kt | 14 ++- .../request/TransactionBundleGeneratorTest.kt | 43 ++++--- 12 files changed, 274 insertions(+), 67 deletions(-) diff --git a/engine/src/main/java/com/google/android/fhir/FhirEngineProvider.kt b/engine/src/main/java/com/google/android/fhir/FhirEngineProvider.kt index 076c0e27ed..b5fba00f7a 100644 --- a/engine/src/main/java/com/google/android/fhir/FhirEngineProvider.kt +++ b/engine/src/main/java/com/google/android/fhir/FhirEngineProvider.kt @@ -188,6 +188,7 @@ data class ServerConfiguration( val networkConfiguration: NetworkConfiguration = NetworkConfiguration(), val authenticator: HttpAuthenticator? = null, val httpLogger: HttpLogger = HttpLogger.NONE, + val referentialIntegrityDuringUpload: Boolean = true, ) /** diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchGenerator.kt index 1a52da87c8..7dff15df76 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchGenerator.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchGenerator.kt @@ -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): List + suspend fun generate(localChanges: List): List } internal object PatchGeneratorFactory { @@ -67,3 +67,9 @@ internal data class PatchMapping( val localChanges: List, val generatedPatch: Patch, ) + +internal sealed interface Mapping { + data class IndividualMapping(val patchMapping: PatchMapping) : Mapping + + data class CombinedMapping(val patchMappings: List) : Mapping +} diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt index c76fb46751..13446b59b3 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt @@ -20,7 +20,9 @@ import androidx.annotation.VisibleForTesting import com.google.android.fhir.db.Database import com.google.android.fhir.db.LocalChangeResourceReference -private typealias Node = String +typealias Node = String + +typealias Graph = Map> /** * Orders the [PatchMapping]s to maintain referential integrity during upload. @@ -59,7 +61,7 @@ internal object PatchOrdering { */ suspend fun List.orderByReferences( database: Database, - ): List { + ): List { val resourceIdToPatchMapping = associateBy { patchMapping -> patchMapping.resourceTypeAndId } /* Get LocalChangeResourceReferences for all the local changes. A single LocalChange may have @@ -71,7 +73,37 @@ internal object PatchOrdering { .groupBy { it.localChangeId } val adjacencyList = createAdjacencyListForCreateReferences(localChangeIdToResourceReferenceMap) - return createTopologicalOrderedList(adjacencyList).mapNotNull { resourceIdToPatchMapping[it] } + + val weakConnectedComponents = connectedComponents(adjacencyList) + + val componentsWithCycles = mutableListOf() + val componentsWithOutCycles = mutableListOf() + weakConnectedComponents.forEach { + try { + checkCycle(it) + componentsWithOutCycles.add(it) + } catch (e: IllegalStateException) { + e.printStackTrace() + componentsWithCycles.add(it) + } + } + + val componentsNodesWithCycles = + if (componentsWithCycles.isNotEmpty()) { + componentsWithCycles.map { graph: Graph -> + (graph.keys + graph.values.flatten().toSet()).toList() + } + } else { + emptyList() + } + + val combinedGraph = combineComponentsWithoutCycle(componentsWithOutCycles) + return componentsNodesWithCycles + .map { it.mapNotNull { resourceIdToPatchMapping[it] } } + .map { Mapping.CombinedMapping(it) } + + createTopologicalOrderedList(combinedGraph) + .mapNotNull { resourceIdToPatchMapping[it] } + .map { Mapping.IndividualMapping(it) } } /** @@ -139,4 +171,82 @@ internal object PatchOrdering { adjacencyList.keys.forEach { dfs(it) } return stack.reversed() } + + fun connectedComponents(diGraph: Graph): List { + // convert digraph to a graph + val graph = mutableMapOf>() // Map> + diGraph.forEach { entry -> + if (!graph.containsKey(entry.key)) graph[entry.key] = mutableListOf() + entry.value.forEach { + graph[entry.key]!!.add(it) + graph.getOrPut(it) { mutableListOf() }.add(entry.key) + // add nodes + } + } + + // find connected components + + val connectedComponents = findConnectedComponents(graph) + + // convert connected components back to digraph + val connectedDigraphs = mutableListOf() + connectedComponents.forEach { + val connectedDigraph = mutableMapOf>() + it.forEach { + if (diGraph.containsKey(it)) { + connectedDigraph[it] = diGraph[it]!! as MutableList + } + } + if (connectedDigraph.isNotEmpty()) { + connectedDigraphs.add(connectedDigraph) + } + } + + return connectedDigraphs + } + + private fun findConnectedComponents(graph: Graph): List> { + val connectedComponents = mutableListOf>() + val visited = mutableSetOf() + val connectedNodes = mutableListOf() + + fun dfs(node: Node) { + if (visited.add(node)) { + connectedNodes.add(node) + graph[node]?.forEach { dfs(it) } + } + } + + graph.keys.forEach { + connectedNodes.clear() + dfs(it) + if (connectedNodes.isNotEmpty()) { + connectedComponents.add(ArrayList(connectedNodes)) + } + } + return connectedComponents + } + + fun checkCycle(diGraph: Graph) { + val stack = ArrayDeque() + val visited = mutableSetOf() + val currentPath = mutableSetOf() + + fun dfs(key: String) { + check(currentPath.add(key)) { "Detected a cycle." } + if (visited.add(key)) { + diGraph[key]?.forEach { dfs(it) } + stack.addFirst(key) + } + currentPath.remove(key) + } + + diGraph.keys.forEach { dfs(it) } + } + + fun combineComponentsWithoutCycle(graphs: List): Graph { + val combinedGraph = mutableMapOf>() + graphs.forEach { combinedGraph.putAll(it) } + return combinedGraph + } } diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerChangePatchGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerChangePatchGenerator.kt index 8c787f644e..8a59e81b9f 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerChangePatchGenerator.kt +++ b/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): List = - 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): List = + 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 { Mapping.IndividualMapping(it) } } diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGenerator.kt index 0eaa944008..1ab34d5288 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGenerator.kt +++ b/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): List { + override suspend fun generate(localChanges: List): List { return generateSquashedChangesMapping(localChanges).orderByReferences(database) } diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/request/TransactionBundleGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/request/TransactionBundleGenerator.kt index fa4fc5bc31..e5e3075cab 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/request/TransactionBundleGenerator.kt +++ b/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.Mapping 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 @@ -30,9 +31,48 @@ internal class TransactionBundleGenerator( ) : UploadRequestGenerator { override fun generateUploadRequests( - mappedPatches: List, + mappedPatches: List, ): List { - return mappedPatches.chunked(generatedBundleSize).map { patchList -> + // Add the combined resource, then if there is space in the Bundle left, add some individual + // resource if possible + val combined = + mappedPatches.filterIsInstance().sortedByDescending { + it.patchMappings.size + } + val individual = mappedPatches.filterIsInstance() + + val mappingsPerBundle = mutableListOf>() + + val cItr = combined.toMutableList().listIterator() + val iItr = individual.listIterator() + + while (cItr.hasNext()) { + val bundle = mutableListOf() + bundle.addAll(cItr.next().patchMappings) + cItr.remove() + + while (cItr.hasNext()) { + val next = cItr.next() + if (bundle.size + next.patchMappings.size <= generatedBundleSize) { + bundle.addAll(next.patchMappings) + cItr.remove() // get rid from itr as we have added it into the + } + } + + while (iItr.hasNext() && bundle.size < generatedBundleSize) { + bundle.add(iItr.next().patchMapping) + } + mappingsPerBundle.add(bundle) + + // go back to start from any remaining sub-graph + while (cItr.hasPrevious()) cItr.previous() + } + + individual.subList(iItr.nextIndex(), individual.size).chunked(generatedBundleSize).forEach { + mappingsPerBundle.add(it.map { it.patchMapping }) + } + + return mappingsPerBundle.map { patchList -> generateBundleRequest(patchList).let { mappedBundleRequest -> BundleUploadRequestMapping( splitLocalChanges = mappedBundleRequest.first, diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/request/UploadRequestGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/request/UploadRequestGenerator.kt index 51ae7711ff..fe76192c95 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/request/UploadRequestGenerator.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/request/UploadRequestGenerator.kt @@ -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.Mapping 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 @@ -31,7 +32,7 @@ import org.hl7.fhir.r4.model.codesystems.HttpVerb internal interface UploadRequestGenerator { /** Generates a list of [UploadRequestMapping] from the [PatchMapping]s */ fun generateUploadRequests( - mappedPatches: List, + mappedPatches: List, ): List } diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/request/UrlRequestGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/request/UrlRequestGenerator.kt index 5fbb174e8a..4fbd792a64 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/request/UrlRequestGenerator.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/request/UrlRequestGenerator.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. @@ -19,8 +19,8 @@ package com.google.android.fhir.sync.upload.request import ca.uhn.fhir.context.FhirContext import ca.uhn.fhir.context.FhirVersionEnum import com.google.android.fhir.ContentTypes +import com.google.android.fhir.sync.upload.patch.Mapping import com.google.android.fhir.sync.upload.patch.Patch -import com.google.android.fhir.sync.upload.patch.PatchMapping import org.hl7.fhir.r4.model.Binary import org.hl7.fhir.r4.model.Resource import org.hl7.fhir.r4.model.codesystems.HttpVerb @@ -31,14 +31,30 @@ internal class UrlRequestGenerator( ) : UploadRequestGenerator { override fun generateUploadRequests( - mappedPatches: List, + mappedPatches: List, ): List = - mappedPatches.map { - UrlUploadRequestMapping( - localChanges = it.localChanges, - generatedRequest = getUrlRequestForPatch(it.generatedPatch), - ) - } + mappedPatches + .map { + when (it) { + is Mapping.IndividualMapping -> { + listOf( + UrlUploadRequestMapping( + localChanges = it.patchMapping.localChanges, + generatedRequest = getUrlRequestForPatch(it.patchMapping.generatedPatch), + ), + ) + } + is Mapping.CombinedMapping -> { + it.patchMappings.map { + UrlUploadRequestMapping( + localChanges = it.localChanges, + generatedRequest = getUrlRequestForPatch(it.generatedPatch), + ) + } + } + } + } + .flatten() companion object Factory { diff --git a/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PatchOrderingTest.kt b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PatchOrderingTest.kt index 44f8889dc8..65f8c23129 100644 --- a/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PatchOrderingTest.kt +++ b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PatchOrderingTest.kt @@ -185,7 +185,9 @@ class PatchOrderingTest { // This order is based on the current implementation of the topological sort in [PatchOrdering], // it's entirely possible to generate different order here which is acceptable/correct, should // we have a different implementation of the topological sort. - assertThat(result.map { it.generatedPatch.resourceId }) + assertThat( + result.map { (it as Mapping.IndividualMapping).patchMapping.generatedPatch.resourceId }, + ) .containsExactly( "patient-1", "patient-2", diff --git a/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGeneratorTest.kt b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGeneratorTest.kt index 4860b66cdc..3276aea509 100644 --- a/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGeneratorTest.kt +++ b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGeneratorTest.kt @@ -69,7 +69,10 @@ class PerResourcePatchGeneratorTest { val patient: Patient = readFromFile(Patient::class.java, "/date_test_patient.json") val insertionLocalChange = createInsertLocalChange(patient) - val patches = patchGenerator.generate(listOf(insertionLocalChange)) + val patches = + patchGenerator.generate(listOf(insertionLocalChange)).map { + (it as Mapping.IndividualMapping).patchMapping + } with(patches.single()) { with(generatedPatch) { @@ -100,7 +103,10 @@ class PerResourcePatchGeneratorTest { val updateLocalChange1 = createUpdateLocalChange(remotePatient, updatedPatient1, 1L) val updatePatch = readJsonArrayFromFile("/update_patch_1.json") - val patches = patchGenerator.generate(listOf(updateLocalChange1)) + val patches = + patchGenerator.generate(listOf(updateLocalChange1)).map { + (it as Mapping.IndividualMapping).patchMapping + } with(patches.single()) { with(generatedPatch) { @@ -129,7 +135,10 @@ class PerResourcePatchGeneratorTest { remotePatient.meta = remoteMeta val deleteLocalChange = createDeleteLocalChange(remotePatient, 3L) - val patches = patchGenerator.generate(listOf(deleteLocalChange)) + val patches = + patchGenerator.generate(listOf(deleteLocalChange)).map { + (it as Mapping.IndividualMapping).patchMapping + } with(patches.single()) { with(generatedPatch) { @@ -155,7 +164,10 @@ class PerResourcePatchGeneratorTest { val updateLocalChange = createUpdateLocalChange(patient, updatedPatient, 1L) val patientString = jsonParser.encodeResourceToString(updatedPatient) - val patches = patchGenerator.generate(listOf(insertionLocalChange, updateLocalChange)) + val patches = + patchGenerator.generate(listOf(insertionLocalChange, updateLocalChange)).map { + (it as Mapping.IndividualMapping).patchMapping + } with(patches.single()) { with(generatedPatch) { @@ -312,7 +324,10 @@ class PerResourcePatchGeneratorTest { val updateLocalChange2 = createUpdateLocalChange(updatedPatient1, updatedPatient2, 2L) val updatePatch = readJsonArrayFromFile("/update_patch_2.json") - val patches = patchGenerator.generate(listOf(updateLocalChange1, updateLocalChange2)) + val patches = + patchGenerator.generate(listOf(updateLocalChange1, updateLocalChange2)).map { + (it as Mapping.IndividualMapping).patchMapping + } with(patches.single()) { with(generatedPatch) { @@ -357,7 +372,10 @@ class PerResourcePatchGeneratorTest { token = LocalChangeToken(listOf(1)), ) - val patches = patchGenerator.generate(listOf(updatedLocalChange1, updatedLocalChange2)) + val patches = + patchGenerator.generate(listOf(updatedLocalChange1, updatedLocalChange2)).map { + (it as Mapping.IndividualMapping).patchMapping + } with(patches.single().generatedPatch) { assertThat(type).isEqualTo(Patch.Type.UPDATE) @@ -385,9 +403,11 @@ class PerResourcePatchGeneratorTest { val deleteLocalChange = createDeleteLocalChange(updatedPatient2, 3L) val patches = - patchGenerator.generate( - listOf(updateLocalChange1, updateLocalChange2, deleteLocalChange), - ) + patchGenerator + .generate( + listOf(updateLocalChange1, updateLocalChange2, deleteLocalChange), + ) + .map { (it as Mapping.IndividualMapping).patchMapping } with(patches.single()) { with(generatedPatch) { diff --git a/engine/src/test/java/com/google/android/fhir/sync/upload/request/IndividualGeneratorTest.kt b/engine/src/test/java/com/google/android/fhir/sync/upload/request/IndividualGeneratorTest.kt index 0e02b6261a..d506ceefd8 100644 --- a/engine/src/test/java/com/google/android/fhir/sync/upload/request/IndividualGeneratorTest.kt +++ b/engine/src/test/java/com/google/android/fhir/sync/upload/request/IndividualGeneratorTest.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. @@ -16,6 +16,7 @@ package com.google.android.fhir.sync.upload.request +import com.google.android.fhir.sync.upload.patch.Mapping import com.google.android.fhir.sync.upload.patch.PatchMapping import com.google.android.fhir.sync.upload.request.RequestGeneratorTestUtils.deleteLocalChange import com.google.android.fhir.sync.upload.request.RequestGeneratorTestUtils.insertionLocalChange @@ -49,7 +50,7 @@ class IndividualGeneratorTest { ) val requests = generator.generateUploadRequests( - listOf(patchOutput), + listOf(Mapping.IndividualMapping(patchOutput)), ) with(requests.single()) { @@ -72,7 +73,7 @@ class IndividualGeneratorTest { ) val requests = generator.generateUploadRequests( - listOf(patchOutput), + listOf(Mapping.IndividualMapping(patchOutput)), ) with(requests.single()) { @@ -92,7 +93,7 @@ class IndividualGeneratorTest { generatedPatch = updateLocalChange.toPatch(), ) val generator = UrlRequestGenerator.Factory.getDefault() - val requests = generator.generateUploadRequests(listOf(patchOutput)) + val requests = generator.generateUploadRequests(listOf(Mapping.IndividualMapping(patchOutput))) with(requests.single()) { with(generatedRequest) { assertThat(requests.size).isEqualTo(1) @@ -115,7 +116,7 @@ class IndividualGeneratorTest { generatedPatch = deleteLocalChange.toPatch(), ) val generator = UrlRequestGenerator.Factory.getDefault() - val requests = generator.generateUploadRequests(listOf(patchOutput)) + val requests = generator.generateUploadRequests(listOf(Mapping.IndividualMapping(patchOutput))) with(requests.single()) { with(generatedRequest) { assertThat(httpVerb).isEqualTo(HttpVerb.DELETE) @@ -132,7 +133,8 @@ class IndividualGeneratorTest { PatchMapping(listOf(it), it.toPatch()) } val generator = UrlRequestGenerator.Factory.getDefault() - val result = generator.generateUploadRequests(patchOutputList) + val result = + generator.generateUploadRequests(patchOutputList.map { Mapping.IndividualMapping(it) }) assertThat(result).hasSize(3) assertThat(result.map { it.generatedRequest.httpVerb }) .containsExactly(HttpVerb.PUT, HttpVerb.PATCH, HttpVerb.DELETE) diff --git a/engine/src/test/java/com/google/android/fhir/sync/upload/request/TransactionBundleGeneratorTest.kt b/engine/src/test/java/com/google/android/fhir/sync/upload/request/TransactionBundleGeneratorTest.kt index 6319ac0eee..d9809e447f 100644 --- a/engine/src/test/java/com/google/android/fhir/sync/upload/request/TransactionBundleGeneratorTest.kt +++ b/engine/src/test/java/com/google/android/fhir/sync/upload/request/TransactionBundleGeneratorTest.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. @@ -20,6 +20,7 @@ import ca.uhn.fhir.context.FhirContext import ca.uhn.fhir.context.FhirVersionEnum import com.google.android.fhir.LocalChange import com.google.android.fhir.LocalChangeToken +import com.google.android.fhir.sync.upload.patch.Mapping import com.google.android.fhir.sync.upload.patch.PatchMapping import com.google.android.fhir.sync.upload.request.RequestGeneratorTestUtils.deleteLocalChange import com.google.android.fhir.sync.upload.request.RequestGeneratorTestUtils.insertionLocalChange @@ -50,9 +51,10 @@ class TransactionBundleGeneratorTest { fun `generateUploadRequests() should return single Transaction Bundle with 3 entries`() = runBlocking { val patches = - listOf(insertionLocalChange, updateLocalChange, deleteLocalChange).map { - PatchMapping(listOf(it), it.toPatch()) - } + listOf(insertionLocalChange, updateLocalChange, deleteLocalChange) + .map { PatchMapping(listOf(it), it.toPatch()) } + .map { Mapping.IndividualMapping(it) } + val generator = TransactionBundleGenerator.Factory.getDefault() val result = generator.generateUploadRequests(patches) @@ -74,9 +76,9 @@ class TransactionBundleGeneratorTest { runBlocking { val jsonParser = FhirContext.forCached(FhirVersionEnum.R4).newJsonParser() val patches = - listOf(insertionLocalChange, updateLocalChange, deleteLocalChange).map { - PatchMapping(listOf(it), it.toPatch()) - } + listOf(insertionLocalChange, updateLocalChange, deleteLocalChange) + .map { PatchMapping(listOf(it), it.toPatch()) } + .map { Mapping.IndividualMapping(it) } val generator = TransactionBundleGenerator.Factory.getGenerator( Bundle.HTTPVerb.PUT, @@ -119,11 +121,12 @@ class TransactionBundleGeneratorTest { ) val patches = listOf( - PatchMapping( - localChanges = listOf(localChange), - generatedPatch = localChange.toPatch(), - ), - ) + PatchMapping( + localChanges = listOf(localChange), + generatedPatch = localChange.toPatch(), + ), + ) + .map { Mapping.IndividualMapping(it) } val generator = TransactionBundleGenerator.Factory.getDefault(useETagForUpload = false) val result = generator.generateUploadRequests(patches) @@ -147,11 +150,12 @@ class TransactionBundleGeneratorTest { ) val patches = listOf( - PatchMapping( - localChanges = listOf(localChange), - generatedPatch = localChange.toPatch(), - ), - ) + PatchMapping( + localChanges = listOf(localChange), + generatedPatch = localChange.toPatch(), + ), + ) + .map { Mapping.IndividualMapping(it) } val generator = TransactionBundleGenerator.Factory.getDefault(useETagForUpload = true) val result = generator.generateUploadRequests(patches) @@ -185,7 +189,10 @@ class TransactionBundleGeneratorTest { token = LocalChangeToken(listOf(2L)), ), ) - val patches = localChanges.map { PatchMapping(listOf(it), it.toPatch()) } + val patches = + localChanges + .map { PatchMapping(listOf(it), it.toPatch()) } + .map { Mapping.IndividualMapping(it) } val generator = TransactionBundleGenerator.Factory.getDefault(useETagForUpload = true) val result = generator.generateUploadRequests(patches) From c9e01eafd34032cdcf6c9e7d65bc2b22b6aff46a Mon Sep 17 00:00:00 2001 From: Aditya Khajuria Date: Tue, 23 Apr 2024 13:00:48 +0530 Subject: [PATCH 02/16] Ignored failing test --- .../google/android/fhir/sync/upload/patch/PatchOrderingTest.kt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PatchOrderingTest.kt b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PatchOrderingTest.kt index 65f8c23129..3172336164 100644 --- a/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PatchOrderingTest.kt +++ b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PatchOrderingTest.kt @@ -39,6 +39,7 @@ import org.hl7.fhir.r4.model.Reference import org.hl7.fhir.r4.model.RelatedPerson import org.hl7.fhir.r4.model.Resource import org.junit.Before +import org.junit.Ignore import org.junit.Test import org.junit.runner.RunWith import org.mockito.Mock @@ -203,6 +204,7 @@ class PatchOrderingTest { .inOrder() } + @Ignore("Invalid test with latest changes.") @Test fun `generate with cyclic references should throw exception`() = runTest { val localChanges = LinkedList() From 9354c15fca4f46da786345c6f015cf5a0144d4e5 Mon Sep 17 00:00:00 2001 From: Aditya Khajuria Date: Thu, 16 May 2024 20:34:29 +0530 Subject: [PATCH 03/16] Refactored and added tests --- .../fhir/sync/upload/patch/PatchGenerator.kt | 21 ++- .../fhir/sync/upload/patch/PatchOrdering.kt | 17 ++- .../upload/patch/PerChangePatchGenerator.kt | 4 +- .../upload/patch/PerResourcePatchGenerator.kt | 2 +- .../request/TransactionBundleGenerator.kt | 25 +++- .../upload/request/UploadRequestGenerator.kt | 6 +- .../upload/request/UrlRequestGenerator.kt | 15 +- .../sync/upload/patch/PatchOrderingTest.kt | 138 +++++++++++------- .../patch/PerResourcePatchGeneratorTest.kt | 14 +- .../upload/request/IndividualGeneratorTest.kt | 14 +- .../request/TransactionBundleGeneratorTest.kt | 15 +- 11 files changed, 172 insertions(+), 99 deletions(-) diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchGenerator.kt index 7dff15df76..7b0b106fd7 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchGenerator.kt +++ b/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): List + suspend fun generate(localChanges: List): List } internal object PatchGeneratorFactory { @@ -68,8 +68,19 @@ internal data class PatchMapping( val generatedPatch: Patch, ) -internal sealed interface Mapping { - data class IndividualMapping(val patchMapping: PatchMapping) : Mapping +/** Structure to help describe the cyclic nature of ordered [PatchMapping]. */ +internal sealed interface OrderedMapping { - data class CombinedMapping(val patchMappings: List) : Mapping + /** + * [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) : OrderedMapping } diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt index 13446b59b3..78eaee0384 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt @@ -19,6 +19,7 @@ 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 timber.log.Timber typealias Node = String @@ -55,13 +56,15 @@ 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.orderByReferences( database: Database, - ): List { + ): List { val resourceIdToPatchMapping = associateBy { patchMapping -> patchMapping.resourceTypeAndId } /* Get LocalChangeResourceReferences for all the local changes. A single LocalChange may have @@ -83,7 +86,7 @@ internal object PatchOrdering { checkCycle(it) componentsWithOutCycles.add(it) } catch (e: IllegalStateException) { - e.printStackTrace() + Timber.i(e) componentsWithCycles.add(it) } } @@ -100,10 +103,10 @@ internal object PatchOrdering { val combinedGraph = combineComponentsWithoutCycle(componentsWithOutCycles) return componentsNodesWithCycles .map { it.mapNotNull { resourceIdToPatchMapping[it] } } - .map { Mapping.CombinedMapping(it) } + + .map { OrderedMapping.CombinedMapping(it) } + createTopologicalOrderedList(combinedGraph) .mapNotNull { resourceIdToPatchMapping[it] } - .map { Mapping.IndividualMapping(it) } + .map { OrderedMapping.IndividualMapping(it) } } /** diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerChangePatchGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerChangePatchGenerator.kt index 8a59e81b9f..35a542233d 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerChangePatchGenerator.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerChangePatchGenerator.kt @@ -25,7 +25,7 @@ import com.google.android.fhir.LocalChange * maintain an audit trail. */ internal object PerChangePatchGenerator : PatchGenerator { - override suspend fun generate(localChanges: List): List = + override suspend fun generate(localChanges: List): List = localChanges .map { PatchMapping( @@ -41,5 +41,5 @@ internal object PerChangePatchGenerator : PatchGenerator { ), ) } - .map { Mapping.IndividualMapping(it) } + .map { OrderedMapping.IndividualMapping(it) } } diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGenerator.kt index 1ab34d5288..439122d0ef 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGenerator.kt +++ b/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): List { + override suspend fun generate(localChanges: List): List { return generateSquashedChangesMapping(localChanges).orderByReferences(database) } diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/request/TransactionBundleGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/request/TransactionBundleGenerator.kt index e5e3075cab..697fe36f69 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/request/TransactionBundleGenerator.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/request/TransactionBundleGenerator.kt @@ -17,7 +17,7 @@ package com.google.android.fhir.sync.upload.request import com.google.android.fhir.LocalChange -import com.google.android.fhir.sync.upload.patch.Mapping +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 @@ -30,16 +30,31 @@ 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, + mappedPatches: List, ): List { // Add the combined resource, then if there is space in the Bundle left, add some individual // resource if possible val combined = - mappedPatches.filterIsInstance().sortedByDescending { + mappedPatches.filterIsInstance().sortedByDescending { it.patchMappings.size } - val individual = mappedPatches.filterIsInstance() + val individual = mappedPatches.filterIsInstance() val mappingsPerBundle = mutableListOf>() @@ -55,7 +70,7 @@ internal class TransactionBundleGenerator( val next = cItr.next() if (bundle.size + next.patchMappings.size <= generatedBundleSize) { bundle.addAll(next.patchMappings) - cItr.remove() // get rid from itr as we have added it into the + cItr.remove() // get rid from itr as we have added it into the bundle } } diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/request/UploadRequestGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/request/UploadRequestGenerator.kt index fe76192c95..ae8fad5cf8 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/request/UploadRequestGenerator.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/request/UploadRequestGenerator.kt @@ -17,7 +17,7 @@ package com.google.android.fhir.sync.upload.request import com.google.android.fhir.LocalChange -import com.google.android.fhir.sync.upload.patch.Mapping +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 @@ -25,14 +25,14 @@ 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, + mappedPatches: List, ): List } diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/request/UrlRequestGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/request/UrlRequestGenerator.kt index 4fbd792a64..5e62f11715 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/request/UrlRequestGenerator.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/request/UrlRequestGenerator.kt @@ -19,8 +19,9 @@ package com.google.android.fhir.sync.upload.request import ca.uhn.fhir.context.FhirContext import ca.uhn.fhir.context.FhirVersionEnum import com.google.android.fhir.ContentTypes -import com.google.android.fhir.sync.upload.patch.Mapping +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.Binary import org.hl7.fhir.r4.model.Resource import org.hl7.fhir.r4.model.codesystems.HttpVerb @@ -30,13 +31,19 @@ internal class UrlRequestGenerator( private val getUrlRequestForPatch: (patch: Patch) -> UrlUploadRequest, ) : UploadRequestGenerator { + /** + * Since a [UrlUploadRequest] can only handle a single resource request, the + * [OrderedMapping.CombinedMapping.patchMappings] are flattened and handled as + * [OrderedMapping.IndividualMapping] mapping to generate [UrlUploadRequestMapping] for each + * [PatchMapping]. + */ override fun generateUploadRequests( - mappedPatches: List, + mappedPatches: List, ): List = mappedPatches .map { when (it) { - is Mapping.IndividualMapping -> { + is OrderedMapping.IndividualMapping -> { listOf( UrlUploadRequestMapping( localChanges = it.patchMapping.localChanges, @@ -44,7 +51,7 @@ internal class UrlRequestGenerator( ), ) } - is Mapping.CombinedMapping -> { + is OrderedMapping.CombinedMapping -> { it.patchMappings.map { UrlUploadRequestMapping( localChanges = it.localChanges, diff --git a/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PatchOrderingTest.kt b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PatchOrderingTest.kt index 3172336164..58a2580971 100644 --- a/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PatchOrderingTest.kt +++ b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PatchOrderingTest.kt @@ -28,8 +28,6 @@ import com.google.android.fhir.versionId import com.google.common.truth.Truth.assertThat import java.time.Instant import java.util.LinkedList -import kotlin.random.Random -import kotlin.test.assertFailsWith import kotlinx.coroutines.test.runTest import org.hl7.fhir.r4.model.Encounter import org.hl7.fhir.r4.model.Group @@ -39,7 +37,6 @@ import org.hl7.fhir.r4.model.Reference import org.hl7.fhir.r4.model.RelatedPerson import org.hl7.fhir.r4.model.Resource import org.junit.Before -import org.junit.Ignore import org.junit.Test import org.junit.runner.RunWith import org.mockito.Mock @@ -187,7 +184,9 @@ class PatchOrderingTest { // it's entirely possible to generate different order here which is acceptable/correct, should // we have a different implementation of the topological sort. assertThat( - result.map { (it as Mapping.IndividualMapping).patchMapping.generatedPatch.resourceId }, + result.map { + (it as OrderedMapping.IndividualMapping).patchMapping.generatedPatch.resourceId + }, ) .containsExactly( "patient-1", @@ -204,55 +203,54 @@ class PatchOrderingTest { .inOrder() } - @Ignore("Invalid test with latest changes.") @Test - fun `generate with cyclic references should throw exception`() = runTest { - val localChanges = LinkedList() - val localChangeResourceReferences = mutableListOf() + fun `generate with cyclic and acyclic references should generate both Individual and Combined mappings`() = + runTest { + val helper = LocalChangeHelper() - Patient() - .apply { - id = "patient-1" - addLink( - Patient.PatientLinkComponent().apply { other = Reference("RelatedPerson/related-1") }, - ) - } - .also { - localChanges.add(createInsertLocalChange(it, Random.nextLong())) - localChangeResourceReferences.add( - LocalChangeResourceReference( - localChanges.last().token.ids.first(), - "RelatedPerson/related-1", - "Patient.other", - ), - ) - } - - RelatedPerson() - .apply { - id = "related-1" - patient = Reference("Patient/patient-1") - } - .also { - localChanges.add(createInsertLocalChange(it)) - localChangeResourceReferences.add( - LocalChangeResourceReference( - localChanges.last().token.ids.first(), - "Patient/patient-1", - "RelatedPerson.patient", - ), - ) - } + // Patient and RelatedPerson have cyclic dependency + helper.createPatient("patient-1", 1, "related-1") + helper.createRelatedPerson("related-1", 2, "Patient/patient-1") - whenever(database.getLocalChangeResourceReferences(any())) - .thenReturn(localChangeResourceReferences) + // Patient, RelatedPerson have cyclic dependency. Observation, Encounter and Patient have + // acyclic dependency and order doesn't matter since they all go in same bundle. + helper.createPatient("patient-2", 3, "related-2") + helper.createRelatedPerson("related-2", 4, "Patient/patient-2") + helper.createObservation("observation-1", 5, "Patient/patient-2", "Encounter/encounter-1") + helper.createEncounter("encounter-1", 6, "Patient/patient-2") - val errorMessage = - assertFailsWith { patchGenerator.generate(localChanges) } - .localizedMessage + // observation , encounter and Patient have acyclic dependency with each other, hence order is + // important here. + helper.createObservation("observation-2", 7, "Patient/patient-3", "Encounter/encounter-2") + helper.createEncounter("encounter-2", 8, "Patient/patient-3") + helper.createPatient("patient-3", 9) - assertThat(errorMessage).isEqualTo("Detected a cycle.") - } + whenever(database.getLocalChangeResourceReferences(any())) + .thenReturn(helper.localChangeResourceReferences) + + val result = patchGenerator.generate(helper.localChanges) + + assertThat(result.filterIsInstance()).hasSize(2) + assertThat(result.filterIsInstance()).hasSize(3) + + assertThat( + result.filterIsInstance().map { + it.patchMappings.map { it.generatedPatch.resourceId } + }, + ) + .containsExactly( + listOf("patient-1", "related-1"), + listOf("patient-2", "related-2", "observation-1", "encounter-1"), + ) + + assertThat( + result.filterIsInstance().map { + it.patchMapping.generatedPatch.resourceId + }, + ) + .containsExactly("patient-3", "encounter-2", "observation-2") + .inOrder() + } companion object { @@ -323,10 +321,29 @@ class PatchOrderingTest { fun createPatient( id: String, changeId: Long, + relatedPersonId: String? = null, ) = Patient() - .apply { this.id = id } - .also { localChanges.add(createInsertLocalChange(it, changeId)) } + .apply { + this.id = id + relatedPersonId?.let { + addLink( + Patient.PatientLinkComponent().apply { other = Reference("RelatedPerson/$it") }, + ) + } + } + .also { + localChanges.add(createInsertLocalChange(it, changeId)) + relatedPersonId?.let { + localChangeResourceReferences.add( + LocalChangeResourceReference( + localChanges.last().token.ids.first(), + "RelatedPerson/$relatedPersonId", + "Patient.other", + ), + ) + } + } fun updatePatient( patient: Patient, @@ -387,5 +404,26 @@ class PatchOrderingTest { ), ) } + + fun createRelatedPerson( + id: String, + changeId: Long, + patient: String, + ) = + RelatedPerson() + .apply { + this.id = id + this.patient = Reference(patient) + } + .also { + localChanges.add(createInsertLocalChange(it, changeId)) + localChangeResourceReferences.add( + LocalChangeResourceReference( + localChanges.last().token.ids.first(), + patient, + "RelatedPerson.patient", + ), + ) + } } } diff --git a/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGeneratorTest.kt b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGeneratorTest.kt index 3276aea509..2ec32b86c8 100644 --- a/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGeneratorTest.kt +++ b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGeneratorTest.kt @@ -71,7 +71,7 @@ class PerResourcePatchGeneratorTest { val patches = patchGenerator.generate(listOf(insertionLocalChange)).map { - (it as Mapping.IndividualMapping).patchMapping + (it as OrderedMapping.IndividualMapping).patchMapping } with(patches.single()) { @@ -105,7 +105,7 @@ class PerResourcePatchGeneratorTest { val patches = patchGenerator.generate(listOf(updateLocalChange1)).map { - (it as Mapping.IndividualMapping).patchMapping + (it as OrderedMapping.IndividualMapping).patchMapping } with(patches.single()) { @@ -137,7 +137,7 @@ class PerResourcePatchGeneratorTest { val patches = patchGenerator.generate(listOf(deleteLocalChange)).map { - (it as Mapping.IndividualMapping).patchMapping + (it as OrderedMapping.IndividualMapping).patchMapping } with(patches.single()) { @@ -166,7 +166,7 @@ class PerResourcePatchGeneratorTest { val patches = patchGenerator.generate(listOf(insertionLocalChange, updateLocalChange)).map { - (it as Mapping.IndividualMapping).patchMapping + (it as OrderedMapping.IndividualMapping).patchMapping } with(patches.single()) { @@ -326,7 +326,7 @@ class PerResourcePatchGeneratorTest { val patches = patchGenerator.generate(listOf(updateLocalChange1, updateLocalChange2)).map { - (it as Mapping.IndividualMapping).patchMapping + (it as OrderedMapping.IndividualMapping).patchMapping } with(patches.single()) { @@ -374,7 +374,7 @@ class PerResourcePatchGeneratorTest { val patches = patchGenerator.generate(listOf(updatedLocalChange1, updatedLocalChange2)).map { - (it as Mapping.IndividualMapping).patchMapping + (it as OrderedMapping.IndividualMapping).patchMapping } with(patches.single().generatedPatch) { @@ -407,7 +407,7 @@ class PerResourcePatchGeneratorTest { .generate( listOf(updateLocalChange1, updateLocalChange2, deleteLocalChange), ) - .map { (it as Mapping.IndividualMapping).patchMapping } + .map { (it as OrderedMapping.IndividualMapping).patchMapping } with(patches.single()) { with(generatedPatch) { diff --git a/engine/src/test/java/com/google/android/fhir/sync/upload/request/IndividualGeneratorTest.kt b/engine/src/test/java/com/google/android/fhir/sync/upload/request/IndividualGeneratorTest.kt index d506ceefd8..3f830183ef 100644 --- a/engine/src/test/java/com/google/android/fhir/sync/upload/request/IndividualGeneratorTest.kt +++ b/engine/src/test/java/com/google/android/fhir/sync/upload/request/IndividualGeneratorTest.kt @@ -16,7 +16,7 @@ package com.google.android.fhir.sync.upload.request -import com.google.android.fhir.sync.upload.patch.Mapping +import com.google.android.fhir.sync.upload.patch.OrderedMapping import com.google.android.fhir.sync.upload.patch.PatchMapping import com.google.android.fhir.sync.upload.request.RequestGeneratorTestUtils.deleteLocalChange import com.google.android.fhir.sync.upload.request.RequestGeneratorTestUtils.insertionLocalChange @@ -50,7 +50,7 @@ class IndividualGeneratorTest { ) val requests = generator.generateUploadRequests( - listOf(Mapping.IndividualMapping(patchOutput)), + listOf(OrderedMapping.IndividualMapping(patchOutput)), ) with(requests.single()) { @@ -73,7 +73,7 @@ class IndividualGeneratorTest { ) val requests = generator.generateUploadRequests( - listOf(Mapping.IndividualMapping(patchOutput)), + listOf(OrderedMapping.IndividualMapping(patchOutput)), ) with(requests.single()) { @@ -93,7 +93,8 @@ class IndividualGeneratorTest { generatedPatch = updateLocalChange.toPatch(), ) val generator = UrlRequestGenerator.Factory.getDefault() - val requests = generator.generateUploadRequests(listOf(Mapping.IndividualMapping(patchOutput))) + val requests = + generator.generateUploadRequests(listOf(OrderedMapping.IndividualMapping(patchOutput))) with(requests.single()) { with(generatedRequest) { assertThat(requests.size).isEqualTo(1) @@ -116,7 +117,8 @@ class IndividualGeneratorTest { generatedPatch = deleteLocalChange.toPatch(), ) val generator = UrlRequestGenerator.Factory.getDefault() - val requests = generator.generateUploadRequests(listOf(Mapping.IndividualMapping(patchOutput))) + val requests = + generator.generateUploadRequests(listOf(OrderedMapping.IndividualMapping(patchOutput))) with(requests.single()) { with(generatedRequest) { assertThat(httpVerb).isEqualTo(HttpVerb.DELETE) @@ -134,7 +136,7 @@ class IndividualGeneratorTest { } val generator = UrlRequestGenerator.Factory.getDefault() val result = - generator.generateUploadRequests(patchOutputList.map { Mapping.IndividualMapping(it) }) + generator.generateUploadRequests(patchOutputList.map { OrderedMapping.IndividualMapping(it) }) assertThat(result).hasSize(3) assertThat(result.map { it.generatedRequest.httpVerb }) .containsExactly(HttpVerb.PUT, HttpVerb.PATCH, HttpVerb.DELETE) diff --git a/engine/src/test/java/com/google/android/fhir/sync/upload/request/TransactionBundleGeneratorTest.kt b/engine/src/test/java/com/google/android/fhir/sync/upload/request/TransactionBundleGeneratorTest.kt index d9809e447f..826964b9f6 100644 --- a/engine/src/test/java/com/google/android/fhir/sync/upload/request/TransactionBundleGeneratorTest.kt +++ b/engine/src/test/java/com/google/android/fhir/sync/upload/request/TransactionBundleGeneratorTest.kt @@ -16,11 +16,9 @@ package com.google.android.fhir.sync.upload.request -import ca.uhn.fhir.context.FhirContext -import ca.uhn.fhir.context.FhirVersionEnum import com.google.android.fhir.LocalChange import com.google.android.fhir.LocalChangeToken -import com.google.android.fhir.sync.upload.patch.Mapping +import com.google.android.fhir.sync.upload.patch.OrderedMapping import com.google.android.fhir.sync.upload.patch.PatchMapping import com.google.android.fhir.sync.upload.request.RequestGeneratorTestUtils.deleteLocalChange import com.google.android.fhir.sync.upload.request.RequestGeneratorTestUtils.insertionLocalChange @@ -53,7 +51,7 @@ class TransactionBundleGeneratorTest { val patches = listOf(insertionLocalChange, updateLocalChange, deleteLocalChange) .map { PatchMapping(listOf(it), it.toPatch()) } - .map { Mapping.IndividualMapping(it) } + .map { OrderedMapping.IndividualMapping(it) } val generator = TransactionBundleGenerator.Factory.getDefault() val result = generator.generateUploadRequests(patches) @@ -74,11 +72,10 @@ class TransactionBundleGeneratorTest { @Test fun `generateUploadRequests() should return 3 Transaction Bundle with single entry each`() = runBlocking { - val jsonParser = FhirContext.forCached(FhirVersionEnum.R4).newJsonParser() val patches = listOf(insertionLocalChange, updateLocalChange, deleteLocalChange) .map { PatchMapping(listOf(it), it.toPatch()) } - .map { Mapping.IndividualMapping(it) } + .map { OrderedMapping.IndividualMapping(it) } val generator = TransactionBundleGenerator.Factory.getGenerator( Bundle.HTTPVerb.PUT, @@ -126,7 +123,7 @@ class TransactionBundleGeneratorTest { generatedPatch = localChange.toPatch(), ), ) - .map { Mapping.IndividualMapping(it) } + .map { OrderedMapping.IndividualMapping(it) } val generator = TransactionBundleGenerator.Factory.getDefault(useETagForUpload = false) val result = generator.generateUploadRequests(patches) @@ -155,7 +152,7 @@ class TransactionBundleGeneratorTest { generatedPatch = localChange.toPatch(), ), ) - .map { Mapping.IndividualMapping(it) } + .map { OrderedMapping.IndividualMapping(it) } val generator = TransactionBundleGenerator.Factory.getDefault(useETagForUpload = true) val result = generator.generateUploadRequests(patches) @@ -192,7 +189,7 @@ class TransactionBundleGeneratorTest { val patches = localChanges .map { PatchMapping(listOf(it), it.toPatch()) } - .map { Mapping.IndividualMapping(it) } + .map { OrderedMapping.IndividualMapping(it) } val generator = TransactionBundleGenerator.Factory.getDefault(useETagForUpload = true) val result = generator.generateUploadRequests(patches) From 6575faef33005d3543d04dc1642cf71be3605c6d Mon Sep 17 00:00:00 2001 From: Aditya Khajuria Date: Thu, 16 May 2024 20:49:31 +0530 Subject: [PATCH 04/16] Refactored --- .../google/android/fhir/FhirEngineProvider.kt | 1 - .../fhir/sync/upload/patch/PatchOrdering.kt | 29 +++++++++---------- 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/engine/src/main/java/com/google/android/fhir/FhirEngineProvider.kt b/engine/src/main/java/com/google/android/fhir/FhirEngineProvider.kt index b5fba00f7a..076c0e27ed 100644 --- a/engine/src/main/java/com/google/android/fhir/FhirEngineProvider.kt +++ b/engine/src/main/java/com/google/android/fhir/FhirEngineProvider.kt @@ -188,7 +188,6 @@ data class ServerConfiguration( val networkConfiguration: NetworkConfiguration = NetworkConfiguration(), val authenticator: HttpAuthenticator? = null, val httpLogger: HttpLogger = HttpLogger.NONE, - val referentialIntegrityDuringUpload: Boolean = true, ) /** diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt index 78eaee0384..011e2771bb 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt @@ -77,34 +77,34 @@ internal object PatchOrdering { val adjacencyList = createAdjacencyListForCreateReferences(localChangeIdToResourceReferenceMap) - val weakConnectedComponents = connectedComponents(adjacencyList) + val weakConnectedComponents = weakConnectedComponents(adjacencyList) - val componentsWithCycles = mutableListOf() - val componentsWithOutCycles = mutableListOf() + val graphsWithCycles = mutableListOf() + val graphsWithOutCycles = mutableListOf() weakConnectedComponents.forEach { try { checkCycle(it) - componentsWithOutCycles.add(it) + graphsWithOutCycles.add(it) } catch (e: IllegalStateException) { Timber.i(e) - componentsWithCycles.add(it) + graphsWithCycles.add(it) } } - val componentsNodesWithCycles = - if (componentsWithCycles.isNotEmpty()) { - componentsWithCycles.map { graph: Graph -> + val nodesPerConnectedGraph: List> = + if (graphsWithCycles.isNotEmpty()) { + graphsWithCycles.map { graph: Graph -> (graph.keys + graph.values.flatten().toSet()).toList() } } else { emptyList() } - val combinedGraph = combineComponentsWithoutCycle(componentsWithOutCycles) - return componentsNodesWithCycles + val combinedGraphWithoutCycles = combineGraphs(graphsWithOutCycles) + return nodesPerConnectedGraph .map { it.mapNotNull { resourceIdToPatchMapping[it] } } .map { OrderedMapping.CombinedMapping(it) } + - createTopologicalOrderedList(combinedGraph) + createTopologicalOrderedList(combinedGraphWithoutCycles) .mapNotNull { resourceIdToPatchMapping[it] } .map { OrderedMapping.IndividualMapping(it) } } @@ -175,7 +175,7 @@ internal object PatchOrdering { return stack.reversed() } - fun connectedComponents(diGraph: Graph): List { + private fun weakConnectedComponents(diGraph: Graph): List { // convert digraph to a graph val graph = mutableMapOf>() // Map> diGraph.forEach { entry -> @@ -188,7 +188,6 @@ internal object PatchOrdering { } // find connected components - val connectedComponents = findConnectedComponents(graph) // convert connected components back to digraph @@ -230,7 +229,7 @@ internal object PatchOrdering { return connectedComponents } - fun checkCycle(diGraph: Graph) { + private fun checkCycle(diGraph: Graph) { val stack = ArrayDeque() val visited = mutableSetOf() val currentPath = mutableSetOf() @@ -247,7 +246,7 @@ internal object PatchOrdering { diGraph.keys.forEach { dfs(it) } } - fun combineComponentsWithoutCycle(graphs: List): Graph { + private fun combineGraphs(graphs: List): Graph { val combinedGraph = mutableMapOf>() graphs.forEach { combinedGraph.putAll(it) } return combinedGraph From 471e5efa551e4e83f7d73a99f74b8d37da2eebc4 Mon Sep 17 00:00:00 2001 From: Aditya Khajuria Date: Thu, 30 May 2024 16:04:29 +0530 Subject: [PATCH 05/16] Updated code to use strongly connected resources --- .../fhir/sync/upload/patch/PatchOrdering.kt | 215 +++++++++++------- .../request/TransactionBundleGenerator.kt | 52 ++--- .../sync/upload/patch/PatchOrderingTest.kt | 12 +- 3 files changed, 156 insertions(+), 123 deletions(-) diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt index 011e2771bb..21b190bb1a 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt @@ -19,7 +19,7 @@ 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 timber.log.Timber +import kotlin.math.min typealias Node = String @@ -66,7 +66,6 @@ internal object PatchOrdering { database: Database, ): List { 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.*/ @@ -76,37 +75,20 @@ internal object PatchOrdering { .groupBy { it.localChangeId } val adjacencyList = createAdjacencyListForCreateReferences(localChangeIdToResourceReferenceMap) - - val weakConnectedComponents = weakConnectedComponents(adjacencyList) - - val graphsWithCycles = mutableListOf() - val graphsWithOutCycles = mutableListOf() - weakConnectedComponents.forEach { - try { - checkCycle(it) - graphsWithOutCycles.add(it) - } catch (e: IllegalStateException) { - Timber.i(e) - graphsWithCycles.add(it) - } - } - - val nodesPerConnectedGraph: List> = - if (graphsWithCycles.isNotEmpty()) { - graphsWithCycles.map { graph: Graph -> - (graph.keys + graph.values.flatten().toSet()).toList() - } + val stronglyConnected: List> = + 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 { - emptyList() + OrderedMapping.CombinedMapping(mappings) } - - val combinedGraphWithoutCycles = combineGraphs(graphsWithOutCycles) - return nodesPerConnectedGraph - .map { it.mapNotNull { resourceIdToPatchMapping[it] } } - .map { OrderedMapping.CombinedMapping(it) } + - createTopologicalOrderedList(combinedGraphWithoutCycles) - .mapNotNull { resourceIdToPatchMapping[it] } - .map { OrderedMapping.IndividualMapping(it) } + } } /** @@ -175,80 +157,137 @@ internal object PatchOrdering { return stack.reversed() } - private fun weakConnectedComponents(diGraph: Graph): List { - // convert digraph to a graph - val graph = mutableMapOf>() // Map> - diGraph.forEach { entry -> - if (!graph.containsKey(entry.key)) graph[entry.key] = mutableListOf() - entry.value.forEach { - graph[entry.key]!!.add(it) - graph.getOrPut(it) { mutableListOf() }.add(entry.key) - // add nodes - } - } + private fun stronglyConnectedComponents(diGraph: Graph, nodesCount: Int): List> { + return sscTarzan(diGraph, nodesCount) + } - // find connected components - val connectedComponents = findConnectedComponents(graph) + private fun reduceToSuperNode( + ssc: List>, + ): Pair>, Map> { + val superNodesMap = mutableMapOf>() + val nodeToSuperNode = mutableMapOf() - // convert connected components back to digraph - val connectedDigraphs = mutableListOf() - connectedComponents.forEach { - val connectedDigraph = mutableMapOf>() - it.forEach { - if (diGraph.containsKey(it)) { - connectedDigraph[it] = diGraph[it]!! as MutableList - } - } - if (connectedDigraph.isNotEmpty()) { - connectedDigraphs.add(connectedDigraph) - } + var counter = 0 + ssc.forEach { + superNodesMap[(++counter).toString()] = it + it.forEach { nodeToSuperNode[it] = (counter).toString() } } - return connectedDigraphs + return superNodesMap to nodeToSuperNode } - private fun findConnectedComponents(graph: Graph): List> { - val connectedComponents = mutableListOf>() - val visited = mutableSetOf() - val connectedNodes = mutableListOf() + private fun transformGraph( + oldGraph: Graph, + nodeMapping: Pair>, Map>, + ): Graph { + val newGraph = mutableMapOf>() + 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]!! - fun dfs(node: Node) { - if (visited.add(node)) { - connectedNodes.add(node) - graph[node]?.forEach { dfs(it) } + 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] } } } - graph.keys.forEach { - connectedNodes.clear() - dfs(it) - if (connectedNodes.isNotEmpty()) { - connectedComponents.add(ArrayList(connectedNodes)) + return newGraph + } + + private fun superNodeToSSC( + orderedNodes: List, + mapping: Map>, + ): List> { + return orderedNodes.mapNotNull { mapping[it] } + } + + private fun sscTarzan(diGraph: Graph, nodesCount: Int): List> { + // 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() + val nodeToInt = mutableMapOf() + + val graph = MutableList>(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) } } - return connectedComponents - } - private fun checkCycle(diGraph: Graph) { - val stack = ArrayDeque() - val visited = mutableSetOf() - val currentPath = mutableSetOf() + 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() - fun dfs(key: String) { - check(currentPath.add(key)) { "Detected a cycle." } - if (visited.add(key)) { - diGraph[key]?.forEach { dfs(it) } - stack.addFirst(key) + 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++ } - currentPath.remove(key) } - diGraph.keys.forEach { dfs(it) } - } + for (i in 0 until nodesCount) { + if (ids[i] == -1) { + dfs(i) + } + } - private fun combineGraphs(graphs: List): Graph { - val combinedGraph = mutableMapOf>() - graphs.forEach { combinedGraph.putAll(it) } - return combinedGraph + val lowLinkToConnectedMap = mutableMapOf>() + 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] } } } } diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/request/TransactionBundleGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/request/TransactionBundleGenerator.kt index 697fe36f69..665626115c 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/request/TransactionBundleGenerator.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/request/TransactionBundleGenerator.kt @@ -48,44 +48,32 @@ internal class TransactionBundleGenerator( override fun generateUploadRequests( mappedPatches: List, ): List { - // Add the combined resource, then if there is space in the Bundle left, add some individual - // resource if possible - val combined = - mappedPatches.filterIsInstance().sortedByDescending { - it.patchMappings.size - } - val individual = mappedPatches.filterIsInstance() - val mappingsPerBundle = mutableListOf>() - val cItr = combined.toMutableList().listIterator() - val iItr = individual.listIterator() - - while (cItr.hasNext()) { - val bundle = mutableListOf() - bundle.addAll(cItr.next().patchMappings) - cItr.remove() - - while (cItr.hasNext()) { - val next = cItr.next() - if (bundle.size + next.patchMappings.size <= generatedBundleSize) { - bundle.addAll(next.patchMappings) - cItr.remove() // get rid from itr as we have added it into the bundle + var bundle = mutableListOf() + 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) + } } } - - while (iItr.hasNext() && bundle.size < generatedBundleSize) { - bundle.add(iItr.next().patchMapping) - } - mappingsPerBundle.add(bundle) - - // go back to start from any remaining sub-graph - while (cItr.hasPrevious()) cItr.previous() } - individual.subList(iItr.nextIndex(), individual.size).chunked(generatedBundleSize).forEach { - mappingsPerBundle.add(it.map { it.patchMapping }) - } + if (bundle.isNotEmpty()) mappingsPerBundle.add(bundle) return mappingsPerBundle.map { patchList -> generateBundleRequest(patchList).let { mappedBundleRequest -> diff --git a/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PatchOrderingTest.kt b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PatchOrderingTest.kt index 58a2580971..d59d010ce6 100644 --- a/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PatchOrderingTest.kt +++ b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PatchOrderingTest.kt @@ -231,7 +231,7 @@ class PatchOrderingTest { val result = patchGenerator.generate(helper.localChanges) assertThat(result.filterIsInstance()).hasSize(2) - assertThat(result.filterIsInstance()).hasSize(3) + assertThat(result.filterIsInstance()).hasSize(5) assertThat( result.filterIsInstance().map { @@ -240,7 +240,7 @@ class PatchOrderingTest { ) .containsExactly( listOf("patient-1", "related-1"), - listOf("patient-2", "related-2", "observation-1", "encounter-1"), + listOf("patient-2", "related-2"), ) assertThat( @@ -248,7 +248,13 @@ class PatchOrderingTest { it.patchMapping.generatedPatch.resourceId }, ) - .containsExactly("patient-3", "encounter-2", "observation-2") + .containsExactly( + "encounter-1", + "observation-1", + "patient-3", + "encounter-2", + "observation-2", + ) .inOrder() } From 500e7e820edadc18d275b31d20b362c526962503 Mon Sep 17 00:00:00 2001 From: Aditya Khajuria Date: Wed, 5 Jun 2024 15:02:32 +0530 Subject: [PATCH 06/16] Review comments: Seperated the code for finding ssc. Added some tets for Bundle generation. --- .../fhir/sync/upload/patch/PatchGenerator.kt | 20 +- .../fhir/sync/upload/patch/PatchOrdering.kt | 168 ++--------------- .../upload/patch/PerChangePatchGenerator.kt | 4 +- .../upload/patch/PerResourcePatchGenerator.kt | 2 +- .../upload/patch/StronglyConnectedPatches.kt | 171 ++++++++++++++++++ .../request/TransactionBundleGenerator.kt | 33 ++-- .../upload/request/UploadRequestGenerator.kt | 6 +- .../upload/request/UrlRequestGenerator.kt | 21 ++- .../sync/upload/patch/PatchOrderingTest.kt | 10 +- .../patch/PerResourcePatchGeneratorTest.kt | 14 +- .../upload/request/IndividualGeneratorTest.kt | 18 +- .../request/TransactionBundleGeneratorTest.kt | 150 ++++++++++++++- 12 files changed, 402 insertions(+), 215 deletions(-) create mode 100644 engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchGenerator.kt index 7b0b106fd7..b9ef9c2a06 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchGenerator.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchGenerator.kt @@ -20,8 +20,8 @@ import com.google.android.fhir.LocalChange import com.google.android.fhir.db.Database /** - * Generates [Patch]es from [LocalChange]s and output [List<[OrderedMapping]>] to keep a mapping of - * the [LocalChange]s to their corresponding generated [Patch] + * Generates [Patch]es from [LocalChange]s and output [List<[PatchMappingGroup]>] 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 * together with other components in the upload package to fulfill a specific upload strategy. @@ -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): List + suspend fun generate(localChanges: List): List } internal object PatchGeneratorFactory { @@ -69,18 +69,18 @@ internal data class PatchMapping( ) /** Structure to help describe the cyclic nature of ordered [PatchMapping]. */ -internal sealed interface OrderedMapping { +internal sealed interface PatchMappingGroup { /** - * [IndividualMapping] doesn't have a cyclic dependency on other [IndividualMapping] / - * [PatchMapping]. It may however have an acyclic dependency on other [IndividualMapping]s / + * [IndividualMappingGroup] doesn't have a cyclic dependency on other [IndividualMappingGroup] / + * [PatchMapping]. It may however have an acyclic dependency on other [IndividualMappingGroup]s / * [PatchMapping]s. */ - data class IndividualMapping(val patchMapping: PatchMapping) : OrderedMapping + data class IndividualMappingGroup(val patchMapping: PatchMapping) : PatchMappingGroup /** - * [CombinedMapping] contains weakly connected [PatchMapping]s where one or more [PatchMapping]s - * have a cyclic dependency between each other. + * [CombinedMappingGroup] contains strongly connected [PatchMapping]s where one or more + * [PatchMapping]s have a cyclic dependency between each other. */ - data class CombinedMapping(val patchMappings: List) : OrderedMapping + data class CombinedMappingGroup(val patchMappings: List) : PatchMappingGroup } diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt index 21b190bb1a..ea473efca0 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt @@ -19,7 +19,6 @@ 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 typealias Node = String @@ -56,15 +55,15 @@ 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 [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 + * @return A ordered list of the [PatchMappingGroup] containing: + * - [PatchMappingGroup.IndividualMappingGroup] for the [PatchMapping] based on the references to + * other [PatchMapping] if the mappings are acyclic + * - [PatchMappingGroup.CombinedMappingGroup] for [PatchMapping]s based on the references to other * [PatchMapping]s if the mappings are cyclic. */ suspend fun List.orderByReferences( database: Database, - ): List { + ): List { 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 @@ -75,20 +74,17 @@ internal object PatchOrdering { .groupBy { it.localChangeId } val adjacencyList = createAdjacencyListForCreateReferences(localChangeIdToResourceReferenceMap) - val stronglyConnected: List> = - 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) + + return StronglyConnectedPatches(adjacencyList, resourceIdToPatchMapping.size) + .sscOrdered { createTopologicalOrderedList(it) } + .map { + val mappings = it.mapNotNull { resourceIdToPatchMapping[it] } + if (mappings.size == 1) { + PatchMappingGroup.IndividualMappingGroup(mappings.first()) + } else { + PatchMappingGroup.CombinedMappingGroup(mappings) + } } - } } /** @@ -156,138 +152,4 @@ internal object PatchOrdering { adjacencyList.keys.forEach { dfs(it) } return stack.reversed() } - - private fun stronglyConnectedComponents(diGraph: Graph, nodesCount: Int): List> { - return sscTarzan(diGraph, nodesCount) - } - - private fun reduceToSuperNode( - ssc: List>, - ): Pair>, Map> { - val superNodesMap = mutableMapOf>() - val nodeToSuperNode = mutableMapOf() - - 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>, - ): Graph { - val newGraph = mutableMapOf>() - 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, - mapping: Map>, - ): List> { - return orderedNodes.mapNotNull { mapping[it] } - } - - private fun sscTarzan(diGraph: Graph, nodesCount: Int): List> { - // 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() - val nodeToInt = mutableMapOf() - - val graph = MutableList>(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() - - 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>() - 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] } } - } } diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerChangePatchGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerChangePatchGenerator.kt index 35a542233d..400c1b291b 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerChangePatchGenerator.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerChangePatchGenerator.kt @@ -25,7 +25,7 @@ import com.google.android.fhir.LocalChange * maintain an audit trail. */ internal object PerChangePatchGenerator : PatchGenerator { - override suspend fun generate(localChanges: List): List = + override suspend fun generate(localChanges: List): List = localChanges .map { PatchMapping( @@ -41,5 +41,5 @@ internal object PerChangePatchGenerator : PatchGenerator { ), ) } - .map { OrderedMapping.IndividualMapping(it) } + .map { PatchMappingGroup.IndividualMappingGroup(it) } } diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGenerator.kt index 439122d0ef..55ad19a300 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGenerator.kt +++ b/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): List { + override suspend fun generate(localChanges: List): List { return generateSquashedChangesMapping(localChanges).orderByReferences(database) } diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt new file mode 100644 index 0000000000..7a4645ce6a --- /dev/null +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt @@ -0,0 +1,171 @@ +/* + * Copyright 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. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.android.fhir.sync.upload.patch + +import kotlin.math.min + +internal class StronglyConnectedPatches(private val diGraph: Graph, private val nodesCount: Int) { + + fun sscOrdered(ordering: (Graph) -> List): List> { + val stronglyConnected = sscTarzan() + val (superNodeToConnectedComponents, connectedComponentNodeToSuperNode) = + reduceToSuperNodes( + stronglyConnected, + ) + val updatedGraph = + transformGraphWithSSCtoGraphWithSuperNodes(diGraph, connectedComponentNodeToSuperNode) + val orderedNodes = ordering(updatedGraph) + return superNodeToSSC(orderedNodes, superNodeToConnectedComponents) + } + + private fun sscTarzan(): List> { + // 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() + val nodeToInt = mutableMapOf() + + val graph = MutableList>(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() + + 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>() + 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] } } + } + + private fun reduceToSuperNodes( + ssc: List>, + ): Pair>, Map> { + val superNodesMap = mutableMapOf>() + val nodeToSuperNode = mutableMapOf() + + var counter = 0 + ssc.forEach { + superNodesMap[(++counter).toString()] = it + it.forEach { nodeToSuperNode[it] = (counter).toString() } + } + + return superNodesMap to nodeToSuperNode + } + + /** + * Converts a graph that has ssc to a graph with super nodes as per the provided [nodeToSuperNode] + * mapping. + * + * **Input** + * * oldGraph `[A : B, C], [B : A, C], [C : A, B, F], [D : E], [E : D], [F : G]` + * * nodeToSuperNode `[A:1, B:1, C:1, D:2, E:2, F:3, G:4]` + * + * **Output** `[1 : 3], [2], [3: 4]` + */ + private fun transformGraphWithSSCtoGraphWithSuperNodes( + oldGraph: Graph, + nodeToSuperNode: Map, + ): Graph { + val newGraph = mutableMapOf>() + // Remove any cyclic dependency from connected components in the graph. + // Reduce all the ssc nodes from graph into to a single node in graph. + // Replace the ssc nodes with super node. + oldGraph.forEach { + 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, + mapping: Map>, + ): List> { + return orderedNodes.mapNotNull { mapping[it] } + } +} diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/request/TransactionBundleGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/request/TransactionBundleGenerator.kt index 665626115c..0ac6bb04e1 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/request/TransactionBundleGenerator.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/request/TransactionBundleGenerator.kt @@ -17,9 +17,9 @@ 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 com.google.android.fhir.sync.upload.patch.PatchMappingGroup import org.hl7.fhir.r4.model.Bundle /** Generates list of [BundleUploadRequest] of type Transaction [Bundle] from the [Patch]es */ @@ -32,28 +32,29 @@ internal class TransactionBundleGenerator( /** * 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. + * integrity on the server, the [PatchMapping]s in a [PatchMappingGroup.CombinedMappingGroup] are + * all put in a single [BundleUploadRequestMapping]. Based on the [generatedBundleSize], the + * remaining space of the [BundleUploadRequestMapping] maybe filled with other + * [PatchMappingGroup.CombinedMappingGroup] or [PatchMappingGroup.IndividualMappingGroup] + * mappings. * - * In case a single [OrderedMapping.CombinedMapping] has more [PatchMapping]s than the + * In case a single [PatchMappingGroup.CombinedMappingGroup] 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]. + * mappings in [PatchMappingGroup.CombinedMappingGroup] 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].** + * **NOTE: The order of the [PatchMappingGroup.IndividualMappingGroup] is always maintained and + * the order of [PatchMappingGroup.CombinedMappingGroup] doesn't matter since it contain all the + * required [PatchMapping] inside the same [Bundle].** */ override fun generateUploadRequests( - mappedPatches: List, + mappedPatches: List, ): List { val mappingsPerBundle = mutableListOf>() var bundle = mutableListOf() mappedPatches.forEach { when (it) { - is OrderedMapping.IndividualMapping -> { + is PatchMappingGroup.IndividualMappingGroup -> { if (bundle.size < generatedBundleSize) { bundle.add(it.patchMapping) } else { @@ -61,12 +62,14 @@ internal class TransactionBundleGenerator( bundle = mutableListOf(it.patchMapping) } } - is OrderedMapping.CombinedMapping -> { + is PatchMappingGroup.CombinedMappingGroup -> { if ((bundle.size + it.patchMappings.size) <= generatedBundleSize) { bundle.addAll(it.patchMappings) } else { - mappingsPerBundle.add(bundle) - bundle = mutableListOf() + if (bundle.isNotEmpty()) { + mappingsPerBundle.add(bundle) + bundle = mutableListOf() + } bundle.addAll(it.patchMappings) } } diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/request/UploadRequestGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/request/UploadRequestGenerator.kt index ae8fad5cf8..15c852ab64 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/request/UploadRequestGenerator.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/request/UploadRequestGenerator.kt @@ -17,22 +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 com.google.android.fhir.sync.upload.patch.PatchMappingGroup 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<[OrderedMapping]>]. Any implementation of this generator is expected to output + * [List<[PatchMappingGroup]>]. 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, + mappedPatches: List, ): List } diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/request/UrlRequestGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/request/UrlRequestGenerator.kt index 5e62f11715..ba241d3fdf 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/request/UrlRequestGenerator.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/request/UrlRequestGenerator.kt @@ -19,9 +19,9 @@ package com.google.android.fhir.sync.upload.request import ca.uhn.fhir.context.FhirContext import ca.uhn.fhir.context.FhirVersionEnum import com.google.android.fhir.ContentTypes -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 com.google.android.fhir.sync.upload.patch.PatchMappingGroup import org.hl7.fhir.r4.model.Binary import org.hl7.fhir.r4.model.Resource import org.hl7.fhir.r4.model.codesystems.HttpVerb @@ -33,17 +33,24 @@ internal class UrlRequestGenerator( /** * Since a [UrlUploadRequest] can only handle a single resource request, the - * [OrderedMapping.CombinedMapping.patchMappings] are flattened and handled as - * [OrderedMapping.IndividualMapping] mapping to generate [UrlUploadRequestMapping] for each - * [PatchMapping]. + * [PatchMappingGroup.CombinedMappingGroup.patchMappings] are flattened and handled as + * [PatchMappingGroup.IndividualMappingGroup] mapping to generate [UrlUploadRequestMapping] for + * each [PatchMapping]. + * + * **NOTE** + * + * Since the referential integrity on the sever may get violated if the subsequent requests have + * cyclic dependency on each other, We may introduce configuration for application to provide + * server's referential integrity settings and make it illegal to generate [UrlUploadRequest] when + * server has strict referential integrity and the requests have cyclic dependency amongst itself. */ override fun generateUploadRequests( - mappedPatches: List, + mappedPatches: List, ): List = mappedPatches .map { when (it) { - is OrderedMapping.IndividualMapping -> { + is PatchMappingGroup.IndividualMappingGroup -> { listOf( UrlUploadRequestMapping( localChanges = it.patchMapping.localChanges, @@ -51,7 +58,7 @@ internal class UrlRequestGenerator( ), ) } - is OrderedMapping.CombinedMapping -> { + is PatchMappingGroup.CombinedMappingGroup -> { it.patchMappings.map { UrlUploadRequestMapping( localChanges = it.localChanges, diff --git a/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PatchOrderingTest.kt b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PatchOrderingTest.kt index d59d010ce6..b6ceeb4fdb 100644 --- a/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PatchOrderingTest.kt +++ b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PatchOrderingTest.kt @@ -185,7 +185,7 @@ class PatchOrderingTest { // we have a different implementation of the topological sort. assertThat( result.map { - (it as OrderedMapping.IndividualMapping).patchMapping.generatedPatch.resourceId + (it as PatchMappingGroup.IndividualMappingGroup).patchMapping.generatedPatch.resourceId }, ) .containsExactly( @@ -230,11 +230,11 @@ class PatchOrderingTest { val result = patchGenerator.generate(helper.localChanges) - assertThat(result.filterIsInstance()).hasSize(2) - assertThat(result.filterIsInstance()).hasSize(5) + assertThat(result.filterIsInstance()).hasSize(2) + assertThat(result.filterIsInstance()).hasSize(5) assertThat( - result.filterIsInstance().map { + result.filterIsInstance().map { it.patchMappings.map { it.generatedPatch.resourceId } }, ) @@ -244,7 +244,7 @@ class PatchOrderingTest { ) assertThat( - result.filterIsInstance().map { + result.filterIsInstance().map { it.patchMapping.generatedPatch.resourceId }, ) diff --git a/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGeneratorTest.kt b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGeneratorTest.kt index 2ec32b86c8..0daf6787c3 100644 --- a/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGeneratorTest.kt +++ b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGeneratorTest.kt @@ -71,7 +71,7 @@ class PerResourcePatchGeneratorTest { val patches = patchGenerator.generate(listOf(insertionLocalChange)).map { - (it as OrderedMapping.IndividualMapping).patchMapping + (it as PatchMappingGroup.IndividualMappingGroup).patchMapping } with(patches.single()) { @@ -105,7 +105,7 @@ class PerResourcePatchGeneratorTest { val patches = patchGenerator.generate(listOf(updateLocalChange1)).map { - (it as OrderedMapping.IndividualMapping).patchMapping + (it as PatchMappingGroup.IndividualMappingGroup).patchMapping } with(patches.single()) { @@ -137,7 +137,7 @@ class PerResourcePatchGeneratorTest { val patches = patchGenerator.generate(listOf(deleteLocalChange)).map { - (it as OrderedMapping.IndividualMapping).patchMapping + (it as PatchMappingGroup.IndividualMappingGroup).patchMapping } with(patches.single()) { @@ -166,7 +166,7 @@ class PerResourcePatchGeneratorTest { val patches = patchGenerator.generate(listOf(insertionLocalChange, updateLocalChange)).map { - (it as OrderedMapping.IndividualMapping).patchMapping + (it as PatchMappingGroup.IndividualMappingGroup).patchMapping } with(patches.single()) { @@ -326,7 +326,7 @@ class PerResourcePatchGeneratorTest { val patches = patchGenerator.generate(listOf(updateLocalChange1, updateLocalChange2)).map { - (it as OrderedMapping.IndividualMapping).patchMapping + (it as PatchMappingGroup.IndividualMappingGroup).patchMapping } with(patches.single()) { @@ -374,7 +374,7 @@ class PerResourcePatchGeneratorTest { val patches = patchGenerator.generate(listOf(updatedLocalChange1, updatedLocalChange2)).map { - (it as OrderedMapping.IndividualMapping).patchMapping + (it as PatchMappingGroup.IndividualMappingGroup).patchMapping } with(patches.single().generatedPatch) { @@ -407,7 +407,7 @@ class PerResourcePatchGeneratorTest { .generate( listOf(updateLocalChange1, updateLocalChange2, deleteLocalChange), ) - .map { (it as OrderedMapping.IndividualMapping).patchMapping } + .map { (it as PatchMappingGroup.IndividualMappingGroup).patchMapping } with(patches.single()) { with(generatedPatch) { diff --git a/engine/src/test/java/com/google/android/fhir/sync/upload/request/IndividualGeneratorTest.kt b/engine/src/test/java/com/google/android/fhir/sync/upload/request/IndividualGeneratorTest.kt index 3f830183ef..509c46975b 100644 --- a/engine/src/test/java/com/google/android/fhir/sync/upload/request/IndividualGeneratorTest.kt +++ b/engine/src/test/java/com/google/android/fhir/sync/upload/request/IndividualGeneratorTest.kt @@ -16,8 +16,8 @@ package com.google.android.fhir.sync.upload.request -import com.google.android.fhir.sync.upload.patch.OrderedMapping import com.google.android.fhir.sync.upload.patch.PatchMapping +import com.google.android.fhir.sync.upload.patch.PatchMappingGroup import com.google.android.fhir.sync.upload.request.RequestGeneratorTestUtils.deleteLocalChange import com.google.android.fhir.sync.upload.request.RequestGeneratorTestUtils.insertionLocalChange import com.google.android.fhir.sync.upload.request.RequestGeneratorTestUtils.toPatch @@ -50,7 +50,7 @@ class IndividualGeneratorTest { ) val requests = generator.generateUploadRequests( - listOf(OrderedMapping.IndividualMapping(patchOutput)), + listOf(PatchMappingGroup.IndividualMappingGroup(patchOutput)), ) with(requests.single()) { @@ -73,7 +73,7 @@ class IndividualGeneratorTest { ) val requests = generator.generateUploadRequests( - listOf(OrderedMapping.IndividualMapping(patchOutput)), + listOf(PatchMappingGroup.IndividualMappingGroup(patchOutput)), ) with(requests.single()) { @@ -94,7 +94,9 @@ class IndividualGeneratorTest { ) val generator = UrlRequestGenerator.Factory.getDefault() val requests = - generator.generateUploadRequests(listOf(OrderedMapping.IndividualMapping(patchOutput))) + generator.generateUploadRequests( + listOf(PatchMappingGroup.IndividualMappingGroup(patchOutput)), + ) with(requests.single()) { with(generatedRequest) { assertThat(requests.size).isEqualTo(1) @@ -118,7 +120,9 @@ class IndividualGeneratorTest { ) val generator = UrlRequestGenerator.Factory.getDefault() val requests = - generator.generateUploadRequests(listOf(OrderedMapping.IndividualMapping(patchOutput))) + generator.generateUploadRequests( + listOf(PatchMappingGroup.IndividualMappingGroup(patchOutput)), + ) with(requests.single()) { with(generatedRequest) { assertThat(httpVerb).isEqualTo(HttpVerb.DELETE) @@ -136,7 +140,9 @@ class IndividualGeneratorTest { } val generator = UrlRequestGenerator.Factory.getDefault() val result = - generator.generateUploadRequests(patchOutputList.map { OrderedMapping.IndividualMapping(it) }) + generator.generateUploadRequests( + patchOutputList.map { PatchMappingGroup.IndividualMappingGroup(it) }, + ) assertThat(result).hasSize(3) assertThat(result.map { it.generatedRequest.httpVerb }) .containsExactly(HttpVerb.PUT, HttpVerb.PATCH, HttpVerb.DELETE) diff --git a/engine/src/test/java/com/google/android/fhir/sync/upload/request/TransactionBundleGeneratorTest.kt b/engine/src/test/java/com/google/android/fhir/sync/upload/request/TransactionBundleGeneratorTest.kt index 826964b9f6..66616945c4 100644 --- a/engine/src/test/java/com/google/android/fhir/sync/upload/request/TransactionBundleGeneratorTest.kt +++ b/engine/src/test/java/com/google/android/fhir/sync/upload/request/TransactionBundleGeneratorTest.kt @@ -18,8 +18,8 @@ package com.google.android.fhir.sync.upload.request import com.google.android.fhir.LocalChange import com.google.android.fhir.LocalChangeToken -import com.google.android.fhir.sync.upload.patch.OrderedMapping import com.google.android.fhir.sync.upload.patch.PatchMapping +import com.google.android.fhir.sync.upload.patch.PatchMappingGroup import com.google.android.fhir.sync.upload.request.RequestGeneratorTestUtils.deleteLocalChange import com.google.android.fhir.sync.upload.request.RequestGeneratorTestUtils.insertionLocalChange import com.google.android.fhir.sync.upload.request.RequestGeneratorTestUtils.toPatch @@ -51,7 +51,7 @@ class TransactionBundleGeneratorTest { val patches = listOf(insertionLocalChange, updateLocalChange, deleteLocalChange) .map { PatchMapping(listOf(it), it.toPatch()) } - .map { OrderedMapping.IndividualMapping(it) } + .map { PatchMappingGroup.IndividualMappingGroup(it) } val generator = TransactionBundleGenerator.Factory.getDefault() val result = generator.generateUploadRequests(patches) @@ -75,7 +75,7 @@ class TransactionBundleGeneratorTest { val patches = listOf(insertionLocalChange, updateLocalChange, deleteLocalChange) .map { PatchMapping(listOf(it), it.toPatch()) } - .map { OrderedMapping.IndividualMapping(it) } + .map { PatchMappingGroup.IndividualMappingGroup(it) } val generator = TransactionBundleGenerator.Factory.getGenerator( Bundle.HTTPVerb.PUT, @@ -123,7 +123,7 @@ class TransactionBundleGeneratorTest { generatedPatch = localChange.toPatch(), ), ) - .map { OrderedMapping.IndividualMapping(it) } + .map { PatchMappingGroup.IndividualMappingGroup(it) } val generator = TransactionBundleGenerator.Factory.getDefault(useETagForUpload = false) val result = generator.generateUploadRequests(patches) @@ -152,7 +152,7 @@ class TransactionBundleGeneratorTest { generatedPatch = localChange.toPatch(), ), ) - .map { OrderedMapping.IndividualMapping(it) } + .map { PatchMappingGroup.IndividualMappingGroup(it) } val generator = TransactionBundleGenerator.Factory.getDefault(useETagForUpload = true) val result = generator.generateUploadRequests(patches) @@ -189,7 +189,7 @@ class TransactionBundleGeneratorTest { val patches = localChanges .map { PatchMapping(listOf(it), it.toPatch()) } - .map { OrderedMapping.IndividualMapping(it) } + .map { PatchMappingGroup.IndividualMappingGroup(it) } val generator = TransactionBundleGenerator.Factory.getDefault(useETagForUpload = true) val result = generator.generateUploadRequests(patches) @@ -338,4 +338,142 @@ class TransactionBundleGeneratorTest { } assertThat(exception.localizedMessage).isEqualTo("Update using PUT is not supported.") } + + @Test + fun `generate() should not split changes in multiple bundle if combined mapping group has more patches than the permitted size`() = + runBlocking { + val localChange = + LocalChange( + resourceType = ResourceType.Patient.name, + resourceId = "Patient-00", + type = LocalChange.Type.UPDATE, + payload = "[]", + versionId = "patient-002-version-", + timestamp = Instant.now(), + token = LocalChangeToken(listOf(1L)), + ) + val patchGroups = + List(10) { + PatchMapping( + localChanges = + listOf( + localChange.copy( + resourceId = "Patient-00-$it", + versionId = "patient-002-version-$it", + ), + ), + generatedPatch = localChange.toPatch(), + ) + } + .let { PatchMappingGroup.CombinedMappingGroup(it) } + val generator = + TransactionBundleGenerator.Factory.getDefault(useETagForUpload = false, bundleSize = 5) + val result = generator.generateUploadRequests(listOf(patchGroups)) + + assertThat(result).hasSize(1) + assertThat(result.single().localChanges.size).isEqualTo(10) + } + + @Test + fun `generate() should put group mappings in respective bundles`() = runBlocking { + val localChange = + LocalChange( + resourceType = ResourceType.Patient.name, + resourceId = "Patient-00", + type = LocalChange.Type.UPDATE, + payload = "[]", + versionId = "patient-002-version-", + timestamp = Instant.now(), + token = LocalChangeToken(listOf(1L)), + ) + + val firstGroup = + PatchMappingGroup.CombinedMappingGroup( + mutableListOf().apply { + for (i in 1..5) { + add( + PatchMapping( + localChanges = + listOf( + localChange.copy( + resourceId = "Patient-00-$i", + versionId = "patient-002-version-$i", + ), + ), + generatedPatch = localChange.toPatch(), + ), + ) + } + }, + ) + + val secondGroup = + PatchMappingGroup.IndividualMappingGroup( + PatchMapping( + localChanges = + listOf( + localChange.copy(resourceId = "Patient-00-6", versionId = "patient-002-version-7"), + ), + generatedPatch = localChange.toPatch(), + ), + ) + + val thirdGroup = + PatchMappingGroup.IndividualMappingGroup( + PatchMapping( + localChanges = + listOf( + localChange.copy(resourceId = "Patient-00-7", versionId = "patient-002-version-8"), + ), + generatedPatch = localChange.toPatch(), + ), + ) + val fourthGroup = + PatchMappingGroup.CombinedMappingGroup( + mutableListOf().apply { + for (i in 9..13) { + add( + PatchMapping( + localChanges = + listOf( + localChange.copy( + resourceId = "Patient-00-$i", + versionId = "patient-002-version-$i", + ), + ), + generatedPatch = localChange.toPatch(), + ), + ) + } + }, + ) + + val patchGroups = listOf(firstGroup, secondGroup, thirdGroup, fourthGroup) + val generator = + TransactionBundleGenerator.Factory.getDefault(useETagForUpload = false, bundleSize = 5) + val result = generator.generateUploadRequests(patchGroups) + + assertThat(result).hasSize(3) + assertThat(result[0].localChanges.map { it.resourceId }) + .containsExactly( + "Patient-00-1", + "Patient-00-2", + "Patient-00-3", + "Patient-00-4", + "Patient-00-5", + ) + .inOrder() + assertThat(result[1].localChanges.map { it.resourceId }) + .containsExactly("Patient-00-6", "Patient-00-7") + .inOrder() + assertThat(result[2].localChanges.map { it.resourceId }) + .containsExactly( + "Patient-00-9", + "Patient-00-10", + "Patient-00-11", + "Patient-00-12", + "Patient-00-13", + ) + .inOrder() + } } From 76c4840a3fd692375c3b37d797439520d4306614 Mon Sep 17 00:00:00 2001 From: Aditya Khajuria Date: Thu, 6 Jun 2024 16:17:42 +0530 Subject: [PATCH 07/16] Refactored code and added tests. --- .../fhir/sync/upload/patch/StronglyConnectedPatchesTest.kt | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 engine/src/test/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatchesTest.kt diff --git a/engine/src/test/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatchesTest.kt b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatchesTest.kt new file mode 100644 index 0000000000..dd5f371a88 --- /dev/null +++ b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatchesTest.kt @@ -0,0 +1,5 @@ +package com.google.android.fhir.sync.upload.patch + +import org.junit.jupiter.api.Assertions.* + +class StronglyConnectedPatchesTest \ No newline at end of file From d346f0e35dbf942e65f13a16896d8276020dfea3 Mon Sep 17 00:00:00 2001 From: Aditya Khajuria Date: Thu, 6 Jun 2024 16:18:29 +0530 Subject: [PATCH 08/16] Refactored code and added tests. --- .../fhir/sync/upload/patch/PatchGenerator.kt | 22 ++-- .../fhir/sync/upload/patch/PatchOrdering.kt | 37 ++----- .../upload/patch/PerChangePatchGenerator.kt | 2 +- .../upload/patch/StronglyConnectedPatches.kt | 91 ++++------------ .../request/TransactionBundleGenerator.kt | 44 +++----- .../upload/request/UrlRequestGenerator.kt | 30 ++---- .../sync/upload/patch/PatchOrderingTest.kt | 31 ++---- .../patch/PerResourcePatchGeneratorTest.kt | 20 ++-- .../patch/StronglyConnectedPatchesTest.kt | 102 +++++++++++++++++- .../upload/request/IndividualGeneratorTest.kt | 10 +- .../request/TransactionBundleGeneratorTest.kt | 48 +++++---- 11 files changed, 198 insertions(+), 239 deletions(-) diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchGenerator.kt index b9ef9c2a06..54785cb0d1 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchGenerator.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchGenerator.kt @@ -68,19 +68,9 @@ internal data class PatchMapping( val generatedPatch: Patch, ) -/** Structure to help describe the cyclic nature of ordered [PatchMapping]. */ -internal sealed interface PatchMappingGroup { - - /** - * [IndividualMappingGroup] doesn't have a cyclic dependency on other [IndividualMappingGroup] / - * [PatchMapping]. It may however have an acyclic dependency on other [IndividualMappingGroup]s / - * [PatchMapping]s. - */ - data class IndividualMappingGroup(val patchMapping: PatchMapping) : PatchMappingGroup - - /** - * [CombinedMappingGroup] contains strongly connected [PatchMapping]s where one or more - * [PatchMapping]s have a cyclic dependency between each other. - */ - data class CombinedMappingGroup(val patchMappings: List) : PatchMappingGroup -} +/** + * Structure to describe the cyclic nature of [PatchMapping]. + * - A single value in [patchMappings] signifies the acyclic nature of the node. + * - Multiple values in [patchMappings] signifies the cyclic nature of the nodes among themselves. + */ +internal data class PatchMappingGroup(val patchMappings: List) diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt index ea473efca0..e78c5474fd 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt @@ -56,9 +56,9 @@ internal object PatchOrdering { * retained. Order of D shouldn't matter for the purpose of referential integrity. * * @return A ordered list of the [PatchMappingGroup] containing: - * - [PatchMappingGroup.IndividualMappingGroup] for the [PatchMapping] based on the references to - * other [PatchMapping] if the mappings are acyclic - * - [PatchMappingGroup.CombinedMappingGroup] for [PatchMapping]s based on the references to other + * - [PatchMappingGroup] with single value for the [PatchMapping] based on the references to other + * [PatchMapping] if the mappings are acyclic + * - [PatchMappingGroup] with multiple values for [PatchMapping]s based on the references to other * [PatchMapping]s if the mappings are cyclic. */ suspend fun List.orderByReferences( @@ -75,16 +75,9 @@ internal object PatchOrdering { val adjacencyList = createAdjacencyListForCreateReferences(localChangeIdToResourceReferenceMap) - return StronglyConnectedPatches(adjacencyList, resourceIdToPatchMapping.size) - .sscOrdered { createTopologicalOrderedList(it) } - .map { - val mappings = it.mapNotNull { resourceIdToPatchMapping[it] } - if (mappings.size == 1) { - PatchMappingGroup.IndividualMappingGroup(mappings.first()) - } else { - PatchMappingGroup.CombinedMappingGroup(mappings) - } - } + return StronglyConnectedPatches.scc(adjacencyList, resourceIdToPatchMapping.size).map { + PatchMappingGroup(it.mapNotNull { resourceIdToPatchMapping[it] }) + } } /** @@ -134,22 +127,4 @@ internal object PatchOrdering { } return references } - - private fun createTopologicalOrderedList(adjacencyList: Map>): List { - val stack = ArrayDeque() - val visited = mutableSetOf() - val currentPath = mutableSetOf() - - fun dfs(key: String) { - check(currentPath.add(key)) { "Detected a cycle." } - if (visited.add(key)) { - adjacencyList[key]?.forEach { dfs(it) } - stack.addFirst(key) - } - currentPath.remove(key) - } - - adjacencyList.keys.forEach { dfs(it) } - return stack.reversed() - } } diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerChangePatchGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerChangePatchGenerator.kt index 400c1b291b..4c59650357 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerChangePatchGenerator.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerChangePatchGenerator.kt @@ -41,5 +41,5 @@ internal object PerChangePatchGenerator : PatchGenerator { ), ) } - .map { PatchMappingGroup.IndividualMappingGroup(it) } + .map { PatchMappingGroup(listOf(it)) } } diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt index 7a4645ce6a..7bf3805770 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt @@ -18,30 +18,30 @@ package com.google.android.fhir.sync.upload.patch import kotlin.math.min -internal class StronglyConnectedPatches(private val diGraph: Graph, private val nodesCount: Int) { +internal object StronglyConnectedPatches { - fun sscOrdered(ordering: (Graph) -> List): List> { - val stronglyConnected = sscTarzan() - val (superNodeToConnectedComponents, connectedComponentNodeToSuperNode) = - reduceToSuperNodes( - stronglyConnected, - ) - val updatedGraph = - transformGraphWithSSCtoGraphWithSuperNodes(diGraph, connectedComponentNodeToSuperNode) - val orderedNodes = ordering(updatedGraph) - return superNodeToSSC(orderedNodes, superNodeToConnectedComponents) + /** + * Takes a [directedGraph] and the number of the nodes [nodeCount] in the [directedGraph] and + * computes all the strongly connected components in the graph. + * + * @return An ordered List of strongly connected components of the [directedGraph]. The SCCs are + * topologically ordered which may change based on the ordering algorithm and the [Node]s inside + * a SSC may be ordered randomly depending on the path taken by algorithm to discover the nodes. + */ + fun scc(directedGraph: Graph, nodeCount: Int): List> { + return sccTarjan(directedGraph, nodeCount) } - private fun sscTarzan(): List> { + private fun sccTarjan(directedGraph: Graph, nodeCount: Int): List> { // 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() val nodeToInt = mutableMapOf() - val graph = MutableList>(nodesCount) { mutableListOf() } + val graph = MutableList>(nodeCount) { mutableListOf() } - diGraph.forEach { (key, value) -> + directedGraph.forEach { (key, value) -> val intForKey = nodeToInt[key] ?: let { @@ -98,14 +98,16 @@ internal class StronglyConnectedPatches(private val diGraph: Graph, private val } } - for (i in 0 until nodesCount) { + for (i in 0 until nodeCount) { if (ids[i] == -1) { dfs(i) } } - val lowLinkToConnectedMap = mutableMapOf>() - for (i in 0 until nodesCount) { + // The algorithm discovers the scc in topological order, so use a sorted map to maintain the + // order. + val lowLinkToConnectedMap = sortedMapOf>() + for (i in 0 until nodeCount) { if (!lowLinkToConnectedMap.containsKey(sccs[i])) { lowLinkToConnectedMap[sccs[i]] = mutableListOf() } @@ -113,59 +115,4 @@ internal class StronglyConnectedPatches(private val diGraph: Graph, private val } return lowLinkToConnectedMap.values.map { it.mapNotNull { intToNode[it] } } } - - private fun reduceToSuperNodes( - ssc: List>, - ): Pair>, Map> { - val superNodesMap = mutableMapOf>() - val nodeToSuperNode = mutableMapOf() - - var counter = 0 - ssc.forEach { - superNodesMap[(++counter).toString()] = it - it.forEach { nodeToSuperNode[it] = (counter).toString() } - } - - return superNodesMap to nodeToSuperNode - } - - /** - * Converts a graph that has ssc to a graph with super nodes as per the provided [nodeToSuperNode] - * mapping. - * - * **Input** - * * oldGraph `[A : B, C], [B : A, C], [C : A, B, F], [D : E], [E : D], [F : G]` - * * nodeToSuperNode `[A:1, B:1, C:1, D:2, E:2, F:3, G:4]` - * - * **Output** `[1 : 3], [2], [3: 4]` - */ - private fun transformGraphWithSSCtoGraphWithSuperNodes( - oldGraph: Graph, - nodeToSuperNode: Map, - ): Graph { - val newGraph = mutableMapOf>() - // Remove any cyclic dependency from connected components in the graph. - // Reduce all the ssc nodes from graph into to a single node in graph. - // Replace the ssc nodes with super node. - oldGraph.forEach { - 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, - mapping: Map>, - ): List> { - return orderedNodes.mapNotNull { mapping[it] } - } } diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/request/TransactionBundleGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/request/TransactionBundleGenerator.kt index 0ac6bb04e1..d3309790b9 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/request/TransactionBundleGenerator.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/request/TransactionBundleGenerator.kt @@ -32,19 +32,13 @@ internal class TransactionBundleGenerator( /** * In order to accommodate cyclic dependencies between [PatchMapping]s and maintain referential - * integrity on the server, the [PatchMapping]s in a [PatchMappingGroup.CombinedMappingGroup] are - * all put in a single [BundleUploadRequestMapping]. Based on the [generatedBundleSize], the - * remaining space of the [BundleUploadRequestMapping] maybe filled with other - * [PatchMappingGroup.CombinedMappingGroup] or [PatchMappingGroup.IndividualMappingGroup] - * mappings. + * integrity on the server, the [PatchMapping]s in a [PatchMappingGroup] are all put in a single + * [BundleUploadRequestMapping]. Based on the [generatedBundleSize], the remaining space of the + * [BundleUploadRequestMapping] maybe filled with other [PatchMappingGroup] mappings. * - * In case a single [PatchMappingGroup.CombinedMappingGroup] has more [PatchMapping]s than the - * [generatedBundleSize], [generatedBundleSize] will be ignored so that all of the dependent - * mappings in [PatchMappingGroup.CombinedMappingGroup] can be sent in a single [Bundle]. - * - * **NOTE: The order of the [PatchMappingGroup.IndividualMappingGroup] is always maintained and - * the order of [PatchMappingGroup.CombinedMappingGroup] doesn't matter since it contain all the - * required [PatchMapping] inside the same [Bundle].** + * In case a single [PatchMappingGroup] has more [PatchMapping]s than the [generatedBundleSize], + * [generatedBundleSize] will be ignored so that all of the dependent mappings in + * [PatchMappingGroup] can be sent in a single [Bundle]. */ override fun generateUploadRequests( mappedPatches: List, @@ -53,26 +47,14 @@ internal class TransactionBundleGenerator( var bundle = mutableListOf() mappedPatches.forEach { - when (it) { - is PatchMappingGroup.IndividualMappingGroup -> { - if (bundle.size < generatedBundleSize) { - bundle.add(it.patchMapping) - } else { - mappingsPerBundle.add(bundle) - bundle = mutableListOf(it.patchMapping) - } - } - is PatchMappingGroup.CombinedMappingGroup -> { - if ((bundle.size + it.patchMappings.size) <= generatedBundleSize) { - bundle.addAll(it.patchMappings) - } else { - if (bundle.isNotEmpty()) { - mappingsPerBundle.add(bundle) - bundle = mutableListOf() - } - bundle.addAll(it.patchMappings) - } + if ((bundle.size + it.patchMappings.size) <= generatedBundleSize) { + bundle.addAll(it.patchMappings) + } else { + if (bundle.isNotEmpty()) { + mappingsPerBundle.add(bundle) + bundle = mutableListOf() } + bundle.addAll(it.patchMappings) } } diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/request/UrlRequestGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/request/UrlRequestGenerator.kt index ba241d3fdf..cdf7bd6bac 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/request/UrlRequestGenerator.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/request/UrlRequestGenerator.kt @@ -33,9 +33,8 @@ internal class UrlRequestGenerator( /** * Since a [UrlUploadRequest] can only handle a single resource request, the - * [PatchMappingGroup.CombinedMappingGroup.patchMappings] are flattened and handled as - * [PatchMappingGroup.IndividualMappingGroup] mapping to generate [UrlUploadRequestMapping] for - * each [PatchMapping]. + * [PatchMappingGroup.patchMappings] are flattened and handled as acyclic mapping to generate + * [UrlUploadRequestMapping] for each [PatchMapping]. * * **NOTE** * @@ -48,27 +47,14 @@ internal class UrlRequestGenerator( mappedPatches: List, ): List = mappedPatches + .map { it.patchMappings } + .flatten() .map { - when (it) { - is PatchMappingGroup.IndividualMappingGroup -> { - listOf( - UrlUploadRequestMapping( - localChanges = it.patchMapping.localChanges, - generatedRequest = getUrlRequestForPatch(it.patchMapping.generatedPatch), - ), - ) - } - is PatchMappingGroup.CombinedMappingGroup -> { - it.patchMappings.map { - UrlUploadRequestMapping( - localChanges = it.localChanges, - generatedRequest = getUrlRequestForPatch(it.generatedPatch), - ) - } - } - } + UrlUploadRequestMapping( + localChanges = it.localChanges, + generatedRequest = getUrlRequestForPatch(it.generatedPatch), + ) } - .flatten() companion object Factory { diff --git a/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PatchOrderingTest.kt b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PatchOrderingTest.kt index b6ceeb4fdb..f6bcae7883 100644 --- a/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PatchOrderingTest.kt +++ b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PatchOrderingTest.kt @@ -183,11 +183,7 @@ class PatchOrderingTest { // This order is based on the current implementation of the topological sort in [PatchOrdering], // it's entirely possible to generate different order here which is acceptable/correct, should // we have a different implementation of the topological sort. - assertThat( - result.map { - (it as PatchMappingGroup.IndividualMappingGroup).patchMapping.generatedPatch.resourceId - }, - ) + assertThat(result.map { it.patchMappings.single().generatedPatch.resourceId }) .containsExactly( "patient-1", "patient-2", @@ -230,30 +226,17 @@ class PatchOrderingTest { val result = patchGenerator.generate(helper.localChanges) - assertThat(result.filterIsInstance()).hasSize(2) - assertThat(result.filterIsInstance()).hasSize(5) - assertThat( - result.filterIsInstance().map { - it.patchMappings.map { it.generatedPatch.resourceId } - }, + result.map { it.patchMappings.map { it.generatedPatch.resourceId } }, ) .containsExactly( listOf("patient-1", "related-1"), listOf("patient-2", "related-2"), - ) - - assertThat( - result.filterIsInstance().map { - it.patchMapping.generatedPatch.resourceId - }, - ) - .containsExactly( - "encounter-1", - "observation-1", - "patient-3", - "encounter-2", - "observation-2", + listOf("encounter-1"), + listOf("observation-1"), + listOf("patient-3"), + listOf("encounter-2"), + listOf("observation-2"), ) .inOrder() } diff --git a/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGeneratorTest.kt b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGeneratorTest.kt index 0daf6787c3..38811c581d 100644 --- a/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGeneratorTest.kt +++ b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGeneratorTest.kt @@ -70,9 +70,7 @@ class PerResourcePatchGeneratorTest { val insertionLocalChange = createInsertLocalChange(patient) val patches = - patchGenerator.generate(listOf(insertionLocalChange)).map { - (it as PatchMappingGroup.IndividualMappingGroup).patchMapping - } + patchGenerator.generate(listOf(insertionLocalChange)).map { it.patchMappings.single() } with(patches.single()) { with(generatedPatch) { @@ -104,9 +102,7 @@ class PerResourcePatchGeneratorTest { val updatePatch = readJsonArrayFromFile("/update_patch_1.json") val patches = - patchGenerator.generate(listOf(updateLocalChange1)).map { - (it as PatchMappingGroup.IndividualMappingGroup).patchMapping - } + patchGenerator.generate(listOf(updateLocalChange1)).map { it.patchMappings.single() } with(patches.single()) { with(generatedPatch) { @@ -136,9 +132,7 @@ class PerResourcePatchGeneratorTest { val deleteLocalChange = createDeleteLocalChange(remotePatient, 3L) val patches = - patchGenerator.generate(listOf(deleteLocalChange)).map { - (it as PatchMappingGroup.IndividualMappingGroup).patchMapping - } + patchGenerator.generate(listOf(deleteLocalChange)).map { it.patchMappings.single() } with(patches.single()) { with(generatedPatch) { @@ -166,7 +160,7 @@ class PerResourcePatchGeneratorTest { val patches = patchGenerator.generate(listOf(insertionLocalChange, updateLocalChange)).map { - (it as PatchMappingGroup.IndividualMappingGroup).patchMapping + it.patchMappings.single() } with(patches.single()) { @@ -326,7 +320,7 @@ class PerResourcePatchGeneratorTest { val patches = patchGenerator.generate(listOf(updateLocalChange1, updateLocalChange2)).map { - (it as PatchMappingGroup.IndividualMappingGroup).patchMapping + it.patchMappings.single() } with(patches.single()) { @@ -374,7 +368,7 @@ class PerResourcePatchGeneratorTest { val patches = patchGenerator.generate(listOf(updatedLocalChange1, updatedLocalChange2)).map { - (it as PatchMappingGroup.IndividualMappingGroup).patchMapping + it.patchMappings.single() } with(patches.single().generatedPatch) { @@ -407,7 +401,7 @@ class PerResourcePatchGeneratorTest { .generate( listOf(updateLocalChange1, updateLocalChange2, deleteLocalChange), ) - .map { (it as PatchMappingGroup.IndividualMappingGroup).patchMapping } + .map { it.patchMappings.single() } with(patches.single()) { with(generatedPatch) { diff --git a/engine/src/test/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatchesTest.kt b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatchesTest.kt index dd5f371a88..802985adf9 100644 --- a/engine/src/test/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatchesTest.kt +++ b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatchesTest.kt @@ -1,5 +1,103 @@ +/* + * Copyright 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. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + package com.google.android.fhir.sync.upload.patch -import org.junit.jupiter.api.Assertions.* +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +@RunWith(RobolectricTestRunner::class) +class StronglyConnectedPatchesTest { + + @Test + fun `sscOrdered should return strongly connected components in order`() { + val graph = mutableMapOf>() + + graph.addEdge("0", "1") + graph.addEdge("1", "2") + graph.addEdge("2", "1") + + graph.addEdge("3", "4") + graph.addEdge("4", "5") + graph.addEdge("5", "3") + + graph.addEdge("6", "7") + graph.addEdge("7", "8") + + val result = StronglyConnectedPatches.scc(graph, 9) + + assertThat(result) + .containsExactly( + listOf("0"), + listOf("1", "2"), + listOf("3", "4", "5"), + listOf("7"), + listOf("8"), + listOf("6"), + ) + } + + @Test + fun `sscOrdered empty graph should return empty result`() { + val graph = mutableMapOf>() + val result = StronglyConnectedPatches.scc(graph, 0) + assertThat(result).isEmpty() + } + + @Test + fun `sscOrdered graph with single node should return single scc`() { + val graph = mutableMapOf>() + graph.addNode("0") + val result = StronglyConnectedPatches.scc(graph, 1) + assertThat(result).containsExactly(listOf("0")) + } + + @Test + fun `sscOrdered graph with two node should return two scc`() { + val graph = mutableMapOf>() + graph.addNode("0") + graph.addNode("1") + val result = StronglyConnectedPatches.scc(graph, 2) + assertThat(result).containsExactly(listOf("0"), listOf("1")) + } + + @Test + fun `sscOrdered graph with two acyclic node should return two scc in order`() { + val graph = mutableMapOf>() + graph.addEdge("1", "0") + val result = StronglyConnectedPatches.scc(graph, 2) + assertThat(result).containsExactly(listOf("0"), listOf("1")).inOrder() + } + + @Test + fun `sscOrdered graph with two cyclic node should return single scc`() { + val graph = mutableMapOf>() + graph.addEdge("0", "1") + graph.addEdge("1", "0") + val result = StronglyConnectedPatches.scc(graph, 2) + assertThat(result).containsExactly(listOf("0", "1")) + } +} + +private fun Graph.addEdge(node: Node, dependsOn: Node) { + (this as MutableMap).getOrPut(node) { mutableListOf() }.let { (it as MutableList).add(dependsOn) } +} -class StronglyConnectedPatchesTest \ No newline at end of file +private fun Graph.addNode(node: Node) { + (this as MutableMap)[node] = mutableListOf() +} diff --git a/engine/src/test/java/com/google/android/fhir/sync/upload/request/IndividualGeneratorTest.kt b/engine/src/test/java/com/google/android/fhir/sync/upload/request/IndividualGeneratorTest.kt index 509c46975b..5bb608418b 100644 --- a/engine/src/test/java/com/google/android/fhir/sync/upload/request/IndividualGeneratorTest.kt +++ b/engine/src/test/java/com/google/android/fhir/sync/upload/request/IndividualGeneratorTest.kt @@ -50,7 +50,7 @@ class IndividualGeneratorTest { ) val requests = generator.generateUploadRequests( - listOf(PatchMappingGroup.IndividualMappingGroup(patchOutput)), + listOf(PatchMappingGroup(listOf(patchOutput))), ) with(requests.single()) { @@ -73,7 +73,7 @@ class IndividualGeneratorTest { ) val requests = generator.generateUploadRequests( - listOf(PatchMappingGroup.IndividualMappingGroup(patchOutput)), + listOf(PatchMappingGroup(listOf(patchOutput))), ) with(requests.single()) { @@ -95,7 +95,7 @@ class IndividualGeneratorTest { val generator = UrlRequestGenerator.Factory.getDefault() val requests = generator.generateUploadRequests( - listOf(PatchMappingGroup.IndividualMappingGroup(patchOutput)), + listOf(PatchMappingGroup(listOf(patchOutput))), ) with(requests.single()) { with(generatedRequest) { @@ -121,7 +121,7 @@ class IndividualGeneratorTest { val generator = UrlRequestGenerator.Factory.getDefault() val requests = generator.generateUploadRequests( - listOf(PatchMappingGroup.IndividualMappingGroup(patchOutput)), + listOf(PatchMappingGroup(listOf(patchOutput))), ) with(requests.single()) { with(generatedRequest) { @@ -141,7 +141,7 @@ class IndividualGeneratorTest { val generator = UrlRequestGenerator.Factory.getDefault() val result = generator.generateUploadRequests( - patchOutputList.map { PatchMappingGroup.IndividualMappingGroup(it) }, + patchOutputList.map { PatchMappingGroup(listOf(it)) }, ) assertThat(result).hasSize(3) assertThat(result.map { it.generatedRequest.httpVerb }) diff --git a/engine/src/test/java/com/google/android/fhir/sync/upload/request/TransactionBundleGeneratorTest.kt b/engine/src/test/java/com/google/android/fhir/sync/upload/request/TransactionBundleGeneratorTest.kt index 66616945c4..037f19e92f 100644 --- a/engine/src/test/java/com/google/android/fhir/sync/upload/request/TransactionBundleGeneratorTest.kt +++ b/engine/src/test/java/com/google/android/fhir/sync/upload/request/TransactionBundleGeneratorTest.kt @@ -51,7 +51,7 @@ class TransactionBundleGeneratorTest { val patches = listOf(insertionLocalChange, updateLocalChange, deleteLocalChange) .map { PatchMapping(listOf(it), it.toPatch()) } - .map { PatchMappingGroup.IndividualMappingGroup(it) } + .map { PatchMappingGroup(listOf(it)) } val generator = TransactionBundleGenerator.Factory.getDefault() val result = generator.generateUploadRequests(patches) @@ -75,7 +75,7 @@ class TransactionBundleGeneratorTest { val patches = listOf(insertionLocalChange, updateLocalChange, deleteLocalChange) .map { PatchMapping(listOf(it), it.toPatch()) } - .map { PatchMappingGroup.IndividualMappingGroup(it) } + .map { PatchMappingGroup(listOf(it)) } val generator = TransactionBundleGenerator.Factory.getGenerator( Bundle.HTTPVerb.PUT, @@ -123,7 +123,7 @@ class TransactionBundleGeneratorTest { generatedPatch = localChange.toPatch(), ), ) - .map { PatchMappingGroup.IndividualMappingGroup(it) } + .map { PatchMappingGroup(listOf(it)) } val generator = TransactionBundleGenerator.Factory.getDefault(useETagForUpload = false) val result = generator.generateUploadRequests(patches) @@ -152,7 +152,7 @@ class TransactionBundleGeneratorTest { generatedPatch = localChange.toPatch(), ), ) - .map { PatchMappingGroup.IndividualMappingGroup(it) } + .map { PatchMappingGroup(listOf(it)) } val generator = TransactionBundleGenerator.Factory.getDefault(useETagForUpload = true) val result = generator.generateUploadRequests(patches) @@ -189,7 +189,7 @@ class TransactionBundleGeneratorTest { val patches = localChanges .map { PatchMapping(listOf(it), it.toPatch()) } - .map { PatchMappingGroup.IndividualMappingGroup(it) } + .map { PatchMappingGroup(listOf(it)) } val generator = TransactionBundleGenerator.Factory.getDefault(useETagForUpload = true) val result = generator.generateUploadRequests(patches) @@ -365,7 +365,7 @@ class TransactionBundleGeneratorTest { generatedPatch = localChange.toPatch(), ) } - .let { PatchMappingGroup.CombinedMappingGroup(it) } + .let { PatchMappingGroup(it) } val generator = TransactionBundleGenerator.Factory.getDefault(useETagForUpload = false, bundleSize = 5) val result = generator.generateUploadRequests(listOf(patchGroups)) @@ -388,7 +388,7 @@ class TransactionBundleGeneratorTest { ) val firstGroup = - PatchMappingGroup.CombinedMappingGroup( + PatchMappingGroup( mutableListOf().apply { for (i in 1..5) { add( @@ -408,28 +408,32 @@ class TransactionBundleGeneratorTest { ) val secondGroup = - PatchMappingGroup.IndividualMappingGroup( - PatchMapping( - localChanges = - listOf( - localChange.copy(resourceId = "Patient-00-6", versionId = "patient-002-version-7"), - ), - generatedPatch = localChange.toPatch(), + PatchMappingGroup( + listOf( + PatchMapping( + localChanges = + listOf( + localChange.copy(resourceId = "Patient-00-6", versionId = "patient-002-version-7"), + ), + generatedPatch = localChange.toPatch(), + ), ), ) val thirdGroup = - PatchMappingGroup.IndividualMappingGroup( - PatchMapping( - localChanges = - listOf( - localChange.copy(resourceId = "Patient-00-7", versionId = "patient-002-version-8"), - ), - generatedPatch = localChange.toPatch(), + PatchMappingGroup( + listOf( + PatchMapping( + localChanges = + listOf( + localChange.copy(resourceId = "Patient-00-7", versionId = "patient-002-version-8"), + ), + generatedPatch = localChange.toPatch(), + ), ), ) val fourthGroup = - PatchMappingGroup.CombinedMappingGroup( + PatchMappingGroup( mutableListOf().apply { for (i in 9..13) { add( From 4fc3e354c2c16773a866c8c94c8eb4af76392107 Mon Sep 17 00:00:00 2001 From: Aditya Khajuria Date: Thu, 6 Jun 2024 16:36:03 +0530 Subject: [PATCH 09/16] Updated StronglyConnectedPatches to compute node size --- .../android/fhir/sync/upload/patch/PatchOrdering.kt | 2 +- .../sync/upload/patch/StronglyConnectedPatches.kt | 6 +++--- .../upload/patch/StronglyConnectedPatchesTest.kt | 12 ++++++------ 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt index e78c5474fd..75aab482a5 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt @@ -75,7 +75,7 @@ internal object PatchOrdering { val adjacencyList = createAdjacencyListForCreateReferences(localChangeIdToResourceReferenceMap) - return StronglyConnectedPatches.scc(adjacencyList, resourceIdToPatchMapping.size).map { + return StronglyConnectedPatches.scc(adjacencyList).map { PatchMappingGroup(it.mapNotNull { resourceIdToPatchMapping[it] }) } } diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt index 7bf3805770..11b8a6effe 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt @@ -21,14 +21,14 @@ import kotlin.math.min internal object StronglyConnectedPatches { /** - * Takes a [directedGraph] and the number of the nodes [nodeCount] in the [directedGraph] and - * computes all the strongly connected components in the graph. + * Takes a [directedGraph] and computes all the strongly connected components in the graph. * * @return An ordered List of strongly connected components of the [directedGraph]. The SCCs are * topologically ordered which may change based on the ordering algorithm and the [Node]s inside * a SSC may be ordered randomly depending on the path taken by algorithm to discover the nodes. */ - fun scc(directedGraph: Graph, nodeCount: Int): List> { + fun scc(directedGraph: Graph): List> { + val nodeCount = (directedGraph.values.flatten().toSet() + directedGraph.keys).size return sccTarjan(directedGraph, nodeCount) } diff --git a/engine/src/test/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatchesTest.kt b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatchesTest.kt index 802985adf9..8ea23f95ef 100644 --- a/engine/src/test/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatchesTest.kt +++ b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatchesTest.kt @@ -39,7 +39,7 @@ class StronglyConnectedPatchesTest { graph.addEdge("6", "7") graph.addEdge("7", "8") - val result = StronglyConnectedPatches.scc(graph, 9) + val result = StronglyConnectedPatches.scc(graph) assertThat(result) .containsExactly( @@ -55,7 +55,7 @@ class StronglyConnectedPatchesTest { @Test fun `sscOrdered empty graph should return empty result`() { val graph = mutableMapOf>() - val result = StronglyConnectedPatches.scc(graph, 0) + val result = StronglyConnectedPatches.scc(graph) assertThat(result).isEmpty() } @@ -63,7 +63,7 @@ class StronglyConnectedPatchesTest { fun `sscOrdered graph with single node should return single scc`() { val graph = mutableMapOf>() graph.addNode("0") - val result = StronglyConnectedPatches.scc(graph, 1) + val result = StronglyConnectedPatches.scc(graph) assertThat(result).containsExactly(listOf("0")) } @@ -72,7 +72,7 @@ class StronglyConnectedPatchesTest { val graph = mutableMapOf>() graph.addNode("0") graph.addNode("1") - val result = StronglyConnectedPatches.scc(graph, 2) + val result = StronglyConnectedPatches.scc(graph) assertThat(result).containsExactly(listOf("0"), listOf("1")) } @@ -80,7 +80,7 @@ class StronglyConnectedPatchesTest { fun `sscOrdered graph with two acyclic node should return two scc in order`() { val graph = mutableMapOf>() graph.addEdge("1", "0") - val result = StronglyConnectedPatches.scc(graph, 2) + val result = StronglyConnectedPatches.scc(graph) assertThat(result).containsExactly(listOf("0"), listOf("1")).inOrder() } @@ -89,7 +89,7 @@ class StronglyConnectedPatchesTest { val graph = mutableMapOf>() graph.addEdge("0", "1") graph.addEdge("1", "0") - val result = StronglyConnectedPatches.scc(graph, 2) + val result = StronglyConnectedPatches.scc(graph) assertThat(result).containsExactly(listOf("0", "1")) } } From 0100dcab202656147b2c5b66e2fcd452410f001f Mon Sep 17 00:00:00 2001 From: Aditya Khajuria Date: Thu, 6 Jun 2024 17:05:57 +0530 Subject: [PATCH 10/16] Added comments --- .../upload/patch/StronglyConnectedPatches.kt | 27 ++++++++++++------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt index 11b8a6effe..47898bb5de 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt @@ -20,6 +20,8 @@ import kotlin.math.min internal object StronglyConnectedPatches { + private const val NOT_VISITED = -1 + /** * Takes a [directedGraph] and computes all the strongly connected components in the graph. * @@ -36,10 +38,13 @@ internal object StronglyConnectedPatches { // Code inspired from // https://github.com/williamfiset/Algorithms/blob/master/src/main/java/com/williamfiset/algorithms/graphtheory/TarjanSccSolverAdjacencyList.java var counter = -1 + // Maps an Integer value between the range of 0 to nodeCount (excluded) to the Node. val intToNode = mutableMapOf() + // Maps the Node to an Integer value between the range of 0 to nodeCount (excluded) and is + // inverse of [intToNode]. val nodeToInt = mutableMapOf() - val graph = MutableList>(nodeCount) { mutableListOf() } + val graphOfInts = MutableList>(nodeCount) { mutableListOf() } directedGraph.forEach { (key, value) -> val intForKey = @@ -58,25 +63,29 @@ internal object StronglyConnectedPatches { nodeToInt[node] = counter counter } - graph[intForKey].add(intForValue) + graphOfInts[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 visited = BooleanArray(graphOfInts.size) + val ids = IntArray(graphOfInts.size) { NOT_VISITED } + val low = IntArray(graphOfInts.size) + val sccs = IntArray(graphOfInts.size) val stack = ArrayDeque() fun dfs(at: Int) { + // set the low value for the visiting node as the id(visited counter) as we are exploring the + // graph. + // This later may get updated to the lowest value of the scc. low[at] = id++ + // set the ids as (visited counter) as we are exploring the graph. ids[at] = low[at] stack.addFirst(at) visited[at] = true - for (to in graph[at]) { - if (ids[to] == -1) { + for (to in graphOfInts[at]) { + if (ids[to] == NOT_VISITED) { dfs(to) } if (visited[to]) { @@ -99,7 +108,7 @@ internal object StronglyConnectedPatches { } for (i in 0 until nodeCount) { - if (ids[i] == -1) { + if (ids[i] == NOT_VISITED) { dfs(i) } } From 7415b6250a6bd7c43ce7b6a389b8aedb72b78ad4 Mon Sep 17 00:00:00 2001 From: Aditya Khajuria Date: Mon, 10 Jun 2024 20:35:03 +0530 Subject: [PATCH 11/16] Updated tarjan --- .../upload/patch/StronglyConnectedPatches.kt | 113 ++++++------------ .../patch/StronglyConnectedPatchesTest.kt | 5 +- 2 files changed, 37 insertions(+), 81 deletions(-) diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt index 47898bb5de..99f07d3380 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt @@ -20,8 +20,6 @@ import kotlin.math.min internal object StronglyConnectedPatches { - private const val NOT_VISITED = -1 - /** * Takes a [directedGraph] and computes all the strongly connected components in the graph. * @@ -30,98 +28,55 @@ internal object StronglyConnectedPatches { * a SSC may be ordered randomly depending on the path taken by algorithm to discover the nodes. */ fun scc(directedGraph: Graph): List> { - val nodeCount = (directedGraph.values.flatten().toSet() + directedGraph.keys).size - return sccTarjan(directedGraph, nodeCount) + return findSCCWithTarjan(directedGraph) } - private fun sccTarjan(directedGraph: Graph, nodeCount: Int): List> { - // Code inspired from - // https://github.com/williamfiset/Algorithms/blob/master/src/main/java/com/williamfiset/algorithms/graphtheory/TarjanSccSolverAdjacencyList.java - var counter = -1 - // Maps an Integer value between the range of 0 to nodeCount (excluded) to the Node. - val intToNode = mutableMapOf() - // Maps the Node to an Integer value between the range of 0 to nodeCount (excluded) and is - // inverse of [intToNode]. - val nodeToInt = mutableMapOf() - - val graphOfInts = MutableList>(nodeCount) { mutableListOf() } - - directedGraph.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 - } - graphOfInts[intForKey].add(intForValue) - } - } + private fun findSCCWithTarjan(diGraph: Graph): List> { + val connectedComponents = mutableListOf>() + val currentLowValueForEachNode = mutableMapOf() + var exploringCounter = 0 + val assignedLowValueForEachNode = mutableMapOf() + val nodesCurrentlyInStack = mutableSetOf() + val visitedNodes = mutableSetOf() + val stack = ArrayDeque() - var id = 0 - var sccCount = 0 - val visited = BooleanArray(graphOfInts.size) - val ids = IntArray(graphOfInts.size) { NOT_VISITED } - val low = IntArray(graphOfInts.size) - val sccs = IntArray(graphOfInts.size) - val stack = ArrayDeque() - - fun dfs(at: Int) { - // set the low value for the visiting node as the id(visited counter) as we are exploring the - // graph. - // This later may get updated to the lowest value of the scc. - low[at] = id++ - // set the ids as (visited counter) as we are exploring the graph. - ids[at] = low[at] + fun dfs(at: Node) { + currentLowValueForEachNode[at] = ++exploringCounter + assignedLowValueForEachNode[at] = exploringCounter + visitedNodes.add(at) stack.addFirst(at) - visited[at] = true - for (to in graphOfInts[at]) { - if (ids[to] == NOT_VISITED) { - dfs(to) + nodesCurrentlyInStack.add(at) + + diGraph[at]?.forEach { + if (!visitedNodes.contains(it)) { + dfs(it) } - if (visited[to]) { - low[at] = min(low[at], low[to]) + + if (nodesCurrentlyInStack.contains(it)) { + currentLowValueForEachNode[at] = + min(currentLowValueForEachNode[at]!!, currentLowValueForEachNode[it]!!) } } - // 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() + // We have found the head node in the scc. + if (currentLowValueForEachNode[at] == assignedLowValueForEachNode[at]) { + val connected = mutableListOf() while (true) { - visited[node] = false - sccs[node] = sccCount - if (node == at) break - node = stack.removeFirst() + val node = stack.removeFirst() + connected.add(node) + nodesCurrentlyInStack.remove(node) + if (node == at || stack.isEmpty()) break } - sccCount++ + connectedComponents.add(connected.reversed()) } } - for (i in 0 until nodeCount) { - if (ids[i] == NOT_VISITED) { - dfs(i) + diGraph.keys.forEach { + if (!visitedNodes.contains(it)) { + dfs(it) } } - // The algorithm discovers the scc in topological order, so use a sorted map to maintain the - // order. - val lowLinkToConnectedMap = sortedMapOf>() - for (i in 0 until nodeCount) { - if (!lowLinkToConnectedMap.containsKey(sccs[i])) { - lowLinkToConnectedMap[sccs[i]] = mutableListOf() - } - lowLinkToConnectedMap[sccs[i]]!!.add(i) - } - return lowLinkToConnectedMap.values.map { it.mapNotNull { intToNode[it] } } + return connectedComponents } } diff --git a/engine/src/test/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatchesTest.kt b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatchesTest.kt index 8ea23f95ef..382dd65e71 100644 --- a/engine/src/test/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatchesTest.kt +++ b/engine/src/test/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatchesTest.kt @@ -43,13 +43,14 @@ class StronglyConnectedPatchesTest { assertThat(result) .containsExactly( - listOf("0"), listOf("1", "2"), + listOf("0"), listOf("3", "4", "5"), - listOf("7"), listOf("8"), + listOf("7"), listOf("6"), ) + .inOrder() } @Test From 31d809821ab923074bcf182124123a2864196486 Mon Sep 17 00:00:00 2001 From: Aditya Khajuria Date: Tue, 11 Jun 2024 14:59:22 +0530 Subject: [PATCH 12/16] Review comments: Updated docs --- .../fhir/sync/upload/patch/PatchGenerator.kt | 3 +++ .../fhir/sync/upload/patch/PatchOrdering.kt | 21 +++++++++++++++++-- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchGenerator.kt index 54785cb0d1..00b9347c42 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchGenerator.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchGenerator.kt @@ -72,5 +72,8 @@ internal data class PatchMapping( * Structure to describe the cyclic nature of [PatchMapping]. * - A single value in [patchMappings] signifies the acyclic nature of the node. * - Multiple values in [patchMappings] signifies the cyclic nature of the nodes among themselves. + * + * [PatchMappingGroup] is used by the engine to make sure that related resources get uploaded to the + * server in the same request to maintain the referential integrity of resources during creation. */ internal data class PatchMappingGroup(val patchMappings: List) diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt index 75aab482a5..6522ec0f9e 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt @@ -20,9 +20,26 @@ import androidx.annotation.VisibleForTesting import com.google.android.fhir.db.Database import com.google.android.fhir.db.LocalChangeResourceReference -typealias Node = String +/** Represents a resource e.g. 'Patient/123' , 'Encounter/123'. */ +internal typealias Node = String -typealias Graph = Map> +/** + * Represents a collection of resources with reference to other resource represented as an edge. + * e.g. Two Patient resources p1 and p2, each with an encounter and subsequent observation will be + * represented as follows + * + * ``` + * [ + * 'Patient/p1' : [], + * 'Patient/p2' : [], + * 'Encounter/e1' : ['Patient/p1'], // Encounter.subject + * 'Encounter/e2' : ['Patient/p2'], // Encounter.subject + * 'Observation/o1' : ['Patient/p1', 'Encounter/e1'], // Observation.subject, Observation.encounter + * 'Observation/o2' : ['Patient/p2', 'Encounter/e2'], // Observation.subject, Observation.encounter + * ] + * ``` + */ +internal typealias Graph = Map> /** * Orders the [PatchMapping]s to maintain referential integrity during upload. From bfcd0c956613f957409bee65b5e1ea23ae0d6872 Mon Sep 17 00:00:00 2001 From: Aditya Khajuria Date: Thu, 13 Jun 2024 12:36:12 +0530 Subject: [PATCH 13/16] Review comments: refactored code and renamed some classes --- .../fhir/sync/upload/patch/PatchGenerator.kt | 13 ++++---- .../fhir/sync/upload/patch/PatchOrdering.kt | 16 +++++----- .../upload/patch/PerChangePatchGenerator.kt | 6 ++-- .../upload/patch/PerResourcePatchGenerator.kt | 8 +++-- .../upload/patch/StronglyConnectedPatches.kt | 31 ++++++++++--------- .../request/TransactionBundleGenerator.kt | 17 +++++----- .../upload/request/UploadRequestGenerator.kt | 10 +++--- .../upload/request/UrlRequestGenerator.kt | 8 ++--- .../upload/request/IndividualGeneratorTest.kt | 12 +++---- .../request/TransactionBundleGeneratorTest.kt | 22 ++++++------- 10 files changed, 76 insertions(+), 67 deletions(-) diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchGenerator.kt index 00b9347c42..6dfda6e191 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchGenerator.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchGenerator.kt @@ -20,8 +20,8 @@ import com.google.android.fhir.LocalChange import com.google.android.fhir.db.Database /** - * Generates [Patch]es from [LocalChange]s and output [List<[PatchMappingGroup]>] to keep a mapping - * of the [LocalChange]s to their corresponding generated [Patch] + * Generates [Patch]es from [LocalChange]s and output [List<[StronglyConnectedPatchMappings]>] 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 * together with other components in the upload package to fulfill a specific upload strategy. @@ -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): List + suspend fun generate(localChanges: List): List } internal object PatchGeneratorFactory { @@ -73,7 +73,8 @@ internal data class PatchMapping( * - A single value in [patchMappings] signifies the acyclic nature of the node. * - Multiple values in [patchMappings] signifies the cyclic nature of the nodes among themselves. * - * [PatchMappingGroup] is used by the engine to make sure that related resources get uploaded to the - * server in the same request to maintain the referential integrity of resources during creation. + * [StronglyConnectedPatchMappings] is used by the engine to make sure that related resources get + * uploaded to the server in the same request to maintain the referential integrity of resources + * during creation. */ -internal data class PatchMappingGroup(val patchMappings: List) +internal data class StronglyConnectedPatchMappings(val patchMappings: List) diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt index 6522ec0f9e..d0bfb5f364 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt @@ -72,15 +72,15 @@ 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 [PatchMappingGroup] containing: - * - [PatchMappingGroup] with single value for the [PatchMapping] based on the references to other - * [PatchMapping] if the mappings are acyclic - * - [PatchMappingGroup] with multiple values for [PatchMapping]s based on the references to other - * [PatchMapping]s if the mappings are cyclic. + * @return A ordered list of the [StronglyConnectedPatchMappings] containing: + * - [StronglyConnectedPatchMappings] with single value for the [PatchMapping] based on the + * references to other [PatchMapping] if the mappings are acyclic + * - [StronglyConnectedPatchMappings] with multiple values for [PatchMapping]s based on the + * references to other [PatchMapping]s if the mappings are cyclic. */ - suspend fun List.orderByReferences( + suspend fun List.sccOrderByReferences( database: Database, - ): List { + ): List { 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 @@ -93,7 +93,7 @@ internal object PatchOrdering { val adjacencyList = createAdjacencyListForCreateReferences(localChangeIdToResourceReferenceMap) return StronglyConnectedPatches.scc(adjacencyList).map { - PatchMappingGroup(it.mapNotNull { resourceIdToPatchMapping[it] }) + StronglyConnectedPatchMappings(it.mapNotNull { resourceIdToPatchMapping[it] }) } } diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerChangePatchGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerChangePatchGenerator.kt index 4c59650357..c208e8c6ea 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerChangePatchGenerator.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerChangePatchGenerator.kt @@ -25,7 +25,9 @@ import com.google.android.fhir.LocalChange * maintain an audit trail. */ internal object PerChangePatchGenerator : PatchGenerator { - override suspend fun generate(localChanges: List): List = + override suspend fun generate( + localChanges: List, + ): List = localChanges .map { PatchMapping( @@ -41,5 +43,5 @@ internal object PerChangePatchGenerator : PatchGenerator { ), ) } - .map { PatchMappingGroup(listOf(it)) } + .map { StronglyConnectedPatchMappings(listOf(it)) } } diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGenerator.kt index 55ad19a300..c524b32302 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGenerator.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/PerResourcePatchGenerator.kt @@ -23,7 +23,7 @@ import com.github.fge.jsonpatch.JsonPatch import com.google.android.fhir.LocalChange import com.google.android.fhir.LocalChange.Type import com.google.android.fhir.db.Database -import com.google.android.fhir.sync.upload.patch.PatchOrdering.orderByReferences +import com.google.android.fhir.sync.upload.patch.PatchOrdering.sccOrderByReferences /** * Generates a [Patch] for all [LocalChange]es made to a single FHIR resource. @@ -35,8 +35,10 @@ 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): List { - return generateSquashedChangesMapping(localChanges).orderByReferences(database) + override suspend fun generate( + localChanges: List, + ): List { + return generateSquashedChangesMapping(localChanges).sccOrderByReferences(database) } @androidx.annotation.VisibleForTesting diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt index 99f07d3380..d4a13dc19c 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt @@ -31,18 +31,22 @@ internal object StronglyConnectedPatches { return findSCCWithTarjan(directedGraph) } + /** + * Finds strongly connected components in topological order. See + * https://en.wikipedia.org/wiki/Tarjan%27s_strongly_connected_components_algorithm. + */ private fun findSCCWithTarjan(diGraph: Graph): List> { - val connectedComponents = mutableListOf>() - val currentLowValueForEachNode = mutableMapOf() + val sccs = mutableListOf>() + val lowLinks = mutableMapOf() var exploringCounter = 0 - val assignedLowValueForEachNode = mutableMapOf() + val discoveryTimes = mutableMapOf() val nodesCurrentlyInStack = mutableSetOf() val visitedNodes = mutableSetOf() val stack = ArrayDeque() fun dfs(at: Node) { - currentLowValueForEachNode[at] = ++exploringCounter - assignedLowValueForEachNode[at] = exploringCounter + lowLinks[at] = ++exploringCounter + discoveryTimes[at] = exploringCounter visitedNodes.add(at) stack.addFirst(at) nodesCurrentlyInStack.add(at) @@ -53,21 +57,20 @@ internal object StronglyConnectedPatches { } if (nodesCurrentlyInStack.contains(it)) { - currentLowValueForEachNode[at] = - min(currentLowValueForEachNode[at]!!, currentLowValueForEachNode[it]!!) + lowLinks[at] = min(lowLinks[at]!!, lowLinks[it]!!) } } // We have found the head node in the scc. - if (currentLowValueForEachNode[at] == assignedLowValueForEachNode[at]) { + if (lowLinks[at] == discoveryTimes[at]) { val connected = mutableListOf() - while (true) { - val node = stack.removeFirst() + var node: Node + do { + node = stack.removeFirst() connected.add(node) nodesCurrentlyInStack.remove(node) - if (node == at || stack.isEmpty()) break - } - connectedComponents.add(connected.reversed()) + } while (node != at && stack.isNotEmpty()) + sccs.add(connected.reversed()) } } @@ -77,6 +80,6 @@ internal object StronglyConnectedPatches { } } - return connectedComponents + return sccs } } diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/request/TransactionBundleGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/request/TransactionBundleGenerator.kt index d3309790b9..d622eee5d9 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/request/TransactionBundleGenerator.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/request/TransactionBundleGenerator.kt @@ -19,7 +19,7 @@ package com.google.android.fhir.sync.upload.request import com.google.android.fhir.LocalChange import com.google.android.fhir.sync.upload.patch.Patch import com.google.android.fhir.sync.upload.patch.PatchMapping -import com.google.android.fhir.sync.upload.patch.PatchMappingGroup +import com.google.android.fhir.sync.upload.patch.StronglyConnectedPatchMappings import org.hl7.fhir.r4.model.Bundle /** Generates list of [BundleUploadRequest] of type Transaction [Bundle] from the [Patch]es */ @@ -32,16 +32,17 @@ internal class TransactionBundleGenerator( /** * In order to accommodate cyclic dependencies between [PatchMapping]s and maintain referential - * integrity on the server, the [PatchMapping]s in a [PatchMappingGroup] are all put in a single - * [BundleUploadRequestMapping]. Based on the [generatedBundleSize], the remaining space of the - * [BundleUploadRequestMapping] maybe filled with other [PatchMappingGroup] mappings. + * integrity on the server, the [PatchMapping]s in a [StronglyConnectedPatchMappings] are all put + * in a single [BundleUploadRequestMapping]. Based on the [generatedBundleSize], the remaining + * space of the [BundleUploadRequestMapping] maybe filled with other + * [StronglyConnectedPatchMappings] mappings. * - * In case a single [PatchMappingGroup] has more [PatchMapping]s than the [generatedBundleSize], - * [generatedBundleSize] will be ignored so that all of the dependent mappings in - * [PatchMappingGroup] can be sent in a single [Bundle]. + * In case a single [StronglyConnectedPatchMappings] has more [PatchMapping]s than the + * [generatedBundleSize], [generatedBundleSize] will be ignored so that all of the dependent + * mappings in [StronglyConnectedPatchMappings] can be sent in a single [Bundle]. */ override fun generateUploadRequests( - mappedPatches: List, + mappedPatches: List, ): List { val mappingsPerBundle = mutableListOf>() diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/request/UploadRequestGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/request/UploadRequestGenerator.kt index 15c852ab64..86af7a4b64 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/request/UploadRequestGenerator.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/request/UploadRequestGenerator.kt @@ -19,20 +19,20 @@ package com.google.android.fhir.sync.upload.request import com.google.android.fhir.LocalChange import com.google.android.fhir.sync.upload.patch.Patch import com.google.android.fhir.sync.upload.patch.PatchMapping -import com.google.android.fhir.sync.upload.patch.PatchMappingGroup +import com.google.android.fhir.sync.upload.patch.StronglyConnectedPatchMappings 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<[PatchMappingGroup]>]. Any implementation of this generator is expected to output - * [List<[UploadRequestMapping]>] which maps [UploadRequest] to the corresponding [LocalChange]s it - * was generated from. + * [List<[StronglyConnectedPatchMappings]>]. 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, + mappedPatches: List, ): List } diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/request/UrlRequestGenerator.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/request/UrlRequestGenerator.kt index cdf7bd6bac..8e576760c3 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/request/UrlRequestGenerator.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/request/UrlRequestGenerator.kt @@ -21,7 +21,7 @@ import ca.uhn.fhir.context.FhirVersionEnum import com.google.android.fhir.ContentTypes import com.google.android.fhir.sync.upload.patch.Patch import com.google.android.fhir.sync.upload.patch.PatchMapping -import com.google.android.fhir.sync.upload.patch.PatchMappingGroup +import com.google.android.fhir.sync.upload.patch.StronglyConnectedPatchMappings import org.hl7.fhir.r4.model.Binary import org.hl7.fhir.r4.model.Resource import org.hl7.fhir.r4.model.codesystems.HttpVerb @@ -33,8 +33,8 @@ internal class UrlRequestGenerator( /** * Since a [UrlUploadRequest] can only handle a single resource request, the - * [PatchMappingGroup.patchMappings] are flattened and handled as acyclic mapping to generate - * [UrlUploadRequestMapping] for each [PatchMapping]. + * [StronglyConnectedPatchMappings.patchMappings] are flattened and handled as acyclic mapping to + * generate [UrlUploadRequestMapping] for each [PatchMapping]. * * **NOTE** * @@ -44,7 +44,7 @@ internal class UrlRequestGenerator( * server has strict referential integrity and the requests have cyclic dependency amongst itself. */ override fun generateUploadRequests( - mappedPatches: List, + mappedPatches: List, ): List = mappedPatches .map { it.patchMappings } diff --git a/engine/src/test/java/com/google/android/fhir/sync/upload/request/IndividualGeneratorTest.kt b/engine/src/test/java/com/google/android/fhir/sync/upload/request/IndividualGeneratorTest.kt index 5bb608418b..92cded4e1d 100644 --- a/engine/src/test/java/com/google/android/fhir/sync/upload/request/IndividualGeneratorTest.kt +++ b/engine/src/test/java/com/google/android/fhir/sync/upload/request/IndividualGeneratorTest.kt @@ -17,7 +17,7 @@ package com.google.android.fhir.sync.upload.request import com.google.android.fhir.sync.upload.patch.PatchMapping -import com.google.android.fhir.sync.upload.patch.PatchMappingGroup +import com.google.android.fhir.sync.upload.patch.StronglyConnectedPatchMappings import com.google.android.fhir.sync.upload.request.RequestGeneratorTestUtils.deleteLocalChange import com.google.android.fhir.sync.upload.request.RequestGeneratorTestUtils.insertionLocalChange import com.google.android.fhir.sync.upload.request.RequestGeneratorTestUtils.toPatch @@ -50,7 +50,7 @@ class IndividualGeneratorTest { ) val requests = generator.generateUploadRequests( - listOf(PatchMappingGroup(listOf(patchOutput))), + listOf(StronglyConnectedPatchMappings(listOf(patchOutput))), ) with(requests.single()) { @@ -73,7 +73,7 @@ class IndividualGeneratorTest { ) val requests = generator.generateUploadRequests( - listOf(PatchMappingGroup(listOf(patchOutput))), + listOf(StronglyConnectedPatchMappings(listOf(patchOutput))), ) with(requests.single()) { @@ -95,7 +95,7 @@ class IndividualGeneratorTest { val generator = UrlRequestGenerator.Factory.getDefault() val requests = generator.generateUploadRequests( - listOf(PatchMappingGroup(listOf(patchOutput))), + listOf(StronglyConnectedPatchMappings(listOf(patchOutput))), ) with(requests.single()) { with(generatedRequest) { @@ -121,7 +121,7 @@ class IndividualGeneratorTest { val generator = UrlRequestGenerator.Factory.getDefault() val requests = generator.generateUploadRequests( - listOf(PatchMappingGroup(listOf(patchOutput))), + listOf(StronglyConnectedPatchMappings(listOf(patchOutput))), ) with(requests.single()) { with(generatedRequest) { @@ -141,7 +141,7 @@ class IndividualGeneratorTest { val generator = UrlRequestGenerator.Factory.getDefault() val result = generator.generateUploadRequests( - patchOutputList.map { PatchMappingGroup(listOf(it)) }, + patchOutputList.map { StronglyConnectedPatchMappings(listOf(it)) }, ) assertThat(result).hasSize(3) assertThat(result.map { it.generatedRequest.httpVerb }) diff --git a/engine/src/test/java/com/google/android/fhir/sync/upload/request/TransactionBundleGeneratorTest.kt b/engine/src/test/java/com/google/android/fhir/sync/upload/request/TransactionBundleGeneratorTest.kt index 037f19e92f..998ffcb4e5 100644 --- a/engine/src/test/java/com/google/android/fhir/sync/upload/request/TransactionBundleGeneratorTest.kt +++ b/engine/src/test/java/com/google/android/fhir/sync/upload/request/TransactionBundleGeneratorTest.kt @@ -19,7 +19,7 @@ package com.google.android.fhir.sync.upload.request import com.google.android.fhir.LocalChange import com.google.android.fhir.LocalChangeToken import com.google.android.fhir.sync.upload.patch.PatchMapping -import com.google.android.fhir.sync.upload.patch.PatchMappingGroup +import com.google.android.fhir.sync.upload.patch.StronglyConnectedPatchMappings import com.google.android.fhir.sync.upload.request.RequestGeneratorTestUtils.deleteLocalChange import com.google.android.fhir.sync.upload.request.RequestGeneratorTestUtils.insertionLocalChange import com.google.android.fhir.sync.upload.request.RequestGeneratorTestUtils.toPatch @@ -51,7 +51,7 @@ class TransactionBundleGeneratorTest { val patches = listOf(insertionLocalChange, updateLocalChange, deleteLocalChange) .map { PatchMapping(listOf(it), it.toPatch()) } - .map { PatchMappingGroup(listOf(it)) } + .map { StronglyConnectedPatchMappings(listOf(it)) } val generator = TransactionBundleGenerator.Factory.getDefault() val result = generator.generateUploadRequests(patches) @@ -75,7 +75,7 @@ class TransactionBundleGeneratorTest { val patches = listOf(insertionLocalChange, updateLocalChange, deleteLocalChange) .map { PatchMapping(listOf(it), it.toPatch()) } - .map { PatchMappingGroup(listOf(it)) } + .map { StronglyConnectedPatchMappings(listOf(it)) } val generator = TransactionBundleGenerator.Factory.getGenerator( Bundle.HTTPVerb.PUT, @@ -123,7 +123,7 @@ class TransactionBundleGeneratorTest { generatedPatch = localChange.toPatch(), ), ) - .map { PatchMappingGroup(listOf(it)) } + .map { StronglyConnectedPatchMappings(listOf(it)) } val generator = TransactionBundleGenerator.Factory.getDefault(useETagForUpload = false) val result = generator.generateUploadRequests(patches) @@ -152,7 +152,7 @@ class TransactionBundleGeneratorTest { generatedPatch = localChange.toPatch(), ), ) - .map { PatchMappingGroup(listOf(it)) } + .map { StronglyConnectedPatchMappings(listOf(it)) } val generator = TransactionBundleGenerator.Factory.getDefault(useETagForUpload = true) val result = generator.generateUploadRequests(patches) @@ -189,7 +189,7 @@ class TransactionBundleGeneratorTest { val patches = localChanges .map { PatchMapping(listOf(it), it.toPatch()) } - .map { PatchMappingGroup(listOf(it)) } + .map { StronglyConnectedPatchMappings(listOf(it)) } val generator = TransactionBundleGenerator.Factory.getDefault(useETagForUpload = true) val result = generator.generateUploadRequests(patches) @@ -365,7 +365,7 @@ class TransactionBundleGeneratorTest { generatedPatch = localChange.toPatch(), ) } - .let { PatchMappingGroup(it) } + .let { StronglyConnectedPatchMappings(it) } val generator = TransactionBundleGenerator.Factory.getDefault(useETagForUpload = false, bundleSize = 5) val result = generator.generateUploadRequests(listOf(patchGroups)) @@ -388,7 +388,7 @@ class TransactionBundleGeneratorTest { ) val firstGroup = - PatchMappingGroup( + StronglyConnectedPatchMappings( mutableListOf().apply { for (i in 1..5) { add( @@ -408,7 +408,7 @@ class TransactionBundleGeneratorTest { ) val secondGroup = - PatchMappingGroup( + StronglyConnectedPatchMappings( listOf( PatchMapping( localChanges = @@ -421,7 +421,7 @@ class TransactionBundleGeneratorTest { ) val thirdGroup = - PatchMappingGroup( + StronglyConnectedPatchMappings( listOf( PatchMapping( localChanges = @@ -433,7 +433,7 @@ class TransactionBundleGeneratorTest { ), ) val fourthGroup = - PatchMappingGroup( + StronglyConnectedPatchMappings( mutableListOf().apply { for (i in 9..13) { add( From 39c0173c634c6ec0e8c31abc2905ad8df8fd393d Mon Sep 17 00:00:00 2001 From: Aditya Khajuria Date: Thu, 13 Jun 2024 13:22:18 +0530 Subject: [PATCH 14/16] Refactor --- .../upload/patch/StronglyConnectedPatches.kt | 29 +++++++++++++------ 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt index d4a13dc19c..496207d9f0 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt @@ -36,27 +36,38 @@ internal object StronglyConnectedPatches { * https://en.wikipedia.org/wiki/Tarjan%27s_strongly_connected_components_algorithm. */ private fun findSCCWithTarjan(diGraph: Graph): List> { + val nodeCount = (diGraph.keys + diGraph.values.flatten().toSet()).size val sccs = mutableListOf>() val lowLinks = mutableMapOf() var exploringCounter = 0 val discoveryTimes = mutableMapOf() - val nodesCurrentlyInStack = mutableSetOf() - val visitedNodes = mutableSetOf() + + val visitedNodes = BooleanArray(nodeCount) + val nodesCurrentlyInStack = BooleanArray(nodeCount) val stack = ArrayDeque() + fun visited(node: Node) = discoveryTimes[node]?.let { visitedNodes[it] } ?: false + + /** + * Discovery time of each Node is unique, starts with 0 and is linearly incremented. Thus, it + * can be used to map (index) each node in an array. + */ + fun Node.discoveryIndex() = discoveryTimes[this] ?: -1 + fun dfs(at: Node) { - lowLinks[at] = ++exploringCounter + lowLinks[at] = exploringCounter discoveryTimes[at] = exploringCounter - visitedNodes.add(at) + visitedNodes[exploringCounter] = true + exploringCounter++ stack.addFirst(at) - nodesCurrentlyInStack.add(at) + nodesCurrentlyInStack[at.discoveryIndex()] = true diGraph[at]?.forEach { - if (!visitedNodes.contains(it)) { + if (!visited(it)) { dfs(it) } - if (nodesCurrentlyInStack.contains(it)) { + if (nodesCurrentlyInStack[it.discoveryIndex()]) { lowLinks[at] = min(lowLinks[at]!!, lowLinks[it]!!) } } @@ -68,14 +79,14 @@ internal object StronglyConnectedPatches { do { node = stack.removeFirst() connected.add(node) - nodesCurrentlyInStack.remove(node) + nodesCurrentlyInStack[node.discoveryIndex()] = false } while (node != at && stack.isNotEmpty()) sccs.add(connected.reversed()) } } diGraph.keys.forEach { - if (!visitedNodes.contains(it)) { + if (!visited(it)) { dfs(it) } } From 0de1b7aca49dddcc0c1404020dfb87fdc439d3b1 Mon Sep 17 00:00:00 2001 From: Aditya Khajuria Date: Thu, 13 Jun 2024 15:42:05 +0530 Subject: [PATCH 15/16] Updated the code to use arrays --- .../upload/patch/StronglyConnectedPatches.kt | 42 +++++++++---------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt index 496207d9f0..ed7674b772 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt @@ -36,57 +36,57 @@ internal object StronglyConnectedPatches { * https://en.wikipedia.org/wiki/Tarjan%27s_strongly_connected_components_algorithm. */ private fun findSCCWithTarjan(diGraph: Graph): List> { - val nodeCount = (diGraph.keys + diGraph.values.flatten().toSet()).size + // Ideally the graph.keys should have all the nodes in the graph. But use values as well in case + // the input graph looks something like [ N1: [N2] ]. + val nodeToIndex = + (diGraph.keys + diGraph.values.flatten().toSet()) + .mapIndexed { index, s -> s to index } + .toMap() + val sccs = mutableListOf>() - val lowLinks = mutableMapOf() + val lowLinks = IntArray(nodeToIndex.size) var exploringCounter = 0 - val discoveryTimes = mutableMapOf() + val discoveryTimes = IntArray(nodeToIndex.size) - val visitedNodes = BooleanArray(nodeCount) - val nodesCurrentlyInStack = BooleanArray(nodeCount) + val visitedNodes = BooleanArray(nodeToIndex.size) + val nodesCurrentlyInStack = BooleanArray(nodeToIndex.size) val stack = ArrayDeque() - fun visited(node: Node) = discoveryTimes[node]?.let { visitedNodes[it] } ?: false - - /** - * Discovery time of each Node is unique, starts with 0 and is linearly incremented. Thus, it - * can be used to map (index) each node in an array. - */ - fun Node.discoveryIndex() = discoveryTimes[this] ?: -1 + fun Node.index() = nodeToIndex[this]!! fun dfs(at: Node) { - lowLinks[at] = exploringCounter - discoveryTimes[at] = exploringCounter + lowLinks[at.index()] = exploringCounter + discoveryTimes[at.index()] = exploringCounter visitedNodes[exploringCounter] = true exploringCounter++ stack.addFirst(at) - nodesCurrentlyInStack[at.discoveryIndex()] = true + nodesCurrentlyInStack[at.index()] = true diGraph[at]?.forEach { - if (!visited(it)) { + if (!visitedNodes[it.index()]) { dfs(it) } - if (nodesCurrentlyInStack[it.discoveryIndex()]) { - lowLinks[at] = min(lowLinks[at]!!, lowLinks[it]!!) + if (nodesCurrentlyInStack[it.index()]) { + lowLinks[at.index()] = min(lowLinks[at.index()], lowLinks[it.index()]) } } // We have found the head node in the scc. - if (lowLinks[at] == discoveryTimes[at]) { + if (lowLinks[at.index()] == discoveryTimes[at.index()]) { val connected = mutableListOf() var node: Node do { node = stack.removeFirst() connected.add(node) - nodesCurrentlyInStack[node.discoveryIndex()] = false + nodesCurrentlyInStack[node.index()] = false } while (node != at && stack.isNotEmpty()) sccs.add(connected.reversed()) } } diGraph.keys.forEach { - if (!visited(it)) { + if (!visitedNodes[it.index()]) { dfs(it) } } From e320d0095d27c2fc7bfa172dc00e522261877c0d Mon Sep 17 00:00:00 2001 From: Aditya Khajuria Date: Thu, 13 Jun 2024 16:14:26 +0530 Subject: [PATCH 16/16] Fixed failing test --- .../android/fhir/sync/upload/patch/StronglyConnectedPatches.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt index ed7674b772..216229492b 100644 --- a/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt +++ b/engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt @@ -57,7 +57,7 @@ internal object StronglyConnectedPatches { fun dfs(at: Node) { lowLinks[at.index()] = exploringCounter discoveryTimes[at.index()] = exploringCounter - visitedNodes[exploringCounter] = true + visitedNodes[at.index()] = true exploringCounter++ stack.addFirst(at) nodesCurrentlyInStack[at.index()] = true