Handle local insert-delete sequence as no-op when squashing (#2133)

* Handle local insert-delete sequence as no-op in change squashing

* throw an exception instead of null

engine/src/main/java/com/google/android/fhir/LocalChange.kt

@@ -43,7 +43,8 @@ data class LocalChange(
  enum class Type(val value: Int) {
  INSERT(1), // create a new resource. payload is the entire resource json.
  UPDATE(2), // patch. payload is the json patch.
- DELETE(3); // delete. payload is empty string.
+ DELETE(3), // delete. payload is empty string.
+ NO_OP(4); // no-op. Discard
 
  companion object {
  fun from(input: Int): Type = values().first { it.value == input }

engine/src/main/java/com/google/android/fhir/db/impl/dao/LocalChangeUtils.kt

@@ -55,14 +55,35 @@ internal object LocalChangeUtils {
  )
  }
  }
- Type.DELETE -> {
- type = Type.DELETE
- payload = ""
- }
+ Type.DELETE ->
+ when (first.type) {
+ Type.INSERT -> {
+ // If an object is inserted and then deleted, return a special LocalChange that
+ // represents no-op
+ return LocalChange(
+ resourceId = second.resourceId,
+ resourceType = second.resourceType,
+ type = Type.NO_OP,
+ payload = "",
+ versionId = second.versionId,
+ token = LocalChangeToken(first.token.ids + second.token.ids),
+ timestamp = second.timestamp
+ )
+ }
+ else -> {
+ type = Type.DELETE
+ payload = ""
+ }
+ }
  Type.INSERT -> {
  type = Type.INSERT
  payload = second.payload
  }
+ Type.NO_OP -> {
+ throw IllegalArgumentException(
+ "Cannot merge local changes with type ${first.type} and ${second.type}."
+ )
+ }
  }
  return LocalChange(
  resourceId = second.resourceId,

engine/src/main/java/com/google/android/fhir/sync/upload/BundleEntryComponentGenerator.kt

@@ -64,6 +64,7 @@ abstract class BundleEntryComponentGenerator(
  LocalChange.Type.UPDATE,
  LocalChange.Type.DELETE -> ifMatch = "W/\"${localChange.versionId}\""
  LocalChange.Type.INSERT -> {}
+ LocalChange.Type.NO_OP -> error("Cannot create a bundle from a NO_OP request")
  }
  }
  }

engine/src/main/java/com/google/android/fhir/sync/upload/TransactionBundleGenerator.kt

@@ -44,12 +44,14 @@ open class TransactionBundleGenerator(
  val bundleRequest =
  Bundle().apply {
  type = Bundle.BundleType.TRANSACTION
- localChanges.forEach {
- this.addEntry(
- getBundleEntryComponentGeneratorForLocalChangeType(it.type, useETagForUpload)
- .getEntry(it)
- )
- }
+ localChanges
+ .filterNot { it.type == Type.NO_OP }
+ .forEach {
+ this.addEntry(
+ getBundleEntryComponentGeneratorForLocalChangeType(it.type, useETagForUpload)
+ .getEntry(it)
+ )
+ }
  }
  return BundleUploadRequest(
  resource = bundleRequest,
@@ -96,6 +98,8 @@ open class TransactionBundleGenerator(
  Type.INSERT -> HttpPutForCreateEntryComponentGenerator(useETagForUpload)
  Type.UPDATE -> HttpPatchForUpdateEntryComponentGenerator(useETagForUpload)
  Type.DELETE -> HttpDeleteEntryComponentGenerator(useETagForUpload)
+ Type.NO_OP ->
+ error("NO_OP type represents a no-operation and is not mapped to an HTTP operation.")
  }
  }
  }

engine/src/test/java/com/google/android/fhir/sync/upload/SquashedChangesUploadWorkManagerTest.kt

