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.

Sign up for GitHub

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

Jump to bottom

Configuration changes to accept update payload type #2687

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Configuration changes to accept update payload type #2687

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion demo/src/main/java/com/google/android/fhir/demo/data/DemoFhirSyncWorker.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import com.google.android.fhir.demo.FhirApplication
import com.google.android.fhir.sync.AcceptLocalConflictResolver
import com.google.android.fhir.sync.DownloadWorkManager
import com.google.android.fhir.sync.FhirSyncWorker
import com.google.android.fhir.sync.upload.UpdatePayloadType
import com.google.android.fhir.sync.upload.UploadStrategy

class DemoFhirSyncWorker(appContext: Context, workerParams: WorkerParameters) :
Expand All @@ -33,7 +34,8 @@ class DemoFhirSyncWorker(appContext: Context, workerParams: WorkerParameters) :

override fun getConflictResolver() = AcceptLocalConflictResolver

override fun getUploadStrategy(): UploadStrategy = UploadStrategy.AllChangesSquashedBundlePut
override fun getUploadStrategy(): UploadStrategy =
UploadStrategy.AllChangesSquashedBundlePut(UpdatePayloadType.PATCH, 500)

override fun getFhirEngine() = FhirApplication.fhirEngine(applicationContext)
}
7 changes: 6 additions & 1 deletion ...nchmark/src/androidTest/java/com/google/android/fhir/benchmark/FhirSyncWorkerBenchmark.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import com.google.android.fhir.sync.AcceptRemoteConflictResolver
import com.google.android.fhir.sync.DownloadWorkManager
import com.google.android.fhir.sync.FhirSyncWorker
import com.google.android.fhir.sync.download.DownloadRequest
import com.google.android.fhir.sync.upload.UpdatePayloadType
import com.google.android.fhir.sync.upload.UploadStrategy
import com.google.common.truth.Truth.assertThat
import java.math.BigDecimal
Expand Down Expand Up @@ -91,7 +92,11 @@ class FhirSyncWorkerBenchmark {

override fun getConflictResolver() = AcceptRemoteConflictResolver

override fun getUploadStrategy(): UploadStrategy = UploadStrategy.AllChangesSquashedBundlePut
override fun getUploadStrategy(): UploadStrategy =
UploadStrategy.AllChangesSquashedBundlePut(
UpdatePayloadType.PATCH,
500,
)
}

open class BenchmarkTestDownloadManagerImpl(queries: List<String> = listOf("List/sync-list")) :
Expand Down
7 changes: 6 additions & 1 deletion engine/src/androidTest/java/com/google/android/fhir/sync/SyncInstrumentedTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import androidx.work.WorkManager
import androidx.work.WorkerParameters
import androidx.work.testing.WorkManagerTestInitHelper
import com.google.android.fhir.FhirEngine
import com.google.android.fhir.sync.upload.UpdatePayloadType
import com.google.android.fhir.sync.upload.UploadStrategy
import com.google.android.fhir.testing.TestDataSourceImpl
import com.google.android.fhir.testing.TestDownloadManagerImpl
Expand Down Expand Up @@ -65,7 +66,11 @@ class SyncInstrumentedTest {

override fun getConflictResolver() = AcceptRemoteConflictResolver

override fun getUploadStrategy(): UploadStrategy = UploadStrategy.AllChangesSquashedBundlePut
override fun getUploadStrategy(): UploadStrategy =
UploadStrategy.AllChangesSquashedBundlePut(
UpdatePayloadType.PATCH,
500,
)
}

