Add current metadata to RestoreCoordinator state

so we know which backup version we need to expect during restore

app/src/main/java/com/stevesoltys/seedvault/restore/RestorableBackup.kt

@@ -3,7 +3,7 @@ package com.stevesoltys.seedvault.restore
 import com.stevesoltys.seedvault.metadata.BackupMetadata
 import com.stevesoltys.seedvault.metadata.PackageMetadataMap
 
-data class RestorableBackup(private val backupMetadata: BackupMetadata) {
+data class RestorableBackup(val backupMetadata: BackupMetadata) {
 
     val name: String
         get() = backupMetadata.deviceName

app/src/main/java/com/stevesoltys/seedvault/restore/RestoreViewModel.kt

@@ -148,6 +148,7 @@ internal class RestoreViewModel(
     }
 
     override fun onRestorableBackupClicked(restorableBackup: RestorableBackup) {
+        restoreCoordinator.beforeStartRestore(restorableBackup.backupMetadata)
         mChosenRestorableBackup.value = restorableBackup
         mDisplayFragment.setEvent(RESTORE_APPS)
     }

app/src/main/java/com/stevesoltys/seedvault/transport/ConfigurableBackupTransport.kt

@@ -204,8 +204,8 @@ class ConfigurableBackupTransport internal constructor(private val context: Cont
         return restoreCoordinator.getCurrentRestoreSet()
     }
 
-    override fun startRestore(token: Long, packages: Array<PackageInfo>): Int {
+    override fun startRestore(token: Long, packages: Array<PackageInfo>): Int = runBlocking {
-        return restoreCoordinator.startRestore(token, packages)
+        restoreCoordinator.startRestore(token, packages)
     }
 
     override fun getNextFullRestoreDataChunk(socket: ParcelFileDescriptor): Int = runBlocking {

app/src/main/java/com/stevesoltys/seedvault/transport/restore/RestoreCoordinator.kt

@@ -2,7 +2,6 @@ package com.stevesoltys.seedvault.transport.restore
 
 import android.app.backup.BackupTransport.TRANSPORT_ERROR
 import android.app.backup.BackupTransport.TRANSPORT_OK
-import android.app.backup.IBackupManager
 import android.app.backup.RestoreDescription
 import android.app.backup.RestoreDescription.NO_MORE_PACKAGES
 import android.app.backup.RestoreDescription.TYPE_FULL_STREAM
@@ -12,7 +11,6 @@ import android.content.Context
 import android.content.pm.PackageInfo
 import android.os.ParcelFileDescriptor
 import android.util.Log
-import androidx.collection.LongSparseArray
 import com.stevesoltys.seedvault.MAGIC_PACKAGE_MANAGER
 import com.stevesoltys.seedvault.R
 import com.stevesoltys.seedvault.header.UnsupportedVersionException
@@ -31,7 +29,8 @@ private data class RestoreCoordinatorState(
     /**
      * Optional [PackageInfo] for single package restore, to reduce data needed to read for @pm@
      */
-    val pmPackageInfo: PackageInfo?
+    val pmPackageInfo: PackageInfo?,
+    val backupMetadata: BackupMetadata
 ) {
     var currentPackage: String? = null
 }
@@ -51,7 +50,7 @@ internal class RestoreCoordinator(
 ) {
 
     private var state: RestoreCoordinatorState? = null
-    private var backupMetadata: LongSparseArray<BackupMetadata>? = null
+    private var backupMetadata: BackupMetadata? = null
     private val failedPackages = ArrayList<String>()
 
     suspend fun getAvailableMetadata(): Map<Long, BackupMetadata>? {
@@ -113,19 +112,27 @@ internal class RestoreCoordinator(
         }
     }
 
+    /**
+     * Call this before starting the restore as an optimization to prevent re-fetching metadata.
+     */
+    fun beforeStartRestore(backupMetadata: BackupMetadata) {
+        this.backupMetadata = backupMetadata
+    }
+
     /**
      * Start restoring application data from backup.
      * After calling this function,
      * there will be alternate calls to [nextRestorePackage] and [getRestoreData]
      * to walk through the actual application data.
      *
-     * @param token A backup token as returned by [getAvailableRestoreSets] or [getCurrentRestoreSet].
+     * @param token A backup token as returned by [getAvailableRestoreSets]
+     * or [getCurrentRestoreSet].
      * @param packages List of applications to restore (if data is available).
      * Application data will be restored in the order given.
      * @return One of [TRANSPORT_OK] (OK so far, call [nextRestorePackage])
      * or [TRANSPORT_ERROR] (an error occurred, the restore should be aborted and rescheduled).
      */
-    fun startRestore(token: Long, packages: Array<out PackageInfo>): Int {
+    suspend fun startRestore(token: Long, packages: Array<out PackageInfo>): Int {
         check(state == null) { "Started new restore with existing state: $state" }
         Log.i(TAG, "Start restore with ${packages.map { info -> info.packageName }}")
 
@@ -151,7 +158,13 @@ internal class RestoreCoordinator(
                 packages[1]
             } else null
 
-        state = RestoreCoordinatorState(token, packages.iterator(), pmPackageInfo)
+        val metadata = if (backupMetadata?.token == token) {
+            backupMetadata!! // if token matches, backupMetadata is non-null
+        } else {
+            getAvailableMetadata()?.get(token) ?: return TRANSPORT_ERROR
+        }
+        state = RestoreCoordinatorState(token, packages.iterator(), pmPackageInfo, metadata)
+        backupMetadata = null
         failedPackages.clear()
         return TRANSPORT_OK
     }
@@ -269,18 +282,6 @@ internal class RestoreCoordinator(
         state = null
     }
 
-    /**
-     * Call this after calling [IBackupManager.getAvailableRestoreTokenForUser]
-     * to retrieve additional [BackupMetadata] that is not available in [RestoreSet].
-     *
-     * It will also clear the saved metadata, so that subsequent calls will return null.
-     */
-    fun getAndClearBackupMetadata(): LongSparseArray<BackupMetadata>? {
-        val result = backupMetadata
-        backupMetadata = null
-        return result
-    }
-
     fun isFailedPackage(packageName: String) = packageName in failedPackages
 
     // TODO this is plugin specific, needs to be factored out when supporting different plugins

app/src/test/java/com/stevesoltys/seedvault/transport/CoordinatorIntegrationTest.kt

@@ -181,6 +181,7 @@ internal class CoordinatorIntegrationTest : TransportTest() {
         assertEquals(TRANSPORT_OK, backup.finishBackup())
 
         // start restore
+        restore.beforeStartRestore(metadata)
         assertEquals(TRANSPORT_OK, restore.startRestore(token, arrayOf(packageInfo)))
 
         // find data for K/V backup
@@ -251,6 +252,7 @@ internal class CoordinatorIntegrationTest : TransportTest() {
         assertEquals(TRANSPORT_OK, backup.finishBackup())
 
         // start restore
+        restore.beforeStartRestore(metadata)
         assertEquals(TRANSPORT_OK, restore.startRestore(token, arrayOf(packageInfo)))
 
         // find data for K/V backup
@@ -311,6 +313,7 @@ internal class CoordinatorIntegrationTest : TransportTest() {
         assertEquals(TRANSPORT_OK, backup.finishBackup())
 
         // start restore
+        restore.beforeStartRestore(metadata)
         assertEquals(TRANSPORT_OK, restore.startRestore(token, arrayOf(packageInfo)))
 
         // find data only for full backup

app/src/test/java/com/stevesoltys/seedvault/transport/TransportTest.kt

@@ -10,6 +10,8 @@ import android.util.Log
 import com.stevesoltys.seedvault.Clock
 import com.stevesoltys.seedvault.MAGIC_PACKAGE_MANAGER
 import com.stevesoltys.seedvault.crypto.Crypto
+import com.stevesoltys.seedvault.getRandomString
+import com.stevesoltys.seedvault.metadata.BackupMetadata
 import com.stevesoltys.seedvault.metadata.MetadataManager
 import com.stevesoltys.seedvault.settings.SettingsManager
 import io.mockk.every
@@ -41,6 +43,12 @@ internal abstract class TransportTest {
     protected val pmPackageInfo = PackageInfo().apply {
         packageName = MAGIC_PACKAGE_MANAGER
     }
+    protected val metadata = BackupMetadata(
+        token = token,
+        androidVersion = Random.nextInt(),
+        androidIncremental = getRandomString(),
+        deviceName = getRandomString()
+    )
 
     init {
         mockkStatic(Log::class)

app/src/test/java/com/stevesoltys/seedvault/transport/restore/RestoreCoordinatorTest.kt

@@ -10,7 +10,6 @@ import android.content.pm.PackageInfo
 import android.os.ParcelFileDescriptor
 import com.stevesoltys.seedvault.coAssertThrows
 import com.stevesoltys.seedvault.getRandomString
-import com.stevesoltys.seedvault.metadata.BackupMetadata
 import com.stevesoltys.seedvault.metadata.EncryptedBackupMetadata
 import com.stevesoltys.seedvault.metadata.MetadataReader
 import com.stevesoltys.seedvault.metadata.PackageMetadata
@@ -26,7 +25,6 @@ import io.mockk.verify
 import kotlinx.coroutines.runBlocking
 import org.junit.jupiter.api.Assertions.assertEquals
 import org.junit.jupiter.api.Assertions.assertNotNull
-import org.junit.jupiter.api.Assertions.assertNull
 import org.junit.jupiter.api.Assertions.assertThrows
 import org.junit.jupiter.api.Assertions.fail
 import org.junit.jupiter.api.Test
@@ -69,18 +67,13 @@ internal class RestoreCoordinatorTest : TransportTest() {
     @Test
     fun `getAvailableRestoreSets() builds set from plugin response`() = runBlocking {
         val encryptedMetadata = EncryptedBackupMetadata(token, inputStream)
-        val metadata = BackupMetadata(
-            token = token,
-            androidVersion = Random.nextInt(),
-            androidIncremental = getRandomString(),
-            deviceName = getRandomString()
-        )
 
         coEvery { plugin.getAvailableBackups() } returns sequenceOf(
             encryptedMetadata,
-            encryptedMetadata
+            EncryptedBackupMetadata(token + 1, inputStream)
         )
         every { metadataReader.readMetadata(inputStream, token) } returns metadata
+        every { metadataReader.readMetadata(inputStream, token + 1) } returns metadata
         every { inputStream.close() } just Runs
 
         val sets = restore.getAvailableRestoreSets() ?: fail()
@@ -97,68 +90,102 @@ internal class RestoreCoordinatorTest : TransportTest() {
     }
 
     @Test
-    fun `startRestore() returns OK`() {
+    fun `startRestore() returns OK`() = runBlocking {
+        restore.beforeStartRestore(metadata)
         assertEquals(TRANSPORT_OK, restore.startRestore(token, packageInfoArray))
     }
 
     @Test
-    fun `startRestore() can not be called twice`() {
+    fun `startRestore() fetches metadata if missing`() = runBlocking {
+        coEvery { plugin.getAvailableBackups() } returns sequenceOf(
+            EncryptedBackupMetadata(token, inputStream),
+            EncryptedBackupMetadata(token + 1, inputStream)
+        )
+        every { metadataReader.readMetadata(inputStream, token) } returns metadata
+        every { metadataReader.readMetadata(inputStream, token + 1) } returns metadata
+        every { inputStream.close() } just Runs
+
+        assertEquals(TRANSPORT_OK, restore.startRestore(token, packageInfoArray))
+    }
+
+    @Test
+    fun `startRestore() errors if metadata is not matching token`() = runBlocking {
+        coEvery { plugin.getAvailableBackups() } returns sequenceOf(
+            EncryptedBackupMetadata(token + 42, inputStream)
+        )
+        every { metadataReader.readMetadata(inputStream, token + 42) } returns metadata
+        every { inputStream.close() } just Runs
+
+        assertEquals(TRANSPORT_ERROR, restore.startRestore(token, packageInfoArray))
+    }
+
+    @Test
+    fun `startRestore() can not be called twice`() = runBlocking {
+        restore.beforeStartRestore(metadata)
         assertEquals(TRANSPORT_OK, restore.startRestore(token, packageInfoArray))
         assertThrows(IllegalStateException::class.javaObjectType) {
-            restore.startRestore(token, packageInfoArray)
+            runBlocking {
+                restore.startRestore(token, packageInfoArray)
+            }
         }
+        Unit
     }
 
     @Test
-    fun `startRestore() can be be called again after restore finished`() {
+    fun `startRestore() can be be called again after restore finished`() = runBlocking {
+        restore.beforeStartRestore(metadata)
         assertEquals(TRANSPORT_OK, restore.startRestore(token, packageInfoArray))
 
         every { full.hasState() } returns false
         restore.finishRestore()
 
+        restore.beforeStartRestore(metadata)
         assertEquals(TRANSPORT_OK, restore.startRestore(token, packageInfoArray))
     }
 
     @Test
-    fun `startRestore() optimized auto-restore with removed storage shows notification`() {
+    fun `startRestore() optimized auto-restore with removed storage shows notification`() =
-        every { settingsManager.getStorage() } returns storage
+        runBlocking {
-        every { storage.isUnavailableUsb(context) } returns true
+            every { settingsManager.getStorage() } returns storage
-        every { metadataManager.getPackageMetadata(packageName) } returns PackageMetadata(42L)
+            every { storage.isUnavailableUsb(context) } returns true
-        every { storage.name } returns storageName
+            every { metadataManager.getPackageMetadata(packageName) } returns PackageMetadata(42L)
-        every {
+            every { storage.name } returns storageName
-            notificationManager.onRemovableStorageNotAvailableForRestore(
+            every {
-                packageName,
+                notificationManager.onRemovableStorageNotAvailableForRestore(
-                storageName
+                    packageName,
-            )
+                    storageName
-        } just Runs
+                )
+            } just Runs
 
-        assertEquals(TRANSPORT_ERROR, restore.startRestore(token, pmPackageInfoArray))
+            assertEquals(TRANSPORT_ERROR, restore.startRestore(token, pmPackageInfoArray))
 
-        verify(exactly = 1) {
+            verify(exactly = 1) {
-            notificationManager.onRemovableStorageNotAvailableForRestore(
+                notificationManager.onRemovableStorageNotAvailableForRestore(
-                packageName,
+                    packageName,
-                storageName
+                    storageName
-            )
+                )
+            }
         }
-    }
 
     @Test
-    fun `startRestore() optimized auto-restore with available storage shows no notification`() {
+    fun `startRestore() optimized auto-restore with available storage shows no notification`() =
-        every { settingsManager.getStorage() } returns storage
+        runBlocking {
-        every { storage.isUnavailableUsb(context) } returns false
+            every { settingsManager.getStorage() } returns storage
+            every { storage.isUnavailableUsb(context) } returns false
 
-        assertEquals(TRANSPORT_OK, restore.startRestore(token, pmPackageInfoArray))
+            restore.beforeStartRestore(metadata)
+            assertEquals(TRANSPORT_OK, restore.startRestore(token, pmPackageInfoArray))
 
-        verify(exactly = 0) {
+            verify(exactly = 0) {
-            notificationManager.onRemovableStorageNotAvailableForRestore(
+                notificationManager.onRemovableStorageNotAvailableForRestore(
-                packageName,
+                    packageName,
-                storageName
+                    storageName
-            )
+                )
+            }
         }
-    }
 
     @Test
-    fun `startRestore() with removed storage shows no notification`() {
+    fun `startRestore() with removed storage shows no notification`() = runBlocking {
         every { settingsManager.getStorage() } returns storage
         every { storage.isUnavailableUsb(context) } returns true
         every { metadataManager.getPackageMetadata(packageName) } returns null
@@ -182,6 +209,7 @@ internal class RestoreCoordinatorTest : TransportTest() {
 
     @Test
     fun `nextRestorePackage() returns KV description and takes precedence`() = runBlocking {
+        restore.beforeStartRestore(metadata)
         restore.startRestore(token, packageInfoArray)
 
         coEvery { kv.hasDataForPackage(token, packageInfo) } returns true
@@ -193,6 +221,7 @@ internal class RestoreCoordinatorTest : TransportTest() {
 
     @Test
     fun `nextRestorePackage() returns full description if no KV data found`() = runBlocking {
+        restore.beforeStartRestore(metadata)
         restore.startRestore(token, packageInfoArray)
 
         coEvery { kv.hasDataForPackage(token, packageInfo) } returns false
@@ -205,6 +234,7 @@ internal class RestoreCoordinatorTest : TransportTest() {
 
     @Test
     fun `nextRestorePackage() returns NO_MORE_PACKAGES if data found`() = runBlocking {
+        restore.beforeStartRestore(metadata)
         restore.startRestore(token, packageInfoArray)
 
         coEvery { kv.hasDataForPackage(token, packageInfo) } returns false
@@ -215,6 +245,7 @@ internal class RestoreCoordinatorTest : TransportTest() {
 
     @Test
     fun `nextRestorePackage() returns all packages from startRestore()`() = runBlocking {
+        restore.beforeStartRestore(metadata)
         restore.startRestore(token, packageInfoArray2)
 
         coEvery { kv.hasDataForPackage(token, packageInfo) } returns true
@@ -234,22 +265,24 @@ internal class RestoreCoordinatorTest : TransportTest() {
     }
 
     @Test
-    fun `when kv#hasDataForPackage() throws return null`() = runBlocking {
+    fun `when kv#hasDataForPackage() throws, it tries next package`() = runBlocking {
+        restore.beforeStartRestore(metadata)
         restore.startRestore(token, packageInfoArray)
 
         coEvery { kv.hasDataForPackage(token, packageInfo) } throws IOException()
 
-        assertNull(restore.nextRestorePackage())
+        assertEquals(NO_MORE_PACKAGES, restore.nextRestorePackage())
     }
 
     @Test
-    fun `when full#hasDataForPackage() throws return null`() = runBlocking {
+    fun `when full#hasDataForPackage() throws, it tries next package`() = runBlocking {
+        restore.beforeStartRestore(metadata)
         restore.startRestore(token, packageInfoArray)
 
         coEvery { kv.hasDataForPackage(token, packageInfo) } returns false
         coEvery { full.hasDataForPackage(token, packageInfo) } throws IOException()
 
-        assertNull(restore.nextRestorePackage())
+        assertEquals(NO_MORE_PACKAGES, restore.nextRestorePackage())
     }
 
     @Test

app/src/test/java/com/stevesoltys/seedvault/transport/restore/RestoreV0IntegrationTest.kt

@@ -61,7 +61,7 @@ internal class RestoreV0IntegrationTest : TransportTest() {
         kvRestore,
         fullRestore,
         metadataReader
-    )
+    ).apply { beforeStartRestore(metadata) }
 
     private val fileDescriptor = mockk<ParcelFileDescriptor>(relaxed = true)
     private val appData = ("562AB665C3543120FC794D7CDA3AC18E5959235A4D" +