Skip to content

fix(dav): avoid nesting transactions in sync services #65937

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
SebastianKrupinski wants to merge 1 commit into from
+143 −72
Open

fix(dav): avoid nesting transactions in sync services #65937

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion 3rdparty
Open in desktop
Submodule 3rdparty updated from 9a181f to 4b8c97
39 changes: 16 additions & 23 deletions apps/dav/lib/CalDAV/Federation/FederatedCalendarSyncService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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);
Expand Down Expand Up @@ -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
Expand Down
19 changes: 6 additions & 13 deletions apps/dav/lib/CalDAV/SyncService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
47 changes: 21 additions & 26 deletions apps/dav/lib/CardDAV/SyncService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
}
Expand Down
72 changes: 68 additions & 4 deletions apps/dav/tests/unit/CalDAV/Federation/FederatedCalendarSyncServiceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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);
Expand All @@ -53,7 +50,6 @@ protected function setUp(): void {
$this->federatedCalendarMapper,
$this->logger,
$this->backend,
$this->dbConnection,
$this->cloudIdManager,
);
}
Expand Down Expand Up @@ -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('<?xml version="1.0"?><d:multistatus xmlns:d="DAV:"><d:response><d:href>/remote.php/dav/remote-calendars/abcdef123/cal1_shared_by_user2/event1.ics</d:href><d:propstat><d:prop><d:getetag>"abc"</d:getetag></d:prop><d:status>HTTP/1.1 200 OK</d:status></d:propstat></d:response><d:sync-token>http://sabre.io/ns/sync/101</d:sync-token></d:multistatus>');
$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/'],
Expand Down
36 changes: 31 additions & 5 deletions apps/dav/tests/unit/CardDAV/SyncServiceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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);
Expand All @@ -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');
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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');

Expand Down Expand Up @@ -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, [
Expand All @@ -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');
Expand All @@ -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);
Expand Down
Loading