From 9c399ba4ac1d94098bc4437705fcc1734c8e2f68 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 20 Feb 2026 16:19:28 +0100 Subject: [PATCH] feat: use time-based cutoff for share updating instead of count Signed-off-by: Robin Appelman --- .../lib/Config/ConfigLexicon.php | 6 +- .../lib/Listener/SharesUpdatedListener.php | 58 +++++++++--------- .../lib/ShareRecipientUpdater.php | 2 +- .../tests/ShareRecipientUpdaterTest.php | 2 +- .../tests/SharesUpdatedListenerTest.php | 59 +++++++++++++++++-- apps/files_sharing/tests/TestCase.php | 3 +- .../features/bootstrap/SharingContext.php | 3 +- .../sharing_features/sharing-v1-part2.feature | 3 +- 8 files changed, 91 insertions(+), 45 deletions(-) diff --git a/apps/files_sharing/lib/Config/ConfigLexicon.php b/apps/files_sharing/lib/Config/ConfigLexicon.php index a6fb9f11ae6..1ec2277f9fc 100644 --- a/apps/files_sharing/lib/Config/ConfigLexicon.php +++ b/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'), ]; } diff --git a/apps/files_sharing/lib/Listener/SharesUpdatedListener.php b/apps/files_sharing/lib/Listener/SharesUpdatedListener.php index 39f9e4e9289..880fd9792b8 100644 --- a/apps/files_sharing/lib/Listener/SharesUpdatedListener.php +++ b/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; } } diff --git a/apps/files_sharing/lib/ShareRecipientUpdater.php b/apps/files_sharing/lib/ShareRecipientUpdater.php index cd4f3890721..996c051749a 100644 --- a/apps/files_sharing/lib/ShareRecipientUpdater.php +++ b/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); diff --git a/apps/files_sharing/tests/ShareRecipientUpdaterTest.php b/apps/files_sharing/tests/ShareRecipientUpdaterTest.php index e32f7d88fb4..5ad995c3b1c 100644 --- a/apps/files_sharing/tests/ShareRecipientUpdaterTest.php +++ b/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); } /** diff --git a/apps/files_sharing/tests/SharesUpdatedListenerTest.php b/apps/files_sharing/tests/SharesUpdatedListenerTest.php index 3e3211dae36..ccba9d41c2e 100644 --- a/apps/files_sharing/tests/SharesUpdatedListenerTest.php +++ b/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)); + } } diff --git a/apps/files_sharing/tests/TestCase.php b/apps/files_sharing/tests/TestCase.php index 9ed3bacc391..02ee66d0961 100644 --- a/apps/files_sharing/tests/TestCase.php +++ b/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 { diff --git a/build/integration/features/bootstrap/SharingContext.php b/build/integration/features/bootstrap/SharingContext.php index 017c1486fcd..3d13c0ae3f5 100644 --- a/build/integration/features/bootstrap/SharingContext.php +++ b/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']); } diff --git a/build/integration/sharing_features/sharing-v1-part2.feature b/build/integration/sharing_features/sharing-v1-part2.feature index ba5cd18f655..02bb84750f2 100644 --- a/build/integration/sharing_features/sharing-v1-part2.feature +++ b/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"