Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cardav): support result truncation for addressbook federation #50092

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions apps/dav/lib/CardDAV/AddressBook.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
namespace OCA\DAV\CardDAV;

use OCA\DAV\DAV\Sharing\IShareable;
use OCA\DAV\Exception\UnsupportedLimitOnInitialSyncException;
use OCP\DB\Exception;
use OCP\IL10N;
use OCP\Server;
Expand Down Expand Up @@ -234,9 +233,6 @@ private function canWrite(): bool {
}

public function getChanges($syncToken, $syncLevel, $limit = null) {
if (!$syncToken && $limit) {
throw new UnsupportedLimitOnInitialSyncException();
}

return parent::getChanges($syncToken, $syncLevel, $limit);
}
Expand Down
60 changes: 55 additions & 5 deletions apps/dav/lib/CardDAV/CardDavBackend.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,11 @@
use OCP\DB\Exception;
use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\EventDispatcher\IEventDispatcher;
use OCP\IConfig;
use OCP\IDBConnection;
use OCP\IUserManager;
use PDO;
use Psr\Log\LoggerInterface;
use Sabre\CardDAV\Backend\BackendInterface;
use Sabre\CardDAV\Backend\SyncSupport;
use Sabre\CardDAV\Plugin;
Expand Down Expand Up @@ -59,6 +61,8 @@ public function __construct(
private IUserManager $userManager,
private IEventDispatcher $dispatcher,
private Sharing\Backend $sharingBackend,
private LoggerInterface $logger,
private IConfig $config,
) {
}

