Rewrite IMAP expunge #2927

rhari991 · 2017-11-10T21:03:04Z

This PR rewrites the expunge() method in ImapFolder to ensure that only messages that were marked with the \Deleted flag by K-9 Mail are expunged. This process is described in section 4.2.4 of RFC 4549

Same as #2701

philipwhiuk · 2017-11-10T21:28:49Z

k9mail/src/main/java/com/fsck/k9/mailstore/LocalFolder.java

@@ -998,6 +998,42 @@ public LocalMessage doDbWork(final SQLiteDatabase db) throws WrappedException, U
        }
    }

+    public Set<Long> getDeletedMessageUids() throws MessagingException {


This is very similar to the method above. It should be possible to use one underlying method for most of this and just passing in a different query.

I'm also grumpy that there's no test, but there's no test for any LocalFolder stuff. Maybe I should look at that.

@philipwhiuk done !

philipwhiuk · 2017-11-10T21:31:35Z

k9mail/src/test/java/com/fsck/k9/controller/MessagingControllerTest.java

@@ -693,7 +693,7 @@ public void synchronizeMailboxSynchronous_withAccountPolicySetToExpungeOnPoll_sh

        controller.synchronizeMailboxSynchronous(account, FOLDER_NAME, listener, null);

-        verify(remoteFolder).expunge();
+        verify(remoteFolder).expunge(anySet());


anySet() is pretty weak. Can't we verify we pass in the right IDs?

cketti · 2017-11-11T18:00:23Z

k9mail-library/src/main/java/com/fsck/k9/mail/Folder.java

@@ -113,7 +113,7 @@ public abstract void setFlags(List<? extends Message> messages, Set<Flag> flags,

    public abstract String getUidFromMessageId(Message message) throws MessagingException;

-    public void expunge() throws MessagingException
+    public void expunge(List<Long> knownDeletedUids) throws MessagingException


With the Folder abstraction UIDs are opaque strings. UIDs being numbers is an implementation detail of ImapFolder. So until we can replace this interface with something more flexible let's keep this interface clean.

Also, sometimes (e.g. when emptying the Trash folder) an unconstrained EXPUNGE makes sense. So I'd keep the original method and add a new one that better describes what it is about. My suggestion:

public void expungeUids(List<String> uids) throws MessagingException

cketti · 2017-11-11T18:35:58Z

k9mail-library/src/main/java/com/fsck/k9/mail/store/imap/ImapFolder.java

        } catch (IOException ioe) {
            throw ioExceptionHandler(connection, ioe);
        }
    }

+    private void expungeWithoutUidPlus(Set<Long> knownDeletedUids) throws IOException, MessagingException {


I know this is what RFC 4549 suggests when UID EXPUNGE is not available, but I don't particularly like it. It has the potential to undelete messages when the connection is lost in the middle of the command sequence. It also requires passing all known deleted UIDs to the method.

By now UIDPLUS and with that UID EXPUNGE should be widely available. So I'm fine with K-9 Mail not making the best effort when using legacy servers. Also, most users don't care about messages marked as deleted. So I'd say fall back to a plain EXPUNGE when UID EXPUNGE is not available. That is what we're doing right now, anyway.

cketti · 2017-11-11T18:39:06Z

k9mail-library/src/main/java/com/fsck/k9/mail/store/imap/ImapFolder.java

@@ -1248,17 +1248,40 @@ public String getUidFromMessageId(Message message) throws MessagingException {
    }

    @Override
-    public void expunge() throws MessagingException {
+    public void expunge(List<Long> knownDeletedUids) throws MessagingException {
+        if (knownDeletedUids == null || knownDeletedUids.isEmpty()) {


Both cases are programming errors. So I'd rather throw an exception than returning silently.

cketti · 2017-11-11T18:40:19Z

k9mail-library/src/main/java/com/fsck/k9/mail/store/imap/ImapFolder.java

+
+    private void setFlagsInternal(Set<Long> uids, final Set<Flag> flags, boolean value)
+            throws MessagingException {
+        if (uids.isEmpty()) {


Again, this should never happen. If it does we want to know about it, so throw an exception.

cketti · 2017-11-11T18:42:46Z

k9mail-library/src/test/java/com/fsck/k9/mail/store/imap/ImapFolderTest.java

@@ -1228,7 +1256,7 @@ private void assertCommandWithIdsIssued(String expectedCommand) throws Messaging
            String command = commandPrefixes.get(i) +
                    " " + ImapUtility.join(",", commandUids.get(i)) + " " +
                    commandSuffixes.get(i);
-            if (command.equals(expectedCommand)) {
+            if (command.trim().equals(expectedCommand)) {


I'd rather fix the construction of command above than trimming the erroneously added space at the end.

cketti · 2017-11-11T18:44:35Z