From 121c3bc96a273790414ae114082053cb649fc49a Mon Sep 17 00:00:00 2001
From: Christian Mollekopf <chrigi_1@fastmail.fm>
Date: Mon, 25 Jun 2018 10:57:46 +0200
Subject: Fixed the lost update scenario

If we get a fetchMore right between when the revision was updated and
the incrementalQuery actually running, we ended up loosing the update
because the result provider ended up with a too recent revision after
the additional initial query.
---
 common/queryrunner.cpp |  6 +++++-
 common/queryrunner.h   | 15 ++++++++++++++-
 tests/querytest.cpp    | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/common/queryrunner.cpp b/common/queryrunner.cpp
index 0977940..9ac3517 100644
--- a/common/queryrunner.cpp
+++ b/common/queryrunner.cpp
@@ -152,7 +152,11 @@ void QueryRunner<DomainType>::fetch(const Sink::Query &query, const QByteArray &
             if (query.liveQuery()) {
                 mResourceAccess->sendRevisionReplayedCommand(result.newRevision);
             }
-            mResultProvider->setRevision(result.newRevision);
+            //Initial queries do not fetch updates, so avoid updating the revision when fetching more content.
+            //Otherwise we end up breaking incremental updates.
+            if (!mResultProvider->revision()) {
+                mResultProvider->setRevision(result.newRevision);
+            }
             mResultProvider->initialResultSetComplete(result.replayedAll);
             if (mRequestFetchMore) {
                 mRequestFetchMore = false;
diff --git a/common/queryrunner.h b/common/queryrunner.h
index e449570..52b5e2d 100644
--- a/common/queryrunner.h
+++ b/common/queryrunner.h
@@ -35,6 +35,16 @@ class QueryRunnerBase : public QObject
 public:
     typedef std::function<void(Sink::ApplicationDomain::ApplicationDomainType &domainObject)> ResultTransformation;
 
+    /// Disable query updates on revision change. Used for testing only.
+    void ignoreRevisionChanges(bool ignore) {
+        mIgnoreRevisionChanges = ignore;
+    }
+
+    /// Manually triger a revision change. Used for testing only.
+    void triggerRevisionChange() {
+        revisionChanged();
+    }
+
 protected:
     typedef std::function<KAsync::Job<void>()> QueryFunction;
 
@@ -52,7 +62,9 @@ protected slots:
      */
     void revisionChanged()
     {
-        run().exec();
+        if (!mIgnoreRevisionChanges) {
+            run().exec();
+        }
     }
 
 private:
@@ -65,6 +77,7 @@ private:
     }
 
     QueryFunction queryFunction;
+    bool mIgnoreRevisionChanges{false};
 };
 
 /**
diff --git a/tests/querytest.cpp b/tests/querytest.cpp
index b52ba96..7685086 100644
--- a/tests/querytest.cpp
+++ b/tests/querytest.cpp
@@ -1336,6 +1336,55 @@ private slots:
         QTRY_COMPARE(added.size(), 4);
     }
 
+    /*
+     * This test excercises the scenario where a fetchMore is triggered after
+     * the revision is already updated in storage, but the incremental query was not run yet.
+     * This resulted in lost modification updates.
+     */
+    void testQueryRunnerDontMissUpdatesWithFetchMore()
+    {
+        // Setup
+        auto folder1 = Folder::createEntity<Folder>("sink.dummy.instance1");
+        folder1.setName("name1");
+        VERIFYEXEC(Sink::Store::create<Folder>(folder1));
+        VERIFYEXEC(Sink::ResourceControl::flushMessageQueue("sink.dummy.instance1"));
+
+        Query query;
+        query.setFlags(Query::LiveQuery);
+
+        Sink::ResourceContext resourceContext{"sink.dummy.instance1", "sink.dummy", Sink::AdaptorFactoryRegistry::instance().getFactories("sink.dummy")};
+        Sink::Log::Context logCtx;
+        auto runner = new QueryRunner<Folder>(query, resourceContext, ApplicationDomain::getTypeName<Folder>(), logCtx);
+
+        auto emitter = runner->emitter();
+        QList<Folder::Ptr> added;
+        emitter->onAdded([&](Folder::Ptr folder) {
+            added << folder;
+        });
+        QList<Folder::Ptr> modified;
+        emitter->onModified([&](Folder::Ptr folder) {
+            modified << folder;
+        });
+
+        emitter->fetch();
+        QTRY_COMPARE(added.size(), 1);
+        QCOMPARE(modified.size(), 0);
+
+        runner->ignoreRevisionChanges(true);
+
+        folder1.setName("name2");
+        VERIFYEXEC(Sink::Store::modify<Folder>(folder1));
+        VERIFYEXEC(Sink::ResourceControl::flushMessageQueue("sink.dummy.instance1"));
+
+        emitter->fetch();
+
+        runner->ignoreRevisionChanges(false);
+        runner->triggerRevisionChange();
+
+        QTRY_COMPARE(added.size(), 1);
+        QTRY_COMPARE(modified.size(), 1);
+    }
+
     /*
      * This test is here to ensure we don't crash if we call removeFromDisk with a running query.
      */
-- 
cgit v1.2.3