Merge pull request #31622 from nextcloud/increase-loglevel-modernize-refresh-webcal

Increase loglevel of Webcal parsing errors and modernize code

2 files changed, 26 insertions, 45 deletions

diff --git a/apps/dav/lib/CalDAV/WebcalCaching/RefreshWebcalService.php b/apps/dav/lib/CalDAV/WebcalCaching/RefreshWebcalService.php
index 0dbe7398f52..eadeea3457c 100644
--- a/apps/dav/lib/CalDAV/WebcalCaching/RefreshWebcalService.php
+++ b/apps/dav/lib/CalDAV/WebcalCaching/RefreshWebcalService.php
@@ -56,14 +56,15 @@ use function count;
 class RefreshWebcalService {
 
 	/** @var CalDavBackend */
-	private $calDavBackend;
+	private CalDavBackend $calDavBackend;
 
 	/** @var IClientService */
-	private $clientService;
+	private IClientService $clientService;
 
 	/** @var IConfig */
-	private $config;
+	private IConfig $config;
 
+	/** @var LoggerInterface */
 	private LoggerInterface $logger;
 
 	public const REFRESH_RATE = '{http://apple.com/ns/ical/}refreshrate';
@@ -71,13 +72,7 @@ class RefreshWebcalService {
 	public const STRIP_ATTACHMENTS = '{http://calendarserver.org/ns/}subscribed-strip-attachments';
 	public const STRIP_TODOS = '{http://calendarserver.org/ns/}subscribed-strip-todos';
 
-	/**
-	 * RefreshWebcalJob constructor.
-	 */
-	public function __construct(CalDavBackend $calDavBackend,
-								IClientService $clientService,
-								IConfig $config,
-								LoggerInterface $logger) {
+	public function __construct(CalDavBackend $calDavBackend, IClientService $clientService, IConfig $config, LoggerInterface $logger) {
 		$this->calDavBackend = $calDavBackend;
 		$this->clientService = $clientService;
 		$this->config = $config;
@@ -131,12 +126,12 @@ class RefreshWebcalService {
 					continue;
 				}
 
-				$uri = $this->getRandomCalendarObjectUri();
+				$objectUri = $this->getRandomCalendarObjectUri();
 				$calendarData = $vObject->serialize();
 				try {
-					$this->calDavBackend->createCalendarObject($subscription['id'], $uri, $calendarData, CalDavBackend::CALENDAR_TYPE_SUBSCRIPTION);
+					$this->calDavBackend->createCalendarObject($subscription['id'], $objectUri, $calendarData, CalDavBackend::CALENDAR_TYPE_SUBSCRIPTION);
 				} catch (NoInstancesException | BadRequest $ex) {
-					$this->logger->error($ex->getMessage(), ['exception' => $ex]);
+					$this->logger->error('Unable to create calendar object from subscription {subscriptionId}', ['exception' => $ex, 'subscriptionId' => $subscription['id'], 'source' => $subscription['source']]);
 				}
 			}
 
@@ -147,20 +142,14 @@ class RefreshWebcalService {
 
 			$this->updateSubscription($subscription, $mutations);
 		} catch (ParseException $ex) {
-			$subscriptionId = $subscription['id'];
-
-			$this->logger->error("Subscription $subscriptionId could not be refreshed due to a parsing error", ['exception' => $ex]);
+			$this->logger->error("Subscription {subscriptionId} could not be refreshed due to a parsing error", ['exception' => $ex, 'subscriptionId' => $subscription['id']]);
 		}
 	}
 
 	/**
 	 * loads subscription from backend
-	 *
-	 * @param string $principalUri
-	 * @param string $uri
-	 * @return array|null
 	 */
-	public function getSubscription(string $principalUri, string $uri) {
+	public function getSubscription(string $principalUri, string $uri): ?array {
 		$subscriptions = array_values(array_filter(
 			$this->calDavBackend->getSubscriptionsForUser($principalUri),
 			function ($sub) use ($uri) {
@@ -177,12 +166,8 @@ class RefreshWebcalService {
 
 	/**
 	 * gets webcal feed from remote server
-	 *
-	 * @param array $subscription
-	 * @param array &$mutations
-	 * @return null|string
 	 */
-	private function queryWebcalFeed(array $subscription, array &$mutations) {
+	private function queryWebcalFeed(array $subscription, array &$mutations): ?string {
 		$client = $this->clientService->newClient();
 
 		$didBreak301Chain = false;
@@ -244,7 +229,7 @@ class RefreshWebcalService {
 						$jCalendar = Reader::readJson($body, Reader::OPTION_FORGIVING);
 					} catch (Exception $ex) {
 						// In case of a parsing error return null
-						$this->logger->debug("Subscription $subscriptionId could not be parsed");
+						$this->logger->warning("Subscription $subscriptionId could not be parsed", ['exception' => $ex]);
 						return null;
 					}
 					return $jCalendar->serialize();
@@ -254,7 +239,7 @@ class RefreshWebcalService {
 						$xCalendar = Reader::readXML($body);
 					} catch (Exception $ex) {
 						// In case of a parsing error return null
-						$this->logger->debug("Subscription $subscriptionId could not be parsed");
+						$this->logger->warning("Subscription $subscriptionId could not be parsed", ['exception' => $ex]);
 						return null;
 					}
 					return $xCalendar->serialize();
@@ -265,7 +250,7 @@ class RefreshWebcalService {
 						$vCalendar = Reader::read($body);
 					} catch (Exception $ex) {
 						// In case of a parsing error return null
-						$this->logger->debug("Subscription $subscriptionId could not be parsed");
+						$this->logger->warning("Subscription $subscriptionId could not be parsed", ['exception' => $ex]);
 						return null;
 					}
 					return $vCalendar->serialize();
@@ -291,11 +276,8 @@ class RefreshWebcalService {
 	 *  - the webcal feed suggests a refreshrate
 	 *  - return suggested refreshrate if user didn't set a custom one
 	 *
-	 * @param array $subscription
-	 * @param string $webcalData
-	 * @return string|null
 	 */
-	private function checkWebcalDataForRefreshRate($subscription, $webcalData) {
+	private function checkWebcalDataForRefreshRate(array $subscription, string $webcalData): ?string {
 		// if there is no refreshrate stored in the database, check the webcal feed
 		// whether it suggests any refresh rate and store that in the database
 		if (isset($subscription[self::REFRESH_RATE]) && $subscription[self::REFRESH_RATE] !== null) {
@@ -353,7 +335,7 @@ class RefreshWebcalService {
 	 * @param string $url
 	 * @return string|null
 	 */
-	private function cleanURL(string $url) {
+	private function cleanURL(string $url): ?string {
 		$parsed = parse_url($url);
 		if ($parsed === false) {
 			return null;
diff --git a/apps/dav/tests/unit/CalDAV/WebcalCaching/RefreshWebcalServiceTest.php b/apps/dav/tests/unit/CalDAV/WebcalCaching/RefreshWebcalServiceTest.php
index 6bd1f6b3206..71d93bf851e 100644
--- a/apps/dav/tests/unit/CalDAV/WebcalCaching/RefreshWebcalServiceTest.php
+++ b/apps/dav/tests/unit/CalDAV/WebcalCaching/RefreshWebcalServiceTest.php
@@ -74,7 +74,7 @@ class RefreshWebcalServiceTest extends TestCase {
 	 */
 	public function testRun(string $body, string $contentType, string $result) {
 		$refreshWebcalService = $this->getMockBuilder(RefreshWebcalService::class)
-			->setMethods(['getRandomCalendarObjectUri'])
+			->onlyMethods(['getRandomCalendarObjectUri'])
 			->setConstructorArgs([$this->caldavBackend, $this->clientService, $this->config, $this->logger])
 			->getMock();
 
@@ -156,7 +156,7 @@ class RefreshWebcalServiceTest extends TestCase {
 		$client = $this->createMock(IClient::class);
 		$response = $this->createMock(IResponse::class);
 		$refreshWebcalService = $this->getMockBuilder(RefreshWebcalService::class)
-			->setMethods(['getRandomCalendarObjectUri', 'getSubscription', 'queryWebcalFeed'])
+			->onlyMethods(['getRandomCalendarObjectUri', 'getSubscription', 'queryWebcalFeed'])
 			->setConstructorArgs([$this->caldavBackend, $this->clientService, $this->config, $this->logger])
 			->getMock();
 
@@ -217,7 +217,7 @@ class RefreshWebcalServiceTest extends TestCase {
 
 		$this->logger->expects($this->once())
 			->method('error')
-			->with($noInstanceException->getMessage(), ['exception' => $noInstanceException]);
+			->with('Unable to create calendar object from subscription {subscriptionId}', ['exception' => $noInstanceException, 'subscriptionId' => '42', 'source' => 'webcal://foo.bar/bla2']);
 
 		$refreshWebcalService->refreshSubscription('principals/users/testuser', 'sub123');
 	}
@@ -233,7 +233,7 @@ class RefreshWebcalServiceTest extends TestCase {
 		$client = $this->createMock(IClient::class);
 		$response = $this->createMock(IResponse::class);
 		$refreshWebcalService = $this->getMockBuilder(RefreshWebcalService::class)
-			->setMethods(['getRandomCalendarObjectUri', 'getSubscription', 'queryWebcalFeed'])
+			->onlyMethods(['getRandomCalendarObjectUri', 'getSubscription', 'queryWebcalFeed'])
 			->setConstructorArgs([$this->caldavBackend, $this->clientService, $this->config, $this->logger])
 			->getMock();
 
@@ -294,7 +294,7 @@ class RefreshWebcalServiceTest extends TestCase {
 
 		$this->logger->expects($this->once())
 			->method('error')
-			->with($badRequestException->getMessage(), ['exception' => $badRequestException]);
+			->with('Unable to create calendar object from subscription {subscriptionId}', ['exception' => $badRequestException, 'subscriptionId' => '42', 'source' => 'webcal://foo.bar/bla2']);
 
 		$refreshWebcalService->refreshSubscription('principals/users/testuser', 'sub123');
 	}
@@ -324,10 +324,8 @@ class RefreshWebcalServiceTest extends TestCase {
 
 	/**
 	 * @dataProvider runLocalURLDataProvider
-	 *
-	 * @param string $source
 	 */
-	public function testRunLocalURL($source) {
+	public function testRunLocalURL(string $source) {
 		$refreshWebcalService = new RefreshWebcalService(
 			$this->caldavBackend,
 			$this->clientService,
@@ -361,14 +359,15 @@ class RefreshWebcalServiceTest extends TestCase {
 			->with('dav', 'webcalAllowLocalAccess', 'no')
 			->willReturn('no');
 
-		$exception = new LocalServerException();
+		$localServerException = new LocalServerException();
+
 		$client->expects($this->once())
 			->method('get')
-			->willThrowException($exception);
+			->willThrowException($localServerException);
 
 		$this->logger->expects($this->once())
 			->method('warning')
-			->with($this->anything(), ['exception' => $exception]);
+			->with("Subscription 42 was not refreshed because it violates local access rules", ['exception' => $localServerException]);
 
 		$refreshWebcalService->refreshSubscription('principals/users/testuser', 'sub123');
 	}