google / android-fhir

The Android FHIR SDK is a set of Kotlin libraries for building offline-capable, mobile-first healthcare applications using the HL7® FHIR® standard on Android.
https://google.github.io/android-fhir/
Apache License 2.0
458 stars 238 forks source link

Sync upload local changes reordering patch issue #2500

Open ndegwamartin opened 1 month ago

ndegwamartin commented 1 month ago

Describe the bug After the latest updates to Sync, an exception is thrown on the code block/line here :

private 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)) {

The check throws an exception if it detects a cycle, however there could be valid cycles .

See screenshot (Shows breakpoint debugging of code block/line above )

Component Core library - Engine

To Reproduce Steps to reproduce the behavior:

  1. Trigger sync upload after a workflow that creates resources with a dependency tree as shown on the screenshot.

Expected behavior Sync upload should be successful

Screenshots Screenshot 2024-03-26 at 15 14 50

Screenshot shows breakpoint debugging of line here

Smartphone (please complete the following information): N/A

Additional context N/A

Would you like to work on the issue? @aditya-07 worked on the original issue

aditya-07 commented 1 month ago

@ndegwamartin Can you add some data for the valid cycle.

santosh-pingle commented 1 month ago

@ndegwamartin Did you share the data with @aditya-07 for the valid cycle? @ndegwamartin Do you mean the resources are already in sync, and it only throws exceptions when uploading updates during a valid cycle?

ndegwamartin commented 1 month ago

@aditya @santosh-pingle for context the above screenshot was captured after migrating to the latest codebase.

Attached is the SQL to recreate the state of the LocalChangeEntity and LocalChangeResourceReferenceEntity resources_db-LocalChanges.sql.zip . Let me know if you need data from any of the other tables.

I'm also happy to jump on a call to clarify further

ndegwamartin commented 1 month ago

@aditya-07 related to the above these are the resources(count by resource type) that could not be synced via Upload

Screenshot 2024-04-03

aditya-07 commented 1 month ago

@aditya @santosh-pingle for context the above screenshot was captured after migrating to the latest codebase.

Attached is the SQL to recreate the state of the LocalChangeEntity and LocalChangeResourceReferenceEntity resources_db-LocalChanges.sql.zip . Let me know if you need data from any of the other tables.

I'm also happy to jump on a call to clarify further

@ndegwamartin The .zip doesn't have sql to recreate LocalChangeResourceReferenceEntity .

ndegwamartin commented 1 month ago

@aditya-07 you are correct, appears the LocalChangeResourceReferenceEntity was not part of the archive. I've recreated the scripts with a new data set attached here - LocalChangesSQL.zip

Screenshot 2024-04-05 at 15 18 51
pld commented 1 month ago

hi @aditya-07 any update on progress here? Thank you!