class TestSyncWorkerForDownloadFailing(appContext: Context, workerParams: WorkerParameters) :
Expand Down
87 changes: 72 additions & 15 deletions engine/src/main/java/com/google/android/fhir/sync/upload/UploadStrategy.kt
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we just remove some of the strategies that we are not going to implement - especially given that we're using this approach where the verbs are being passed to the strategy.

Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import org.hl7.fhir.r4.model.codesystems.HttpVerb
* [getUploadStrategy][com.google.android.fhir.sync.FhirSyncWorker.getUploadStrategy] in your app's
* [FhirSyncWorker][com.google.android.fhir.sync.FhirSyncWorker], for example:
* ```
* override fun getUploadStrategy(): UploadStrategy = UploadStrategy.AllChangesSquashedBundlePut
* override fun getUploadStrategy(): UploadStrategy = UploadStrategy.AllChangesSquashedBundlePut(UpdatePayloadType.PATCH, 500)
* ```
*
* The strategy you select depends on the server's capabilities (for example, support for `PUT` vs
Expand Down Expand Up @@ -62,22 +62,32 @@ private constructor(
* bundled PUT request. This strategy is efficient and minimizes the number of requests sent to
* the server, but does not maintain individual change history.
*/
data object AllChangesSquashedBundlePut :
data class AllChangesSquashedBundlePut(
val updatePayloadType: UpdatePayloadType,
val bundleSize: Int,
) :
UploadStrategy(
LocalChangesFetchMode.AllChanges,
PatchGeneratorMode.PerResource,
UploadRequestGeneratorMode.BundleRequest(Bundle.HTTPVerb.PUT, Bundle.HTTPVerb.PATCH),
UploadRequestGeneratorMode.BundleRequest(
httpVerbToUseForCreate = Bundle.HTTPVerb.PUT,
httpVerbToUseForUpdate = updatePayloadType.toBundleHttpVerb(),
bundleSize = bundleSize,
),
)

/**
* Fetches all changes for a single resource, generates a patch for that resource, and creates a
* single POST request with the patch.
*/
data object SingleResourcePost :
data class SingleResourcePost(val updatePayloadType: UpdatePayloadType) :
UploadStrategy(
LocalChangesFetchMode.PerResource,
PatchGeneratorMode.PerResource,
UploadRequestGeneratorMode.UrlRequest(HttpVerb.POST, HttpVerb.PATCH),
UploadRequestGeneratorMode.BundleRequest(
httpVerbToUseForCreate = Bundle.HTTPVerb.POST,
httpVerbToUseForUpdate = updatePayloadType.toBundleHttpVerb(),
),
)

/*
Expand All @@ -90,54 +100,101 @@ private constructor(
* Not yet implemented - Fetches all local changes, generates one patch per resource, and uploads
* them in a single bundled POST request.
*/
object AllChangesSquashedBundlePost :
private data class AllChangesSquashedBundlePost(
val updatePayloadType: UpdatePayloadType,
val bundleSize: Int,
) :
UploadStrategy(
LocalChangesFetchMode.AllChanges,
PatchGeneratorMode.PerResource,
UploadRequestGeneratorMode.BundleRequest(Bundle.HTTPVerb.POST, Bundle.HTTPVerb.PATCH),
UploadRequestGeneratorMode.BundleRequest(
Bundle.HTTPVerb.POST,
updatePayloadType.toBundleHttpVerb(),
),
)

/**
* Not yet implemented - Fetches the earliest local change, generates a patch for that change, and
* creates a single PUT request with the patch.
*/
private object SingleChangePut :
private data class SingleChangePut(val updatePayloadType: UpdatePayloadType) :
UploadStrategy(
LocalChangesFetchMode.EarliestChange,
PatchGeneratorMode.PerChange,
UploadRequestGeneratorMode.UrlRequest(HttpVerb.PUT, HttpVerb.PATCH),
UploadRequestGeneratorMode.UrlRequest(HttpVerb.PUT, updatePayloadType.toHttpVerb()),
)

/**
* Not yet implemented - Fetches the earliest local change, generates a patch for that change, and
* creates a single POST request with the patch.
*/
private object SingleChangePost :
private data class SingleChangePost(val updatePayloadType: UpdatePayloadType) :
UploadStrategy(
LocalChangesFetchMode.EarliestChange,
PatchGeneratorMode.PerChange,
UploadRequestGeneratorMode.UrlRequest(HttpVerb.POST, HttpVerb.PATCH),
UploadRequestGeneratorMode.UrlRequest(HttpVerb.PUT, updatePayloadType.toHttpVerb()),
)

/**
* Not yet implemented - Fetches all changes for a single resource, generates a patch for that
* resource, and creates a single PUT request with the patch.
*/
private object SingleResourcePut :
private data class SingleResourcePut(val updatePayloadType: UpdatePayloadType) :
UploadStrategy(
LocalChangesFetchMode.PerResource,
PatchGeneratorMode.PerResource,
UploadRequestGeneratorMode.UrlRequest(HttpVerb.PUT, HttpVerb.PATCH),
UploadRequestGeneratorMode.UrlRequest(HttpVerb.PUT, updatePayloadType.toHttpVerb()),
)

/**
* Not yet implemented - Fetches all local changes, generates a patch for each individual change,
* and creates a single bundle POST request containing all the patches.
*/
private object AllChangesBundlePost :
private data class AllChangesBundlePut(
val updatePayloadType: UpdatePayloadType,
val bundleSize: Int,
) :
UploadStrategy(
LocalChangesFetchMode.AllChanges,
PatchGeneratorMode.PerChange,
UploadRequestGeneratorMode.BundleRequest(Bundle.HTTPVerb.POST, Bundle.HTTPVerb.PATCH),
UploadRequestGeneratorMode.BundleRequest(
Bundle.HTTPVerb.PUT,
updatePayloadType.toBundleHttpVerb(),
),
)

/**
* Not yet implemented - Fetches all local changes, generates a patch for each individual change,
* and creates a single bundle POST request containing all the patches.
*/
private data class AllChangesBundlePost(
val updatePayloadType: UpdatePayloadType,
val bundleSize: Int,
) :
UploadStrategy(
LocalChangesFetchMode.AllChanges,
PatchGeneratorMode.PerChange,
UploadRequestGeneratorMode.BundleRequest(
Bundle.HTTPVerb.POST,
updatePayloadType.toBundleHttpVerb(),
),
)
}

