Merge pull request #3413 from element-hq/feature/bma/deleteOldLogFiles

Feature/bma/delete old log files
2024-09-06 16:29:32 +02:00
parent 617dfb0e82 8ce1687bf5
commit f7d28f8eed
14 changed files with 259 additions and 18 deletions
--- a/features/migration/impl/build.gradle.kts
+++ b/features/migration/impl/build.gradle.kts
@@ -40,6 +40,7 @@ dependencies {
    testImplementation(libs.test.robolectric)
    testImplementation(libs.test.truth)
    testImplementation(libs.test.turbine)
+    testImplementation(projects.libraries.matrix.test)
    testImplementation(projects.libraries.sessionStorage.implMemory)
    testImplementation(projects.libraries.sessionStorage.test)
    testImplementation(projects.libraries.preferences.test)
--- a/features/migration/impl/src/main/kotlin/io/element/android/features/migration/impl/DefaultMigrationStore.kt
+++ b/features/migration/impl/src/main/kotlin/io/element/android/features/migration/impl/DefaultMigrationStore.kt
@@ -46,7 +46,7 @@ class DefaultMigrationStore @Inject constructor(

    override fun applicationMigrationVersion(): Flow<Int> {
        return store.data.map { prefs ->
-            prefs[applicationMigrationVersion] ?: 0
+            prefs[applicationMigrationVersion] ?: -1
        }
    }
 }
--- a/features/migration/impl/src/main/kotlin/io/element/android/features/migration/impl/MigrationPresenter.kt
+++ b/features/migration/impl/src/main/kotlin/io/element/android/features/migration/impl/MigrationPresenter.kt
@@ -53,6 +53,12 @@ class MigrationPresenter @Inject constructor(

        LaunchedEffect(migrationStoreVersion) {
            val migrationValue = migrationStoreVersion ?: return@LaunchedEffect
+            if (migrationValue == -1) {
+                // Fresh install, no migration needed
+                Timber.d("Fresh install, no migration needed.")
+                migrationStore.setApplicationMigrationVersion(lastMigration)
+                return@LaunchedEffect
+            }
            if (migrationValue == lastMigration) {
                Timber.d("Current app migration version: $migrationValue. No migration needed.")
                migrationAction = AsyncData.Success(Unit)
--- a/features/migration/impl/src/main/kotlin/io/element/android/features/migration/impl/migrations/AppMigration07.kt
+++ b/features/migration/impl/src/main/kotlin/io/element/android/features/migration/impl/migrations/AppMigration07.kt
@@ -0,0 +1,38 @@
+/*
+ * Copyright (c) 2024 New Vector Ltd
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package io.element.android.features.migration.impl.migrations
+
+import com.squareup.anvil.annotations.ContributesMultibinding
+import io.element.android.features.rageshake.api.logs.LogFilesRemover
+import io.element.android.libraries.di.AppScope
+import javax.inject.Inject
+
+/**
+ * Delete the previous log files.
+ */
+@ContributesMultibinding(AppScope::class)
+class AppMigration07 @Inject constructor(
+    private val logFilesRemover: LogFilesRemover,
+) : AppMigration {
+    override val order: Int = 7
+
+    override suspend fun migrate() {
+        logFilesRemover.perform { file ->
+            file.name.startsWith("logs-")
+        }
+    }
+}
--- a/features/migration/impl/src/test/kotlin/io/element/android/features/migration/impl/MigrationPresenterTest.kt
+++ b/features/migration/impl/src/test/kotlin/io/element/android/features/migration/impl/MigrationPresenterTest.kt
@@ -35,6 +35,32 @@ class MigrationPresenterTest {
    @get:Rule
    val warmUpRule = WarmUpRule()

+    @Test
+    fun `present - no migration should occurs on fresh installation, and last version should be stored`() = runTest {
+        val migrations = (1..10).map { order ->
+            FakeAppMigration(
+                order = order,
+                migrateLambda = LambdaNoParamRecorder(ensureNeverCalled = true) { },
+            )
+        }
+        val store = InMemoryMigrationStore(initialApplicationMigrationVersion = -1)
+        val presenter = createPresenter(
+            migrationStore = store,
+            migrations = migrations.toSet(),
+        )
+        moleculeFlow(RecompositionMode.Immediate) {
+            presenter.present()
+        }.test {
+            val initialState = awaitItem()
+            assertThat(initialState.migrationAction).isEqualTo(AsyncData.Uninitialized)
+            skipItems(1)
+            awaitItem().also { state ->
+                assertThat(state.migrationAction).isEqualTo(AsyncData.Success(Unit))
+            }
+            assertThat(store.applicationMigrationVersion().first()).isEqualTo(migrations.maxOf { it.order })
+        }
+    }
+
    @Test
    fun `present - no migration should occurs if ApplicationMigrationVersion is the last one`() = runTest {
        val migrations = (1..10).map { FakeAppMigration(it) }
@@ -89,7 +115,7 @@ private fun createPresenter(

 private class FakeAppMigration(
    override val order: Int,
-    var migrateLambda: LambdaNoParamRecorder<Unit> = lambdaRecorder { -> },
+    val migrateLambda: LambdaNoParamRecorder<Unit> = lambdaRecorder { -> },
 ) : AppMigration {
    override suspend fun migrate() {
        migrateLambda()
--- a/features/migration/impl/src/test/kotlin/io/element/android/features/migration/impl/migrations/AppMigration05Test.kt
+++ b/features/migration/impl/src/test/kotlin/io/element/android/features/migration/impl/migrations/AppMigration05Test.kt
@@ -0,0 +1,59 @@
+/*
+ * Copyright (c) 2024 New Vector Ltd
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package io.element.android.features.migration.impl.migrations
+
+import com.google.common.truth.Truth.assertThat
+import io.element.android.libraries.matrix.test.A_SESSION_ID
+import io.element.android.libraries.sessionstorage.impl.memory.InMemorySessionStore
+import io.element.android.libraries.sessionstorage.test.aSessionData
+import kotlinx.coroutines.test.runTest
+import org.junit.Test
+import java.io.File
+
+class AppMigration05Test {
+    @Test
+    fun `empty session path should be set to an expected path`() = runTest {
+        val sessionStore = InMemorySessionStore().apply {
+            updateData(
+                aSessionData(
+                    sessionId = A_SESSION_ID,
+                    sessionPath = "",
+                )
+            )
+        }
+        val migration = AppMigration05(sessionStore = sessionStore, baseDirectory = File("/a/path"))
+        migration.migrate()
+        val storedData = sessionStore.getSession(A_SESSION_ID.value)!!
+        assertThat(storedData.sessionPath).isEqualTo("/a/path/${A_SESSION_ID.value.replace(':', '_')}")
+    }
+
+    @Test
+    fun `non empty session path should not be impacted by the migration`() = runTest {
+        val sessionStore = InMemorySessionStore().apply {
+            updateData(
+                aSessionData(
+                    sessionId = A_SESSION_ID,
+                    sessionPath = "/a/path/existing",
+                )
+            )
+        }
+        val migration = AppMigration05(sessionStore = sessionStore, baseDirectory = File("/a/path"))
+        migration.migrate()
+        val storedData = sessionStore.getSession(A_SESSION_ID.value)!!
+        assertThat(storedData.sessionPath).isEqualTo("/a/path/existing")
+    }
+}
--- a/features/migration/impl/src/test/kotlin/io/element/android/features/migration/impl/migrations/AppMigration06Test.kt
+++ b/features/migration/impl/src/test/kotlin/io/element/android/features/migration/impl/migrations/AppMigration06Test.kt
@@ -0,0 +1,60 @@
+/*
+ * Copyright (c) 2024 New Vector Ltd
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package io.element.android.features.migration.impl.migrations
+
+import com.google.common.truth.Truth.assertThat
+import io.element.android.libraries.matrix.test.A_SESSION_ID
+import io.element.android.libraries.sessionstorage.impl.memory.InMemorySessionStore
+import io.element.android.libraries.sessionstorage.test.aSessionData
+import kotlinx.coroutines.test.runTest
+import org.junit.Test
+import java.io.File
+
+class AppMigration06Test {
+    @Test
+    fun `empty cache path should be set to an expected path`() = runTest {
+        val sessionStore = InMemorySessionStore().apply {
+            updateData(
+                aSessionData(
+                    sessionId = A_SESSION_ID,
+                    sessionPath = "/a/path/to/a/session/AN_ID",
+                    cachePath = "",
+                )
+            )
+        }
+        val migration = AppMigration06(sessionStore = sessionStore, cacheDirectory = File("/a/path/cache"))
+        migration.migrate()
+        val storedData = sessionStore.getSession(A_SESSION_ID.value)!!
+        assertThat(storedData.cachePath).isEqualTo("/a/path/cache/AN_ID")
+    }
+
+    @Test
+    fun `non empty cache path should not be impacted by the migration`() = runTest {
+        val sessionStore = InMemorySessionStore().apply {
+            updateData(
+                aSessionData(
+                    sessionId = A_SESSION_ID,
+                    cachePath = "/a/path/existing",
+                )
+            )
+        }
+        val migration = AppMigration05(sessionStore = sessionStore, baseDirectory = File("/a/path/cache"))
+        migration.migrate()
+        val storedData = sessionStore.getSession(A_SESSION_ID.value)!!
+        assertThat(storedData.cachePath).isEqualTo("/a/path/existing")
+    }
+}
--- a/features/migration/impl/src/test/kotlin/io/element/android/features/migration/impl/migrations/AppMigration07Test.kt
+++ b/features/migration/impl/src/test/kotlin/io/element/android/features/migration/impl/migrations/AppMigration07Test.kt
@@ -0,0 +1,39 @@
+/*
+ * Copyright (c) 2024 New Vector Ltd
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package io.element.android.features.migration.impl.migrations
+
+import com.google.common.truth.Truth.assertThat
+import io.element.android.features.rageshake.test.logs.FakeLogFilesRemover
+import io.element.android.tests.testutils.lambda.lambdaRecorder
+import kotlinx.coroutines.test.runTest
+import org.junit.Test
+import java.io.File
+
+class AppMigration07Test {
+    @Test
+    fun `test migration`() = runTest {
+        val performLambda = lambdaRecorder<(File) -> Boolean, Unit> { predicate ->
+            // Test the predicate
+            assertThat(predicate(File("logs-0433.log.gz"))).isTrue()
+            assertThat(predicate(File("logs.2024-08-01-20.log.gz"))).isFalse()
+        }
+        val logsFileRemover = FakeLogFilesRemover(performLambda = performLambda)
+        val migration = AppMigration07(logsFileRemover)
+        migration.migrate()
+        performLambda.assertions().isCalledOnce()
+    }
+}
--- a/features/rageshake/api/src/main/kotlin/io/element/android/features/rageshake/api/logs/LogFilesRemover.kt
+++ b/features/rageshake/api/src/main/kotlin/io/element/android/features/rageshake/api/logs/LogFilesRemover.kt
@@ -16,6 +16,12 @@

 package io.element.android.features.rageshake.api.logs

+import java.io.File
+
 interface LogFilesRemover {
-    suspend fun perform()
+    /**
+     * Perform the log files removal.
+     * @param predicate a predicate to filter the files to remove. By default, all files are removed.
+     */
+    suspend fun perform(predicate: (File) -> Boolean = { true })
 }
--- a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/logs/DefaultLogFilesRemover.kt
+++ b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/logs/DefaultLogFilesRemover.kt
@@ -20,13 +20,14 @@ import com.squareup.anvil.annotations.ContributesBinding
 import io.element.android.features.rageshake.api.logs.LogFilesRemover
 import io.element.android.features.rageshake.impl.reporter.DefaultBugReporter
 import io.element.android.libraries.di.AppScope
+import java.io.File
 import javax.inject.Inject

@ContributesBinding(AppScope::class)
 class DefaultLogFilesRemover @Inject constructor(
    private val bugReporter: DefaultBugReporter,
 ) : LogFilesRemover {
-    override suspend fun perform() {
-        bugReporter.deleteAllFiles()
+    override suspend fun perform(predicate: (File) -> Boolean) {
+        bugReporter.deleteAllFiles(predicate)
    }
 }
--- a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/reporter/DefaultBugReporter.kt
+++ b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/reporter/DefaultBugReporter.kt
@@ -301,9 +301,11 @@ class DefaultBugReporter @Inject constructor(
        }
    }

-    suspend fun deleteAllFiles() {
+    suspend fun deleteAllFiles(predicate: (File) -> Boolean) {
        withContext(coroutineDispatchers.io) {
-            getLogFiles().forEach { it.safeDelete() }
+            getLogFiles()
+                .filter(predicate)
+                .forEach { it.safeDelete() }
        }
    }

--- a/features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportPresenterTest.kt
+++ b/features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportPresenterTest.kt
@@ -32,7 +32,7 @@ import io.element.android.features.rageshake.test.screenshot.FakeScreenshotHolde
 import io.element.android.libraries.architecture.AsyncAction
 import io.element.android.libraries.matrix.test.A_FAILURE_REASON
 import io.element.android.tests.testutils.WarmUpRule
-import io.element.android.tests.testutils.lambda.lambdaRecorder
+import io.element.android.tests.testutils.lambda.LambdaOneParamRecorder
 import kotlinx.coroutines.test.TestScope
 import kotlinx.coroutines.test.runTest
 import org.junit.Rule
@@ -120,11 +120,11 @@ class BugReportPresenterTest {

    @Test
    fun `present - reset all`() = runTest {
-        val logFilesRemoverLambda = lambdaRecorder { -> }
+        val logFilesRemover = FakeLogFilesRemover()
        val presenter = createPresenter(
            crashDataStore = FakeCrashDataStore(crashData = A_CRASH_DATA, appHasCrashed = true),
            screenshotHolder = FakeScreenshotHolder(screenshotUri = A_SCREENSHOT_URI),
-            logFilesRemover = FakeLogFilesRemover(logFilesRemoverLambda),
+            logFilesRemover = logFilesRemover,
        )
        moleculeFlow(RecompositionMode.Immediate) {
            presenter.present()
@@ -136,7 +136,7 @@ class BugReportPresenterTest {
            initialState.eventSink.invoke(BugReportEvents.ResetAll)
            val resetState = awaitItem()
            assertThat(resetState.hasCrashLogs).isFalse()
-            logFilesRemoverLambda.assertions().isCalledOnce()
+            logFilesRemover.performLambda.assertions().isCalledOnce()
            // TODO Make it live assertThat(resetState.screenshotUri).isNull()
        }
    }
@@ -245,7 +245,7 @@ class BugReportPresenterTest {
        bugReporter: BugReporter = FakeBugReporter(),
        crashDataStore: CrashDataStore = FakeCrashDataStore(),
        screenshotHolder: ScreenshotHolder = FakeScreenshotHolder(),
-        logFilesRemover: LogFilesRemover = FakeLogFilesRemover(lambdaRecorder(ensureNeverCalled = true) { -> }),
+        logFilesRemover: LogFilesRemover = FakeLogFilesRemover(LambdaOneParamRecorder(ensureNeverCalled = true) { }),
    ) = BugReportPresenter(
        bugReporter = bugReporter,
        crashDataStore = crashDataStore,
--- a/features/rageshake/test/src/main/kotlin/io/element/android/features/rageshake/test/logs/FakeLogFilesRemover.kt
+++ b/features/rageshake/test/src/main/kotlin/io/element/android/features/rageshake/test/logs/FakeLogFilesRemover.kt
@@ -17,13 +17,14 @@
 package io.element.android.features.rageshake.test.logs

 import io.element.android.features.rageshake.api.logs.LogFilesRemover
-import io.element.android.tests.testutils.lambda.LambdaNoParamRecorder
+import io.element.android.tests.testutils.lambda.LambdaOneParamRecorder
 import io.element.android.tests.testutils.lambda.lambdaRecorder
+import java.io.File

 class FakeLogFilesRemover(
-    var performLambda: LambdaNoParamRecorder<Unit> = lambdaRecorder { -> },
+    val performLambda: LambdaOneParamRecorder<(File) -> Boolean, Unit> = lambdaRecorder<(File) -> Boolean, Unit> { },
 ) : LogFilesRemover {
-    override suspend fun perform() {
-        performLambda()
+    override suspend fun perform(predicate: (File) -> Boolean) {
+        performLambda(predicate)
    }
 }
--- a/libraries/session-storage/test/src/main/kotlin/io/element/android/libraries/sessionstorage/test/SessionData.kt
+++ b/libraries/session-storage/test/src/main/kotlin/io/element/android/libraries/sessionstorage/test/SessionData.kt
@@ -23,6 +23,8 @@ import io.element.android.libraries.sessionstorage.api.SessionData
 fun aSessionData(
    sessionId: SessionId = SessionId("@alice:server.org"),
    isTokenValid: Boolean = false,
+    sessionPath: String = "/a/path/to/a/session",
+    cachePath: String = "/a/path/to/a/cache",
 ): SessionData {
    return SessionData(
        userId = sessionId.value,
@@ -36,7 +38,7 @@ fun aSessionData(
        isTokenValid = isTokenValid,
        loginType = LoginType.UNKNOWN,
        passphrase = null,
-        sessionPath = "/a/path/to/a/session",
-        cachePath = "/a/path/to/a/cache",
+        sessionPath = sessionPath,
+        cachePath = cachePath,
    )
 }