fix(dav): Add retention time to sync token cleanup

Signed-off-by: Christoph Wurst <[email protected]>

[skip ci]

apps/dav/composer/composer/autoload_classmap.php

@@ -296,6 +296,7 @@
     'OCA\\DAV\\Migration\\Version1017Date20210216083742' => $baseDir . '/../lib/Migration/Version1017Date20210216083742.php',
     'OCA\\DAV\\Migration\\Version1018Date20210312100735' => $baseDir . '/../lib/Migration/Version1018Date20210312100735.php',
     'OCA\\DAV\\Migration\\Version1024Date20211221144219' => $baseDir . '/../lib/Migration/Version1024Date20211221144219.php',
+    'OCA\\DAV\\Migration\\Version1025Date20240308063933' => $baseDir . '/../lib/Migration/Version1025Date20240308063933.php',
     'OCA\\DAV\\Migration\\Version1027Date20230504122946' => $baseDir . '/../lib/Migration/Version1027Date20230504122946.php',
     'OCA\\DAV\\Profiler\\ProfilerPlugin' => $baseDir . '/../lib/Profiler/ProfilerPlugin.php',
     'OCA\\DAV\\Provisioning\\Apple\\AppleProvisioningNode' => $baseDir . '/../lib/Provisioning/Apple/AppleProvisioningNode.php',

apps/dav/composer/composer/autoload_static.php

@@ -311,6 +311,7 @@ class ComposerStaticInitDAV
         'OCA\\DAV\\Migration\\Version1017Date20210216083742' => __DIR__ . '/..' . '/../lib/Migration/Version1017Date20210216083742.php',
         'OCA\\DAV\\Migration\\Version1018Date20210312100735' => __DIR__ . '/..' . '/../lib/Migration/Version1018Date20210312100735.php',
         'OCA\\DAV\\Migration\\Version1024Date20211221144219' => __DIR__ . '/..' . '/../lib/Migration/Version1024Date20211221144219.php',
+        'OCA\\DAV\\Migration\\Version1025Date20240308063933' => __DIR__ . '/..' . '/../lib/Migration/Version1025Date20240308063933.php',
         'OCA\\DAV\\Migration\\Version1027Date20230504122946' => __DIR__ . '/..' . '/../lib/Migration/Version1027Date20230504122946.php',
         'OCA\\DAV\\Profiler\\ProfilerPlugin' => __DIR__ . '/..' . '/../lib/Profiler/ProfilerPlugin.php',
         'OCA\\DAV\\Provisioning\\Apple\\AppleProvisioningNode' => __DIR__ . '/..' . '/../lib/Provisioning/Apple/AppleProvisioningNode.php',

apps/dav/lib/BackgroundJob/PruneOutdatedSyncTokensJob.php

