Skip to content

Commit f0cd11b

Browse files
madbobmadbob
committed
feat: tracking issues with calendar subscriptions
Signed-off-by: Roberto Guido <info@madbob.org>
1 parent d3aba32 commit f0cd11b

10 files changed

Lines changed: 147 additions & 36 deletions

File tree

‎apps/dav/composer/composer/autoload_classmap.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -319,6 +319,8 @@
319319
'OCA\\DAV\\Events\\SubscriptionDeletedEvent' => $baseDir . '/../lib/Events/SubscriptionDeletedEvent.php',
320320
'OCA\\DAV\\Events\\SubscriptionUpdatedEvent' => $baseDir . '/../lib/Events/SubscriptionUpdatedEvent.php',
321321
'OCA\\DAV\\Exception\\ExampleEventException' => $baseDir . '/../lib/Exception/ExampleEventException.php',
322+
'OCA\\DAV\\Exception\\InvalidSubscriptionPayload' => $baseDir . '/../lib/Exception/InvalidSubscriptionPayload.php',
323+
'OCA\\DAV\\Exception\\InvalidSubscriptionUrl' => $baseDir . '/../lib/Exception/InvalidSubscriptionUrl.php',
322324
'OCA\\DAV\\Exception\\ServerMaintenanceMode' => $baseDir . '/../lib/Exception/ServerMaintenanceMode.php',
323325
'OCA\\DAV\\Exception\\UidConflict' => $baseDir . '/../lib/Exception/UidConflict.php',
324326
'OCA\\DAV\\Exception\\UnsupportedLimitOnInitialSyncException' => $baseDir . '/../lib/Exception/UnsupportedLimitOnInitialSyncException.php',
@@ -401,6 +403,7 @@
401403
'OCA\\DAV\\Migration\\Version1038Date20260302000000' => $baseDir . '/../lib/Migration/Version1038Date20260302000000.php',
402404
'OCA\\DAV\\Migration\\Version1039Date20260408000000' => $baseDir . '/../lib/Migration/Version1039Date20260408000000.php',
403405
'OCA\\DAV\\Migration\\Version1040Date20260805000000' => $baseDir . '/../lib/Migration/Version1040Date20260805000000.php',
406+
'OCA\\DAV\\Migration\\Version2000Date20260815202617' => $baseDir . '/../lib/Migration/Version2000Date20260815202617.php',
404407
'OCA\\DAV\\Model\\ExampleEvent' => $baseDir . '/../lib/Model/ExampleEvent.php',
405408
'OCA\\DAV\\Paginate\\LimitedCopyIterator' => $baseDir . '/../lib/Paginate/LimitedCopyIterator.php',
406409
'OCA\\DAV\\Paginate\\PaginateCache' => $baseDir . '/../lib/Paginate/PaginateCache.php',

‎apps/dav/composer/composer/autoload_static.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -334,6 +334,8 @@ class ComposerStaticInitDAV
334334
'OCA\\DAV\\Events\\SubscriptionDeletedEvent' => __DIR__ . '/..' . '/../lib/Events/SubscriptionDeletedEvent.php',
335335
'OCA\\DAV\\Events\\SubscriptionUpdatedEvent' => __DIR__ . '/..' . '/../lib/Events/SubscriptionUpdatedEvent.php',
336336
'OCA\\DAV\\Exception\\ExampleEventException' => __DIR__ . '/..' . '/../lib/Exception/ExampleEventException.php',
337+
'OCA\\DAV\\Exception\\InvalidSubscriptionPayload' => __DIR__ . '/..' . '/../lib/Exception/InvalidSubscriptionPayload.php',
338+
'OCA\\DAV\\Exception\\InvalidSubscriptionUrl' => __DIR__ . '/..' . '/../lib/Exception/InvalidSubscriptionUrl.php',
337339
'OCA\\DAV\\Exception\\ServerMaintenanceMode' => __DIR__ . '/..' . '/../lib/Exception/ServerMaintenanceMode.php',
338340
'OCA\\DAV\\Exception\\UidConflict' => __DIR__ . '/..' . '/../lib/Exception/UidConflict.php',
339341
'OCA\\DAV\\Exception\\UnsupportedLimitOnInitialSyncException' => __DIR__ . '/..' . '/../lib/Exception/UnsupportedLimitOnInitialSyncException.php',
@@ -416,6 +418,7 @@ class ComposerStaticInitDAV
416418
'OCA\\DAV\\Migration\\Version1038Date20260302000000' => __DIR__ . '/..' . '/../lib/Migration/Version1038Date20260302000000.php',
417419
'OCA\\DAV\\Migration\\Version1039Date20260408000000' => __DIR__ . '/..' . '/../lib/Migration/Version1039Date20260408000000.php',
418420
'OCA\\DAV\\Migration\\Version1040Date20260805000000' => __DIR__ . '/..' . '/../lib/Migration/Version1040Date20260805000000.php',
421+
'OCA\\DAV\\Migration\\Version2000Date20260815202617' => __DIR__ . '/..' . '/../lib/Migration/Version2000Date20260815202617.php',
419422
'OCA\\DAV\\Model\\ExampleEvent' => __DIR__ . '/..' . '/../lib/Model/ExampleEvent.php',
420423
'OCA\\DAV\\Paginate\\LimitedCopyIterator' => __DIR__ . '/..' . '/../lib/Paginate/LimitedCopyIterator.php',
421424
'OCA\\DAV\\Paginate\\PaginateCache' => __DIR__ . '/..' . '/../lib/Paginate/PaginateCache.php',

