Add support for CONDSTORE and QRESYNC #2607

rhari991 · 2017-06-27T21:50:30Z

This PR does several things

Moves the folder synchronization code from MessagingController into separate classes depending on the protocol (ImapSyncInteractor for IMAP accounts and LegacySyncInteractor for POP3 and WebDAV accounts)
Modifies the MessagingControllerPushReceiver so that a normal synchronization is performed when untagged responses are received through IDLE, instead of response-specific actions. This synchronization is done through a separate connection instead of reusing the connection through which IDLE responses are received.
Adds support for the CONDSTORE and QRESYNC IMAP extensions as described in RFC 7162.

philipwhiuk · 2017-06-28T10:16:34Z

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

+    public List<ImapMessage> getChangedMessagesUsingCondstore(long modseq) throws MessagingException {
+        checkOpen();
+        if (!supportsModSeq()) {
+            throw new MessagingException(String.format(Locale.US, "Folder %s does not support modification " +


Put the string on a new line so you don't have to build a new one every time.

rhari991 · 2017-07-06T01:25:02Z

Currently, the sync process is as follows :

Get the UIDs of the most recent n messages on the server, where n is the number of messages displayed to the user (via UID SEARCH). From these, messages which arrived on a date earlier than the earliest poll date are removed.
Delete local messages which are not present in the above group. Then check and update whether more messages are present in the mailbox (via one or more UID SEARCH).
Divide the messages into two groups - one for messages that need to be downloaded fully (if not present locally) and one for syncing flags (if it is present locally).
The unsynced messages are fetched.
a) Initially, the flags and envelope for all unsynced messages are fetched.
b) The messages are then divided into large and small message groups depending on the maximum auto-download message size.
c) Download all the small messages fully (using grouped UID FETCH requests).
d) Download partial/sane bodies for large messages (one UID FETCH per message, and multiple if structure is present).
The flags for all the messages present in the local store are fetched and updated.

When CONDSTORE is used :

In step 1, the UIDs for all messages that are present/going to be present in the mailbox are fetched. This is necessary in order to sync remote deletions and some use cases such as the user pressing the "Load more messages" button (historical data needs to be fetched). Because of these reasons, this step cannot be optimized using the previously stored mod-sequence.
Related to the point above, one optimization could be to remove the "Load more messages" button and instead automatically download more messages if the user scrolls to the bottom of the message list. This should feel more natural, and will free us from having to make a network request to check if more messages are available.
For step 5, the optimization that can be done is to only fetch flags for those messages whose contents have changed. This is done by sending an extra UID SEARCH command with the previously known mod sequence and then syncing the flags for the returned message uids only.
An extra UID FETCH command also needs to be issued every time we get a response through IDLE in order to update the highest modseq value.

Thoughts ? @Valodim @philipwhiuk @cketti

Valodim · 2017-07-06T18:38:04Z

description matches with my understanding of the algorithm.

Regarding the other points:

Fetching all UIDs for finding deleted messages - shouldn't that be solved by QRESYNC?
I don't understand how a fetch-on-scroll mechanism would free us from that network request. Either way, it would also be too much of a semantic shift to bring in here.
Regarding IDLE, I think we are not doing us any favors trying to be clever there. Just open a connection, then when anything happens just trigger the usual sync process.

rhari991 · 2017-08-08T06:54:17Z

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

