Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated PerResourcePatchGenerator to return ordered PatchMapping to avoid referential integrity issues. #2442

Merged
merged 14 commits into from
Mar 11, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Updated PerResourcePatchGenerator to return ordererd PatchMapping to …
…avoid referential integrity issues.
  • Loading branch information
aditya-07 committed Feb 15, 2024
commit ea57ccb4bf3e5cc0b1319261eb6c2aa6475f5c9f
aditya-07 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,12 @@ import com.google.android.fhir.LocalChange.Type
internal object PerResourcePatchGenerator : PatchGenerator {

override fun generate(localChanges: List<LocalChange>): List<PatchMapping> {
return localChanges
return generateSquashedChangesMapping(localChanges).orderByReferences()
}

@androidx.annotation.VisibleForTesting
internal fun generateSquashedChangesMapping(localChanges: List<LocalChange>) =
localChanges
.groupBy { it.resourceType to it.resourceId }
.values
.mapNotNull { resourceLocalChanges ->
Expand All @@ -44,7 +49,6 @@ internal object PerResourcePatchGenerator : PatchGenerator {
)
}
}
}

private fun mergeLocalChangesForSingleResource(localChanges: List<LocalChange>): Patch? {
// TODO (maybe this should throw exception when two entities don't have the same versionID)
Expand Down Expand Up @@ -139,3 +143,97 @@ internal object PerResourcePatchGenerator : PatchGenerator {
return objectMapper.writeValueAsString(mergedNode)
}
}

private typealias Node = String

/**
* @return - A ordered list of the [PatchMapping]s based on the references to other [PatchMapping]
* if the mappings are acyclic
* - throws [IllegalStateException] otherwise
*/
@androidx.annotation.VisibleForTesting
internal fun List<PatchMapping>.orderByReferences(): List<PatchMapping> {
// if the resource A has outgoing references (B,C) and these referenced resources are getting
// created on device,
// then these referenced resources (B,C) needs to go before the resource A so that referential
// integrity is retained.
val resourceIdToPatchMapping = associateBy { patchMapping ->
"${patchMapping.generatedPatch.resourceType}/${patchMapping.generatedPatch.resourceId}"
}

val adjacencyList =
createReferenceAdjacencyList(
resourceIdToPatchMapping.keys
.filter { resourceIdToPatchMapping[it]?.generatedPatch?.type == Patch.Type.INSERT }
.toSet(),
)
return createTopologicalOrderedList(adjacencyList).mapNotNull { resourceIdToPatchMapping[it] }
}

/**
* @return A map of [PatchMapping] to all the outgoing references to the other [PatchMapping]s of
* type [Patch.Type.INSERT] .
*/
@androidx.annotation.VisibleForTesting
internal fun List<PatchMapping>.createReferenceAdjacencyList(
aditya-07 marked this conversation as resolved.
Show resolved Hide resolved
insertResourceIds: Set<String>,
): Map<Node, List<Node>> {
val adjacencyList = mutableMapOf<Node, List<Node>>()
forEach { patchMapping ->
adjacencyList[
"${patchMapping.generatedPatch.resourceType}/${patchMapping.generatedPatch.resourceId}",
] = patchMapping.findOutgoingReferences().filter { insertResourceIds.contains(it) }
}
return adjacencyList
}

// if the outgoing reference is to a resource that's just an update and not create, then don't link
// to it. This may make the sub graphs smaller and also help avoid cyclic dependencies.
@androidx.annotation.VisibleForTesting
internal fun PatchMapping.findOutgoingReferences(): List<Node> {
aditya-07 marked this conversation as resolved.
Show resolved Hide resolved
val references = mutableListOf<Node>()
when (generatedPatch.type) {
Patch.Type.INSERT -> {
JsonLoader.fromString(generatedPatch.payload).search(references)
}
Patch.Type.UPDATE -> {
JsonLoader.fromString(generatedPatch.payload).forEach {
aditya-07 marked this conversation as resolved.
Show resolved Hide resolved
aditya-07 marked this conversation as resolved.
Show resolved Hide resolved
if (it.get("path").asText().endsWith("/reference")) {
references.add(it.get("value").asText())
} else {
it.get("value").search(references)
}
}
}
Patch.Type.DELETE -> {
// do nothing
}
}
return references
}

private fun JsonNode.search(reference: MutableList<String>) {
if (has("reference") && get("reference").isTextual) {
reference.add(get("reference").asText())
}
elements().forEach { it.search(reference) }
}

@androidx.annotation.VisibleForTesting
internal fun createTopologicalOrderedList(adjacencyList: Map<Node, List<Node>>): List<Node> {
val stack = ArrayDeque<String>()
val visited = mutableSetOf<String>()
val currentPath = mutableSetOf<String>()

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()
}
Loading
Loading