From 78058e5572e5581bc4cab5c5a9f7a8b3ae7cf70e Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Wed, 17 Apr 2024 14:19:25 +0200 Subject: [PATCH 1/7] fix(FileListener): Listen to file movements across share boundaries Signed-off-by: Marcel Klehr --- lib/AppInfo/Application.php | 6 +++ lib/Hooks/FileListener.php | 84 ++++++++++++++++++++++++++++++++++++- 2 files changed, 88 insertions(+), 2 deletions(-) diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 97cefa71..92145937 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -21,6 +21,7 @@ use OCP\Files\Events\Node\NodeDeletedEvent; use OCP\Files\Events\Node\NodeRenamedEvent; use OCP\Files\Events\NodeRemovedFromCache; +use OCP\Files\IRootFolder; use OCP\Share\Events\ShareCreatedEvent; use OCP\Share\Events\ShareDeletedEvent; @@ -42,6 +43,11 @@ public function __construct() { $dispatcher->addServiceListener(ShareDeletedEvent::class, FileListener::class); $dispatcher->addServiceListener(CacheEntryInsertedEvent::class, FileListener::class); $dispatcher->addServiceListener(NodeRemovedFromCache::class, FileListener::class); + $rootFolder = $this->getContainer()->get(IRootFolder::class); + $rootFolder->listen('\OC\Files', 'postRename', function ($source, $target) { + $fileListener = $this->getContainer()->get(FileListener::class); + $fileListener->postRename($source, $target); + }); } public function register(IRegistrationContext $context): void { diff --git a/lib/Hooks/FileListener.php b/lib/Hooks/FileListener.php index a7a663be..a2682e8f 100644 --- a/lib/Hooks/FileListener.php +++ b/lib/Hooks/FileListener.php @@ -39,7 +39,7 @@ use Psr\Log\LoggerInterface; /** - * @psalm-implements IEventListener + * @template-implements IEventListener */ class FileListener implements IEventListener { private FaceDetectionMapper $faceDetectionMapper; @@ -73,7 +73,7 @@ public function handle(Event $event): void { $node = $share->getNode(); $accessList = $this->shareManager->getAccessList($node, true, true); - $userIds = array_map(fn (int|string $id) => (string)$id, array_keys($accessList['users'])); + $userIds = array_map(fn ($id) => strval($id), array_keys($accessList['users'])); if ($node->getType() === FileInfo::TYPE_FOLDER) { $mount = $node->getMountPoint(); @@ -315,6 +315,86 @@ public function postInsert(Node $node, bool $recurse = true): void { } } + public function preRename(Node $source, Node $target): void { + $sourceAccessList = $this->shareManager->getAccessList($source, true, true); + $sourceUserIds = array_map(fn ($id) => strval($id), array_keys($sourceAccessList['users'])); + $targetAccessList = $this->shareManager->getAccessList($target, true, true); + $targetUserIds = array_map(fn ($id) => strval($id), array_keys($targetAccessList['users'])); + + $usersToRemove = array_diff($sourceUserIds, $targetUserIds); + + if ($target->getType() === FileInfo::TYPE_FOLDER) { + $mount = $target->getMountPoint(); + if ($mount->getNumericStorageId() === null) { + return; + } + $files = $this->storageService->getFilesInMount($mount->getNumericStorageId(), $source->getId(), [ClusteringFaceClassifier::MODEL_NAME], 0, 0); + foreach ($files as $fileInfo) { + $this->faceDetectionMapper->removeDetectionsForFileFromUsersNotInList($fileInfo['fileid'], $usersToRemove); + } + } else { + $this->faceDetectionMapper->removeDetectionsForFileFromUsersNotInList($source->getId(), $usersToRemove); + } + } + + public function postRename(Node $source, Node $target): void { + if (in_array($source->getName(), [...Constants::IGNORE_MARKERS_ALL, ...Constants::IGNORE_MARKERS_IMAGE, ...Constants::IGNORE_MARKERS_AUDIO, ...Constants::IGNORE_MARKERS_VIDEO], true) && + !in_array($target->getName(), [...Constants::IGNORE_MARKERS_ALL, ...Constants::IGNORE_MARKERS_IMAGE, ...Constants::IGNORE_MARKERS_AUDIO, ...Constants::IGNORE_MARKERS_VIDEO], true)) { + $this->resetIgnoreCache($source); + $this->postInsert($target->getParent()); + return; + } + + if (!in_array($source->getName(), [...Constants::IGNORE_MARKERS_ALL, ...Constants::IGNORE_MARKERS_IMAGE, ...Constants::IGNORE_MARKERS_AUDIO, ...Constants::IGNORE_MARKERS_VIDEO], true) && + in_array($target->getName(), [...Constants::IGNORE_MARKERS_ALL, ...Constants::IGNORE_MARKERS_IMAGE, ...Constants::IGNORE_MARKERS_AUDIO, ...Constants::IGNORE_MARKERS_VIDEO], true)) { + $this->resetIgnoreCache($target); + $this->postDelete($target->getParent()); + return; + } + + if ($this->isIgnored($target)) { + $this->postDelete($target); + return; + } + if ($this->isIgnored($source) && !$this->isIgnored($target)) { + $this->postInsert($target); + return; + } + + $sourceAccessList = $this->shareManager->getAccessList($source, true, true); + $sourceUserIds = array_map(fn ($id) => strval($id), array_keys($sourceAccessList['users'])); + $targetAccessList = $this->shareManager->getAccessList($target, true, true); + $targetUserIds = array_map(fn ($id) => strval($id), array_keys($targetAccessList['users'])); + + $usersToAdd = array_diff($targetUserIds, $sourceUserIds); + $existingUsers = array_diff($targetUserIds, $usersToAdd); + // *handwaving* I know this is a stretch but it's good enough + $ownerId = $existingUsers[0]; + + if ($target->getType() === FileInfo::TYPE_FOLDER) { + $mount = $target->getMountPoint(); + if ($mount->getNumericStorageId() === null) { + return; + } + $files = $this->storageService->getFilesInMount($mount->getNumericStorageId(), $target->getId(), [ClusteringFaceClassifier::MODEL_NAME], 0, 0); + foreach ($files as $fileInfo) { + foreach ($usersToAdd as $userId) { + if (count($this->faceDetectionMapper->findByFileIdAndUser($target->getId(), $userId)) > 0) { + continue; + } + $this->faceDetectionMapper->copyDetectionsForFileFromUserToUser($fileInfo['fileid'], $ownerId, $userId); + } + } + } else { + foreach ($usersToAdd as $userId) { + if (count($this->faceDetectionMapper->findByFileIdAndUser($target->getId(), $userId)) > 0) { + continue; + } + $this->faceDetectionMapper->copyDetectionsForFileFromUserToUser($target->getId(), $ownerId, $userId); + } + } + } + /** * @param \OCP\Files\Node $node * @return bool From 745fa4401a9f3b38788df43e81ef4b974dfbbb88 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Wed, 17 Apr 2024 14:50:30 +0200 Subject: [PATCH 2/7] First try at a test --- tests/ClassifierTest.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/ClassifierTest.php b/tests/ClassifierTest.php index 75082842..150fd6b2 100644 --- a/tests/ClassifierTest.php +++ b/tests/ClassifierTest.php @@ -22,6 +22,7 @@ use OCA\Recognize\Service\QueueService; use OCP\AppFramework\Services\IAppConfig; use OCP\BackgroundJob\IJobList; +use OCP\Constants; use OCP\Files\Folder; use OCP\Files\IRootFolder; use OCP\SystemTag\ISystemTagObjectMapper; @@ -32,6 +33,7 @@ */ class ClassifierTest extends TestCase { public const TEST_USER1 = 'test-user1'; + public const TEST_USER2 = 'test-user2'; public const TEST_FILES = ['alpine.jpg' ,'eiffeltower.jpg', 'Rock_Rejam.mp3', 'jumpingjack.gif', 'test']; public const ALL_MODELS = [ @@ -58,6 +60,7 @@ class ClassifierTest extends TestCase { private FaceDetectionMapper $faceDetectionMapper; private IJobList $jobList; private IAppConfig $config; + private \OCP\Share\IManager $shareManager; public static function setUpBeforeClass(): void { parent::setUpBeforeClass(); @@ -79,6 +82,7 @@ public function setUp(): void { $this->jobList = \OC::$server->get(IJobList::class); $this->config = \OC::$server->getRegisteredAppContainer('recognize')->get(IAppConfig::class); $this->queue = \OC::$server->get(QueueService::class); + $this->shareManager = \OC::$server->get(\OCP\Share\IManager::class); foreach (self::TEST_FILES as $filename) { try { $this->userFolder->get($filename)->delete(); @@ -143,6 +147,14 @@ public function testFileListener(string $ignoreFileName) : void { $this->testFile = $this->userFolder->newFile('/alpine.jpg', file_get_contents(__DIR__.'/res/alpine.JPG')); $this->userFolder->newFolder('/test/ignore/'); + $sharedFolder = $this->userFolder->newFolder('/test/shared/'); + $share = $this->shareManager->newShare(); + $share->setSharedBy(self::TEST_USER1); + $share->setSharedWith(self::TEST_USER2); + $share->setShareType(\OCP\Share\IShare::TYPE_USER); + $share->setNode($sharedFolder); + $share->setPermissions(Constants::PERMISSION_ALL); + $this->shareManager->createShare($share); $ignoreFile = $this->userFolder->newFile('/test/' . $ignoreFileName, ''); $this->ignoredFile = $this->userFolder->newFile('/test/ignore/alpine-2.jpg', file_get_contents(__DIR__.'/res/alpine.JPG')); From 7071cc48fb881b7ef42258e91be78a9a149a045f Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Wed, 17 Apr 2024 18:49:33 +0200 Subject: [PATCH 3/7] fix: File listener - Catch all errors - Don't access nodes that are non-existent Signed-off-by: Marcel Klehr --- lib/AppInfo/Application.php | 6 - lib/Hooks/FileListener.php | 312 ++++++++++++++++-------------------- tests/ClassifierTest.php | 28 ++-- 3 files changed, 161 insertions(+), 185 deletions(-) diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 92145937..97cefa71 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -21,7 +21,6 @@ use OCP\Files\Events\Node\NodeDeletedEvent; use OCP\Files\Events\Node\NodeRenamedEvent; use OCP\Files\Events\NodeRemovedFromCache; -use OCP\Files\IRootFolder; use OCP\Share\Events\ShareCreatedEvent; use OCP\Share\Events\ShareDeletedEvent; @@ -43,11 +42,6 @@ public function __construct() { $dispatcher->addServiceListener(ShareDeletedEvent::class, FileListener::class); $dispatcher->addServiceListener(CacheEntryInsertedEvent::class, FileListener::class); $dispatcher->addServiceListener(NodeRemovedFromCache::class, FileListener::class); - $rootFolder = $this->getContainer()->get(IRootFolder::class); - $rootFolder->listen('\OC\Files', 'postRename', function ($source, $target) { - $fileListener = $this->getContainer()->get(FileListener::class); - $fileListener->postRename($source, $target); - }); } public function register(IRegistrationContext $context): void { diff --git a/lib/Hooks/FileListener.php b/lib/Hooks/FileListener.php index a2682e8f..d15d25f1 100644 --- a/lib/Hooks/FileListener.php +++ b/lib/Hooks/FileListener.php @@ -20,7 +20,6 @@ use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventListener; use OCP\Files\Cache\CacheEntryInsertedEvent; -use OCP\Files\Config\IUserMountCache; use OCP\Files\Events\Node\BeforeNodeDeletedEvent; use OCP\Files\Events\Node\BeforeNodeRenamedEvent; use OCP\Files\Events\Node\NodeCreatedEvent; @@ -49,11 +48,11 @@ class FileListener implements IEventListener { private ?bool $movingFromIgnoredTerritory; private StorageService $storageService; private IManager $shareManager; - private IUserMountCache $userMountCache; private IRootFolder $rootFolder; - private array $nodeCache; + /** @var list */ + private array $sourceUserIds; - public function __construct(FaceDetectionMapper $faceDetectionMapper, LoggerInterface $logger, QueueService $queue, IgnoreService $ignoreService, StorageService $storageService, IManager $shareManager, IUserMountCache $userMountCache, IRootFolder $rootFolder) { + public function __construct(FaceDetectionMapper $faceDetectionMapper, LoggerInterface $logger, QueueService $queue, IgnoreService $ignoreService, StorageService $storageService, IManager $shareManager, IRootFolder $rootFolder) { $this->faceDetectionMapper = $faceDetectionMapper; $this->logger = $logger; $this->queue = $queue; @@ -61,27 +60,37 @@ public function __construct(FaceDetectionMapper $faceDetectionMapper, LoggerInte $this->movingFromIgnoredTerritory = null; $this->storageService = $storageService; $this->shareManager = $shareManager; - $this->userMountCache = $userMountCache; $this->rootFolder = $rootFolder; - $this->nodeCache = []; } public function handle(Event $event): void { - if ($event instanceof ShareCreatedEvent) { - $share = $event->getShare(); - $ownerId = $share->getShareOwner(); - $node = $share->getNode(); - - $accessList = $this->shareManager->getAccessList($node, true, true); - $userIds = array_map(fn ($id) => strval($id), array_keys($accessList['users'])); - - if ($node->getType() === FileInfo::TYPE_FOLDER) { - $mount = $node->getMountPoint(); - if ($mount->getNumericStorageId() === null) { - return; - } - $files = $this->storageService->getFilesInMount($mount->getNumericStorageId(), $node->getId(), [ClusteringFaceClassifier::MODEL_NAME], 0, 0); - foreach ($files as $fileInfo) { + try { + if ($event instanceof ShareCreatedEvent) { + $share = $event->getShare(); + $ownerId = $share->getShareOwner(); + $node = $share->getNode(); + + $accessList = $this->shareManager->getAccessList($node, true, true); + $userIds = array_map(fn ($id) => strval($id), array_keys($accessList['users'])); + + if ($node->getType() === FileInfo::TYPE_FOLDER) { + $mount = $node->getMountPoint(); + if ($mount->getNumericStorageId() === null) { + return; + } + $files = $this->storageService->getFilesInMount($mount->getNumericStorageId(), $node->getId(), [ClusteringFaceClassifier::MODEL_NAME], 0, 0); + foreach ($files as $fileInfo) { + foreach ($userIds as $userId) { + if ($userId === $ownerId) { + continue; + } + if (count($this->faceDetectionMapper->findByFileIdAndUser($node->getId(), $userId)) > 0) { + continue; + } + $this->faceDetectionMapper->copyDetectionsForFileFromUserToUser($fileInfo['fileid'], $ownerId, $userId); + } + } + } else { foreach ($userIds as $userId) { if ($userId === $ownerId) { continue; @@ -89,130 +98,138 @@ public function handle(Event $event): void { if (count($this->faceDetectionMapper->findByFileIdAndUser($node->getId(), $userId)) > 0) { continue; } - $this->faceDetectionMapper->copyDetectionsForFileFromUserToUser($fileInfo['fileid'], $ownerId, $userId); + $this->faceDetectionMapper->copyDetectionsForFileFromUserToUser($node->getId(), $ownerId, $userId); } } - } else { - foreach ($userIds as $userId) { - if ($userId === $ownerId) { - continue; + } + if ($event instanceof ShareDeletedEvent) { + $share = $event->getShare(); + $node = $share->getNode(); + + $accessList = $this->shareManager->getAccessList($node, true, true); + /** + * @var string[] $userIds + */ + $userIds = array_keys($accessList['users']); + + if ($node->getType() === FileInfo::TYPE_FOLDER) { + $mount = $node->getMountPoint(); + if ($mount->getNumericStorageId() === null) { + return; } - if (count($this->faceDetectionMapper->findByFileIdAndUser($node->getId(), $userId)) > 0) { - continue; + $files = $this->storageService->getFilesInMount($mount->getNumericStorageId(), $node->getId(), [ClusteringFaceClassifier::MODEL_NAME], 0, 0); + foreach ($files as $fileInfo) { + $this->faceDetectionMapper->removeDetectionsForFileFromUsersNotInList($fileInfo['fileid'], $userIds); } - $this->faceDetectionMapper->copyDetectionsForFileFromUserToUser($node->getId(), $ownerId, $userId); + } else { + $this->faceDetectionMapper->removeDetectionsForFileFromUsersNotInList($node->getId(), $userIds); } } - } - if ($event instanceof ShareDeletedEvent) { - $share = $event->getShare(); - $node = $share->getNode(); - - $accessList = $this->shareManager->getAccessList($node, true, true); - /** - * @var string[] $userIds - */ - $userIds = array_keys($accessList['users']); - - if ($node->getType() === FileInfo::TYPE_FOLDER) { - $mount = $node->getMountPoint(); - if ($mount->getNumericStorageId() === null) { + if ($event instanceof BeforeNodeRenamedEvent) { + if (in_array($event->getSource()->getName(), [...Constants::IGNORE_MARKERS_ALL, ...Constants::IGNORE_MARKERS_IMAGE, ...Constants::IGNORE_MARKERS_AUDIO, ...Constants::IGNORE_MARKERS_VIDEO], true)) { + $this->resetIgnoreCache($event->getSource()); return; } - $files = $this->storageService->getFilesInMount($mount->getNumericStorageId(), $node->getId(), [ClusteringFaceClassifier::MODEL_NAME], 0, 0); - foreach ($files as $fileInfo) { - $this->faceDetectionMapper->removeDetectionsForFileFromUsersNotInList($fileInfo['fileid'], $userIds); + // We try to remember whether the source node is in ignored territory + // because after moving isIgnored doesn't work anymore :( + if ($this->isIgnored($event->getSource())) { + $this->movingFromIgnoredTerritory = true; + } else { + $this->movingFromIgnoredTerritory = false; } - } else { - $this->faceDetectionMapper->removeDetectionsForFileFromUsersNotInList($node->getId(), $userIds); - } - } - if ($event instanceof BeforeNodeRenamedEvent) { - if (in_array($event->getSource()->getName(), [...Constants::IGNORE_MARKERS_ALL, ...Constants::IGNORE_MARKERS_IMAGE, ...Constants::IGNORE_MARKERS_AUDIO, ...Constants::IGNORE_MARKERS_VIDEO], true) && - in_array($event->getTarget()->getName(), [...Constants::IGNORE_MARKERS_ALL, ...Constants::IGNORE_MARKERS_IMAGE, ...Constants::IGNORE_MARKERS_AUDIO, ...Constants::IGNORE_MARKERS_VIDEO], true)) { - $this->resetIgnoreCache($event->getSource()); + $sourceAccessList = $this->shareManager->getAccessList($event->getSource(), true, true); + $this->sourceUserIds = array_map(fn ($id) => strval($id), array_keys($sourceAccessList['users'])); return; } - // We try to remember whether the source node is in ignored territory - // because after moving isIgnored doesn't work anymore :( - if ($this->isIgnored($event->getSource())) { - $this->movingFromIgnoredTerritory = true; - } else { - $this->movingFromIgnoredTerritory = false; - } - return; - } - if ($event instanceof NodeRenamedEvent) { - if (in_array($event->getSource()->getName(), [...Constants::IGNORE_MARKERS_ALL, ...Constants::IGNORE_MARKERS_IMAGE, ...Constants::IGNORE_MARKERS_AUDIO, ...Constants::IGNORE_MARKERS_VIDEO], true) && - in_array($event->getTarget()->getName(), [...Constants::IGNORE_MARKERS_ALL, ...Constants::IGNORE_MARKERS_IMAGE, ...Constants::IGNORE_MARKERS_AUDIO, ...Constants::IGNORE_MARKERS_VIDEO], true)) { - $this->resetIgnoreCache($event->getTarget()); - $this->postInsert($event->getSource()->getParent()); - $this->postDelete($event->getTarget()->getParent()); - return; - } - if ($this->movingFromIgnoredTerritory) { + if ($event instanceof NodeRenamedEvent) { + if (in_array($event->getSource()->getName(), [...Constants::IGNORE_MARKERS_ALL, ...Constants::IGNORE_MARKERS_IMAGE, ...Constants::IGNORE_MARKERS_AUDIO, ...Constants::IGNORE_MARKERS_VIDEO], true) && + in_array($event->getTarget()->getName(), [...Constants::IGNORE_MARKERS_ALL, ...Constants::IGNORE_MARKERS_IMAGE, ...Constants::IGNORE_MARKERS_AUDIO, ...Constants::IGNORE_MARKERS_VIDEO], true)) { + $this->resetIgnoreCache($event->getTarget()); + $this->postInsert($event->getSource()->getParent()); + $this->postDelete($event->getTarget()->getParent()); + return; + } + + if (in_array($event->getSource()->getName(), [...Constants::IGNORE_MARKERS_ALL, ...Constants::IGNORE_MARKERS_IMAGE, ...Constants::IGNORE_MARKERS_AUDIO, ...Constants::IGNORE_MARKERS_VIDEO], true) && + !in_array($event->getTarget()->getName(), [...Constants::IGNORE_MARKERS_ALL, ...Constants::IGNORE_MARKERS_IMAGE, ...Constants::IGNORE_MARKERS_AUDIO, ...Constants::IGNORE_MARKERS_VIDEO], true)) { + $this->postInsert($event->getSource()->getParent()); + return; + } + + if (!in_array($event->getSource()->getName(), [...Constants::IGNORE_MARKERS_ALL, ...Constants::IGNORE_MARKERS_IMAGE, ...Constants::IGNORE_MARKERS_AUDIO, ...Constants::IGNORE_MARKERS_VIDEO], true) && + in_array($event->getTarget()->getName(), [...Constants::IGNORE_MARKERS_ALL, ...Constants::IGNORE_MARKERS_IMAGE, ...Constants::IGNORE_MARKERS_AUDIO, ...Constants::IGNORE_MARKERS_VIDEO], true)) { + $this->resetIgnoreCache($event->getTarget()); + $this->postDelete($event->getTarget()->getParent()); + return; + } + + if ($this->movingFromIgnoredTerritory) { + if ($this->isIgnored($event->getTarget())) { + return; + } + $this->postInsert($event->getTarget()); + return; + } if ($this->isIgnored($event->getTarget())) { + $this->postDelete($event->getTarget()); return; } - $this->postInsert($event->getTarget()); - return; - } - if ($this->isIgnored($event->getTarget())) { - $this->postDelete($event->getTarget()); - return; - } - return; - } - if ($event instanceof BeforeNodeDeletedEvent) { - $this->postDelete($event->getNode(), false); - return; - } - if ($event instanceof NodeDeletedEvent) { - if (in_array($event->getNode()->getName(), [...Constants::IGNORE_MARKERS_ALL, ...Constants::IGNORE_MARKERS_IMAGE, ...Constants::IGNORE_MARKERS_AUDIO, ...Constants::IGNORE_MARKERS_VIDEO], true)) { - $this->resetIgnoreCache($event->getNode()); - $this->postInsert($event->getNode()->getParent()); - return; - } - } - if ($event instanceof NodeCreatedEvent) { - if (in_array($event->getNode()->getName(), [...Constants::IGNORE_MARKERS_ALL, ...Constants::IGNORE_MARKERS_IMAGE, ...Constants::IGNORE_MARKERS_AUDIO, ...Constants::IGNORE_MARKERS_VIDEO], true)) { - $this->resetIgnoreCache($event->getNode()); - $this->postDelete($event->getNode()->getParent()); - return; - } - $this->postInsert($event->getNode(), false); - return; - } - if ($event instanceof CacheEntryInsertedEvent) { - $node = current($this->rootFolder->getById($event->getFileId())); - if ($node === false) { + $this->postRename($event->getSource(), $event->getTarget()); return; } - if ($node instanceof Folder) { + if ($event instanceof BeforeNodeDeletedEvent) { + $this->postDelete($event->getNode(), false); return; } - if (in_array($node->getName(), [...Constants::IGNORE_MARKERS_ALL, ...Constants::IGNORE_MARKERS_IMAGE, ...Constants::IGNORE_MARKERS_AUDIO, ...Constants::IGNORE_MARKERS_VIDEO], true)) { - $this->resetIgnoreCache($node); - $this->postDelete($node->getParent()); - return; + if ($event instanceof NodeDeletedEvent) { + if (in_array($event->getNode()->getName(), [...Constants::IGNORE_MARKERS_ALL, ...Constants::IGNORE_MARKERS_IMAGE, ...Constants::IGNORE_MARKERS_AUDIO, ...Constants::IGNORE_MARKERS_VIDEO], true)) { + $this->resetIgnoreCache($event->getNode()); + $this->postInsert($event->getNode()->getParent()); + return; + } } - $this->postInsert($node); - } - if ($event instanceof NodeRemovedFromCache) { - $cacheEntry = $event->getStorage()->getCache()->get($event->getPath()); - if ($cacheEntry === false) { + if ($event instanceof NodeCreatedEvent) { + if (in_array($event->getNode()->getName(), [...Constants::IGNORE_MARKERS_ALL, ...Constants::IGNORE_MARKERS_IMAGE, ...Constants::IGNORE_MARKERS_AUDIO, ...Constants::IGNORE_MARKERS_VIDEO], true)) { + $this->resetIgnoreCache($event->getNode()); + $this->postDelete($event->getNode()->getParent()); + return; + } + $this->postInsert($event->getNode(), false); return; } - $node = current($this->rootFolder->getById($cacheEntry->getId())); - if ($node === false) { - return; + if ($event instanceof CacheEntryInsertedEvent) { + $node = current($this->rootFolder->getById($event->getFileId())); + if ($node === false) { + return; + } + if ($node instanceof Folder) { + return; + } + if (in_array($node->getName(), [...Constants::IGNORE_MARKERS_ALL, ...Constants::IGNORE_MARKERS_IMAGE, ...Constants::IGNORE_MARKERS_AUDIO, ...Constants::IGNORE_MARKERS_VIDEO], true)) { + $this->resetIgnoreCache($node); + $this->postDelete($node->getParent()); + return; + } + $this->postInsert($node); } - if (in_array($node->getName(), [...Constants::IGNORE_MARKERS_ALL, ...Constants::IGNORE_MARKERS_IMAGE, ...Constants::IGNORE_MARKERS_AUDIO, ...Constants::IGNORE_MARKERS_VIDEO], true)) { - $this->resetIgnoreCache($node); - $this->postInsert($node->getParent()); - return; + if ($event instanceof NodeRemovedFromCache) { + $cacheEntry = $event->getStorage()->getCache()->get($event->getPath()); + if ($cacheEntry === false) { + return; + } + $node = current($this->rootFolder->getById($cacheEntry->getId())); + if ($node === false) { + return; + } + if (in_array($node->getName(), [...Constants::IGNORE_MARKERS_ALL, ...Constants::IGNORE_MARKERS_IMAGE, ...Constants::IGNORE_MARKERS_AUDIO, ...Constants::IGNORE_MARKERS_VIDEO], true)) { + $this->resetIgnoreCache($node); + $this->postInsert($node->getParent()); + return; + } + $this->postDelete($node); } - $this->postDelete($node); + } catch (\Throwable $e) { + $this->logger->error('Error in recognize file listener', ['exception' => $e]); } } @@ -315,58 +332,11 @@ public function postInsert(Node $node, bool $recurse = true): void { } } - public function preRename(Node $source, Node $target): void { - $sourceAccessList = $this->shareManager->getAccessList($source, true, true); - $sourceUserIds = array_map(fn ($id) => strval($id), array_keys($sourceAccessList['users'])); - $targetAccessList = $this->shareManager->getAccessList($target, true, true); - $targetUserIds = array_map(fn ($id) => strval($id), array_keys($targetAccessList['users'])); - - $usersToRemove = array_diff($sourceUserIds, $targetUserIds); - - if ($target->getType() === FileInfo::TYPE_FOLDER) { - $mount = $target->getMountPoint(); - if ($mount->getNumericStorageId() === null) { - return; - } - $files = $this->storageService->getFilesInMount($mount->getNumericStorageId(), $source->getId(), [ClusteringFaceClassifier::MODEL_NAME], 0, 0); - foreach ($files as $fileInfo) { - $this->faceDetectionMapper->removeDetectionsForFileFromUsersNotInList($fileInfo['fileid'], $usersToRemove); - } - } else { - $this->faceDetectionMapper->removeDetectionsForFileFromUsersNotInList($source->getId(), $usersToRemove); - } - } - public function postRename(Node $source, Node $target): void { - if (in_array($source->getName(), [...Constants::IGNORE_MARKERS_ALL, ...Constants::IGNORE_MARKERS_IMAGE, ...Constants::IGNORE_MARKERS_AUDIO, ...Constants::IGNORE_MARKERS_VIDEO], true) && - !in_array($target->getName(), [...Constants::IGNORE_MARKERS_ALL, ...Constants::IGNORE_MARKERS_IMAGE, ...Constants::IGNORE_MARKERS_AUDIO, ...Constants::IGNORE_MARKERS_VIDEO], true)) { - $this->resetIgnoreCache($source); - $this->postInsert($target->getParent()); - return; - } - - if (!in_array($source->getName(), [...Constants::IGNORE_MARKERS_ALL, ...Constants::IGNORE_MARKERS_IMAGE, ...Constants::IGNORE_MARKERS_AUDIO, ...Constants::IGNORE_MARKERS_VIDEO], true) && - in_array($target->getName(), [...Constants::IGNORE_MARKERS_ALL, ...Constants::IGNORE_MARKERS_IMAGE, ...Constants::IGNORE_MARKERS_AUDIO, ...Constants::IGNORE_MARKERS_VIDEO], true)) { - $this->resetIgnoreCache($target); - $this->postDelete($target->getParent()); - return; - } - - if ($this->isIgnored($target)) { - $this->postDelete($target); - return; - } - if ($this->isIgnored($source) && !$this->isIgnored($target)) { - $this->postInsert($target); - return; - } - - $sourceAccessList = $this->shareManager->getAccessList($source, true, true); - $sourceUserIds = array_map(fn ($id) => strval($id), array_keys($sourceAccessList['users'])); $targetAccessList = $this->shareManager->getAccessList($target, true, true); $targetUserIds = array_map(fn ($id) => strval($id), array_keys($targetAccessList['users'])); - $usersToAdd = array_diff($targetUserIds, $sourceUserIds); + $usersToAdd = array_diff($targetUserIds, $this->sourceUserIds); $existingUsers = array_diff($targetUserIds, $usersToAdd); // *handwaving* I know this is a stretch but it's good enough $ownerId = $existingUsers[0]; @@ -384,6 +354,7 @@ public function postRename(Node $source, Node $target): void { } $this->faceDetectionMapper->copyDetectionsForFileFromUserToUser($fileInfo['fileid'], $ownerId, $userId); } + $this->faceDetectionMapper->removeDetectionsForFileFromUsersNotInList($fileInfo['fileid'], $targetUserIds); } } else { foreach ($usersToAdd as $userId) { @@ -392,6 +363,7 @@ public function postRename(Node $source, Node $target): void { } $this->faceDetectionMapper->copyDetectionsForFileFromUserToUser($target->getId(), $ownerId, $userId); } + $this->faceDetectionMapper->removeDetectionsForFileFromUsersNotInList($source->getId(), $targetUserIds); } } diff --git a/tests/ClassifierTest.php b/tests/ClassifierTest.php index 150fd6b2..e81630c1 100644 --- a/tests/ClassifierTest.php +++ b/tests/ClassifierTest.php @@ -22,7 +22,6 @@ use OCA\Recognize\Service\QueueService; use OCP\AppFramework\Services\IAppConfig; use OCP\BackgroundJob\IJobList; -use OCP\Constants; use OCP\Files\Folder; use OCP\Files\IRootFolder; use OCP\SystemTag\ISystemTagObjectMapper; @@ -66,6 +65,7 @@ public static function setUpBeforeClass(): void { parent::setUpBeforeClass(); $backend = new \Test\Util\User\Dummy(); $backend->createUser(self::TEST_USER1, self::TEST_USER1); + $backend->createUser(self::TEST_USER2, self::TEST_USER2); \OC::$server->get(\OCP\IUserManager::class)->registerBackend($backend); } @@ -147,14 +147,6 @@ public function testFileListener(string $ignoreFileName) : void { $this->testFile = $this->userFolder->newFile('/alpine.jpg', file_get_contents(__DIR__.'/res/alpine.JPG')); $this->userFolder->newFolder('/test/ignore/'); - $sharedFolder = $this->userFolder->newFolder('/test/shared/'); - $share = $this->shareManager->newShare(); - $share->setSharedBy(self::TEST_USER1); - $share->setSharedWith(self::TEST_USER2); - $share->setShareType(\OCP\Share\IShare::TYPE_USER); - $share->setNode($sharedFolder); - $share->setPermissions(Constants::PERMISSION_ALL); - $this->shareManager->createShare($share); $ignoreFile = $this->userFolder->newFile('/test/' . $ignoreFileName, ''); $this->ignoredFile = $this->userFolder->newFile('/test/ignore/alpine-2.jpg', file_get_contents(__DIR__.'/res/alpine.JPG')); @@ -495,6 +487,24 @@ public function testFacesPipeline() : void { } } } + + // Test FileListener for moving files across share boundaries + + self::assertCount(0, $this->faceDetectionMapper->findByUserId(self::TEST_USER2), 'user 2 should have no face detections'); + + $sharedFolder = $this->userFolder->newFolder('/shared/'); + $share = $this->shareManager->newShare(); + $share->setSharedBy(self::TEST_USER1); + $share->setSharedWith(self::TEST_USER2); + $share->setShareType(\OCP\Share\IShare::TYPE_USER); + $share->setNode($sharedFolder); + $share->setPermissions(\OCP\Constants::PERMISSION_ALL); + $this->shareManager->createShare($share); + $this->shareManager->acceptShare($share, self::TEST_USER2); + + $testFiles[0]->move($sharedFolder->getPath().'/'.$testFiles[0]->getName()); + + self::assertCount(1, $this->faceDetectionMapper->findByUserId(self::TEST_USER2), 'user 2 should have 1 face detection now'); } /** From 57a435282f167b4946e5770c7c7d334eeaca84b4 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Thu, 18 Apr 2024 12:42:07 +0200 Subject: [PATCH 4/7] fix: psalm issues Signed-off-by: Marcel Klehr --- lib/Hooks/FileListener.php | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/Hooks/FileListener.php b/lib/Hooks/FileListener.php index d15d25f1..fa38c56b 100644 --- a/lib/Hooks/FileListener.php +++ b/lib/Hooks/FileListener.php @@ -61,6 +61,7 @@ public function __construct(FaceDetectionMapper $faceDetectionMapper, LoggerInte $this->storageService = $storageService; $this->shareManager = $shareManager; $this->rootFolder = $rootFolder; + $this->sourceUserIds = []; } public function handle(Event $event): void { @@ -70,6 +71,7 @@ public function handle(Event $event): void { $ownerId = $share->getShareOwner(); $node = $share->getNode(); + /** @var array{users:array, remote: array, mail: array} $accessList */ $accessList = $this->shareManager->getAccessList($node, true, true); $userIds = array_map(fn ($id) => strval($id), array_keys($accessList['users'])); @@ -106,10 +108,8 @@ public function handle(Event $event): void { $share = $event->getShare(); $node = $share->getNode(); + /** @var array{users:array, remote: array, mail: array} $accessList */ $accessList = $this->shareManager->getAccessList($node, true, true); - /** - * @var string[] $userIds - */ $userIds = array_keys($accessList['users']); if ($node->getType() === FileInfo::TYPE_FOLDER) { @@ -137,6 +137,7 @@ public function handle(Event $event): void { } else { $this->movingFromIgnoredTerritory = false; } + /** @var array{users:array, remote: array, mail: array} $sourceAccessList */ $sourceAccessList = $this->shareManager->getAccessList($event->getSource(), true, true); $this->sourceUserIds = array_map(fn ($id) => strval($id), array_keys($sourceAccessList['users'])); return; @@ -333,6 +334,7 @@ public function postInsert(Node $node, bool $recurse = true): void { } public function postRename(Node $source, Node $target): void { + /** @var array{users:array, remote: array, mail: array} $targetAccessList */ $targetAccessList = $this->shareManager->getAccessList($target, true, true); $targetUserIds = array_map(fn ($id) => strval($id), array_keys($targetAccessList['users'])); @@ -343,10 +345,11 @@ public function postRename(Node $source, Node $target): void { if ($target->getType() === FileInfo::TYPE_FOLDER) { $mount = $target->getMountPoint(); - if ($mount->getNumericStorageId() === null) { + $numericStorageId = $mount->getNumericStorageId(); + if ($numericStorageId === null) { return; } - $files = $this->storageService->getFilesInMount($mount->getNumericStorageId(), $target->getId(), [ClusteringFaceClassifier::MODEL_NAME], 0, 0); + $files = $this->storageService->getFilesInMount($numericStorageId, $target->getId(), [ClusteringFaceClassifier::MODEL_NAME], 0, 0); foreach ($files as $fileInfo) { foreach ($usersToAdd as $userId) { if (count($this->faceDetectionMapper->findByFileIdAndUser($target->getId(), $userId)) > 0) { From ffa0d4922b65ee93d6ac7c517d400ba4f5491167 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Thu, 18 Apr 2024 12:55:57 +0200 Subject: [PATCH 5/7] fix(php-cs-fixer) Signed-off-by: Marcel Klehr --- .github/workflows/lint-php-cs.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/lint-php-cs.yml b/.github/workflows/lint-php-cs.yml index db3d7705..68fd3f42 100644 --- a/.github/workflows/lint-php-cs.yml +++ b/.github/workflows/lint-php-cs.yml @@ -43,7 +43,9 @@ jobs: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - name: Install dependencies - run: composer i + run: | + composer i + composer i - name: Lint run: composer run cs:check || ( echo 'Please run `composer run cs:fix` to format your code' && exit 1 ) From 7d16b4214d1eda26c39bc1299b11b86a7c699455 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Thu, 18 Apr 2024 15:07:55 +0200 Subject: [PATCH 6/7] fix tests Signed-off-by: Marcel Klehr --- tests/ClassifierTest.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/ClassifierTest.php b/tests/ClassifierTest.php index e81630c1..b80c101a 100644 --- a/tests/ClassifierTest.php +++ b/tests/ClassifierTest.php @@ -505,6 +505,8 @@ public function testFacesPipeline() : void { $testFiles[0]->move($sharedFolder->getPath().'/'.$testFiles[0]->getName()); self::assertCount(1, $this->faceDetectionMapper->findByUserId(self::TEST_USER2), 'user 2 should have 1 face detection now'); + $this->shareManager->deleteShare($share); + self::assertCount(0, $this->faceDetectionMapper->findByUserId(self::TEST_USER2), 'user 2 should have 0 face detections after deleting the share'); } /** From 2d786e0cf42bd7777fafac54cd38c0f9510cba51 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Thu, 18 Apr 2024 15:52:09 +0200 Subject: [PATCH 7/7] Fix(ci): Update shivammathur/setup-php Signed-off-by: Marcel Klehr --- .github/workflows/lint-php-cs.yml | 2 +- .github/workflows/lint-php.yml | 2 +- .github/workflows/phpunit-mysql.yml | 2 +- .github/workflows/phpunit-pgsql.yml | 2 +- .github/workflows/phpunit-sqlite.yml | 2 +- .github/workflows/psalm.yml | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/workflows/lint-php-cs.yml b/.github/workflows/lint-php-cs.yml index 68fd3f42..055c2488 100644 --- a/.github/workflows/lint-php-cs.yml +++ b/.github/workflows/lint-php-cs.yml @@ -34,7 +34,7 @@ jobs: uses: actions/checkout@8e5e7e5ab8b370d6c329ec480221332ada57f0ab # v3.5.2 - name: Set up php - uses: shivammathur/setup-php@c5fc0d8281aba02c7fda07d3a70cc5371548067d # v2 + uses: shivammathur/setup-php@v2 # v2 with: php-version: 8.1 coverage: none diff --git a/.github/workflows/lint-php.yml b/.github/workflows/lint-php.yml index 384a92ce..498e8580 100644 --- a/.github/workflows/lint-php.yml +++ b/.github/workflows/lint-php.yml @@ -46,7 +46,7 @@ jobs: uses: actions/checkout@8e5e7e5ab8b370d6c329ec480221332ada57f0ab # v3.5.2 - name: Set up php ${{ matrix.php-versions }} - uses: shivammathur/setup-php@c5fc0d8281aba02c7fda07d3a70cc5371548067d # v2 + uses: shivammathur/setup-php@v2 # v2 with: php-version: ${{ matrix.php-versions }} coverage: none diff --git a/.github/workflows/phpunit-mysql.yml b/.github/workflows/phpunit-mysql.yml index c2ce7566..66f1ffc3 100644 --- a/.github/workflows/phpunit-mysql.yml +++ b/.github/workflows/phpunit-mysql.yml @@ -70,7 +70,7 @@ jobs: path: apps/${{ env.APP_NAME }} - name: Set up php ${{ matrix.php-versions }} - uses: shivammathur/setup-php@c5fc0d8281aba02c7fda07d3a70cc5371548067d # v2 + uses: shivammathur/setup-php@v2 # v2 with: php-version: ${{ matrix.php-versions }} # https://docs.nextcloud.com/server/stable/admin_manual/installation/source_installation.html#prerequisites-for-manual-installation diff --git a/.github/workflows/phpunit-pgsql.yml b/.github/workflows/phpunit-pgsql.yml index 5fa84f5d..795c566d 100644 --- a/.github/workflows/phpunit-pgsql.yml +++ b/.github/workflows/phpunit-pgsql.yml @@ -73,7 +73,7 @@ jobs: path: apps/${{ env.APP_NAME }} - name: Set up php ${{ matrix.php-versions }} - uses: shivammathur/setup-php@c5fc0d8281aba02c7fda07d3a70cc5371548067d # v2 + uses: shivammathur/setup-php@v2 # v2 with: php-version: ${{ matrix.php-versions }} # https://docs.nextcloud.com/server/stable/admin_manual/installation/source_installation.html#prerequisites-for-manual-installation diff --git a/.github/workflows/phpunit-sqlite.yml b/.github/workflows/phpunit-sqlite.yml index 1a1c1f8a..c9a4bafe 100644 --- a/.github/workflows/phpunit-sqlite.yml +++ b/.github/workflows/phpunit-sqlite.yml @@ -63,7 +63,7 @@ jobs: path: apps/${{ env.APP_NAME }} - name: Set up php ${{ matrix.php-versions }} - uses: shivammathur/setup-php@c5fc0d8281aba02c7fda07d3a70cc5371548067d # v2 + uses: shivammathur/setup-php@v2 # v2 with: php-version: ${{ matrix.php-versions }} # https://docs.nextcloud.com/server/stable/admin_manual/installation/source_installation.html#prerequisites-for-manual-installation diff --git a/.github/workflows/psalm.yml b/.github/workflows/psalm.yml index fe4d9fac..0e2806d4 100644 --- a/.github/workflows/psalm.yml +++ b/.github/workflows/psalm.yml @@ -40,7 +40,7 @@ jobs: uses: actions/checkout@8e5e7e5ab8b370d6c329ec480221332ada57f0ab # v3.5.2 - name: Set up php - uses: shivammathur/setup-php@c5fc0d8281aba02c7fda07d3a70cc5371548067d # v2 + uses: shivammathur/setup-php@v2 # v2 with: php-version: ${{ matrix.php-versions }} coverage: none