From 6c9898f89d1433cdbcbf7cbc0a9ea0326470bb37 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 30 Jan 2026 16:18:24 +0100 Subject: [PATCH 1/2] fix: don't rely on share providers being avaiable in CleanupShareTarget Signed-off-by: Robin Appelman --- .../lib/Repair/CleanupShareTarget.php | 47 ++++++++++++------- .../lib/ShareTargetValidator.php | 12 ++--- 2 files changed, 36 insertions(+), 23 deletions(-) diff --git a/apps/files_sharing/lib/Repair/CleanupShareTarget.php b/apps/files_sharing/lib/Repair/CleanupShareTarget.php index 6a97bc48913..1c6706d99a2 100644 --- a/apps/files_sharing/lib/Repair/CleanupShareTarget.php +++ b/apps/files_sharing/lib/Repair/CleanupShareTarget.php @@ -13,14 +13,16 @@ use OCA\Files_Sharing\ShareTargetValidator; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\Files\Config\ICachedMountInfo; use OCP\Files\Config\IUserMountCache; +use OCP\Files\IRootFolder; use OCP\IDBConnection; use OCP\IUserManager; use OCP\Migration\IOutput; use OCP\Migration\IRepairStep; -use OCP\Share\IManager; -use OCP\Share\IProviderFactory; use OCP\Share\IShare; +/** + * @psalm-type ShareInfo = array{id: string|int, share_type: string, share_with: string, file_source: string, file_target: string} + */ class CleanupShareTarget implements IRepairStep { /** we only care about shares with a user target, * since the underling group/deck/talk share doesn't get moved @@ -34,12 +36,11 @@ class CleanupShareTarget implements IRepairStep { public function __construct( private readonly IDBConnection $connection, - private readonly IManager $shareManager, - private readonly IProviderFactory $shareProviderFactory, private readonly ShareTargetValidator $shareTargetValidator, private readonly IUserManager $userManager, private readonly SetupManager $setupManager, private readonly IUserMountCache $userMountCache, + private readonly IRootFolder $rootFolder, ) { } @@ -58,12 +59,10 @@ class CleanupShareTarget implements IRepairStep { $lastUser = ''; $userMounts = []; + $userFolder = null; foreach ($this->getProblemShares() as $shareInfo) { $recipient = $this->userManager->getExistingUser($shareInfo['share_with']); - $share = $this->shareProviderFactory - ->getProviderForType((int)$shareInfo['share_type']) - ->getShareById($shareInfo['id'], $recipient->getUID()); // since we ordered the share by user, we can reuse the last data until we get to the next user if ($lastUser !== $recipient->getUID()) { @@ -74,19 +73,23 @@ class CleanupShareTarget implements IRepairStep { $mounts = $this->userMountCache->getMountsForUser($recipient); $mountPoints = array_map(fn (ICachedMountInfo $mount) => $mount->getMountPoint(), $mounts); $userMounts = array_combine($mountPoints, $mounts); + $userFolder = $this->rootFolder->getUserFolder($recipient->getUID()); } - $oldTarget = $share->getTarget(); + $oldTarget = $shareInfo['file_target']; $newTarget = $this->cleanTarget($oldTarget); - $share->setTarget($newTarget); - $this->shareManager->moveShare($share, $recipient->getUID()); + $absoluteNewTarget = $userFolder->getFullPath($newTarget); + $targetParentNode = $this->rootFolder->get(dirname($absoluteNewTarget)); - $this->shareTargetValidator->verifyMountPoint( - $recipient, - $share, + $absoluteNewTarget = $this->shareTargetValidator->generateUniqueTarget( + (int)$shareInfo['file_source'], + $absoluteNewTarget, + $targetParentNode->getMountPoint(), $userMounts, - [$share], ); + $newTarget = $userFolder->getRelativePath($absoluteNewTarget); + + $this->moveShare((string)$shareInfo['id'], $newTarget); $oldMountPoint = "/{$recipient->getUID()}/files$oldTarget/"; $newMountPoint = "/{$recipient->getUID()}/files$newTarget/"; @@ -108,19 +111,29 @@ class CleanupShareTarget implements IRepairStep { return (int)$query->executeQuery()->fetchOne(); } + private function moveShare(string $id, string $target): void { + // since we only process user-specific shares, we can just move them + // without having to check if we need to create a user-specific override + $query = $this->connection->getQueryBuilder(); + $query->update('share') + ->set('file_target', $query->createNamedParameter($target)) + ->where($query->expr()->eq('id', $query->createNamedParameter($id))) + ->executeStatement(); + } + /** - * @return \Traversable + * @return \Traversable */ private function getProblemShares(): \Traversable { $query = $this->connection->getQueryBuilder(); - $query->select('id', 'share_type', 'share_with') + $query->select('id', 'share_type', 'share_with', 'file_source', 'file_target') ->from('share') ->where($query->expr()->like('file_target', $query->createNamedParameter('% (_) (_)%'))) ->andWhere($query->expr()->in('share_type', $query->createNamedParameter(self::USER_SHARE_TYPES, IQueryBuilder::PARAM_INT_ARRAY), IQueryBuilder::PARAM_INT_ARRAY)) ->orderBy('share_with') ->addOrderBy('id'); $result = $query->executeQuery(); - /** @var \Traversable $rows */ + /** @var \Traversable $rows */ $rows = $result->iterateAssociative(); return $rows; } diff --git a/apps/files_sharing/lib/ShareTargetValidator.php b/apps/files_sharing/lib/ShareTargetValidator.php index b47dde11381..554f76c0dca 100644 --- a/apps/files_sharing/lib/ShareTargetValidator.php +++ b/apps/files_sharing/lib/ShareTargetValidator.php @@ -85,7 +85,7 @@ class ShareTargetValidator { } $newAbsoluteMountPoint = $this->generateUniqueTarget( - $share, + $share->getNodeId(), Filesystem::normalizePath($absoluteParent . '/' . $mountPoint), $parentMount, $allCachedMounts, @@ -108,8 +108,8 @@ class ShareTargetValidator { /** * @param ICachedMountInfo[] $allCachedMounts */ - private function generateUniqueTarget( - IShare $share, + public function generateUniqueTarget( + int $shareNodeId, string $absolutePath, IMountPoint $parentMount, array $allCachedMounts, @@ -122,7 +122,7 @@ class ShareTargetValidator { $i = 2; $parentCache = $parentMount->getStorage()->getCache(); $internalPath = $parentMount->getInternalPath($absolutePath); - while ($parentCache->inCache($internalPath) || $this->hasConflictingMount($share, $allCachedMounts, $absolutePath)) { + while ($parentCache->inCache($internalPath) || $this->hasConflictingMount($shareNodeId, $allCachedMounts, $absolutePath)) { $absolutePath = Filesystem::normalizePath($dir . '/' . $name . ' (' . $i . ')' . $ext); $internalPath = $parentMount->getInternalPath($absolutePath); $i++; @@ -134,13 +134,13 @@ class ShareTargetValidator { /** * @param ICachedMountInfo[] $allCachedMounts */ - private function hasConflictingMount(IShare $share, array $allCachedMounts, string $absolutePath): bool { + private function hasConflictingMount(int $shareNodeId, array $allCachedMounts, string $absolutePath): bool { if (!isset($allCachedMounts[$absolutePath . '/'])) { return false; } $mount = $allCachedMounts[$absolutePath . '/']; - if ($mount->getMountProvider() === MountProvider::class && $mount->getRootId() === $share->getNodeId()) { + if ($mount->getMountProvider() === MountProvider::class && $mount->getRootId() === $shareNodeId) { // "conflicting" mount is a mount for the current share return false; } From 0ce5cbab8397fcaa33e6a938e5726ba7dc60aae4 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 2 Feb 2026 20:04:58 +0100 Subject: [PATCH 2/2] fix: don't stop the entire share target repair on an error Signed-off-by: Robin Appelman --- .../lib/Repair/CleanupShareTarget.php | 35 ++++++++++++------- 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/apps/files_sharing/lib/Repair/CleanupShareTarget.php b/apps/files_sharing/lib/Repair/CleanupShareTarget.php index 1c6706d99a2..6ffe50709db 100644 --- a/apps/files_sharing/lib/Repair/CleanupShareTarget.php +++ b/apps/files_sharing/lib/Repair/CleanupShareTarget.php @@ -19,6 +19,7 @@ use OCP\IUserManager; use OCP\Migration\IOutput; use OCP\Migration\IRepairStep; use OCP\Share\IShare; +use Psr\Log\LoggerInterface; /** * @psalm-type ShareInfo = array{id: string|int, share_type: string, share_with: string, file_source: string, file_target: string} @@ -41,6 +42,7 @@ class CleanupShareTarget implements IRepairStep { private readonly SetupManager $setupManager, private readonly IUserMountCache $userMountCache, private readonly IRootFolder $rootFolder, + private readonly LoggerInterface $logger, ) { } @@ -63,6 +65,9 @@ class CleanupShareTarget implements IRepairStep { foreach ($this->getProblemShares() as $shareInfo) { $recipient = $this->userManager->getExistingUser($shareInfo['share_with']); + if (!$recipient->isEnabled()) { + continue; + } // since we ordered the share by user, we can reuse the last data until we get to the next user if ($lastUser !== $recipient->getUID()) { @@ -81,20 +86,26 @@ class CleanupShareTarget implements IRepairStep { $absoluteNewTarget = $userFolder->getFullPath($newTarget); $targetParentNode = $this->rootFolder->get(dirname($absoluteNewTarget)); - $absoluteNewTarget = $this->shareTargetValidator->generateUniqueTarget( - (int)$shareInfo['file_source'], - $absoluteNewTarget, - $targetParentNode->getMountPoint(), - $userMounts, - ); - $newTarget = $userFolder->getRelativePath($absoluteNewTarget); + try { + $absoluteNewTarget = $this->shareTargetValidator->generateUniqueTarget( + (int)$shareInfo['file_source'], + $absoluteNewTarget, + $targetParentNode->getMountPoint(), + $userMounts, + ); + $newTarget = $userFolder->getRelativePath($absoluteNewTarget); - $this->moveShare((string)$shareInfo['id'], $newTarget); + $this->moveShare((string)$shareInfo['id'], $newTarget); - $oldMountPoint = "/{$recipient->getUID()}/files$oldTarget/"; - $newMountPoint = "/{$recipient->getUID()}/files$newTarget/"; - $userMounts[$newMountPoint] = $userMounts[$oldMountPoint]; - unset($userMounts[$oldMountPoint]); + $oldMountPoint = "/{$recipient->getUID()}/files$oldTarget/"; + $newMountPoint = "/{$recipient->getUID()}/files$newTarget/"; + $userMounts[$newMountPoint] = $userMounts[$oldMountPoint]; + unset($userMounts[$oldMountPoint]); + } catch (\Exception $e) { + $msg = 'error cleaning up share target: ' . $e->getMessage(); + $this->logger->error($msg, ['exception' => $e, 'app' => 'files_sharing']); + $output->warning($msg); + } $output->advance(); }