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

[aditya-07]

IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).

Fixes #[issue number]

Description

1. Create a Directed Graph based on all the outgoing references for each Patch. Since the referential integrity comes to picture if the resource is not created yet, the edges of the graph are only the outgoing references to resources that are getting created during this upload. This may helps avoid some cycles as well.
2. The references are taken from LocalChangeResourceReferenceEntity which stores each outgoing references associated with a particular LocalChange as new entry.
3. Order the Patches so that if the resource A has outgoing references {B,C} (CREATE) and {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.

Alternative(s) considered

1. Splitting changes to CREATE first and subsequent UPDATE, but this would also require ordering the the create list incase it can overflow to multiple Bundles. This would be similar to the current approach but, would require multiple PUSH for the same resource incase the resource was Created and then Updated on the device before being pushed to the server.

Type
Choose one: Bug fix

Screenshots (if applicable)

Checklist

• I have read and acknowledged the Code of conduct.
• I have read the Contributing page.
• I have signed the Google Individual CLA, or I am covered by my company's Corporate CLA.
• I have discussed my proposed solution with code owners in the linked issue(s) and we have agreed upon the general approach.
• I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the style guide of this project.
• I have run ./gradlew check and ./gradlew connectedCheck to test my changes locally.
• I have built and run the demo app(s) to verify my change fixes the issue and/or does not break the demo app(s).

[MJ1998]

I feel we can improve complexity of this solution by using LocalChangeReferenceEntity table to fetch all the outgoing references.

[MJ1998]

Nice job stitching the LocalChangeResourceReferenceEntity here.

Although I dont like PatchGenerator having access to database which is from another package. I think we should abstract this out in a separate interface which is parent of the two packages: db and sync.
Maybe FhirSyncDbInteractor works well in this case as well. Will raise a PR soon so that this can be imagined better.

[MJ1998]

I feel this approach cannot solve for SingResource uploading strategy. Issue #2454.

But I also believe we should not discard this strategy as some fhir servers may not support bundle transaction and relying on SingleChange strategies can become expensive.

Currently for SingleResource upload strategy we fetch relevant LocalChanges from db, generate a patch and upload it.
What if we fetch all LocalChanges irrespective of the strategy, generate patches, order the patches according to this PR and then based on the upload strategy create upload requests ?!

@jingtang10 @aditya-07

[jingtang10]

thanks aditya! great job!

[MJ1998]

I think we can have a better design:-
Crux: Instead of fetching LocalChanges we will fetch LocalChange.id as we don't need full LocalChange objects to create a toposort of Patches.

Steps:-

1. fetchLocalChangeIds(): List<LocalChangeId>
2. orderByResourceTypeAndId(): Map<ResourceTypeWithId, List<LocalChangeId>>
   [ referencing ResourceTypeWithId as RTWI, and List<LocalChangeId> as IDPatch ]
   So orderByResourceTypeAndId(): Map<RTWI, IDPatch>
3. buildTopoSort(): List<Pair<RTWI, IDPatch>> (use database.getLocalChangeResourceReferenceIds)
4. Process this list based on UploadStrategy. For "single-resource" strategy process one by one, for "all-local-changes" process all of them at once. process = fetch whole of local changes: Flow<List<LocalChange>>
5. patchGenerator(List<LocalChange>>): List<Patch>
6. uploadRequestGenerator(List<Patch>): List<UploadRequest>

Pros:-
This will significantly reduce the memory consumed by delaying the fetching of full LocalChange objects.
Supports both upload strategies.

Con:-
One caveat could be double database fetching. But fetching just the LocalChangeIds should not take much time as its LocalChangeEntity's PrimaryKey.

I think given we want to support offline, memory is a more concerning issue than time complexity to sync here. But without profiling the two designs agains a db it's difficult to say.

Also, maybe we can take this up as a new issue and a new PR.

[jingtang10]

I think we can have a better design:- Crux: Instead of fetching LocalChanges we will fetch LocalChange.id as we don't need full LocalChange objects to create a toposort of Patches.

Steps:-

1. fetchLocalChangeIds(): List
2. orderByResourceTypeAndId(): Map<ResourceTypeWithId, List>
   set alias RTWI = ResourceTypeWithId
   set alias IDPatch = List
   So orderByResourceTypeAndId(): Map<RTWI, IDPatch>
3. buildTopoSort(): List<Pair<RTWI, IDPatch>> (use database.getLocalChangeResourceReferenceIds)
4. Process this list based on UploadStrategy. For "single-resource" strategy process one by one, for "all-local-changes" process all of them at once. process = fetch whole of local changes: Flow<List>
   Now our old approach
5. patchGenerator(List>): List
6. uploadRequestGenerator(List): List

Pros:- This will significantly reduce the memory consumed by delaying the fetching of full LocalChange objects. Supports both upload strategies.

Con:- One caveat could be double database fetching. But fetching just the LocalChangeIds should not take much time as its LocalChangeEntity's PrimaryKey.

I think given we want to support offline, memory is a more concerning issue than time complexity to sync here. But without profiling the two designs agains a db it's difficult to say.

Also, maybe we can take this up as a new issue and a new PR.

thanks for this suggestion jajoo! i'd like to understand a bit better the exact savings of this approach. but yes we should do this in a separate issue.

[jingtang10]

looks good - if jajoo is ok with this you can merge this aditya!

thanks!

[MJ1998]

LGTM

[LZRS]

Upload on sync fails here with

(1) row value misused in "SELECT *
        FROM LocalChangeResourceReferenceEntity
        WHERE localChangeId = (?,?,?,?,?,?,?,?,?,?,?)"
        
java.util.concurrent.ExecutionException: android.database.sqlite.SQLiteException: row value misused (code 1 SQLITE_ERROR): , while compiling: SELECT *
        FROM LocalChangeResourceReferenceEntity
        WHERE localChangeId = (?,?,?,?,?,?,?,?,?,?,?)
	at androidx.work.impl.utils.futures.AbstractFuture.getDoneValue(AbstractFuture.java:515)
	at androidx.work.impl.utils.futures.AbstractFuture.get(AbstractFuture.java:474)
	at androidx.work.impl.WorkerWrapper$2.run(WorkerWrapper.java:316)
	at androidx.work.impl.utils.SerialExecutorImpl$Task.run(SerialExecutorImpl.java:96)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:644)
	at java.lang.Thread.run(Thread.java:1012)
