Merge pull request #1248 from nhirokinet/feature/caldav-sync-limit

CalDAV to sync properly when limit is set in PDO backend
sabre-io · Dec 2, 2021 · 5f273e4 · 5f273e4
2 parents d1a3e9a + 69f114f
commit 5f273e4
Show file tree

Hide file tree

Showing 6 changed files with 102 additions and 21 deletions.
diff --git a/lib/CalDAV/Backend/PDO.php b/lib/CalDAV/Backend/PDO.php
@@ -948,42 +948,46 @@ public function getChangesForCalendar($calendarId, $syncToken, $syncLevel, $limi
         }
         list($calendarId, $instanceId) = $calendarId;
 
-        // Current synctoken
-        $stmt = $this->pdo->prepare('SELECT synctoken FROM '.$this->calendarTableName.' WHERE id = ?');
-        $stmt->execute([$calendarId]);
-        $currentToken = $stmt->fetchColumn(0);
-
-        if (is_null($currentToken)) {
-            return null;
-        }
-
         $result = [
-            'syncToken' => $currentToken,
             'added' => [],
             'modified' => [],
             'deleted' => [],
         ];
 
         if ($syncToken) {
-            $query = 'SELECT uri, operation FROM '.$this->calendarChangesTableName.' WHERE synctoken >= ? AND synctoken < ? AND calendarid = ? ORDER BY synctoken';
+            $query = 'SELECT uri, operation, synctoken FROM '.$this->calendarChangesTableName.' WHERE synctoken >= ?  AND calendarid = ? ORDER BY synctoken';
             if ($limit > 0) {
-                $query .= ' LIMIT '.(int) $limit;
+                // Fetch one more raw to detect result truncation
+                $query .= ' LIMIT '.((int) $limit + 1);
             }
 
             // Fetching all changes
             $stmt = $this->pdo->prepare($query);
-            $stmt->execute([$syncToken, $currentToken, $calendarId]);
+            $stmt->execute([$syncToken, $calendarId]);
 
             $changes = [];
 
             // This loop ensures that any duplicates are overwritten, only the
             // last change on a node is relevant.
             while ($row = $stmt->fetch(\PDO::FETCH_ASSOC)) {
-                $changes[$row['uri']] = $row['operation'];
+                $changes[$row['uri']] = $row;
             }
+            $currentToken = null;
 
+            $result_count = 0;
             foreach ($changes as $uri => $operation) {
-                switch ($operation) {
+                if (!is_null($limit) && $result_count >= $limit) {
+                    $result['result_truncated'] = true;
+                    break;
+                }
+
+                if (null === $currentToken || $currentToken < $operation['synctoken'] + 1) {
+                    // SyncToken in CalDAV perspective is consistently the next number of the last synced change event in this class.
+                    $currentToken = $operation['synctoken'] + 1;
+                }
+
+                ++$result_count;
+                switch ($operation['operation']) {
                     case 1:
                         $result['added'][] = $uri;
                         break;
@@ -995,7 +999,24 @@ public function getChangesForCalendar($calendarId, $syncToken, $syncLevel, $limi
                         break;
                 }
             }
+
+            if (!is_null($currentToken)) {
+                $result['syncToken'] = $currentToken;
+            } else {
+                // This means returned value is equivalent to syncToken
+                $result['syncToken'] = $syncToken;
+            }
         } else {
+            // Current synctoken
+            $stmt = $this->pdo->prepare('SELECT synctoken FROM '.$this->calendarTableName.' WHERE id = ?');
+            $stmt->execute([$calendarId]);
+            $currentToken = $stmt->fetchColumn(0);
+
+            if (is_null($currentToken)) {
+                return null;
+            }
+            $result['syncToken'] = $currentToken;
+
             // No synctoken supplied, this is the initial sync.
             $query = 'SELECT uri FROM '.$this->calendarObjectTableName.' WHERE calendarid = ?';
             $stmt = $this->pdo->prepare($query);

diff --git a/lib/CalDAV/Calendar.php b/lib/CalDAV/Calendar.php
@@ -396,7 +396,8 @@ public function getSyncToken()
      *   'deleted' => [
      *      'foo.php.bak',
      *      'old.txt'
-     *   ]
+     *   ],
+     *   'result_truncated' : true
      * ];
      *
      * The syncToken property should reflect the *current* syncToken of the
@@ -406,6 +407,9 @@ public function getSyncToken()
      * If the syncToken is specified as null, this is an initial sync, and all
      * members should be reported.
      *
+     * If result is truncated due to server limitation or limit by client,
+     * set result_truncated to true, otherwise set to false or do not add the key.
+     *
      * The modified property is an array of nodenames that have changed since
      * the last token.
      *
@@ -422,12 +426,17 @@ public function getSyncToken()
      * should be treated as infinite.
      *
      * If the limit (infinite or not) is higher than you're willing to return,
-     * you should throw a Sabre\DAV\Exception\TooMuchMatches() exception.
+     * the result should be truncated to fit the limit.
+     * Note that even when the result is truncated, syncToken must be consistent
+     * with the truncated result, not the result before truncation.
+     * (See RFC6578 Section 3.6 for detail)
      *
      * If the syncToken is expired (due to data cleanup) or unknown, you must
      * return null.
      *
      * The limit is 'suggestive'. You are free to ignore it.
+     * TODO: RFC6578 Setion 3.7 says that the server must fail when the server
+     * cannot truncate according to the limit, so it may not be just suggestive.
      *
      * @param string $syncToken
      * @param int    $syncLevel

diff --git a/lib/DAV/Sync/Plugin.php b/lib/DAV/Sync/Plugin.php
@@ -124,14 +124,19 @@ public function syncCollection($uri, SyncCollectionReport $report)
             throw new DAV\Exception\InvalidSyncToken('Invalid or unknown sync token');
         }
 
