diff --git a/3rdparty b/3rdparty index 9a181f79a2ad8..4b8c973530ab1 160000 --- a/3rdparty +++ b/3rdparty @@ -1 +1 @@ -Subproject commit 9a181f79a2ad8fa3be4b24a6bfbc0af57ed51e49 +Subproject commit 4b8c973530ab1464bbcb76ed9a59478bbaecc9ca diff --git a/apps/dav/lib/CalDAV/Federation/FederatedCalendarSyncService.php b/apps/dav/lib/CalDAV/Federation/FederatedCalendarSyncService.php index 45ea9be17be7f..9c05f3f5b4151 100644 --- a/apps/dav/lib/CalDAV/Federation/FederatedCalendarSyncService.php +++ b/apps/dav/lib/CalDAV/Federation/FederatedCalendarSyncService.php @@ -11,18 +11,14 @@ use OCA\DAV\CalDAV\CalDavBackend; use OCA\DAV\Service\ASyncService; -use OCP\AppFramework\Db\TTransactional; use OCP\AppFramework\Http; use OCP\Federation\ICloudIdManager; use OCP\Http\Client\IClientService; use OCP\IConfig; -use OCP\IDBConnection; use Psr\Http\Client\ClientExceptionInterface; use Psr\Log\LoggerInterface; class FederatedCalendarSyncService extends ASyncService { - use TTransactional; - private const string SYNC_TOKEN_PREFIX = 'http://sabre.io/ns/sync/'; public function __construct( @@ -31,7 +27,6 @@ public function __construct( private readonly FederatedCalendarMapper $federatedCalendarMapper, private readonly LoggerInterface $logger, private readonly CalDavBackend $backend, - private readonly IDBConnection $dbConnection, private readonly ICloudIdManager $cloudIdManager, ) { parent::__construct($clientService, $config); @@ -94,28 +89,26 @@ public function syncOne(FederatedCalendarEntity $calendar): int { // Object created or updated $absoluteUrl = $this->prepareUri($credentials['remoteUrl'], $resource); $calendarData = $this->download($absoluteUrl, $credentials['username'], $credentials['token']); - $this->atomic(function () use ($calendar, $objectUri, $calendarData): void { - $existingObject = $this->backend->getCalendarObject( + $existingObject = $this->backend->getCalendarObject( + $calendar->getId(), + $objectUri, + CalDavBackend::CALENDAR_TYPE_FEDERATED + ); + if (!$existingObject) { + $this->backend->createCalendarObject( + $calendar->getId(), + $objectUri, + $calendarData, + CalDavBackend::CALENDAR_TYPE_FEDERATED + ); + } else { + $this->backend->updateCalendarObject( $calendar->getId(), $objectUri, + $calendarData, CalDavBackend::CALENDAR_TYPE_FEDERATED ); - if (!$existingObject) { - $this->backend->createCalendarObject( - $calendar->getId(), - $objectUri, - $calendarData, - CalDavBackend::CALENDAR_TYPE_FEDERATED - ); - } else { - $this->backend->updateCalendarObject( - $calendar->getId(), - $objectUri, - $calendarData, - CalDavBackend::CALENDAR_TYPE_FEDERATED - ); - } - }, $this->dbConnection); + } $downloadedEvents++; } else { // Object deleted diff --git a/apps/dav/lib/CalDAV/SyncService.php b/apps/dav/lib/CalDAV/SyncService.php index f11f0a4bf301e..11b4b6f38c8a4 100644 --- a/apps/dav/lib/CalDAV/SyncService.php +++ b/apps/dav/lib/CalDAV/SyncService.php @@ -12,23 +12,18 @@ use OCA\DAV\CalDAV\Federation\FederatedCalendarEntity; use OCA\DAV\CalDAV\Federation\FederatedCalendarMapper; use OCA\DAV\Service\ASyncService; -use OCP\AppFramework\Db\TTransactional; use OCP\AppFramework\Http; use OCP\Http\Client\IClientService; use OCP\IConfig; -use OCP\IDBConnection; use Psr\Http\Client\ClientExceptionInterface; use Psr\Log\LoggerInterface; class SyncService extends ASyncService { - use TTransactional; - public function __construct( IClientService $clientService, IConfig $config, private readonly CalDavBackend $backend, private readonly FederatedCalendarMapper $federatedCalendarMapper, - private readonly IDBConnection $dbConnection, private readonly LoggerInterface $logger, ) { parent::__construct($clientService, $config); @@ -70,14 +65,12 @@ public function syncRemoteCalendar( if (isset($status[200])) { $absoluteUrl = $this->prepareUri($url, $resource); $vCard = $this->download($absoluteUrl, $username, $sharedSecret); - $this->atomic(function () use ($calendar, $objectUri, $vCard): void { - $existingObject = $this->backend->getCalendarObject($calendar->getId(), $objectUri, CalDavBackend::CALENDAR_TYPE_FEDERATED); - if (!$existingObject) { - $this->backend->createCalendarObject($calendar->getId(), $objectUri, $vCard, CalDavBackend::CALENDAR_TYPE_FEDERATED); - } else { - $this->backend->updateCalendarObject($calendar->getId(), $objectUri, $vCard, CalDavBackend::CALENDAR_TYPE_FEDERATED); - } - }, $this->dbConnection); + $existingObject = $this->backend->getCalendarObject($calendar->getId(), $objectUri, CalDavBackend::CALENDAR_TYPE_FEDERATED); + if (!$existingObject) { + $this->backend->createCalendarObject($calendar->getId(), $objectUri, $vCard, CalDavBackend::CALENDAR_TYPE_FEDERATED); + } else { + $this->backend->updateCalendarObject($calendar->getId(), $objectUri, $vCard, CalDavBackend::CALENDAR_TYPE_FEDERATED); + } $downloadedEvents++; } else { $this->backend->deleteCalendarObject($calendar->getId(), $objectUri, CalDavBackend::CALENDAR_TYPE_FEDERATED, true); diff --git a/apps/dav/lib/CardDAV/SyncService.php b/apps/dav/lib/CardDAV/SyncService.php index 0dec815ec769a..4be3f1dc91ed4 100644 --- a/apps/dav/lib/CardDAV/SyncService.php +++ b/apps/dav/lib/CardDAV/SyncService.php @@ -73,14 +73,12 @@ public function syncRemoteAddressBook(string $url, string $userName, string $add if (isset($status[200])) { $absoluteUrl = $this->prepareUri($url, $resource); $vCard = $this->download($absoluteUrl, $userName, $sharedSecret); - $this->atomic(function () use ($addressBookId, $cardUri, $vCard): void { - $existingCard = $this->backend->getCard($addressBookId, $cardUri); - if ($existingCard === false) { - $this->backend->createCard($addressBookId, $cardUri, $vCard); - } else { - $this->backend->updateCard($addressBookId, $cardUri, $vCard); - } - }, $this->dbConnection); + $existingCard = $this->backend->getCard($addressBookId, $cardUri); + if ($existingCard === false) { + $this->backend->createCard($addressBookId, $cardUri, $vCard); + } else { + $this->backend->updateCard($addressBookId, $cardUri, $vCard); + } } else { $this->backend->deleteCard($addressBookId, $cardUri); } @@ -140,27 +138,24 @@ public function updateUser(IUser $user): void { $cardId = self::getCardUri($user); if ($user->isEnabled()) { - $this->atomic(function () use ($addressBookId, $cardId, $user): void { - $card = $this->backend->getCard($addressBookId, $cardId); - if ($card === false) { - $vCard = $this->converter->createCardFromUser($user); - if ($vCard !== null) { - $this->backend->createCard($addressBookId, $cardId, $vCard->serialize(), false); - } + $vCard = $this->converter->createCardFromUser($user); + $card = $this->backend->getCard($addressBookId, $cardId); + if ($card === false) { + if ($vCard !== null) { + $this->backend->createCard($addressBookId, $cardId, $vCard->serialize(), false); + } + } else { + if (is_null($vCard)) { + $this->backend->deleteCard($addressBookId, $cardId); } else { - $vCard = $this->converter->createCardFromUser($user); - if (is_null($vCard)) { - $this->backend->deleteCard($addressBookId, $cardId); - } else { - $cardData = $vCard->serialize(); - // Writing an identical card would still bump the address book - // sync token and make every client re-download the card - if ($card['carddata'] !== $cardData) { - $this->backend->updateCard($addressBookId, $cardId, $cardData); - } + $cardData = $vCard->serialize(); + // Writing an identical card would still bump the address book + // sync token and make every client re-download the card + if ($card['carddata'] !== $cardData) { + $this->backend->updateCard($addressBookId, $cardId, $cardData); } } - }, $this->dbConnection); + } } else { $this->backend->deleteCard($addressBookId, $cardId); } diff --git a/apps/dav/tests/unit/CalDAV/Federation/FederatedCalendarSyncServiceTest.php b/apps/dav/tests/unit/CalDAV/Federation/FederatedCalendarSyncServiceTest.php index 21c410a879e1a..f536d030a3c12 100644 --- a/apps/dav/tests/unit/CalDAV/Federation/FederatedCalendarSyncServiceTest.php +++ b/apps/dav/tests/unit/CalDAV/Federation/FederatedCalendarSyncServiceTest.php @@ -19,7 +19,6 @@ use OCP\Http\Client\IClientService; use OCP\Http\Client\IResponse; use OCP\IConfig; -use OCP\IDBConnection; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; @@ -31,7 +30,6 @@ class FederatedCalendarSyncServiceTest extends TestCase { private FederatedCalendarMapper&MockObject $federatedCalendarMapper; private LoggerInterface&MockObject $logger; private CalDavBackend&MockObject $backend; - private IDBConnection&MockObject $dbConnection; private ICloudIdManager&MockObject $cloudIdManager; private IClientService&MockObject $clientService; private IConfig&MockObject $config; @@ -42,7 +40,6 @@ protected function setUp(): void { $this->federatedCalendarMapper = $this->createMock(FederatedCalendarMapper::class); $this->logger = $this->createMock(LoggerInterface::class); $this->backend = $this->createMock(CalDavBackend::class); - $this->dbConnection = $this->createMock(IDBConnection::class); $this->cloudIdManager = $this->createMock(ICloudIdManager::class); $this->clientService = $this->createMock(IClientService::class); $this->config = $this->createMock(IConfig::class); @@ -53,7 +50,6 @@ protected function setUp(): void { $this->federatedCalendarMapper, $this->logger, $this->backend, - $this->dbConnection, $this->cloudIdManager, ); } @@ -146,6 +142,74 @@ public function testSyncOneUnchanged(): void { $this->assertEquals(0, $this->federatedCalendarSyncService->syncOne($calendar)); } + public static function provideExistingObjectData(): array { + return [ + 'new object' => [null, 1, 0], + 'existing object' => [['id' => 10], 0, 1], + ]; + } + + #[DataProvider(methodName: 'provideExistingObjectData')] + public function testSyncOneWithChangedObject(?array $existingObject, int $createCalls, int $updateCalls): void { + $calendar = new FederatedCalendarEntity(); + $calendar->setId(1); + $calendar->setPrincipaluri('principals/users/user1'); + $calendar->setRemoteUrl('https://remote.tld/remote.php/dav/remote-calendars/abcdef123/cal1_shared_by_user2'); + $calendar->setSyncToken(100); + $calendar->setToken('token'); + + $cloudId = $this->createMock(ICloudId::class); + $cloudId->method('getId') + ->willReturn('user1@nextcloud.testing'); + $this->cloudIdManager->expects(self::once()) + ->method('getCloudId') + ->with('user1') + ->willReturn($cloudId); + + $client = $this->createMock(IClient::class); + $reportResponse = $this->createMock(IResponse::class); + $reportResponse->method('getBody') + ->willReturn('/remote.php/dav/remote-calendars/abcdef123/cal1_shared_by_user2/event1.ics"abc"HTTP/1.1 200 OKhttp://sabre.io/ns/sync/101'); + $client->expects(self::once()) + ->method('request') + ->with('REPORT', 'https://remote.tld/remote.php/dav/remote-calendars/abcdef123/cal1_shared_by_user2', self::anything()) + ->willReturn($reportResponse); + + $calendarData = "BEGIN:VCALENDAR\r\nVERSION:2.0\r\nEND:VCALENDAR\r\n"; + $getResponse = $this->createMock(IResponse::class); + $getResponse->method('getBody') + ->willReturn($calendarData); + $client->expects(self::once()) + ->method('get') + ->with('https://remote.tld/remote.php/dav/remote-calendars/abcdef123/cal1_shared_by_user2/event1.ics', self::anything()) + ->willReturn($getResponse); + + $this->clientService->method('newClient') + ->willReturn($client); + + $this->config->method('getSystemValueInt') + ->willReturn(30); + $this->config->method('getSystemValue') + ->willReturn(false); + + $this->backend->expects(self::once()) + ->method('getCalendarObject') + ->with(1, 'event1.ics', CalDavBackend::CALENDAR_TYPE_FEDERATED) + ->willReturn($existingObject); + $this->backend->expects(self::exactly($createCalls)) + ->method('createCalendarObject') + ->with(1, 'event1.ics', $calendarData, CalDavBackend::CALENDAR_TYPE_FEDERATED); + $this->backend->expects(self::exactly($updateCalls)) + ->method('updateCalendarObject') + ->with(1, 'event1.ics', $calendarData, CalDavBackend::CALENDAR_TYPE_FEDERATED); + + $this->federatedCalendarMapper->expects(self::once()) + ->method('updateSyncTokenAndTime') + ->with(1, 101); + + $this->assertEquals(1, $this->federatedCalendarSyncService->syncOne($calendar)); + } + public static function provideUnexpectedSyncTokenData(): array { return [ ['http://sabre.io/ns/sync/'], diff --git a/apps/dav/tests/unit/CardDAV/SyncServiceTest.php b/apps/dav/tests/unit/CardDAV/SyncServiceTest.php index 232c5c392abae..267888b95c7e9 100644 --- a/apps/dav/tests/unit/CardDAV/SyncServiceTest.php +++ b/apps/dav/tests/unit/CardDAV/SyncServiceTest.php @@ -38,6 +38,7 @@ class SyncServiceTest extends TestCase { protected IClient&MockObject $client; protected IConfig&MockObject $config; protected SyncService $service; + protected int $transactionDepth = 0; public function setUp(): void { parent::setUp(); @@ -57,6 +58,7 @@ public function setUp(): void { $this->userManager = $this->createMock(IUserManager::class); $this->dbConnection = $this->createMock(IDBConnection::class); + $this->trackTransactionDepth($this->dbConnection); $this->logger = new NullLogger(); $this->converter = $this->createMock(Converter::class); $this->client = $this->createMock(IClient::class); @@ -77,6 +79,22 @@ public function setUp(): void { ); } + private function trackTransactionDepth(IDBConnection&MockObject $dbConnection): void { + $dbConnection->method('beginTransaction')->willReturnCallback(function (): void { + $this->transactionDepth++; + }); + $dbConnection->method('commit')->willReturnCallback(function (): void { + $this->transactionDepth--; + }); + $dbConnection->method('rollBack')->willReturnCallback(function (): void { + $this->transactionDepth--; + }); + } + + private function assertNotInTransaction(): void { + $this->assertSame(0, $this->transactionDepth, 'Expected no open transaction'); + } + public function testEmptySync(): void { $this->backend->expects($this->exactly(0)) ->method('createCard'); @@ -116,7 +134,8 @@ public function testEmptySync(): void { public function testSyncWithNewElement(): void { $this->backend->expects($this->exactly(1)) - ->method('createCard'); + ->method('createCard') + ->willReturnCallback(fn () => $this->assertNotInTransaction()); $this->backend->expects($this->exactly(0)) ->method('updateCard'); $this->backend->expects($this->exactly(0)) @@ -188,7 +207,8 @@ public function testSyncWithUpdatedElement(): void { $this->backend->expects($this->exactly(0)) ->method('createCard'); $this->backend->expects($this->exactly(1)) - ->method('updateCard'); + ->method('updateCard') + ->willReturnCallback(fn () => $this->assertNotInTransaction()); $this->backend->expects($this->exactly(0)) ->method('deleteCard'); @@ -421,8 +441,10 @@ public function testUpdateAndDeleteUser(bool $activated, int $createCalls, int $ $backend = $this->getMockBuilder(CardDavBackend::class)->disableOriginalConstructor()->getMock(); $logger = $this->getMockBuilder(LoggerInterface::class)->disableOriginalConstructor()->getMock(); - $backend->expects($this->exactly($createCalls))->method('createCard'); - $backend->expects($this->exactly($updateCalls))->method('updateCard'); + $backend->expects($this->exactly($createCalls))->method('createCard') + ->willReturnCallback(fn () => $this->assertNotInTransaction()); + $backend->expects($this->exactly($updateCalls))->method('updateCard') + ->willReturnCallback(fn () => $this->assertNotInTransaction()); $backend->expects($this->exactly($deleteCalls))->method('deleteCard'); $backend->method('getCard')->willReturnOnConsecutiveCalls(false, [ @@ -435,6 +457,7 @@ public function testUpdateAndDeleteUser(bool $activated, int $createCalls, int $ $userManager = $this->createMock(IUserManager::class); $dbConnection = $this->createMock(IDBConnection::class); + $this->trackTransactionDepth($dbConnection); $user = $this->createMock(IUser::class); $user->method('getBackendClassName')->willReturn('unittest'); $user->method('getUID')->willReturn('test-user'); @@ -444,7 +467,10 @@ public function testUpdateAndDeleteUser(bool $activated, int $createCalls, int $ $converter = $this->createMock(Converter::class); $converter->expects($this->any()) ->method('createCardFromUser') - ->willReturn($this->createMock(VCard::class)); + ->willReturnCallback(function () { + $this->assertNotInTransaction(); + return $this->createMock(VCard::class); + }); $clientService = $this->createMock(IClientService::class); $config = $this->createMock(IConfig::class);