Caused by: android.database.sqlite.SQLiteException: row value misused (code 1 SQLITE_ERROR): , while compiling: SELECT *
        FROM LocalChangeResourceReferenceEntity
        WHERE localChangeId = (?,?,?,?,?,?,?,?,?,?,?)
	at android.database.sqlite.SQLiteConnection.nativePrepareStatement(Native Method)
	at android.database.sqlite.SQLiteConnection.acquirePreparedStatement(SQLiteConnection.java:1069)
	at android.database.sqlite.SQLiteConnection.prepare(SQLiteConnection.java:673)
	at android.database.sqlite.SQLiteSession.prepare(SQLiteSession.java:590)
	at android.database.sqlite.SQLiteProgram.<init>(SQLiteProgram.java:62)
	at android.database.sqlite.SQLiteQuery.<init>(SQLiteQuery.java:37)
	at android.database.sqlite.SQLiteDirectCursorDriver.query(SQLiteDirectCursorDriver.java:46)
	at android.database.sqlite.SQLiteDatabase.rawQueryWithFactory(SQLiteDatabase.java:1714)
	at android.database.sqlite.SQLiteDatabase.rawQueryWithFactory(SQLiteDatabase.java:1689)
	at androidx.sqlite.db.framework.FrameworkSQLiteDatabase.query(FrameworkSQLiteDatabase.kt:156)
	at androidx.room.RoomDatabase.query(RoomDatabase.kt:490)
	at androidx.room.util.DBUtil.query(DBUtil.kt:75)
	at com.google.android.fhir.db.impl.dao.LocalChangeDao_Impl$20.call(LocalChangeDao_Impl.java:927)
	at com.google.android.fhir.db.impl.dao.LocalChangeDao_Impl$20.call(LocalChangeDao_Impl.java:924)
	at androidx.room.CoroutinesRoom$Companion$execute$4$job$1.invokeSuspend(CoroutinesRoom.kt:88)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:108)
	... 3 more

Guessing the query should probably use an IN instead of =

[santosh-pingle]

Same here.

[santosh-pingle]

the temporary change below worked for me

SELECT *
        FROM LocalChangeResourceReferenceEntity
        WHERE localChangeId IN (:localChangeId)

[MJ1998]

Yes it should be IN. Strange the test case didn't track this.
Probably the function is being called with one LocalChangeId in the list.

@aditya-07 might wanna add / modify the test case to cover this scenario ?!