From 6114176b71faa5cbe9b28a307888e2ea0a21dcc4 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Fri, 24 Jun 2022 20:25:38 +0200 Subject: [PATCH] Cleanup CardDav SyncService Signed-off-by: Carl Schwan --- apps/dav/lib/CardDAV/CardDavBackend.php | 8 +- apps/dav/lib/CardDAV/SyncService.php | 81 ++++--------------- apps/federation/lib/DbHandler.php | 4 +- .../BackgroundJob/GetSharedSecretTest.php | 6 +- 4 files changed, 24 insertions(+), 75 deletions(-) diff --git a/apps/dav/lib/CardDAV/CardDavBackend.php b/apps/dav/lib/CardDAV/CardDavBackend.php index 2c99e6084c1..a26307b02a8 100644 --- a/apps/dav/lib/CardDAV/CardDavBackend.php +++ b/apps/dav/lib/CardDAV/CardDavBackend.php @@ -296,18 +296,14 @@ class CardDavBackend implements BackendInterface, SyncSupport { return $addressBook; } - /** - * @param $addressBookUri - * @return array|null - */ - public function getAddressBooksByUri($principal, $addressBookUri) { + public function getAddressBooksByUri(string $principal, string $addressBookUri): ?array { $query = $this->db->getQueryBuilder(); $result = $query->select(['id', 'uri', 'displayname', 'principaluri', 'description', 'synctoken']) ->from('addressbooks') ->where($query->expr()->eq('uri', $query->createNamedParameter($addressBookUri))) ->andWhere($query->expr()->eq('principaluri', $query->createNamedParameter($principal))) ->setMaxResults(1) - ->execute(); + ->executeQuery(); $row = $result->fetch(); $result->closeCursor(); diff --git a/apps/dav/lib/CardDAV/SyncService.php b/apps/dav/lib/CardDAV/SyncService.php index 76377c21969..5094b7f3f5c 100644 --- a/apps/dav/lib/CardDAV/SyncService.php +++ b/apps/dav/lib/CardDAV/SyncService.php @@ -40,27 +40,13 @@ use Sabre\HTTP\ClientHttpException; use Sabre\VObject\Reader; class SyncService { - - /** @var CardDavBackend */ - private $backend; - - /** @var IUserManager */ - private $userManager; - + private CardDavBackend $backend; + private IUserManager $userManager; private LoggerInterface $logger; + private ?array $localSystemAddressBook = null; + private Converter $converter; + protected string $certPath; - /** @var array */ - private $localSystemAddressBook; - - /** @var Converter */ - private $converter; - - /** @var string */ - protected $certPath; - - /** - * SyncService constructor. - */ public function __construct(CardDavBackend $backend, IUserManager $userManager, LoggerInterface $logger, @@ -73,13 +59,11 @@ class SyncService { } /** - * @param array $targetProperties - * @return string * @throws \Exception */ - public function syncRemoteAddressBook(string $url, string $userName, string $addressBookUrl, string $sharedSecret, string $syncToken, string $targetBookId, string $targetPrincipal, array $targetProperties) { + public function syncRemoteAddressBook(string $url, string $userName, string $addressBookUrl, string $sharedSecret, ?string $syncToken, string $targetBookHash, string $targetPrincipal, array $targetProperties): string { // 1. create addressbook - $book = $this->ensureSystemAddressBookExists($targetPrincipal, $targetBookId, $targetProperties); + $book = $this->ensureSystemAddressBookExists($targetPrincipal, $targetBookHash, $targetProperties); $addressBookId = $book['id']; // 2. query changes @@ -115,28 +99,23 @@ class SyncService { } /** - * @param string $principal - * @param string $id - * @param array $properties - * @return array|null * @throws \Sabre\DAV\Exception\BadRequest */ - public function ensureSystemAddressBookExists($principal, $id, $properties) { - $book = $this->backend->getAddressBooksByUri($principal, $id); + public function ensureSystemAddressBookExists(string $principal, string $uri, array $properties): ?array { + $book = $this->backend->getAddressBooksByUri($principal, $uri); if (!is_null($book)) { return $book; } - $this->backend->createAddressBook($principal, $id, $properties); + // FIXME This might break in clustered DB setup + $this->backend->createAddressBook($principal, $uri, $properties); - return $this->backend->getAddressBooksByUri($principal, $id); + return $this->backend->getAddressBooksByUri($principal, $uri); } /** * Check if there is a valid certPath we should use - * - * @return string */ - protected function getCertPath() { + protected function getCertPath(): string { // we already have a valid certPath if ($this->certPath !== '') { @@ -152,14 +131,7 @@ class SyncService { return $this->certPath; } - /** - * @param string $url - * @param string $userName - * @param string $addressBookUrl - * @param string $sharedSecret - * @return Client - */ - protected function getClient($url, $userName, $sharedSecret) { + protected function getClient(string $url, string $userName, string $sharedSecret): Client { $settings = [ 'baseUri' => $url . '/', 'userName' => $userName, @@ -176,15 +148,7 @@ class SyncService { return $client; } - /** - * @param string $url - * @param string $userName - * @param string $addressBookUrl - * @param string $sharedSecret - * @param string $syncToken - * @return array - */ - protected function requestSyncReport($url, $userName, $addressBookUrl, $sharedSecret, $syncToken) { + protected function requestSyncReport(string $url, string $userName, string $addressBookUrl, string $sharedSecret, ?string $syncToken): array { $client = $this->getClient($url, $userName, $sharedSecret); $body = $this->buildSyncCollectionRequestBody($syncToken); @@ -196,23 +160,12 @@ class SyncService { return $this->parseMultiStatus($response['body']); } - /** - * @param string $url - * @param string $userName - * @param string $sharedSecret - * @param string $resourcePath - * @return array - */ - protected function download($url, $userName, $sharedSecret, $resourcePath) { + protected function download(string $url, string $userName, string $sharedSecret, string $resourcePath): array { $client = $this->getClient($url, $userName, $sharedSecret); return $client->request('GET', $resourcePath); } - /** - * @param string|null $syncToken - * @return string - */ - private function buildSyncCollectionRequestBody($syncToken) { + private function buildSyncCollectionRequestBody(?string $syncToken): string { $dom = new \DOMDocument('1.0', 'UTF-8'); $dom->formatOutput = true; $root = $dom->createElementNS('DAV:', 'd:sync-collection'); diff --git a/apps/federation/lib/DbHandler.php b/apps/federation/lib/DbHandler.php index abdabee6a08..3142cfd353d 100644 --- a/apps/federation/lib/DbHandler.php +++ b/apps/federation/lib/DbHandler.php @@ -99,7 +99,7 @@ class DbHandler { /** * Get trusted server with given ID * - * @return array{id: int, url: string, url_hash: string, token: string, shared_secret: string, status: int, sync_token: string} + * @return array{id: int, url: string, url_hash: string, token: ?string, shared_secret: ?string, status: int, sync_token: ?string} * @throws \Exception */ public function getServerById(int $id): array { @@ -122,7 +122,7 @@ class DbHandler { /** * Get all trusted servers * - * @return list + * @return list * @throws DBException */ public function getAllServer(): array { diff --git a/apps/federation/tests/BackgroundJob/GetSharedSecretTest.php b/apps/federation/tests/BackgroundJob/GetSharedSecretTest.php index baefa86aeda..5344736b7f9 100644 --- a/apps/federation/tests/BackgroundJob/GetSharedSecretTest.php +++ b/apps/federation/tests/BackgroundJob/GetSharedSecretTest.php @@ -36,9 +36,9 @@ use OCP\BackgroundJob\IJobList; use OCP\Http\Client\IClient; use OCP\Http\Client\IClientService; use OCP\Http\Client\IResponse; -use OCP\ILogger; use OCP\IURLGenerator; use OCP\OCS\IDiscoveryService; +use Psr\Log\LoggerInterface; /** * Class GetSharedSecretTest @@ -64,7 +64,7 @@ class GetSharedSecretTest extends TestCase { /** @var \PHPUnit\Framework\MockObject\MockObject|TrustedServers */ private $trustedServers; - /** @var \PHPUnit\Framework\MockObject\MockObject|ILogger */ + /** @var \PHPUnit\Framework\MockObject\MockObject|LoggerInterface */ private $logger; /** @var \PHPUnit\Framework\MockObject\MockObject|IResponse */ @@ -88,7 +88,7 @@ class GetSharedSecretTest extends TestCase { $this->urlGenerator = $this->getMockBuilder(IURLGenerator::class)->getMock(); $this->trustedServers = $this->getMockBuilder(TrustedServers::class) ->disableOriginalConstructor()->getMock(); - $this->logger = $this->getMockBuilder(ILogger::class)->getMock(); + $this->logger = $this->getMockBuilder(LoggerInterface::class)->getMock(); $this->response = $this->getMockBuilder(IResponse::class)->getMock(); $this->discoverService = $this->getMockBuilder(IDiscoveryService::class)->getMock(); $this->timeFactory = $this->createMock(ITimeFactory::class); -- 2.39.5