Add logging to federation sync

Signed-off-by: Anna Larch <anna@nextcloud.com>

diff --git a/apps/dav/lib/CardDAV/SyncService.php b/apps/dav/lib/CardDAV/SyncService.php
index 73bfaf01b604e253f53069d93a8630df9610ffa4..021d6abea8bcc994f12dbcd2c65352fc6c6498a9 100644 (file)
--- a/apps/dav/lib/CardDAV/SyncService.php
+++ b/apps/dav/lib/CardDAV/SyncService.php
@@ -99,9 +99,11 @@ class SyncService {
                        if ($ex->getCode() === Http::STATUS_UNAUTHORIZED) {
                                // remote server revoked access to the address book, remove it
                                $this->backend->deleteAddressBook($addressBookId);
-                               $this->logger->info('Authorization failed, remove address book: ' . $url, ['app' => 'dav']);
-                               throw $ex;
+                               $this->logger->error('Authorization failed, remove address book: ' . $url, ['app' => 'dav']);
+                       } else {
+                               $this->logger->error('Client exception:', ['app' => 'dav', 'exception' => $ex]);
                        }
+                       throw $ex;
                }
 
                // 3. apply changes

diff --git a/apps/federation/lib/SyncFederationAddressBooks.php b/apps/federation/lib/SyncFederationAddressBooks.php
index ace5c07065a57a4df1bc0e83fe3a7ec3b5c5f011..f1803ad78648c528fe05f9418ed27c7872886d50 100644 (file)
--- a/apps/federation/lib/SyncFederationAddressBooks.php
+++ b/apps/federation/lib/SyncFederationAddressBooks.php
@@ -29,6 +29,7 @@ use OC\OCS\DiscoveryService;
 use OCA\DAV\CardDAV\SyncService;
 use OCP\AppFramework\Http;
 use OCP\OCS\IDiscoveryService;