-        final String queryString, final String[] placeHolders
+            final MessageRetrievalListener<LocalMessage> listener,
+            final LocalFolder folder,
+            final String queryString, final String[] placeHolders
    ) throws MessagingException {
        final List<LocalMessage> messages = new ArrayList<>();
        final int j = database.execute(false, new DbCallback<Integer>() {


@Valodim @cketti Any idea why we first query 10 rows and then the remaining ones in this method ?

rhari991 · 2017-08-13T14:34:56Z

There are 7 tests in MessagingControllerTest (which test the sendPendingMessagesSynchronous method) and 8 tests in MigrationTest that are failing in the Travis build. These are failing for me when I run them on the master branch too. Does this happen with you guys too or am I missing something ? @Valodim @philipwhiuk @cketti

When running MigrationTest , I get this exception for all the failed tests

`android.content.res.Resources$NotFoundException: Unable to find resource ID #0x7f07018e in packages [android, org.robolectric.default]

at org.robolectric.shadows.ShadowAssetManager.getResName(ShadowAssetManager.java:868)
at org.robolectric.shadows.ShadowAssetManager.resolveResourceValue(ShadowAssetManager.java:676)
at org.robolectric.shadows.ShadowAssetManager.resolve(ShadowAssetManager.java:636)
at org.robolectric.shadows.ShadowAssetManager.getAndResolve(ShadowAssetManager.java:630)
at org.robolectric.shadows.ShadowAssetManager.getResourceText(ShadowAssetManager.java:226)
at android.content.res.AssetManager.getResourceText(AssetManager.java)
at android.content.res.Resources.getText(Resources.java:295)
at android.content.res.Resources.getString(Resources.java:385)
at android.content.Context.getString(Context.java:377)
at com.fsck.k9.Account.<init>(Account.java:331)
at com.fsck.k9.Preferences.newAccount(Preferences.java:113)
at com.fsck.k9.mailstore.MigrationTest.getNewAccount(MigrationTest.java:725)
at com.fsck.k9.mailstore.MigrationTest.setUp(MigrationTest.java:49)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:24)
at org.robolectric.RobolectricTestRunner$2.evaluate(RobolectricTestRunner.java:339)
at org.robolectric.RobolectricTestRunner.runChild(RobolectricTestRunner.java:259)
at org.robolectric.RobolectricTestRunner.runChild(RobolectricTestRunner.java:41)
at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
at org.robolectric.RobolectricTestRunner$1.evaluate(RobolectricTestRunner.java:199)
at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:117)
at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:42)
at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:262)
at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:84)
at com.intellij.rt.execution.application.AppMain.main(AppMain.java:147)`

The related line, Account.java:331 seems to take a String resource which has not been mocked yet ?

For the failing tests in MessagingControllerTest, the assertions are failing.

philipwhiuk · 2017-08-13T20:34:20Z

I believe this occurs because IntelliJ (and thus Android Studio)'s default Run Configuration is based on the project working directory not the module working directory ( / instead of /k9mail ). Hence it can't find the resources.

When I run the tests for master via /gradlew test they pass.

When I run your branch's tests in the same way they fail. A lot of the failures are due to this:

java.lang.NullPointerException
	at com.fsck.k9.mail.store.imap.ImapStore.isStoreUriImap(ImapStore.java:140)
	at com.fsck.k9.controller.MessagingController.synchronizeMailboxSynchronous(MessagingController.java:757)
	at com.fsck.k9.controller.MessagingControllerTest.synchronizeMailboxSynchronous_withAccountSupportingFetchingFlags_shouldFetchUnsychronizedMessagesListAndFlags(MessagingControllerTest.java:793)
        at ...

which should be fairly trivial to fix.

rhari991 · 2017-08-13T22:02:05Z

@philipwhiuk thanks for the tip, setting the JUnit working directory did the trick

philipwhiuk · 2017-08-14T22:04:25Z

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

@@ -3,6 +3,7 @@



I'm not a big fan of making this a super object in terms of configuration. ImapConfig would be a good name for a class that could house the new stuff being added here.

philipwhiuk · 2017-08-14T22:05:56Z

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

        return hasCapability(Capabilities.CONDSTORE);
    }

+    boolean isQresyncCapable() throws IOException, MessagingException {


Why is isCondstoreCapable public and this not?

isCondstoreCapable is used in the FolderSelectedStateCommand.getCommandLengthLimit() method, which is in the selectedstate subpackage.

philipwhiuk · 2017-08-14T22:07:59Z

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

+            if (response.hasOpenMode()) {
+                this.mode = response.getOpenMode();
+            }
+            if (response == null) {


This null check is pointless. Line 182 would throw an NPE before it got here.

philipwhiuk · 2017-08-14T22:10:13Z

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

                        }
+                    } catch (Exception e) {


Exception is a very broad catch case. Try to narrow this down a bit.

Funny, the ConcurrentHashMap.put() method does not throw any checked exceptions, so this try/catch block is useless. I'm not sure why this was there in the first place. I moved around a lot of original code without modifying it.

philipwhiuk · 2017-08-14T22:12:39Z

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

+        }
+    }
+
+    private void handleExpungeResponses(List<ImapResponse> imapResponses) {


This doesn't really 'handle' the response - it just searches them for a higher mod seq.

updateHighestModSequenceFromResponses.

philipwhiuk · 2017-08-14T22:17:43Z

k9mail/src/main/java/com/fsck/k9/controller/NonQresyncExtensionHandler.java

+                    new MessageRetrievalListener<ImapMessage>() {
+                        @Override
+                        public void messageStarted(String uid, int number, int ofTotal) {
+


I don't think anything is required here ?

philipwhiuk · 2017-08-14T22:18:57Z

k9mail/src/main/java/com/fsck/k9/controller/SyncHelper.java

+        if (folderName.equals(account.getTrashFolderName()) ||
+                folderName.equals(account.getSentFolderName()) ||
+                folderName.equals(account.getDraftsFolderName())) {
+            if (!remoteFolder.exists()) {


Can you not combine these if's to reduce the nesting a bit?

philipwhiuk · 2017-08-14T22:20:19Z

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

@@ -74,6 +75,8 @@
    private static final long serialVersionUID = -1973296520918624767L;
    private static final int MAX_BODY_SIZE_FOR_DATABASE = 16 * 1024;
    static final long INVALID_MESSAGE_PART_ID = -1;
+    private static final int INVALID_UID_VALIDITY = -1;


Redefining the same constants as in the library. Maybe they should be visible if you need them here?

philipwhiuk · 2017-08-14T22:20:58Z

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

        mFolderId = cursor.getInt(LocalStore.FOLDER_ID_INDEX);
        mName = cursor.getString(LocalStore.FOLDER_NAME_INDEX);
        mVisibleLimit = cursor.getInt(LocalStore.FOLDER_VISIBLE_LIMIT_INDEX);
+
+        //The index check here is because of an issue that occurs while upgrading the db to V55 - at that point, the
+        //uid_validity and highest_mod_Seq columns do not exist yet


This is why we have DatabaseMigration...

The problem is that the open(Cursor) method is called when upgrading to V55, but the uid_validity and highest_mod_seq columns do not exist yet. They are added only in V61. There does not seem to be a reliable way to recreate the open(Cursor) method in MigrationTo61 .

philipwhiuk · 2017-08-14T22:22:11Z

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

+
+                catch (Exception e) {
+                    if (!"X_BAD_FLAG".equals(flag)) {
+                        Timber.w("Unable to parse flag %s", flag);


Why not check for X_BAD_FLAG first?

rhari991 · 2017-08-15T20:29:28Z

@philipwhiuk made the changes

rhari991 · 2017-09-12T14:19:27Z

@Valodim @philipwhiuk I'm not sure why/how the CertificationException is showing up, could you have a look ?

Valodim · 2017-09-12T14:26:34Z

possibly related to #2675?

Hari added 13 commits June 12, 2017 11:17

Added base, search and capability classes

3e8b043

Finished SEARCH and CAPABILITY commands

b6f2807

Add SelectByIdCommand, refactor and add tests

2c4d521

Added UidStoreCommand

4e8829f

Added UidFetchCommand

0f46f5a

Added UidCopyCommand

0a1e8e7

Clean up and refactor

9c311d6

Fix and add tests

6079820

Remove repeated code and refactor

8f073e0

Create ImapCommandSplitterTest

7305efe

Store highestmodseq locally

c7a1c5d

Add modSeq parameter to Uid Search

feea224

Use Condstore for synchronization when possible

7858104

philipwhiuk reviewed Jun 28, 2017

View reviewed changes

Hari added 3 commits June 30, 2017 02:50

Add modseq field to the fetch command and ImapMessage

4055e1f

Support updating of highestModSeq for IDLE

8c3beb4

Split SynchronizeMailboxSynchronous

be9058c

rhari991 self-assigned this Jun 30, 2017

rhari991 added type: architecture Architecture of the project and high level design being worked on and removed type: architecture Architecture of the project and high level design being worked on labels Jun 30, 2017

Add tests and some small changes

afe1f01

Small changes

cc03e43

rhari991 mentioned this pull request Jul 8, 2017

Add support for the UIDVALIDITY field in IMAP accounts #2330

Closed

Hari added 3 commits July 9, 2017 15:49

Add support for UIDVALIDITY

b852b8e

Enable Qresync if available

ec52b2d

Add QRESYNC params to SELECT and EXAMINE commands

f73f26a

rhari991 commented Aug 8, 2017

View reviewed changes

rhari991 removed their assignment Aug 13, 2017

rhari991 mentioned this pull request Aug 13, 2017

Rewrite IMAP IDLE #2671

Closed

rhari991 force-pushed the condstore-support branch from 0870342 to dcd230c Compare August 14, 2017 14:11

Hari added 3 commits August 14, 2017 20:09

Fix MigrationsTest and move new columns to the end of the table

4118a14

Fix and add tests

4d1febf

Remove custom manifest config in controller tests

cf1bc5d

rhari991 force-pushed the condstore-support branch from 68a688b to cf1bc5d Compare August 14, 2017 14:40

philipwhiuk reviewed Aug 14, 2017

View reviewed changes

Hari added 2 commits August 16, 2017 00:07

Create ImapConfig for IMAP-related debugging options

74aa857

Small changes

7831ffe

rhari991 force-pushed the condstore-support branch 2 times, most recently from 8e03dc0 to 7831ffe Compare August 22, 2017 18:23

cketti mentioned this pull request Aug 27, 2017

Add support for IMAP labels #769

Open

rhari991 force-pushed the gsoc-2017-rhari991 branch 4 times, most recently from 39156ee to 524d8ba Compare September 8, 2017 15:32

rhari991 force-pushed the condstore-support branch 2 times, most recently from 48d146b to 33dc02c Compare September 10, 2017 06:37

Rebase

75a0452

rhari991 force-pushed the condstore-support branch from 33dc02c to 75a0452 Compare September 12, 2017 03:41

cketti closed this Dec 28, 2022

cketti deleted the condstore-support branch January 28, 2023 18:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for CONDSTORE and QRESYNC #2607

Add support for CONDSTORE and QRESYNC #2607

rhari991 commented Jun 27, 2017 •

edited

Loading

philipwhiuk Jun 28, 2017

rhari991 commented Jul 6, 2017 •

edited

Loading

Valodim commented Jul 6, 2017

rhari991 Aug 8, 2017

rhari991 commented Aug 13, 2017

philipwhiuk commented Aug 13, 2017 •

edited

Loading

rhari991 commented Aug 13, 2017

philipwhiuk Aug 14, 2017

rhari991 Aug 15, 2017

philipwhiuk Aug 14, 2017

rhari991 Aug 15, 2017

philipwhiuk Aug 14, 2017

rhari991 Aug 15, 2017

philipwhiuk Aug 14, 2017

rhari991 Aug 15, 2017

philipwhiuk Aug 14, 2017

rhari991 Aug 15, 2017

philipwhiuk Aug 14, 2017

rhari991 Aug 15, 2017

philipwhiuk Aug 14, 2017

rhari991 Aug 15, 2017

philipwhiuk Aug 14, 2017

rhari991 Aug 15, 2017

philipwhiuk Aug 14, 2017

rhari991 Aug 15, 2017

philipwhiuk Aug 14, 2017

rhari991 Aug 15, 2017

rhari991 commented Aug 15, 2017

rhari991 commented Sep 12, 2017

Valodim commented Sep 12, 2017

Add support for CONDSTORE and QRESYNC #2607

Add support for CONDSTORE and QRESYNC #2607

Conversation

rhari991 commented Jun 27, 2017 • edited Loading

Choose a reason for hiding this comment

rhari991 commented Jul 6, 2017 • edited Loading

Valodim commented Jul 6, 2017

Choose a reason for hiding this comment

rhari991 commented Aug 13, 2017

philipwhiuk commented Aug 13, 2017 • edited Loading

rhari991 commented Aug 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhari991 commented Aug 15, 2017

rhari991 commented Sep 12, 2017

Valodim commented Sep 12, 2017

rhari991 commented Jun 27, 2017 •

edited

Loading

rhari991 commented Jul 6, 2017 •

edited

Loading

philipwhiuk commented Aug 13, 2017 •

edited

Loading