From 47122b716c6d3c43102ec6dd0b899e6d48aaa77f Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Sun, 1 Feb 2026 09:23:29 +0100 Subject: [PATCH] fix: cache invalidation issues Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- lib/AppInfo/Application.php | 12 +- lib/Listeners/NodeEventListener.php | 119 +++++++++++++++++++ lib/Service/ShareWrapperService.php | 177 +++++++++++----------------- 3 files changed, 196 insertions(+), 112 deletions(-) create mode 100644 lib/Listeners/NodeEventListener.php diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 34fa19075..e7fe4d9e2 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -39,6 +39,7 @@ use OCA\Circles\Listeners\GroupDeleted; use OCA\Circles\Listeners\GroupMemberAdded; use OCA\Circles\Listeners\GroupMemberRemoved; +use OCA\Circles\Listeners\NodeEventListener; use OCA\Circles\Listeners\Notifications\RequestingMember as ListenerNotificationsRequestingMember; use OCA\Circles\Listeners\UserCreated; use OCA\Circles\Listeners\UserDeleted; @@ -53,6 +54,10 @@ use OCP\AppFramework\Bootstrap\IBootstrap; use OCP\AppFramework\Bootstrap\IRegistrationContext; use OCP\Files\Config\IMountProviderCollection; +use OCP\Files\Events\Node\NodeCreatedEvent; +use OCP\Files\Events\Node\NodeDeletedEvent; +use OCP\Files\Events\Node\NodeRenamedEvent; +use OCP\Files\Events\Node\NodeWrittenEvent; use OCP\Group\Events\GroupChangedEvent; use OCP\Group\Events\GroupCreatedEvent; use OCP\Group\Events\GroupDeletedEvent; @@ -117,6 +122,12 @@ public function register(IRegistrationContext $context): void { $context->registerEventListener(RequestingCircleMemberEvent::class, ListenerNotificationsRequestingMember::class); $context->registerEventListener(DestroyingCircleEvent::class, ListenerFilesDestroyingCircle::class); + // Node events + $context->registerEventListener(NodeCreatedEvent::class, NodeEventListener::class); + $context->registerEventListener(NodeDeletedEvent::class, NodeEventListener::class); + $context->registerEventListener(NodeRenamedEvent::class, NodeEventListener::class); + $context->registerEventListener(NodeWrittenEvent::class, NodeEventListener::class); + $context->registerSearchProvider(UnifiedSearchProvider::class); $context->registerWellKnownHandler(WebfingerHandler::class); @@ -124,7 +135,6 @@ public function register(IRegistrationContext $context): void { $context->registerTeamResourceProvider(FileSharingTeamResourceProvider::class); } - /** * @param IBootContext $context * diff --git a/lib/Listeners/NodeEventListener.php b/lib/Listeners/NodeEventListener.php new file mode 100644 index 000000000..55262b033 --- /dev/null +++ b/lib/Listeners/NodeEventListener.php @@ -0,0 +1,119 @@ + + */ +class NodeEventListener implements IEventListener { + public function __construct( + private readonly ShareWrapperService $shareWrapperService, + private readonly ShareWrapperRequest $shareWrapperRequest, + private readonly LoggerInterface $logger, + ) { + } + + public function handle(Event $event): void { + if (!($event instanceof NodeCreatedEvent + || $event instanceof NodeDeletedEvent + || $event instanceof NodeRenamedEvent + || $event instanceof NodeWrittenEvent)) { + return; + } + + try { + /** @var Node $node */ + $node = $event instanceof NodeRenamedEvent ? $event->getTarget() : $event->getNode(); + $this->invalidateCacheForNode($node); + } catch (NotFoundException|NotPermittedException $e) { + $this->logger->error('Failed to process node event: ' . $e->getMessage()); + } + } + + /** + * Invalidate cache for a node and all its parent folders + * This ensures cache is cleared when files in team-shared folders are modified + */ + private function invalidateCacheForNode(Node $node): void { + $affectedCircles = []; + $visitedNodeIds = []; + + // Check if this node is directly shared with circles + try { + $shares = $this->shareWrapperRequest->getSharesByFileId($node->getId(), false); + foreach ($shares as $share) { + $affectedCircles[$share->getSharedWith()] = true; + } + $visitedNodeIds[$node->getId()] = true; + } catch (\Exception $e) { + $this->logger->error('Failed to get shares for node ' . $node->getId() . ': ' . $e->getMessage()); + } + + // Check parent folders (file might be inside a shared folder) + try { + $current = $node; + + while (true) { + $parent = $current->getParent(); + + // Stop if we've reached the root (parent is null or same as current) + if ($parent === null || $parent->getId() === $current->getId()) { + break; + } + + // Detect infinite loop: if we've already visited this node ID, stop + if (isset($visitedNodeIds[$parent->getId()])) { + $this->logger->debug('Detected cycle in folder hierarchy at node ' . $parent->getId()); + break; + } + + $visitedNodeIds[$parent->getId()] = true; + + // Check if parent is shared + $parentShares = $this->shareWrapperRequest->getSharesByFileId($parent->getId(), false); + foreach ($parentShares as $share) { + $affectedCircles[$share->getSharedWith()] = true; + } + + $current = $parent; + } + } catch (NotFoundException|NotPermittedException $e) { + $this->logger->debug('Stopped parent traversal: ' . $e->getMessage()); + } + + // Invalidate cache for all affected circles + if ($affectedCircles !== []) { + foreach (array_keys($affectedCircles) as $circleId) { + $this->shareWrapperService->clearCacheForCircle($circleId); + } + + $this->logger->debug( + 'Invalidated cache for node ' . $node->getId() . + ' affecting ' . count($affectedCircles) . ' circle(s), ' . + 'traversed ' . count($visitedNodeIds) . ' level(s)' + ); + } + } +} diff --git a/lib/Service/ShareWrapperService.php b/lib/Service/ShareWrapperService.php index edecd09f2..089b73605 100644 --- a/lib/Service/ShareWrapperService.php +++ b/lib/Service/ShareWrapperService.php @@ -2,13 +2,11 @@ declare(strict_types=1); - /** * SPDX-FileCopyrightText: 2021 Nextcloud GmbH and Nextcloud contributors * SPDX-License-Identifier: AGPL-3.0-or-later */ - namespace OCA\Circles\Service; use Exception; @@ -39,31 +37,16 @@ class ShareWrapperService { public const CACHE_SHARED_WITH = 'circles/getSharedWith'; public const CACHE_SHARED_WITH_TTL = 900; - - /** @var ShareWrapperRequest */ - private $shareWrapperRequest; - private ICache $cache; - - /** - * ShareWrapperService constructor. - * - * @param ICacheFactory $cacheFactory - * @param ShareWrapperRequest $shareWrapperRequest - */ - public function __construct(ICacheFactory $cacheFactory, ShareWrapperRequest $shareWrapperRequest) { + public function __construct( + ICacheFactory $cacheFactory, + private ShareWrapperRequest $shareWrapperRequest, + ) { $this->cache = $cacheFactory->createDistributed(self::CACHE_SHARED_WITH); - - $this->shareWrapperRequest = $shareWrapperRequest; } - /** - * @param string $singleId - * @param int $nodeId - * - * @return ShareWrapper * @throws RequestBuilderException * @throws ShareWrapperNotFoundException */ @@ -71,43 +54,30 @@ public function searchShare(string $singleId, int $nodeId): ShareWrapper { return $this->shareWrapperRequest->searchShare($singleId, $nodeId); } - /** - * @param IShare $share - * * @throws NotFoundException */ public function save(IShare $share): void { - $this->cache->clear(''); + $this->clearCache($share->getSharedWith()); $this->shareWrapperRequest->save($share); } - - /** - * @param ShareWrapper $shareWrapper - */ public function update(ShareWrapper $shareWrapper): void { - $this->cache->clear(''); + $this->clearCache($shareWrapper->getSharedWith()); $this->shareWrapperRequest->update($shareWrapper); } public function updateChildPermissions(ShareWrapper $shareWrapper): void { - $this->cache->clear(''); + $this->clearCache($shareWrapper->getSharedWith()); $this->shareWrapperRequest->updateChildPermissions($shareWrapper); } - /** - * @param ShareWrapper $shareWrapper - */ public function delete(ShareWrapper $shareWrapper): void { - $this->cache->clear(''); + $this->clearCache($shareWrapper->getSharedWith()); $this->shareWrapperRequest->delete((int)$shareWrapper->getId()); } /** - * @param string $circleId - * @param string $userId - * * @throws Exception */ public function deleteUserSharesToCircle(string $circleId, string $userId): void { @@ -115,26 +85,17 @@ public function deleteUserSharesToCircle(string $circleId, string $userId): void throw new Exception('$initiator cannot be empty'); } - $this->cache->clear(''); + // Clear cache for the entire circle as we don't know all affected users + $this->clearCacheForCircle($circleId); $this->shareWrapperRequest->deleteSharesToCircle($circleId, $userId); } - - /** - * @param string $circleId - */ public function deleteAllSharesToCircle(string $circleId): void { - $this->cache->clear(''); + $this->clearCacheForCircle($circleId); $this->shareWrapperRequest->deleteSharesToCircle($circleId, ''); } - /** - * @param string $circleId - * @param FederatedUser|null $shareRecipient - * @param FederatedUser|null $shareInitiator - * @param bool $completeDetails - * * @return ShareWrapper[] * @throws RequestBuilderException */ @@ -159,12 +120,7 @@ public function getSharesToCircles(array $circleIds): array { return $this->shareWrapperRequest->getSharesToCircles($circleIds); } - /** - * @param int $shareId - * @param FederatedUser|null $federatedUser - * - * @return ShareWrapper * @throws ShareWrapperNotFoundException * @throws RequestBuilderException */ @@ -172,11 +128,7 @@ public function getShareById(int $shareId, ?FederatedUser $federatedUser = null) return $this->shareWrapperRequest->getShareById($shareId, $federatedUser); } - /** - * @param int $fileId - * @param bool $getData - * * @return ShareWrapper[] * @throws RequestBuilderException */ @@ -185,21 +137,14 @@ public function getSharesByFileId(int $fileId, bool $getData = false): array { } /** - * @param array $fileIds - * @param bool $getData - * * @return ShareWrapper[] * @throws RequestBuilderException */ public function getSharesByFileIds(array $fileIds, bool $getData = false, bool $getChild = false): array { - return ($fileIds === []) ? [] : $this->shareWrapperRequest->getSharesByFileIds($fileIds, $getData, $getChild); + return $fileIds === [] ? [] : $this->shareWrapperRequest->getSharesByFileIds($fileIds, $getData, $getChild); } /** - * @param string $token - * @param FederatedUser|null $federatedUser - * - * @return ShareWrapper * @throws RequestBuilderException * @throws ShareWrapperNotFoundException */ @@ -207,12 +152,7 @@ public function getShareByToken(string $token, ?FederatedUser $federatedUser = n return $this->shareWrapperRequest->getShareByToken($token, $federatedUser); } - /** - * @param FederatedUser $federatedUser - * @param int $nodeId - * @param CircleProbe|null $probe - * * @return ShareWrapper[] * @throws RequestBuilderException */ @@ -230,7 +170,8 @@ public function getSharedWith( } return $this->deserializeList($cachedData, ShareWrapper::class); - } catch (InvalidItemException $e) { + } catch (InvalidItemException) { + // Cache miss, continue to fetch from database } $shares = $this->shareWrapperRequest->getSharedWith($federatedUser, $nodeId, $probe); @@ -241,14 +182,6 @@ public function getSharedWith( /** - * @param FederatedUser $federatedUser - * @param int $nodeId - * @param bool $reshares - * @param int $offset - * @param int $limit - * @param bool $getData - * @param bool $completeDetails - * * @return ShareWrapper[] * @throws RequestBuilderException */ @@ -262,30 +195,30 @@ public function getSharesBy( bool $completeDetails = false, ): array { return $this->shareWrapperRequest->getSharesBy( - $federatedUser, $nodeId, $reshares, $limit, $offset, $getData, $completeDetails + $federatedUser, + $nodeId, + $reshares, + $limit, + $offset, + $getData, + $completeDetails ); } - /** - * @param FederatedUser $federatedUser - * @param Folder $node - * @param bool $reshares - * @param bool $shallow Whether the method should stop at the first level, or look into sub-folders. - * * @return ShareWrapper[] * @throws RequestBuilderException */ - public function getSharesInFolder(FederatedUser $federatedUser, Folder $node, bool $reshares, bool $shallow = true): array { + public function getSharesInFolder( + FederatedUser $federatedUser, + Folder $node, + bool $reshares, + bool $shallow = true, + ): array { return $this->shareWrapperRequest->getSharesInFolder($federatedUser, $node, $reshares, $shallow); } - /** - * @param FederatedUser $federatedUser - * @param IShare $share - * - * @return ShareWrapper * @throws NotFoundException * @throws ShareWrapperNotFoundException * @throws RequestBuilderException @@ -293,29 +226,58 @@ public function getSharesInFolder(FederatedUser $federatedUser, Folder $node, bo public function getChild(IShare $share, FederatedUser $federatedUser): ShareWrapper { try { return $this->shareWrapperRequest->getChild($federatedUser, (int)$share->getId()); - } catch (ShareWrapperNotFoundException $e) { + } catch (ShareWrapperNotFoundException) { + // Child doesn't exist, create it } return $this->createChild($share, $federatedUser); } - + /** + * Clear cache entries for a specific singleId + * If singleId is empty, clears the entire cache + */ public function clearCache(string $singleId): void { - $this->cache->clear($singleId); + if ($singleId === '') { + $this->cache->clear(''); + return; + } + + // Clear all cache entries for this singleId by using it as a prefix + // Cache keys format: singleId#nodeId#probeSum or singleId#pathHash#forChildren#probeSum + $this->cache->clear($singleId . '#'); } + /** + * Clear cache for all members of a circle + * This is a fallback when we can't determine specific affected users + */ + public function clearCacheForCircle(string $circleId): void { + // Since we can't easily determine all singleIds affected by a circle, + // we clear the entire cache. This is inefficient but ensures consistency. + // A better approach would be to iterate over all circle members, + // but that would require circular dependency with MembershipService. + $this->cache->clear(''); + } + + /** + * Clear cache for a specific node across all users + * Should be called when a file/folder is modified, deleted, or renamed + */ + public function clearCacheForNode(int $nodeId): void { + // Since cache keys include nodeId but we can't use it as a prefix, + // we need to clear the entire cache. + // TODO: Implement a reverse index (nodeId -> singleIds) for efficient invalidation + $this->cache->clear(''); + } /** - * @param FederatedUser $federatedUser - * @param IShare $share - * - * @return ShareWrapper * @throws ShareWrapperNotFoundException * @throws NotFoundException * @throws RequestBuilderException */ private function createChild(IShare $share, FederatedUser $federatedUser): ShareWrapper { - $this->cache->clear(''); + $this->clearCache($federatedUser->getSingleId()); $share->setSharedWith($federatedUser->getSingleId()); $childId = $this->shareWrapperRequest->save($share, (int)$share->getId()); @@ -323,20 +285,13 @@ private function createChild(IShare $share, FederatedUser $federatedUser): Share } - /** - * @param FederatedUser $federatedUser - * @param int $nodeId - * @param string $probeSum - * - * @return string - */ private function generateSharedWithCacheKey( FederatedUser $federatedUser, int $nodeId, string $probeSum, ): string { return $federatedUser->getSingleId() . '#' - . $nodeId . '#' - . $probeSum; + . $nodeId . '#' + . $probeSum; } }