fix: improve performance of handling delete shares

Signed-off-by: Robin Appelman <robin@icewind.nl>

Author: icewind1991

apps/files_sharing/lib/Listener/SharesUpdatedListener.php

@@ -55,11 +55,11 @@ public function __construct(
 	public function handle(Event $event): void {
 		if ($event instanceof UserShareAccessUpdatedEvent) {
 			foreach ($event->getUsers() as $user) {
-				$this->updateOrMarkUser($user, true);
+				$this->updateOrMarkUser($user);
 			}
 		}
 		if ($event instanceof UserAddedEvent || $event instanceof UserRemovedEvent) {
-			$this->updateOrMarkUser($event->getUser(), true);
+			$this->updateOrMarkUser($event->getUser());
 		}
 		if ($event instanceof ShareCreatedEvent || $event instanceof ShareTransferredEvent) {
 			$share = $event->getShare();
@@ -75,8 +75,11 @@ public function handle(Event $event): void {
 			}
 		}
 		if ($event instanceof BeforeShareDeletedEvent) {
-			foreach ($this->shareManager->getUsersForShare($event->getShare()) as $user) {
-				$this->updateOrMarkUser($user, false, [$event->getShare()]);
+			$share = $event->getShare();
+			foreach ($this->shareManager->getUsersForShare($share) as $user) {
+				$this->markOrRun($user, function () use ($user, $share) {
+					$this->shareUpdater->updateForDeletedShare($user, $share);
+				});
 			}
 		}
 	}
@@ -92,9 +95,9 @@ private function markOrRun(IUser $user, callable $callback): void {
 		$this->updatedTime += $end - $start;
 	}
 
-	private function updateOrMarkUser(IUser $user, bool $verifyMountPoints, array $ignoreShares = []): void {
-		$this->markOrRun($user, function () use ($user, $verifyMountPoints, $ignoreShares) {
-			$this->shareUpdater->updateForUser($user, $verifyMountPoints, $ignoreShares);
+	private function updateOrMarkUser(IUser $user): void {
+		$this->markOrRun($user, function () use ($user) {
+			$this->shareUpdater->updateForUser($user);
 		});
 	}
 

apps/files_sharing/lib/ShareRecipientUpdater.php

@@ -28,7 +28,7 @@ public function __construct(
 	/**
 	 * Validate all received shares for a user
 	 */
-	public function updateForUser(IUser $user, bool $verifyMountPoints = true, array $ignoreShares = []): void {
+	public function updateForUser(IUser $user): void {
 		// prevent recursion
 		if (isset($this->inUpdate[$user->getUID()])) {
 			return;
@@ -40,20 +40,18 @@ public function updateForUser(IUser $user, bool $verifyMountPoints = true, array
 		$mountPoints = array_map(fn (ICachedMountInfo $mount) => $mount->getMountPoint(), $cachedMounts);
 		$mountsByPath = array_combine($mountPoints, $cachedMounts);
 
-		$shares = $this->shareMountProvider->getSuperSharesForUser($user, $ignoreShares);
+		$shares = $this->shareMountProvider->getSuperSharesForUser($user);
 
 		// the share mounts have changed if either the number of shares doesn't matched the number of share mounts
 		// or there is a share for which we don't have a mount yet.
 		$mountsChanged = count($shares) !== count($shareMounts);
-		foreach ($shares as &$share) {
+		foreach ($shares as $share) {
 			[$parentShare, $groupedShares] = $share;
 			$mountPoint = '/' . $user->getUID() . '/files/' . trim($parentShare->getTarget(), '/') . '/';
 			$mountKey = $parentShare->getNodeId() . '::' . $mountPoint;
 			if (!isset($cachedMounts[$mountKey])) {
 				$mountsChanged = true;
-				if ($verifyMountPoints) {
-					$this->shareTargetValidator->verifyMountPoint($user, $parentShare, $mountsByPath, $groupedShares);
-				}
+				$this->shareTargetValidator->verifyMountPoint($user, $parentShare, $mountsByPath, $groupedShares);
 			}
 		}
 
@@ -78,4 +76,13 @@ public function updateForAddedShare(IUser $user, IShare $share): void {
 
 		$this->userMountCache->addMount($user, $mountPoint, $share->getNode()->getData(), MountProvider::class);
 	}
+
+	/**
+	 * Process a single deleted share for a user
+	 */
+	public function updateForDeletedShare(IUser $user, IShare $share): void {
+		$mountPoint = '/' . $user->getUID() . '/files/' . trim($share->getTarget(), '/') . '/';
+
+		$this->userMountCache->removeMount($mountPoint);
+	}
 }

apps/files_sharing/tests/ShareRecipientUpdaterTest.php

@@ -185,4 +185,22 @@ public function testUpdateForUserRemoved() {
 
 		$this->updater->updateForUser($user1);
 	}
+
+	public function testDeletedShare() {
+		$share = $this->createMock(IShare::class);
+		$share->method('getTarget')
+			->willReturn('/target');
+		$share->method('getNodeId')
+			->willReturn(111);
+		$user1 = $this->createUser('user1', '');
+
+		$this->shareTargetValidator->expects($this->never())
+			->method('verifyMountPoint');
+
+		$this->userMountCache->expects($this->exactly(1))
+			->method('removeMount')
+			->with('/user1/files/target/');
+
+		$this->updater->updateForDeletedShare($user1, $share);
+	}
 }

apps/files_sharing/tests/SharesUpdatedListenerTest.php

@@ -116,10 +116,8 @@ public function testShareAccessUpdated() {
 		$this->shareRecipientUpdater
 			->expects($this->exactly(2))
 			->method('updateForUser')
-			->willReturnCallback(function (IUser $user, bool $verifyMountPoints = true, array $ignoreShares = []) use ($user1, $user2) {
+			->willReturnCallback(function (IUser $user) use ($user1, $user2) {
 				$this->assertContains($user, [$user1, $user2]);
-				$this->assertEquals(true, $verifyMountPoints);
-				$this->assertEquals([], $ignoreShares);
 			});
 
 		$this->sharesUpdatedListener->handle($event);
@@ -137,11 +135,9 @@ public function testShareDeleted() {
 
 		$this->shareRecipientUpdater
 			->expects($this->exactly(2))
-			->method('updateForUser')
-			->willReturnCallback(function (IUser $user, bool $verifyMountPoints = true, array $ignoreShares = []) use ($user1, $user2, $share) {
+			->method('updateForDeletedShare')
+			->willReturnCallback(function (IUser $user) use ($user1, $user2, $share) {
 				$this->assertContains($user, [$user1, $user2]);
-				$this->assertEquals(false, $verifyMountPoints);
-				$this->assertEquals([$share], $ignoreShares);
 			});
 
 		$this->sharesUpdatedListener->handle($event);