enum class UpdatePayloadType {
PATCH,
FULL,
;

fun toBundleHttpVerb() =
when (this) {
PATCH -> Bundle.HTTPVerb.PATCH
FULL -> Bundle.HTTPVerb.PUT
}

fun toHttpVerb() =
when (this) {
PATCH -> HttpVerb.PATCH
FULL -> HttpVerb.PUT
}
}
7 changes: 4 additions & 3 deletions engine/src/test/java/com/google/android/fhir/impl/FhirEngineImplTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import com.google.android.fhir.sync.AcceptRemoteConflictResolver
import com.google.android.fhir.sync.ResourceSyncException
import com.google.android.fhir.sync.upload.ResourceUploadResponseMapping
import com.google.android.fhir.sync.upload.SyncUploadProgress
import com.google.android.fhir.sync.upload.UpdatePayloadType
import com.google.android.fhir.sync.upload.UploadRequestResult
import com.google.android.fhir.sync.upload.UploadStrategy
import com.google.android.fhir.testing.assertResourceEquals
Expand Down Expand Up @@ -323,7 +324,7 @@ class FhirEngineImplTest {
val emittedProgress = mutableListOf<SyncUploadProgress>()

fhirEngine
.syncUpload(UploadStrategy.AllChangesSquashedBundlePut) {
.syncUpload(UploadStrategy.AllChangesSquashedBundlePut(UpdatePayloadType.PATCH, 500)) {
localChanges.addAll(it)
flowOf(
UploadRequestResult.Success(
Expand Down Expand Up @@ -356,7 +357,7 @@ class FhirEngineImplTest {
val emittedProgress = mutableListOf<SyncUploadProgress>()
val uploadError = ResourceSyncException(ResourceType.Patient, FHIRException("Did not work"))
fhirEngine
.syncUpload(UploadStrategy.AllChangesSquashedBundlePut) {
.syncUpload(UploadStrategy.AllChangesSquashedBundlePut(UpdatePayloadType.PATCH, 500)) {
flowOf(
UploadRequestResult.Failure(
it,
Expand Down Expand Up @@ -767,7 +768,7 @@ class FhirEngineImplTest {
fun `test local changes are consumed when using POST upload strategy`() = runBlocking {
assertThat(services.database.getLocalChangesCount()).isEqualTo(1)
fhirEngine
.syncUpload(UploadStrategy.SingleResourcePost) {
.syncUpload(UploadStrategy.SingleResourcePost(UpdatePayloadType.PATCH)) {
flowOf(
UploadRequestResult.Success(
listOf(
Expand Down
13 changes: 10 additions & 3 deletions engine/src/test/java/com/google/android/fhir/sync/FhirSyncWorkerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import androidx.work.testing.TestListenableWorkerBuilder
import com.google.android.fhir.FhirEngine
import com.google.android.fhir.FhirEngineConfiguration
import com.google.android.fhir.FhirEngineProvider
import com.google.android.fhir.sync.upload.UpdatePayloadType
import com.google.android.fhir.sync.upload.UploadStrategy
import com.google.android.fhir.testing.TestDataSourceImpl
import com.google.android.fhir.testing.TestDownloadManagerImpl
Expand Down Expand Up @@ -56,7 +57,8 @@ internal class FhirSyncWorkerTest {

override fun getConflictResolver() = AcceptRemoteConflictResolver

override fun getUploadStrategy(): UploadStrategy = UploadStrategy.AllChangesSquashedBundlePut
override fun getUploadStrategy(): UploadStrategy =
UploadStrategy.AllChangesSquashedBundlePut(UpdatePayloadType.PATCH, 500)
}

class FailingPeriodicSyncWorker(appContext: Context, workerParams: WorkerParameters) :
Expand All @@ -70,7 +72,11 @@ internal class FhirSyncWorkerTest {

override fun getConflictResolver() = AcceptRemoteConflictResolver

override fun getUploadStrategy(): UploadStrategy = UploadStrategy.AllChangesSquashedBundlePut
override fun getUploadStrategy(): UploadStrategy =
UploadStrategy.AllChangesSquashedBundlePut(
UpdatePayloadType.PATCH,
500,
)
}

class FailingPeriodicSyncWorkerWithoutDataSource(
Expand All @@ -86,7 +92,8 @@ internal class FhirSyncWorkerTest {

override fun getConflictResolver() = AcceptRemoteConflictResolver

override fun getUploadStrategy(): UploadStrategy = UploadStrategy.AllChangesSquashedBundlePut
override fun getUploadStrategy(): UploadStrategy =
UploadStrategy.AllChangesSquashedBundlePut(UpdatePayloadType.PATCH, 500)
}

companion object {
Expand Down
11 changes: 9 additions & 2 deletions engine/src/test/java/com/google/android/fhir/sync/FhirSynchronizerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package com.google.android.fhir.sync

import com.google.android.fhir.sync.download.DownloadState
import com.google.android.fhir.sync.download.Downloader
import com.google.android.fhir.sync.upload.UpdatePayloadType
import com.google.android.fhir.sync.upload.UploadRequestResult
import com.google.android.fhir.sync.upload.UploadStrategy
import com.google.android.fhir.sync.upload.Uploader
Expand Down Expand Up @@ -60,7 +61,10 @@ class FhirSynchronizerTest {
fhirSynchronizer =
FhirSynchronizer(
TestFhirEngineImpl,
UploadConfiguration(uploader, UploadStrategy.AllChangesSquashedBundlePut),
UploadConfiguration(
uploader,
UploadStrategy.AllChangesSquashedBundlePut(UpdatePayloadType.PATCH, 500),
),
DownloadConfiguration(downloader, conflictResolver),
fhirDataStore,
)
Expand Down Expand Up @@ -159,7 +163,10 @@ class FhirSynchronizerTest {
val fhirSynchronizerWithDelayInDownload =
FhirSynchronizer(
TestFhirEngineImpl,
UploadConfiguration(uploader, UploadStrategy.AllChangesSquashedBundlePut),
UploadConfiguration(
uploader,
UploadStrategy.AllChangesSquashedBundlePut(UpdatePayloadType.PATCH, 500),
),
DownloadConfiguration(
object : Downloader {
override suspend fun download(): Flow<DownloadState> {
Expand Down
7 changes: 6 additions & 1 deletion engine/src/test/java/com/google/android/fhir/sync/SyncTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import android.content.Context
import androidx.work.BackoffPolicy
import androidx.work.WorkerParameters
import com.google.android.fhir.FhirEngine
import com.google.android.fhir.sync.upload.UpdatePayloadType
import com.google.android.fhir.sync.upload.UploadStrategy
import com.google.android.fhir.testing.TestDataSourceImpl
import com.google.android.fhir.testing.TestDownloadManagerImpl
Expand All @@ -45,7 +46,11 @@ class SyncTest {

override fun getConflictResolver() = AcceptRemoteConflictResolver

override fun getUploadStrategy(): UploadStrategy = UploadStrategy.AllChangesSquashedBundlePut
override fun getUploadStrategy(): UploadStrategy =
UploadStrategy.AllChangesSquashedBundlePut(
UpdatePayloadType.PATCH,
500,
)
}

@Test
Expand Down
5 changes: 3 additions & 2 deletions engine/src/test/java/com/google/android/fhir/sync/upload/ResourceConsolidatorFactoryTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class ResourceConsolidatorFactoryTest {
fun `return HttpPostResourceConsolidator instance `() {
val httpPostResourceConsolidator =
ResourceConsolidatorFactory.byHttpVerb(
UploadStrategy.SingleResourcePost.requestGeneratorMode,
UploadStrategy.SingleResourcePost(UpdatePayloadType.PATCH).requestGeneratorMode,
services.database,
)
assertTrue(httpPostResourceConsolidator is HttpPostResourceConsolidator)
Expand All @@ -42,7 +42,8 @@ class ResourceConsolidatorFactoryTest {
fun `return DefaultResourceConsolidator instance `() {
val httpPostResourceConsolidator =
ResourceConsolidatorFactory.byHttpVerb(
UploadStrategy.AllChangesSquashedBundlePut.requestGeneratorMode,
UploadStrategy.AllChangesSquashedBundlePut(UpdatePayloadType.PATCH, 500)
.requestGeneratorMode,
services.database,
)
assertTrue(httpPostResourceConsolidator is DefaultResourceConsolidator)
Expand Down
Loading