fix(pagination): render multistatus to XML before caching

Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com>
7 months ago · 7ba0cb4e2c
parent 85e1e06a6d
commit 7ba0cb4e2c
2 changed files with 396 additions and 6 deletions
--- a/apps/dav/lib/Paginate/PaginatePlugin.php
+++ b/apps/dav/lib/Paginate/PaginatePlugin.php
@ -11,6 +11,7 @@ namespace OCA\DAV\Paginate;

 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 @@ class PaginatePlugin extends ServerPlugin {
 		) {
 			$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 @@ class PaginatePlugin extends ServerPlugin {
 		}
 	}

+	/**
+	 * 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 @@ class PaginatePlugin extends ServerPlugin {
 			$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;
 		}
--- a/apps/dav/tests/unit/Paginate/PaginatePluginTest.php
+++ b/apps/dav/tests/unit/Paginate/PaginatePluginTest.php
@ -0,0 +1,337 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OCA\DAV\Tests\unit\Paginate;
+
+use OCA\DAV\Paginate\PaginateCache;
+use OCA\DAV\Paginate\PaginatePlugin;
+use PHPUnit\Framework\MockObject\MockObject;
+use Sabre\DAV\Server;
+use Sabre\HTTP\RequestInterface;
+use Sabre\HTTP\ResponseInterface;
+use Sabre\Xml\Service;
+use Test\TestCase;
+
+class PaginatePluginTest extends TestCase {
+
+	private PaginateCache&MockObject $cache;
+	private PaginatePlugin $plugin;
+	private Server&MockObject $server;
+	private RequestInterface&MockObject $request;
+	private ResponseInterface&MockObject $response;
+
+	public function testOnMultiStatusCachesAndUpdatesResponse(): void {
+		$this->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<int, array<int, mixed>> $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+</', '><', <<<XML
+			<d:response>
+				<d:href>$fileName</d:href>
+				<d:propstat>
+					<d:prop>
+						<d:displayname>$displayName</d:displayname>
+						<d:resourcetype/>
+					</d:prop>
+					<d:status>HTTP/1.1 200 OK</d:status>
+				</d:propstat>
+			</d:response>
+			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
+					<?xml version="1.0"?>
+					<d:multistatus xmlns:d="DAV:">
+					XML,
+					$body);
+				self::assertStringContainsString($this->getResponseXmlForFile('/file1', 'File 1'), $body);
+				self::assertStringContainsString($this->getResponseXmlForFile('/file2', 'File 2'), $body);
+				// footer of the XML
+				self::assertStringContainsString('</d:multistatus>', $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);
+	}
+}