Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Unverified Commit 2c9f07a9 authored by Stephen King's avatar Stephen King Committed by GitHub
Browse files

Merge pull request #9827 from rafaeltonholo/uplift/beta/9826/fix-db-migration-crash

uplift(beta): fix db migration 90 and drawer app crashes
parents d7c898a5 be298f2f
Loading
Loading
Loading
Loading
+2 −1
Original line number Diff line number Diff line
@@ -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<MailDisplayFolder>().map {
            val path = flattenPath(it.folder.name, pathDelimiter, maxDepth)
            logger.debug { "Flattened path for ${it.folder.name} → $path" }
+39 −22
Original line number Diff line number Diff line
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,8 @@ 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
import org.koin.core.qualifier.named
@@ -29,6 +33,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,35 +49,33 @@ 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)

        try {
            logger.verbose(TAG) { "fetching IMAP prefix" }
            imapStore.fetchImapPrefix()
        } catch (e: AuthenticationFailedException) {
            logger.warn(TAG, e) { "failed to fetch IMAP prefix." }
            return
        }

            val imapPrefix = imapStore.combinedPrefix

            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}" }
        } 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"
            }
        }
    }

    private fun createImapStore(account: LegacyAccount): ImapStore {
@@ -87,7 +90,7 @@ internal class MigrationTo90(
            null
        }

        return ImapStore.create(
        return imapStoreFactory.create(
            serverSettings = serverSettings,
            config = createImapStoreConfig(account),
            trustedSocketFactory = trustedSocketFactory,
@@ -119,4 +122,18 @@ 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 IS NOT NULL
            |    AND server_id LIKE '$imapPrefix%'
            |    AND type <> 'outbox'
            |    AND local_only <> 1
        """.trimMargin()
    }
}
+493 −0
Original line number Diff line number Diff line
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
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 java.io.IOException
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> { trustedSocketFactory }
                single<OAuth2TokenProviderFactory> { 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 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
        populateDatabase()
        val imapStore = createImapStoreSpy()
        val incomingServerSettings = createIncomingServerSettings(
            protocolType = Protocols.POP3,
        )
        val account = createAccount(incomingServerSettings)
        val migrationHelper = createMigrationsHelper(account)
        val spyDb = spy<SQLiteDatabase> { 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 due to authentication error - 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<SQLiteDatabase> { 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)
            }
    }

    @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<SQLiteDatabase> { 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,
        ioException: IOException? = null,
    ): ImapStore = spy<ImapStore> {
        on { this.combinedPrefix } doReturn imapPrefix
            ?.takeIf { it.isNotBlank() }
            ?.let { "$it$folderPathDelimiter" }

        on { fetchImapPrefix() } doAnswer {
            if (authenticationFailedException != null) {
                throw authenticationFailedException
            }
            if (ioException != null) {
                throw ioException
            }
        }
    }

    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,
                isLocalOnly = false,
            )
        }
    }

    private fun wipeDatabase() {
        database.delete("folders", null, null)
    }

    private fun TestLogger.dumpLogs() {
        events.forEach { event -> println(event) }
    }
}
+1 −1
Original line number Diff line number Diff line
@@ -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,
+12 −4
Original line number Diff line number Diff line
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