‎apps/dav/lib/CalDAV/CalDavBackend.php‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
use DateTime;
1212
use DateTimeImmutable;
1313
use DateTimeInterface;
14+
use Exception as NativeException;
1415
use Generator;
1516
use OCA\DAV\AppInfo\Application;
1617
use OCA\DAV\CalDAV\Federation\FederatedCalendarEntity;
@@ -3160,6 +3161,29 @@ public function deleteSubscription($subscriptionId) {
31603161
}, $this->db);
31613162
}
31623163

3164+
/**
3165+
* Update the error status of a subscription
3166+
*
3167+
* @param mixed $subscriptionId
3168+
* @param null|NativeException $exception
3169+
* @param null|string $error
3170+
* @return void
3171+
*/
3172+
public function trackSubscriptionError($subscriptionId, $exception, $error): void {
3173+
$query = $this->db->getQueryBuilder();
3174+
$query->update('calendarsubscriptions')
3175+
->set('lasterror', $query->createNamedParameter($error))
3176+
->executeStatement();
3177+
3178+
if ($error) {
3179+
$this->logger->error('Subscription {subscriptionId} could not be refreshed: {error}', [
3180+
'exception' => $exception,
3181+
'subscriptionId' => $subscriptionId,
3182+
'error' => $error,
3183+
]);
3184+
}
3185+
}
3186+
31633187
/**
31643188
* Returns a single scheduling object for the inbox collection.
31653189
*

‎apps/dav/lib/CalDAV/WebcalCaching/Connection.php‎

Lines changed: 6 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -9,18 +9,16 @@
99

1010
namespace OCA\DAV\CalDAV\WebcalCaching;
1111

12-
use Exception;
1312
use GuzzleHttp\RequestOptions;
13+
use OCA\DAV\Exception\InvalidSubscriptionPayload;
14+
use OCA\DAV\Exception\InvalidSubscriptionUrl;
1415
use OCP\Http\Client\IClientService;
15-
use OCP\Http\Client\LocalServerException;
1616
use OCP\IAppConfig;
17-
use Psr\Log\LoggerInterface;
1817

1918
class Connection {
2019
public function __construct(
2120
private IClientService $clientService,
2221
private IAppConfig $config,
23-
private LoggerInterface $logger,
2422
) {
2523
}
2624

@@ -33,7 +31,7 @@ public function queryWebcalFeed(array $subscription): ?array {
3331
$subscriptionId = $subscription['id'];
3432
$url = $this->cleanURL($subscription['source']);
3533
if ($url === null) {
36-
return null;
34+
throw new InvalidSubscriptionUrl('Wrong URL: ' . $subscription['source']);
3735
}
3836

3937
// ICS feeds hosted on O365 can return HTTP 500 when the UA string isn't satisfactory
@@ -65,20 +63,8 @@ public function queryWebcalFeed(array $subscription): ?array {
6563
$params[RequestOptions::AUTH] = [$user, $pass];
6664
}
6765

68-
try {
69-
$client = $this->clientService->newClient();
70-
$response = $client->get($url, $params);
71-
} catch (LocalServerException $ex) {
72-
$this->logger->warning("Subscription $subscriptionId was not refreshed because it violates local access rules", [
73-
'exception' => $ex,
74-
]);
75-
return null;
76-
} catch (Exception $ex) {
77-
$this->logger->warning("Subscription $subscriptionId could not be refreshed due to a network error", [
78-
'exception' => $ex,
79-
]);
80-
return null;
81-
}
66+
$client = $this->clientService->newClient();
67+
$response = $client->get($url, $params);
8268

8369
$contentType = $response->getHeader('Content-Type');
8470
$contentType = explode(';', $contentType, 2)[0];
@@ -92,7 +78,7 @@ public function queryWebcalFeed(array $subscription): ?array {
9278
// With 'stream' => true, getBody() returns the underlying stream resource
9379
$stream = $response->getBody();
9480
if (!is_resource($stream)) {
95-
return null;
81+
throw new InvalidSubscriptionPayload('Cannot open a stream for ' . $url);
9682
}
9783

9884
return ['data' => $stream, 'format' => $format];

‎apps/dav/lib/CalDAV/WebcalCaching/RefreshWebcalService.php‎

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,13 @@
99

1010
namespace OCA\DAV\CalDAV\WebcalCaching;
1111

12+
use Exception;
1213
use OCA\DAV\CalDAV\CalDavBackend;
1314
use OCA\DAV\CalDAV\Import\ImportService;
15+
use OCA\DAV\Exception\InvalidSubscriptionPayload;
16+
use OCA\DAV\Exception\InvalidSubscriptionUrl;
1417
use OCP\AppFramework\Utility\ITimeFactory;
18+
use OCP\Http\Client\LocalServerException;
1519
use Psr\Log\LoggerInterface;
1620
use Sabre\DAV\PropPatch;
1721
use Sabre\VObject\Component;
@@ -54,19 +58,18 @@ public function refreshSubscription(string $principalUri, string $uri) {
5458
}
5559
}
5660

57-
$result = $this->connection->queryWebcalFeed($subscription);
58-
if (!$result) {
59-
return;
60-
}
61+
$data = null;
62+
63+
try {
64+
$result = $this->connection->queryWebcalFeed($subscription);
6165

62-
$data = $result['data'];
63-
$format = $result['format'];
66+
$data = $result['data'];
67+
$format = $result['format'];
6468

65-
$stripTodos = ($subscription[self::STRIP_TODOS] ?? 1) === 1;
66-
$stripAlarms = ($subscription[self::STRIP_ALARMS] ?? 1) === 1;
67-
$stripAttachments = ($subscription[self::STRIP_ATTACHMENTS] ?? 1) === 1;
69+
$stripTodos = ($subscription[self::STRIP_TODOS] ?? 1) === 1;
70+
$stripAlarms = ($subscription[self::STRIP_ALARMS] ?? 1) === 1;
71+
$stripAttachments = ($subscription[self::STRIP_ATTACHMENTS] ?? 1) === 1;
6872

69-
try {
7073
$existingObjects = $this->calDavBackend->getLimitedCalendarObjects((int)$subscription['id'], CalDavBackend::CALENDAR_TYPE_SUBSCRIPTION, ['id', 'uid', 'etag', 'uri']);
7174

7275
$generator = match ($format) {
@@ -158,11 +161,21 @@ public function refreshSubscription(string $principalUri, string $uri) {
158161
if (isset($vObject)) {
159162
$this->updateRefreshRate($subscription, $vObject);
160163
}
164+
165+
$this->calDavBackend->trackSubscriptionError($subscription['id'], null, null);
161166
} catch (ParseException $ex) {
162-
$this->logger->error('Subscription {subscriptionId} could not be refreshed due to a parsing error', ['exception' => $ex, 'subscriptionId' => $subscription['id']]);
167+
$this->calDavBackend->trackSubscriptionError($subscription['id'], $ex, 'Parsing error');
168+
} catch (LocalServerException $ex) {
169+
$this->calDavBackend->trackSubscriptionError($subscription['id'], $ex, 'Subscription violates local access rules');
170+
} catch (InvalidSubscriptionUrl $ex) {
171+
$this->calDavBackend->trackSubscriptionError($subscription['id'], $ex, 'Invalid URL');
172+
} catch (InvalidSubscriptionPayload $ex) {
173+
$this->calDavBackend->trackSubscriptionError($subscription['id'], $ex, 'Invalid contents');
174+
} catch (Exception $ex) {
175+
$this->calDavBackend->trackSubscriptionError($subscription['id'], $ex, 'Network error');
163176
} finally {
164177
// Close the data stream to free resources
165-
if (is_resource($data)) {
178+
if ($data && is_resource($data)) {
166179
fclose($data);
167180
}
168181
}

‎apps/dav/lib/Exception/InvalidSubscriptionPayload.php‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\DAV\Exception;
11+
12+
use Exception;
13+
14+
class InvalidSubscriptionPayload extends Exception {
15+
}

‎apps/dav/lib/Exception/InvalidSubscriptionUrl.php‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\DAV\Exception;
11+
12+
use Exception;
13+
14+
class InvalidSubscriptionUrl extends Exception {
15+
}

‎apps/dav/lib/Migration/Version2000Date20260815202617.php‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\DAV\Migration;
11+
12+
use Closure;
13+
use OCP\DB\ISchemaWrapper;
14+
use OCP\DB\Types;
15+
use OCP\Migration\IOutput;
16+
use OCP\Migration\Attributes\AddColumn;
17+
use OCP\Migration\Attributes\ColumnType;
18+
use OCP\Migration\SimpleMigrationStep;
19+
20+
#[AddColumn(table: 'calendarsubscriptions', name: 'lasterror', type: ColumnType::STRING)]
21+
class Version2000Date20260815202617 extends SimpleMigrationStep {
22+
#[\Override]
23+
public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper {
24+
/** @var ISchemaWrapper $schema */
25+
$schema = $schemaClosure();
26+
27+
$table = $schema->getTable('calendarsubscriptions');
28+
29+
if (!$table->hasColumn('lasterror')) {
30+
$table->addColumn('lasterror', Types::STRING, [
31+
'notnull' => false,
32+
'length' => 255,
33+
'default' => null,
34+
]);
35+
}
36+
37+
return $schema;
38+
}
39+
}