Expand Down Expand Up @@ -851,6 +855,10 @@ public function deleteCard($addressBookId, $cardUri) {
* @return array
*/
public function getChangesForAddressBook($addressBookId, $syncToken, $syncLevel, $limit = null) {
if ($limit === null) {
$limit = $this->config->getSystemValueInt('carddav_sync_request_limit', 1);
Copy link
Member

@st3iny st3iny Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Debugging leftover has to be removed (default limit 1) before the merge.


}
// Current synctoken
return $this->atomic(function () use ($addressBookId, $syncToken, $syncLevel, $limit) {
$qb = $this->db->getQueryBuilder();
Expand All @@ -873,8 +881,34 @@ public function getChangesForAddressBook($addressBookId, $syncToken, $syncLevel,
'modified' => [],
'deleted' => [],
];

if ($syncToken) {
$this->logger->error('getChangesForAddressBook', ['syncToken' => $syncToken, 'currentToken' => $currentToken, 'limit' => $limit]);
if (str_starts_with($syncToken, 'init_')) {
$syncValues = explode('_', $syncToken);
$lastID = $syncValues[1];
$initialSyncToken = $syncValues[2];
$qb = $this->db->getQueryBuilder();
$qb->select('id', 'uri')
->from('cards')
->where(
$qb->expr()->andX(
$qb->expr()->eq('addressbookid', $qb->createNamedParameter($addressBookId)),
$qb->expr()->gt('id', $qb->createNamedParameter($lastID)))
)->orderBy('id')
->setMaxResults($limit);
$stmt = $qb->executeQuery();
$values = $stmt->fetchAll(\PDO::FETCH_ASSOC);
$stmt->closeCursor();
if (count($values) === 0) {
$result['syncToken'] = $initialSyncToken;
$result['result_truncated'] = false;
$result['added'] = [];
} else {
$lastID = end($values)['id'];
$result['added'] = array_column($values, 'uri');
$result['syncToken'] = count($result['added']) === $limit ? "init_{$lastID}_$initialSyncToken" : $initialSyncToken ;
$result['result_truncated'] = count($result['added']) === $limit;
}
} elseif ($syncToken) {
$qb = $this->db->getQueryBuilder();
$qb->select('uri', 'operation')
->from('addressbookchanges')
Expand All @@ -899,6 +933,8 @@ public function getChangesForAddressBook($addressBookId, $syncToken, $syncLevel,
// 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'];
}
$stmt->closeCursor();

Expand All @@ -917,14 +953,28 @@ public function getChangesForAddressBook($addressBookId, $syncToken, $syncLevel,
}
} else {
$qb = $this->db->getQueryBuilder();
$qb->select('uri')
$qb->select('id', 'uri')
->from('cards')
->where(
$qb->expr()->eq('addressbookid', $qb->createNamedParameter($addressBookId))
);
// No synctoken supplied, this is the initial sync.
$stmt = $qb->executeQuery();
$result['added'] = $stmt->fetchAll(\PDO::FETCH_COLUMN);
if (is_int($limit) && $limit > 0) {
$qb->setMaxResults($limit);
$stmt = $qb->executeQuery();
$values = $stmt->fetchAll(\PDO::FETCH_ASSOC);
$lastID = end($values)['id'];
if (count(array_values($values)) === $limit) {
$result['syncToken'] = 'init_' . $lastID . '_' . $currentToken;
$result['result_truncated'] = true;
}
} else {
$stmt = $qb->executeQuery();
$values = $stmt->fetchAll(\PDO::FETCH_ASSOC);
$this->logger->error('getChangesForAddressBook', ['values' => $values]);
}
$result['added'] = array_column($values, 'uri');

$stmt->closeCursor();
}
return $result;
Expand Down
1 change: 1 addition & 0 deletions apps/dav/lib/CardDAV/SyncService.php
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ public function syncRemoteAddressBook(string $url, string $userName, string $add
$this->logger->error('Client exception:', ['app' => 'dav', 'exception' => $ex]);
throw $ex;
}
$this->logger->error('sync response', [$response]);

// 3. apply changes
// TODO: use multi-get for download
Expand Down
8 changes: 0 additions & 8 deletions apps/dav/lib/CardDAV/SystemAddressbook.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
*/
namespace OCA\DAV\CardDAV;

use OCA\DAV\Exception\UnsupportedLimitOnInitialSyncException;
use OCA\Federation\TrustedServers;
use OCP\Accounts\IAccountManager;
use OCP\IConfig;
Expand Down Expand Up @@ -212,14 +211,7 @@ public function getChild($name): Card {
}
return new Card($this->carddavBackend, $this->addressBookInfo, $obj);
}

/**
* @throws UnsupportedLimitOnInitialSyncException
*/
public function getChanges($syncToken, $syncLevel, $limit = null) {
if (!$syncToken && $limit) {
throw new UnsupportedLimitOnInitialSyncException();
}

if (!$this->carddavBackend instanceof SyncSupport) {
return null;
Expand Down
6 changes: 6 additions & 0 deletions apps/dav/lib/RootCollection.php
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,8 @@ public function __construct() {
);

$contactsSharingBackend = \OC::$server->get(\OCA\DAV\CardDAV\Sharing\Backend::class);
$logger = \OC::$server->get(\Psr\Log\LoggerInterface::class);
$config = \OC::$server->get(IConfig::class);

$pluginManager = new PluginManager(\OC::$server, \OC::$server->query(IAppManager::class));
$usersCardDavBackend = new CardDavBackend(
Expand All @@ -132,6 +134,8 @@ public function __construct() {
$userManager,
$dispatcher,
$contactsSharingBackend,
$logger,
$config
);
$usersAddressBookRoot = new AddressBookRoot($userPrincipalBackend, $usersCardDavBackend, $pluginManager, $userSession->getUser(), $groupManager, 'principals/users');
$usersAddressBookRoot->disableListing = $disableListing;
Expand All @@ -142,6 +146,8 @@ public function __construct() {
$userManager,
$dispatcher,
$contactsSharingBackend,
$logger,
$config
);
$systemAddressBookRoot = new AddressBookRoot(new SystemPrincipalBackend(), $systemCardDavBackend, $pluginManager, $userSession->getUser(), $groupManager, 'principals/system');
$systemAddressBookRoot->disableListing = $disableListing;
Expand Down
3 changes: 2 additions & 1 deletion apps/dav/lib/Server.php
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,8 @@

$this->server->addPlugin(new ExceptionLoggerPlugin('webdav', $logger));
$this->server->addPlugin(new LockPlugin());
$this->server->addPlugin(new \Sabre\DAV\Sync\Plugin());
$logger = \OC::$server->get(LoggerInterface::class);
$this->server->addPlugin(new \Sabre\DAV\Sync\Plugin($logger));

Check failure on line 164 in apps/dav/lib/Server.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

TooManyArguments

apps/dav/lib/Server.php:164:28: TooManyArguments: Class Sabre\DAV\Sync\Plugin has no __construct, but arguments were passed (see https://psalm.dev/026)

// acl
$acl = new DavAclPlugin();
Expand Down
12 changes: 12 additions & 0 deletions apps/federation/lib/SyncFederationAddressBooks.php
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@
try {
$newToken = $this->syncService->syncRemoteAddressBook($url, $cardDavUser, $addressBookUrl, $sharedSecret, $syncToken, $targetBookId, $targetPrincipal, $targetBookProperties);
if ($newToken !== $syncToken) {
// Finish truncated initial sync.
if (strpos($newToken, 'init') !== false) {
hamza221 marked this conversation as resolved.
Show resolved Hide resolved
$newToken = $this->syncTruncatedAddressBook($url, $cardDavUser, $addressBookUrl, $sharedSecret, $newToken, $targetBookId, $targetPrincipal, $targetBookProperties);

Check failure on line 57 in apps/federation/lib/SyncFederationAddressBooks.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

InvalidScalarArgument

apps/federation/lib/SyncFederationAddressBooks.php:57:114: InvalidScalarArgument: Argument 6 of OCA\Federation\SyncFederationAddressBooks::syncTruncatedAddressBook expects int, but string provided (see https://psalm.dev/012)
}
$this->dbHandler->setServerStatus($url, TrustedServers::STATUS_OK, $newToken);
} else {
$this->logger->debug("Sync Token for $url unchanged from previous sync");
Expand All @@ -76,4 +80,12 @@
}
}
}

private function syncTruncatedAddressBook(string $url, string $cardDavUser, string $addressBookUrl, string $sharedSecret, string $syncToken, int $targetBookId, string $targetPrincipal, array $targetBookProperties): string {
$newToken = $this->syncService->syncRemoteAddressBook($url, $cardDavUser, $addressBookUrl, $sharedSecret, $syncToken, $targetBookId, $targetPrincipal, $targetBookProperties);

Check failure on line 85 in apps/federation/lib/SyncFederationAddressBooks.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

InvalidScalarArgument

apps/federation/lib/SyncFederationAddressBooks.php:85:121: InvalidScalarArgument: Argument 6 of OCA\DAV\CardDAV\SyncService::syncRemoteAddressBook expects string, but int provided (see https://psalm.dev/012)
while (strpos($newToken, 'init') !== false) {
$newToken = $this->syncService->syncRemoteAddressBook($url, $cardDavUser, $addressBookUrl, $sharedSecret, $syncToken, $targetBookId, $targetPrincipal, $targetBookProperties);

Check failure on line 87 in apps/federation/lib/SyncFederationAddressBooks.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

InvalidScalarArgument

apps/federation/lib/SyncFederationAddressBooks.php:87:122: InvalidScalarArgument: Argument 6 of OCA\DAV\CardDAV\SyncService::syncRemoteAddressBook expects string, but int provided (see https://psalm.dev/012)
}
return $newToken;
}
}
5 changes: 5 additions & 0 deletions config/config.sample.php
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,11 @@
*/
'carddav_sync_request_timeout' => 30,

/**
* The limit applied to the initial synchronization report request, e.g. federated system address books (as run by `occ federation:sync-addressbooks`).
*/
'carddav_initial_sync_request_limit' => 1000,

/**
* `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
Expand Down
8 changes: 8 additions & 0 deletions lib/public/Http/Client/IClient.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,14 @@ interface IClient {
*/
public const DEFAULT_REQUEST_TIMEOUT = 30;

/**
* Default limit for address book intial sync
*
* @since 31.0.0
*/

public const DEFAULT_ADDRESSBOOK_INITIAL_SYNC_LIMIT = 1;

/**
* Sends a GET request
* @param string $uri
Expand Down
Loading