fix(carddav): return correct sync token for non-truncated requests

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
pull/54276/head
Daniel Kesselberg 2025-06-17 19:32:00 +07:00 committed by Hamza
parent f5ddac141e
commit 481aaf11fc
6 changed files with 44 additions and 17 deletions

@ -56,6 +56,8 @@ $cardDavBackend = new CardDavBackend(
\OC::$server->getUserManager(),
\OC::$server->get(IEventDispatcher::class),
\OC::$server->get(\OCA\DAV\CardDAV\Sharing\Backend::class),
\OC::$server->get(OCP\IConfig::class),
);
$debugging = \OC::$server->getConfig()->getSystemValue('debug', false);

@ -920,18 +920,20 @@ class CardDavBackend implements BackendInterface, SyncSupport {
// Fetching all changes
$stmt = $qb->executeQuery();
$rowCount = $stmt->rowCount();
$changes = [];
$highestSyncToken = 0;
// 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'];
// get the last synctoken, needed in case a limit was set
$result['syncToken'] = $row['synctoken'];
$highestSyncToken = $row['synctoken'];
}
$stmt->closeCursor();
// No changes found, use current token
if (empty($changes)) {
$result['syncToken'] = $currentToken;
@ -950,6 +952,20 @@ class CardDavBackend implements BackendInterface, SyncSupport {
break;
}
}
/*
* The synctoken in oc_addressbooks is always the highest synctoken in oc_addressbookchanges for a given addressbook plus one (see addChange).
*
* For truncated results, it is expected that we return the highest token from the response, so the client can continue from the latest change.
*
* For non-truncated results, it is expected to return the currentToken. If we return the highest token, as with truncated results, the client will always think it is one change behind.
*
* Therefore, we differentiate between truncated and non-truncated results when returning the synctoken.
*/
if ($rowCount === $limit && $highestSyncToken < $currentToken) {
$result['syncToken'] = $highestSyncToken;
$result['result_truncated'] = true;
}
} else {
$qb = $this->db->getQueryBuilder();
$qb->select('id', 'uri')

@ -56,6 +56,9 @@ class CardDavBackendTest extends TestCase {
/** @var IGroupManager|\PHPUnit\Framework\MockObject\MockObject */
private $groupManager;
/** @var IConfig|MockObject */
private $config;
/** @var IEventDispatcher|MockObject */
private $dispatcher;
private Backend $sharingBackend;
@ -239,7 +242,7 @@ class CardDavBackendTest extends TestCase {
public function testCardOperations(): void {
/** @var CardDavBackend | \PHPUnit\Framework\MockObject\MockObject $backend */
$backend = $this->getMockBuilder(CardDavBackend::class)
->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend])
->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend, $this->config])
->onlyMethods(['updateProperties', 'purgeProperties'])->getMock();
// create a new address book
@ -294,7 +297,7 @@ class CardDavBackendTest extends TestCase {
public function testMultiCard(): void {
$this->backend = $this->getMockBuilder(CardDavBackend::class)
->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend])
->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend, $this->config])
->setMethods(['updateProperties'])->getMock();
// create a new address book
@ -347,7 +350,7 @@ class CardDavBackendTest extends TestCase {
public function testMultipleUIDOnDifferentAddressbooks(): void {
$this->backend = $this->getMockBuilder(CardDavBackend::class)
->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend])
->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend, $this->config])
->onlyMethods(['updateProperties'])->getMock();
// create 2 new address books
@ -369,7 +372,7 @@ class CardDavBackendTest extends TestCase {
public function testMultipleUIDDenied(): void {
$this->backend = $this->getMockBuilder(CardDavBackend::class)
->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend])
->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend, $this->config])
->setMethods(['updateProperties'])->getMock();
// create a new address book
@ -390,7 +393,7 @@ class CardDavBackendTest extends TestCase {
public function testNoValidUID(): void {
$this->backend = $this->getMockBuilder(CardDavBackend::class)
->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend])
->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend, $this->config])
->setMethods(['updateProperties'])->getMock();
// create a new address book
@ -447,7 +450,7 @@ class CardDavBackendTest extends TestCase {
public function testSyncSupport(): void {
$this->backend = $this->getMockBuilder(CardDavBackend::class)
->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend])
->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend, $this->config])
->setMethods(['updateProperties'])->getMock();
// create a new address book

@ -104,7 +104,7 @@ class SyncServiceTest extends TestCase {
'1',
'principals/system/system',
[]
)['token'];
)[0];
$this->assertEquals('http://sabre.io/ns/sync/1', $token);
}
@ -175,7 +175,7 @@ END:VCARD';
'1',
'principals/system/system',
[]
)['token'];
)[0];
$this->assertEquals('http://sabre.io/ns/sync/2', $token);
}
@ -246,7 +246,7 @@ END:VCARD';
'1',
'principals/system/system',
[]
)['token'];
)[0];
$this->assertEquals('http://sabre.io/ns/sync/3', $token);
}
@ -287,7 +287,7 @@ END:VCARD';
'1',
'principals/system/system',
[]
)['token'];
)[0];
$this->assertEquals('http://sabre.io/ns/sync/4', $token);
}
@ -430,7 +430,7 @@ END:VCARD';
'1',
'principals/system/system',
[]
)['token'];
);
}
/**
@ -472,7 +472,7 @@ END:VCARD';
'1',
'principals/system/system',
[]
)['token'];
);
}
public function providerUseAbsoluteUriReport(): array {

@ -55,7 +55,7 @@ class SyncFederationAddressbooksTest extends \Test\TestCase {
->disableOriginalConstructor()
->getMock();
$syncService->expects($this->once())->method('syncRemoteAddressBook')
->willReturn('1');
->willReturn(['1', false]);
/** @var SyncService $syncService */
$s = new SyncFederationAddressBooks($dbHandler, $syncService, $this->discoveryService, $this->logger);
@ -114,7 +114,7 @@ class SyncFederationAddressbooksTest extends \Test\TestCase {
->disableOriginalConstructor()
->getMock();
$syncService->expects($this->once())->method('syncRemoteAddressBook')
->willReturn('0');
->willReturn(['0', false]);
/** @var SyncService $syncService */
$s = new SyncFederationAddressBooks($dbHandler, $syncService, $this->discoveryService, $this->logger);

@ -346,6 +346,12 @@ $CONFIG = [
*/
'carddav_sync_request_timeout' => 30,
/**
* The limit applied to the synchronization report request, e.g. federated system address books (as run by `occ federation:sync-addressbooks`).
*/
'carddav_sync_request_truncation' => 2500,
/**
* `true` enabled a relaxed session timeout, where the session timeout would no longer be
* handled by Nextcloud but by either the PHP garbage collection or the expiration of