From edd55468782dbf70156830e2604fbf5d0718eb46 Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Wed, 6 May 2026 23:33:44 +0200 Subject: [PATCH 1/2] refactor: flatten nesting with early returns and extract helper methods - Data::setOffsetFromSince(): invert guards with early returns and extract the repeated fallback fetch into getFirstKnownActivityHeader() - MailQueueHandler::getAffectedUsers(): early return in the restrictEmails branch and extract the duplicated result loop into fetchAffectedUsers() Signed-off-by: Anna Larch AI-Assisted-By: Claude Sonnet 4.6 --- lib/Data.php | 62 ++++++++++++++++++++-------------------- lib/MailQueueHandler.php | 17 ++++------- 2 files changed, 36 insertions(+), 43 deletions(-) diff --git a/lib/Data.php b/lib/Data.php index 79d737b5c..510f1d7b3 100644 --- a/lib/Data.php +++ b/lib/Data.php @@ -324,35 +324,38 @@ public function get(GroupHelper $groupHelper, UserSettings $userSettings, $user, * @throws \OutOfBoundsException If $since is not owned by $user */ protected function setOffsetFromSince(IQueryBuilder $query, $user, $since, $sort) { - if ($since) { - $queryBuilder = $this->connection->getQueryBuilder(); - $queryBuilder->select(['affecteduser', 'timestamp']) - ->from('activity') - ->where($queryBuilder->expr()->eq('activity_id', $queryBuilder->createNamedParameter((int)$since))); - $result = $queryBuilder->executeQuery(); - $activity = $result->fetch(); - $result->closeCursor(); - - if ($activity) { - if ($activity['affecteduser'] !== $user) { - throw new \OutOfBoundsException('Invalid since', 2); - } - $timestamp = (int)$activity['timestamp']; - - if ($sort === 'DESC') { - $query->andWhere($query->expr()->lte('timestamp', $query->createNamedParameter($timestamp))); - $query->andWhere($query->expr()->lt('activity_id', $query->createNamedParameter($since))); - } else { - $query->andWhere($query->expr()->gte('timestamp', $query->createNamedParameter($timestamp))); - $query->andWhere($query->expr()->gt('activity_id', $query->createNamedParameter($since))); - } - return []; - } + if (!$since) { + return $this->getFirstKnownActivityHeader($user, $sort); } - /** - * Couldn't find the since, so find the oldest one and set the header - */ + $queryBuilder = $this->connection->getQueryBuilder(); + $queryBuilder->select(['affecteduser', 'timestamp']) + ->from('activity') + ->where($queryBuilder->expr()->eq('activity_id', $queryBuilder->createNamedParameter((int)$since))); + $result = $queryBuilder->executeQuery(); + $activity = $result->fetch(); + $result->closeCursor(); + + if (!$activity) { + return $this->getFirstKnownActivityHeader($user, $sort); + } + + if ($activity['affecteduser'] !== $user) { + throw new \OutOfBoundsException('Invalid since', 2); + } + + $timestamp = (int)$activity['timestamp']; + if ($sort === 'DESC') { + $query->andWhere($query->expr()->lte('timestamp', $query->createNamedParameter($timestamp))); + $query->andWhere($query->expr()->lt('activity_id', $query->createNamedParameter($since))); + } else { + $query->andWhere($query->expr()->gte('timestamp', $query->createNamedParameter($timestamp))); + $query->andWhere($query->expr()->gt('activity_id', $query->createNamedParameter($since))); + } + return []; + } + + private function getFirstKnownActivityHeader(string $user, string $sort): array { $fetchQuery = $this->connection->getQueryBuilder(); $fetchQuery->select('activity_id') ->from('activity') @@ -364,11 +367,8 @@ protected function setOffsetFromSince(IQueryBuilder $query, $user, $since, $sort $result->closeCursor(); if ($activity !== false) { - return [ - 'X-Activity-First-Known' => (int)$activity['activity_id'], - ]; + return ['X-Activity-First-Known' => (int)$activity['activity_id']]; } - return []; } diff --git a/lib/MailQueueHandler.php b/lib/MailQueueHandler.php index 55b2fe4ed..58efc0c6c 100644 --- a/lib/MailQueueHandler.php +++ b/lib/MailQueueHandler.php @@ -156,16 +156,7 @@ protected function getAffectedUsers(?int $limit, int $latestSend, bool $forceSen } elseif ($restrictEmails === UserSettings::EMAIL_SEND_ASAP) { $query->where($query->expr()->eq('amq_timestamp', 'amq_latest_send')); } - - $result = $query->executeQuery(); - - $affectedUsers = []; - while ($row = $result->fetch()) { - $affectedUsers[] = $row['amq_affecteduser']; - } - $result->closeCursor(); - - return $affectedUsers; + return $this->fetchAffectedUsers($query); } if ($forceSending) { @@ -174,14 +165,16 @@ protected function getAffectedUsers(?int $limit, int $latestSend, bool $forceSen $query->where($query->expr()->lt('amq_latest_send', $query->createNamedParameter($latestSend))); } - $result = $query->executeQuery(); + return $this->fetchAffectedUsers($query); + } + private function fetchAffectedUsers(IQueryBuilder $query): array { + $result = $query->executeQuery(); $affectedUsers = []; while ($row = $result->fetch()) { $affectedUsers[] = $row['amq_affecteduser']; } $result->closeCursor(); - return $affectedUsers; } From 2b6b29b083b6878525ed4fb0bb75646994bea4f2 Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Wed, 6 May 2026 23:49:32 +0200 Subject: [PATCH 2/2] refactor: flatten nesting with early returns in FilesHooks and ViewInfoCache refactor: flatten nesting with early returns in FilesHooks and ViewInfoCache - FilesHooks::unShare(): invert guard clause to early return; add tests covering the guard (non-file/folder nodeType, deleted node) and all dispatch branches (TYPE_USER, TYPE_GROUP, TYPE_LINK) - FilesHooks::unShareSelf(): same guard-inversion treatment - ViewInfoCache::findInfoById(): replace nested try-catch and $notFound flag with sequential try-catch blocks and early returns Signed-off-by: Anna Larch AI-Assisted-By: Claude Sonnet 4.6 [skip ci] --- lib/FilesHooks.php | 30 ++++++++------- lib/ViewInfoCache.php | 20 +++++++--- tests/FilesHooksTest.php | 81 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+), 19 deletions(-) diff --git a/lib/FilesHooks.php b/lib/FilesHooks.php index 33d999127..8c982183a 100644 --- a/lib/FilesHooks.php +++ b/lib/FilesHooks.php @@ -810,14 +810,15 @@ protected function shareWithTeam(string $shareWith, Node $fileSource, string $fi * @throws \OCP\Files\NotFoundException */ public function unShare(IShare $share) { - if (in_array($share->getNodeType(), ['file', 'folder'], true) && !$this->isDeletedNode($share->getShareOwner(), $share->getNodeId())) { - if ($share->getShareType() === IShare::TYPE_USER) { - $this->unshareFromUser($share); - } elseif ($share->getShareType() === IShare::TYPE_GROUP) { - $this->unshareFromGroup($share); - } elseif ($share->getShareType() === IShare::TYPE_LINK) { - $this->unshareLink($share); - } + if (!in_array($share->getNodeType(), ['file', 'folder'], true) || $this->isDeletedNode($share->getShareOwner(), $share->getNodeId())) { + return; + } + if ($share->getShareType() === IShare::TYPE_USER) { + $this->unshareFromUser($share); + } elseif ($share->getShareType() === IShare::TYPE_GROUP) { + $this->unshareFromGroup($share); + } elseif ($share->getShareType() === IShare::TYPE_LINK) { + $this->unshareLink($share); } } @@ -828,12 +829,13 @@ public function unShare(IShare $share) { * @throws \OCP\Files\NotFoundException */ public function unShareSelf(IShare $share) { - if (in_array($share->getNodeType(), ['file', 'folder'], true)) { - if ($share->getShareType() === IShare::TYPE_GROUP) { - $this->unshareFromSelfGroup($share); - } elseif ($share->getShareType() === IShare::TYPE_USER) { - $this->unshareFromUser($share); - } + if (!in_array($share->getNodeType(), ['file', 'folder'], true)) { + return; + } + if ($share->getShareType() === IShare::TYPE_GROUP) { + $this->unshareFromSelfGroup($share); + } elseif ($share->getShareType() === IShare::TYPE_USER) { + $this->unshareFromUser($share); } } diff --git a/lib/ViewInfoCache.php b/lib/ViewInfoCache.php index 0405fd0ab..0d56a406e 100644 --- a/lib/ViewInfoCache.php +++ b/lib/ViewInfoCache.php @@ -56,7 +56,6 @@ protected function findInfoById($user, $fileId, $filePath) { 'view' => '', ]; - $notFound = false; try { $userFolder = $this->rootFolder->getUserFolder($user); $entries = $userFolder->getById($fileId); @@ -94,13 +93,24 @@ protected function findInfoById($user, $fileId, $filePath) { } catch (NotFoundException $e) { $notFound = true; } + $entry = $userTrashBin->getFirstNodeById($fileId); + if ($entry === null) { + throw new NotFoundException('No entries returned'); + } + $cache = [ + 'path' => $userTrashBin->getRelativePath($entry->getPath()), + 'exists' => true, + 'is_dir' => $entry instanceof Folder, + 'view' => 'trashbin', + 'node' => $entry, + ]; + } catch (NotFoundException) { + // Not found anywhere — cache path as null but return original filePath + $this->cacheId[$user][$fileId] = array_merge($cache, ['path' => null]); + return $cache; } $this->cacheId[$user][$fileId] = $cache; - if ($notFound) { - $this->cacheId[$user][$fileId]['path'] = null; - } - return $cache; } } diff --git a/tests/FilesHooksTest.php b/tests/FilesHooksTest.php index 33f025750..2ce305063 100644 --- a/tests/FilesHooksTest.php +++ b/tests/FilesHooksTest.php @@ -1189,4 +1189,85 @@ public function testGetUserPathsFromPathSuccess(): void { $this->assertSame(['remote1' => ['token' => 'abc']], $result['remotes']); $this->assertSame('/test/path', $result['ownerPath']); } + + private function getShareMock(string $nodeType, int $shareType, string $owner = 'owner', int $nodeId = 42): IShare&MockObject { + $share = $this->createMock(IShare::class); + $share->method('getNodeType')->willReturn($nodeType); + $share->method('getShareType')->willReturn($shareType); + $share->method('getShareOwner')->willReturn($owner); + $share->method('getNodeId')->willReturn($nodeId); + return $share; + } + + private function mockNodeNotDeleted(string $owner, int $nodeId): void { + $node = $this->createMock(File::class); + $userFolder = $this->createMock(Folder::class); + $userFolder->method('getFirstNodeById')->with($nodeId)->willReturn($node); + $this->rootFolder->method('getUserFolder')->with($owner)->willReturn($userFolder); + } + + private function mockNodeDeleted(string $owner): void { + $this->rootFolder->method('getUserFolder') + ->with($owner) + ->willThrowException(new NotFoundException()); + } + + public function testUnShareIgnoresNonFileOrFolder(): void { + $filesHooks = $this->getFilesHooks(['unshareFromUser', 'unshareFromGroup', 'unshareLink']); + $share = $this->getShareMock('other', IShare::TYPE_USER); + + $filesHooks->expects($this->never())->method('unshareFromUser'); + $filesHooks->expects($this->never())->method('unshareFromGroup'); + $filesHooks->expects($this->never())->method('unshareLink'); + + $filesHooks->unShare($share); + } + + public function testUnShareIgnoresDeletedNode(): void { + $filesHooks = $this->getFilesHooks(['unshareFromUser', 'unshareFromGroup', 'unshareLink']); + $share = $this->getShareMock('file', IShare::TYPE_USER); + $this->mockNodeDeleted('owner'); + + $filesHooks->expects($this->never())->method('unshareFromUser'); + $filesHooks->expects($this->never())->method('unshareFromGroup'); + $filesHooks->expects($this->never())->method('unshareLink'); + + $filesHooks->unShare($share); + } + + public function testUnShareUser(): void { + $filesHooks = $this->getFilesHooks(['unshareFromUser', 'unshareFromGroup', 'unshareLink']); + $share = $this->getShareMock('file', IShare::TYPE_USER); + $this->mockNodeNotDeleted('owner', 42); + + $filesHooks->expects($this->once())->method('unshareFromUser')->with($share); + $filesHooks->expects($this->never())->method('unshareFromGroup'); + $filesHooks->expects($this->never())->method('unshareLink'); + + $filesHooks->unShare($share); + } + + public function testUnShareGroup(): void { + $filesHooks = $this->getFilesHooks(['unshareFromUser', 'unshareFromGroup', 'unshareLink']); + $share = $this->getShareMock('folder', IShare::TYPE_GROUP); + $this->mockNodeNotDeleted('owner', 42); + + $filesHooks->expects($this->never())->method('unshareFromUser'); + $filesHooks->expects($this->once())->method('unshareFromGroup')->with($share); + $filesHooks->expects($this->never())->method('unshareLink'); + + $filesHooks->unShare($share); + } + + public function testUnShareLink(): void { + $filesHooks = $this->getFilesHooks(['unshareFromUser', 'unshareFromGroup', 'unshareLink']); + $share = $this->getShareMock('file', IShare::TYPE_LINK); + $this->mockNodeNotDeleted('owner', 42); + + $filesHooks->expects($this->never())->method('unshareFromUser'); + $filesHooks->expects($this->never())->method('unshareFromGroup'); + $filesHooks->expects($this->once())->method('unshareLink')->with($share); + + $filesHooks->unShare($share); + } }