From 5523e57fe74f1d9e72a84a8f3b3163b0b503ed57 Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Tue, 14 Sep 2021 17:06:54 +0200 Subject: [PATCH] Add current metadata to RestoreCoordinator state so we know which backup version we need to expect during restore --- .../seedvault/restore/RestorableBackup.kt | 2 +- .../seedvault/restore/RestoreViewModel.kt | 1 + .../transport/ConfigurableBackupTransport.kt | 4 +- .../transport/restore/RestoreCoordinator.kt | 39 +++--- .../transport/CoordinatorIntegrationTest.kt | 3 + .../seedvault/transport/TransportTest.kt | 8 ++ .../restore/RestoreCoordinatorTest.kt | 125 +++++++++++------- .../restore/RestoreV0IntegrationTest.kt | 2 +- 8 files changed, 115 insertions(+), 69 deletions(-) diff --git a/app/src/main/java/com/stevesoltys/seedvault/restore/RestorableBackup.kt b/app/src/main/java/com/stevesoltys/seedvault/restore/RestorableBackup.kt index 643ae84c..ddcaeb82 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/restore/RestorableBackup.kt +++ b/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 diff --git a/app/src/main/java/com/stevesoltys/seedvault/restore/RestoreViewModel.kt b/app/src/main/java/com/stevesoltys/seedvault/restore/RestoreViewModel.kt index 6a0c15ae..99f8c206 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/restore/RestoreViewModel.kt +++ b/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) } diff --git a/app/src/main/java/com/stevesoltys/seedvault/transport/ConfigurableBackupTransport.kt b/app/src/main/java/com/stevesoltys/seedvault/transport/ConfigurableBackupTransport.kt index 2d39c3d2..35926664 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/transport/ConfigurableBackupTransport.kt +++ b/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): Int { - return restoreCoordinator.startRestore(token, packages) + override fun startRestore(token: Long, packages: Array): Int = runBlocking { + restoreCoordinator.startRestore(token, packages) } override fun getNextFullRestoreDataChunk(socket: ParcelFileDescriptor): Int = runBlocking { diff --git a/app/src/main/java/com/stevesoltys/seedvault/transport/restore/RestoreCoordinator.kt b/app/src/main/java/com/stevesoltys/seedvault/transport/restore/RestoreCoordinator.kt index 7889a4e2..4c77e92a 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/transport/restore/RestoreCoordinator.kt +++ b/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? = null + private var backupMetadata: BackupMetadata? = null private val failedPackages = ArrayList() suspend fun getAvailableMetadata(): Map? { @@ -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): Int { + suspend fun startRestore(token: Long, packages: Array): 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? { - 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 diff --git a/app/src/test/java/com/stevesoltys/seedvault/transport/CoordinatorIntegrationTest.kt b/app/src/test/java/com/stevesoltys/seedvault/transport/CoordinatorIntegrationTest.kt index c2e0c39b..ee5084bd 100644 --- a/app/src/test/java/com/stevesoltys/seedvault/transport/CoordinatorIntegrationTest.kt +++ b/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 diff --git a/app/src/test/java/com/stevesoltys/seedvault/transport/TransportTest.kt b/app/src/test/java/com/stevesoltys/seedvault/transport/TransportTest.kt index 712ee855..6691804a 100644 --- a/app/src/test/java/com/stevesoltys/seedvault/transport/TransportTest.kt +++ b/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) diff --git a/app/src/test/java/com/stevesoltys/seedvault/transport/restore/RestoreCoordinatorTest.kt b/app/src/test/java/com/stevesoltys/seedvault/transport/restore/RestoreCoordinatorTest.kt index 9318f923..bafef625 100644 --- a/app/src/test/java/com/stevesoltys/seedvault/transport/restore/RestoreCoordinatorTest.kt +++ b/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`() { - every { settingsManager.getStorage() } returns storage - every { storage.isUnavailableUsb(context) } returns true - every { metadataManager.getPackageMetadata(packageName) } returns PackageMetadata(42L) - every { storage.name } returns storageName - every { - notificationManager.onRemovableStorageNotAvailableForRestore( - packageName, - storageName - ) - } just Runs + fun `startRestore() optimized auto-restore with removed storage shows notification`() = + runBlocking { + every { settingsManager.getStorage() } returns storage + every { storage.isUnavailableUsb(context) } returns true + every { metadataManager.getPackageMetadata(packageName) } returns PackageMetadata(42L) + every { storage.name } returns storageName + every { + notificationManager.onRemovableStorageNotAvailableForRestore( + packageName, + storageName + ) + } just Runs - assertEquals(TRANSPORT_ERROR, restore.startRestore(token, pmPackageInfoArray)) + assertEquals(TRANSPORT_ERROR, restore.startRestore(token, pmPackageInfoArray)) - verify(exactly = 1) { - notificationManager.onRemovableStorageNotAvailableForRestore( - packageName, - storageName - ) + verify(exactly = 1) { + notificationManager.onRemovableStorageNotAvailableForRestore( + packageName, + storageName + ) + } } - } @Test - fun `startRestore() optimized auto-restore with available storage shows no notification`() { - every { settingsManager.getStorage() } returns storage - every { storage.isUnavailableUsb(context) } returns false + fun `startRestore() optimized auto-restore with available storage shows no notification`() = + runBlocking { + 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) { - notificationManager.onRemovableStorageNotAvailableForRestore( - packageName, - storageName - ) + verify(exactly = 0) { + notificationManager.onRemovableStorageNotAvailableForRestore( + packageName, + 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 diff --git a/app/src/test/java/com/stevesoltys/seedvault/transport/restore/RestoreV0IntegrationTest.kt b/app/src/test/java/com/stevesoltys/seedvault/transport/restore/RestoreV0IntegrationTest.kt index 92bc1b0f..b0ee9835 100644 --- a/app/src/test/java/com/stevesoltys/seedvault/transport/restore/RestoreV0IntegrationTest.kt +++ b/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(relaxed = true) private val appData = ("562AB665C3543120FC794D7CDA3AC18E5959235A4D" +