Skip to content

Commit 3dea99f

Browse files
fix(dav): Add retention time to sync token cleanup
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
1 parent a7dfec0 commit 3dea99f

File tree

10 files changed

+140
-21
lines changed

10 files changed

+140
-21
lines changed

apps/dav/appinfo/info.xml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
<name>WebDAV</name>
66
<summary>WebDAV endpoint</summary>
77
<description>WebDAV endpoint</description>
8-
<version>1.30.0</version>
8+
<version>1.30.1</version>
99
<licence>agpl</licence>
1010
<author>owncloud.org</author>
1111
<namespace>DAV</namespace>

apps/dav/composer/composer/autoload_classmap.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -315,6 +315,7 @@
315315
'OCA\\DAV\\Migration\\Version1017Date20210216083742' => $baseDir . '/../lib/Migration/Version1017Date20210216083742.php',
316316
'OCA\\DAV\\Migration\\Version1018Date20210312100735' => $baseDir . '/../lib/Migration/Version1018Date20210312100735.php',
317317
'OCA\\DAV\\Migration\\Version1024Date20211221144219' => $baseDir . '/../lib/Migration/Version1024Date20211221144219.php',
318+
'OCA\\DAV\\Migration\\Version1025Date20240308063933' => $baseDir . '/../lib/Migration/Version1025Date20240308063933.php',
318319
'OCA\\DAV\\Migration\\Version1027Date20230504122946' => $baseDir . '/../lib/Migration/Version1027Date20230504122946.php',
319320
'OCA\\DAV\\Migration\\Version1029Date20221114151721' => $baseDir . '/../lib/Migration/Version1029Date20221114151721.php',
320321
'OCA\\DAV\\Migration\\Version1029Date20231004091403' => $baseDir . '/../lib/Migration/Version1029Date20231004091403.php',

apps/dav/composer/composer/autoload_static.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -330,6 +330,7 @@ class ComposerStaticInitDAV
330330
'OCA\\DAV\\Migration\\Version1017Date20210216083742' => __DIR__ . '/..' . '/../lib/Migration/Version1017Date20210216083742.php',
331331
'OCA\\DAV\\Migration\\Version1018Date20210312100735' => __DIR__ . '/..' . '/../lib/Migration/Version1018Date20210312100735.php',
332332
'OCA\\DAV\\Migration\\Version1024Date20211221144219' => __DIR__ . '/..' . '/../lib/Migration/Version1024Date20211221144219.php',
333+
'OCA\\DAV\\Migration\\Version1025Date20240308063933' => __DIR__ . '/..' . '/../lib/Migration/Version1025Date20240308063933.php',
333334
'OCA\\DAV\\Migration\\Version1027Date20230504122946' => __DIR__ . '/..' . '/../lib/Migration/Version1027Date20230504122946.php',
334335
'OCA\\DAV\\Migration\\Version1029Date20221114151721' => __DIR__ . '/..' . '/../lib/Migration/Version1029Date20221114151721.php',
335336
'OCA\\DAV\\Migration\\Version1029Date20231004091403' => __DIR__ . '/..' . '/../lib/Migration/Version1029Date20231004091403.php',

