From 16e49c3b23da49c4e578735c32195a2fc31ff0bf Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Wed, 23 Sep 2026 15:18:11 +0200 Subject: [PATCH 1/8] fix: Small fixes Signed-off-by: Marcel Klehr --- lib/Db/FsActionMapper.php | 56 +++++++++++++++++++++++++++++++++ lib/Service/FsActionService.php | 4 ++- 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/lib/Db/FsActionMapper.php b/lib/Db/FsActionMapper.php index 56d46585c..c4f4ae5af 100644 --- a/lib/Db/FsActionMapper.php +++ b/lib/Db/FsActionMapper.php @@ -235,7 +235,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); @@ -286,6 +298,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 23bd2b643..46c7b3b5f 100644 --- a/lib/Service/FsActionService.php +++ b/lib/Service/FsActionService.php @@ -336,7 +336,9 @@ private function onMove(string $ownerId, array $usersToAdd, array $targetUserIds 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($ownerId, $usersToAdd, $targetUserIds, $n); From c56af16586fe54572afe4f2851b8167c14882231 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Thu, 24 Sep 2026 11:08:24 +0200 Subject: [PATCH 2/8] 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 c4f4ae5af..abce84de3 100644 --- a/lib/Db/FsActionMapper.php +++ b/lib/Db/FsActionMapper.php @@ -308,7 +308,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 68304d8faa846baa01f0e590105db39e4cad4660 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Thu, 24 Sep 2026 11:11:08 +0200 Subject: [PATCH 3/8] 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 | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/Service/FsActionService.php b/lib/Service/FsActionService.php index 46c7b3b5f..0a579a983 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) { @@ -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(); @@ -343,8 +346,6 @@ private function onMove(string $ownerId, array $usersToAdd, array $targetUserIds } $this->onMove($ownerId, $usersToAdd, $targetUserIds, $n); } - } catch (NotFoundException|Exception|InvalidPathException $e) { - $this->logger->warning('Error in recognize file listener', ['exception' => $e]); } return; } From 9edc44f3a534a078df648c83980b6753261a6fd7 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Thu, 24 Sep 2026 12:44:08 +0200 Subject: [PATCH 4/8] fix(onAccessUpdate): Correct user access for shared subdirectories Assisted-by: ClaudeCode:claude-opus-5.5 Signed-off-by: Marcel Klehr --- lib/Service/FsActionService.php | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/Service/FsActionService.php b/lib/Service/FsActionService.php index 0a579a983..2c0b4465a 100644 --- a/lib/Service/FsActionService.php +++ b/lib/Service/FsActionService.php @@ -167,7 +167,6 @@ 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) { From 6e949e7396c92c41add096d6bc7536bab781fdc4 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Thu, 24 Sep 2026 12:47:29 +0200 Subject: [PATCH 5/8] fix(insertMove): Use a transaction Assisted-by: ClaudeCode:claude-opus-5.5 Signed-off-by: Marcel Klehr --- lib/Db/FsActionMapper.php | 82 ++++++++++++--------------------------- 1 file changed, 25 insertions(+), 57 deletions(-) diff --git a/lib/Db/FsActionMapper.php b/lib/Db/FsActionMapper.php index abce84de3..1a68d6d89 100644 --- a/lib/Db/FsActionMapper.php +++ b/lib/Db/FsActionMapper.php @@ -231,24 +231,36 @@ 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); @@ -298,50 +310,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 82068aa652c08ed3c9df60a75877fdb23630e23d Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Thu, 24 Sep 2026 12:58:09 +0200 Subject: [PATCH 6/8] 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 | 9 +--- lib/Hooks/FileListener.php | 27 +---------- .../Version013002000Date20260924120000.php | 47 +++++++++++++++++++ 4 files changed, 56 insertions(+), 60 deletions(-) create mode 100644 lib/Migration/Version013002000Date20260924120000.php diff --git a/lib/Db/FsActionMapper.php b/lib/Db/FsActionMapper.php index 1a68d6d89..f5ac0753e 100644 --- a/lib/Db/FsActionMapper.php +++ b/lib/Db/FsActionMapper.php @@ -227,33 +227,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(); @@ -263,9 +245,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); $arguments = [ 'type' => FsDeletion::class ]; if (!$this->jobList->has(ProcessFsActionsJob::class, $arguments)) { diff --git a/lib/Db/FsMove.php b/lib/Db/FsMove.php index 198994b24..4abc004db 100644 --- a/lib/Db/FsMove.php +++ b/lib/Db/FsMove.php @@ -15,24 +15,19 @@ * @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'; diff --git a/lib/Hooks/FileListener.php b/lib/Hooks/FileListener.php index 1426b6384..3cb3f0bd4 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) { 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 9c722437338dd78102cf32c2ec8c15402dfcd9bf Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Thu, 24 Sep 2026 13:01:15 +0200 Subject: [PATCH 7/8] fix: Update psalm baseline Signed-off-by: Marcel Klehr --- psalm-baseline.xml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 3cbe274bb..e6f2bc3c3 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -965,7 +965,12 @@ + + + + getUserId()]]> + From 0d2445102be019f24edcd31ab6dda61d69fc028b Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Thu, 24 Sep 2026 13:02:23 +0200 Subject: [PATCH 8/8] fix: Make migration run Assisted-by: ClaudeCode:claude-opus-5.5 Signed-off-by: Marcel Klehr