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

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

Author: icewind1991

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 @@ public function getAppConfigs(): array {
 			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, 'For how how long do 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\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 @@
  */
 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;
+	private float $cutOffMarkTime;
+
 	/**
-	 * for how many users do we update the share date immediately,
-	 * before just marking the other users when the relevant shares are unknown
+	 * The total amount of time we've spent so far processing updates
 	 */
-	private int $cutOffMarkAllShares ;
-
-	private int $updatedCount = 0;
+	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,16 @@ public function handle(Event $event): void {
 			$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) {
+					$start = floatval($this->clock->now()->format('U.u'));
+					if ($this->cutOffMarkTime === -1.0 || $this->updatedTime < $this->cutOffMarkTime) {
 						$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);
 					}
+					$end = floatval($this->clock->now()->format('U.u'));
+					$this->updatedTime += $end - $start;
 				}
 			}
 		}
@@ -86,23 +87,21 @@ public function handle(Event $event): void {
 	}
 
 	private function updateOrMarkUser(IUser $user, bool $verifyMountPoints, array $ignoreShares = []): void {
-		$this->updatedCount++;
-		if ($this->cutOffMarkAllShares === -1 || $this->updatedCount <= $this->cutOffMarkAllShares) {
+		$start = $this->clock->now();
+		if ($this->cutOffMarkTime === -1.0 || $this->updatedTime < $this->cutOffMarkTime) {
 			$this->shareUpdater->updateForUser($user, $verifyMountPoints, $ignoreShares);
 		} else {
 			$this->markUserForRefresh($user);
 		}
+		$end = $this->clock->now();
+		$this->updatedTime += $start->diff($end)->f;
 	}
 
 	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/tests/SharesUpdatedListenerTest.php

@@ -18,7 +18,9 @@
 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,21 +33,32 @@ 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();
 
 		$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,
 		);
 	}
@@ -133,4 +146,42 @@ public function testShareDeleted() {
 
 		$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('updateForShare');
+
+		$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 @@ public static function setUpBeforeClass(): void {
 		$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

@@ -6,6 +6,7 @@
  */
 use Behat\Behat\Context\Context;
 use Behat\Behat\Context\SnippetAcceptingContext;
+use OCA\Files_Sharing\Config\ConfigLexicon;
 
 require __DIR__ . '/autoload.php';
 
@@ -32,8 +33,7 @@ protected function resetAppConfigs() {
 		$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', ConfigLexicon::UPDATE_CUTOFF_TIME);
 
 		$this->runOcc(['config:system:delete', 'share_folder']);
 	}

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

@@ -49,8 +49,7 @@ Feature: sharing
 
 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"