apps/dav/lib/BackgroundJob/PruneOutdatedSyncTokensJob.php

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,9 +52,10 @@ public function __construct(ITimeFactory $timeFactory, CalDavBackend $calDavBack
5252

5353
public function run($argument) {
5454
$limit = max(1, (int) $this->config->getAppValue(Application::APP_ID, 'totalNumberOfSyncTokensToKeep', '10000'));
55+
$retention = max(7, (int) $this->config->getAppValue(Application::APP_ID, 'syncTokensRetentionDays', '60')) * 24 * 3600;
5556

56-
$prunedCalendarSyncTokens = $this->calDavBackend->pruneOutdatedSyncTokens($limit);
57-
$prunedAddressBookSyncTokens = $this->cardDavBackend->pruneOutdatedSyncTokens($limit);
57+
$prunedCalendarSyncTokens = $this->calDavBackend->pruneOutdatedSyncTokens($limit, $retention);
58+
$prunedAddressBookSyncTokens = $this->cardDavBackend->pruneOutdatedSyncTokens($limit, $retention);
5859

5960
$this->logger->info('Pruned {calendarSyncTokensNumber} calendar sync tokens and {addressBooksSyncTokensNumber} address book sync tokens', [
6061
'calendarSyncTokensNumber' => $prunedCalendarSyncTokens,

apps/dav/lib/CalDAV/CalDavBackend.php

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2785,6 +2785,7 @@ protected function addChanges(int $calendarId, array $objectUris, int $operation
27852785
'calendarid' => $query->createNamedParameter($calendarId),
27862786
'operation' => $query->createNamedParameter($operation),
27872787
'calendartype' => $query->createNamedParameter($calendarType),
2788+
'created_at' => time(),
27882789
]);
27892790
foreach ($objectUris as $uri) {
27902791
$query->setParameter('uri', $uri);
@@ -3259,7 +3260,7 @@ protected function getCalendarObjectId($calendarId, $uri, $calendarType):int {
32593260
/**
32603261
* @throws \InvalidArgumentException
32613262
*/
3262-
public function pruneOutdatedSyncTokens(int $keep = 10_000): int {
3263+
public function pruneOutdatedSyncTokens(int $keep, int $retention): int {
32633264
if ($keep < 0) {
32643265
throw new \InvalidArgumentException();
32653266
}
@@ -3277,7 +3278,10 @@ public function pruneOutdatedSyncTokens(int $keep = 10_000): int {
32773278

32783279
$query = $this->db->getQueryBuilder();
32793280
$query->delete('calendarchanges')
3280-
->where($query->expr()->lte('id', $query->createNamedParameter($maxId - $keep, IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT));
3281+
->where(
3282+
$query->expr()->lte('id', $query->createNamedParameter($maxId - $keep, IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT),
3283+
$query->expr()->lte('created_at', $query->createNamedParameter($retention)),
3284+
);
32813285
return $query->executeStatement();
32823286
}
32833287

apps/dav/lib/CardDAV/CardDavBackend.php

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -983,6 +983,7 @@ protected function addChange(int $addressBookId, string $objectUri, int $operati
983983
'synctoken' => $query->createNamedParameter($syncToken),
984984
'addressbookid' => $query->createNamedParameter($addressBookId),
985985
'operation' => $query->createNamedParameter($operation),
986+
'created_at' => time(),
986987
])
987988
->executeStatement();
988989

@@ -1415,7 +1416,7 @@ public function applyShareAcl(int $addressBookId, array $acl): array {
14151416
/**
14161417
* @throws \InvalidArgumentException
14171418
*/
1418-
public function pruneOutdatedSyncTokens(int $keep = 10_000): int {
1419+
public function pruneOutdatedSyncTokens(int $keep, int $retention): int {
14191420
if ($keep < 0) {
14201421
throw new \InvalidArgumentException();
14211422
}
@@ -1433,7 +1434,10 @@ public function pruneOutdatedSyncTokens(int $keep = 10_000): int {
14331434

14341435
$query = $this->db->getQueryBuilder();
14351436
$query->delete('addressbookchanges')
1436-
->where($query->expr()->lte('id', $query->createNamedParameter($maxId - $keep, IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT));
1437+
->where(
1438+
$query->expr()->lte('id', $query->createNamedParameter($maxId - $keep, IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT),
1439+
$query->expr()->lte('created_at', $query->createNamedParameter($retention)),
1440+
);
14371441
return $query->executeStatement();
14381442
}
14391443

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* @copyright Copyright (c) 2024 Christoph Wurst <christoph@winzerhof-wurst.at>
7+
*
8+
* @author Christoph Wurst <christoph@winzerhof-wurst.at>
9+
*
10+
* @license GNU AGPL version 3 or any later version
11+
*
12+
* This program is free software: you can redistribute it and/or modify
13+
* it under the terms of the GNU Affero General Public License as
14+
* published by the Free Software Foundation, either version 3 of the
15+
* License, or (at your option) any later version.
16+
*
17+
* This program is distributed in the hope that it will be useful,
18+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
19+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
20+
* GNU Affero General Public License for more details.
21+
*
22+
* You should have received a copy of the GNU Affero General Public License
23+
* along with this program. If not, see <http://www.gnu.org/licenses/>.
24+
*
25+
*/
26+
27+
namespace OCA\DAV\Migration;
28+
29+
use Closure;
30+
use OCP\DB\ISchemaWrapper;
31+
use OCP\DB\QueryBuilder\IQueryBuilder;
32+
use OCP\DB\Types;
33+
use OCP\IDBConnection;
34+
use OCP\Migration\IOutput;
35+
use OCP\Migration\SimpleMigrationStep;
36+
37+
class Version1025Date20240308063933 extends SimpleMigrationStep {
38+
39+
private IDBConnection $db;
40+
41+
public function __construct(IDBConnection $db) {
42+
$this->db = $db;
43+
}
44+
45+
/**
46+
* @param IOutput $output
47+
* @param Closure(): ISchemaWrapper $schemaClosure
48+
* @param array $options
49+
* @return null|ISchemaWrapper
50+
*/
51+
public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper {
52+
/** @var ISchemaWrapper $schema */
53+
$schema = $schemaClosure();
54+
55+
foreach (['addressbookchanges', 'calendarchanges'] as $tableName) {
56+
$table = $schema->getTable($tableName);
57+
if (!$table->hasColumn('created_at')) {
58+
$table->addColumn('created_at', Types::INTEGER, [
59+
'notnull' => true,
60+
'length' => 4,
61+
'default' => 0,
62+
]);
63+
}
64+
}
65+
66+
return $schema;
67+
}
68+
69+
public function postSchemaChange(IOutput $output, \Closure $schemaClosure, array $options): void {
70+
foreach (['addressbookchanges', 'calendarchanges'] as $tableName) {
71+
$qb = $this->db->getQueryBuilder();
72+
73+
$update = $qb->update($tableName)
74+
->set('created_at', $qb->createNamedParameter(time(), IQueryBuilder::PARAM_INT))
75+
->where(
76+
$qb->expr()->eq('created_at', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT)),
77+
);
78+
79+
$updated = $update->executeStatement();
80+
$output->debug('Added a default creation timestamp to ' . $updated . ' rows in ' . $tableName);
81+
}
82+
}
83+
84+
}

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

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
*/
3030
namespace OCA\DAV\Tests\unit\BackgroundJob;
3131

32+
use InvalidArgumentException;
3233
use OCA\DAV\AppInfo\Application;
3334
use OCA\DAV\BackgroundJob\PruneOutdatedSyncTokensJob;
3435
use OCA\DAV\CalDAV\CalDavBackend;
@@ -72,18 +73,27 @@ protected function setUp(): void {
7273
/**
7374
* @dataProvider dataForTestRun
7475
*/
75-
public function testRun(string $configValue, int $actualLimit, int $deletedCalendarSyncTokens, int $deletedAddressBookSyncTokens): void {
76-
$this->config->expects($this->once())
76+
public function testRun(string $configToKeep, string $configRetentionDays, int $actualLimit, int $retentionDays, int $deletedCalendarSyncTokens, int $deletedAddressBookSyncTokens): void {
77+
$this->config->expects($this->exactly(2))
7778
->method('getAppValue')
78-
->with(Application::APP_ID, 'totalNumberOfSyncTokensToKeep', '10000')
79-
->willReturn($configValue);
79+
->with(Application::APP_ID, self::anything(), self::anything())
80+
->willReturnCallback(function ($app, $key) use ($configToKeep, $configRetentionDays) {
81+
switch ($key) {
82+
case 'totalNumberOfSyncTokensToKeep':
83+
return $configToKeep;
84+
case 'syncTokensRetentionDays':
85+
return $configRetentionDays;
86+
default:
87+
throw new InvalidArgumentException();
88+
}
89+
});
8090
$this->calDavBackend->expects($this->once())
8191
->method('pruneOutdatedSyncTokens')
8292
->with($actualLimit)
8393
->willReturn($deletedCalendarSyncTokens);
8494
$this->cardDavBackend->expects($this->once())
8595
->method('pruneOutdatedSyncTokens')
86-
->with($actualLimit)
96+
->with($actualLimit, $retentionDays)
8797
->willReturn($deletedAddressBookSyncTokens);
8898
$this->logger->expects($this->once())
8999
->method('info')
@@ -97,8 +107,9 @@ public function testRun(string $configValue, int $actualLimit, int $deletedCalen
97107

98108
public function dataForTestRun(): array {
99109
return [
100-
['100', 100, 2, 3],
101-
['0', 1, 0, 0]
110+
['100', '2', 100, 7 * 24 * 3600, 2, 3],
111+
['100', '14', 100, 14 * 24 * 3600, 2, 3],
112+
['0', '60', 1, 60 * 24 * 3600, 0, 0]
102113
];
103114
}
104115
}

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

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@
4646
use Sabre\DAV\PropPatch;
4747
use Sabre\DAV\Xml\Property\Href;
4848
use Sabre\DAVACL\IACL;
49+
use function time;
4950

5051
/**
5152
* Class CalDavBackendTest
@@ -1357,7 +1358,12 @@ public function testPruneOutdatedSyncTokens(): void {
13571358
END:VCALENDAR
13581359
EOD;
13591360
$this->backend->updateCalendarObject($calendarId, $uri, $calData);
1360-
$deleted = $this->backend->pruneOutdatedSyncTokens(0);
1361+
1362+
// Keep everything
1363+
$deleted = $this->backend->pruneOutdatedSyncTokens(0, 0);
1364+
self::assertSame(0, $deleted);
1365+
1366+
$deleted = $this->backend->pruneOutdatedSyncTokens(0, time());
13611367
// At least one from the object creation and one from the object update
13621368
$this->assertGreaterThanOrEqual(2, $deleted);
13631369
$changes = $this->backend->getChangesForCalendar($calendarId, $syncToken, 1);
@@ -1423,7 +1429,7 @@ public function testPruneOutdatedSyncTokens(): void {
14231429
$this->assertEmpty($changes['deleted']);
14241430

14251431
// Delete all but last change
1426-
$deleted = $this->backend->pruneOutdatedSyncTokens(1);
1432+
$deleted = $this->backend->pruneOutdatedSyncTokens(1, time());
14271433
$this->assertEquals(1, $deleted); // We had two changes before, now one
14281434

14291435
// Only update should remain
@@ -1433,7 +1439,8 @@ public function testPruneOutdatedSyncTokens(): void {
14331439
$this->assertEmpty($changes['deleted']);
14341440

14351441
// Check that no crash occurs when prune is called without current changes
1436-
$deleted = $this->backend->pruneOutdatedSyncTokens(1);
1442+
$deleted = $this->backend->pruneOutdatedSyncTokens(1, time());
1443+
self::assertSame(0, $deleted);
14371444
}
14381445

14391446
public function testSearchAndExpandRecurrences() {

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

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@
6060
use Sabre\VObject\Component\VCard;
6161
use Sabre\VObject\Property\Text;
6262
use Test\TestCase;
63+
use function time;
6364

6465
/**
6566
* Class CardDavBackendTest
@@ -880,7 +881,12 @@ public function testPruneOutdatedSyncTokens(): void {
880881
$uri = $this->getUniqueID('card');
881882
$this->backend->createCard($addressBookId, $uri, $this->vcardTest0);
882883
$this->backend->updateCard($addressBookId, $uri, $this->vcardTest1);
883-
$deleted = $this->backend->pruneOutdatedSyncTokens(0);
884+
885+
// Do not delete anything if week data as old as ts=0
886+
$deleted = $this->backend->pruneOutdatedSyncTokens(0, 0);
887+
self::assertSame(0, $deleted);
888+
889+
$deleted = $this->backend->pruneOutdatedSyncTokens(0, time());
884890
// At least one from the object creation and one from the object update
885891
$this->assertGreaterThanOrEqual(2, $deleted);
886892
$changes = $this->backend->getChangesForAddressBook($addressBookId, $syncToken, 1);
@@ -912,16 +918,16 @@ public function testPruneOutdatedSyncTokens(): void {
912918
$this->assertEmpty($changes['deleted']);
913919

914920
// Delete all but last change
915-
$deleted = $this->backend->pruneOutdatedSyncTokens(1);
921+
$deleted = $this->backend->pruneOutdatedSyncTokens(1, time());
916922
$this->assertEquals(1, $deleted); // We had two changes before, now one
917923

918924
// Only update should remain
919925
$changes = $this->backend->getChangesForAddressBook($addressBookId, $syncToken, 100);
920926
$this->assertEmpty($changes['added']);
921927
$this->assertEquals(1, count($changes['modified']));
922928
$this->assertEmpty($changes['deleted']);
923-
929+
924930
// Check that no crash occurs when prune is called without current changes
925-
$deleted = $this->backend->pruneOutdatedSyncTokens(1);
931+
$deleted = $this->backend->pruneOutdatedSyncTokens(1, time());
926932
}
927933
}

0 commit comments

Comments
 (0)