From 402bb777534ddc0ace2ccfb144a687cf08d1989a Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Fri, 8 Mar 2024 08:14:05 +0100 Subject: [PATCH] fix(dav): Add retention time to sync token cleanup Signed-off-by: Christoph Wurst --- apps/dav/appinfo/info.xml | 2 +- .../composer/composer/autoload_classmap.php | 1 + .../dav/composer/composer/autoload_static.php | 1 + .../PruneOutdatedSyncTokensJob.php | 5 +- apps/dav/lib/CalDAV/CalDavBackend.php | 8 +- apps/dav/lib/CardDAV/CardDavBackend.php | 8 +- .../Version1025Date20240308063933.php | 84 +++++++++++++++++++ .../PruneOutdatedSyncTokensJobTest.php | 25 ++++-- .../tests/unit/CalDAV/CalDavBackendTest.php | 13 ++- .../tests/unit/CardDAV/CardDavBackendTest.php | 14 +++- 10 files changed, 140 insertions(+), 21 deletions(-) create mode 100644 apps/dav/lib/Migration/Version1025Date20240308063933.php diff --git a/apps/dav/appinfo/info.xml b/apps/dav/appinfo/info.xml index ba98d1f18253a..d2266c36749f3 100644 --- a/apps/dav/appinfo/info.xml +++ b/apps/dav/appinfo/info.xml @@ -5,7 +5,7 @@ WebDAV WebDAV endpoint WebDAV endpoint - 1.29.1 + 1.29.2 agpl owncloud.org DAV diff --git a/apps/dav/composer/composer/autoload_classmap.php b/apps/dav/composer/composer/autoload_classmap.php index f3cfdf49c4400..630dbb32d43ba 100644 --- a/apps/dav/composer/composer/autoload_classmap.php +++ b/apps/dav/composer/composer/autoload_classmap.php @@ -312,6 +312,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\\Migration\\Version1029Date20221114151721' => $baseDir . '/../lib/Migration/Version1029Date20221114151721.php', 'OCA\\DAV\\Migration\\Version1029Date20231004091403' => $baseDir . '/../lib/Migration/Version1029Date20231004091403.php', diff --git a/apps/dav/composer/composer/autoload_static.php b/apps/dav/composer/composer/autoload_static.php index b8a3d0d5964b6..627bd3d3e0ed7 100644 --- a/apps/dav/composer/composer/autoload_static.php +++ b/apps/dav/composer/composer/autoload_static.php @@ -327,6 +327,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\\Migration\\Version1029Date20221114151721' => __DIR__ . '/..' . '/../lib/Migration/Version1029Date20221114151721.php', 'OCA\\DAV\\Migration\\Version1029Date20231004091403' => __DIR__ . '/..' . '/../lib/Migration/Version1029Date20231004091403.php', diff --git a/apps/dav/lib/BackgroundJob/PruneOutdatedSyncTokensJob.php b/apps/dav/lib/BackgroundJob/PruneOutdatedSyncTokensJob.php index 749690693874c..d5a877a174244 100644 --- a/apps/dav/lib/BackgroundJob/PruneOutdatedSyncTokensJob.php +++ b/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, diff --git a/apps/dav/lib/CalDAV/CalDavBackend.php b/apps/dav/lib/CalDAV/CalDavBackend.php index 5c11cbc186ead..218fe4fe32de5 100644 --- a/apps/dav/lib/CalDAV/CalDavBackend.php +++ b/apps/dav/lib/CalDAV/CalDavBackend.php @@ -2890,6 +2890,7 @@ protected function addChanges(int $calendarId, array $objectUris, int $operation 'calendarid' => $query->createNamedParameter($calendarId), 'operation' => $query->createNamedParameter($operation), 'calendartype' => $query->createNamedParameter($calendarType), + 'created_at' => time(), ]); foreach ($objectUris as $uri) { $query->setParameter('uri', $uri); @@ -3363,7 +3364,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(); } @@ -3381,7 +3382,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(); } diff --git a/apps/dav/lib/CardDAV/CardDavBackend.php b/apps/dav/lib/CardDAV/CardDavBackend.php index c1f0fe0c93c9c..0f7c0a9083f8e 100644 --- a/apps/dav/lib/CardDAV/CardDavBackend.php +++ b/apps/dav/lib/CardDAV/CardDavBackend.php @@ -993,6 +993,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(); @@ -1424,7 +1425,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(); } @@ -1442,7 +1443,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(); } diff --git a/apps/dav/lib/Migration/Version1025Date20240308063933.php b/apps/dav/lib/Migration/Version1025Date20240308063933.php new file mode 100644 index 0000000000000..a75fc85eccc39 --- /dev/null +++ b/apps/dav/lib/Migration/Version1025Date20240308063933.php @@ -0,0 +1,84 @@ + + * + * @author Christoph Wurst + * + * @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 . + * + */ + +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); + } + } + +} diff --git a/apps/dav/tests/unit/BackgroundJob/PruneOutdatedSyncTokensJobTest.php b/apps/dav/tests/unit/BackgroundJob/PruneOutdatedSyncTokensJobTest.php index be6298b3372ef..20169072687c5 100644 --- a/apps/dav/tests/unit/BackgroundJob/PruneOutdatedSyncTokensJobTest.php +++ b/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] ]; } } diff --git a/apps/dav/tests/unit/CalDAV/CalDavBackendTest.php b/apps/dav/tests/unit/CalDAV/CalDavBackendTest.php index ced00ef6024e6..ae3cf4aa5fe23 100644 --- a/apps/dav/tests/unit/CalDAV/CalDavBackendTest.php +++ b/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() { diff --git a/apps/dav/tests/unit/CardDAV/CardDavBackendTest.php b/apps/dav/tests/unit/CardDAV/CardDavBackendTest.php index 425e7c44ba7fd..deed0a06d2925 100644 --- a/apps/dav/tests/unit/CardDAV/CardDavBackendTest.php +++ b/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,7 +897,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 @@ -899,8 +905,8 @@ public function testPruneOutdatedSyncTokens(): void { $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()); } }