From 49bea767ea3621aa1814ba99d6c667f7ab291281 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Wed, 23 Sep 2026 14:57:53 +0200 Subject: [PATCH 01/11] fix(FileListener): Queue move actions correctly and fix FsActionMapper see #1118 Signed-off-by: Marcel Klehr --- .../Images/ClusteringFaceClassifier.php | 1 + lib/Db/FsActionMapper.php | 6 +- lib/Db/FsMove.php | 17 +++++- lib/Hooks/FileListener.php | 16 ++--- lib/Service/FsActionService.php | 60 ++++++++++++------- lib/TaskProcessing/TaskResultListener.php | 1 + 6 files changed, 66 insertions(+), 35 deletions(-) diff --git a/lib/Classifiers/Images/ClusteringFaceClassifier.php b/lib/Classifiers/Images/ClusteringFaceClassifier.php index 81eff74a6..a47f1c037 100644 --- a/lib/Classifiers/Images/ClusteringFaceClassifier.php +++ b/lib/Classifiers/Images/ClusteringFaceClassifier.php @@ -54,6 +54,7 @@ public function __construct( * @throws NotFoundException */ private function getUsersWithFileAccess(Node $node): array { + $this->userMountCache->clear(); $mountInfos = $this->userMountCache->getMountsForFileId($node->getId()); $userIds = array_map(static function (ICachedMountInfo $mountInfo) { return $mountInfo->getUser()->getUID(); diff --git a/lib/Db/FsActionMapper.php b/lib/Db/FsActionMapper.php index 56d46585c..1a4abadf8 100644 --- a/lib/Db/FsActionMapper.php +++ b/lib/Db/FsActionMapper.php @@ -54,12 +54,8 @@ public function findByStorageId(string $className, int $storageId, int $limit = * @param class-string $className * @return list * @throws \OCP\DB\Exception - * @throws \Exception */ public function find(string $className, int $limit = 0): array { - if (!in_array('storage_id', $className::$columns, true)) { - throw new \Exception('entity does not have a storage_id column'); - } $qb = $this->db->getQueryBuilder(); $qb->selectDistinct($className::$columns) ->from($className::$tableName); @@ -243,7 +239,7 @@ public function insertMove(int $nodeId, string $owner, array $addedUsers, array $move->setAddedUsers($addedUsers); $move->setTargetUsers($targetUsers); $this->insert($move); - $arguments = [ 'type' => FsDeletion::class ]; + $arguments = [ 'type' => FsMove::class ]; if (!$this->jobList->has(ProcessFsActionsJob::class, $arguments)) { $this->jobList->add(ProcessFsActionsJob::class, $arguments); } diff --git a/lib/Db/FsMove.php b/lib/Db/FsMove.php index 198994b24..8aa351bcc 100644 --- a/lib/Db/FsMove.php +++ b/lib/Db/FsMove.php @@ -48,14 +48,27 @@ public function __construct() { * @return list */ public function getAddedUsers(): array { - return explode(',', $this->addedUsers ?? ''); + return self::explodeUsers($this->addedUsers); } /** * @return list */ public function getTargetUsers(): array { - return explode(',', $this->targetUsers ?? ''); + return self::explodeUsers($this->targetUsers); + } + + /** + * explode() on an empty string yields [''], which would be treated as a user + * with an empty user ID, so map the empty column to an empty list instead. + * + * @return list + */ + private static function explodeUsers(?string $users): array { + if ($users === null || $users === '') { + return []; + } + return explode(',', $users); } /** diff --git a/lib/Hooks/FileListener.php b/lib/Hooks/FileListener.php index 1426b6384..1ebee83f1 100644 --- a/lib/Hooks/FileListener.php +++ b/lib/Hooks/FileListener.php @@ -283,18 +283,18 @@ public function postInsert(Node $node, bool $recurse = true, ?array $mimeTypes = * @throws Exception */ public function postRename(Node $source, Node $target): void { - $targetUserIds = $this->getUsersWithFileAccess($target->getId()); - - $usersToAdd = array_values(array_diff($targetUserIds, $this->sourceUserIds)); - $existingUsers = array_diff($targetUserIds, $usersToAdd); - $sourceOwner = $source->getOwner(); - $targetOwner = $target->getOwner(); - $ownerId = $sourceOwner?->getUID() ?? $targetOwner?->getUID() ?? $existingUsers[0]; - if (preg_match('#^/[^/]*?/files/#', $target->getPath()) !== 1 && preg_match('#^/groupfolders/#', $target->getPath()) !== 1) { return; } + $targetUserIds = $this->getUsersWithFileAccess($target->getId()); + $usersToAdd = array_values(array_diff($targetUserIds, $this->sourceUserIds)); + + // Recorded for diagnostics only, and empty for group folder nodes, which have no + // owner: FsActionService picks the user to copy detections from by looking at who + // actually holds detections for the file. + $ownerId = $source->getOwner()?->getUID() ?? $target->getOwner()?->getUID() ?? ''; + $this->fsActionMapper->insertMove($target->getId(), $ownerId, $usersToAdd, $targetUserIds); } diff --git a/lib/Service/FsActionService.php b/lib/Service/FsActionService.php index 23bd2b643..a1122c694 100644 --- a/lib/Service/FsActionService.php +++ b/lib/Service/FsActionService.php @@ -125,7 +125,7 @@ public function processActions(array $actions): void { break; } try { - $this->onMove($action->getOwner(), $action->getAddedUsers(), $action->getTargetUsers(), $node); + $this->onMove($action->getAddedUsers(), $action->getTargetUsers(), $node); } catch (Exception|InvalidPathException|NotFoundException $e) { $this->logger->warning('Failed to process move action: ' . $e->getMessage() . ' Continuing.', ['exception' => $e]); } @@ -168,24 +168,17 @@ private function onAccessUpdate(int $storageId, int $rootId): void { $files = $this->storageService->getFilesInMount($storageId, $rootId, [ClusteringFaceClassifier::MODEL_NAME], 0, 0); $userIdsToScheduleClustering = []; foreach ($files as $fileInfo) { - $node = $this->rootFolder->getFirstNodeById($fileInfo['fileid']) ?: null; - $ownerId = $node?->getOwner()?->getUID(); - if ($ownerId === null) { + $detectionCountByUser = $this->getDetectionCountByUser($fileInfo['fileid']); + if (count($detectionCountByUser) === 0) { + // Nothing detected for this file (yet): nothing to copy, nothing to prune continue; } - $detectionsForFile = $this->faceDetectionMapper->findByFileId($fileInfo['fileid']); - $userHasDetectionForFile = []; - foreach ($detectionsForFile as $detection) { - $userHasDetectionForFile[$detection->getUserId()] = true; - } + $sourceUserId = (string)array_key_first($detectionCountByUser); foreach ($userIds as $userId) { - if ($userId === $ownerId) { - continue; - } - if ($userHasDetectionForFile[$userId] ?? false) { + if (isset($detectionCountByUser[(string)$userId])) { continue; } - $this->faceDetectionMapper->copyDetectionsForFileFromUserToUser($fileInfo['fileid'], $ownerId, $userId); + $this->faceDetectionMapper->copyDetectionsForFileFromUserToUser($fileInfo['fileid'], $sourceUserId, (string)$userId); $userIdsToScheduleClustering[$userId] = true; } $this->faceDetectionMapper->removeDetectionsForFileFromUsersNotInList($fileInfo['fileid'], $userIds); @@ -195,6 +188,28 @@ private function onAccessUpdate(int $storageId, int $rootId): void { } } + /** + * How many face detections each user holds for a file, most complete set first. + * + * The first key is the user to copy detections from when granting access to further + * users. The file system owner cannot be used for that: group folder nodes have no + * owner when they are resolved outside of a user session, which is the case in the + * background jobs that process fs actions. Whoever already holds detections for the + * file is both a more reliable and a more direct answer to the question. + * + * @return array + * @throws Exception + */ + private function getDetectionCountByUser(int $fileId): array { + $detectionCountByUser = []; + foreach ($this->faceDetectionMapper->findByFileId($fileId) as $detection) { + $userId = (string)$detection->getUserId(); + $detectionCountByUser[$userId] = ($detectionCountByUser[$userId] ?? 0) + 1; + } + arsort($detectionCountByUser); + return $detectionCountByUser; + } + /** * @throws \OCP\Files\InvalidPathException */ @@ -325,33 +340,38 @@ public function onDeletion(int $nodeId, ?array $mimeTypes = null): void { } /** - * @param string $ownerId * @param list $usersToAdd * @param list $targetUserIds * @param Node $node * @return void * @throws Exception|InvalidPathException|NotFoundException */ - private function onMove(string $ownerId, array $usersToAdd, array $targetUserIds, Node $node): void { + private function onMove(array $usersToAdd, array $targetUserIds, Node $node): void { if ($node instanceof Folder) { try { foreach ($node->getDirectoryListing() as $n) { if (!in_array($n->getMimetype(), Constants::IMAGE_FORMATS)) { continue; } - $this->onMove($ownerId, $usersToAdd, $targetUserIds, $n); + $this->onMove($usersToAdd, $targetUserIds, $n); } } catch (NotFoundException|Exception|InvalidPathException $e) { $this->logger->warning('Error in recognize file listener', ['exception' => $e]); } return; } + $detectionCountByUser = $this->getDetectionCountByUser($node->getId()); + if (count($detectionCountByUser) === 0) { + // Nothing detected for this file (yet): nothing to copy, nothing to prune + return; + } + $sourceUserId = (string)array_key_first($detectionCountByUser); foreach ($usersToAdd as $userId) { - if (count($this->faceDetectionMapper->findByFileIdAndUser($node->getId(), $userId)) > 0) { + if (isset($detectionCountByUser[(string)$userId])) { continue; } - $this->faceDetectionMapper->copyDetectionsForFileFromUserToUser($node->getId(), $ownerId, $userId); - $this->jobList->add(ClusterFacesJob::class, ['userId' => $userId]); + $this->faceDetectionMapper->copyDetectionsForFileFromUserToUser($node->getId(), $sourceUserId, (string)$userId); + $this->jobList->add(ClusterFacesJob::class, ['userId' => (string)$userId]); } $this->faceDetectionMapper->removeDetectionsForFileFromUsersNotInList($node->getId(), $targetUserIds); } diff --git a/lib/TaskProcessing/TaskResultListener.php b/lib/TaskProcessing/TaskResultListener.php index bfcad9f6f..40cfff635 100644 --- a/lib/TaskProcessing/TaskResultListener.php +++ b/lib/TaskProcessing/TaskResultListener.php @@ -274,6 +274,7 @@ private function enqueueForLandmarks(int $fileId): void { */ private function getUsersWithFileAccess(int $fileId): array { try { + $this->userMountCache->clear(); $mountInfos = $this->userMountCache->getMountsForFileId($fileId); } catch (\Throwable $e) { $this->logger->warning('Could not look up users with access for file ' . $fileId, ['exception' => $e]); From 7780bad49e3f677370cb141490d8a75209153fd1 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Wed, 23 Sep 2026 15:18:11 +0200 Subject: [PATCH 02/11] fix: Small fixes Signed-off-by: Marcel Klehr --- lib/Db/FsActionMapper.php | 64 ++++++++++++++++++++++++++++++--- lib/Service/FsActionService.php | 4 ++- 2 files changed, 63 insertions(+), 5 deletions(-) diff --git a/lib/Db/FsActionMapper.php b/lib/Db/FsActionMapper.php index 1a4abadf8..96575edd0 100644 --- a/lib/Db/FsActionMapper.php +++ b/lib/Db/FsActionMapper.php @@ -231,7 +231,19 @@ public function insertDeletion(int $storageId, int $nodeId): FsCreation|FsDeleti */ public function insertMove(int $nodeId, string $owner, array $addedUsers, array $targetUsers): Entity { try { + /** @var FsMove $move */ $move = $this->findByNodeId(FsMove::class, $nodeId); + // A move for this node is still pending: the row carries the payload the job will + // act on, so it has to reflect the latest move, not the first one. Union the added + // users (an earlier move may have granted access to users this move didn't touch) + // and take the newest access list verbatim, dropping users it no longer contains. + $addedUsers = array_values(array_intersect( + array_unique(array_merge($move->getAddedUsers(), $addedUsers)), + $targetUsers + )); + $move->setAddedUsers($addedUsers); + $move->setTargetUsers($targetUsers); + $this->update($move); } catch (DoesNotExistException $e) { $move = new FsMove(); $move->setNodeId($nodeId); @@ -239,10 +251,10 @@ public function insertMove(int $nodeId, string $owner, array $addedUsers, array $move->setAddedUsers($addedUsers); $move->setTargetUsers($targetUsers); $this->insert($move); - $arguments = [ 'type' => FsMove::class ]; - if (!$this->jobList->has(ProcessFsActionsJob::class, $arguments)) { - $this->jobList->add(ProcessFsActionsJob::class, $arguments); - } + } + $arguments = [ 'type' => FsMove::class ]; + if (!$this->jobList->has(ProcessFsActionsJob::class, $arguments)) { + $this->jobList->add(ProcessFsActionsJob::class, $arguments); } return $move; } @@ -282,6 +294,50 @@ public function insert(Entity $entity): Entity { return $entity; } + /** + * Like QBMapper::update(), but takes the table name from the entity, since this + * mapper serves several tables and has none of its own. + * + * @param FsCreation|FsDeletion|FsMove|FsAccessUpdate $entity + * @return FsCreation|FsDeletion|FsMove|FsAccessUpdate + * @throws Exception + */ + public function update(Entity $entity): Entity { + // if entity wasn't changed it makes no sense to run a db query + /** @var array $properties */ + $properties = $entity->getUpdatedFields(); + unset($properties['id']); + if (count($properties) === 0) { + return $entity; + } + + $id = $entity->getId(); + if ($id === null) { + throw new \InvalidArgumentException('Entity which should be updated has no id'); + } + + $qb = $this->db->getQueryBuilder(); + $qb->update($entity::$tableName); + + // build the fields + foreach ($properties as $property => $updated) { + $column = $entity->propertyToColumn($property); + $getter = 'get' . ucfirst($property); + $value = $entity->$getter(); + + $type = $this->getParameterTypeForProperty($entity, $property); + $qb->set($column, $qb->createNamedParameter($value, $type)); + } + + $idType = $this->getParameterTypeForProperty($entity, 'id'); + $qb->where( + $qb->expr()->eq('id', $qb->createNamedParameter($id, $idType)) + ); + $qb->executeStatement(); + + return $entity; + } + /** * Returns an db result and throws exceptions when there are more or less * results diff --git a/lib/Service/FsActionService.php b/lib/Service/FsActionService.php index a1122c694..e60d7bda0 100644 --- a/lib/Service/FsActionService.php +++ b/lib/Service/FsActionService.php @@ -350,7 +350,9 @@ private function onMove(array $usersToAdd, array $targetUserIds, Node $node): vo if ($node instanceof Folder) { try { foreach ($node->getDirectoryListing() as $n) { - if (!in_array($n->getMimetype(), Constants::IMAGE_FORMATS)) { + // Recurse into subfolders: we only get a rename event for the top node, + // so the whole subtree has to be walked here. + if ($n->getType() !== FileInfo::TYPE_FOLDER && !in_array($n->getMimetype(), Constants::IMAGE_FORMATS)) { continue; } $this->onMove($usersToAdd, $targetUserIds, $n); From ba75086aa83f3f5deeff0683f0ad92b1d7704d62 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Thu, 24 Sep 2026 11:08:24 +0200 Subject: [PATCH 03/11] fix: Fix a psalm issue Signed-off-by: Marcel Klehr --- lib/Db/FsActionMapper.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Db/FsActionMapper.php b/lib/Db/FsActionMapper.php index 96575edd0..aea150ea1 100644 --- a/lib/Db/FsActionMapper.php +++ b/lib/Db/FsActionMapper.php @@ -304,7 +304,7 @@ public function insert(Entity $entity): Entity { */ public function update(Entity $entity): Entity { // if entity wasn't changed it makes no sense to run a db query - /** @var array $properties */ + /** @var array $properties */ $properties = $entity->getUpdatedFields(); unset($properties['id']); if (count($properties) === 0) { From 6a2e4bae2fb908ab1ec767f8c0edab59e2e0577b Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Thu, 24 Sep 2026 11:11:08 +0200 Subject: [PATCH 04/11] fix(FsActionService): Don't lose subdirectory files in onMove Assisted-by: ClaudeCode:claude-opus-5.5 Signed-off-by: Marcel Klehr --- lib/Service/FsActionService.php | 59 +++++++++++++++++++++------------ 1 file changed, 38 insertions(+), 21 deletions(-) diff --git a/lib/Service/FsActionService.php b/lib/Service/FsActionService.php index e60d7bda0..03d3041f3 100644 --- a/lib/Service/FsActionService.php +++ b/lib/Service/FsActionService.php @@ -32,6 +32,7 @@ final class FsActionService { public const BATCH_SIZE = 1000; + public function __construct( private FsActionMapper $fsActionMapper, private LoggerInterface $logger, @@ -75,6 +76,9 @@ public function processActionsByClass(string $className): void { * @param array $actions */ public function processActions(array $actions): void { + // The mount tables are read repeatedly while processing a batch, so refresh them + // once here rather than on every lookup. + $this->userMountCache->clear(); $lastUserId = null; foreach ($actions as $action) { switch ($action::class) { @@ -125,7 +129,7 @@ public function processActions(array $actions): void { break; } try { - $this->onMove($action->getAddedUsers(), $action->getTargetUsers(), $node); + $this->onMove($node); } catch (Exception|InvalidPathException|NotFoundException $e) { $this->logger->warning('Failed to process move action: ' . $e->getMessage() . ' Continuing.', ['exception' => $e]); } @@ -149,7 +153,6 @@ public function processActions(array $actions): void { * @return list */ private function getUsersWithFileAccess(int $nodeId): array { - $this->userMountCache->clear(); $mountInfos = $this->userMountCache->getMountsForFileId($nodeId); $userIds = array_map(static function (ICachedMountInfo $mountInfo) { return $mountInfo->getUser()->getUID(); @@ -340,25 +343,35 @@ public function onDeletion(int $nodeId, ?array $mimeTypes = null): void { } /** - * @param list $usersToAdd - * @param list $targetUserIds - * @param Node $node - * @return void * @throws Exception|InvalidPathException|NotFoundException */ - private function onMove(array $usersToAdd, array $targetUserIds, Node $node): void { + private function onMove(Node $node): void { + $userIdsToScheduleClustering = []; + $this->moveNode($node, $userIdsToScheduleClustering); + foreach (array_keys($userIdsToScheduleClustering) as $userId) { + $this->jobList->add(ClusterFacesJob::class, ['userId' => (string)$userId]); + } + } + + /** + * @param array $userIdsToScheduleClustering + * @throws Exception|InvalidPathException|NotFoundException + */ + private function moveNode(Node $node, array &$userIdsToScheduleClustering): void { if ($node instanceof Folder) { - try { - foreach ($node->getDirectoryListing() as $n) { - // Recurse into subfolders: we only get a rename event for the top node, - // so the whole subtree has to be walked here. - if ($n->getType() !== FileInfo::TYPE_FOLDER && !in_array($n->getMimetype(), Constants::IMAGE_FORMATS)) { - continue; - } - $this->onMove($usersToAdd, $targetUserIds, $n); + foreach ($node->getDirectoryListing() as $n) { + // Recurse into subfolders: we only get a rename event for the top node, + // so the whole subtree has to be walked here. + if ($n->getType() !== FileInfo::TYPE_FOLDER && !in_array($n->getMimetype(), Constants::IMAGE_FORMATS)) { + continue; + } + try { + $this->moveNode($n, $userIdsToScheduleClustering); + } catch (NotFoundException|Exception|InvalidPathException $e) { + // Per child, so one unreadable node doesn't abandon the rest of the subtree: + // the action row is deleted either way, so skipped files are never retried. + $this->logger->warning('Failed to process move for node ' . $n->getId(), ['exception' => $e]); } - } catch (NotFoundException|Exception|InvalidPathException $e) { - $this->logger->warning('Error in recognize file listener', ['exception' => $e]); } return; } @@ -367,13 +380,17 @@ private function onMove(array $usersToAdd, array $targetUserIds, Node $node): vo // Nothing detected for this file (yet): nothing to copy, nothing to prune return; } + // Resolved per node rather than taken from the moved root: a descendant can be shared + // directly, in which case it is reachable by users the root is not shared with, and + // pruning against the root's list would delete detections they still have access to. + $targetUserIds = $this->getUsersWithFileAccess($node->getId()); $sourceUserId = (string)array_key_first($detectionCountByUser); - foreach ($usersToAdd as $userId) { - if (isset($detectionCountByUser[(string)$userId])) { + foreach ($targetUserIds as $userId) { + if (isset($detectionCountByUser[$userId])) { continue; } - $this->faceDetectionMapper->copyDetectionsForFileFromUserToUser($node->getId(), $sourceUserId, (string)$userId); - $this->jobList->add(ClusterFacesJob::class, ['userId' => (string)$userId]); + $this->faceDetectionMapper->copyDetectionsForFileFromUserToUser($node->getId(), $sourceUserId, $userId); + $userIdsToScheduleClustering[$userId] = true; } $this->faceDetectionMapper->removeDetectionsForFileFromUsersNotInList($node->getId(), $targetUserIds); } From e5d33dbc53c0106283fe36ea8da3f28929c4270d Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Thu, 24 Sep 2026 12:44:08 +0200 Subject: [PATCH 05/11] fix(onAccessUpdate): Correct user access for shared subdirectories Assisted-by: ClaudeCode:claude-opus-5.5 Signed-off-by: Marcel Klehr --- lib/Service/FsActionService.php | 70 +++++++++++++++++---------------- 1 file changed, 37 insertions(+), 33 deletions(-) diff --git a/lib/Service/FsActionService.php b/lib/Service/FsActionService.php index 03d3041f3..0d7ec0bc2 100644 --- a/lib/Service/FsActionService.php +++ b/lib/Service/FsActionService.php @@ -167,30 +167,51 @@ private function getUsersWithFileAccess(int $nodeId): array { * @throws Exception */ private function onAccessUpdate(int $storageId, int $rootId): void { - $userIds = $this->getUsersWithFileAccess($rootId); $files = $this->storageService->getFilesInMount($storageId, $rootId, [ClusteringFaceClassifier::MODEL_NAME], 0, 0); $userIdsToScheduleClustering = []; foreach ($files as $fileInfo) { - $detectionCountByUser = $this->getDetectionCountByUser($fileInfo['fileid']); - if (count($detectionCountByUser) === 0) { - // Nothing detected for this file (yet): nothing to copy, nothing to prune - continue; - } - $sourceUserId = (string)array_key_first($detectionCountByUser); - foreach ($userIds as $userId) { - if (isset($detectionCountByUser[(string)$userId])) { - continue; - } - $this->faceDetectionMapper->copyDetectionsForFileFromUserToUser($fileInfo['fileid'], $sourceUserId, (string)$userId); - $userIdsToScheduleClustering[$userId] = true; - } - $this->faceDetectionMapper->removeDetectionsForFileFromUsersNotInList($fileInfo['fileid'], $userIds); + $this->syncDetectionsForFile($fileInfo['fileid'], $userIdsToScheduleClustering); } foreach (array_keys($userIdsToScheduleClustering) as $userId) { $this->jobList->add(ClusterFacesJob::class, ['userId' => (string)$userId]); } } + /** + * Gives every user with access to the file a copy of its face detections and removes + * the detections of users who lost access. + * + * Access is resolved per file rather than taken from an ancestor: a descendant can be + * shared directly, in which case it is reachable by users the ancestor is not shared + * with, and pruning against the ancestor's list would delete detections they still + * have access to. + * + * @param array $userIdsToScheduleClustering + * @throws Exception + */ + private function syncDetectionsForFile(int $fileId, array &$userIdsToScheduleClustering): void { + $detectionCountByUser = $this->getDetectionCountByUser($fileId); + if (count($detectionCountByUser) === 0) { + // Nothing detected for this file (yet): nothing to copy, nothing to prune + return; + } + $targetUserIds = $this->getUsersWithFileAccess($fileId); + if (count($targetUserIds) === 0) { + // No mounts found, e.g. because the file vanished in the meantime: don't treat + // that as everyone having lost access + return; + } + $sourceUserId = (string)array_key_first($detectionCountByUser); + foreach ($targetUserIds as $userId) { + if (isset($detectionCountByUser[$userId])) { + continue; + } + $this->faceDetectionMapper->copyDetectionsForFileFromUserToUser($fileId, $sourceUserId, $userId); + $userIdsToScheduleClustering[$userId] = true; + } + $this->faceDetectionMapper->removeDetectionsForFileFromUsersNotInList($fileId, $targetUserIds); + } + /** * How many face detections each user holds for a file, most complete set first. * @@ -375,23 +396,6 @@ private function moveNode(Node $node, array &$userIdsToScheduleClustering): void } return; } - $detectionCountByUser = $this->getDetectionCountByUser($node->getId()); - if (count($detectionCountByUser) === 0) { - // Nothing detected for this file (yet): nothing to copy, nothing to prune - return; - } - // Resolved per node rather than taken from the moved root: a descendant can be shared - // directly, in which case it is reachable by users the root is not shared with, and - // pruning against the root's list would delete detections they still have access to. - $targetUserIds = $this->getUsersWithFileAccess($node->getId()); - $sourceUserId = (string)array_key_first($detectionCountByUser); - foreach ($targetUserIds as $userId) { - if (isset($detectionCountByUser[$userId])) { - continue; - } - $this->faceDetectionMapper->copyDetectionsForFileFromUserToUser($node->getId(), $sourceUserId, $userId); - $userIdsToScheduleClustering[$userId] = true; - } - $this->faceDetectionMapper->removeDetectionsForFileFromUsersNotInList($node->getId(), $targetUserIds); + $this->syncDetectionsForFile($node->getId(), $userIdsToScheduleClustering); } } From 2dc93b10a243c79a3dd17e9363831d05eba10c36 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Thu, 24 Sep 2026 12:45:26 +0200 Subject: [PATCH 06/11] perf: Only run userMountCache->clear() per batch Assisted-by: ClaudeCode:claude-opus-5.5 Signed-off-by: Marcel Klehr --- lib/Classifiers/Images/ClusteringFaceClassifier.php | 5 ++++- lib/TaskProcessing/TaskResultListener.php | 4 +++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/Classifiers/Images/ClusteringFaceClassifier.php b/lib/Classifiers/Images/ClusteringFaceClassifier.php index a47f1c037..dcdcad4c7 100644 --- a/lib/Classifiers/Images/ClusteringFaceClassifier.php +++ b/lib/Classifiers/Images/ClusteringFaceClassifier.php @@ -54,7 +54,6 @@ public function __construct( * @throws NotFoundException */ private function getUsersWithFileAccess(Node $node): array { - $this->userMountCache->clear(); $mountInfos = $this->userMountCache->getMountsForFileId($node->getId()); $userIds = array_map(static function (ICachedMountInfo $mountInfo) { return $mountInfo->getUser()->getUID(); @@ -93,6 +92,10 @@ public function classify(array $queueFiles): void { $classifierProcess = $this->classifyFiles(self::MODEL_NAME, $filteredQueueFiles, $timeout); + // The mount tables are read for every file in this batch, so refresh them once here + // rather than on every lookup. + $this->userMountCache->clear(); + /** * @var list $faces */ diff --git a/lib/TaskProcessing/TaskResultListener.php b/lib/TaskProcessing/TaskResultListener.php index 40cfff635..97f5764e8 100644 --- a/lib/TaskProcessing/TaskResultListener.php +++ b/lib/TaskProcessing/TaskResultListener.php @@ -194,6 +194,9 @@ private function applyTagResults(array $fileIds, array $results, string $model, private function applyFaceResults(array $fileIds, array $results): void { $model = ClusteringFaceClassifier::MODEL_NAME; $scheduledClusterJobsFor = []; + // The mount tables are read for every file in this batch, so refresh them once here + // rather than on every lookup. + $this->userMountCache->clear(); foreach ($fileIds as $i => $fileId) { if (!isset($results[$i])) { continue; @@ -274,7 +277,6 @@ private function enqueueForLandmarks(int $fileId): void { */ private function getUsersWithFileAccess(int $fileId): array { try { - $this->userMountCache->clear(); $mountInfos = $this->userMountCache->getMountsForFileId($fileId); } catch (\Throwable $e) { $this->logger->warning('Could not look up users with access for file ' . $fileId, ['exception' => $e]); From 5ca7b27b43806c35cfb28ae2829121df2a77f9d5 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Thu, 24 Sep 2026 12:47:29 +0200 Subject: [PATCH 07/11] fix(insertMove): Use a transaction Assisted-by: ClaudeCode:claude-opus-5.5 Signed-off-by: Marcel Klehr --- lib/Db/FsActionMapper.php | 87 ++++++++++++++------------------------- 1 file changed, 30 insertions(+), 57 deletions(-) diff --git a/lib/Db/FsActionMapper.php b/lib/Db/FsActionMapper.php index aea150ea1..aeeb1425f 100644 --- a/lib/Db/FsActionMapper.php +++ b/lib/Db/FsActionMapper.php @@ -227,31 +227,48 @@ public function insertDeletion(int $storageId, int $nodeId): FsCreation|FsDeleti * @param list $addedUsers * @param list $targetUsers * @return FsCreation|FsDeletion|FsMove|FsAccessUpdate - * @throws Exception|MultipleObjectsReturnedException + * @throws Exception */ public function insertMove(int $nodeId, string $owner, array $addedUsers, array $targetUsers): Entity { - try { - /** @var FsMove $move */ - $move = $this->findByNodeId(FsMove::class, $nodeId); - // A move for this node is still pending: the row carries the payload the job will - // act on, so it has to reflect the latest move, not the first one. Union the added - // users (an earlier move may have granted access to users this move didn't touch) - // and take the newest access list verbatim, dropping users it no longer contains. + // A move for this node may still be pending. Replace it with a fresh row rather than + // updating it in place: the job deletes the rows it has processed by ID, so a row + // updated while the job was already working on it would be deleted along with the + // stale action, and this move would never be processed. A fresh row has a new ID, + // which the running job doesn't know about, so it survives until the next run. + $qb = $this->db->getQueryBuilder(); + $qb->selectDistinct(FsMove::$columns) + ->from(FsMove::$tableName) + ->where($qb->expr()->eq('node_id', $qb->createPositionalParameter($nodeId, IQueryBuilder::PARAM_INT))); + /** @var list $pendingMoves */ + $pendingMoves = $this->findItems(FsMove::class, $qb); + if (count($pendingMoves) > 0) { + // Union the added users (an earlier move may have granted access to users this + // move didn't touch), dropping users the newest access list no longer contains. $addedUsers = array_values(array_intersect( - array_unique(array_merge($move->getAddedUsers(), $addedUsers)), + array_unique(array_merge($addedUsers, ...array_map(static fn (FsMove $move) => $move->getAddedUsers(), $pendingMoves))), $targetUsers )); - $move->setAddedUsers($addedUsers); - $move->setTargetUsers($targetUsers); - $this->update($move); - } catch (DoesNotExistException $e) { + } + + $this->db->beginTransaction(); + try { + $qb = $this->db->getQueryBuilder(); + $qb->delete(FsMove::$tableName) + ->where($qb->expr()->eq('node_id', $qb->createPositionalParameter($nodeId, IQueryBuilder::PARAM_INT))); + $qb->executeStatement(); + $move = new FsMove(); $move->setNodeId($nodeId); $move->setOwner($owner); $move->setAddedUsers($addedUsers); $move->setTargetUsers($targetUsers); $this->insert($move); + $this->db->commit(); + } catch (\Throwable $e) { + $this->db->rollBack(); + throw $e; } + $arguments = [ 'type' => FsMove::class ]; if (!$this->jobList->has(ProcessFsActionsJob::class, $arguments)) { $this->jobList->add(ProcessFsActionsJob::class, $arguments); @@ -294,50 +311,6 @@ public function insert(Entity $entity): Entity { return $entity; } - /** - * Like QBMapper::update(), but takes the table name from the entity, since this - * mapper serves several tables and has none of its own. - * - * @param FsCreation|FsDeletion|FsMove|FsAccessUpdate $entity - * @return FsCreation|FsDeletion|FsMove|FsAccessUpdate - * @throws Exception - */ - public function update(Entity $entity): Entity { - // if entity wasn't changed it makes no sense to run a db query - /** @var array $properties */ - $properties = $entity->getUpdatedFields(); - unset($properties['id']); - if (count($properties) === 0) { - return $entity; - } - - $id = $entity->getId(); - if ($id === null) { - throw new \InvalidArgumentException('Entity which should be updated has no id'); - } - - $qb = $this->db->getQueryBuilder(); - $qb->update($entity::$tableName); - - // build the fields - foreach ($properties as $property => $updated) { - $column = $entity->propertyToColumn($property); - $getter = 'get' . ucfirst($property); - $value = $entity->$getter(); - - $type = $this->getParameterTypeForProperty($entity, $property); - $qb->set($column, $qb->createNamedParameter($value, $type)); - } - - $idType = $this->getParameterTypeForProperty($entity, 'id'); - $qb->where( - $qb->expr()->eq('id', $qb->createNamedParameter($id, $idType)) - ); - $qb->executeStatement(); - - return $entity; - } - /** * Returns an db result and throws exceptions when there are more or less * results From 994e97c6e560b2eeb0fea4348637a80a659feb2e Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Thu, 24 Sep 2026 12:58:09 +0200 Subject: [PATCH 08/11] fix(FsMove): Remove unused columns Assisted-by: ClaudeCode:claude-opus-5.5 Signed-off-by: Marcel Klehr --- lib/Db/FsActionMapper.php | 33 +++--------- lib/Db/FsMove.php | 53 +------------------ lib/Hooks/FileListener.php | 41 ++------------ .../Version013002000Date20260924120000.php | 47 ++++++++++++++++ 4 files changed, 60 insertions(+), 114 deletions(-) create mode 100644 lib/Migration/Version013002000Date20260924120000.php diff --git a/lib/Db/FsActionMapper.php b/lib/Db/FsActionMapper.php index aeeb1425f..2b92ec409 100644 --- a/lib/Db/FsActionMapper.php +++ b/lib/Db/FsActionMapper.php @@ -223,33 +223,15 @@ public function insertDeletion(int $storageId, int $nodeId): FsCreation|FsDeleti /** * @param int $nodeId - * @param string $owner - * @param list $addedUsers - * @param list $targetUsers - * @return FsCreation|FsDeletion|FsMove|FsAccessUpdate + * @return FsMove * @throws Exception */ - public function insertMove(int $nodeId, string $owner, array $addedUsers, array $targetUsers): Entity { + public function insertMove(int $nodeId): FsMove { // A move for this node may still be pending. Replace it with a fresh row rather than - // updating it in place: the job deletes the rows it has processed by ID, so a row - // updated while the job was already working on it would be deleted along with the - // stale action, and this move would never be processed. A fresh row has a new ID, - // which the running job doesn't know about, so it survives until the next run. - $qb = $this->db->getQueryBuilder(); - $qb->selectDistinct(FsMove::$columns) - ->from(FsMove::$tableName) - ->where($qb->expr()->eq('node_id', $qb->createPositionalParameter($nodeId, IQueryBuilder::PARAM_INT))); - /** @var list $pendingMoves */ - $pendingMoves = $this->findItems(FsMove::class, $qb); - if (count($pendingMoves) > 0) { - // Union the added users (an earlier move may have granted access to users this - // move didn't touch), dropping users the newest access list no longer contains. - $addedUsers = array_values(array_intersect( - array_unique(array_merge($addedUsers, ...array_map(static fn (FsMove $move) => $move->getAddedUsers(), $pendingMoves))), - $targetUsers - )); - } - + // keeping it: the job deletes the rows it has processed by ID, so a row the job was + // already working on would be deleted without this move having been processed. A + // fresh row has a new ID, which the running job doesn't know about, so it survives + // until the next run. $this->db->beginTransaction(); try { $qb = $this->db->getQueryBuilder(); @@ -259,9 +241,6 @@ public function insertMove(int $nodeId, string $owner, array $addedUsers, array $move = new FsMove(); $move->setNodeId($nodeId); - $move->setOwner($owner); - $move->setAddedUsers($addedUsers); - $move->setTargetUsers($targetUsers); $this->insert($move); $this->db->commit(); } catch (\Throwable $e) { diff --git a/lib/Db/FsMove.php b/lib/Db/FsMove.php index 8aa351bcc..de0ea5427 100644 --- a/lib/Db/FsMove.php +++ b/lib/Db/FsMove.php @@ -15,73 +15,24 @@ * @package OCA\Recognize\Db * @method int getNodeId() * @method setNodeId(int $nodeId) - * @method string getOwner() - * @method setOwner(string $owner) */ final class FsMove extends Entity { protected ?int $nodeId = null; - protected ?string $owner = null; - protected ?string $addedUsers = null; - protected ?string $targetUsers = null; /** * @var string[] */ - public static array $columns = ['id', 'node_id', 'owner', 'added_users', 'target_users']; + public static array $columns = ['id', 'node_id']; /** * @var string[] */ - public static array $fields = ['id', 'nodeId', 'owner', 'addedUsers', 'targetUsers']; + public static array $fields = ['id', 'nodeId']; public static string $tableName = 'recognize_fs_moves'; public function __construct() { // add types in constructor $this->addType('nodeId', 'integer'); - $this->addType('owner', 'string'); - $this->addType('addedUsers', 'string'); - $this->addType('targetUsers', 'string'); - } - - /** - * @return list - */ - public function getAddedUsers(): array { - return self::explodeUsers($this->addedUsers); - } - - /** - * @return list - */ - public function getTargetUsers(): array { - return self::explodeUsers($this->targetUsers); - } - - /** - * explode() on an empty string yields [''], which would be treated as a user - * with an empty user ID, so map the empty column to an empty list instead. - * - * @return list - */ - private static function explodeUsers(?string $users): array { - if ($users === null || $users === '') { - return []; - } - return explode(',', $users); - } - - /** - * @param list $users - */ - public function setAddedUsers(array $users): void { - $this->setter('addedUsers', [implode(',', $users)]); - } - - /** - * @param list $users - */ - public function setTargetUsers(array $users): void { - $this->setter('targetUsers', [implode(',', $users)]); } } diff --git a/lib/Hooks/FileListener.php b/lib/Hooks/FileListener.php index 1ebee83f1..17b8681d9 100644 --- a/lib/Hooks/FileListener.php +++ b/lib/Hooks/FileListener.php @@ -15,8 +15,6 @@ use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventListener; use OCP\Files\Cache\CacheEntryInsertedEvent; -use OCP\Files\Config\ICachedMountInfo; -use OCP\Files\Config\IUserMountCache; use OCP\Files\Events\Node\BeforeNodeDeletedEvent; use OCP\Files\Events\Node\BeforeNodeRenamedEvent; use OCP\Files\Events\Node\NodeCreatedEvent; @@ -37,9 +35,6 @@ final class FileListener implements IEventListener { private ?bool $movingFromIgnoredTerritory; private ?array $movingDirFromIgnoredTerritory; - /** @var list */ - private array $sourceUserIds; - private ?Node $source = null; /** @var array */ private array $addedMounts = []; @@ -48,28 +43,10 @@ public function __construct( private LoggerInterface $logger, private IgnoreService $ignoreService, private IRootFolder $rootFolder, - private IUserMountCache $userMountCache, private FsActionMapper $fsActionMapper, ) { $this->movingFromIgnoredTerritory = null; $this->movingDirFromIgnoredTerritory = null; - $this->sourceUserIds = []; - } - - /** - * @param int $nodeId - * @return list - * @throws InvalidPathException - * @throws NotFoundException - */ - private function getUsersWithFileAccess(int $nodeId): array { - $this->userMountCache->clear(); - $mountInfos = $this->userMountCache->getMountsForFileId($nodeId); - $userIds = array_map(static function (ICachedMountInfo $mountInfo) { - return $mountInfo->getUser()->getUID(); - }, $mountInfos); - - return array_values(array_unique($userIds)); } public function handle(Event $event): void { @@ -112,8 +89,6 @@ public function handle(Event $event): void { } else { $this->movingDirFromIgnoredTerritory = $this->getDirIgnores($event->getSource()); } - $this->sourceUserIds = $this->getUsersWithFileAccess($event->getSource()->getId()); - $this->source = $event->getSource(); return; } if ($event instanceof NodeRenamedEvent) { @@ -171,7 +146,7 @@ public function handle(Event $event): void { return; } } - $this->postRename($this->source ?? $event->getSource(), $event->getTarget()); + $this->postRename($event->getTarget()); return; } if ($event instanceof BeforeNodeDeletedEvent) { @@ -282,20 +257,14 @@ public function postInsert(Node $node, bool $recurse = true, ?array $mimeTypes = * @throws NotFoundException * @throws Exception */ - public function postRename(Node $source, Node $target): void { + public function postRename(Node $target): void { if (preg_match('#^/[^/]*?/files/#', $target->getPath()) !== 1 && preg_match('#^/groupfolders/#', $target->getPath()) !== 1) { return; } - $targetUserIds = $this->getUsersWithFileAccess($target->getId()); - $usersToAdd = array_values(array_diff($targetUserIds, $this->sourceUserIds)); - - // Recorded for diagnostics only, and empty for group folder nodes, which have no - // owner: FsActionService picks the user to copy detections from by looking at who - // actually holds detections for the file. - $ownerId = $source->getOwner()?->getUID() ?? $target->getOwner()?->getUID() ?? ''; - - $this->fsActionMapper->insertMove($target->getId(), $ownerId, $usersToAdd, $targetUserIds); + // Who has access to the moved node, and to each file below it, is resolved when + // the move is processed, so the node ID is all that needs to be recorded. + $this->fsActionMapper->insertMove($target->getId()); } /** diff --git a/lib/Migration/Version013002000Date20260924120000.php b/lib/Migration/Version013002000Date20260924120000.php new file mode 100644 index 000000000..0f0bab26c --- /dev/null +++ b/lib/Migration/Version013002000Date20260924120000.php @@ -0,0 +1,47 @@ +hasTable('recognize_fs_moves')) { + return null; + } + + $changed = false; + $table = $schema->getTable('recognize_fs_moves'); + foreach (['owner', 'added_users', 'target_users'] as $column) { + if ($table->hasColumn($column)) { + $table->dropColumn($column); + $changed = true; + } + } + return $changed ? $schema : null; + } +} From 8fed20c7e2f3b2eb37920cb363f1260e2a3bfe78 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Thu, 24 Sep 2026 13:01:15 +0200 Subject: [PATCH 09/11] fix: Update psalm baseline Signed-off-by: Marcel Klehr --- psalm-baseline.xml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 190286a67..899e81642 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -1,10 +1,5 @@ - - - getContainer()->get(PropFindPlugin::class)]]> - - @@ -947,7 +942,12 @@ + + + + getUserId()]]> + From f1f2c8f0552ebc218c6d92884c5dfe65a69b7484 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Thu, 24 Sep 2026 13:02:23 +0200 Subject: [PATCH 10/11] fix: Make migration run Assisted-by: ClaudeCode:claude-opus-5.5 Signed-off-by: Marcel Klehr --- appinfo/info.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/appinfo/info.xml b/appinfo/info.xml index e7e201b50..6ea182e38 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -75,7 +75,7 @@ Requirements: The app does not send any sensitive data to cloud providers or similar services. All processing is done on your Nextcloud machine, using Tensorflow.js running in Node.js. ]]> - 13.1.0 + 13.2.0-dev agpl Marcel Klehr From 9c5891d8a875bd0d092bb46aa3fc9b21fb478e98 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Thu, 24 Sep 2026 13:48:20 +0200 Subject: [PATCH 11/11] tests: Add tests for fs actions Assisted-by: ClaudeCode:claude-opus-5.5 Signed-off-by: Marcel Klehr --- tests/FsActionServiceTest.php | 246 ++++++++++++++++++++++++++++++++++ 1 file changed, 246 insertions(+) create mode 100644 tests/FsActionServiceTest.php diff --git a/tests/FsActionServiceTest.php b/tests/FsActionServiceTest.php new file mode 100644 index 000000000..3062c4859 --- /dev/null +++ b/tests/FsActionServiceTest.php @@ -0,0 +1,246 @@ +createUser(self::OWNER, self::OWNER); + $backend->createUser(self::RECIPIENT, self::RECIPIENT); + Server::get(\OCP\IUserManager::class)->registerBackend($backend); + + // The file listener relies on node events, which are emitted by hooks that + // TestCase::tearDownAfterClass() clears after every test class + \OC_Hook::clear('OC_Filesystem'); + Server::get(\OC\Files\Node\HookConnector::class)->viewToNode(); + } + + public function setUp(): void { + parent::setUp(); + $this->rootFolder = Server::get(IRootFolder::class); + $this->faceDetectionMapper = Server::get(FaceDetectionMapper::class); + $this->fsActionMapper = Server::get(FsActionMapper::class); + $this->jobList = Server::get(IJobList::class); + $this->shareManager = Server::get(IShareManager::class); + $this->db = Server::get(IDBConnection::class); + + $this->loginAsUser(self::OWNER); + $this->userFolder = $this->rootFolder->getUserFolder(self::OWNER); + + foreach ($this->shareManager->getSharesBy(self::OWNER, IShare::TYPE_USER, null, true, -1) as $share) { + $this->shareManager->deleteShare($share); + } + foreach ($this->userFolder->getDirectoryListing() as $node) { + $node->delete(); + } + \OCP\Server::get(\OCA\Files_Trashbin\Trashbin::class)->deleteAll(); + $this->refreshMounts(self::RECIPIENT); + + foreach (['recognize_face_detections', FsMove::$tableName, FsAccessUpdate::$tableName, FsCreation::$tableName, FsDeletion::$tableName] as $table) { + $qb = $this->db->getQueryBuilder(); + $qb->delete($table)->executeStatement(); + } + $this->jobList->remove(ProcessFsActionsJob::class); + $this->jobList->remove(ClusterFacesJob::class); + } + + public function testMoveQueuesExactlyOneAction(): void { + $file = $this->userFolder->newFile('photo.jpg', 'content'); + $this->runFsActionJobs(); + + $file->move($this->userFolder->getPath() . '/photo-1.jpg'); + self::assertEquals(1, $this->fsActionMapper->count(FsMove::class), 'moving a file should queue a move action'); + self::assertTrue($this->jobList->has(ProcessFsActionsJob::class, ['type' => FsMove::class]), 'moving a file should schedule the job processing move actions'); + + // A second move before the job ran replaces the pending action instead of failing or piling up + $file->move($this->userFolder->getPath() . '/photo-2.jpg'); + self::assertEquals(1, $this->fsActionMapper->count(FsMove::class), 'moving a file again should replace the pending move action'); + + $this->runFsActionJobs(); + self::assertEquals(0, $this->fsActionMapper->count(FsMove::class), 'the job should process all move actions'); + self::assertFalse($this->jobList->has(ProcessFsActionsJob::class, ['type' => FsMove::class]), 'the job should remove itself once all move actions are processed'); + } + + public function testMoveDuringProcessingIsNotLost(): void { + $file = $this->userFolder->newFile('photo.jpg', 'content'); + $this->runFsActionJobs(); + + $file->move($this->userFolder->getPath() . '/photo-1.jpg'); + // The job has fetched the pending action, but not processed it yet, when the file is moved again + $actions = $this->fsActionMapper->find(FsMove::class); + $file->move($this->userFolder->getPath() . '/photo-2.jpg'); + Server::get(FsActionService::class)->processActions($actions); + self::assertEquals(1, $this->fsActionMapper->count(FsMove::class), 'a move made while the job was processing should stay queued'); + } + + public function testMoveFileIntoAndOutOfSharedFolder(): void { + $shared = $this->userFolder->newFolder('shared'); + $this->shareWithRecipient($shared); + $file = $this->userFolder->newFile('photo.jpg', 'content'); + $this->addDetection($file->getId(), self::OWNER); + $this->runFsActionJobs(); + self::assertEquals([self::OWNER], $this->getUsersWithDetections($file->getId())); + + $file->move($shared->getPath() . '/photo.jpg'); + $this->runFsActionJobs(); + self::assertEquals([self::OWNER, self::RECIPIENT], $this->getUsersWithDetections($file->getId()), 'recipient should get detections for a file moved into a folder shared with them'); + self::assertTrue($this->jobList->has(ClusterFacesJob::class, ['userId' => self::RECIPIENT]), 'clustering should be scheduled for the recipient'); + + $file->move($this->userFolder->getPath() . '/photo.jpg'); + $this->runFsActionJobs(); + self::assertEquals([self::OWNER], $this->getUsersWithDetections($file->getId()), 'recipient should lose detections for a file moved out of a folder shared with them'); + } + + public function testMoveFolderWithSubfoldersIntoSharedFolder(): void { + $shared = $this->userFolder->newFolder('shared'); + $this->shareWithRecipient($shared); + $album = $this->userFolder->newFolder('album'); + $files = [ + $album->newFile('a.jpg', 'content'), + $album->newFolder('sub')->newFile('b.jpg', 'content'), + $album->get('sub')->newFolder('deeper')->newFile('c.jpg', 'content'), + ]; + foreach ($files as $file) { + $this->addDetection($file->getId(), self::OWNER); + } + $this->runFsActionJobs(); + + $album->move($shared->getPath() . '/album'); + $this->runFsActionJobs(); + foreach ($files as $file) { + self::assertEquals([self::OWNER, self::RECIPIENT], $this->getUsersWithDetections($file->getId()), 'recipient should get detections for ' . $file->getInternalPath()); + } + } + + public function testMoveFolderKeepsDetectionsOfDirectlySharedSubfolder(): void { + $album = $this->userFolder->newFolder('album'); + $sub = $album->newFolder('sub'); + $file = $sub->newFile('b.jpg', 'content'); + $this->shareWithRecipient($sub); + $this->addDetection($file->getId(), self::OWNER); + $this->addDetection($file->getId(), self::RECIPIENT); + $this->runFsActionJobs(); + + // The moved folder itself is not shared with the recipient, but the subfolder still is + $album->move($this->userFolder->getPath() . '/album-renamed'); + $this->runFsActionJobs(); + self::assertEquals([self::OWNER, self::RECIPIENT], $this->getUsersWithDetections($file->getId()), 'recipient should keep detections for a file in a subfolder shared with them directly'); + } + + public function testSharingAndUnsharingFolderUpdatesDetectionsInSubfolders(): void { + $folder = $this->userFolder->newFolder('toshare'); + $files = [ + $folder->newFile('a.jpg', 'content'), + $folder->newFolder('sub')->newFile('b.jpg', 'content'), + ]; + foreach ($files as $file) { + $this->addDetection($file->getId(), self::OWNER); + } + $this->runFsActionJobs(); + + $share = $this->shareWithRecipient($folder); + $this->runFsActionJobs(); + foreach ($files as $file) { + self::assertEquals([self::OWNER, self::RECIPIENT], $this->getUsersWithDetections($file->getId()), 'recipient should get detections after sharing for ' . $file->getInternalPath()); + } + + $this->shareManager->deleteShare($share); + $this->refreshMounts(self::RECIPIENT); + $this->runFsActionJobs(); + foreach ($files as $file) { + self::assertEquals([self::OWNER], $this->getUsersWithDetections($file->getId()), 'recipient should lose detections after unsharing for ' . $file->getInternalPath()); + } + } + + private function shareWithRecipient(Folder $folder): IShare { + $share = $this->shareManager->newShare(); + $share->setNode($folder) + ->setShareType(IShare::TYPE_USER) + ->setSharedWith(self::RECIPIENT) + ->setSharedBy(self::OWNER) + ->setPermissions(Constants::PERMISSION_ALL); + $share = $this->shareManager->createShare($share); + $this->refreshMounts(self::RECIPIENT); + return $share; + } + + /** + * Sets up the user's file system from scratch, so that the mount cache (and the listeners for + * mounts being added or removed) sees shares created or deleted in the meantime + */ + private function refreshMounts(string $userId): void { + Server::get(SetupManager::class)->tearDown(); + $this->rootFolder->getUserFolder($userId); + $this->loginAsUser(self::OWNER); + $this->userFolder = $this->rootFolder->getUserFolder(self::OWNER); + } + + private function addDetection(int $fileId, string $userId): void { + $detection = new FaceDetection(); + $detection->setFileId($fileId); + $detection->setUserId($userId); + $detection->setX(0.1); + $detection->setY(0.2); + $detection->setHeight(0.3); + $detection->setWidth(0.4); + $detection->setThreshold(0.5); + $detection->setVector([1, 2, 3, 4, 5, 6, 7, 8, 9, 0]); + $this->faceDetectionMapper->insertWithoutDeduplication($detection); + } + + /** + * @return list + */ + private function getUsersWithDetections(int $fileId): array { + $userIds = array_values(array_unique(array_map(static fn (FaceDetection $detection) => $detection->getUserId(), $this->faceDetectionMapper->findByFileId($fileId)))); + sort($userIds); + return $userIds; + } + + private function runFsActionJobs(): void { + $runs = 0; + while ($job = $this->jobList->getNext(jobClasses: [ProcessFsActionsJob::class])) { + // A job that fails to process its actions never removes itself + self::assertLessThan(100, $runs++, 'fs action jobs should finish'); + $this->jobList->resetBackgroundJob($job); + $job->start($this->jobList); + $this->jobList->resetBackgroundJob($job); + } + } +}