@@ -52,9 +52,10 @@ public function __construct(ITimeFactory $timeFactory, CalDavBackend $calDavBack
 
 	public function run($argument) {
 		$limit = max(1, (int) $this->config->getAppValue(Application::APP_ID, 'totalNumberOfSyncTokensToKeep', '10000'));
+		$retention = max(7, (int) $this->config->getAppValue(Application::APP_ID, 'syncTokensRetentionDays', '60')) * 24 * 3600;
 
-		$prunedCalendarSyncTokens = $this->calDavBackend->pruneOutdatedSyncTokens($limit);
-		$prunedAddressBookSyncTokens = $this->cardDavBackend->pruneOutdatedSyncTokens($limit);
+		$prunedCalendarSyncTokens = $this->calDavBackend->pruneOutdatedSyncTokens($limit, $retention);
+		$prunedAddressBookSyncTokens = $this->cardDavBackend->pruneOutdatedSyncTokens($limit, $retention);
 
 		$this->logger->info('Pruned {calendarSyncTokensNumber} calendar sync tokens and {addressBooksSyncTokensNumber} address book sync tokens', [
 			'calendarSyncTokensNumber' => $prunedCalendarSyncTokens,

apps/dav/lib/CalDAV/CalDavBackend.php

@@ -3199,7 +3199,7 @@ protected function getCalendarObjectId($calendarId, $uri, $calendarType):int {
 	/**
 	 * @throws \InvalidArgumentException
 	 */
-	public function pruneOutdatedSyncTokens(int $keep = 10_000): int {
+	public function pruneOutdatedSyncTokens(int $keep, int $retention): int {
 		if ($keep < 0) {
 			throw new \InvalidArgumentException();
 		}
@@ -3217,7 +3217,10 @@ public function pruneOutdatedSyncTokens(int $keep = 10_000): int {
 
 		$query = $this->db->getQueryBuilder();
 		$query->delete('calendarchanges')
-			->where($query->expr()->lte('id', $query->createNamedParameter($maxId - $keep, IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT));
+			->where(
+				$query->expr()->lte('id', $query->createNamedParameter($maxId - $keep, IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT),
+				$query->expr()->lte('created_at', $query->createNamedParameter($retention)),
+			);
 		return $query->executeStatement();
 	}
 

apps/dav/lib/CardDAV/CardDavBackend.php

@@ -992,6 +992,7 @@ protected function addChange(int $addressBookId, string $objectUri, int $operati
 					'synctoken' => $query->createNamedParameter($syncToken),
 					'addressbookid' => $query->createNamedParameter($addressBookId),
 					'operation' => $query->createNamedParameter($operation),
+					'created_at' => time(),
 				])
 				->executeStatement();
 
@@ -1397,7 +1398,7 @@ public function applyShareAcl(int $addressBookId, array $acl): array {
 	/**
 	 * @throws \InvalidArgumentException
 	 */
-	public function pruneOutdatedSyncTokens(int $keep = 10_000): int {
+	public function pruneOutdatedSyncTokens(int $keep, int $retention): int {
 		if ($keep < 0) {
 			throw new \InvalidArgumentException();
 		}
@@ -1415,7 +1416,10 @@ public function pruneOutdatedSyncTokens(int $keep = 10_000): int {
 
 		$query = $this->db->getQueryBuilder();
 		$query->delete('addressbookchanges')
-			->where($query->expr()->lte('id', $query->createNamedParameter($maxId - $keep, IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT));
+			->where(
+				$query->expr()->lte('id', $query->createNamedParameter($maxId - $keep, IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT),
+				$query->expr()->lte('created_at', $query->createNamedParameter($retention)),
+			);
 		return $query->executeStatement();
 	}
 

apps/dav/lib/Migration/Version1025Date20240308063933.php

@@ -0,0 +1,84 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * @copyright Copyright (c) 2024 Christoph Wurst <[email protected]>
+ *
+ * @author Christoph Wurst <[email protected]>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+namespace OCA\DAV\Migration;
+
+use Closure;
+use OCP\DB\ISchemaWrapper;
+use OCP\DB\QueryBuilder\IQueryBuilder;
+use OCP\DB\Types;
+use OCP\IDBConnection;
+use OCP\Migration\IOutput;
+use OCP\Migration\SimpleMigrationStep;
+
+class Version1025Date20240308063933 extends SimpleMigrationStep {
+
+	private IDBConnection $db;
+
+	public function __construct(IDBConnection $db) {
+		$this->db = $db;
+	}
+
+	/**
+	 * @param IOutput $output
+	 * @param Closure(): ISchemaWrapper $schemaClosure
+	 * @param array $options
+	 * @return null|ISchemaWrapper
+	 */
+	public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper {
+		/** @var ISchemaWrapper $schema */
+		$schema = $schemaClosure();
+
+		foreach (['addressbookchanges', 'calendarchanges'] as $tableName) {
+			$table = $schema->getTable($tableName);
+			if (!$table->hasColumn('created_at')) {
+				$table->addColumn('created_at', Types::INTEGER, [
+					'notnull' => true,
+					'length' => 4,
+					'default' => 0,
+				]);
+			}
+		}
+
+		return $schema;
+	}
+
+	public function postSchemaChange(IOutput $output, \Closure $schemaClosure, array $options): void {
+		foreach (['addressbookchanges', 'calendarchanges'] as $tableName) {
+			$qb = $this->db->getQueryBuilder();
+
+			$update = $qb->update($tableName)
+				->set('created_at', $qb->createNamedParameter(time(), IQueryBuilder::PARAM_INT))
+				->where(
+					$qb->expr()->eq('created_at', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT)),
+				);
+
+			$updated = $update->executeStatement();
+			$output->debug('Added a default creation timestamp to ' . $updated . ' rows in ' . $tableName);
+		}
+	}
+
+}

apps/dav/tests/unit/BackgroundJob/PruneOutdatedSyncTokensJobTest.php

@@ -29,6 +29,7 @@
  */
 namespace OCA\DAV\Tests\unit\BackgroundJob;
 
+use InvalidArgumentException;
 use OCA\DAV\AppInfo\Application;
 use OCA\DAV\BackgroundJob\PruneOutdatedSyncTokensJob;
 use OCA\DAV\CalDAV\CalDavBackend;
@@ -72,18 +73,27 @@ protected function setUp(): void {
 	/**
 	 * @dataProvider dataForTestRun
 	 */
-	public function testRun(string $configValue, int $actualLimit, int $deletedCalendarSyncTokens, int $deletedAddressBookSyncTokens): void {
-		$this->config->expects($this->once())
+	public function testRun(string $configToKeep, string $configRetentionDays, int $actualLimit, int $retentionDays, int $deletedCalendarSyncTokens, int $deletedAddressBookSyncTokens): void {
+		$this->config->expects($this->exactly(2))
 			->method('getAppValue')
-			->with(Application::APP_ID, 'totalNumberOfSyncTokensToKeep', '10000')
-			->willReturn($configValue);
+			->with(Application::APP_ID, self::anything(), self::anything())
+			->willReturnCallback(function ($app, $key) use ($configToKeep, $configRetentionDays) {
+				switch ($key) {
+					case 'totalNumberOfSyncTokensToKeep':
+						return $configToKeep;
+					case 'syncTokensRetentionDays':
+						return $configRetentionDays;
+					default:
+						throw new InvalidArgumentException();
+				}
+			});
 		$this->calDavBackend->expects($this->once())
 			->method('pruneOutdatedSyncTokens')
 			->with($actualLimit)
 			->willReturn($deletedCalendarSyncTokens);
 		$this->cardDavBackend->expects($this->once())
 			->method('pruneOutdatedSyncTokens')
-			->with($actualLimit)
+			->with($actualLimit, $retentionDays)
 			->willReturn($deletedAddressBookSyncTokens);
 		$this->logger->expects($this->once())
 			->method('info')
@@ -97,8 +107,9 @@ public function testRun(string $configValue, int $actualLimit, int $deletedCalen
 
 	public function dataForTestRun(): array {
 		return [
-			['100', 100, 2, 3],
-			['0', 1, 0, 0]
+			['100', '2', 100, 7 * 24 * 3600, 2, 3],
+			['100', '14', 100, 14 * 24 * 3600, 2, 3],
+			['0', '60', 1, 60 * 24 * 3600, 0, 0]
 		];
 	}
 }

apps/dav/tests/unit/CalDAV/CalDavBackendTest.php

@@ -46,6 +46,7 @@
 use Sabre\DAV\PropPatch;
 use Sabre\DAV\Xml\Property\Href;
 use Sabre\DAVACL\IACL;
+use function time;
 
 /**
  * Class CalDavBackendTest
@@ -1344,7 +1345,12 @@ public function testPruneOutdatedSyncTokens(): void {
 END:VCALENDAR
 EOD;
 		$this->backend->updateCalendarObject($calendarId, $uri, $calData);
-		$deleted = $this->backend->pruneOutdatedSyncTokens(0);
+
+		// Keep everything
+		$deleted = $this->backend->pruneOutdatedSyncTokens(0, 0);
+		self::assertSame(0, $deleted);
+
+		$deleted = $this->backend->pruneOutdatedSyncTokens(0, time());
 		// At least one from the object creation and one from the object update
 		$this->assertGreaterThanOrEqual(2, $deleted);
 		$changes = $this->backend->getChangesForCalendar($calendarId, $syncToken, 1);
@@ -1410,7 +1416,7 @@ public function testPruneOutdatedSyncTokens(): void {
 		$this->assertEmpty($changes['deleted']);	
 
 		// Delete all but last change
-		$deleted = $this->backend->pruneOutdatedSyncTokens(1);
+		$deleted = $this->backend->pruneOutdatedSyncTokens(1, time());
 		$this->assertEquals(1, $deleted); // We had two changes before, now one
 
 		// Only update should remain
@@ -1420,7 +1426,8 @@ public function testPruneOutdatedSyncTokens(): void {
 		$this->assertEmpty($changes['deleted']);
 
 		// Check that no crash occurs when prune is called without current changes
-		$deleted = $this->backend->pruneOutdatedSyncTokens(1);
+		$deleted = $this->backend->pruneOutdatedSyncTokens(1, time());
+		self::assertSame(0, $deleted);
 	}
 
 	public function testSearchAndExpandRecurrences() {

apps/dav/tests/unit/CardDAV/CardDavBackendTest.php

@@ -55,6 +55,7 @@
 use Sabre\VObject\Component\VCard;
 use Sabre\VObject\Property\Text;
 use Test\TestCase;
+use function time;
 
 /**
  * Class CardDavBackendTest
@@ -859,7 +860,12 @@ public function testPruneOutdatedSyncTokens(): void {
 		$uri = $this->getUniqueID('card');
 		$this->backend->createCard($addressBookId, $uri, $this->vcardTest0);
 		$this->backend->updateCard($addressBookId, $uri, $this->vcardTest1);
-		$deleted = $this->backend->pruneOutdatedSyncTokens(0);
+
+		// Do not delete anything if week data as old as ts=0
+		$deleted = $this->backend->pruneOutdatedSyncTokens(0, 0);
+		self::assertSame(0, $deleted);
+
+		$deleted = $this->backend->pruneOutdatedSyncTokens(0, time());
 		// At least one from the object creation and one from the object update
 		$this->assertGreaterThanOrEqual(2, $deleted);
 		$changes = $this->backend->getChangesForAddressBook($addressBookId, $syncToken, 1);
@@ -891,16 +897,16 @@ public function testPruneOutdatedSyncTokens(): void {
 		$this->assertEmpty($changes['deleted']);
 
 		// Delete all but last change
-		$deleted = $this->backend->pruneOutdatedSyncTokens(1);
+		$deleted = $this->backend->pruneOutdatedSyncTokens(1, time());
 		$this->assertEquals(1, $deleted); // We had two changes before, now one
 
 		// Only update should remain
 		$changes = $this->backend->getChangesForAddressBook($addressBookId, $syncToken, 100);
 		$this->assertEmpty($changes['added']);
 		$this->assertEquals(1, count($changes['modified']));
 		$this->assertEmpty($changes['deleted']);
-		
+
 		// Check that no crash occurs when prune is called without current changes
-		$deleted = $this->backend->pruneOutdatedSyncTokens(1);
+		$deleted = $this->backend->pruneOutdatedSyncTokens(1, time());
 	}
 }