From 4681956e6213db4f607c3f6af5f6a42ab587238c Mon Sep 17 00:00:00 2001 From: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> Date: Mon, 26 Oct 2026 15:20:39 +0100 Subject: [PATCH 1/3] fix: add context to paginate cache Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> Assisted-by: ClaudeCode:claude-opus-5-5 --- apps/dav/lib/Paginate/PaginateCache.php | 30 +++++++- apps/dav/lib/Paginate/PaginatePlugin.php | 26 +++++-- .../tests/unit/Paginate/PaginateCacheTest.php | 73 +++++++++++++++++++ .../unit/Paginate/PaginatePluginTest.php | 41 +++++++++-- 4 files changed, 156 insertions(+), 14 deletions(-) create mode 100644 apps/dav/tests/unit/Paginate/PaginateCacheTest.php diff --git a/apps/dav/lib/Paginate/PaginateCache.php b/apps/dav/lib/Paginate/PaginateCache.php index 58219b03621e3..115ef7d98b016 100644 --- a/apps/dav/lib/Paginate/PaginateCache.php +++ b/apps/dav/lib/Paginate/PaginateCache.php @@ -15,6 +15,9 @@ use OCP\IDBConnection; use OCP\Security\ISecureRandom; +/** + * @psalm-type PaginateContext = array + */ class PaginateCache { public const TTL = 60 * 60; private const CACHE_COUNT_SUFFIX = 'count'; @@ -32,11 +35,13 @@ public function __construct( /** * @param string $uri * @param \Iterator $items + * @param PaginateContext $context * @return array{'token': string, 'count': int} */ - public function store(string $uri, \Iterator $items): array { + public function store(string $uri, \Iterator $items, array $context): array { $token = $this->random->generate(32); $cacheKey = $this->buildCacheKey($uri, $token); + $this->cache->set($cacheKey . self::CACHE_CONTEXT_SUFFIX, $context, self::TTL + 60); $count = 0; foreach ($items as $item) { @@ -50,10 +55,15 @@ public function store(string $uri, \Iterator $items): array { } /** + * @param PaginateContext $context * @return Generator */ - public function get(string $uri, string $token, int $offset, int $count): Generator { + public function get(string $uri, string $token, int $offset, int $count, array $context): Generator { $cacheKey = $this->buildCacheKey($uri, $token); + if (!$this->matchesContext($cacheKey, $context)) { + return []; + } + $nbItems = $this->cache->get($cacheKey . self::CACHE_COUNT_SUFFIX); if (!$nbItems || $offset > $nbItems) { return []; @@ -65,8 +75,20 @@ public function get(string $uri, string $token, int $offset, int $count): Genera } } - public function exists(string $uri, string $token): bool { - return $this->cache->get($this->buildCacheKey($uri, $token) . self::CACHE_COUNT_SUFFIX) > 0; + /** + * @param PaginateContext $context + */ + public function exists(string $uri, string $token, array $context): bool { + $cacheKey = $this->buildCacheKey($uri, $token); + return $this->matchesContext($cacheKey, $context) + && $this->cache->get($cacheKey . self::CACHE_COUNT_SUFFIX) > 0; + } + + /** + * @param PaginateContext $context + */ + private function matchesContext(string $cacheKey, array $context): bool { + return $this->cache->get($cacheKey . self::CACHE_CONTEXT_SUFFIX) === $context; } private function buildCacheKey(string $uri, string $token): string { diff --git a/apps/dav/lib/Paginate/PaginatePlugin.php b/apps/dav/lib/Paginate/PaginatePlugin.php index 740c14b64ffb1..c38c2f8ed06da 100644 --- a/apps/dav/lib/Paginate/PaginatePlugin.php +++ b/apps/dav/lib/Paginate/PaginatePlugin.php @@ -9,6 +9,7 @@ namespace OCA\DAV\Paginate; +use Sabre\DAV\Auth\Plugin as AuthPlugin; use Sabre\DAV\Server; use Sabre\DAV\ServerPlugin; use Sabre\DAV\Xml\Element\Response; @@ -51,7 +52,7 @@ public function onMultiStatus(&$fileProperties): void { $url = $request->getUrl(); if ( $request->hasHeader(self::PAGINATE_HEADER) - && (!$request->hasHeader(self::PAGINATE_TOKEN_HEADER) || !$this->cache->exists($url, $request->getHeader(self::PAGINATE_TOKEN_HEADER))) + && (!$request->hasHeader(self::PAGINATE_TOKEN_HEADER) || !$this->cache->exists($url, $request->getHeader(self::PAGINATE_TOKEN_HEADER), $this->getCacheContext())) ) { $pageSize = (int)$request->getHeader(self::PAGINATE_COUNT_HEADER) ?: $this->pageSize; $offset = (int)$request->getHeader(self::PAGINATE_OFFSET_HEADER); @@ -77,8 +78,7 @@ public function onMultiStatus(&$fileProperties): void { */ private function getXmlRendererGenerator(iterable $fileProperties): \Generator { $writer = $this->server->xml->getWriter(); - $prefer = $this->server->getHTTPPrefer(); - $minimal = $prefer['return'] === 'minimal'; + $minimal = $this->isMinimal(); $writer->contextUri = $this->server->getBaseUri(); $writer->openMemory(); @@ -109,18 +109,34 @@ private function getXmlRendererGenerator(iterable $fileProperties): \Generator { } } + private function isMinimal(): bool { + return $this->server->getHTTPPrefer()['return'] === 'minimal'; + } + + /** + * @return array{principal: ?string, minimal: bool} + */ + private function getCacheContext(): array { + $authPlugin = $this->server->getPlugin('auth'); + return [ + 'principal' => $authPlugin instanceof AuthPlugin ? $authPlugin->getCurrentPrincipal() : null, + 'minimal' => $this->isMinimal(), + ]; + } + public function onMethod(RequestInterface $request, ResponseInterface $response) { $url = $this->server->httpRequest->getUrl(); + $context = $this->getCacheContext(); if ( $request->hasHeader(self::PAGINATE_TOKEN_HEADER) && $request->hasHeader(self::PAGINATE_OFFSET_HEADER) - && $this->cache->exists($url, $request->getHeader(self::PAGINATE_TOKEN_HEADER)) + && $this->cache->exists($url, $request->getHeader(self::PAGINATE_TOKEN_HEADER), $context) ) { $token = $request->getHeader(self::PAGINATE_TOKEN_HEADER); $offset = (int)$request->getHeader(self::PAGINATE_OFFSET_HEADER); $count = (int)$request->getHeader(self::PAGINATE_COUNT_HEADER) ?: $this->pageSize; - $items = $this->cache->get($url, $token, $offset, $count); + $items = $this->cache->get($url, $token, $offset, $count, $context); $response->setStatus(207); $response->addHeader(self::PAGINATE_HEADER, 'true'); diff --git a/apps/dav/tests/unit/Paginate/PaginateCacheTest.php b/apps/dav/tests/unit/Paginate/PaginateCacheTest.php new file mode 100644 index 0000000000000..cf92dc49e9776 --- /dev/null +++ b/apps/dav/tests/unit/Paginate/PaginateCacheTest.php @@ -0,0 +1,73 @@ + 'principals/users/alice', 'minimal' => false]; + + private PaginateCache $paginateCache; + + protected function setUp(): void { + parent::setUp(); + + $random = $this->createMock(ISecureRandom::class); + $random->method('generate') + ->willReturn('token'); + + $cacheFactory = $this->createMock(ICacheFactory::class); + $cacheFactory->method('createDistributed') + ->willReturn(new ArrayCache('pagination_')); + + $this->paginateCache = new PaginateCache( + $this->createMock(IDBConnection::class), + $random, + $cacheFactory, + ); + } + + public function testStoreAndGetWithSameContext(): void { + $result = $this->paginateCache->store('url', new \ArrayIterator(['a', 'b', 'c']), self::CONTEXT); + + self::assertSame(['token' => 'token', 'count' => 3], $result); + self::assertTrue($this->paginateCache->exists('url', 'token', self::CONTEXT)); + self::assertSame(['b', 'c'], iterator_to_array($this->paginateCache->get('url', 'token', 1, 5, self::CONTEXT), false)); + } + + public static function dataDifferentContextDoesNotUseCache(): array { + return [ + 'other principal' => [['principal' => 'principals/users/bob', 'minimal' => false]], + 'no principal' => [['principal' => null, 'minimal' => false]], + 'other preference' => [['principal' => 'principals/users/alice', 'minimal' => true]], + 'empty context' => [[]], + ]; + } + + #[DataProvider(methodName: 'dataDifferentContextDoesNotUseCache')] + public function testDifferentContextDoesNotUseCache(array $context): void { + $this->paginateCache->store('url', new \ArrayIterator(['a', 'b']), self::CONTEXT); + + self::assertFalse($this->paginateCache->exists('url', 'token', $context)); + self::assertSame([], iterator_to_array($this->paginateCache->get('url', 'token', 0, 5, $context), false)); + } + + public function testDifferentUrlCannotAccess(): void { + $this->paginateCache->store('url', new \ArrayIterator(['a', 'b']), self::CONTEXT); + + self::assertFalse($this->paginateCache->exists('other-url', 'token', self::CONTEXT)); + } +} diff --git a/apps/dav/tests/unit/Paginate/PaginatePluginTest.php b/apps/dav/tests/unit/Paginate/PaginatePluginTest.php index 2f93b27044d61..ce4da35a557ad 100644 --- a/apps/dav/tests/unit/Paginate/PaginatePluginTest.php +++ b/apps/dav/tests/unit/Paginate/PaginatePluginTest.php @@ -12,6 +12,7 @@ use OCA\DAV\Paginate\PaginateCache; use OCA\DAV\Paginate\PaginatePlugin; use PHPUnit\Framework\MockObject\MockObject; +use Sabre\DAV\Auth\Plugin as AuthPlugin; use Sabre\DAV\Server; use Sabre\HTTP\RequestInterface; use Sabre\HTTP\ResponseInterface; @@ -19,12 +20,14 @@ use Test\TestCase; class PaginatePluginTest extends TestCase { + private const CONTEXT = ['principal' => 'principals/users/alice', 'minimal' => false]; private PaginateCache&MockObject $cache; private PaginatePlugin $plugin; private Server&MockObject $server; private RequestInterface&MockObject $request; private ResponseInterface&MockObject $response; + private ?string $preferReturn = null; public function testOnMultiStatusCachesAndUpdatesResponse(): void { $this->initializePlugin(); @@ -88,6 +91,7 @@ public function testOnMultiStatusCachesAndUpdatesResponse(): void { self::assertStringContainsString($this->getResponseXmlForFile('/dav/file3', 'File 3'), $items[2]); return true; }), + self::CONTEXT, ) ->willReturn([ 'token' => 'token', @@ -187,7 +191,7 @@ public function testOnMultiStatusSkipsWhenHeadersAndCacheExist(): void { $this->cache->expects(self::once()) ->method('exists') - ->with('', 'token') + ->with('', 'token', self::CONTEXT) ->willReturn(true); $this->cache->expects(self::never()) @@ -231,12 +235,12 @@ public function testOnMethodReturnsCachedResponse(): void { $this->cache->expects(self::once()) ->method('exists') - ->with('url', 'token') + ->with('url', 'token', self::CONTEXT) ->willReturn(true); $this->cache->expects(self::once()) ->method('get') - ->with('url', 'token', 2, 4) + ->with('url', 'token', 2, 4, self::CONTEXT) ->willReturn((function (): \Generator { yield $this->getResponseXmlForFile('/file1', 'File 1'); yield $this->getResponseXmlForFile('/file2', 'File 2'); @@ -279,6 +283,27 @@ public function testOnMethodReturnsCachedResponse(): void { self::assertFalse($this->plugin->onMethod($this->request, $response)); } + public function testOnMethodPassesMinimalPreferenceInContext(): void { + $this->initializePlugin(); + $this->preferReturn = 'minimal'; + + $this->request->method('hasHeader') + ->willReturn(true); + $this->request->method('getUrl') + ->willReturn('url'); + $this->request->method('getHeader') + ->willReturn('token'); + + $this->cache->expects(self::once()) + ->method('exists') + ->with('url', 'token', ['principal' => 'principals/users/alice', 'minimal' => true]) + ->willReturn(false); + $this->cache->expects(self::never()) + ->method('get'); + + $this->plugin->onMethod($this->request, $this->response); + } + public function testOnMultiStatusNoPaginateHeaderShouldSucceed(): void { $this->initializePlugin(); @@ -316,7 +341,7 @@ protected function setUp(): void { $this->server = $this->getMockBuilder(Server::class) ->disableOriginalConstructor() - ->onlyMethods(['on', 'getHTTPPrefer', 'getBaseUri']) + ->onlyMethods(['on', 'getHTTPPrefer', 'getBaseUri', 'getPlugin']) ->getMock(); $this->request = $this->createMock(RequestInterface::class); @@ -328,7 +353,13 @@ protected function setUp(): void { $this->server->xml->namespaceMap = [ 'DAV:' => 'd' ]; $this->server->method('getHTTPPrefer') - ->willReturn(['return' => null]); + ->willReturnCallback(fn () => ['return' => $this->preferReturn]); + + $authPlugin = $this->createMock(AuthPlugin::class); + $authPlugin->method('getCurrentPrincipal') + ->willReturn('principals/users/alice'); + $this->server->method('getPlugin') + ->willReturnMap([['auth', $authPlugin]]); $this->server->method('getBaseUri') ->willReturn('/dav/'); From 09b487d90e3728ffcf6175d3c87bd894b7482712 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Thu, 29 Oct 2026 17:23:36 +0100 Subject: [PATCH 2/3] refactor(dav): embed paginate context in the cache key Hash the context into the cache key instead of storing it as a separate entry. A request with a different context now misses the cache directly, which saves a cache read on every exists() and get() call. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ferdinand Thiessen --- apps/dav/lib/Paginate/PaginateCache.php | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/apps/dav/lib/Paginate/PaginateCache.php b/apps/dav/lib/Paginate/PaginateCache.php index 115ef7d98b016..2793cfe6c3b6e 100644 --- a/apps/dav/lib/Paginate/PaginateCache.php +++ b/apps/dav/lib/Paginate/PaginateCache.php @@ -40,8 +40,7 @@ public function __construct( */ public function store(string $uri, \Iterator $items, array $context): array { $token = $this->random->generate(32); - $cacheKey = $this->buildCacheKey($uri, $token); - $this->cache->set($cacheKey . self::CACHE_CONTEXT_SUFFIX, $context, self::TTL + 60); + $cacheKey = $this->buildCacheKey($uri, $token, $context); $count = 0; foreach ($items as $item) { @@ -59,11 +58,7 @@ public function store(string $uri, \Iterator $items, array $context): array { * @return Generator */ public function get(string $uri, string $token, int $offset, int $count, array $context): Generator { - $cacheKey = $this->buildCacheKey($uri, $token); - if (!$this->matchesContext($cacheKey, $context)) { - return []; - } - + $cacheKey = $this->buildCacheKey($uri, $token, $context); $nbItems = $this->cache->get($cacheKey . self::CACHE_COUNT_SUFFIX); if (!$nbItems || $offset > $nbItems) { return []; @@ -79,19 +74,13 @@ public function get(string $uri, string $token, int $offset, int $count, array $ * @param PaginateContext $context */ public function exists(string $uri, string $token, array $context): bool { - $cacheKey = $this->buildCacheKey($uri, $token); - return $this->matchesContext($cacheKey, $context) - && $this->cache->get($cacheKey . self::CACHE_COUNT_SUFFIX) > 0; + return $this->cache->get($this->buildCacheKey($uri, $token, $context) . self::CACHE_COUNT_SUFFIX) > 0; } /** * @param PaginateContext $context */ - private function matchesContext(string $cacheKey, array $context): bool { - return $this->cache->get($cacheKey . self::CACHE_CONTEXT_SUFFIX) === $context; - } - - private function buildCacheKey(string $uri, string $token): string { - return $token . '_' . crc32($uri) . '_'; + private function buildCacheKey(string $uri, string $token, array $context): string { + return $token . '_' . crc32($uri) . '_' . hash('sha256', json_encode($context, JSON_THROW_ON_ERROR)) . '_'; } } From 2329767c848f1c630413ab0124e0c678a9871f44 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Mon, 2 Nov 2026 18:26:40 +0100 Subject: [PATCH 3/3] fix(dav): pass paginate context when storing the cache Complete the backport: the store() call was missed due to the different code structure on this branch. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ferdinand Thiessen --- apps/dav/lib/Paginate/PaginatePlugin.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/dav/lib/Paginate/PaginatePlugin.php b/apps/dav/lib/Paginate/PaginatePlugin.php index c38c2f8ed06da..4de0ba5e0abcd 100644 --- a/apps/dav/lib/Paginate/PaginatePlugin.php +++ b/apps/dav/lib/Paginate/PaginatePlugin.php @@ -62,7 +62,7 @@ public function onMultiStatus(&$fileProperties): void { // cache XML, but we keep the first $pageSize elements as objects // to use for the response of the first page. $rendererGenerator = $this->getXmlRendererGenerator($copyIterator); - ['token' => $token, 'count' => $count] = $this->cache->store($url, $rendererGenerator); + ['token' => $token, 'count' => $count] = $this->cache->store($url, $rendererGenerator, $this->getCacheContext()); $fileProperties = $copyIterator->getRequestedItems(); $this->server->httpResponse->addHeader(self::PAGINATE_HEADER, 'true');