@@ -0,0 +1,218 @@
+/*
+ * Copyright 2023 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:https://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
+
+import ca.uhn.fhir.context.FhirContext
+import ca.uhn.fhir.context.FhirVersionEnum
+import com.google.android.fhir.LocalChange
+import com.google.android.fhir.db.impl.dao.LocalChangeToken
+import com.google.android.fhir.db.impl.dao.LocalChangeUtils
+import com.google.android.fhir.db.impl.dao.toLocalChange
+import com.google.android.fhir.db.impl.entities.LocalChangeEntity
+import com.google.common.truth.Truth.assertThat
+import java.time.Instant
+import org.hl7.fhir.r4.model.HumanName
+import org.hl7.fhir.r4.model.Patient
+import org.hl7.fhir.r4.model.ResourceType
+import org.junit.Assert.assertThrows
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.robolectric.RobolectricTestRunner
+
+@RunWith(RobolectricTestRunner::class)
+class SquashedChangesUploadWorkManagerTest {
+
+ private val jsonParser = FhirContext.forCached(FhirVersionEnum.R4).newJsonParser()
+
+ @Test
+ fun `prepareChangesForUpload should return no LocalChanges for same resource locally inserted and deleted`() {
+ val changes =
+ listOf(
+ LocalChangeEntity(
+ id = 1,
+ resourceType = ResourceType.Patient.name,
+ resourceId = "Patient-001",
+ type = LocalChangeEntity.Type.INSERT,
+ payload =
+ FhirContext.forCached(FhirVersionEnum.R4)
+ .newJsonParser()
+ .encodeResourceToString(
+ Patient().apply {
+ id = "Patient-001"
+ addName(
+ HumanName().apply {
+ addGiven("John")
+ family = "Doe"
+ }
+ )
+ }
+ ),
+ timestamp = Instant.now()
+ )
+ .toLocalChange()
+ .apply { LocalChangeToken(listOf(1)) },
+ LocalChangeEntity(
+ id = 2,
+ resourceType = ResourceType.Patient.name,
+ resourceId = "Patient-001",
+ type = LocalChangeEntity.Type.DELETE,
+ payload = "",
+ timestamp = Instant.now()
+ )
+ .toLocalChange()
+ .apply { LocalChangeToken(listOf(2)) }
+ )
+ val patchToUpload = SquashedChangesUploadWorkManager().prepareChangesForUpload(changes)
+
+ assertThat(patchToUpload).hasSize(1)
+ assertThat(patchToUpload.first().type).isEqualTo(LocalChange.Type.NO_OP)
+ }
+
+ @Test
+ fun `prepareChangesForUpload should squash change to NO_OP if insert is followed by an update, then a delete`() {
+ val changes =
+ listOf(
+ LocalChangeEntity(
+ id = 1,
+ resourceType = ResourceType.Patient.name,
+ resourceId = "Patient-001",
+ type = LocalChangeEntity.Type.INSERT,
+ payload =
+ FhirContext.forCached(FhirVersionEnum.R4)
+ .newJsonParser()
+ .encodeResourceToString(
+ Patient().apply {
+ id = "Patient-001"
+ addName(
+ HumanName().apply {
+ addGiven("John")
+ family = "Doe"
+ }
+ )
+ }
+ ),
+ timestamp = Instant.now()
+ )
+ .toLocalChange()
+ .apply { LocalChangeToken(listOf(1)) },
+ LocalChangeEntity(
+ id = 2,
+ resourceType = ResourceType.Patient.name,
+ resourceId = "Patient-001",
+ type = LocalChangeEntity.Type.UPDATE,
+ payload =
+ LocalChangeUtils.diff(
+ jsonParser,
+ Patient().apply {
+ id = "Patient-001"
+ addName(
+ HumanName().apply {
+ addGiven("Jane")
+ family = "Doe"
+ }
+ )
+ },
+ Patient().apply {
+ id = "Patient-001"
+ addName(
+ HumanName().apply {
+ addGiven("Janet")
+ family = "Doe"
+ }
+ )
+ }
+ )
+ .toString(),
+ timestamp = Instant.now()
+ )
+ .toLocalChange()
+ .apply { LocalChangeToken(listOf(1)) },
+ LocalChangeEntity(
+ id = 3,
+ resourceType = ResourceType.Patient.name,
+ resourceId = "Patient-001",
+ type = LocalChangeEntity.Type.DELETE,
+ payload = "",
+ timestamp = Instant.now()
+ )
+ .toLocalChange()
+ .apply { LocalChangeToken(listOf(3)) },
+ )
+ val patchToUpload = SquashedChangesUploadWorkManager().prepareChangesForUpload(changes)
+
+ assertThat(patchToUpload).hasSize(1)
+ assertThat(patchToUpload.first().type).isEqualTo(LocalChange.Type.NO_OP)
+ }
+
+ @Test
+ fun `prepareChangesForUpload should throw an error if a change is done after a resource is deleted locally`() {
+ val changes =
+ listOf(
+ LocalChangeEntity(
+ id = 1,
+ resourceType = ResourceType.Patient.name,
+ resourceId = "Patient-001",
+ type = LocalChangeEntity.Type.INSERT,
+ payload =
+ FhirContext.forCached(FhirVersionEnum.R4)
+ .newJsonParser()
+ .encodeResourceToString(
+ Patient().apply {
+ id = "Patient-001"
+ addName(
+ HumanName().apply {
+ addGiven("John")
+ family = "Doe"
+ }
+ )
+ }
+ ),
+ timestamp = Instant.now()
+ )
+ .toLocalChange()
+ .apply { LocalChangeToken(listOf(1)) },
+ LocalChangeEntity(
+ id = 2,
+ resourceType = ResourceType.Patient.name,
+ resourceId = "Patient-001",
+ type = LocalChangeEntity.Type.DELETE,
+ payload = "",
+ timestamp = Instant.now()
+ )
+ .toLocalChange()
+ .apply { LocalChangeToken(listOf(2)) },
+ LocalChangeEntity(
+ id = 3,
+ resourceType = ResourceType.Patient.name,
+ resourceId = "Patient-001",
+ type = LocalChangeEntity.Type.UPDATE,
+ payload = "",
+ timestamp = Instant.now()
+ )
+ .toLocalChange()
+ .apply { LocalChangeToken(listOf(3)) }
+ )
+
+ val errorMessage =
+ assertThrows(IllegalArgumentException::class.java) {
+ SquashedChangesUploadWorkManager().prepareChangesForUpload(changes)
+ }
+ .localizedMessage
+
+ assertThat(errorMessage).isEqualTo("Cannot merge local changes with type NO_OP and UPDATE.")
+ }
+}