diff --git a/apps/files/lib/Listener/SyncLivePhotosListener.php b/apps/files/lib/Listener/SyncLivePhotosListener.php index 4ad4a44b597d3..552dcea792552 100644 --- a/apps/files/lib/Listener/SyncLivePhotosListener.php +++ b/apps/files/lib/Listener/SyncLivePhotosListener.php @@ -58,22 +58,24 @@ public function handle(Event $event): void { if ($event instanceof BeforeNodeCopiedEvent || $event instanceof NodeCopiedEvent) { $this->handleCopyRecursive($event, $event->getSource(), $event->getTarget()); } else { - $peerFileId = null; - + $node = null; if ($event instanceof BeforeNodeRenamedEvent) { - $peerFileId = $this->livePhotosService->getLivePhotoPeerId($event->getSource()); + $node = $event->getSource(); } elseif ($event instanceof BeforeNodeDeletedEvent) { - $peerFileId = $this->livePhotosService->getLivePhotoPeerId($event->getNode()); + $node = $event->getNode(); } elseif ($event instanceof CacheEntriesRemovedEvent) { $this->handleCacheEntriesRemovedEvent($event); + return; } + $peerFileId = $this->livePhotosService->getLivePhotoPeerId($node); + if ($peerFileId === null) { return; // Not a live photo. } // Check the user's folder. - $peerFile = $this->userFolder->getFirstNodeById($peerFileId); + $peerFile = $this->getPeerById($node, $peerFileId); if ($peerFile === null) { return; // Peer file not found. @@ -88,22 +90,51 @@ public function handle(Event $event): void { } } + /** + * Look for a peer file in the same directory as a file. + * + * If it's in a different folder, we consider the peer-relationship invalid + */ + private function getPeerById(Node $node, int $peerId): ?Node { + $parent = $node->getParent(); + foreach ($parent->getDirectoryListing() as $sibling) { + if ($sibling->getId() === $peerId) { + return $sibling; + } + } + return null; + } + public function handleCacheEntriesRemovedEvent(CacheEntriesRemovedEvent $cacheEntriesRemovedEvent): void { $entries = $cacheEntriesRemovedEvent->getCacheEntryRemovedEvents(); $fileIds = []; + $eventsById = []; foreach ($entries as $entry) { $fileIds[] = $entry->getFileId(); + $eventsById[$entry->getFileId()] = $entry; } $peerFileIds = $this->livePhotosService->getLivePhotoPeerIds($fileIds); - foreach ($peerFileIds as $peerFileId) { + foreach ($peerFileIds as $sourceFileId => $peerFileId) { + if (in_array($peerFileId, $this->pendingRenames, true)) { + continue; // The file is moved to another storage along with its peer. + } + // Check the user's folder. $peerFile = $this->userFolder->getFirstNodeById($peerFileId); + $sourceFile = $eventsById[$sourceFileId]; if ($peerFile === null) { continue; // Peer file not found. } + + if ( + $sourceFile->getStorageId() !== $peerFile->getData()->getStorageId() + || dirname($sourceFile->getPath()) !== dirname($peerFile->getInternalPath()) + ) { + continue; // Peer file is not in the same folder as the deleted file + } $peerFile->delete(); } } @@ -251,7 +282,7 @@ private function handleCopyRecursive(Event $event, Node $sourceNode, Node $targe if ($peerFileId === null) { return; } - $peerFile = $this->userFolder->getFirstNodeById($peerFileId); + $peerFile = $this->getPeerById($sourceNode, $peerFileId); if ($peerFile === null) { return; } diff --git a/apps/files/lib/Service/LivePhotosService.php b/apps/files/lib/Service/LivePhotosService.php index 608e08bf478bc..793c66266d5f7 100644 --- a/apps/files/lib/Service/LivePhotosService.php +++ b/apps/files/lib/Service/LivePhotosService.php @@ -22,6 +22,21 @@ public function __construct( * Get the associated live photo for a given file id */ public function getLivePhotoPeerId(Node $file): ?int { + $peer = $this->getLivePhotoPeerIdUnchecked($file); + if ($peer === null) { + return null; + } + + $peerOfPeer = $this->getLivePhotoPeerIdsUnchecked([$peer])[$peer] ?? null; + if ($peerOfPeer === $file->getId()) { + // the peer relationship is only valid if it's mutual + return $peer; + } else { + return null; + } + } + + private function getLivePhotoPeerIdUnchecked(Node $file): ?int { try { $metadata = $this->filesMetadataManager->getMetadataForNode($file); } catch (FilesMetadataNotFoundException $ex) { @@ -38,17 +53,25 @@ public function getLivePhotoPeerId(Node $file): ?int { /** * Get the associated live photo for multiple file ids * @param int[] $fileIds - * @return int[] + * @return array */ public function getLivePhotoPeerIds(array $fileIds): array { + $peersIds = $this->getLivePhotoPeerIdsUnchecked($fileIds); + $peersOfPeers = $this->getLivePhotoPeerIdsUnchecked($peersIds); + + // the peer relationship is only valid if it's mutual + return array_filter($peersIds, fn (int $peerId, int $fileId) => ($peersOfPeers[$peerId] ?? null) === $fileId, ARRAY_FILTER_USE_BOTH); + } + + private function getLivePhotoPeerIdsUnchecked(array $fileIds): array { $metadata = $this->filesMetadataManager->getMetadataForFiles($fileIds); $peersIds = []; - foreach ($metadata as $item) { + foreach ($metadata as $fileId => $item) { if (!$item->hasKey('files-live-photo')) { continue; } - $peersIds[] = (int)$item->getString('files-live-photo'); + $peersIds[$fileId] = (int)$item->getString('files-live-photo'); } return $peersIds; } diff --git a/apps/files/tests/Listener/SyncLivePhotosListenerTest.php b/apps/files/tests/Listener/SyncLivePhotosListenerTest.php index b57ec6cd9a512..5c68ca6dcc7fb 100644 --- a/apps/files/tests/Listener/SyncLivePhotosListenerTest.php +++ b/apps/files/tests/Listener/SyncLivePhotosListenerTest.php @@ -12,11 +12,13 @@ use OCA\Files\Listener\SyncLivePhotosListener; use OCA\Files\Service\LivePhotosService; use OCP\Files\Cache\CacheEntriesRemovedEvent; +use OCP\Files\Cache\ICacheEntry; use OCP\Files\Cache\ICacheEvent; use OCP\Files\File; use OCP\Files\Folder; use OCP\Files\IRootFolder; use OCP\FilesMetadata\IFilesMetadataManager; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; @@ -41,18 +43,35 @@ protected function setUp(): void { ); } + private function createCacheEvent(int $fileId, int $storageId, string $path): ICacheEvent { + $entry = $this->createMock(ICacheEvent::class); + $entry->method('getFileId')->willReturn($fileId); + $entry->method('getStorageId')->willReturn($storageId); + $entry->method('getPath')->willReturn($path); + return $entry; + } + + private function createPeer(int $storageId, string $internalPath): File&MockObject { + $cacheEntry = $this->createMock(ICacheEntry::class); + $cacheEntry->method('getStorageId')->willReturn($storageId); + + $peer = $this->createMock(File::class); + $peer->method('getData')->willReturn($cacheEntry); + $peer->method('getInternalPath')->willReturn($internalPath); + return $peer; + } + public function testCacheEntriesRemovedDeletesPeersAfterMissingOne(): void { - $entries = array_map(function (int $fileId): ICacheEvent { - $entry = $this->createMock(ICacheEvent::class); - $entry->method('getFileId')->willReturn($fileId); - return $entry; - }, [1, 2]); + $entries = [ + $this->createCacheEvent(1, 10, 'files/IMG_1.jpg'), + $this->createCacheEvent(2, 10, 'files/IMG_2.jpg'), + ]; $this->livePhotosService->method('getLivePhotoPeerIds') ->with([1, 2]) - ->willReturn([11, 12]); + ->willReturn([1 => 11, 2 => 12]); - $peer = $this->createMock(File::class); + $peer = $this->createPeer(10, 'files/IMG_2.mov'); $peer->expects($this->once())->method('delete'); $this->userFolder->method('getFirstNodeById') @@ -63,4 +82,37 @@ public function testCacheEntriesRemovedDeletesPeersAfterMissingOne(): void { $this->listener->handle(new CacheEntriesRemovedEvent($entries)); } + + public static function dataCacheEntriesRemovedKeepsPeer(): array { + return [ + 'other folder' => [10, 'files/videos/IMG.mov'], + 'other folder with the same name' => [10, 'files/other/photos/IMG.mov'], + 'other storage' => [20, 'files/photos/IMG.mov'], + ]; + } + + #[DataProvider('dataCacheEntriesRemovedKeepsPeer')] + public function testCacheEntriesRemovedKeepsPeer(int $peerStorageId, string $peerPath): void { + $this->livePhotosService->method('getLivePhotoPeerIds') + ->willReturn([1 => 11]); + + $peer = $this->createPeer($peerStorageId, $peerPath); + $peer->expects($this->never())->method('delete'); + $this->userFolder->method('getFirstNodeById')->willReturn($peer); + + $this->listener->handle(new CacheEntriesRemovedEvent([ + $this->createCacheEvent(1, 10, 'files/photos/IMG.jpg'), + ])); + } + + public function testCacheEntriesRemovedKeepsPeerThatIsBeingMoved(): void { + $this->livePhotosService->method('getLivePhotoPeerIds') + ->willReturn([1 => 11]); + $this->userFolder->expects($this->never())->method('getFirstNodeById'); + + self::invokePrivate($this->listener, 'pendingRenames', [[11]]); + $this->listener->handle(new CacheEntriesRemovedEvent([ + $this->createCacheEvent(1, 10, 'files/IMG.jpg'), + ])); + } } diff --git a/apps/files/tests/Service/LivePhotosServiceTest.php b/apps/files/tests/Service/LivePhotosServiceTest.php new file mode 100644 index 0000000000000..b9f82f7770bbf --- /dev/null +++ b/apps/files/tests/Service/LivePhotosServiceTest.php @@ -0,0 +1,76 @@ +createMock(IFilesMetadataManager::class); + $manager->method('getMetadataForNode') + ->willReturnCallback(function (Node $node) use ($peers) { + $id = $node->getId(); + $metadata = new FilesMetadata($id); + if (isset($peers[$id])) { + $metadata->setString('files-live-photo', (string)$peers[$id]); + } + return $metadata; + }); + $manager->method('getMetadataForFiles') + ->willReturnCallback(function (array $ids) use ($peers) { + $result = []; + foreach ($ids as $id) { + $metadata = new FilesMetadata($id); + if (isset($peers[$id])) { + $metadata->setString('files-live-photo', (string)$peers[$id]); + } + $result[$id] = $metadata; + } + return $result; + }); + return $manager; + } + + public static function getPeerIdProvider(): array { + return [ + [[1 => 2, 2 => 1], 1, 2], + [[1 => 2], 1, null], + [[1 => 2], 2, null], + ]; + } + + #[DataProvider('getPeerIdProvider')] + public function testGetPeerId(array $peers, int $fileId, ?int $expected): void { + $metadataManager = $this->getMetadataManager($peers); + $livePhotoService = new LivePhotosService($metadataManager); + $file = $this->createMock(Node::class); + $file->method('getId')->willReturn($fileId); + $this->assertEquals($expected, $livePhotoService->getLivePhotoPeerId($file)); + } + + public static function getPeerIdsProvider(): array { + return [ + [[1 => 2, 2 => 1], [1], [1 => 2]], + [[1 => 2], [1], []], + [[1 => 2], [2], []], + ]; + } + + #[DataProvider('getPeerIdsProvider')] + public function testGetPeerIds(array $peers, array $fileIds, array $expected): void { + $metadataManager = $this->getMetadataManager($peers); + $livePhotoService = new LivePhotosService($metadataManager); + $this->assertEquals($expected, $livePhotoService->getLivePhotoPeerIds($fileIds)); + } +} diff --git a/build/integration/features/bootstrap/MetadataContext.php b/build/integration/features/bootstrap/MetadataContext.php index 1a60a24ce1d6e..8d9d10ae8a4dc 100644 --- a/build/integration/features/bootstrap/MetadataContext.php +++ b/build/integration/features/bootstrap/MetadataContext.php @@ -6,6 +6,7 @@ */ use Behat\Behat\Context\Context; +use Behat\Step\Given; use Behat\Step\Then; use Behat\Step\When; use PHPUnit\Framework\Assert; @@ -50,6 +51,24 @@ public function userSetsProp(string $user, string $metadataKey, string $metadata $client->request('PROPPATCH', $this->baseUrl . $davUrl, $body); } + #[Given('User :user links :fileName as live photo to :peerName')] + public function userLinksLivePhoto(string $user, string $fileName, string $peerName) { + $client = new SClient([ + 'baseUri' => $this->baseUrl, + 'userName' => $user, + 'password' => '123456', + 'authType' => SClient::AUTH_BASIC, + ]); + + $response = $client->propFind($this->baseUrl . $this->getDavUrl($user, $peerName), ['{http://owncloud.org/ns}fileid']); + $peerId = $response['{http://owncloud.org/ns}fileid']; + + $response = $client->propPatch($this->baseUrl . $this->getDavUrl($user, $fileName), [ + '{http://nextcloud.org/ns}metadata-files-live-photo' => (string)$peerId, + ]); + Assert::assertTrue($response); + } + #[When('User :user deletes the :metadataKey prop on :fileName')] public function userDeletesProp(string $user, string $metadataKey, string $fileName) { $client = new SClient([ diff --git a/build/integration/files_features/live-photos.feature b/build/integration/files_features/live-photos.feature new file mode 100644 index 0000000000000..d104dd1ab50fa --- /dev/null +++ b/build/integration/files_features/live-photos.feature @@ -0,0 +1,91 @@ +# SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors +# SPDX-License-Identifier: AGPL-3.0-only +Feature: live photos + Background: + Given using new dav path + And user "user0" exists + + Scenario: Deleting a live photo deletes its video + Given User "user0" uploads file with content "photo" to "/IMG.jpg" + And User "user0" uploads file with content "video" to "/IMG.mov" + And User "user0" links "/IMG.jpg" as live photo to "/IMG.mov" + And User "user0" links "/IMG.mov" as live photo to "/IMG.jpg" + When User "user0" deletes file "/IMG.jpg" + Then the HTTP status code should be "204" + And as "user0" the file "/IMG.mov" does not exist + + Scenario: Deleting a live photo keeps a linked video in another folder + Given User "user0" created a folder "/photos" + And User "user0" created a folder "/videos" + And User "user0" uploads file with content "photo" to "/photos/IMG.jpg" + And User "user0" uploads file with content "video" to "/videos/IMG.mov" + And User "user0" links "/photos/IMG.jpg" as live photo to "/videos/IMG.mov" + And User "user0" links "/videos/IMG.mov" as live photo to "/photos/IMG.jpg" + When User "user0" deletes file "/photos/IMG.jpg" + Then the HTTP status code should be "204" + And as "user0" the file "/videos/IMG.mov" exists + + Scenario: Renaming a live photo keeps a linked video in another folder + Given User "user0" created a folder "/photos" + And User "user0" created a folder "/videos" + And User "user0" uploads file with content "photo" to "/photos/IMG.jpg" + And User "user0" uploads file with content "video" to "/videos/IMG.mov" + And User "user0" links "/photos/IMG.jpg" as live photo to "/videos/IMG.mov" + And User "user0" links "/videos/IMG.mov" as live photo to "/photos/IMG.jpg" + When User "user0" moves file "/photos/IMG.jpg" to "/photos/renamed.jpg" + Then the HTTP status code should be "201" + And as "user0" the file "/videos/IMG.mov" exists + And as "user0" the file "/photos/renamed.mov" does not exist + + Scenario: Copying a live photo does not copy a linked video in another folder + Given User "user0" created a folder "/photos" + And User "user0" created a folder "/videos" + And User "user0" uploads file with content "photo" to "/photos/IMG.jpg" + And User "user0" uploads file with content "video" to "/videos/IMG.mov" + And User "user0" links "/photos/IMG.jpg" as live photo to "/videos/IMG.mov" + And User "user0" links "/videos/IMG.mov" as live photo to "/photos/IMG.jpg" + When User "user0" copies file "/photos/IMG.jpg" to "/copy.jpg" + Then the HTTP status code should be "201" + And as "user0" the file "/copy.mov" does not exist + + Scenario: Deleting a photo keeps a video that does not link back to it + Given User "user0" uploads file with content "photo" to "/IMG.jpg" + And User "user0" uploads file with content "video" to "/IMG.mov" + And User "user0" links "/IMG.jpg" as live photo to "/IMG.mov" + When User "user0" deletes file "/IMG.jpg" + Then the HTTP status code should be "204" + And as "user0" the file "/IMG.mov" exists + + Scenario: Deleting a video is allowed when the photo does not link back to it + Given User "user0" uploads file with content "photo" to "/IMG.jpg" + And User "user0" uploads file with content "video" to "/IMG.mov" + And User "user0" links "/IMG.mov" as live photo to "/IMG.jpg" + When User "user0" deletes file "/IMG.mov" + Then the HTTP status code should be "204" + And as "user0" the file "/IMG.jpg" exists + + Scenario: Moving a live photo into another storage moves its video along + Given User "user0" uploads file with content "photo" to "/IMG.jpg" + And User "user0" uploads file with content "video" to "/IMG.mov" + And User "user0" links "/IMG.jpg" as live photo to "/IMG.mov" + And User "user0" links "/IMG.mov" as live photo to "/IMG.jpg" + And User "user0" created a folder "/local_storage/live-photos" + When User "user0" moves file "/IMG.jpg" to "/local_storage/live-photos/IMG.jpg" + Then the HTTP status code should be "201" + And as "user0" the file "/local_storage/live-photos/IMG.jpg" exists + And as "user0" the file "/local_storage/live-photos/IMG.mov" exists + And as "user0" the file "/IMG.mov" does not exist + And User "user0" deletes folder "/local_storage/live-photos" + + Scenario: Moving a live photo video into another storage moves its photo along + Given User "user0" uploads file with content "photo" to "/IMG.jpg" + And User "user0" uploads file with content "video" to "/IMG.mov" + And User "user0" links "/IMG.jpg" as live photo to "/IMG.mov" + And User "user0" links "/IMG.mov" as live photo to "/IMG.jpg" + And User "user0" created a folder "/local_storage/live-photos" + When User "user0" moves file "/IMG.mov" to "/local_storage/live-photos/IMG.mov" + Then the HTTP status code should be "201" + And as "user0" the file "/local_storage/live-photos/IMG.mov" exists + And as "user0" the file "/local_storage/live-photos/IMG.jpg" exists + And as "user0" the file "/IMG.jpg" does not exist + And User "user0" deletes folder "/local_storage/live-photos"