From 7f3aa42c8302f83fde9f79fe42d839bf10eaa09d Mon Sep 17 00:00:00 2001 From: Rafael Tonholo Date: Wed, 17 Sep 2025 07:17:55 -0300 Subject: [PATCH 1/3] Merge pull request #9799 from rafaeltonholo/chore/9789/add-tests-to-migration-v90 chore(migration): add unit tests to MigrationTo90 --- .../k9/storage/migrations/MigrationTo90.kt | 31 +- .../storage/migrations/MigrationTo90Test.kt | 375 ++++++++++++++++++ .../k9/mail/store/imap/ImapStoreFactory.kt | 2 +- .../k9/mail/store/imap/ImapStoreSettings.kt | 16 +- 4 files changed, 408 insertions(+), 16 deletions(-) create mode 100644 legacy/storage/src/test/java/com/fsck/k9/storage/migrations/MigrationTo90Test.kt diff --git a/legacy/storage/src/main/java/com/fsck/k9/storage/migrations/MigrationTo90.kt b/legacy/storage/src/main/java/com/fsck/k9/storage/migrations/MigrationTo90.kt index 56ad3f55a22..c9ca544f89a 100644 --- a/legacy/storage/src/main/java/com/fsck/k9/storage/migrations/MigrationTo90.kt +++ b/legacy/storage/src/main/java/com/fsck/k9/storage/migrations/MigrationTo90.kt @@ -1,6 +1,7 @@ package com.fsck.k9.storage.migrations import android.database.sqlite.SQLiteDatabase +import androidx.annotation.VisibleForTesting import com.fsck.k9.mail.AuthType import com.fsck.k9.mail.AuthenticationFailedException import com.fsck.k9.mail.ServerSettings @@ -10,6 +11,7 @@ import com.fsck.k9.mail.ssl.TrustedSocketFactory import com.fsck.k9.mail.store.imap.ImapClientInfo import com.fsck.k9.mail.store.imap.ImapStore import com.fsck.k9.mail.store.imap.ImapStoreConfig +import com.fsck.k9.mail.store.imap.ImapStoreFactory import com.fsck.k9.mail.store.imap.ImapStoreSettings import com.fsck.k9.mail.store.imap.ImapStoreSettings.autoDetectNamespace import com.fsck.k9.mail.store.imap.ImapStoreSettings.pathPrefix @@ -19,6 +21,7 @@ import net.thunderbird.core.android.account.LegacyAccount import net.thunderbird.core.common.mail.Protocols import net.thunderbird.core.logging.Logger import net.thunderbird.core.logging.legacy.Log +import org.intellij.lang.annotations.Language import org.koin.core.component.KoinComponent import org.koin.core.component.inject import org.koin.core.qualifier.named @@ -29,6 +32,7 @@ internal class MigrationTo90( private val db: SQLiteDatabase, private val migrationsHelper: MigrationsHelper, private val logger: Logger = Log, + private val imapStoreFactory: ImapStoreFactory = ImapStore.Companion, ) : KoinComponent { private val trustedSocketFactory: TrustedSocketFactory by inject() private val clientInfoAppName: String by inject(named("ClientInfoAppName")) @@ -44,7 +48,7 @@ internal class MigrationTo90( return } - logger.verbose(TAG) { "started db migration to version 107 to account ${account.uuid}" } + logger.verbose(TAG) { "started db migration to version 90 to account ${account.uuid}" } val imapStore = createImapStore(account) @@ -52,7 +56,7 @@ internal class MigrationTo90( logger.verbose(TAG) { "fetching IMAP prefix" } imapStore.fetchImapPrefix() } catch (e: AuthenticationFailedException) { - logger.warn(TAG, e) { "failed to fetch IMAP prefix." } + logger.warn(TAG, e) { "failed to fetch IMAP prefix. skipping db migration" } return } @@ -60,19 +64,13 @@ internal class MigrationTo90( if (imapPrefix?.isNotBlank() == true) { logger.verbose(TAG) { "Imap Prefix ($imapPrefix) detected, updating folder's server_id" } - val query = """ - |UPDATE folders - | SET server_id = REPLACE(server_id, '$imapPrefix', '') - |WHERE - | server_id LIKE '$imapPrefix%' - """.trimMargin() - + val query = buildQuery(imapPrefix) db.execSQL(query) } else { logger.verbose(TAG) { "No Imap Prefix detected, skipping db migration" } } - logger.verbose(TAG) { "completed db migration to version 107 for account ${account.uuid}" } + logger.verbose(TAG) { "completed db migration to version 90 for account ${account.uuid}" } } private fun createImapStore(account: LegacyAccount): ImapStore { @@ -87,7 +85,7 @@ internal class MigrationTo90( null } - return ImapStore.create( + return imapStoreFactory.create( serverSettings = serverSettings, config = createImapStoreConfig(account), trustedSocketFactory = trustedSocketFactory, @@ -119,4 +117,15 @@ internal class MigrationTo90( override fun clientInfo() = ImapClientInfo(appName = clientInfoAppName, appVersion = clientInfoAppVersion) } } + + @Language("RoomSql") + @VisibleForTesting + internal fun buildQuery(imapPrefix: String): String { + return """ + |UPDATE folders + | SET server_id = REPLACE(server_id, '$imapPrefix', '') + |WHERE + | server_id LIKE '$imapPrefix%' + """.trimMargin() + } } diff --git a/legacy/storage/src/test/java/com/fsck/k9/storage/migrations/MigrationTo90Test.kt b/legacy/storage/src/test/java/com/fsck/k9/storage/migrations/MigrationTo90Test.kt new file mode 100644 index 00000000000..91ef4dfcaab --- /dev/null +++ b/legacy/storage/src/test/java/com/fsck/k9/storage/migrations/MigrationTo90Test.kt @@ -0,0 +1,375 @@ +package com.fsck.k9.storage.migrations + +import android.database.sqlite.SQLiteDatabase +import assertk.all +import assertk.assertThat +import assertk.assertions.hasSize +import assertk.assertions.isEqualTo +import com.fsck.k9.mail.AuthType +import com.fsck.k9.mail.AuthenticationFailedException +import com.fsck.k9.mail.ConnectionSecurity +import com.fsck.k9.mail.ServerSettings +import com.fsck.k9.mail.oauth.OAuth2TokenProviderFactory +import com.fsck.k9.mail.ssl.TrustedSocketFactory +import com.fsck.k9.mail.store.imap.ImapStore +import com.fsck.k9.mail.store.imap.ImapStoreFactory +import com.fsck.k9.mail.store.imap.ImapStoreSettings +import com.fsck.k9.mailstore.MigrationsHelper +import com.fsck.k9.storage.messages.createFolder +import com.fsck.k9.storage.messages.readFolders +import net.thunderbird.core.android.account.LegacyAccount +import net.thunderbird.core.common.mail.Protocols +import net.thunderbird.core.logging.legacy.Log +import net.thunderbird.core.logging.testing.TestLogger +import net.thunderbird.feature.mail.folder.api.FOLDER_DEFAULT_PATH_DELIMITER +import net.thunderbird.feature.mail.folder.api.FolderPathDelimiter +import org.junit.After +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.koin.core.qualifier.named +import org.koin.dsl.module +import org.koin.test.KoinTest +import org.koin.test.KoinTestRule +import org.mockito.kotlin.any +import org.mockito.kotlin.doAnswer +import org.mockito.kotlin.doReturn +import org.mockito.kotlin.mock +import org.mockito.kotlin.spy +import org.mockito.kotlin.times +import org.mockito.kotlin.verify +import org.robolectric.RobolectricTestRunner + +@RunWith(RobolectricTestRunner::class) +@Suppress("MaxLineLength", "LongParameterList") +class MigrationTo90Test : KoinTest { + private val testLogger = TestLogger() + private val database = createDatabaseVersion89() + private val trustedSocketFactory: TrustedSocketFactory = mock() + private val oAuth2TokenProviderFactory: OAuth2TokenProviderFactory = mock() + + @get:Rule + val koinTestRule = KoinTestRule.create { + modules( + module { + single { trustedSocketFactory } + single { oAuth2TokenProviderFactory } + single(named("ClientInfoAppName")) { "MigrationTo90" } + single(named("ClientInfoAppVersion")) { "1.0.0" } + }, + ) + } + + @Before + fun setup() { + Log.logger = testLogger + } + + @After + fun tearDown() { + testLogger.events.clear() + wipeDatabase() + database.close() + } + + @Test + fun `given the user set an empty imap prefix manually - when running the migration - server_id must keep the same value`() { + // Arrange + val folderCount = 100 + populateDatabase( + folderCount = folderCount, + serverIdPrefix = "INBOX", + folderPathDelimiter = FOLDER_DEFAULT_PATH_DELIMITER, + ) + val imapStore = createImapStoreSpy() + val incomingServerSettings = createIncomingServerSettings( + pathPrefix = "", + autoDetectNamespace = false, + ) + val account = createAccount(incomingServerSettings) + val migrationHelper = createMigrationsHelper(account) + val migration = MigrationTo90( + db = database, + migrationsHelper = migrationHelper, + imapStoreFactory = createImapStoreFactory(imapStore), + ) + val expected = database.readFolders().map { it.serverId } + assert(expected.size == folderCount) + + // Act + migration.removeImapPrefixFromFolderServerId() + val actual = database.readFolders().mapNotNull { it.serverId } + testLogger.dumpLogs() + + // Assert + verify(imapStore, times(1)).fetchImapPrefix() + assertThat(actual) + .all { + hasSize(expected.size) + isEqualTo(expected) + } + } + + @Test + fun `given the server returns an empty imap prefix - when running the migration - server_id must keep the same value`() { + // Arrange + val folderCount = 100 + populateDatabase( + folderCount = folderCount, + serverIdPrefix = "INBOX", + folderPathDelimiter = FOLDER_DEFAULT_PATH_DELIMITER, + ) + val imapStore = createImapStoreSpy( + imapPrefix = "", + ) + val incomingServerSettings = createIncomingServerSettings() + val account = createAccount(incomingServerSettings) + val migrationHelper = createMigrationsHelper(account) + val migration = MigrationTo90( + db = database, + migrationsHelper = migrationHelper, + imapStoreFactory = createImapStoreFactory(imapStore), + ) + val expected = database.readFolders().map { it.serverId } + assert(expected.size == folderCount) + + // Act + migration.removeImapPrefixFromFolderServerId() + val actual = database.readFolders().mapNotNull { it.serverId } + testLogger.dumpLogs() + + // Assert + verify(imapStore, times(1)).fetchImapPrefix() + assertThat(actual) + .all { + hasSize(expected.size) + isEqualTo(expected) + } + } + + @Test + fun `given the server return an imap prefix - when folder's server_id includes imap prefix - server_id must remove the prefix`() { + // Arrange + val prefix = "INBOX" + val folderDelimiter = "." + populateDatabase(serverIdPrefix = prefix, folderPathDelimiter = folderDelimiter) + val imapStore = createImapStoreSpy( + imapPrefix = prefix, + folderPathDelimiter = folderDelimiter, + ) + val incomingServerSettings = createIncomingServerSettings(pathPrefix = prefix, autoDetectNamespace = false) + val account = createAccount( + incomingServerSettings = incomingServerSettings, + folderPathDelimiter = folderDelimiter, + ) + val migrationHelper = createMigrationsHelper(account) + val migration = MigrationTo90( + db = database, + migrationsHelper = migrationHelper, + imapStoreFactory = createImapStoreFactory(imapStore), + ) + val expected = database.readFolders().map { it.serverId } + + // Act + migration.removeImapPrefixFromFolderServerId() + val actual = database.readFolders().map { it.serverId } + testLogger.dumpLogs() + + // Assert + verify(imapStore, times(1)).fetchImapPrefix() + + assertThat(actual) + .all { + hasSize(expected.size) + isEqualTo(expected.map { it?.removePrefix("$prefix$folderDelimiter") }) + } + } + + @Test + fun `given a non-imap account - when running the migration - server_id must keep the same value`() { + // Arrange + populateDatabase() + val imapStore = createImapStoreSpy() + val incomingServerSettings = createIncomingServerSettings( + protocolType = Protocols.POP3, + ) + val account = createAccount(incomingServerSettings) + val migrationHelper = createMigrationsHelper(account) + val spyDb = spy { database } + val migration = MigrationTo90( + db = spyDb, + migrationsHelper = migrationHelper, + imapStoreFactory = createImapStoreFactory(imapStore), + ) + val expected = database.readFolders().map { it.serverId } + + // Act + migration.removeImapPrefixFromFolderServerId() + val actual = database.readFolders().mapNotNull { it.serverId } + testLogger.dumpLogs() + + // Assert + verify(imapStore, times(0)).fetchImapPrefix() + verify(spyDb, times(0)).execSQL(any()) + assertThat(actual) + .all { + hasSize(expected.size) + isEqualTo(expected) + } + } + + @Test + fun `given an imap account - when can't fetch imap prefix during the migration - migration should not execute sql queries`() { + // Arrange + populateDatabase() + val prefix = "INBOX." + val imapStore = createImapStoreSpy( + imapPrefix = prefix, + folderPathDelimiter = ".", + authenticationFailedException = AuthenticationFailedException(message = "failed authenticate"), + ) + val incomingServerSettings = createIncomingServerSettings() + val account = createAccount(incomingServerSettings) + val migrationHelper = createMigrationsHelper(account) + val dbSpy = spy { database } + val migration = MigrationTo90( + db = dbSpy, + migrationsHelper = migrationHelper, + imapStoreFactory = createImapStoreFactory(imapStore), + ) + val expected = database.readFolders().map { it.serverId } + val updateQuery = migration.buildQuery(imapPrefix = prefix) + + // Act + migration.removeImapPrefixFromFolderServerId() + val actual = database.readFolders().mapNotNull { it.serverId } + testLogger.dumpLogs() + + // Assert + verify(imapStore, times(1)).fetchImapPrefix() + verify(dbSpy, times(0)).execSQL(updateQuery) + assertThat(actual) + .all { + hasSize(expected.size) + isEqualTo(expected) + } + } + + private fun createIncomingServerSettings( + protocolType: String = Protocols.IMAP, + authType: AuthType = AuthType.NONE, + autoDetectNamespace: Boolean = false, + pathPrefix: String = "", + useCompression: Boolean = false, + isSendClientInfoEnabled: Boolean = false, + ): ServerSettings = ServerSettings( + type = protocolType, + host = "imap.test.com", + port = 993, + connectionSecurity = ConnectionSecurity.SSL_TLS_REQUIRED, + authenticationType = authType, + username = "user", + password = "pass", + clientCertificateAlias = null, + extra = buildMap { + put(ImapStoreSettings.AUTODETECT_NAMESPACE_KEY, autoDetectNamespace.toString()) + put(ImapStoreSettings.PATH_PREFIX_KEY, pathPrefix) + put(ImapStoreSettings.USE_COMPRESSION, useCompression.toString()) + put(ImapStoreSettings.SEND_CLIENT_INFO, isSendClientInfoEnabled.toString()) + }, + ) + + private fun createAccount( + incomingServerSettings: ServerSettings = createIncomingServerSettings(), + oAuthState: String? = null, + folderPathDelimiter: FolderPathDelimiter = FOLDER_DEFAULT_PATH_DELIMITER, + ): LegacyAccount { + return mock { + on { this.incomingServerSettings } doReturn incomingServerSettings + on { this.oAuthState } doReturn oAuthState + on { this.folderPathDelimiter } doReturn folderPathDelimiter + } + } + + private fun createMigrationsHelper(account: LegacyAccount): MigrationsHelper { + return object : MigrationsHelper { + override fun getAccount(): LegacyAccount { + return account + } + + override fun saveAccount() { + throw UnsupportedOperationException("not implemented") + } + } + } + + private fun createImapStoreSpy( + imapPrefix: String? = null, + folderPathDelimiter: FolderPathDelimiter = FOLDER_DEFAULT_PATH_DELIMITER, + authenticationFailedException: AuthenticationFailedException? = null, + ): ImapStore = spy { + on { this.combinedPrefix } doReturn imapPrefix + ?.takeIf { it.isNotBlank() } + ?.let { "$it$folderPathDelimiter" } + + on { fetchImapPrefix() } doAnswer { + if (authenticationFailedException != null) { + throw authenticationFailedException + } + } + } + + private fun createImapStoreFactory(imapStore: ImapStore = createImapStoreSpy()): ImapStoreFactory = + ImapStoreFactory { _, _, _, _ -> + imapStore + } + + private fun createDatabaseVersion89(): SQLiteDatabase = SQLiteDatabase.create(null).apply { + execSQL( + """ + CREATE TABLE folders ( + id INTEGER PRIMARY KEY, + name TEXT, + last_updated INTEGER, + unread_count INTEGER, + visible_limit INTEGER, + status TEXT, + flagged_count INTEGER default 0, + integrate INTEGER, + top_group INTEGER, + sync_enabled INTEGER DEFAULT 0, + push_enabled INTEGER DEFAULT 0, + notifications_enabled INTEGER DEFAULT 0, + more_messages TEXT default "unknown", + server_id TEXT, + local_only INTEGER, + type TEXT DEFAULT "regular", + visible INTEGER DEFAULT 1 + ) + """.trimIndent(), + ) + } + + private fun populateDatabase( + folderCount: Int = 10, + serverIdPrefix: String? = null, + folderPathDelimiter: FolderPathDelimiter = FOLDER_DEFAULT_PATH_DELIMITER, + ) { + repeat(folderCount) { folderNumber -> + val folderName = "Folder $folderNumber" + database.createFolder( + name = folderName, + serverId = serverIdPrefix?.let { prefix -> "$prefix$folderPathDelimiter$folderName" } ?: folderName, + ) + } + } + + private fun wipeDatabase() { + database.delete("folders", null, null) + } + + private fun TestLogger.dumpLogs() { + events.forEach { event -> println(event) } + } +} diff --git a/mail/protocols/imap/src/main/java/com/fsck/k9/mail/store/imap/ImapStoreFactory.kt b/mail/protocols/imap/src/main/java/com/fsck/k9/mail/store/imap/ImapStoreFactory.kt index 39c423fd609..702d1fe528f 100644 --- a/mail/protocols/imap/src/main/java/com/fsck/k9/mail/store/imap/ImapStoreFactory.kt +++ b/mail/protocols/imap/src/main/java/com/fsck/k9/mail/store/imap/ImapStoreFactory.kt @@ -4,7 +4,7 @@ import com.fsck.k9.mail.ServerSettings import com.fsck.k9.mail.oauth.OAuth2TokenProvider import com.fsck.k9.mail.ssl.TrustedSocketFactory -internal fun interface ImapStoreFactory { +fun interface ImapStoreFactory { fun create( serverSettings: ServerSettings, config: ImapStoreConfig, diff --git a/mail/protocols/imap/src/main/java/com/fsck/k9/mail/store/imap/ImapStoreSettings.kt b/mail/protocols/imap/src/main/java/com/fsck/k9/mail/store/imap/ImapStoreSettings.kt index 4252fd9c41d..2c917575a5f 100644 --- a/mail/protocols/imap/src/main/java/com/fsck/k9/mail/store/imap/ImapStoreSettings.kt +++ b/mail/protocols/imap/src/main/java/com/fsck/k9/mail/store/imap/ImapStoreSettings.kt @@ -1,15 +1,23 @@ package com.fsck.k9.mail.store.imap +import androidx.annotation.VisibleForTesting import com.fsck.k9.mail.ServerSettings /** * Extract IMAP-specific server settings from [ServerSettings] */ object ImapStoreSettings { - private const val AUTODETECT_NAMESPACE_KEY = "autoDetectNamespace" - private const val PATH_PREFIX_KEY = "pathPrefix" - private const val SEND_CLIENT_INFO = "sendClientInfo" - private const val USE_COMPRESSION = "useCompression" + @VisibleForTesting + const val AUTODETECT_NAMESPACE_KEY = "autoDetectNamespace" + + @VisibleForTesting + const val PATH_PREFIX_KEY = "pathPrefix" + + @VisibleForTesting + const val SEND_CLIENT_INFO = "sendClientInfo" + + @VisibleForTesting + const val USE_COMPRESSION = "useCompression" @JvmStatic val ServerSettings.autoDetectNamespace: Boolean From d0fa1575470ed241fac5394d3ce1f17517605115 Mon Sep 17 00:00:00 2001 From: Rafael Tonholo Date: Fri, 19 Sep 2025 18:30:36 -0300 Subject: [PATCH 2/3] Merge pull request #9826 from rafaeltonholo/fix/9825/fix-app-crash fix: db migration to version 90 crashing the app and causing database wipe --- .../domain/usecase/GetDisplayTreeFolder.kt | 3 +- .../k9/storage/migrations/MigrationTo90.kt | 33 ++++++++------ .../storage/migrations/MigrationTo90Test.kt | 44 ++++++++++++++++++- 3 files changed, 64 insertions(+), 16 deletions(-) diff --git a/feature/navigation/drawer/dropdown/src/main/kotlin/net/thunderbird/feature/navigation/drawer/dropdown/domain/usecase/GetDisplayTreeFolder.kt b/feature/navigation/drawer/dropdown/src/main/kotlin/net/thunderbird/feature/navigation/drawer/dropdown/domain/usecase/GetDisplayTreeFolder.kt index 8d9caf01262..c48f45dd60d 100644 --- a/feature/navigation/drawer/dropdown/src/main/kotlin/net/thunderbird/feature/navigation/drawer/dropdown/domain/usecase/GetDisplayTreeFolder.kt +++ b/feature/navigation/drawer/dropdown/src/main/kotlin/net/thunderbird/feature/navigation/drawer/dropdown/domain/usecase/GetDisplayTreeFolder.kt @@ -3,6 +3,7 @@ package net.thunderbird.feature.navigation.drawer.dropdown.domain.usecase import kotlinx.collections.immutable.persistentListOf import kotlinx.collections.immutable.toImmutableList import net.thunderbird.core.logging.Logger +import net.thunderbird.feature.mail.folder.api.FOLDER_DEFAULT_PATH_DELIMITER import net.thunderbird.feature.mail.folder.api.Folder import net.thunderbird.feature.mail.folder.api.FolderPathDelimiter import net.thunderbird.feature.mail.folder.api.FolderType @@ -28,7 +29,7 @@ internal class GetDisplayTreeFolder( ) } - val pathDelimiter = folders.first().pathDelimiter + val pathDelimiter = folders.firstOrNull()?.pathDelimiter ?: FOLDER_DEFAULT_PATH_DELIMITER val accountFolders = folders.filterIsInstance().map { val path = flattenPath(it.folder.name, pathDelimiter, maxDepth) logger.debug { "Flattened path for ${it.folder.name} → $path" } diff --git a/legacy/storage/src/main/java/com/fsck/k9/storage/migrations/MigrationTo90.kt b/legacy/storage/src/main/java/com/fsck/k9/storage/migrations/MigrationTo90.kt index c9ca544f89a..23cc5b6688d 100644 --- a/legacy/storage/src/main/java/com/fsck/k9/storage/migrations/MigrationTo90.kt +++ b/legacy/storage/src/main/java/com/fsck/k9/storage/migrations/MigrationTo90.kt @@ -21,6 +21,7 @@ import net.thunderbird.core.android.account.LegacyAccount import net.thunderbird.core.common.mail.Protocols import net.thunderbird.core.logging.Logger import net.thunderbird.core.logging.legacy.Log +import okio.IOException import org.intellij.lang.annotations.Language import org.koin.core.component.KoinComponent import org.koin.core.component.inject @@ -55,22 +56,26 @@ internal class MigrationTo90( try { logger.verbose(TAG) { "fetching IMAP prefix" } imapStore.fetchImapPrefix() - } catch (e: AuthenticationFailedException) { - logger.warn(TAG, e) { "failed to fetch IMAP prefix. skipping db migration" } - return - } - - val imapPrefix = imapStore.combinedPrefix + val imapPrefix = imapStore.combinedPrefix + + if (imapPrefix?.isNotBlank() == true) { + logger.verbose(TAG) { "Imap Prefix ($imapPrefix) detected, updating folder's server_id" } + val query = buildQuery(imapPrefix) + db.execSQL(query) + } else { + logger.verbose(TAG) { "No Imap Prefix detected, skipping db migration" } + } - if (imapPrefix?.isNotBlank() == true) { - logger.verbose(TAG) { "Imap Prefix ($imapPrefix) detected, updating folder's server_id" } - val query = buildQuery(imapPrefix) - db.execSQL(query) - } else { - logger.verbose(TAG) { "No Imap Prefix detected, skipping db migration" } + logger.verbose(TAG) { "completed db migration to version 90 for account ${account.uuid}" } + } catch (e: AuthenticationFailedException) { + logger.warn(TAG, e) { + "failed to fetch IMAP prefix due to authentication error. skipping db migration" + } + } catch (e: IOException) { + logger.warn(TAG, e) { + "failed to fetch IMAP prefix due to network error. skipping db migration" + } } - - logger.verbose(TAG) { "completed db migration to version 90 for account ${account.uuid}" } } private fun createImapStore(account: LegacyAccount): ImapStore { diff --git a/legacy/storage/src/test/java/com/fsck/k9/storage/migrations/MigrationTo90Test.kt b/legacy/storage/src/test/java/com/fsck/k9/storage/migrations/MigrationTo90Test.kt index 91ef4dfcaab..4f14e5a53ea 100644 --- a/legacy/storage/src/test/java/com/fsck/k9/storage/migrations/MigrationTo90Test.kt +++ b/legacy/storage/src/test/java/com/fsck/k9/storage/migrations/MigrationTo90Test.kt @@ -17,6 +17,7 @@ import com.fsck.k9.mail.store.imap.ImapStoreSettings import com.fsck.k9.mailstore.MigrationsHelper import com.fsck.k9.storage.messages.createFolder import com.fsck.k9.storage.messages.readFolders +import java.io.IOException import net.thunderbird.core.android.account.LegacyAccount import net.thunderbird.core.common.mail.Protocols import net.thunderbird.core.logging.legacy.Log @@ -220,7 +221,7 @@ class MigrationTo90Test : KoinTest { } @Test - fun `given an imap account - when can't fetch imap prefix during the migration - migration should not execute sql queries`() { + fun `given an imap account - when can't fetch imap prefix during the migration due to authentication error - migration should not execute sql queries`() { // Arrange populateDatabase() val prefix = "INBOX." @@ -256,6 +257,43 @@ class MigrationTo90Test : KoinTest { } } + @Test + fun `given an imap account - when can't fetch imap prefix during the migration due to network error - migration should not execute sql queries`() { + // Arrange + populateDatabase() + val prefix = "INBOX." + val imapStore = createImapStoreSpy( + imapPrefix = prefix, + folderPathDelimiter = ".", + ioException = IOException("failed to fetch"), + ) + val incomingServerSettings = createIncomingServerSettings() + val account = createAccount(incomingServerSettings) + val migrationHelper = createMigrationsHelper(account) + val dbSpy = spy { database } + val migration = MigrationTo90( + db = dbSpy, + migrationsHelper = migrationHelper, + imapStoreFactory = createImapStoreFactory(imapStore), + ) + val expected = database.readFolders().map { it.serverId } + val updateQuery = migration.buildQuery(imapPrefix = prefix) + + // Act + migration.removeImapPrefixFromFolderServerId() + val actual = database.readFolders().mapNotNull { it.serverId } + testLogger.dumpLogs() + + // Assert + verify(imapStore, times(1)).fetchImapPrefix() + verify(dbSpy, times(0)).execSQL(updateQuery) + assertThat(actual) + .all { + hasSize(expected.size) + isEqualTo(expected) + } + } + private fun createIncomingServerSettings( protocolType: String = Protocols.IMAP, authType: AuthType = AuthType.NONE, @@ -308,6 +346,7 @@ class MigrationTo90Test : KoinTest { imapPrefix: String? = null, folderPathDelimiter: FolderPathDelimiter = FOLDER_DEFAULT_PATH_DELIMITER, authenticationFailedException: AuthenticationFailedException? = null, + ioException: IOException? = null, ): ImapStore = spy { on { this.combinedPrefix } doReturn imapPrefix ?.takeIf { it.isNotBlank() } @@ -317,6 +356,9 @@ class MigrationTo90Test : KoinTest { if (authenticationFailedException != null) { throw authenticationFailedException } + if (ioException != null) { + throw ioException + } } } From be298f2f616b2d4669afd07f60751d603415116a Mon Sep 17 00:00:00 2001 From: Rafael Tonholo Date: Mon, 22 Sep 2025 12:12:36 -0300 Subject: [PATCH 3/3] Merge pull request #9837 from rafaeltonholo/chore/exclude-outbox-folder-from-migration-90 chore(migration-v90): add extra where clause to ensure the outbox folder or local folder's won't change --- .../k9/storage/migrations/MigrationTo90.kt | 5 +- .../storage/migrations/MigrationTo90Test.kt | 76 +++++++++++++++++++ 2 files changed, 80 insertions(+), 1 deletion(-) diff --git a/legacy/storage/src/main/java/com/fsck/k9/storage/migrations/MigrationTo90.kt b/legacy/storage/src/main/java/com/fsck/k9/storage/migrations/MigrationTo90.kt index 23cc5b6688d..b0d26f82de4 100644 --- a/legacy/storage/src/main/java/com/fsck/k9/storage/migrations/MigrationTo90.kt +++ b/legacy/storage/src/main/java/com/fsck/k9/storage/migrations/MigrationTo90.kt @@ -130,7 +130,10 @@ internal class MigrationTo90( |UPDATE folders | SET server_id = REPLACE(server_id, '$imapPrefix', '') |WHERE - | server_id LIKE '$imapPrefix%' + | server_id IS NOT NULL + | AND server_id LIKE '$imapPrefix%' + | AND type <> 'outbox' + | AND local_only <> 1 """.trimMargin() } } diff --git a/legacy/storage/src/test/java/com/fsck/k9/storage/migrations/MigrationTo90Test.kt b/legacy/storage/src/test/java/com/fsck/k9/storage/migrations/MigrationTo90Test.kt index 4f14e5a53ea..503801c3992 100644 --- a/legacy/storage/src/test/java/com/fsck/k9/storage/migrations/MigrationTo90Test.kt +++ b/legacy/storage/src/test/java/com/fsck/k9/storage/migrations/MigrationTo90Test.kt @@ -3,6 +3,7 @@ package com.fsck.k9.storage.migrations import android.database.sqlite.SQLiteDatabase import assertk.all import assertk.assertThat +import assertk.assertions.contains import assertk.assertions.hasSize import assertk.assertions.isEqualTo import com.fsck.k9.mail.AuthType @@ -187,6 +188,80 @@ class MigrationTo90Test : KoinTest { } } + @Test + fun `given the server return an imap prefix - when folder's is local only - server_id must not be changed`() { + // Arrange + val prefix = "INBOX" + val folderDelimiter = "." + populateDatabase(serverIdPrefix = prefix, folderPathDelimiter = folderDelimiter) + val localOnlyServerId = "$prefix${folderDelimiter}Local Only" + + database.createFolder("Local Only", isLocalOnly = true, serverId = localOnlyServerId) + + val imapStore = createImapStoreSpy( + imapPrefix = prefix, + folderPathDelimiter = folderDelimiter, + ) + val incomingServerSettings = createIncomingServerSettings(pathPrefix = prefix, autoDetectNamespace = false) + val account = createAccount( + incomingServerSettings = incomingServerSettings, + folderPathDelimiter = folderDelimiter, + ) + val migrationHelper = createMigrationsHelper(account) + val migration = MigrationTo90( + db = database, + migrationsHelper = migrationHelper, + imapStoreFactory = createImapStoreFactory(imapStore), + ) + + // Act + migration.removeImapPrefixFromFolderServerId() + val actual = database.readFolders().map { it.serverId } + testLogger.dumpLogs() + + // Assert + verify(imapStore, times(1)).fetchImapPrefix() + + assertThat(actual).contains(localOnlyServerId) + } + + @Test + fun `given the server return an imap prefix - when folder's is an outbox - server_id must not be changed`() { + // Arrange + val prefix = "INBOX" + val folderDelimiter = "." + populateDatabase(serverIdPrefix = prefix, folderPathDelimiter = folderDelimiter) + val outboxFolderServerId = "$prefix${folderDelimiter}Outbox" + + database.createFolder("Outbox", isLocalOnly = true, serverId = outboxFolderServerId, type = "outbox") + + val imapStore = createImapStoreSpy( + imapPrefix = prefix, + folderPathDelimiter = folderDelimiter, + ) + val incomingServerSettings = createIncomingServerSettings(pathPrefix = prefix, autoDetectNamespace = false) + val account = createAccount( + incomingServerSettings = incomingServerSettings, + folderPathDelimiter = folderDelimiter, + ) + val migrationHelper = createMigrationsHelper(account) + val migration = MigrationTo90( + db = database, + migrationsHelper = migrationHelper, + imapStoreFactory = createImapStoreFactory(imapStore), + ) + + // Act + migration.removeImapPrefixFromFolderServerId() + val actual = database.readFolders().map { it.serverId } + testLogger.dumpLogs() + + // Assert + verify(imapStore, times(1)).fetchImapPrefix() + + assertThat(actual).contains(outboxFolderServerId) + } + @Test fun `given a non-imap account - when running the migration - server_id must keep the same value`() { // Arrange @@ -403,6 +478,7 @@ class MigrationTo90Test : KoinTest { database.createFolder( name = folderName, serverId = serverIdPrefix?.let { prefix -> "$prefix$folderPathDelimiter$folderName" } ?: folderName, + isLocalOnly = false, ) } }