Merge pull request #7793 from thunderbird/remove_unnecessary_ImapFold…

…er_open_calls IMAP: Remove unnecessary calls to `RealImapFolder.open()`
thunderbird · Apr 30, 2024 · 10862a0 · 10862a0
2 parents 11e2e34 + a8af489
commit 10862a0
Show file tree

Hide file tree

Showing 9 changed files with 114 additions and 19 deletions.
diff --git a/backend/imap/src/main/java/com/fsck/k9/backend/imap/CommandDeleteAll.kt b/backend/imap/src/main/java/com/fsck/k9/backend/imap/CommandDeleteAll.kt
@@ -13,7 +13,7 @@ internal class CommandDeleteAll(private val imapStore: ImapStore) {
         try {
             remoteFolder.open(OpenMode.READ_WRITE)
 
-            remoteFolder.setFlags(setOf(Flag.DELETED), true)
+            remoteFolder.setFlagsForAllMessages(setOf(Flag.DELETED), true)
         } finally {
             remoteFolder.close()
         }

diff --git a/backend/imap/src/main/java/com/fsck/k9/backend/imap/CommandMarkAllAsRead.kt b/backend/imap/src/main/java/com/fsck/k9/backend/imap/CommandMarkAllAsRead.kt
@@ -12,7 +12,7 @@ internal class CommandMarkAllAsRead(private val imapStore: ImapStore) {
             remoteFolder.open(OpenMode.READ_WRITE)
             if (remoteFolder.mode != OpenMode.READ_WRITE) return
 
-            remoteFolder.setFlags(setOf(Flag.SEEN), true)
+            remoteFolder.setFlagsForAllMessages(setOf(Flag.SEEN), true)
         } finally {
             remoteFolder.close()
         }

diff --git a/backend/imap/src/main/java/com/fsck/k9/backend/imap/CommandSearch.kt b/backend/imap/src/main/java/com/fsck/k9/backend/imap/CommandSearch.kt
@@ -2,6 +2,7 @@ package com.fsck.k9.backend.imap
 
 import com.fsck.k9.mail.Flag
 import com.fsck.k9.mail.store.imap.ImapStore
+import com.fsck.k9.mail.store.imap.OpenMode
 
 internal class CommandSearch(private val imapStore: ImapStore) {
 
@@ -14,6 +15,8 @@ internal class CommandSearch(private val imapStore: ImapStore) {
     ): List<String> {
         val folder = imapStore.getFolder(folderServerId)
         try {
+            folder.open(OpenMode.READ_ONLY)
+
             return folder.search(query, requiredFlags, forbiddenFlags, performFullTextSearch)
                 .sortedWith(UidReverseComparator())
                 .map { it.uid }

diff --git a/backend/imap/src/main/java/com/fsck/k9/backend/imap/ImapSync.kt b/backend/imap/src/main/java/com/fsck/k9/backend/imap/ImapSync.kt
@@ -75,6 +75,9 @@ internal class ImapSync(
 
             if (syncConfig.expungePolicy === ExpungePolicy.ON_POLL) {
                 Timber.d("SYNC: Expunging folder %s:%s", accountName, folder)
+                if (!remoteFolder.isOpen || remoteFolder.mode != OpenMode.READ_WRITE) {
+                    remoteFolder.open(OpenMode.READ_WRITE)
+                }
                 remoteFolder.expunge()
             }
 

diff --git a/backend/imap/src/test/java/com/fsck/k9/backend/imap/TestImapFolder.kt b/backend/imap/src/test/java/com/fsck/k9/backend/imap/TestImapFolder.kt
@@ -17,6 +17,9 @@ open class TestImapFolder(override val serverId: String) : ImapFolder {
     override var mode: OpenMode? = null
         protected set
 
+    override val isOpen: Boolean
+        get() = mode != null
+
     override var messageCount: Int = 0
 
     var wasExpunged: Boolean = false
@@ -136,7 +139,7 @@ open class TestImapFolder(override val serverId: String) : ImapFolder {
         throw UnsupportedOperationException("not implemented")
     }
 
-    override fun setFlags(flags: Set<Flag>, value: Boolean) {
+    override fun setFlagsForAllMessages(flags: Set<Flag>, value: Boolean) {
         if (value) {
             for (messageFlagSet in messageFlags.values) {
                 messageFlagSet.addAll(flags)

diff --git a/mail/protocols/imap/src/main/java/com/fsck/k9/mail/store/imap/ImapFolder.kt b/mail/protocols/imap/src/main/java/com/fsck/k9/mail/store/imap/ImapFolder.kt
@@ -14,6 +14,7 @@ interface ImapFolder {
     val serverId: String
     val mode: OpenMode?
     val messageCount: Int
+    val isOpen: Boolean
 
     @Throws(MessagingException::class)
     fun exists(): Boolean
@@ -69,7 +70,7 @@ interface ImapFolder {
     fun appendMessages(messages: List<Message>): Map<String, String>?
 
     @Throws(MessagingException::class)
-    fun setFlags(flags: Set<Flag>, value: Boolean)
+    fun setFlagsForAllMessages(flags: Set<Flag>, value: Boolean)
 
     @Throws(MessagingException::class)
     fun setFlags(messages: List<ImapMessage>, flags: Set<Flag>, value: Boolean)

diff --git a/mail/protocols/imap/src/main/java/com/fsck/k9/mail/store/imap/RealImapFolder.kt b/mail/protocols/imap/src/main/java/com/fsck/k9/mail/store/imap/RealImapFolder.kt
@@ -46,7 +46,7 @@ internal class RealImapFolder(
     override var mode: OpenMode? = null
         private set
 
-    val isOpen: Boolean
+    override val isOpen: Boolean
         get() = connection != null
 
     override fun getUidValidity(): Long? {
@@ -964,7 +964,6 @@ internal class RealImapFolder(
      */
     @Throws(MessagingException::class)
     override fun appendMessages(messages: List<Message>): Map<String, String>? {
-        open(OpenMode.READ_WRITE)
         checkOpen()
 
         return try {
@@ -1074,7 +1073,6 @@ internal class RealImapFolder(
 
     @Throws(MessagingException::class)
     override fun expunge() {
-        open(OpenMode.READ_WRITE)
         checkOpen()
 
         try {
@@ -1085,6 +1083,7 @@ internal class RealImapFolder(
     }
 
     override fun expungeUids(uids: List<String>) {
+        checkOpen()
         expungeUids(uids, fullExpungeFallback = true)
     }
 
@@ -1095,9 +1094,6 @@ internal class RealImapFolder(
     private fun expungeUids(uids: List<String>, fullExpungeFallback: Boolean) {
         require(uids.isNotEmpty()) { "expungeUids() must be called with a non-empty set of UIDs" }
 
-        open(OpenMode.READ_WRITE)
-        checkOpen()
-
         try {
             if (connection!!.isUidPlusCapable) {
                 val longUids = uids.map { it.toLong() }.toSet()
@@ -1113,8 +1109,7 @@ internal class RealImapFolder(
     }
 
     @Throws(MessagingException::class)
-    override fun setFlags(flags: Set<Flag>, value: Boolean) {
-        open(OpenMode.READ_WRITE)
+    override fun setFlagsForAllMessages(flags: Set<Flag>, value: Boolean) {
         checkOpen()
 
         val canCreateForwardedFlag = canCreateKeywords ||
@@ -1137,7 +1132,6 @@ internal class RealImapFolder(
 
     @Throws(MessagingException::class)
     override fun setFlags(messages: List<ImapMessage>, flags: Set<Flag>, value: Boolean) {
-        open(OpenMode.READ_WRITE)
         checkOpen()
 
         val uids = messages.map { it.uid.toLong() }.toSet()
@@ -1155,7 +1149,7 @@ internal class RealImapFolder(
     @Throws(MessagingException::class)
     private fun checkOpen() {
         if (!isOpen) {
-            throw MessagingException("Folder $prefixedName is not open.")
+            throw MessagingException("Folder $serverId is not open.")
         }
     }
 
@@ -1197,7 +1191,6 @@ internal class RealImapFolder(
         performFullTextSearch: Boolean,
     ): List<ImapMessage> {
         try {
-            open(OpenMode.READ_ONLY)
             checkOpen()
 
             inSearch = true

diff --git a/mail/protocols/imap/src/test/java/com/fsck/k9/mail/store/imap/RealImapFolderTest.kt b/mail/protocols/imap/src/test/java/com/fsck/k9/mail/store/imap/RealImapFolderTest.kt
@@ -994,6 +994,19 @@ class RealImapFolderTest {
         assertThat(buffer.readUtf8()).isEqualTo("text")
     }
 
+    @Test
+    fun `appendMessages() on closed folder should throw`() {
+        val folder = createFolder("Folder")
+        val messages = listOf(createImapMessage("1"))
+
+        assertFailure {
+            folder.appendMessages(messages)
+        }.isInstanceOf<MessagingException>()
+            .hasMessage("Folder Folder is not open.")
+
+        verifyNoMoreInteractions(imapConnection)
+    }
+
     @Test
     fun appendMessages_shouldIssueRespectiveCommand() {
         val folder = createFolder("Folder")
@@ -1063,20 +1076,46 @@ class RealImapFolderTest {
         assertThat(uid).isEqualTo("23")
     }
 
+    @Test
+    fun `expunge() on closed folder should throw`() {
+        val folder = createFolder("Folder")
+
+        assertFailure {
+            folder.expunge()
+        }.isInstanceOf<MessagingException>()
+            .hasMessage("Folder Folder is not open.")
+
+        verifyNoMoreInteractions(imapConnection)
+    }
+
     @Test
     fun expunge_shouldIssueExpungeCommand() {
         val folder = createFolder("Folder")
         prepareImapFolderForOpen(OpenMode.READ_WRITE)
+        folder.open(OpenMode.READ_WRITE)
 
         folder.expunge()
 
         verify(imapConnection).executeSimpleCommand("EXPUNGE")
     }
 
+    @Test
+    fun `expungeUids() on closed folder should throw`() {
+        val folder = createFolder("Folder")
+
+        assertFailure {
+            folder.expungeUids(listOf("1"))
+        }.isInstanceOf<MessagingException>()
+            .hasMessage("Folder Folder is not open.")
+
+        verifyNoMoreInteractions(imapConnection)
+    }
+
     @Test
     fun expungeUids_withUidPlus_shouldIssueUidExpungeCommand() {
         val folder = createFolder("Folder")
         prepareImapFolderForOpen(OpenMode.READ_WRITE)
+        folder.open(OpenMode.READ_WRITE)
         whenever(imapConnection.isUidPlusCapable).thenReturn(true)
 
         folder.expungeUids(listOf("1"))
@@ -1088,6 +1127,7 @@ class RealImapFolderTest {
     fun expungeUids_withoutUidPlus_shouldIssueExpungeCommand() {
         val folder = createFolder("Folder")
         prepareImapFolderForOpen(OpenMode.READ_WRITE)
+        folder.open(OpenMode.READ_WRITE)
         whenever(imapConnection.isUidPlusCapable).thenReturn(false)
 
         folder.expungeUids(listOf("1"))
@@ -1096,19 +1136,70 @@ class RealImapFolderTest {
     }
 
     @Test
-    fun setFlags_shouldIssueUidStoreCommand() {
+    fun `setFlagsForAllMessages() on closed folder should throw`() {
+        val folder = createFolder("Folder")
+
+        assertFailure {
+            folder.setFlagsForAllMessages(setOf(Flag.SEEN), true)
+        }.isInstanceOf<MessagingException>()
+            .hasMessage("Folder Folder is not open.")
+
+        verifyNoMoreInteractions(imapConnection)
+    }
+
+    @Test
+    fun setFlagsForAllMessages_shouldIssueUidStoreCommand() {
         val folder = createFolder("Folder")
         prepareImapFolderForOpen(OpenMode.READ_WRITE)
+        folder.open(OpenMode.READ_WRITE)
 
-        folder.setFlags(setOf(Flag.SEEN), true)
+        folder.setFlagsForAllMessages(setOf(Flag.SEEN), true)
 
         assertCommandIssued("UID STORE 1:* +FLAGS.SILENT (\\Seen)")
     }
 
+    @Test
+    fun `setFlags() on closed folder should throw`() {
+        val folder = createFolder("Folder")
+        val messages = listOf(createImapMessage("1"))
+
+        assertFailure {
+            folder.setFlags(messages, setOf(Flag.SEEN), true)
+        }.isInstanceOf<MessagingException>()
+            .hasMessage("Folder Folder is not open.")
+
+        verifyNoMoreInteractions(imapConnection)
+    }
+
+    @Test
+    fun `setFlags() should issue UID STORE command`() {
+        val folder = createFolder("Folder")
+        prepareImapFolderForOpen(OpenMode.READ_WRITE)
+        folder.open(OpenMode.READ_WRITE)
+        val messages = listOf(createImapMessage("1"))
+
+        folder.setFlags(messages, setOf(Flag.SEEN), true)
+
+        assertCommandWithIdsIssued("UID STORE 1 +FLAGS.SILENT (\\Seen)")
+    }
+
+    @Test
+    fun `search() on closed folder should throw`() {
+        val folder = createFolder("Folder")
+
+        assertFailure {
+            folder.search("query", setOf(Flag.SEEN), emptySet(), true)
+        }.isInstanceOf<MessagingException>()
+            .hasMessage("Folder Folder is not open.")
+
+        verifyNoMoreInteractions(imapConnection)
+    }
+
     @Test
     fun search_withFullTextSearchEnabled_shouldIssueRespectiveCommand() {
         val folder = createFolder("Folder")
         prepareImapFolderForOpen(OpenMode.READ_ONLY)
+        folder.open(OpenMode.READ_ONLY)
         setupUidSearchResponses("1 OK SEARCH completed")
 
         folder.search("query", setOf(Flag.SEEN), emptySet(), true)
@@ -1120,6 +1211,7 @@ class RealImapFolderTest {
     fun search_withFullTextSearchDisabled_shouldIssueRespectiveCommand() {
         val folder = createFolder("Folder")
         prepareImapFolderForOpen(OpenMode.READ_ONLY)
+        folder.open(OpenMode.READ_ONLY)
         setupUidSearchResponses("1 OK SEARCH completed")
 
         folder.search("query", emptySet(), emptySet(), false)

diff --git a/mail/protocols/imap/src/test/java/com/fsck/k9/mail/store/imap/TestImapFolder.kt b/mail/protocols/imap/src/test/java/com/fsck/k9/mail/store/imap/TestImapFolder.kt
@@ -18,7 +18,7 @@ internal open class TestImapFolder(
     override var messageCount: Int = 0
         protected set
 
-    var isOpen: Boolean = false
+    override var isOpen: Boolean = false
         protected set
 
     private var openAction: () -> Unit = {}
@@ -96,7 +96,7 @@ internal open class TestImapFolder(
         throw UnsupportedOperationException("not implemented")
     }
 
-    override fun setFlags(flags: Set<Flag>, value: Boolean) {
+    override fun setFlagsForAllMessages(flags: Set<Flag>, value: Boolean) {
         throw UnsupportedOperationException("not implemented")
     }