‎apps/dav/tests/unit/CalDAV/WebcalCaching/ConnectionTest.php‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99
namespace OCA\DAV\Tests\unit\CalDAV\WebcalCaching;
1010

1111
use OCA\DAV\CalDAV\WebcalCaching\Connection;
12+
use OCA\DAV\Exception\InvalidSubscriptionPayload;
13+
use OCA\DAV\Exception\InvalidSubscriptionUrl;
1214
use OCP\Http\Client\IClient;
1315
use OCP\Http\Client\IClientService;
1416
use OCP\Http\Client\IResponse;
@@ -60,10 +62,8 @@ public function testLocalUrl($source): void {
6062
$client->expects(self::once())
6163
->method('get')
6264
->willThrowException($localServerException);
63-
$this->logger->expects(self::once())
64-
->method('warning')
65-
->with('Subscription 42 was not refreshed because it violates local access rules', ['exception' => $localServerException]);
6665

66+
$this->expectException(LocalServerException::class);
6767
$this->connection->queryWebcalFeed($subscription);
6868
}
6969

@@ -85,6 +85,7 @@ public function testInvalidUrl(): void {
8585
$client->expects(self::never())
8686
->method('get');
8787

88+
$this->expectException(InvalidSubscriptionUrl::class);
8889
$this->connection->queryWebcalFeed($subscription);
8990

9091
}
@@ -186,6 +187,7 @@ public function testConnectionReturnsNullWhenBodyIsNotResource(): void {
186187
->method('getBody')
187188
->willReturn('not a resource');
188189

190+
$this->expectException(InvalidSubscriptionPayload::class);
189191
$output = $this->connection->queryWebcalFeed($subscription);
190192

191193
$this->assertNull($output);

‎apps/dav/tests/unit/CalDAV/WebcalCaching/RefreshWebcalServiceTest.php‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
use OCA\DAV\CalDAV\Import\ImportService;
1313
use OCA\DAV\CalDAV\WebcalCaching\Connection;
1414
use OCA\DAV\CalDAV\WebcalCaching\RefreshWebcalService;
15+
use OCA\DAV\Exception\InvalidSubscriptionUrl;
1516
use OCP\AppFramework\Utility\ITimeFactory;
1617
use PHPUnit\Framework\MockObject\MockObject;
1718
use Psr\Log\LoggerInterface;
@@ -92,6 +93,10 @@ public function testRun(string $body, string $format, string $result): void {
9293
->method('queryWebcalFeed')
9394
->willReturn(['data' => $stream, 'format' => $format]);
9495

96+
$this->caldavBackend->expects(self::once())
97+
->method('trackSubscriptionError')
98+
->with('42', null, null);
99+
95100
$this->caldavBackend->expects(self::once())
96101
->method('getLimitedCalendarObjects')
97102
->willReturn([]);
@@ -226,9 +231,15 @@ public function testConnectionReturnsNull(): void {
226231
],
227232
]);
228233

234+
$exception = new InvalidSubscriptionUrl('broken');
235+
229236
$this->connection->expects(self::once())
230237
->method('queryWebcalFeed')
231-
->willReturn(null);
238+
->willThrowException($exception);
239+
240+
$this->caldavBackend->expects(self::once())
241+
->method('trackSubscriptionError')
242+
->with('42', $exception, 'Invalid URL');
232243

233244
$this->importService->expects(self::never())
234245
->method('importText');

0 commit comments

Comments
 (0)