Stop throwing exceptions when DatabaseImpl encounters an old version (#4841)

* Stop throwing exceptions when DatabaseImpl encounters an old version

insertOrUpdate now returns a Boolean instead of a version number Int, which indicates whether the operation was applied or not. Previously we would throw an exception if the version number given in the request was the same or older than the one stored in the database. Now, getting an old version number is a no-op, and will return false.

* Fixes for failing tests

* lint

Author: csilvestrini

java/arcs/android/storage/database/DatabaseImpl.kt

@@ -351,7 +351,7 @@ class DatabaseImpl(
         storageKey: StorageKey,
         data: DatabaseData,
         originatingClientId: Int?
-    ): Int = stats.insertUpdate.timeSuspending { counters ->
+    ): Boolean = stats.insertUpdate.timeSuspending { counters ->
         when (data) {
             is DatabaseData.Entity -> {
                 counters.increment(DatabaseCounters.INSERTUPDATE_ENTITY)
@@ -366,20 +366,20 @@ class DatabaseImpl(
                 insertOrUpdate(storageKey, data, counters)
             }
         }
-        // TODO: Return a proper database version number.
-        return@timeSuspending data.databaseVersion
-    }.also { newVersion ->
-        clientFlow.filter { it.storageKey == storageKey }
-            .onEach { it.onDatabaseUpdate(data, newVersion, originatingClientId) }
-            .launchIn(CoroutineScope(coroutineContext))
+    }.also { success ->
+        if (success) {
+            clientFlow.filter { it.storageKey == storageKey }
+                .onEach { it.onDatabaseUpdate(data, data.databaseVersion, originatingClientId) }
+                .launchIn(CoroutineScope(coroutineContext))
+        }
     }
 
     @VisibleForTesting
     suspend fun insertOrUpdate(
         storageKey: StorageKey,
         data: DatabaseData.Entity,
         counters: Counters? = null
-    ) = writableDatabase.useTransaction {
+    ): Boolean = writableDatabase.useTransaction {
         val db = this
         val entity = data.rawEntity
         // Fetch/create the entity's type ID.
@@ -395,7 +395,8 @@ class DatabaseImpl(
             data.databaseVersion,
             db,
             counters
-        )
+        ) ?: return@useTransaction false // Database has newer data. Don't apply the given op.
+
         // Insert the entity's field types.
         counters?.increment(DatabaseCounters.GET_ENTITY_FIELDS)
         val fields = getSchemaFields(schemaTypeId, db)
@@ -452,6 +453,7 @@ class DatabaseImpl(
                     SQLiteDatabase.CONFLICT_REPLACE
                 )
             }
+            true
     }
 
     /**
@@ -505,7 +507,7 @@ class DatabaseImpl(
         data: DatabaseData.Collection,
         dataType: DataType,
         counters: Counters?
-    ) = writableDatabase.useTransaction {
+    ): Boolean = writableDatabase.useTransaction {
         val db = this
 
         // Fetch/create the entity's type ID.
@@ -560,7 +562,9 @@ class DatabaseImpl(
         } else {
             // Collection already exists; delete all existing entries.
             val collectionId = metadata.collectionId
-            if (data.databaseVersion <= metadata.versionNumber) return@useTransaction
+            if (data.databaseVersion != metadata.versionNumber + 1) {
+                return@useTransaction false
+            }
 
             // TODO: Don't blindly delete everything and re-insert: only insert/remove the diff.
             when (dataType) {
@@ -612,14 +616,15 @@ class DatabaseImpl(
                     content.apply { put("value_id", referenceId) }
                 )
             }
+        true
     }
 
     @VisibleForTesting
     suspend fun insertOrUpdate(
         storageKey: StorageKey,
         data: DatabaseData.Singleton,
         counters: Counters? = null
-    ) {
+    ): Boolean {
         // Convert Singleton into a a zero-or-one-element Collection.
         val set = mutableSetOf<Reference>()
         data.reference?.let { set.add(it) }
@@ -632,7 +637,7 @@ class DatabaseImpl(
             )
         }
         // Store the Collection as a Singleton.
-        insertOrUpdate(storageKey, collectionData, DataType.Singleton, counters)
+        return insertOrUpdate(storageKey, collectionData, DataType.Singleton, counters)
     }
 
     override suspend fun delete(
@@ -765,6 +770,9 @@ class DatabaseImpl(
     /**
      * Creates a new storage key ID for the given entity [StorageKey]. If one already exists, it and
      * all data for the existing entity are deleted first before creating a new one.
+     *
+     * @returns the new storage key ID for the entity if one was successfully created, or null
+     *     otherwise (e.g. if the given version number was too low)
      */
     @VisibleForTesting
     suspend fun createEntityStorageKeyId(
@@ -777,7 +785,7 @@ class DatabaseImpl(
         databaseVersion: Int,
         db: SQLiteDatabase,
         counters: Counters? = null
-    ): StorageKeyId = db.transaction {
+    ): StorageKeyId? = db.transaction {
         // TODO: Use an LRU cache.
         counters?.increment(DatabaseCounters.GET_ENTITY_STORAGEKEY_ID)
         rawQuery(
@@ -803,9 +811,8 @@ class DatabaseImpl(
                     "$storedEntityId."
             }
             val storedVersion = it.getInt(2)
-            require(databaseVersion > storedVersion) {
-                "Given version ($databaseVersion) must be greater than version in database " +
-                    "($storedVersion) when updating storage key $storageKey."
+            if (databaseVersion != storedVersion + 1) {
+                return@transaction null
             }
 
             // Remove the existing entity.
@@ -1220,6 +1227,7 @@ class DatabaseImpl(
         val isCollection: Boolean
     )
 
+    @VisibleForTesting
     data class CollectionMetadata(
         val collectionId: CollectionId,
         val versionMap: VersionMap,

java/arcs/core/storage/database/Database.kt

@@ -34,7 +34,7 @@ interface Database {
         storageKey: StorageKey,
         data: DatabaseData,
         originatingClientId: Int? = null
-    ): Int
+    ): Boolean
 
     /** Fetches the data at [storageKey] from the database. */
     suspend fun get(

java/arcs/core/storage/driver/Database.kt

@@ -346,7 +346,7 @@ class DatabaseDriver<Data : Any>(
         }
 
         // Store the prepped data.
-        return (database.insertOrUpdate(storageKey, databaseData, clientId) == version).also {
+        return database.insertOrUpdate(storageKey, databaseData, clientId).also {
             // If the update was successful, update our local data/version.
             if (it) localDataMutex.withLock {
                 localData = data

java/arcs/jvm/storage/database/testutil/MockDatabaseManager.kt

@@ -11,7 +11,6 @@
 
 package arcs.jvm.storage.database.testutil
 
-import arcs.core.crdt.VersionMap
 import arcs.core.data.Schema
 import arcs.core.storage.StorageKey
 import arcs.core.storage.database.Database
@@ -73,10 +72,11 @@ open class MockDatabase : Database {
         storageKey: StorageKey,
         data: DatabaseData,
         originatingClientId: Int?
-    ): Int = stats.insertUpdate.timeSuspending {
+    ): Boolean = stats.insertUpdate.timeSuspending {
         val (version, isNew) = dataMutex.withLock {
             val oldData = this.data[storageKey]
-            if (oldData?.databaseVersion != data.databaseVersion) {
+            // Must be exactly old version + 1, or have no previous version.
+            if (oldData == null || data.databaseVersion == oldData.databaseVersion + 1) {
                 this.data[storageKey] = data
                 data.databaseVersion to true
             } else {
@@ -90,7 +90,7 @@ open class MockDatabase : Database {
                 .launchIn(CoroutineScope(coroutineContext))
         }
 
-        return@timeSuspending version
+        isNew
     }
 
     @Suppress("UNCHECKED_CAST")
@@ -99,18 +99,7 @@ open class MockDatabase : Database {
         dataType: KClass<out DatabaseData>,
         schema: Schema
     ): DatabaseData? = stats.get.timeSuspending {
-        val dataVal = dataMutex.withLock { data[storageKey] }
-
-        if (dataVal != null) return@timeSuspending dataVal
-
-        return@timeSuspending when (dataType) {
-            DatabaseData.Singleton::class ->
-                DatabaseData.Singleton(null, schema, -1, VersionMap())
-            DatabaseData.Collection::class ->
-                DatabaseData.Collection(emptySet(), schema, -1, VersionMap())
-            DatabaseData.Entity::class -> null
-            else -> throw IllegalArgumentException("Illegal type.")
-        }
+        dataMutex.withLock { data[storageKey] }
     }
 
     override suspend fun delete(storageKey: StorageKey, originatingClientId: Int?) =

javatests/arcs/android/storage/ReferenceModeStoreDatabaseImplIntegrationTest.kt

@@ -447,7 +447,7 @@ class ReferenceModeStoreDatabaseImplIntegrationTest {
                     "t1" to t1Ref
                 )
             ),
-            1
+            3
         )
         driver.receiver!!(
             CrdtSet.DataImpl(
@@ -458,7 +458,7 @@ class ReferenceModeStoreDatabaseImplIntegrationTest {
                     "t2" to t2Ref
                 )
             ),
-            2
+            4
         )
 
         activeStore.idle()

javatests/arcs/android/storage/database/DatabaseImplTest.kt

@@ -307,11 +307,11 @@ class DatabaseImplTest {
     }
 
     @Test
-    fun createEntityStorageKeyId_versionNumberMustBeLarger() = runBlockingTest {
+    fun createEntityStorageKeyId_versionNumberMustBeOneLarger() = runBlockingTest {
         val key = DummyStorageKey("key")
         val entityId = "entity-id"
         val typeId = 123L
-        database.createEntityStorageKeyId(
+        val originalStorageKeyId = database.createEntityStorageKeyId(
             key,
             entityId,
             CREATION_TIMESTAMP,
@@ -321,9 +321,10 @@ class DatabaseImplTest {
             10,
             db
         )
+        assertThat(originalStorageKeyId).isNotNull()
 
         // Same version number is rejected.
-        val exception1 = assertSuspendingThrows(IllegalArgumentException::class) {
+        assertThat(
             database.createEntityStorageKeyId(
                 key,
                 entityId,
@@ -334,14 +335,10 @@ class DatabaseImplTest {
                 10,
                 db
             )
-        }
-        assertThat(exception1).hasMessageThat().isEqualTo(
-            "Given version (10) must be greater than version in database (10) when updating " +
-                "storage key dummy://key."
-        )
+        ).isNull()
 
         // Smaller version number is rejected.
-        val exception2 = assertSuspendingThrows(IllegalArgumentException::class) {
+        assertThat(
             database.createEntityStorageKeyId(
                 key,
                 entityId,
@@ -352,14 +349,24 @@ class DatabaseImplTest {
                 9,
                 db
             )
-        }
-        assertThat(exception2).hasMessageThat().isEqualTo(
-            "Given version (9) must be greater than version in database (10) when updating " +
-                "storage key dummy://key."
-        )
+        ).isNull()
 
-        // Increasing version number is ok.
-        database.createEntityStorageKeyId(
+        // Increasing version number by more than 1 is rejected.
+        assertThat(
+            database.createEntityStorageKeyId(
+                key,
+                entityId,
+                CREATION_TIMESTAMP,
+                EXPIRATION_TIMESTAMP,
+                typeId,
+                VERSION_MAP,
+                12,
+                db
+            )
+        ).isNull()
+
+        // Increasing version number by 1 is ok.
+        val newStorageKeyId = database.createEntityStorageKeyId(
             key,
             entityId,
             CREATION_TIMESTAMP,
@@ -369,6 +376,10 @@ class DatabaseImplTest {
             11,
             db
         )
+        assertThat(newStorageKeyId).isNotNull()
+        // TODO: If the storage key is the same, there's no need to delete the old one and create a
+        // new one.
+        assertThat(newStorageKeyId).isNotEqualTo(originalStorageKeyId)
     }
 
     @Test
@@ -956,15 +967,13 @@ class DatabaseImplTest {
                 Reference("ref", DummyStorageKey("backing"), VersionMap("ref" to 1))
             ),
             schema = newSchema("hash"),
-            databaseVersion = 1,
+            databaseVersion = 2,
             versionMap = VERSION_MAP
         )
-        val oldVersion = database.insertOrUpdate(key, collection)
+        assertThat(database.insertOrUpdate(key, collection)).isTrue()
 
-        // TODO - DatabaseImpl should return the last version when an insert/update could not be
-        //  applied.
-        assertThat(database.insertOrUpdate(key, collection.copy(databaseVersion = oldVersion - 1)))
-            .isEqualTo(oldVersion - 1)
+        assertThat(database.insertOrUpdate(key, collection.copy(databaseVersion = 1)))
+            .isFalse()
     }
 
     @Test
@@ -1040,20 +1049,18 @@ class DatabaseImplTest {
         val singleton = DatabaseData.Singleton(
             reference = Reference("ref", DummyStorageKey("backing"), VersionMap("ref" to 1)),
             schema = newSchema("hash"),
-            databaseVersion = 1,
+            databaseVersion = 2,
             versionMap = VERSION_MAP
         )
-        val oldVersion = database.insertOrUpdate(key, singleton, originatingClientId = null)
+        assertThat(database.insertOrUpdate(key, singleton, originatingClientId = null)).isTrue()
 
-        // TODO - DatabaseImpl should return the last version when an insert/update could not be
-        //  applied.
         assertThat(
             database.insertOrUpdate(
                 key,
-                singleton.copy(databaseVersion = oldVersion - 1),
+                singleton.copy(databaseVersion = 1),
                 originatingClientId = null
             )
-        ).isEqualTo(oldVersion - 1)
+        ).isFalse()
     }
 
     @Test

javatests/arcs/core/storage/ReferenceModeStoreDatabaseIntegrationTest.kt

@@ -432,7 +432,7 @@ class ReferenceModeStoreDatabaseIntegrationTest {
                     "t1" to t1Ref
                 )
             ),
-            1
+            3
         )
         driver.receiver!!(
             CrdtSet.DataImpl(
@@ -443,7 +443,7 @@ class ReferenceModeStoreDatabaseIntegrationTest {
                     "t2" to t2Ref
                 )
             ),
-            2
+            4
         )
 
         activeStore.idle()