+        if (!array_key_exists('result_truncated', $changeInfo)) {
+            $changeInfo['result_truncated'] = false;
+        }
+
         // Encoding the response
         $this->sendSyncCollectionResponse(
             $changeInfo['syncToken'],
             $uri,
             $changeInfo['added'],
             $changeInfo['modified'],
             $changeInfo['deleted'],
-            $report->properties
+            $report->properties,
+            $changeInfo['result_truncated']
         );
     }
 
@@ -141,7 +146,7 @@ public function syncCollection($uri, SyncCollectionReport $report)
      * @param string $syncToken
      * @param string $collectionUrl
      */
-    protected function sendSyncCollectionResponse($syncToken, $collectionUrl, array $added, array $modified, array $deleted, array $properties)
+    protected function sendSyncCollectionResponse($syncToken, $collectionUrl, array $added, array $modified, array $deleted, array $properties, bool $resultTruncated = false)
     {
         $fullPaths = [];
 
@@ -164,6 +169,10 @@ protected function sendSyncCollectionResponse($syncToken, $collectionUrl, array
             $fullPath = $collectionUrl.'/'.$item;
             $responses[] = new DAV\Xml\Element\Response($fullPath, [], 404);
         }
+        if ($resultTruncated) {
+            $responses[] = new DAV\Xml\Element\Response($collectionUrl.'/', [], 507);
+        }
+
         $multiStatus = new DAV\Xml\Response\MultiStatus($responses, self::SYNCTOKEN_PREFIX.$syncToken);
 
         $this->server->httpResponse->setStatus(207);

diff --git a/tests/Sabre/CalDAV/Backend/AbstractPDOTest.php b/tests/Sabre/CalDAV/Backend/AbstractPDOTest.php
@@ -943,6 +943,34 @@ public function testGetChanges()
             'deleted' => [],
             'added' => ['todo1.ics', 'todo3.ics'],
         ], $result);
+
+        $backend->createCalendarObject($id, 'todo7.ics', $dummyTodo);
+        $backend->createCalendarObject($id, 'todo8.ics', $dummyTodo);
+        $backend->createCalendarObject($id, 'todo9.ics', $dummyTodo);
+
+        $currentToken = $result['syncToken'];
+
+        $result = $backend->getChangesForCalendar($id, $currentToken, 1, 2);
+
+        // according to RFC6578 Section 3.6, the result including syncToken must correpsond to one time (syncToken=8 here)
+        $this->assertEquals([
+            'syncToken' => 8,
+            'modified' => [],
+            'deleted' => [],
+            'added' => ['todo7.ics', 'todo8.ics'],
+            'result_truncated' => true,
+        ], $result);
+
+        $currentToken = $result['syncToken'];
+
+        $result = $backend->getChangesForCalendar($id, $currentToken, 1, 2);
+
+        $this->assertEquals([
+            'syncToken' => 9,
+            'modified' => [],
+            'deleted' => [],
+            'added' => ['todo9.ics'],
+        ], $result);
     }
 
     /**

diff --git a/tests/Sabre/DAV/Sync/MockSyncCollection.php b/tests/Sabre/DAV/Sync/MockSyncCollection.php
@@ -127,6 +127,8 @@ function ($item) {
         $modified = [];
         $deleted = [];
 
+        $result_truncated = false;
+
         foreach ($this->changeLog as $token => $change) {
             if ($token > $syncToken) {
                 $added = array_merge($added, $change['added']);
@@ -143,6 +145,7 @@ function ($item) {
                     if (0 === $left) {
                         break;
                     }
+                    $result_truncated = true;
                     if ($left < 0) {
                         $modified = array_slice($modified, 0, $left);
                     }
@@ -158,11 +161,17 @@ function ($item) {
             }
         }
 
-        return [
+        $result = [
             'syncToken' => $this->token,
             'added' => $added,
             'modified' => $modified,
             'deleted' => $deleted,
         ];
+
+        if ($result_truncated) {
+            $result += ['result_truncated' => $result_truncated];
+        }
+
+        return $result;
     }
 }
diff --git a/tests/Sabre/DAV/Sync/PluginTest.php b/tests/Sabre/DAV/Sync/PluginTest.php
@@ -224,13 +224,18 @@ public function testSubsequentSyncSyncCollectionLimit()
         );
 
         $responses = $multiStatus->getResponses();
-        $this->assertEquals(1, count($responses), 'We expected exactly 1 {DAV:}response');
+        $this->assertEquals(2, count($responses), 'We expected exactly 2 {DAV:}responses');
 
         $response = $responses[0];
 
         $this->assertEquals('404', $response->getHttpStatus());
         $this->assertEquals('/coll/file3.txt', $response->getHref());
         $this->assertEquals([], $response->getResponseProperties());
+
+        $response = $responses[1];
+
+        $this->assertEquals('507', $response->getHttpStatus());
+        $this->assertEquals('/coll/', $response->getHref());
     }
 
     public function testSubsequentSyncSyncCollectionDepthFallBack()