From d997b1f7b0619e263616d1d34747e2e6cc24c8d2 Mon Sep 17 00:00:00 2001 From: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> Date: Mon, 27 Oct 2025 17:57:59 +0100 Subject: [PATCH] fix(pagination): render multistatus to XML before caching Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> --- apps/dav/lib/Paginate/PaginatePlugin.php | 65 +++- .../unit/Paginate/PaginatePluginTest.php | 337 ++++++++++++++++++ 2 files changed, 396 insertions(+), 6 deletions(-) create mode 100644 apps/dav/tests/unit/Paginate/PaginatePluginTest.php diff --git a/apps/dav/lib/Paginate/PaginatePlugin.php b/apps/dav/lib/Paginate/PaginatePlugin.php index c02eb9f21eb22..5da3acf191a7f 100644 --- a/apps/dav/lib/Paginate/PaginatePlugin.php +++ b/apps/dav/lib/Paginate/PaginatePlugin.php @@ -11,6 +11,7 @@ use Sabre\DAV\Server; use Sabre\DAV\ServerPlugin; +use Sabre\DAV\Xml\Element\Response; use Sabre\HTTP\RequestInterface; use Sabre\HTTP\ResponseInterface; @@ -54,8 +55,13 @@ public function onMultiStatus(&$fileProperties): void { ) { $pageSize = (int)$request->getHeader(self::PAGINATE_COUNT_HEADER) ?: $this->pageSize; $offset = (int)$request->getHeader(self::PAGINATE_OFFSET_HEADER); + $copyIterator = new LimitedCopyIterator($fileProperties, $pageSize, $offset); - ['token' => $token, 'count' => $count] = $this->cache->store($url, $copyIterator); + // wrap the iterator with another that renders XML, this way we + // 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); $fileProperties = $copyIterator->getRequestedItems(); $this->server->httpResponse->addHeader(self::PAGINATE_HEADER, 'true'); @@ -65,6 +71,44 @@ public function onMultiStatus(&$fileProperties): void { } } + /** + * Returns a generator that yields rendered XML entries for the provided + * $fileProperties, as they would appear in the MultiStatus response. + */ + private function getXmlRendererGenerator(iterable $fileProperties): \Generator { + $writer = $this->server->xml->getWriter(); + $prefer = $this->server->getHTTPPrefer(); + $minimal = $prefer['return'] === 'minimal'; + $writer->contextUri = $this->server->getBaseUri(); + + $writer->openMemory(); + $writer->startDocument(); + $writer->startElement('{DAV:}multistatus'); + + // throw away the beginning of the document + $writer->flush(); + + foreach ($fileProperties as $entry) { + $href = $entry['href']; + unset($entry['href']); + if ($minimal) { + unset($entry[404]); + } + $response = new Response( + ltrim($href, '/'), + $entry + ); + $writer->write([ + 'name' => '{DAV:}response', + 'value' => $response, + ]); + + // flushing does not remove the > for the previous element + // (multistatus) + yield ltrim($writer->flush(), '>'); + } + } + public function onMethod(RequestInterface $request, ResponseInterface $response) { $url = $this->server->httpRequest->getUrl(); if ( @@ -83,11 +127,20 @@ public function onMethod(RequestInterface $request, ResponseInterface $response) $response->setHeader('Content-Type', 'application/xml; charset=utf-8'); $response->setHeader('Vary', 'Brief,Prefer'); - $prefer = $this->server->getHTTPPrefer(); - $minimal = $prefer['return'] === 'minimal'; - - $data = $this->server->generateMultiStatus($items, $minimal); - $response->setBody($data); + // as we cached strings of XML, rebuild the multistatus response + // and output the RAW entries, as stored in the cache + $writer = $this->server->xml->getWriter(); + $writer->contextUri = $this->server->getBaseUri(); + $writer->openMemory(); + $writer->startDocument(); + $writer->startElement('{DAV:}multistatus'); + foreach ($items as $item) { + $writer->writeRaw($item); + } + $writer->endElement(); + $writer->endDocument(); + + $response->setBody($writer->flush()); return false; } diff --git a/apps/dav/tests/unit/Paginate/PaginatePluginTest.php b/apps/dav/tests/unit/Paginate/PaginatePluginTest.php new file mode 100644 index 0000000000000..2f93b27044d61 --- /dev/null +++ b/apps/dav/tests/unit/Paginate/PaginatePluginTest.php @@ -0,0 +1,337 @@ +initializePlugin(); + + $fileProperties = [ + [ + 'href' => '/file1', + 200 => [ + '{DAV:}displayname' => 'File 1', + '{DAV:}resourcetype' => null + ], + ], + [ + 'href' => '/file2', + 200 => [ + '{DAV:}displayname' => 'File 2', + '{DAV:}resourcetype' => null + ], + ], + [ + 'href' => '/file3', + 200 => [ + '{DAV:}displayname' => 'File 3', + '{DAV:}resourcetype' => null + ], + ], + ]; + + $this->request->expects(self::exactly(2)) + ->method('hasHeader') + ->willReturnMap([ + [PaginatePlugin::PAGINATE_HEADER, true], + [PaginatePlugin::PAGINATE_TOKEN_HEADER, false], + ]); + + $this->request->expects(self::once()) + ->method('getUrl') + ->willReturn('url'); + + $this->request->expects(self::exactly(2)) + ->method('getHeader') + ->willReturnMap([ + [PaginatePlugin::PAGINATE_COUNT_HEADER, 2], + [PaginatePlugin::PAGINATE_OFFSET_HEADER, 0], + ]); + + $this->request->expects(self::once()) + ->method('setHeader') + ->with(PaginatePlugin::PAGINATE_TOKEN_HEADER, 'token'); + + $this->cache->expects(self::once()) + ->method('store') + ->with( + 'url', + $this->callback(function ($generator) { + self::assertInstanceOf(\Generator::class, $generator); + $items = iterator_to_array($generator); + self::assertCount(3, $items); + self::assertStringContainsString($this->getResponseXmlForFile('/dav/file1', 'File 1'), $items[0]); + self::assertStringContainsString($this->getResponseXmlForFile('/dav/file2', 'File 2'), $items[1]); + self::assertStringContainsString($this->getResponseXmlForFile('/dav/file3', 'File 3'), $items[2]); + return true; + }), + ) + ->willReturn([ + 'token' => 'token', + 'count' => 3, + ]); + + $this->expectSequentialCalls( + $this->response, + 'addHeader', + [ + [PaginatePlugin::PAGINATE_HEADER, 'true'], + [PaginatePlugin::PAGINATE_TOKEN_HEADER, 'token'], + [PaginatePlugin::PAGINATE_TOTAL_HEADER, '3'], + ], + ); + + $this->plugin->onMultiStatus($fileProperties); + + self::assertInstanceOf(\Iterator::class, $fileProperties); + // the iterator should be replaced with one that has the amount of + // items for the page + $items = iterator_to_array($fileProperties, false); + $this->assertCount(2, $items); + } + + private function initializePlugin(): void { + $this->expectSequentialCalls( + $this->server, + 'on', + [ + ['beforeMultiStatus', [$this->plugin, 'onMultiStatus'], 100], + ['method:SEARCH', [$this->plugin, 'onMethod'], 1], + ['method:PROPFIND', [$this->plugin, 'onMethod'], 1], + ['method:REPORT', [$this->plugin, 'onMethod'], 1], + ], + ); + + $this->plugin->initialize($this->server); + } + + /** + * @param array> $expectedCalls + */ + private function expectSequentialCalls(MockObject $mock, string $method, array $expectedCalls): void { + $mock->expects(self::exactly(\count($expectedCalls))) + ->method($method) + ->willReturnCallback(function (...$args) use (&$expectedCalls) { + $expected = array_shift($expectedCalls); + self::assertNotNull($expected); + self::assertSame($expected, $args); + }); + } + + private function getResponseXmlForFile(string $fileName, string $displayName): string { + return preg_replace('/>\s+<', << + $fileName + + + $displayName + + + HTTP/1.1 200 OK + + + XML + ); + } + + public function testOnMultiStatusSkipsWhenHeadersAndCacheExist(): void { + $this->initializePlugin(); + + $fileProperties = [ + [ + 'href' => '/file1', + ], + [ + 'href' => '/file2', + ], + ]; + + $this->request->expects(self::exactly(2)) + ->method('hasHeader') + ->willReturnMap([ + [PaginatePlugin::PAGINATE_HEADER, true], + [PaginatePlugin::PAGINATE_TOKEN_HEADER, true], + ]); + + $this->request->expects(self::once()) + ->method('getUrl') + ->willReturn(''); + + $this->request->expects(self::once()) + ->method('getHeader') + ->with(PaginatePlugin::PAGINATE_TOKEN_HEADER) + ->willReturn('token'); + + $this->cache->expects(self::once()) + ->method('exists') + ->with('', 'token') + ->willReturn(true); + + $this->cache->expects(self::never()) + ->method('store'); + + $this->plugin->onMultiStatus($fileProperties); + + self::assertInstanceOf(\Iterator::class, $fileProperties); + self::assertSame( + [ + ['href' => '/file1'], + ['href' => '/file2'], + ], + iterator_to_array($fileProperties) + ); + } + + public function testOnMethodReturnsCachedResponse(): void { + $this->initializePlugin(); + + $response = $this->createMock(ResponseInterface::class); + + $this->request->expects(self::exactly(2)) + ->method('hasHeader') + ->willReturnMap([ + [PaginatePlugin::PAGINATE_TOKEN_HEADER, true], + [PaginatePlugin::PAGINATE_OFFSET_HEADER, true], + ]); + + $this->request->expects(self::once()) + ->method('getUrl') + ->willReturn('url'); + + $this->request->expects(self::exactly(4)) + ->method('getHeader') + ->willReturnMap([ + [PaginatePlugin::PAGINATE_TOKEN_HEADER, 'token'], + [PaginatePlugin::PAGINATE_OFFSET_HEADER, '2'], + [PaginatePlugin::PAGINATE_COUNT_HEADER, '4'], + ]); + + $this->cache->expects(self::once()) + ->method('exists') + ->with('url', 'token') + ->willReturn(true); + + $this->cache->expects(self::once()) + ->method('get') + ->with('url', 'token', 2, 4) + ->willReturn((function (): \Generator { + yield $this->getResponseXmlForFile('/file1', 'File 1'); + yield $this->getResponseXmlForFile('/file2', 'File 2'); + })()); + + $response->expects(self::once()) + ->method('setStatus') + ->with(207); + + $response->expects(self::once()) + ->method('addHeader') + ->with(PaginatePlugin::PAGINATE_HEADER, 'true'); + + $this->expectSequentialCalls( + $response, + 'setHeader', + [ + ['Content-Type', 'application/xml; charset=utf-8'], + ['Vary', 'Brief,Prefer'], + ], + ); + + $response->expects(self::once()) + ->method('setBody') + ->with($this->callback(function (string $body) { + // header of the XML + self::assertStringContainsString(<< + + XML, + $body); + self::assertStringContainsString($this->getResponseXmlForFile('/file1', 'File 1'), $body); + self::assertStringContainsString($this->getResponseXmlForFile('/file2', 'File 2'), $body); + // footer of the XML + self::assertStringContainsString('', $body); + + return true; + })); + + self::assertFalse($this->plugin->onMethod($this->request, $response)); + } + + public function testOnMultiStatusNoPaginateHeaderShouldSucceed(): void { + $this->initializePlugin(); + + $this->request->expects(self::once()) + ->method('getUrl') + ->willReturn(''); + + $this->cache->expects(self::never()) + ->method('exists'); + $this->cache->expects(self::never()) + ->method('store'); + + $this->plugin->onMultiStatus($this->request); + } + + public function testOnMethodNoTokenHeaderShouldSucceed(): void { + $this->initializePlugin(); + $this->request->expects(self::once()) + ->method('hasHeader') + ->with(PaginatePlugin::PAGINATE_TOKEN_HEADER) + ->willReturn(false); + + $this->cache->expects(self::never()) + ->method('exists'); + $this->cache->expects(self::never()) + ->method('get'); + + $this->plugin->onMethod($this->request, $this->response); + } + + protected function setUp(): void { + parent::setUp(); + + $this->cache = $this->createMock(PaginateCache::class); + + $this->server = $this->getMockBuilder(Server::class) + ->disableOriginalConstructor() + ->onlyMethods(['on', 'getHTTPPrefer', 'getBaseUri']) + ->getMock(); + + $this->request = $this->createMock(RequestInterface::class); + $this->response = $this->createMock(ResponseInterface::class); + + $this->server->httpRequest = $this->request; + $this->server->httpResponse = $this->response; + $this->server->xml = new Service(); + $this->server->xml->namespaceMap = [ 'DAV:' => 'd' ]; + + $this->server->method('getHTTPPrefer') + ->willReturn(['return' => null]); + $this->server->method('getBaseUri') + ->willReturn('/dav/'); + + $this->plugin = new PaginatePlugin($this->cache, 2); + } +}