+use Psr\Log\LoggerInterface;
 
 class SyncFederationAddressBooks {
 
@@ -41,18 +42,18 @@ class SyncFederationAddressBooks {
        /** @var  DiscoveryService */
        private $ocsDiscoveryService;
 
-       /**
-        * @param DbHandler $dbHandler
-        * @param SyncService $syncService
-        * @param IDiscoveryService $ocsDiscoveryService
-        */
+       /** @var LoggerInterface */
+       private $logger;
+
        public function __construct(DbHandler $dbHandler,
                                                                SyncService $syncService,
-                                                               IDiscoveryService $ocsDiscoveryService
+                                                               IDiscoveryService $ocsDiscoveryService,
+                                                               LoggerInterface $logger
        ) {
                $this->syncService = $syncService;
                $this->dbHandler = $dbHandler;
                $this->ocsDiscoveryService = $ocsDiscoveryService;
+               $this->logger = $logger;
        }
 
        /**
@@ -71,6 +72,7 @@ class SyncFederationAddressBooks {
                        $addressBookUrl = isset($endPoints['system-address-book']) ? trim($endPoints['system-address-book'], '/') : 'remote.php/dav/addressbooks/system/system/system';
 
                        if (is_null($sharedSecret)) {
+                               $this->logger->error('Shared secret is null');
                                continue;
                        }
                        $targetBookId = $trustedServer['url_hash'];
@@ -82,10 +84,16 @@ class SyncFederationAddressBooks {
                                $newToken = $this->syncService->syncRemoteAddressBook($url, $cardDavUser, $addressBookUrl, $sharedSecret, $syncToken, $targetBookId, $targetPrincipal, $targetBookProperties);
                                if ($newToken !== $syncToken) {
                                        $this->dbHandler->setServerStatus($url, TrustedServers::STATUS_OK, $newToken);
+                               } else {
+                                       $this->logger->info('Token unchanged from previous sync.');
                                }
                        } catch (\Exception $ex) {
                                if ($ex->getCode() === Http::STATUS_UNAUTHORIZED) {
                                        $this->dbHandler->setServerStatus($url, TrustedServers::STATUS_ACCESS_REVOKED);
+                                       $this->logger->error('Server sync failed because of revoked access.');
+                               } else {
+                                       $this->dbHandler->setServerStatus($url, TrustedServers::STATUS_FAILURE);
+                                       $this->logger->error('Server sync failed.');
                                }
                                $callback($url, $ex);
                        }

diff --git a/apps/federation/lib/SyncJob.php b/apps/federation/lib/SyncJob.php
index f16d08a80d81c1432e5fe3f9598c66ba1d53ec18..bbf346f3192da43a70851e02056293161b157e15 100644 (file)
--- a/apps/federation/lib/SyncJob.php
+++ b/apps/federation/lib/SyncJob.php
@@ -26,21 +26,20 @@
 namespace OCA\Federation;
 
 use OC\BackgroundJob\TimedJob;
-use OCP\ILogger;
+use Psr\Log\LoggerInterface;
 
 class SyncJob extends TimedJob {
 
        /** @var SyncFederationAddressBooks */
        protected $syncService;
 
-       /** @var ILogger */
+       /** @var LoggerInterface */
        protected $logger;
 
        /**
         * @param SyncFederationAddressBooks $syncService
-        * @param ILogger $logger
         */
-       public function __construct(SyncFederationAddressBooks $syncService, ILogger $logger) {
+       public function __construct(SyncFederationAddressBooks $syncService, LoggerInterface $logger) {
                // Run once a day
                $this->setInterval(24 * 60 * 60);
                $this->syncService = $syncService;
@@ -50,11 +49,11 @@ class SyncJob extends TimedJob {
        protected function run($argument) {
                $this->syncService->syncThemAll(function ($url, $ex) {
                        if ($ex instanceof \Exception) {
-                               $this->logger->logException($ex, [
-                                       'message' => "Error while syncing $url.",
-                                       'level' => ILogger::INFO,
-                                       'app' => 'fed-sync',
-                               ]);
+                               $this->logger->error("Error while syncing $url.",
+                                       [
+                                               'exception' => $ex
+                                       ]
+                               );
                        }
                });
        }

diff --git a/apps/federation/tests/SyncFederationAddressbooksTest.php b/apps/federation/tests/SyncFederationAddressbooksTest.php
index 36dd43e7cd208bebdc8da612d6ca936ac13017de..b39a80d95e4d693d0b5a2de683878621348cf7de 100644 (file)
--- a/apps/federation/tests/SyncFederationAddressbooksTest.php
+++ b/apps/federation/tests/SyncFederationAddressbooksTest.php
@@ -31,25 +31,31 @@ namespace OCA\Federation\Tests;
 use OC\OCS\DiscoveryService;
 use OCA\Federation\DbHandler;
 use OCA\Federation\SyncFederationAddressBooks;
+use PHPUnit\Framework\MockObject\MockObject;
+use Psr\Log\LoggerInterface;
 
 class SyncFederationAddressbooksTest extends \Test\TestCase {
 
        /** @var array */
        private $callBacks = [];
 
-       /** @var  \PHPUnit\Framework\MockObject\MockObject | DiscoveryService */
+       /** @var  MockObject | DiscoveryService */
        private $discoveryService;
 
+       /** @var MockObject|LoggerInterface  */
+       private $logger;
+
        protected function setUp(): void {
                parent::setUp();
 
                $this->discoveryService = $this->getMockBuilder(DiscoveryService::class)
                        ->disableOriginalConstructor()->getMock();
                $this->discoveryService->expects($this->any())->method('discover')->willReturn([]);
+               $this->logger = $this->createMock(LoggerInterface::class);
        }
 
        public function testSync() {
-               /** @var DbHandler | \PHPUnit\Framework\MockObject\MockObject $dbHandler */
+               /** @var DbHandler | MockObject $dbHandler */
                $dbHandler = $this->getMockBuilder('OCA\Federation\DbHandler')->
                        disableOriginalConstructor()->
                        getMock();
@@ -62,8 +68,8 @@ class SyncFederationAddressbooksTest extends \Test\TestCase {
                                        'sync_token' => '0'
                                ]
                        ]);
-               $dbHandler->expects($this->once())->method('setServerStatus')->
-                       with('https://cloud.drop.box', 1, '1');
+               $dbHandler->expects($this->once())->method('setServerStatus')
+                       ->with('https://cloud.drop.box', 1, '1');
                $syncService = $this->getMockBuilder('OCA\DAV\CardDAV\SyncService')
                        ->disableOriginalConstructor()
                        ->getMock();
@@ -71,7 +77,7 @@ class SyncFederationAddressbooksTest extends \Test\TestCase {
                        ->willReturn(1);
 
                /** @var \OCA\DAV\CardDAV\SyncService $syncService */
-               $s = new SyncFederationAddressBooks($dbHandler, $syncService, $this->discoveryService);
+               $s = new SyncFederationAddressBooks($dbHandler, $syncService, $this->discoveryService, $this->logger);
                $s->syncThemAll(function ($url, $ex) {
                        $this->callBacks[] = [$url, $ex];
                });
@@ -79,7 +85,7 @@ class SyncFederationAddressbooksTest extends \Test\TestCase {
        }
 
        public function testException() {
-               /** @var DbHandler | \PHPUnit\Framework\MockObject\MockObject $dbHandler */
+               /** @var DbHandler | MockObject $dbHandler */
                $dbHandler = $this->getMockBuilder('OCA\Federation\DbHandler')->
                disableOriginalConstructor()->
                getMock();
@@ -99,7 +105,7 @@ class SyncFederationAddressbooksTest extends \Test\TestCase {
                        ->willThrowException(new \Exception('something did not work out'));
 
                /** @var \OCA\DAV\CardDAV\SyncService $syncService */
-               $s = new SyncFederationAddressBooks($dbHandler, $syncService, $this->discoveryService);
+               $s = new SyncFederationAddressBooks($dbHandler, $syncService, $this->discoveryService, $this->logger);
                $s->syncThemAll(function ($url, $ex) {
                        $this->callBacks[] = [$url, $ex];
                });