Merge main into perf-187-covering-index

This commit is contained in:
Jason Ross
2026-07-04 03:01:21 -05:00
committed by GitHub
13 changed files with 297 additions and 13 deletions
@@ -67,7 +67,9 @@ class AccountDaoTest {
}
@Test
fun upsertReplacesAnAccountWithTheSameId() = runBlocking {
fun upsertOnAConflictingIdUpdatesTheRowInPlace() = runBlocking {
// Non-destructive by design (issue #309): a second upsert of the same id refreshes the row
// rather than delete-then-reinserting it (which would cascade-delete settings/signatures).
dao.upsert(account("acct", "ada@example.org", displayName = "Ada"))
dao.upsert(account("acct", "ada@example.org", displayName = "Ada Lovelace"))
@@ -110,4 +110,40 @@ class AccountDatabaseTest {
db.signatureDao().observeForAccount("acct").first().isEmpty(),
)
}
@Test
fun upsertOnAnExistingIdUpdatesInPlaceWithoutCascadingSettingsOrSignatures() = runBlocking<Unit> {
// Regression for issue #309: an @Insert(REPLACE) upsert deletes-then-reinserts on an id
// conflict, firing the ON DELETE CASCADE that wipes the account's settings + signatures.
db.accountDao().upsert(account("acct"))
db.accountSettingsDao().upsert(AccountSettingsEntity("acct", signature = "Keep me"))
db.signatureDao().upsert(SignatureEntity("sig-1", "acct", "Work", "<p>Regards</p>", isDefault = true))
db.accountDao().upsert(account("acct").copy(displayName = "Updated"))
assertEquals("Updated", db.accountDao().getById("acct")?.displayName)
assertEquals("Keep me", db.accountSettingsDao().get("acct")?.signature)
assertEquals(1, db.signatureDao().observeForAccount("acct").first().size)
assertEquals(1, db.accountDao().getAll().size)
}
@Test
fun insertAtEndReAddingAnExistingAccountKeepsItsSettingsSignaturesAndPosition() = runBlocking<Unit> {
// The production re-add path (addOutlookAccount -> insertAtEnd -> upsert). Re-adding a
// deterministic id must not cascade-delete its settings/signatures nor move it to the end.
db.accountDao().insertAtEnd(account("a"))
db.accountDao().insertAtEnd(account("b"))
db.accountSettingsDao().upsert(AccountSettingsEntity("b", signature = "Sig B", signatureEnabled = false))
db.signatureDao().upsert(SignatureEntity("sig-b", "b", "Work", "<p>Regards</p>", isDefault = true))
db.accountDao().insertAtEnd(account("b").copy(displayName = "Renamed"))
// Updated in place, position preserved (still 1, not appended after), children intact.
assertEquals("Renamed", db.accountDao().getById("b")?.displayName)
assertEquals(1, db.accountDao().getById("b")?.sortOrder)
assertEquals("Sig B", db.accountSettingsDao().get("b")?.signature)
assertEquals(false, db.accountSettingsDao().get("b")?.signatureEnabled)
assertEquals(1, db.signatureDao().observeForAccount("b").first().size)
assertEquals(2, db.accountDao().getAll().size)
}
}
@@ -6,6 +6,7 @@ import androidx.room.Insert
import androidx.room.OnConflictStrategy
import androidx.room.Query
import androidx.room.Transaction
import androidx.room.Update
import kotlinx.coroutines.flow.Flow
import org.libremail.data.local.entity.AccountEntity
@@ -27,8 +28,30 @@ interface AccountDao {
@Query("SELECT * FROM accounts WHERE id = :id LIMIT 1")
suspend fun getById(id: String): AccountEntity?
@Insert(onConflict = OnConflictStrategy.REPLACE)
suspend fun upsert(account: AccountEntity)
/**
* Insert [account] only if its id is absent; a conflicting id is left untouched (returns -1).
* Non-destructive by design: an `@Insert(REPLACE)` would delete-then-reinsert the row on an id
* conflict, firing the `ON DELETE CASCADE` that permanently drops the account's `account_settings`
* + `signatures` (issue #309). Pair it with [update] to refresh an existing row in place instead.
*/
@Insert(onConflict = OnConflictStrategy.IGNORE)
suspend fun insertIfAbsent(account: AccountEntity): Long
/** Refreshes an existing account's columns in place (matched by primary key); no cascade. */
@Update
suspend fun update(account: AccountEntity)
/**
* Insert [account], or refresh the existing row **in place** when its id is already present —
* never the delete-then-reinsert an `@Insert(REPLACE)` does, so it does NOT cascade-delete the
* account's `account_settings` + `signatures` (issue #309). Assigns no list position: new accounts
* are appended via [insertAtEnd]; a plain upsert leaves [AccountEntity.sortOrder] as supplied.
*/
@Transaction
suspend fun upsert(account: AccountEntity) {
// -1 = the IGNORE insert was skipped because the id already exists → update the row instead.
if (insertIfAbsent(account) == -1L) update(account)
}
/** The sortOrder that appends a new account to the end of the list (0 when there are none yet). */
@Query("SELECT COALESCE(MAX(sortOrder), -1) + 1 FROM accounts")
@@ -36,12 +59,19 @@ interface AccountDao {
/**
* Insert [account] at the end of the user-defined order (issue #164), stamping it with the current
* max + 1. Both steps run in one transaction so two near-simultaneous adds can't collide on a
* sortOrder.
* max + 1. Re-adding an already-present id (e.g. re-authing an Outlook account, whose id is the
* deterministic `outlook:<email>`) instead refreshes the row in place, keeping its list position
* and — crucially — its settings + signatures, which a REPLACE would cascade-delete (issue #309).
* Runs in one transaction so the read + write can't interleave with a concurrent add.
*/
@Transaction
suspend fun insertAtEnd(account: AccountEntity) {
upsert(account.copy(sortOrder = nextSortOrder()))
val existing = getById(account.id)
if (existing == null) {
insertIfAbsent(account.copy(sortOrder = nextSortOrder()))
} else {
update(account.copy(sortOrder = existing.sortOrder))
}
}
@Query("UPDATE accounts SET sortOrder = :sortOrder WHERE id = :id")
@@ -23,9 +23,17 @@ interface DraftDao {
@Query("SELECT * FROM drafts")
suspend fun getAll(): List<DraftEntity>
/** The drafts composed under [accountId] — enumerated to release their URI grants on account delete (#299). */
@Query("SELECT * FROM drafts WHERE accountId = :accountId")
suspend fun getByAccount(accountId: String): List<DraftEntity>
@Insert(onConflict = OnConflictStrategy.REPLACE)
suspend fun upsert(draft: DraftEntity)
@Query("DELETE FROM drafts WHERE id = :id")
suspend fun delete(id: String)
/** Removes every draft composed under [accountId] (the account is being deleted, #299). */
@Query("DELETE FROM drafts WHERE accountId = :accountId")
suspend fun deleteByAccount(accountId: String)
}
@@ -217,6 +217,15 @@ interface MessageDao {
@Query("DELETE FROM messages WHERE id IN (:ids)")
suspend fun deleteByIds(ids: List<String>)
/**
* Ids of every row belonging to [accountId] (all folders; synced rows plus transient search-only
* hits). Lets [org.libremail.data.repository.AccountRepositoryImpl.deleteAccount] enumerate a
* deleted account's messages to purge their on-disk attachment cache before [deleteByAccount]
* removes the rows that name them (issue #299).
*/
@Query("SELECT id FROM messages WHERE accountId = :accountId")
suspend fun getIdsForAccount(accountId: String): List<String>
@Query("DELETE FROM messages WHERE accountId = :accountId")
suspend fun deleteByAccount(accountId: String)
@@ -1,15 +1,21 @@
// SPDX-License-Identifier: GPL-3.0-or-later
package org.libremail.data.repository
import android.content.Context
import dagger.hilt.android.qualifiers.ApplicationContext
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.map
import org.libremail.data.attachment.AttachmentUriGrants
import org.libremail.data.attachmentCacheDir
import org.libremail.data.local.dao.AccountDao
import org.libremail.data.local.dao.BackfillProgressDao
import org.libremail.data.local.dao.DraftDao
import org.libremail.data.local.dao.FolderDao
import org.libremail.data.local.dao.MessageDao
import org.libremail.data.local.toDomain
import org.libremail.data.local.toEntity
import org.libremail.data.local.toImapParams
import org.libremail.data.local.toOutgoingAttachments
import org.libremail.data.security.CredentialStore
import org.libremail.data.settings.AccountSettingsRepository
import org.libremail.data.sync.SyncScheduler
@@ -23,15 +29,18 @@ import javax.inject.Singleton
@Singleton
class AccountRepositoryImpl @Inject constructor(
@ApplicationContext private val context: Context,
private val accountDao: AccountDao,
private val messageDao: MessageDao,
private val folderDao: FolderDao,
private val backfillProgressDao: BackfillProgressDao,
private val draftDao: DraftDao,
private val credentialStore: CredentialStore,
private val imapClient: ImapClient,
private val syncScheduler: SyncScheduler,
private val accountSettingsRepository: AccountSettingsRepository,
private val mailNotifier: MailNotifier,
private val attachmentUriGrants: AttachmentUriGrants,
) : AccountRepository {
override fun observeAccounts(): Flow<List<Account>> = accountDao.observeAll().map { rows ->
@@ -72,14 +81,32 @@ class AccountRepositoryImpl @Inject constructor(
override suspend fun reorderAccounts(orderedIds: List<String>) = accountDao.reorder(orderedIds)
override suspend fun deleteAccount(id: String) {
// Enumerate the on-disk artifacts to clean up WHILE the rows that name them still exist: once
// the message + draft rows are gone, nothing can recover those message ids / draft URIs again,
// so the files/grants would leak forever (issue #299).
val messageIds = messageDao.getIdsForAccount(id)
val draftUris = draftDao.getByAccount(id)
.flatMap { it.attachments.toOutgoingAttachments() }
.map { it.uri }
accountDao.deleteById(id)
credentialStore.delete(id)
mailNotifier.deleteAccountChannel(id)
// Remove the account's cached mail (attachment rows cascade via the foreign key), folders, and
// backfill progress. The account_settings row is removed automatically by its cascading FK.
// Remove the account's cached mail (attachment rows cascade via the foreign key), folders,
// backfill progress, and drafts. The account_settings + signatures rows are removed
// automatically by their cascading FK when the account row above is deleted.
messageDao.deleteByAccount(id)
folderDao.deleteForAccount(id)
backfillProgressDao.deleteForAccount(id)
draftDao.deleteByAccount(id)
// Now the rows are gone: delete each message's on-disk attachment cache (keyed by message id,
// the same path MailRepositoryImpl writes), and release any persistable draft-URI grant no
// remaining draft/outbox row still needs (issue #299).
messageIds.forEach { messageId ->
runCatching { attachmentCacheDir(context.cacheDir, messageId).deleteRecursively() }
}
attachmentUriGrants.releaseUnreferenced(draftUris)
}
override suspend fun resetBackfillProgress(accountId: String?) {
@@ -22,9 +22,13 @@ data class Account(
private const val OUTLOOK_IMAP_PORT = 993
private const val OUTLOOK_SMTP_PORT = 587
/** An Outlook/Microsoft account using the unified office365 endpoints (personal + M365). */
/**
* An Outlook/Microsoft account using the unified office365 endpoints (personal + M365). The id
* lowercases the whole address ([normalizeEmailForAccountId] with `lowercaseLocalPart = true`)
* so a re-auth with differently-cased casing resolves to the same account (issue #305).
*/
fun outlook(email: String, displayName: String = email): Account = Account(
id = "outlook:$email",
id = "outlook:${normalizeEmailForAccountId(email, lowercaseLocalPart = true)}",
email = email,
displayName = displayName.ifBlank { email },
authType = AuthType.OAUTH_OUTLOOK,
@@ -33,3 +37,23 @@ data class Account(
)
}
}
/**
* Normalizes [email] for use in an account id (issue #305): a mailbox is one identity regardless of
* how the address is cased, and the id is the primary key. The domain is always lowercased — mail
* domains are case-insensitive — so `user@Gmail.com` and `user@gmail.com` never spawn two accounts
* syncing one mailbox. The local part is lowercased only when [lowercaseLocalPart] is true: the major
* consumer providers (Gmail, Yahoo, iCloud, AOL, Outlook) treat the whole address case-insensitively,
* but an arbitrary manually-configured IMAP server's local part MAY be case-sensitive (RFC 5321
* §2.3.11), so a generic account normalizes only the domain. Surrounding whitespace is trimmed either
* way. An address with no `@` has no domain to isolate, so it is lowercased whole (consumer) or left
* as-is (generic).
*/
fun normalizeEmailForAccountId(email: String, lowercaseLocalPart: Boolean): String {
val trimmed = email.trim()
val at = trimmed.lastIndexOf('@')
if (at < 0) return if (lowercaseLocalPart) trimmed.lowercase() else trimmed
val local = trimmed.substring(0, at)
val domain = trimmed.substring(at + 1).lowercase()
return "${if (lowercaseLocalPart) local.lowercase() else local}@$domain"
}
@@ -116,7 +116,9 @@ enum class MailProvider(
fun createAccount(email: String, displayName: String = email): Account {
val address = email.trim()
return Account(
id = "imap:$address",
// Consumer providers treat addresses case-insensitively, so the id lowercases the whole
// address (issue #305) while the displayed [email] keeps the user's casing.
id = "imap:${normalizeEmailForAccountId(address, lowercaseLocalPart = true)}",
email = address,
displayName = displayName.trim().ifBlank { address },
authType = AuthType.PASSWORD_IMAP,
@@ -13,6 +13,7 @@ import org.libremail.domain.model.Account
import org.libremail.domain.model.AuthType
import org.libremail.domain.model.MailSecurity
import org.libremail.domain.model.ServerConfig
import org.libremail.domain.model.normalizeEmailForAccountId
import org.libremail.domain.repository.AccountRepository
import javax.inject.Inject
@@ -65,7 +66,9 @@ class ManualSetupViewModel @Inject constructor(private val accountRepository: Ac
return
}
val account = Account(
id = "imap:${f.email.trim()}",
// Generic IMAP: lowercase only the domain for the id (issue #305). An arbitrary server's
// local part MAY be case-sensitive (RFC 5321), so it is left as typed.
id = "imap:${normalizeEmailForAccountId(f.email, lowercaseLocalPart = false)}",
email = f.email.trim(),
displayName = f.email.trim(),
authType = AuthType.PASSWORD_IMAP,
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-3.0-or-later
package org.libremail.data.repository
import android.content.Context
import app.cash.turbine.test
import io.mockk.Runs
import io.mockk.coEvery
@@ -13,11 +14,15 @@ import io.mockk.verify
import kotlinx.coroutines.flow.flowOf
import kotlinx.coroutines.test.runTest
import org.junit.Test
import org.libremail.data.attachment.AttachmentUriGrants
import org.libremail.data.attachmentCacheDir
import org.libremail.data.local.dao.AccountDao
import org.libremail.data.local.dao.BackfillProgressDao
import org.libremail.data.local.dao.DraftDao
import org.libremail.data.local.dao.FolderDao
import org.libremail.data.local.dao.MessageDao
import org.libremail.data.local.entity.AccountEntity
import org.libremail.data.local.entity.DraftEntity
import org.libremail.data.local.entity.ServerConfigEmbedded
import org.libremail.data.security.CredentialStore
import org.libremail.data.settings.AccountSettingsRepository
@@ -30,6 +35,7 @@ import org.libremail.domain.model.ServerConfig
import org.libremail.mail.FetchedFolder
import org.libremail.mail.ImapClient
import org.libremail.notifications.MailNotifier
import java.io.File
import kotlin.test.assertEquals
import kotlin.test.assertFalse
import kotlin.test.assertTrue
@@ -42,26 +48,32 @@ import kotlin.test.assertTrue
*/
class AccountRepositoryImplTest {
private val context = mockk<Context>(relaxed = true)
private val accountDao = mockk<AccountDao>()
private val messageDao = mockk<MessageDao>(relaxed = true)
private val folderDao = mockk<FolderDao>(relaxed = true)
private val backfillProgressDao = mockk<BackfillProgressDao>(relaxed = true)
private val draftDao = mockk<DraftDao>(relaxed = true)
private val credentialStore = mockk<CredentialStore>(relaxed = true)
private val imapClient = mockk<ImapClient>()
private val syncScheduler = mockk<SyncScheduler>(relaxed = true)
private val accountSettingsRepository = mockk<AccountSettingsRepository>(relaxed = true)
private val mailNotifier = mockk<MailNotifier>(relaxed = true)
private val attachmentUriGrants = mockk<AttachmentUriGrants>(relaxed = true)
private val repository = AccountRepositoryImpl(
context = context,
accountDao = accountDao,
messageDao = messageDao,
folderDao = folderDao,
backfillProgressDao = backfillProgressDao,
draftDao = draftDao,
credentialStore = credentialStore,
imapClient = imapClient,
syncScheduler = syncScheduler,
accountSettingsRepository = accountSettingsRepository,
mailNotifier = mailNotifier,
attachmentUriGrants = attachmentUriGrants,
)
@Test
@@ -180,6 +192,37 @@ class AccountRepositoryImplTest {
coVerify { messageDao.deleteByAccount("acct") }
coVerify { folderDao.deleteForAccount("acct") }
coVerify { backfillProgressDao.deleteForAccount("acct") }
coVerify { draftDao.deleteByAccount("acct") }
}
@Test
fun `deleteAccount purges on-disk attachment caches and releases draft URI grants`() = runTest {
// A real temp cacheDir with one message's attachment cache dir populated on disk.
val cacheDir = File(System.getProperty("java.io.tmpdir"), "libremail-delete-test-${System.nanoTime()}")
every { context.cacheDir } returns cacheDir
val messageId = "acct:INBOX:1"
val attachmentDir = attachmentCacheDir(cacheDir, messageId).apply { mkdirs() }
File(attachmentDir, "0/report.pdf").apply { parentFile?.mkdirs() }.writeText("bytes")
// The account owns that message and a draft carrying a picked attachment URI.
coEvery { accountDao.deleteById("acct") } just Runs
coEvery { messageDao.getIdsForAccount("acct") } returns listOf(messageId)
coEvery { draftDao.getByAccount("acct") } returns listOf(
draftEntity(id = "d1", attachments = """[{"uri":"content://pick/1","name":"report.pdf"}]"""),
)
val released = slot<Collection<String>>()
coEvery { attachmentUriGrants.releaseUnreferenced(capture(released)) } just Runs
repository.deleteAccount("acct")
// The message ids must be read BEFORE the rows are deleted, and the on-disk cache purged.
coVerify { messageDao.getIdsForAccount("acct") }
assertFalse(attachmentDir.exists(), "attachment cache dir must be deleted with the account")
// The deleted draft's picked-URI grant is released.
coVerify { draftDao.deleteByAccount("acct") }
assertEquals(listOf("content://pick/1"), released.captured.toList())
cacheDir.deleteRecursively()
}
@Test
@@ -227,6 +270,17 @@ class AccountRepositoryImplTest {
smtp = ServerConfigEmbedded("smtp.example.org", 465, "SSL_TLS"),
)
private fun draftEntity(id: String, attachments: String) = DraftEntity(
id = id,
accountId = "acct",
toAddresses = "",
ccAddresses = "",
subject = "",
body = "",
updatedAt = 0L,
attachments = attachments,
)
private val params = ImapConnectionParams(
host = "imap.example.org",
port = 993,
@@ -0,0 +1,58 @@
// SPDX-License-Identifier: GPL-3.0-or-later
package org.libremail.domain.model
import org.junit.Test
import kotlin.test.assertEquals
/**
* Locks down account-id normalization (issue #305): the id is the primary key, so casing that varies
* between adds must not spawn a second account syncing the same mailbox. [normalizeEmailForAccountId]
* always lowercases the domain and — for consumer providers — the whole address; the generic path
* leaves the local part as typed. [Account.outlook] derives a deterministic id from the address.
*/
class AccountTest {
@Test
fun `normalize always lowercases the domain, keeping the local part when not a consumer provider`() {
assertEquals("User@example.org", normalizeEmailForAccountId("User@Example.ORG", lowercaseLocalPart = false))
assertEquals("ADA@gmail.com", normalizeEmailForAccountId("ADA@GMAIL.COM", lowercaseLocalPart = false))
}
@Test
fun `normalize lowercases the whole address for consumer providers`() {
assertEquals("user@example.org", normalizeEmailForAccountId("User@Example.ORG", lowercaseLocalPart = true))
assertEquals("ada@gmail.com", normalizeEmailForAccountId("ADA@GMAIL.COM", lowercaseLocalPart = true))
}
@Test
fun `normalize trims surrounding whitespace either way`() {
assertEquals("user@example.org", normalizeEmailForAccountId(" user@Example.org ", lowercaseLocalPart = false))
assertEquals("user@example.org", normalizeEmailForAccountId(" User@Example.org ", lowercaseLocalPart = true))
}
@Test
fun `normalize maps different casings of one address to the same value`() {
assertEquals(
normalizeEmailForAccountId("User@Gmail.com", lowercaseLocalPart = true),
normalizeEmailForAccountId("user@gmail.com", lowercaseLocalPart = true),
)
}
@Test
fun `normalize handles an address with no at-sign without throwing`() {
// No domain to isolate: lowercased whole for consumer, left as-is for generic.
assertEquals("root", normalizeEmailForAccountId("Root", lowercaseLocalPart = true))
assertEquals("Root", normalizeEmailForAccountId("Root", lowercaseLocalPart = false))
}
@Test
fun `outlook derives a lowercased, deterministic id but keeps the displayed email casing`() {
val account = Account.outlook("User@Outlook.com")
assertEquals("outlook:user@outlook.com", account.id)
assertEquals("User@Outlook.com", account.email)
assertEquals(AuthType.OAUTH_OUTLOOK, account.authType)
// Any casing of the same address resolves to one id, so a re-auth never duplicates the account.
assertEquals(Account.outlook("USER@OUTLOOK.COM").id, account.id)
}
}
@@ -145,11 +145,23 @@ class MailProviderTest {
fun `createAccount trims the email and derives a stable id and display name`() {
val account = MailProvider.GMAIL.createAccount(" User@Gmail.com ")
// The displayed email/name keep the user's casing, but the id lowercases the whole address so
// it is stable across casing (issue #305).
assertEquals("User@Gmail.com", account.email)
assertEquals("imap:User@Gmail.com", account.id)
assertEquals("imap:user@gmail.com", account.id)
assertEquals("User@Gmail.com", account.displayName)
}
@Test
fun `createAccount lowercases the whole address in the id so casing never duplicates an account`() {
// A consumer provider treats the address case-insensitively, so any casing maps to one id.
assertEquals(
MailProvider.GMAIL.createAccount("User@Gmail.com").id,
MailProvider.GMAIL.createAccount("user@gmail.com").id,
)
assertEquals("imap:user@gmail.com", MailProvider.GMAIL.createAccount("USER@GMAIL.COM").id)
}
@Test
fun `createAccount keeps an explicit non-blank display name`() {
val account = MailProvider.GMAIL.createAccount("user@gmail.com", displayName = "Work")
@@ -135,6 +135,25 @@ class ManualSetupViewModelTest {
assertEquals("imap:user@example.org", vm.form.value.addedAccountId)
}
@Test
fun `testAndSave lowercases the domain in the id but keeps the local-part casing`() = runTest(dispatcher) {
// Generic IMAP servers may treat the local part case-sensitively, so only the domain is
// lowercased for the id (issue #305); the displayed email keeps the typed casing.
val repo = mockk<AccountRepository>()
val account = slot<Account>()
coEvery { repo.addImapAccount(capture(account), any()) } returns Result.success(emptyList())
val vm = ManualSetupViewModel(repo)
vm.onEmail(" User@Example.ORG ")
vm.onPassword("secret")
vm.onImapHost("imap.example.org")
vm.onSmtpHost("smtp.example.org")
vm.testAndSave()
assertEquals("imap:User@example.org", account.captured.id)
assertEquals("User@Example.ORG", account.captured.email)
}
@Test
fun `testAndSave falls back to the default ports when the port fields are blank`() = runTest(dispatcher) {
val repo = mockk<AccountRepository>()