Merge pull request #7813 from thunderbird/imap_delete

IMAP: Make expunge logic part of low-level delete operation

app/core/src/main/java/com/fsck/k9/controller/DraftOperations.kt

@@ -1,7 +1,6 @@
 package com.fsck.k9.controller
 
 import com.fsck.k9.Account
-import com.fsck.k9.Account.Expunge
 import com.fsck.k9.K9
 import com.fsck.k9.backend.api.Backend
 import com.fsck.k9.controller.MessagingControllerCommands.PendingAppend
@@ -115,7 +114,7 @@ internal class DraftOperations(
             uploadMessage(backend, account, localFolder, localMessage)
         }
 
-        deleteMessage(backend, account, localFolder, command.deleteMessageId)
+        deleteMessage(backend, localFolder, command.deleteMessageId)
     }
 
     private fun uploadMessage(
@@ -152,7 +151,7 @@ internal class DraftOperations(
         }
     }
 
-    private fun deleteMessage(backend: Backend, account: Account, localFolder: LocalFolder, messageId: Long) {
+    private fun deleteMessage(backend: Backend, localFolder: LocalFolder, messageId: Long) {
         val messageServerId = localFolder.getMessageUidById(messageId) ?: run {
             Timber.i("Couldn't find local copy of message [ID: %d] to be deleted. Skipping delete.", messageId)
             return
@@ -162,10 +161,6 @@ internal class DraftOperations(
         val folderServerId = localFolder.serverId
         backend.deleteMessages(folderServerId, messageServerIds)
 
-        if (backend.supportsExpunge && account.expungePolicy == Expunge.EXPUNGE_IMMEDIATELY) {
-            backend.expungeMessages(folderServerId, messageServerIds)
-        }
-
         messagingController.destroyPlaceholderMessages(localFolder, messageServerIds)
     }
 

app/core/src/main/java/com/fsck/k9/controller/MessagingController.java

@@ -29,7 +29,6 @@ import androidx.annotation.NonNull;
 import androidx.annotation.VisibleForTesting;
 import com.fsck.k9.Account;
 import com.fsck.k9.Account.DeletePolicy;
-import com.fsck.k9.Account.Expunge;
 import com.fsck.k9.DI;
 import com.fsck.k9.K9;
 import com.fsck.k9.Preferences;
@@ -992,10 +991,6 @@ public class MessagingController {
         String folderServerId = getFolderServerId(account, folderId);
         backend.deleteMessages(folderServerId, uids);
 
-        if (backend.getSupportsExpunge() && account.getExpungePolicy() == Expunge.EXPUNGE_IMMEDIATELY) {
-            backend.expungeMessages(folderServerId, uids);
-        }
-
         LocalStore localStore = localStoreProvider.getInstance(account);
         LocalFolder localFolder = localStore.getFolder(folderId);
         localFolder.open();
@@ -2094,10 +2089,6 @@ public class MessagingController {
         Backend backend = getBackend(account);
         backend.deleteAllMessages(trashFolderServerId);
 
-        if (account.getExpungePolicy() == Expunge.EXPUNGE_IMMEDIATELY && backend.getSupportsExpunge()) {
-            backend.expunge(trashFolderServerId);
-        }
-
         // Remove all messages marked as deleted
         folder.destroyDeletedMessages();
 

backend/api/src/main/java/com/fsck/k9/backend/api/Backend.kt

@@ -41,9 +41,6 @@ interface Backend {
     @Throws(MessagingException::class)
     fun expunge(folderServerId: String)
 
-    @Throws(MessagingException::class)
-    fun expungeMessages(folderServerId: String, messageServerIds: List<String>)
-
     @Throws(MessagingException::class)
     fun deleteMessages(folderServerId: String, messageServerIds: List<String>)
 

backend/demo/src/main/java/app/k9mail/backend/demo/DemoBackend.kt

@@ -104,10 +104,6 @@ class DemoBackend(private val backendStorage: BackendStorage) : Backend {
         throw UnsupportedOperationException("not implemented")
     }
 
-    override fun expungeMessages(folderServerId: String, messageServerIds: List<String>) {
-        throw UnsupportedOperationException("not implemented")
-    }
-
     override fun deleteMessages(folderServerId: String, messageServerIds: List<String>) = Unit
 
     override fun deleteAllMessages(folderServerId: String) = Unit

backend/imap/src/main/java/com/fsck/k9/backend/imap/CommandDelete.kt

@@ -0,0 +1,22 @@
+package com.fsck.k9.backend.imap
+
+import com.fsck.k9.mail.MessagingException
+import com.fsck.k9.mail.store.imap.ImapStore
+import com.fsck.k9.mail.store.imap.OpenMode
+
+internal class CommandDelete(private val imapStore: ImapStore) {
+
+    @Throws(MessagingException::class)
+    fun deleteMessages(folderServerId: String, messageServerIds: List<String>) {
+        val remoteFolder = imapStore.getFolder(folderServerId)
+        try {
+            remoteFolder.open(OpenMode.READ_WRITE)
+
+            val messages = messageServerIds.map { uid -> remoteFolder.getMessage(uid) }
+
+            remoteFolder.deleteMessages(messages)
+        } finally {
+            remoteFolder.close()
+        }
+    }
+}

backend/imap/src/main/java/com/fsck/k9/backend/imap/CommandDeleteAll.kt

@@ -1,6 +1,5 @@
 package com.fsck.k9.backend.imap
 
-import com.fsck.k9.mail.Flag
 import com.fsck.k9.mail.MessagingException
 import com.fsck.k9.mail.store.imap.ImapStore
 import com.fsck.k9.mail.store.imap.OpenMode
@@ -13,7 +12,7 @@ internal class CommandDeleteAll(private val imapStore: ImapStore) {
         try {
             remoteFolder.open(OpenMode.READ_WRITE)
 
-            remoteFolder.setFlagsForAllMessages(setOf(Flag.DELETED), true)
+            remoteFolder.deleteAllMessages()
         } finally {
             remoteFolder.close()
         }

backend/imap/src/main/java/com/fsck/k9/backend/imap/ImapBackend.kt

@@ -30,6 +30,7 @@ class ImapBackend(
     private val commandMarkAllAsRead = CommandMarkAllAsRead(imapStore)
     private val commandExpunge = CommandExpunge(imapStore)
     private val commandMoveOrCopyMessages = CommandMoveOrCopyMessages(imapStore)
+    private val commandDelete = CommandDelete(imapStore)
     private val commandDeleteAll = CommandDeleteAll(imapStore)
     private val commandSearch = CommandSearch(imapStore)
     private val commandDownloadMessage = CommandDownloadMessage(backendStorage, imapStore)
@@ -79,12 +80,8 @@ class ImapBackend(
         commandExpunge.expunge(folderServerId)
     }
 
-    override fun expungeMessages(folderServerId: String, messageServerIds: List<String>) {
-        commandExpunge.expungeMessages(folderServerId, messageServerIds)
-    }
-
     override fun deleteMessages(folderServerId: String, messageServerIds: List<String>) {
-        commandSetFlag.setFlag(folderServerId, messageServerIds, Flag.DELETED, true)
+        commandDelete.deleteMessages(folderServerId, messageServerIds)
     }
 
     override fun deleteAllMessages(folderServerId: String) {

backend/imap/src/test/java/com/fsck/k9/backend/imap/TestImapFolder.kt

@@ -171,6 +171,14 @@ open class TestImapFolder(override val serverId: String) : ImapFolder {
         throw UnsupportedOperationException("not implemented")
     }
 
+    override fun deleteMessages(messages: List<ImapMessage>) {
+        setFlags(messages, setOf(Flag.DELETED), true)
+    }
+
+    override fun deleteAllMessages() {
+        setFlagsForAllMessages(setOf(Flag.DELETED), true)
+    }
+
     override fun expunge() {
         mode = OpenMode.READ_WRITE
         wasExpunged = true

backend/jmap/src/main/java/com/fsck/k9/backend/jmap/JmapBackend.kt

@@ -72,10 +72,6 @@ class JmapBackend(
         throw UnsupportedOperationException("not implemented")
     }
 
-    override fun expungeMessages(folderServerId: String, messageServerIds: List<String>) {
-        throw UnsupportedOperationException("not implemented")
-    }
-
     override fun deleteMessages(folderServerId: String, messageServerIds: List<String>) {
         commandDelete.deleteMessages(messageServerIds)
     }

backend/pop3/src/main/java/com/fsck/k9/backend/pop3/Pop3Backend.kt

@@ -66,10 +66,6 @@ class Pop3Backend(
         throw UnsupportedOperationException("not supported")
     }
 
-    override fun expungeMessages(folderServerId: String, messageServerIds: List<String>) {
-        throw UnsupportedOperationException("not supported")
-    }
-
     override fun deleteMessages(folderServerId: String, messageServerIds: List<String>) {
         commandSetFlag.setFlag(folderServerId, messageServerIds, Flag.DELETED, true)
     }

mail/protocols/imap/src/main/java/com/fsck/k9/mail/store/imap/ImapFolder.kt

@@ -81,6 +81,12 @@ interface ImapFolder {
     @Throws(MessagingException::class)
     fun moveMessages(messages: List<ImapMessage>, folder: ImapFolder): Map<String, String>?
 
+    @Throws(MessagingException::class)
+    fun deleteMessages(messages: List<ImapMessage>)
+
+    @Throws(MessagingException::class)
+    fun deleteAllMessages()
+
     @Throws(MessagingException::class)
     fun expunge()
 

mail/protocols/imap/src/main/java/com/fsck/k9/mail/store/imap/RealImapFolder.kt

@@ -1087,6 +1087,29 @@ internal class RealImapFolder(
         return searchResponse.numbers.firstOrNull()?.toString()
     }
 
+    @Throws(MessagingException::class)
+    override fun deleteMessages(messages: List<ImapMessage>) {
+        checkOpenWithWriteAccess()
+
+        setFlags(messages, setOf(Flag.DELETED), true)
+
+        if (internalImapStore.config.isExpungeImmediately()) {
+            val uids = messages.map { it.uid }
+            expungeUids(uids)
+        }
+    }
+
+    @Throws(MessagingException::class)
+    override fun deleteAllMessages() {
+        checkOpenWithWriteAccess()
+
+        setFlagsForAllMessages(setOf(Flag.DELETED), true)
+
+        if (internalImapStore.config.isExpungeImmediately()) {
+            expunge()
+        }
+    }
+
     @Throws(MessagingException::class)
     override fun expunge() {
         checkOpenWithWriteAccess()

mail/protocols/imap/src/test/java/com/fsck/k9/mail/store/imap/RealImapFolderTest.kt

@@ -1150,6 +1150,117 @@ class RealImapFolderTest {
         assertThat(uid).isEqualTo("23")
     }
 
+    @Test
+    fun `deleteMessages() on closed folder should throw`() {
+        val folder = createFolder("Folder")
+        val messages = listOf(createImapMessage("1"))
+
+        assertFailure {
+            folder.deleteMessages(messages)
+        }.isInstanceOf<IllegalStateException>()
+            .hasMessage("Folder 'Folder' is not open.")
+
+        verifyNoMoreInteractions(imapConnection)
+    }
+
+    @Test
+    fun `deleteMessages() on folder opened as read-only should throw`() {
+        val folder = createFolder("Folder")
+        prepareImapFolderForOpen(OpenMode.READ_ONLY)
+        folder.open(OpenMode.READ_ONLY)
+        val messages = listOf(createImapMessage("1"))
+
+        assertFailure {
+            folder.deleteMessages(messages)
+        }.isInstanceOf<IllegalStateException>()
+            .hasMessage("Folder 'Folder' needs to be opened for read-write access.")
+    }
+
+    @Test
+    fun `deleteMessages() with expungeImmediately = true should set deleted flag and expunge message`() {
+        imapStoreConfig.expungeImmediately = true
+        val folder = createFolder("Folder")
+        prepareImapFolderForOpen(OpenMode.READ_WRITE)
+        imapConnection.stub {
+            on { isUidPlusCapable } doReturn true
+        }
+        folder.open(OpenMode.READ_WRITE)
+        val messages = listOf(createImapMessage("1"))
+
+        folder.deleteMessages(messages)
+
+        assertCommandWithIdsIssued("UID STORE 1 +FLAGS.SILENT (\\Deleted)")
+        assertCommandWithIdsIssued("UID EXPUNGE 1")
+    }
+
+    @Test
+    fun `deleteMessages() with expungeImmediately = false should set deleted flag but not expunge message`() {
+        imapStoreConfig.expungeImmediately = false
+        val folder = createFolder("Folder")
+        prepareImapFolderForOpen(OpenMode.READ_WRITE)
+        imapConnection.stub {
+            on { isUidPlusCapable } doReturn true
+        }
+        folder.open(OpenMode.READ_WRITE)
+        val messages = listOf(createImapMessage("1"))
+
+        folder.deleteMessages(messages)
+
+        assertCommandWithIdsIssued("UID STORE 1 +FLAGS.SILENT (\\Deleted)")
+        verify(imapConnection, never()).executeCommandWithIdSet("UID EXPUNGE", "", setOf(1L))
+        verify(imapConnection, never()).executeSimpleCommand("EXPUNGE")
+    }
+
+    @Test
+    fun `deleteAllMessages() on closed folder should throw`() {
+        val folder = createFolder("Folder")
+
+        assertFailure {
+            folder.deleteAllMessages()
+        }.isInstanceOf<IllegalStateException>()
+            .hasMessage("Folder 'Folder' is not open.")
+
+        verifyNoMoreInteractions(imapConnection)
+    }
+
+    @Test
+    fun `deleteAllMessages() on folder opened as read-only should throw`() {
+        val folder = createFolder("Folder")
+        prepareImapFolderForOpen(OpenMode.READ_ONLY)
+        folder.open(OpenMode.READ_ONLY)
+
+        assertFailure {
+            folder.deleteAllMessages()
+        }.isInstanceOf<IllegalStateException>()
+            .hasMessage("Folder 'Folder' needs to be opened for read-write access.")
+    }
+
+    @Test
+    fun `deleteAllMessages() with expungeImmediately = true should set deleted flag and expunge`() {
+        imapStoreConfig.expungeImmediately = true
+        val folder = createFolder("Folder")
+        prepareImapFolderForOpen(OpenMode.READ_WRITE)
+        folder.open(OpenMode.READ_WRITE)
+
+        folder.deleteAllMessages()
+
+        assertCommandIssued("UID STORE 1:* +FLAGS.SILENT (\\Deleted)")
+        assertCommandIssued("EXPUNGE")
+    }
+
+    @Test
+    fun `deleteAllMessages() with expungeImmediately = false should set deleted flag but not expunge`() {
+        imapStoreConfig.expungeImmediately = false
+        val folder = createFolder("Folder")
+        prepareImapFolderForOpen(OpenMode.READ_WRITE)
+        folder.open(OpenMode.READ_WRITE)
+
+        folder.deleteAllMessages()
+
+        assertCommandIssued("UID STORE 1:* +FLAGS.SILENT (\\Deleted)")
+        verify(imapConnection, never()).executeSimpleCommand("EXPUNGE")
+    }
+
     @Test
     fun `expunge() on closed folder should throw`() {
         val folder = createFolder("Folder")

mail/protocols/imap/src/test/java/com/fsck/k9/mail/store/imap/TestImapFolder.kt

@@ -112,6 +112,14 @@ internal open class TestImapFolder(
         throw UnsupportedOperationException("not implemented")
     }
 
+    override fun deleteMessages(messages: List<ImapMessage>) {
+        throw UnsupportedOperationException("not implemented")
+    }
+
+    override fun deleteAllMessages() {
+        throw UnsupportedOperationException("not implemented")
+    }
+
     override fun expunge() {
         throw UnsupportedOperationException("not implemented")
     }