feat: use time-based cutoff for share updating instead of count

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

apps/files_sharing/lib/Config/ConfigLexicon.php

@@ -24,8 +24,7 @@ class ConfigLexicon implements ILexicon {
 	public const SHOW_FEDERATED_AS_INTERNAL = 'show_federated_shares_as_internal';
 	public const SHOW_FEDERATED_TO_TRUSTED_AS_INTERNAL = 'show_federated_shares_to_trusted_servers_as_internal';
 	public const EXCLUDE_RESHARE_FROM_EDIT = 'shareapi_exclude_reshare_from_edit';
-	public const UPDATE_SINGLE_CUTOFF = 'update_single_cutoff';
-	public const UPDATE_ALL_CUTOFF = 'update_all_cutoff';
+	public const UPDATE_CUTOFF_TIME = 'update_cutoff_time';
 	public const USER_NEEDS_SHARE_REFRESH = 'user_needs_share_refresh';
 
 	public function getStrictness(): Strictness {
@@ -38,8 +37,7 @@ class ConfigLexicon implements ILexicon {
 			new Entry(self::SHOW_FEDERATED_TO_TRUSTED_AS_INTERNAL, ValueType::BOOL, false, 'shows federated shares to trusted servers as internal shares', true),
 			new Entry(self::EXCLUDE_RESHARE_FROM_EDIT, ValueType::BOOL, false, 'Exclude reshare permission from "Allow editing" bundled permissions'),
 
-			new Entry(self::UPDATE_SINGLE_CUTOFF, ValueType::INT, 10, 'For how many users do we update the share data immediately for single-share updates'),
-			new Entry(self::UPDATE_ALL_CUTOFF, ValueType::INT, 3, 'For how many users do we update the share data immediately for all-share updates'),
+			new Entry(self::UPDATE_CUTOFF_TIME, ValueType::FLOAT, 3.0, 'Maximum time in second during which we update the share data immediately before switching to only marking the user'),
 		];
 	}
 

apps/files_sharing/lib/Listener/SharesUpdatedListener.php

@@ -23,6 +23,7 @@ use OCP\Share\Events\BeforeShareDeletedEvent;
 use OCP\Share\Events\ShareCreatedEvent;
 use OCP\Share\Events\ShareTransferredEvent;
 use OCP\Share\IManager;
+use Psr\Clock\ClockInterface;
 
 /**
  * Listen to various events that can change what shares a user has access to
@@ -31,26 +32,24 @@ use OCP\Share\IManager;
  */
 class SharesUpdatedListener implements IEventListener {
 	/**
-	 * for how many users do we update the share date immediately,
-	 * before just marking the other users when we know the relevant share
+	 * for how long do we update the share date immediately,
+	 * before just marking the other users
 	 */
-	private int $cutOffMarkAllSingleShare;
-	/**
-	 * for how many users do we update the share date immediately,
-	 * before just marking the other users when the relevant shares are unknown
-	 */
-	private int $cutOffMarkAllShares ;
+	private float $cutOffMarkTime;
 
-	private int $updatedCount = 0;
+	/**
+	 * The total amount of time we've spent so far processing updates
+	 */
+	private float $updatedTime = 0.0;
 
 	public function __construct(
 		private readonly IManager $shareManager,
 		private readonly ShareRecipientUpdater $shareUpdater,
 		private readonly IUserConfig $userConfig,
+		private readonly ClockInterface $clock,
 		IAppConfig $appConfig,
 	) {
-		$this->cutOffMarkAllSingleShare = $appConfig->getValueInt(Application::APP_ID, ConfigLexicon::UPDATE_SINGLE_CUTOFF, 10);
-		$this->cutOffMarkAllShares = $appConfig->getValueInt(Application::APP_ID, ConfigLexicon::UPDATE_ALL_CUTOFF, 3);
+		$this->cutOffMarkTime = $appConfig->getValueFloat(Application::APP_ID, ConfigLexicon::UPDATE_CUTOFF_TIME, 3.0);
 	}
 
 	public function handle(Event $event): void {
@@ -67,14 +66,11 @@ class SharesUpdatedListener implements IEventListener {
 			$shareTarget = $share->getTarget();
 			foreach ($this->shareManager->getUsersForShare($share) as $user) {
 				if ($share->getSharedBy() !== $user->getUID()) {
-					$this->updatedCount++;
-					if ($this->cutOffMarkAllSingleShare === -1 || $this->updatedCount <= $this->cutOffMarkAllSingleShare) {
-						$this->shareUpdater->updateForShare($user, $share);
-						// Share target validation might have changed the target, restore it for the next user
-						$share->setTarget($shareTarget);
-					} else {
-						$this->markUserForRefresh($user);
-					}
+					$this->markOrRun($user, function () use ($user, $share) {
+						$this->shareUpdater->updateForAddedShare($user, $share);
+					});
+					// Share target validation might have changed the target, restore it for the next user
+					$share->setTarget($shareTarget);
 				}
 			}
 		}
@@ -85,24 +81,28 @@ class SharesUpdatedListener implements IEventListener {
 		}
 	}
 
-	private function updateOrMarkUser(IUser $user, bool $verifyMountPoints, array $ignoreShares = []): void {
-		$this->updatedCount++;
-		if ($this->cutOffMarkAllShares === -1 || $this->updatedCount <= $this->cutOffMarkAllShares) {
-			$this->shareUpdater->updateForUser($user, $verifyMountPoints, $ignoreShares);
+	private function markOrRun(IUser $user, callable $callback): void {
+		$start = floatval($this->clock->now()->format('U.u'));
+		if ($this->cutOffMarkTime === -1.0 || $this->updatedTime < $this->cutOffMarkTime) {
+			$callback();
 		} else {
 			$this->markUserForRefresh($user);
 		}
+		$end = floatval($this->clock->now()->format('U.u'));
+		$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 markUserForRefresh(IUser $user): void {
 		$this->userConfig->setValueBool($user->getUID(), Application::APP_ID, ConfigLexicon::USER_NEEDS_SHARE_REFRESH, true);
 	}
 
-	public function setCutOffMarkAllSingleShare(int $cutOffMarkAllSingleShare): void {
-		$this->cutOffMarkAllSingleShare = $cutOffMarkAllSingleShare;
-	}
-
-	public function setCutOffMarkAllShares(int $cutOffMarkAllShares): void {
-		$this->cutOffMarkAllShares = $cutOffMarkAllShares;
+	public function setCutOffMarkTime(float|int $cutOffMarkTime): void {
+		$this->cutOffMarkTime = (float)$cutOffMarkTime;
 	}
 }

apps/files_sharing/lib/ShareRecipientUpdater.php

@@ -68,7 +68,7 @@ class ShareRecipientUpdater {
 	/**
 	 * Validate a single received share for a user
 	 */
-	public function updateForShare(IUser $user, IShare $share): void {
+	public function updateForAddedShare(IUser $user, IShare $share): void {
 		$cachedMounts = $this->userMountCache->getMountsForUser($user);
 		$mountPoints = array_map(fn (ICachedMountInfo $mount) => $mount->getMountPoint(), $cachedMounts);
 		$mountsByPath = array_combine($mountPoints, $cachedMounts);

apps/files_sharing/tests/ShareRecipientUpdaterTest.php

@@ -68,7 +68,7 @@ class ShareRecipientUpdaterTest extends \Test\TestCase {
 			->method('addMount')
 			->with($user1, '/user1/files/new-target/', $cacheEntry, MountProvider::class);
 
-		$this->updater->updateForShare($user1, $share);
+		$this->updater->updateForAddedShare($user1, $share);
 	}
 
 	/**

apps/files_sharing/tests/SharesUpdatedListenerTest.php

@@ -18,7 +18,9 @@ use OCP\Share\Events\BeforeShareDeletedEvent;
 use OCP\Share\Events\ShareCreatedEvent;
 use OCP\Share\IManager;
 use OCP\Share\IShare;
+use PHPUnit\Framework\Attributes\DataProvider;
 use PHPUnit\Framework\MockObject\MockObject;
+use Psr\Clock\ClockInterface;
 use Test\Mock\Config\MockAppConfig;
 use Test\Mock\Config\MockUserConfig;
 use Test\Traits\UserTrait;
@@ -31,6 +33,8 @@ class SharesUpdatedListenerTest extends \Test\TestCase {
 	private IManager&MockObject $manager;
 	private IUserConfig $userConfig;
 	private IAppConfig $appConfig;
+	private ClockInterface&MockObject $clock;
+	private $clockFn;
 
 	protected function setUp(): void {
 		parent::setUp();
@@ -38,14 +42,23 @@ class SharesUpdatedListenerTest extends \Test\TestCase {
 		$this->shareRecipientUpdater = $this->createMock(ShareRecipientUpdater::class);
 		$this->manager = $this->createMock(IManager::class);
 		$this->appConfig = new MockAppConfig([
-			ConfigLexicon::UPDATE_ALL_CUTOFF => -1,
-			ConfigLexicon::UPDATE_SINGLE_CUTOFF => -1,
+			ConfigLexicon::UPDATE_CUTOFF_TIME => -1,
 		]);
 		$this->userConfig = new MockUserConfig();
+		$this->clock = $this->createMock(ClockInterface::class);
+		$this->clockFn = function () {
+			return new \DateTimeImmutable('@0');
+		};
+		$this->clock->method('now')
+			->willReturnCallback(function () {
+				// extra wrapper so we can modify clockFn
+				return ($this->clockFn)();
+			});
 		$this->sharesUpdatedListener = new SharesUpdatedListener(
 			$this->manager,
 			$this->shareRecipientUpdater,
 			$this->userConfig,
+			$this->clock,
 			$this->appConfig,
 		);
 	}
@@ -62,7 +75,7 @@ class SharesUpdatedListenerTest extends \Test\TestCase {
 
 		$this->shareRecipientUpdater
 			->expects($this->exactly(2))
-			->method('updateForShare')
+			->method('updateForAddedShare')
 			->willReturnCallback(function (IUser $user, IShare $eventShare) use ($user1, $user2, $share) {
 				$this->assertContains($user, [$user1, $user2]);
 				$this->assertEquals($share, $eventShare);
@@ -85,7 +98,7 @@ class SharesUpdatedListenerTest extends \Test\TestCase {
 
 		$this->shareRecipientUpdater
 			->expects($this->exactly(1))
-			->method('updateForShare')
+			->method('updateForAddedShare')
 			->willReturnCallback(function (IUser $user, IShare $eventShare) use ($user2, $share) {
 				$this->assertEquals($user, $user2);
 				$this->assertEquals($share, $eventShare);
@@ -133,4 +146,42 @@ class SharesUpdatedListenerTest extends \Test\TestCase {
 
 		$this->sharesUpdatedListener->handle($event);
 	}
+
+	public static function shareMarkAfterTimeProvider(): array {
+		// note that each user will take exactly 1s in this test
+		return [
+			[0, 0],
+			[0.9, 1],
+			[1.1, 2],
+			[-1, 2],
+		];
+	}
+
+	#[DataProvider('shareMarkAfterTimeProvider')]
+	public function testShareMarkAfterTime(float $cutOff, int $expectedCount) {
+		$share = $this->createMock(IShare::class);
+		$user1 = $this->createUser('user1', '');
+		$user2 = $this->createUser('user2', '');
+
+		$this->manager->method('getUsersForShare')
+			->willReturn([$user1, $user2]);
+
+		$event = new ShareCreatedEvent($share);
+
+		$this->sharesUpdatedListener->setCutOffMarkTime($cutOff);
+		$time = 0;
+		$this->clockFn = function () use (&$time) {
+			$time++;
+			return new \DateTimeImmutable('@' . $time);
+		};
+
+		$this->shareRecipientUpdater
+			->expects($this->exactly($expectedCount))
+			->method('updateForAddedShare');
+
+		$this->sharesUpdatedListener->handle($event);
+
+		$this->assertEquals($expectedCount < 1, $this->userConfig->getValueBool($user1->getUID(), 'files_sharing', ConfigLexicon::USER_NEEDS_SHARE_REFRESH));
+		$this->assertEquals($expectedCount < 2, $this->userConfig->getValueBool($user2->getUID(), 'files_sharing', ConfigLexicon::USER_NEEDS_SHARE_REFRESH));
+	}
 }

apps/files_sharing/tests/TestCase.php

@@ -101,8 +101,7 @@ abstract class TestCase extends \Test\TestCase {
 		$groupBackend->addToGroup(self::TEST_FILES_SHARING_API_USER2, self::TEST_FILES_SHARING_API_GROUP1);
 		Server::get(IGroupManager::class)->addBackend($groupBackend);
 
-		Server::get(SharesUpdatedListener::class)->setCutOffMarkAllShares(-1);
-		Server::get(SharesUpdatedListener::class)->setCutOffMarkAllSingleShare(-1);
+		Server::get(SharesUpdatedListener::class)->setCutOffMarkTime(-1);
 	}
 
 	protected function setUp(): void {

build/integration/features/bootstrap/SharingContext.php

@@ -34,8 +34,7 @@ class SharingContext implements Context, SnippetAcceptingContext {
 		$this->deleteServerConfig('core', 'shareapi_allow_federation_on_public_shares');
 		$this->deleteServerConfig('files_sharing', 'outgoing_server2server_share_enabled');
 		$this->deleteServerConfig('core', 'shareapi_allow_view_without_download');
-		$this->deleteServerConfig('files_sharing', 'update_single_cutoff');
-		$this->deleteServerConfig('files_sharing', 'update_all_cutoff');
+		$this->deleteServerConfig('files_sharing', 'update_cutoff_time');
 
 		$this->runOcc(['config:system:delete', 'share_folder']);
 	}

build/integration/sharing_features/sharing-v1-part2.feature

@@ -79,8 +79,7 @@ Scenario: getting all shares of a file with reshares with link share with less p
 
 Scenario: getting all shares of a file with a received share after revoking the resharing rights with delayed share check
 	Given user "user0" exists
-	And parameter "update_single_cutoff" of app "files_sharing" is set to "0"
-	And parameter "update_all_cutoff" of app "files_sharing" is set to "0"
+	And parameter "update_cutoff_time" of app "files_sharing" is set to "0"
 	And user "user1" exists
 	And user "user2" exists
 	And file "textfile0.txt" of user "user1" is shared with user "user0"