Add resource UUID to LocalChangeEntity, fixing changing UUID issue fo…

…r the same resource (#2210)

* Adding resource UUID in LocalChangeEntity

* fixing migration tests

* returning resource UUID for insert resource

* review comments

engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt

@@ -467,6 +467,21 @@ class DatabaseImplTest {
       .isTrue()
   }
 
+  @Test
+  fun insert_existingRemoteResource_shouldNotChangeResourceEntityUuidOrId() = runBlocking {
+    val patient: Patient = readFromFile(Patient::class.java, "/date_test_patient.json")
+    database.insertRemote(patient)
+    val patientEntityAfterFirstRemoteSync =
+      database.selectEntity(ResourceType.Patient, patient.logicalId)
+    database.insertRemote(patient)
+    val patientEntityAfterSecondRemoteSync =
+      database.selectEntity(ResourceType.Patient, patient.logicalId)
+    assertThat(patientEntityAfterSecondRemoteSync.resourceUuid)
+      .isEqualTo(patientEntityAfterFirstRemoteSync.resourceUuid)
+    assertThat(patientEntityAfterSecondRemoteSync.id)
+      .isEqualTo(patientEntityAfterFirstRemoteSync.id)
+  }
+
   @Test
   fun insert_remoteResource_shouldSaveVersionIdAndLastUpdated() = runBlocking {
     val patient =

engine/src/androidTest/java/com/google/android/fhir/db/impl/ResourceDatabaseMigrationTest.kt

@@ -16,7 +16,6 @@
 
 package com.google.android.fhir.db.impl
 
-import androidx.room.Room
 import androidx.room.testing.MigrationTestHelper
 import androidx.test.ext.junit.runners.AndroidJUnit4
 import androidx.test.platform.app.InstrumentationRegistry
@@ -30,7 +29,6 @@ import java.util.Date
 import kotlinx.coroutines.runBlocking
 import org.hl7.fhir.r4.model.HumanName
 import org.hl7.fhir.r4.model.Patient
-import org.hl7.fhir.r4.model.ResourceType
 import org.hl7.fhir.r4.model.Task
 import org.junit.Rule
 import org.junit.Test
@@ -69,14 +67,16 @@ class ResourceDatabaseMigrationTest {
 
     // Open latest version of the database. Room will validate the schema
     // once all migrations execute.
-    helper.runMigrationsAndValidate(DB_NAME, 2, true, MIGRATION_1_2)
+    val migratedDatabase = helper.runMigrationsAndValidate(DB_NAME, 2, true, MIGRATION_1_2)
 
     val readPatientJson: String?
-    getMigratedRoomDatabase().apply {
-      readPatientJson = this.resourceDao().getResource("migrate1-2-test", ResourceType.Patient)
-      openHelper.writableDatabase.close()
+    migratedDatabase.let { database ->
+      database.query("SELECT serializedResource FROM ResourceEntity").let {
+        it.moveToFirst()
+        readPatientJson = it.getString(0)
+      }
     }
-
+    migratedDatabase.close()
     assertThat(readPatientJson).isEqualTo(insertedPatientJson)
   }
 
@@ -101,14 +101,16 @@ class ResourceDatabaseMigrationTest {
     }
 
     // Re-open the database with version 3 and provide MIGRATION_2_3 as the migration process.
-    helper.runMigrationsAndValidate(DB_NAME, 3, true, MIGRATION_2_3)
+    val migratedDatabase = helper.runMigrationsAndValidate(DB_NAME, 3, true, MIGRATION_2_3)
 
     val retrievedTask: String?
-    getMigratedRoomDatabase().apply {
-      retrievedTask = this.resourceDao().getResource(taskId, ResourceType.Task)
-      openHelper.writableDatabase.close()
+    migratedDatabase.let { database ->
+      database.query("SELECT serializedResource FROM ResourceEntity").let {
+        it.moveToFirst()
+        retrievedTask = it.getString(0)
+      }
     }
-
+    migratedDatabase.close()
     assertThat(retrievedTask).isEqualTo(bedNetTask)
   }
 
@@ -133,14 +135,16 @@ class ResourceDatabaseMigrationTest {
     }
 
     // Re-open the database with version 4 and provide MIGRATION_3_4 as the migration process.
-    helper.runMigrationsAndValidate(DB_NAME, 4, true, MIGRATION_3_4)
+    val migratedDatabase = helper.runMigrationsAndValidate(DB_NAME, 4, true, MIGRATION_3_4)
 
     val retrievedTask: String?
-    getMigratedRoomDatabase().apply {
-      retrievedTask = this.resourceDao().getResource(taskId, ResourceType.Task)
-      openHelper.writableDatabase.close()
+    migratedDatabase.let { database ->
+      database.query("SELECT serializedResource FROM ResourceEntity").let {
+        it.moveToFirst()
+        retrievedTask = it.getString(0)
+      }
     }
-
+    migratedDatabase.close()
     assertThat(retrievedTask).isEqualTo(bedNetTask)
   }
 
@@ -163,15 +167,17 @@ class ResourceDatabaseMigrationTest {
       close()
     }
 
-    // Re-open the database with version 4 and provide MIGRATION_3_4 as the migration process.
-    helper.runMigrationsAndValidate(DB_NAME, 5, true, MIGRATION_4_5)
+    // Re-open the database with version 5 and provide MIGRATION_4_5 as the migration process.
+    val migratedDatabase = helper.runMigrationsAndValidate(DB_NAME, 5, true, MIGRATION_4_5)
 
     val retrievedTask: String?
-    getMigratedRoomDatabase().apply {
-      retrievedTask = this.resourceDao().getResource(taskId, ResourceType.Task)
-      openHelper.writableDatabase.close()
+    migratedDatabase.let { database ->
+      database.query("SELECT serializedResource FROM ResourceEntity").let {
+        it.moveToFirst()
+        retrievedTask = it.getString(0)
+      }
     }
-
+    migratedDatabase.close()
     assertThat(retrievedTask).isEqualTo(bedNetTask)
   }
 
@@ -205,44 +211,87 @@ class ResourceDatabaseMigrationTest {
       close()
     }
 
-    helper.runMigrationsAndValidate(DB_NAME, 6, true, MIGRATION_5_6)
+    val migratedDatabase = helper.runMigrationsAndValidate(DB_NAME, 6, true, MIGRATION_5_6)
 
     val retrievedTask: String?
     val localChangeEntityTimeStamp: Long
     val resourceEntityLastUpdatedLocal: Long
     val localChangeEntityCorruptedTimeStamp: Long
 
-    getMigratedRoomDatabase().apply {
-      retrievedTask = this.resourceDao().getResource(taskId, ResourceType.Task)
+    migratedDatabase.let { database ->
+      database.query("SELECT serializedResource FROM ResourceEntity").let {
+        it.moveToFirst()
+        retrievedTask = it.getString(0)
+      }
+
       resourceEntityLastUpdatedLocal =
-        query("Select lastUpdatedLocal from ResourceEntity", null).let {
+        database.query("Select lastUpdatedLocal from ResourceEntity").let {
           it.moveToFirst()
           it.getLong(0)
         }
 
-      query("SELECT timestamp FROM LocalChangeEntity", null).let {
+      database.query("SELECT timestamp FROM LocalChangeEntity").let {
         it.moveToFirst()
         localChangeEntityTimeStamp = it.getLong(0)
         it.moveToNext()
         localChangeEntityCorruptedTimeStamp = it.getLong(0)
       }
-
-      openHelper.writableDatabase.close()
     }
-
+    migratedDatabase.close()
     assertThat(retrievedTask).isEqualTo(bedNetTask)
     assertThat(localChangeEntityTimeStamp).isEqualTo(resourceEntityLastUpdatedLocal)
     assertThat(Instant.ofEpochMilli(localChangeEntityCorruptedTimeStamp)).isEqualTo(Instant.EPOCH)
   }
 
-  private fun getMigratedRoomDatabase(): ResourceDatabase =
-    Room.databaseBuilder(
-        InstrumentationRegistry.getInstrumentation().targetContext,
-        ResourceDatabase::class.java,
-        DB_NAME,
+  @Test
+  fun migrate6To7_should_execute_with_no_exception(): Unit = runBlocking {
+    val taskId = "bed-net-001"
+    val taskResourceUuid = "e2c79e28-ed4d-4029-a12c-108d1eb5bedb"
+    val bedNetTask: String =
+      Task()
+        .apply {
+          id = taskId
+          description = "Issue bed net"
+          meta.lastUpdated = Date()
+        }
+        .let { iParser.encodeResourceToString(it) }
+
+    helper.createDatabase(DB_NAME, 6).apply {
+      val date = Date()
+      execSQL(
+        "INSERT INTO ResourceEntity (resourceUuid, resourceType, resourceId, serializedResource, lastUpdatedLocal) VALUES ('$taskResourceUuid', 'Task', '$taskId', '$bedNetTask', '${DbTypeConverters.instantToLong(date.toInstant())}' );",
+      )
+
+      execSQL(
+        "INSERT INTO LocalChangeEntity (resourceType, resourceId, timestamp, type, payload) VALUES ('Task', '$taskId', '${date.toTimeZoneString()}', '${DbTypeConverters.localChangeTypeToInt(LocalChangeEntity.Type.INSERT)}', '$bedNetTask'  );",
       )
-      .addMigrations(MIGRATION_1_2, MIGRATION_2_3, MIGRATION_3_4, MIGRATION_4_5, MIGRATION_5_6)
-      .build()
+      close()
+    }
+
+    val migratedDatabase = helper.runMigrationsAndValidate(DB_NAME, 7, true, MIGRATION_6_7)
+
+    val retrievedTaskResourceId: String?
+    val retrievedTaskResourceUuid: String?
+    val localChangeResourceUuid: String?
+    val localChangeResourceId: String?
+
+    migratedDatabase.let { database ->
+      database.query("SELECT resourceId, resourceUuid FROM ResourceEntity").let {
+        it.moveToFirst()
+        retrievedTaskResourceId = it.getString(0)
+        retrievedTaskResourceUuid = String(it.getBlob(1), Charsets.UTF_8)
+      }
+
+      database.query("SELECT resourceId,resourceUuid FROM LocalChangeEntity").let {
+        it.moveToFirst()
+        localChangeResourceId = it.getString(0)
+        localChangeResourceUuid = String(it.getBlob(1), Charsets.UTF_8)
+      }
+    }
+    migratedDatabase.close()
+    assertThat(retrievedTaskResourceUuid).isEqualTo(localChangeResourceUuid)
+    assertThat(localChangeResourceId).isEqualTo(retrievedTaskResourceId)
+  }
 
   companion object {
     const val DB_NAME = "migration_tests.db"

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

@@ -95,7 +95,14 @@ internal class DatabaseImpl(
             }
           }
 
-          addMigrations(MIGRATION_1_2, MIGRATION_2_3, MIGRATION_3_4, MIGRATION_4_5, MIGRATION_5_6)
+          addMigrations(
+            MIGRATION_1_2,
+            MIGRATION_2_3,
+            MIGRATION_3_4,
+            MIGRATION_4_5,
+            MIGRATION_5_6,
+            MIGRATION_6_7,
+          )
         }
         .build()
   }
@@ -115,8 +122,9 @@ internal class DatabaseImpl(
       logicalIds.addAll(
         resource.map {
           val timeOfLocalChange = Instant.now()
-          localChangeDao.addInsert(it, timeOfLocalChange)
-          resourceDao.insertLocalResource(it, timeOfLocalChange)
+          val resourceUuid = resourceDao.insertLocalResource(it, timeOfLocalChange)
+          localChangeDao.addInsert(it, resourceUuid, timeOfLocalChange)
+          it.logicalId
         },
       )
     }
@@ -169,19 +177,16 @@ internal class DatabaseImpl(
 
   override suspend fun delete(type: ResourceType, id: String) {
     db.withTransaction {
-      val remoteVersionId: String? =
-        try {
-          selectEntity(type, id).versionId
-        } catch (e: ResourceNotFoundException) {
-          null
+      resourceDao.getResourceEntity(id, type)?.let {
+        val rowsDeleted = resourceDao.deleteResource(resourceId = id, resourceType = type)
+        if (rowsDeleted > 0) {
+          localChangeDao.addDelete(
+            resourceId = id,
+            resourceType = type,
+            resourceUuid = it.resourceUuid,
+            remoteVersionId = it.versionId,
+          )
         }
-      val rowsDeleted = resourceDao.deleteResource(resourceId = id, resourceType = type)
-      if (rowsDeleted > 0) {
-        localChangeDao.addDelete(
-          resourceId = id,
-          resourceType = type,
-          remoteVersionId = remoteVersionId,
-        )
       }
     }
   }

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

@@ -50,7 +50,7 @@ import com.google.android.fhir.db.impl.entities.UriIndexEntity
       LocalChangeEntity::class,
       PositionIndexEntity::class,
     ],
-  version = 6,
+  version = 7,
   exportSchema = true,
 )
 @TypeConverters(DbTypeConverters::class)
@@ -127,3 +127,25 @@ val MIGRATION_5_6 =
       )
     }
   }
+
+/** Add column resourceUuid in [LocalChangeEntity] */
+val MIGRATION_6_7 =
+  object : Migration(6, 7) {
+    override fun migrate(database: SupportSQLiteDatabase) {
+      database.execSQL(
+        "CREATE TABLE IF NOT EXISTS `_new_LocalChangeEntity` (`id` INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, `resourceType` TEXT NOT NULL, `resourceId` TEXT NOT NULL, `resourceUuid` BLOB NOT NULL, `timestamp` INTEGER NOT NULL, `type` INTEGER NOT NULL, `payload` TEXT NOT NULL, `versionId` TEXT)",
+      )
+      database.execSQL(
+        "INSERT INTO `_new_LocalChangeEntity` (`id`,`resourceType`,`resourceId`,`resourceUuid`,`timestamp`,`type`,`payload`,`versionId`) " +
+          "SELECT localChange.id, localChange.resourceType, localChange.resourceId, resource.resourceUuid, localChange.timestamp, localChange.type, localChange.payload, localChange.versionId FROM `LocalChangeEntity` localChange LEFT JOIN ResourceEntity resource ON localChange.resourceId= resource.resourceId",
+      )
+      database.execSQL("DROP TABLE `LocalChangeEntity`")
+      database.execSQL("ALTER TABLE `_new_LocalChangeEntity` RENAME TO `LocalChangeEntity`")
+      database.execSQL(
+        "CREATE INDEX IF NOT EXISTS `index_LocalChangeEntity_resourceType_resourceId` ON `LocalChangeEntity` (`resourceType`, `resourceId`)",
+      )
+      database.execSQL(
+        "CREATE INDEX IF NOT EXISTS `index_LocalChangeEntity_resourceUuid` ON `LocalChangeEntity` (`resourceUuid`)",
+      )
+    }
+  }

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

@@ -32,6 +32,7 @@ import com.google.android.fhir.logicalId
 import com.google.android.fhir.versionId
 import java.time.Instant
 import java.util.Date
+import java.util.UUID
 import org.hl7.fhir.r4.model.Resource
 import org.hl7.fhir.r4.model.ResourceType
 import org.json.JSONArray
@@ -51,7 +52,7 @@ internal abstract class LocalChangeDao {
   @Insert abstract suspend fun addLocalChange(localChangeEntity: LocalChangeEntity)
 
   @Transaction
-  open suspend fun addInsert(resource: Resource, timeOfLocalChange: Instant) {
+  open suspend fun addInsert(resource: Resource, resourceUuid: UUID, timeOfLocalChange: Instant) {
     val resourceId = resource.logicalId
     val resourceType = resource.resourceType
     val resourceString = iParser.encodeResourceToString(resource)
@@ -61,6 +62,7 @@ internal abstract class LocalChangeDao {
         id = 0,
         resourceType = resourceType.name,
         resourceId = resourceId,
+        resourceUuid = resourceUuid,
         timestamp = timeOfLocalChange,
         type = Type.INSERT,
         payload = resourceString,
@@ -95,6 +97,7 @@ internal abstract class LocalChangeDao {
         id = 0,
         resourceType = resourceType.name,
         resourceId = resourceId,
+        resourceUuid = oldEntity.resourceUuid,
         timestamp = timeOfLocalChange,
         type = Type.UPDATE,
         payload = jsonDiff.toString(),
@@ -103,12 +106,18 @@ internal abstract class LocalChangeDao {
     )
   }
 
-  suspend fun addDelete(resourceId: String, resourceType: ResourceType, remoteVersionId: String?) {
+  suspend fun addDelete(
+    resourceId: String,
+    resourceUuid: UUID,
+    resourceType: ResourceType,
+    remoteVersionId: String?,
+  ) {
     addLocalChange(
       LocalChangeEntity(
         id = 0,
         resourceType = resourceType.name,
         resourceId = resourceId,
+        resourceUuid = resourceUuid,
         timestamp = Date().toInstant(),
         type = Type.DELETE,
         payload = "",