From c83e8f392e6049803f1db5488461b3b94590b25f Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Wed, 28 Aug 2024 11:38:02 -0300 Subject: [PATCH] Fix issue with DocumentFileCache --- .../seedvault/backend/saf/SafBackendTest.kt | 28 +++++++------ .../seedvault/transport/backup/KVBackup.kt | 4 +- .../main/resources/simplelogger.properties | 1 + .../seedvault/core/backends/BackendTest.kt | 39 ++++++++++------- .../core/backends/saf/DocumentFileCache.kt | 42 ++++++++++++++++--- .../seedvault/core/backends/saf/SafBackend.kt | 41 ++++++++++++++---- .../core/backends/webdav/WebDavBackendTest.kt | 5 +++ 7 files changed, 117 insertions(+), 43 deletions(-) create mode 100644 app/src/main/resources/simplelogger.properties diff --git a/app/src/androidTest/java/com/stevesoltys/seedvault/backend/saf/SafBackendTest.kt b/app/src/androidTest/java/com/stevesoltys/seedvault/backend/saf/SafBackendTest.kt index e42fd4cb..173246bf 100644 --- a/app/src/androidTest/java/com/stevesoltys/seedvault/backend/saf/SafBackendTest.kt +++ b/app/src/androidTest/java/com/stevesoltys/seedvault/backend/saf/SafBackendTest.kt @@ -25,21 +25,23 @@ class SafBackendTest : BackendTest(), KoinComponent { private val context = InstrumentationRegistry.getInstrumentation().targetContext private val settingsManager by inject() - override val plugin: Backend - get() { - val safStorage = settingsManager.getSafProperties() ?: error("No SAF storage") - val safProperties = SafProperties( - config = safStorage.config, - name = safStorage.name, - isUsb = safStorage.isUsb, - requiresNetwork = safStorage.requiresNetwork, - rootId = safStorage.rootId, - ) - return SafBackend(context, safProperties, ".SeedvaultTest") - } + private val safStorage = settingsManager.getSafProperties() ?: error("No SAF storage") + private val safProperties = SafProperties( + config = safStorage.config, + name = safStorage.name, + isUsb = safStorage.isUsb, + requiresNetwork = safStorage.requiresNetwork, + rootId = safStorage.rootId, + ) + override val plugin: Backend = SafBackend(context, safProperties, ".SeedvaultTest") @Test - fun test(): Unit = runBlocking { + fun `test write list read rename delete`(): Unit = runBlocking { testWriteListReadRenameDelete() } + + @Test + fun `test remove create write file`(): Unit = runBlocking { + testRemoveCreateWriteFile() + } } diff --git a/app/src/main/java/com/stevesoltys/seedvault/transport/backup/KVBackup.kt b/app/src/main/java/com/stevesoltys/seedvault/transport/backup/KVBackup.kt index b3a823a5..acdfc855 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/transport/backup/KVBackup.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/transport/backup/KVBackup.kt @@ -15,11 +15,11 @@ import android.content.pm.PackageInfo import android.os.ParcelFileDescriptor import android.util.Log import com.stevesoltys.seedvault.MAGIC_PACKAGE_MANAGER +import com.stevesoltys.seedvault.backend.BackendManager +import com.stevesoltys.seedvault.backend.isOutOfSpace import com.stevesoltys.seedvault.crypto.Crypto import com.stevesoltys.seedvault.header.VERSION import com.stevesoltys.seedvault.header.getADForKV -import com.stevesoltys.seedvault.backend.BackendManager -import com.stevesoltys.seedvault.backend.isOutOfSpace import com.stevesoltys.seedvault.settings.SettingsManager import com.stevesoltys.seedvault.ui.notification.BackupNotificationManager import org.calyxos.seedvault.core.backends.LegacyAppBackupFile diff --git a/app/src/main/resources/simplelogger.properties b/app/src/main/resources/simplelogger.properties new file mode 100644 index 00000000..beb56b2e --- /dev/null +++ b/app/src/main/resources/simplelogger.properties @@ -0,0 +1 @@ +org.slf4j.simpleLogger.defaultLogLevel=debug diff --git a/core/src/main/java/org/calyxos/seedvault/core/backends/BackendTest.kt b/core/src/main/java/org/calyxos/seedvault/core/backends/BackendTest.kt index 39a507f1..1fd40ee9 100644 --- a/core/src/main/java/org/calyxos/seedvault/core/backends/BackendTest.kt +++ b/core/src/main/java/org/calyxos/seedvault/core/backends/BackendTest.kt @@ -6,14 +6,12 @@ package org.calyxos.seedvault.core.backends import androidx.annotation.VisibleForTesting -import at.bitfire.dav4jvm.exception.HttpException import org.calyxos.seedvault.core.toHexString -import org.junit.Assert.assertArrayEquals import kotlin.random.Random +import kotlin.test.assertContentEquals import kotlin.test.assertEquals import kotlin.test.assertFailsWith import kotlin.test.assertNotNull -import kotlin.test.fail @VisibleForTesting public abstract class BackendTest { @@ -21,12 +19,9 @@ public abstract class BackendTest { public abstract val plugin: Backend protected suspend fun testWriteListReadRenameDelete() { - try { - plugin.removeAll() - } catch (e: HttpException) { - if (e.code != 404) fail(e.message, e) - } + plugin.removeAll() + val androidId = "0123456789abcdef" val now = System.currentTimeMillis() val bytes1 = Random.nextBytes(1337) val bytes2 = Random.nextBytes(1337 * 8) @@ -34,7 +29,7 @@ public abstract class BackendTest { it.write(bytes1) } - plugin.save(FileBackupFileType.Snapshot("0123456789abcdef", now)).use { + plugin.save(FileBackupFileType.Snapshot(androidId, now)).use { it.write(bytes2) } @@ -56,13 +51,13 @@ public abstract class BackendTest { assertNotNull(metadata) assertNotNull(snapshot) - assertArrayEquals(bytes1, plugin.load(metadata as FileHandle).readAllBytes()) - assertArrayEquals(bytes2, plugin.load(snapshot as FileHandle).readAllBytes()) + assertContentEquals(bytes1, plugin.load(metadata as FileHandle).readAllBytes()) + assertContentEquals(bytes2, plugin.load(snapshot as FileHandle).readAllBytes()) val blobName = Random.nextBytes(32).toHexString() var blob: FileBackupFileType.Blob? = null val bytes3 = Random.nextBytes(1337 * 16) - plugin.save(FileBackupFileType.Blob("0123456789abcdef", blobName)).use { + plugin.save(FileBackupFileType.Blob(androidId, blobName)).use { it.write(bytes3) } plugin.list( @@ -77,7 +72,7 @@ public abstract class BackendTest { } } assertNotNull(blob) - assertArrayEquals(bytes3, plugin.load(blob as FileHandle).readAllBytes()) + assertContentEquals(bytes3, plugin.load(blob as FileHandle).readAllBytes()) // try listing with top-level folder, should find two files of FileBackupFileType in there var numFiles = 0 @@ -92,7 +87,7 @@ public abstract class BackendTest { plugin.remove(snapshot as FileHandle) // rename snapshots - val snapshotNewFolder = TopLevelFolder("0123456789abcdee.sv") + val snapshotNewFolder = TopLevelFolder("a123456789abcdef.sv") plugin.rename(snapshot!!.topLevelFolder, snapshotNewFolder) // rename to existing folder should fail @@ -105,4 +100,20 @@ public abstract class BackendTest { plugin.remove(snapshotNewFolder) } + protected suspend fun testRemoveCreateWriteFile() { + val now = System.currentTimeMillis() + val blob = LegacyAppBackupFile.Blob(now, Random.nextBytes(32).toHexString()) + val bytes = Random.nextBytes(2342) + + plugin.remove(blob) + try { + plugin.save(blob).use { + it.write(bytes) + } + assertContentEquals(bytes, plugin.load(blob as FileHandle).readAllBytes()) + } finally { + plugin.remove(blob) + } + } + } diff --git a/core/src/main/java/org/calyxos/seedvault/core/backends/saf/DocumentFileCache.kt b/core/src/main/java/org/calyxos/seedvault/core/backends/saf/DocumentFileCache.kt index 123bc07a..9adf0da8 100644 --- a/core/src/main/java/org/calyxos/seedvault/core/backends/saf/DocumentFileCache.kt +++ b/core/src/main/java/org/calyxos/seedvault/core/backends/saf/DocumentFileCache.kt @@ -11,6 +11,7 @@ import org.calyxos.seedvault.core.backends.FileBackupFileType import org.calyxos.seedvault.core.backends.FileHandle import org.calyxos.seedvault.core.backends.LegacyAppBackupFile import org.calyxos.seedvault.core.backends.TopLevelFolder +import java.util.concurrent.ConcurrentHashMap internal class DocumentFileCache( private val context: Context, @@ -18,7 +19,7 @@ internal class DocumentFileCache( private val root: String, ) { - private val cache = mutableMapOf() + private val cache = ConcurrentHashMap() internal suspend fun getRootFile(): DocumentFile { return cache.getOrPut(root) { @@ -26,24 +27,53 @@ internal class DocumentFileCache( } } - internal suspend fun getFile(fh: FileHandle): DocumentFile = when (fh) { + internal suspend fun getOrCreateFile(fh: FileHandle): DocumentFile = when (fh) { is TopLevelFolder -> cache.getOrPut("$root/${fh.relativePath}") { getRootFile().getOrCreateDirectory(context, fh.name) } is LegacyAppBackupFile -> cache.getOrPut("$root/${fh.relativePath}") { - getFile(fh.topLevelFolder).getOrCreateFile(context, fh.name) - } + getOrCreateFile(fh.topLevelFolder).getOrCreateFile(context, fh.name) + } is FileBackupFileType.Blob -> { val subFolderName = fh.name.substring(0, 2) cache.getOrPut("$root/${fh.topLevelFolder.name}/$subFolderName") { - getFile(fh.topLevelFolder).getOrCreateDirectory(context, subFolderName) + getOrCreateFile(fh.topLevelFolder).getOrCreateDirectory(context, subFolderName) }.getOrCreateFile(context, fh.name) } is FileBackupFileType.Snapshot -> { - getFile(fh.topLevelFolder).getOrCreateFile(context, fh.name) + getOrCreateFile(fh.topLevelFolder).getOrCreateFile(context, fh.name) } } + + internal suspend fun getFile(fh: FileHandle): DocumentFile? = when (fh) { + is TopLevelFolder -> cache.getOrElse("$root/${fh.relativePath}") { + getRootFile().findFileBlocking(context, fh.name) + } + + is LegacyAppBackupFile -> cache.getOrElse("$root/${fh.relativePath}") { + getFile(fh.topLevelFolder)?.findFileBlocking(context, fh.name) + } + + is FileBackupFileType.Blob -> { + val subFolderName = fh.name.substring(0, 2) + cache.getOrElse("$root/${fh.topLevelFolder.name}/$subFolderName") { + getFile(fh.topLevelFolder)?.findFileBlocking(context, subFolderName) + }?.findFileBlocking(context, fh.name) + } + + is FileBackupFileType.Snapshot -> { + getFile(fh.topLevelFolder)?.findFileBlocking(context, fh.name) + } + } + + internal fun removeFromCache(fh: FileHandle) { + cache.remove("$root/${fh.relativePath}") + } + + internal fun clearAll() { + cache.clear() + } } diff --git a/core/src/main/java/org/calyxos/seedvault/core/backends/saf/SafBackend.kt b/core/src/main/java/org/calyxos/seedvault/core/backends/saf/SafBackend.kt index 535e5b25..bf731337 100644 --- a/core/src/main/java/org/calyxos/seedvault/core/backends/saf/SafBackend.kt +++ b/core/src/main/java/org/calyxos/seedvault/core/backends/saf/SafBackend.kt @@ -14,6 +14,7 @@ import android.provider.DocumentsContract.Root.COLUMN_ROOT_ID import android.provider.DocumentsContract.renameDocument import androidx.core.database.getIntOrNull import androidx.documentfile.provider.DocumentFile +import io.github.oshai.kotlinlogging.KLogger import io.github.oshai.kotlinlogging.KotlinLogging import org.calyxos.seedvault.core.backends.Backend import org.calyxos.seedvault.core.backends.Constants.DIRECTORY_ROOT @@ -37,6 +38,8 @@ import kotlin.reflect.KClass internal const val AUTHORITY_STORAGE = "com.android.externalstorage.documents" internal const val ROOT_ID_DEVICE = "primary" +private const val DEBUG_LOG = true + public class SafBackend( private val appContext: Context, private val safProperties: SafProperties, @@ -52,10 +55,12 @@ public class SafBackend( private val cache = DocumentFileCache(context, safProperties.getDocumentFile(context), root) override suspend fun test(): Boolean { + log.debugLog { "test()" } return cache.getRootFile().isDirectory } override suspend fun getFreeSpace(): Long? { + log.debugLog { "getFreeSpace()" } val rootId = safProperties.rootId ?: return null val authority = safProperties.uri.authority // using DocumentsContract#buildRootUri(String, String) with rootId directly doesn't work @@ -82,12 +87,14 @@ public class SafBackend( } override suspend fun save(handle: FileHandle): OutputStream { - val file = cache.getFile(handle) + log.debugLog { "save($handle)" } + val file = cache.getOrCreateFile(handle) return file.getOutputStream(context.contentResolver) } override suspend fun load(handle: FileHandle): InputStream { - val file = cache.getFile(handle) + log.debugLog { "load($handle)" } + val file = cache.getOrCreateFile(handle) return file.getInputStream(context.contentResolver) } @@ -101,10 +108,12 @@ public class SafBackend( if (LegacyAppBackupFile.IconsFile::class in fileTypes) throw UnsupportedOperationException() if (LegacyAppBackupFile.Blob::class in fileTypes) throw UnsupportedOperationException() + log.debugLog { "list($topLevelFolder, $fileTypes)" } + val folder = if (topLevelFolder == null) { cache.getRootFile() } else { - cache.getFile(topLevelFolder) + cache.getOrCreateFile(topLevelFolder) } // limit depth based on wanted types and if top-level folder is given var depth = if (FileBackupFileType.Blob::class in fileTypes) 3 else 2 @@ -161,12 +170,16 @@ public class SafBackend( } override suspend fun remove(handle: FileHandle) { - val file = cache.getFile(handle) - if (!file.delete()) throw IOException("could not delete ${handle.relativePath}") + log.debugLog { "remove($handle)" } + cache.getFile(handle)?.let { file -> + if (!file.delete()) throw IOException("could not delete ${handle.relativePath}") + cache.removeFromCache(handle) + } } override suspend fun rename(from: TopLevelFolder, to: TopLevelFolder) { - val fromFile = cache.getFile(from) + log.debugLog { "rename($from, ${to.name})" } + val fromFile = cache.getOrCreateFile(from) // don't use fromFile.renameTo(to.name) as that creates "${to.name} (1)" val newUri = renameDocument(context.contentResolver, fromFile.uri, to.name) ?: throw IOException("could not rename ${from.relativePath}") @@ -179,16 +192,28 @@ public class SafBackend( } override suspend fun removeAll() { - cache.getRootFile().listFilesBlocking(context).forEach { - it.delete() + log.debugLog { "removeAll()" } + try { + cache.getRootFile().listFilesBlocking(context).forEach { file -> + log.debugLog { " remove ${file.uri}" } + file.delete() + } + } finally { + cache.clearAll() } } override val providerPackageName: String? by lazy { + log.debugLog { "providerPackageName" } val authority = safProperties.uri.authority ?: return@lazy null val providerInfo = context.packageManager.resolveContentProvider(authority, 0) ?: return@lazy null + log.debugLog { " ${providerInfo.packageName}" } providerInfo.packageName } } + +private inline fun KLogger.debugLog(crossinline block: () -> String) { + if (DEBUG_LOG) debug { block() } +} diff --git a/core/src/test/java/org/calyxos/seedvault/core/backends/webdav/WebDavBackendTest.kt b/core/src/test/java/org/calyxos/seedvault/core/backends/webdav/WebDavBackendTest.kt index 0edf9f6a..8b366c82 100644 --- a/core/src/test/java/org/calyxos/seedvault/core/backends/webdav/WebDavBackendTest.kt +++ b/core/src/test/java/org/calyxos/seedvault/core/backends/webdav/WebDavBackendTest.kt @@ -17,4 +17,9 @@ public class WebDavBackendTest : BackendTest() { public fun `test write, list, read, rename, delete`(): Unit = runBlocking { testWriteListReadRenameDelete() } + + @Test + public fun `test remove, create, write file`(): Unit = runBlocking { + testRemoveCreateWriteFile() + } }