Merge main into fix-303-304-ui-correctness
This commit is contained in:
Jason Ross
@@ -326,7 +326,7 @@ class MailRepositoryImpl @Inject constructor(
|
||||
val routings = messageDao.getRoutingByIds(ids)
|
||||
messageDao.deleteByIds(ids) // optimistic
|
||||
forEachAccountFolder(routings) { params, folder, group ->
|
||||
group.forEach { imapClient.deleteMessage(params, folder, uidOf(it.id)) }
|
||||
imapClient.deleteMessages(params, folder, group.map { uidOf(it.id) })
|
||||
}
|
||||
}
|
||||
|
||||
@@ -387,7 +387,7 @@ class MailRepositoryImpl @Inject constructor(
|
||||
when (val dest = destByAccount[group.first().accountId]) {
|
||||
null ->
|
||||
if (fallbackExpunge) {
|
||||
group.forEach { imapClient.deleteMessage(params, folder, uidOf(it.id)) }
|
||||
imapClient.deleteMessages(params, folder, group.map { uidOf(it.id) })
|
||||
} else {
|
||||
error("No ${role.name.lowercase()} folder for this account")
|
||||
}
|
||||
|
||||
@@ -348,22 +348,43 @@ class ImapClient(private val reuseConnections: Boolean) {
|
||||
}
|
||||
}
|
||||
|
||||
suspend fun deleteMessage(params: ImapConnectionParams, folder: String, uid: String) = withContext(Dispatchers.IO) {
|
||||
withStore(params) { store ->
|
||||
val mailbox = store.getFolder(folder)
|
||||
mailbox.open(Folder.READ_WRITE)
|
||||
try {
|
||||
(mailbox as UIDFolder).getMessageByUID(uid.toLong())?.setFlag(Flags.Flag.DELETED, true)
|
||||
mailbox.expunge()
|
||||
} finally {
|
||||
runCatching { mailbox.close(false) }
|
||||
/**
|
||||
* Deletes a single message by UID. Delegates to [deleteMessages] so even a lone delete uses the
|
||||
* same **targeted** UID EXPUNGE — never the untargeted expunge that would also remove other
|
||||
* `\Deleted`-flagged mail in the folder (issue #295).
|
||||
*/
|
||||
suspend fun deleteMessage(params: ImapConnectionParams, folder: String, uid: String) =
|
||||
deleteMessages(params, folder, listOf(uid))
|
||||
|
||||
/**
|
||||
* Permanently deletes the messages with [uids] from [folder] in a **single** IMAP session: opens
|
||||
* the folder once, flags the matched messages `\Deleted`, then issues one targeted UID EXPUNGE via
|
||||
* [expungeTargeted]. This is the batch, data-safe path the repository's multi-message delete/expunge
|
||||
* routes through — one login + one expunge for the whole selection, instead of the N logins + N
|
||||
* expunges a per-UID [deleteMessage] loop would pay, and it never touches unrelated `\Deleted` mail
|
||||
* (issue #295). A no-op when [uids] is empty or none of them resolve to a message in [folder].
|
||||
*/
|
||||
suspend fun deleteMessages(params: ImapConnectionParams, folder: String, uids: List<String>) =
|
||||
withContext(Dispatchers.IO) {
|
||||
if (uids.isEmpty()) return@withContext
|
||||
withStore(params) { store ->
|
||||
val mailbox = store.getFolder(folder)
|
||||
mailbox.open(Folder.READ_WRITE)
|
||||
try {
|
||||
val uidFolder = mailbox as UIDFolder
|
||||
val messages = uids.mapNotNull { uidFolder.getMessageByUID(it.toLong()) }.toTypedArray()
|
||||
if (messages.isEmpty()) return@withStore
|
||||
mailbox.setFlags(messages, Flags(Flags.Flag.DELETED), true)
|
||||
expungeTargeted(mailbox, messages)
|
||||
} finally {
|
||||
runCatching { mailbox.close(false) }
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Moves messages from [sourceFolder] to [destFolder] by UID. Implemented as copy + \Deleted +
|
||||
* expunge so it works on every IMAP server (no reliance on the RFC 6851 MOVE extension).
|
||||
* targeted expunge so it works on every IMAP server (no reliance on the RFC 6851 MOVE extension).
|
||||
*/
|
||||
suspend fun moveMessages(
|
||||
params: ImapConnectionParams,
|
||||
@@ -381,13 +402,27 @@ class ImapClient(private val reuseConnections: Boolean) {
|
||||
if (messages.isEmpty()) return@withStore
|
||||
mailbox.copyMessages(messages, store.getFolder(destFolder))
|
||||
mailbox.setFlags(messages, Flags(Flags.Flag.DELETED), true)
|
||||
mailbox.expunge()
|
||||
expungeTargeted(mailbox, messages)
|
||||
} finally {
|
||||
runCatching { mailbox.close(false) }
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Permanently removes exactly [messages] — which the caller has already flagged `\Deleted` — with a
|
||||
* **targeted** UID EXPUNGE (RFC 4315, [IMAPFolder.expunge]). Deliberately never the untargeted
|
||||
* [Folder.expunge], which expunges *every* `\Deleted`-flagged message in [mailbox] — including ones
|
||||
* a second client, Gmail, or a partial earlier move left flagged — the data-loss bug in issue #295.
|
||||
* Every folder this client opens is an [IMAPFolder], so the cast holds in production and under
|
||||
* GreenMail. The server must advertise UIDPLUS (Gmail, Outlook, and GreenMail do); on one that does
|
||||
* not, Angus raises "UID EXPUNGE not supported" here rather than silently falling back to the
|
||||
* unrelated-mail-destroying untargeted expunge — a loud failure is the correct, safe outcome.
|
||||
*/
|
||||
private fun expungeTargeted(mailbox: Folder, messages: Array<Message>) {
|
||||
(mailbox as IMAPFolder).expunge(messages)
|
||||
}
|
||||
|
||||
/**
|
||||
* Fetches the fields needed to compose a reply/forward (recipients + body) without marking the
|
||||
* original \Seen (opens READ_ONLY), so building a reply doesn't change the message's read state.
|
||||
|
||||
@@ -576,7 +576,7 @@ class MailRepositoryImplCoverageTest {
|
||||
|
||||
assertTrue(result.isSuccess)
|
||||
coVerify { messageDao.deleteByIds(listOf(id)) } // local removal still happens
|
||||
coVerify(exactly = 0) { imapClient.deleteMessage(any(), any(), any()) } // nothing pushed server-side
|
||||
coVerify(exactly = 0) { imapClient.deleteMessages(any(), any(), any()) } // nothing pushed server-side
|
||||
}
|
||||
|
||||
// --- role-folder resolution: a same-role folder that is not selectable ----------------------
|
||||
|
||||
@@ -367,7 +367,8 @@ class MailRepositoryImplTest {
|
||||
val result = repository.trash(listOf(id))
|
||||
|
||||
assertTrue(result.isSuccess)
|
||||
coVerify { imapClient.deleteMessage(any(), "INBOX", "7") }
|
||||
// The trash fallback deletes the whole group in one batch call, not a per-UID loop (issue #295).
|
||||
coVerify { imapClient.deleteMessages(any(), "INBOX", listOf("7")) }
|
||||
}
|
||||
|
||||
@Test
|
||||
@@ -394,7 +395,8 @@ class MailRepositoryImplTest {
|
||||
|
||||
repository.expunge(listOf(id))
|
||||
|
||||
coVerify { imapClient.deleteMessage(any(), "Trash", "3") }
|
||||
// Expunge routes the group through the batch, targeted UID EXPUNGE path (issue #295).
|
||||
coVerify { imapClient.deleteMessages(any(), "Trash", listOf("3")) }
|
||||
}
|
||||
|
||||
@Test
|
||||
|
||||
@@ -267,6 +267,62 @@ class ImapClientTest {
|
||||
assertTrue(remaining.contains("Keep me"), "remaining=$remaining")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `deleteMessages expunges only the given uids and spares other Deleted-flagged mail`() = runTest {
|
||||
GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Target", "delete this")
|
||||
GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Bystander", "flagged elsewhere")
|
||||
GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Untouched", "no flag at all")
|
||||
greenMail.waitForIncomingEmail(3)
|
||||
val bySubject = client.fetchRecent(params(), "INBOX", limit = 50).associateBy { it.subject }
|
||||
|
||||
// Simulate a second client (or a partial earlier move): "Bystander" is flagged \Deleted but has
|
||||
// NOT been expunged — setFlag closes the folder with expunge=false, so the flag persists.
|
||||
client.setFlag(params(), "INBOX", bySubject.getValue("Bystander").uid, Flags.Flag.DELETED, value = true)
|
||||
|
||||
// Delete only "Target". The old untargeted mailbox.expunge() would ALSO permanently drop the
|
||||
// \Deleted "Bystander"; a targeted UID EXPUNGE removes just the target (issue #295).
|
||||
client.deleteMessages(params(), "INBOX", listOf(bySubject.getValue("Target").uid))
|
||||
|
||||
val remaining = client.fetchRecent(params(), "INBOX", limit = 50).map { it.subject }
|
||||
assertFalse(remaining.contains("Target"), "the targeted message must be expunged, remaining=$remaining")
|
||||
assertTrue(remaining.contains("Bystander"), "an unrelated \\Deleted message must survive, remaining=$remaining")
|
||||
assertTrue(remaining.contains("Untouched"), "an unflagged message must survive, remaining=$remaining")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `deleteMessages removes every uid in the batch`() = runTest {
|
||||
listOf("A", "B", "C", "D").forEach {
|
||||
GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", it, "Body $it")
|
||||
}
|
||||
greenMail.waitForIncomingEmail(4)
|
||||
val bySubject = client.fetchRecent(params(), "INBOX", limit = 50).associateBy { it.subject }
|
||||
|
||||
client.deleteMessages(params(), "INBOX", listOf(bySubject.getValue("A").uid, bySubject.getValue("C").uid))
|
||||
|
||||
val remaining = client.fetchRecent(params(), "INBOX", limit = 50).map { it.subject }.toSet()
|
||||
assertEquals(setOf("B", "D"), remaining, "remaining=$remaining")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `moveMessages expunges only the moved uids and spares other Deleted-flagged mail`() = runTest {
|
||||
GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Move me", "relocate this")
|
||||
GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Bystander", "flagged elsewhere")
|
||||
greenMail.waitForIncomingEmail(2)
|
||||
appendMessage("Archive", "carol@example.org", "Seed", "Creates the Archive folder")
|
||||
val bySubject = client.fetchRecent(params(), "INBOX", limit = 50).associateBy { it.subject }
|
||||
// "Bystander" is flagged \Deleted (by another client) but left in the source folder, un-expunged.
|
||||
client.setFlag(params(), "INBOX", bySubject.getValue("Bystander").uid, Flags.Flag.DELETED, value = true)
|
||||
|
||||
client.moveMessages(params(), "INBOX", listOf(bySubject.getValue("Move me").uid), "Archive")
|
||||
|
||||
val inbox = client.fetchRecent(params(), "INBOX", limit = 50).map { it.subject }
|
||||
assertFalse(inbox.contains("Move me"), "the moved message must leave the source, inbox=$inbox")
|
||||
// The move's expunge must be targeted: an unrelated \Deleted message must NOT be dropped (#295).
|
||||
assertTrue(inbox.contains("Bystander"), "an unrelated \\Deleted message must survive the move, inbox=$inbox")
|
||||
val archive = client.fetchRecent(params(), "Archive", limit = 50).map { it.subject }
|
||||
assertTrue(archive.contains("Move me"), "archive=$archive")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `fetchRecent returns empty for an empty folder`() = runTest {
|
||||
createFolder("Empty")
|
||||
|
||||
@@ -112,6 +112,24 @@ class ImapFolderOpenLatencyTest {
|
||||
assertEquals(1, proxy.commandCount("LOGOUT"), "one LOGOUT — the connection is torn down, not kept alive")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a batch delete opens one connection and pays one LOGIN for the whole selection`() = runTest {
|
||||
seedInbox(3)
|
||||
val uids = client.fetchRecent(params(), "INBOX", limit = 50).map { it.uid } // the fetch: connection 1
|
||||
proxy.awaitClientStreamsSettled()
|
||||
val connectionsBefore = proxy.connectionCount
|
||||
val loginsBefore = proxy.authCommandCount()
|
||||
|
||||
client.deleteMessages(params(), "INBOX", uids)
|
||||
proxy.awaitClientStreamsSettled()
|
||||
|
||||
// Deleting all THREE messages is a single CONNECT + LOGIN + SELECT + STORE + UID EXPUNGE + LOGOUT
|
||||
// — one connection and one LOGIN for the whole batch, not the three connects / three LOGINs a
|
||||
// per-UID deleteMessage() loop would pay (the #295 N-login inefficiency).
|
||||
assertEquals(1, proxy.connectionCount - connectionsBefore, "batch delete must open exactly one connection")
|
||||
assertEquals(1, proxy.authCommandCount() - loginsBefore, "batch delete must pay exactly one LOGIN")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `opening a folder then reading a message uses two separate connections (compounding cost)`() = runTest {
|
||||
seedInbox(1)
|
||||
|
||||
Reference in New Issue
Block a user