Merge pull request #26507 from owncloud/filelist-pagination-webdav-report

Introduce pagination in files-filter report

Merge pull request #26507 from owncloud/filelist-pagination-webdav-report
Introduce pagination in files-filter report
b31294ae · Vincent Petry · GitHub · 659b5849 · fad6a050 · b31294ae
Commit b31294ae authored Mar 22, 2017 by Vincent Petry Committed by GitHub Mar 22, 2017
5 changed files
--- a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php
+++ b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php
@@ -24,6 +24,7 @@
 namespace OCA\DAV\Connector\Sabre;

 use OC\Files\View;
+use OCA\DAV\Files\Xml\FilterRequest;
 use Sabre\DAV\Exception\PreconditionFailed;
 use Sabre\DAV\Exception\BadRequest;
 use Sabre\DAV\ServerPlugin;
@@ -139,6 +140,8 @@ class FilesReportPlugin extends ServerPlugin {

 		$server->xml->namespaceMap[self::NS_OWNCLOUD] = 'oc';

+		$server->xml->elementMap[self::REPORT_NAME] = FilterRequest::class;
+
 		$this->server = $server;
 		$this->server->on('report', [$this, 'onReport']);
 	}
@@ -159,7 +162,7 @@ class FilesReportPlugin extends ServerPlugin {
 	 * REPORT operations to look for files
 	 *
 	 * @param string $reportName
-	 * @param $report
+	 * @param mixed $report
 	 * @param string $uri
 	 * @return bool
 	 * @throws BadRequest
@@ -167,50 +170,44 @@ class FilesReportPlugin extends ServerPlugin {
 	 * @internal param $ [] $report
 	 */
 	public function onReport($reportName, $report, $uri) {
+
 		$reportTargetNode = $this->server->tree->getNodeForPath($uri);
 		if (!$reportTargetNode instanceof Directory || $reportName !== self::REPORT_NAME) {
 			return;
 		}

-		$ns = '{' . $this::NS_OWNCLOUD . '}';
-		$requestedProps = [];
-		$filterRules = [];
-
-		// parse report properties and gather filter info
-		foreach ($report as $reportProps) {
-			$name = $reportProps['name'];
-			if ($name === $ns . 'filter-rules') {
-				$filterRules = $reportProps['value'];
-			} else if ($name === '{DAV:}prop') {
-				// propfind properties
-				foreach ($reportProps['value'] as $propVal) {
-					$requestedProps[] = $propVal['name'];
-				}
+		$requestedProps = $report->properties;
+		$filterRules = $report->filters;
+
+		// "systemtag" is always an array of tags, favorite a string/int/null
+		if (empty($filterRules['systemtag']) && is_null($filterRules['favorite'])) {
+			// FIXME: search currently not possible because results are missing properties!
+			throw new BadRequest('No filter criteria specified');
+		} else {
+			if (isset($report->search['pattern'])) {
+				// TODO: implement this at some point...
+				throw new BadRequest('Search pattern cannot be combined with filter');
 			}
-		}

-		if (empty($filterRules)) {
-			// an empty filter would return all existing files which would be slow
-			throw new BadRequest('Missing filter-rule block in request');
-		}
+			// gather all file ids matching filter
+			try {
+				$resultFileIds = $this->processFilterRules($filterRules);
+			} catch (TagNotFoundException $e) {
+				throw new PreconditionFailed('Cannot filter by non-existing tag', 0, $e);
+			}

-		// gather all file ids matching filter
-		try {
-			$resultFileIds = $this->processFilterRules($filterRules);
-		} catch (TagNotFoundException $e) {
-			throw new PreconditionFailed('Cannot filter by non-existing tag', 0, $e);
-		}
+			// pre-slice the results if needed for pagination to not waste
+			// time resolving nodes that will not be returned anyway
+			$resultFileIds = $this->slice($resultFileIds, $report);

-		// find sabre nodes by file id, restricted to the root node path
-		$results = $this->findNodesByFileIds($reportTargetNode, $resultFileIds);
+			// find sabre nodes by file id, restricted to the root node path
+			$results = $this->findNodesByFileIds($reportTargetNode, $resultFileIds);
+		}

 		$filesUri = $this->getFilesBaseUri($uri, $reportTargetNode->getPath());
-		$responses = $this->prepareResponses($filesUri, $requestedProps, $results);
+		$results = $this->prepareResponses($filesUri, $requestedProps, $results);

-		$xml = $this->server->xml->write(
-			'{DAV:}multistatus',
-			new MultiStatus($responses)
-		);
+		$xml = $this->server->generateMultiStatus($results);

 		$this->server->httpResponse->setStatus(207);
 		$this->server->httpResponse->setHeader('Content-Type', 'application/xml; charset=utf-8');
@@ -219,6 +216,15 @@ class FilesReportPlugin extends ServerPlugin {
 		return false;
 	}

+	private function slice($results, $report) {
+		if (!is_null($report->search)) {
+			$length = $report->search['limit'];
+			$offset = $report->search['offset'];
+			$results = array_slice($results, $offset, $length);
+		}
+		return $results;
+	}
+
 	/**
 	 * Returns the base uri of the files root by removing
 	 * the subpath from the URI
@@ -252,18 +258,9 @@ class FilesReportPlugin extends ServerPlugin {
 	 * @throws TagNotFoundException whenever a tag was not found
 	 */
 	protected function processFilterRules($filterRules) {
-		$ns = '{' . $this::NS_OWNCLOUD . '}';
 		$resultFileIds = null;
-		$systemTagIds = [];
-		$favoriteFilter = null;
-		foreach ($filterRules as $filterRule) {
-			if ($filterRule['name'] === $ns . 'systemtag') {
-				$systemTagIds[] = $filterRule['value'];
-			}
-			if ($filterRule['name'] === $ns . 'favorite') {
-				$favoriteFilter = true;
-			}
-		}
+		$systemTagIds = $filterRules['systemtag'];
+		$favoriteFilter = $filterRules['favorite'];

 		if ($favoriteFilter !== null) {
 			$resultFileIds = $this->fileTagger->load('files')->getFavorites();
@@ -337,7 +334,7 @@ class FilesReportPlugin extends ServerPlugin {
 	 * @return Response[]
 	 */
 	public function prepareResponses($filesUri, $requestedProps, $nodes) {
-		$responses = [];
+		$results = [];
 		foreach ($nodes as $node) {
 			$propFind = new PropFind($filesUri . $node->getPath(), $requestedProps);

@@ -346,18 +343,9 @@ class FilesReportPlugin extends ServerPlugin {
 			$result = $propFind->getResultForMultiStatus();
 			$result['href'] = $propFind->getPath();

-			$resourceType = $this->server->getResourceTypeForNode($node);
-			if (in_array('{DAV:}collection', $resourceType) || in_array('{DAV:}principal', $resourceType)) {
-				$result['href'] .= '/';
-			}
-
-			$responses[] = new Response(
-				rtrim($this->server->getBaseUri(), '/') . $filesUri . $node->getPath(),
-				$result,
-				200
-			);
+			$results[] = $result;
 		}
-		return $responses;
+		return $results;
 	}

 	/**
@@ -378,10 +366,9 @@ class FilesReportPlugin extends ServerPlugin {
 			$entry = $folder->getById($fileId);
 			if ($entry) {
 				$entry = current($entry);
-				if ($entry instanceof \OCP\Files\File) {
-					$results[] = new File($this->fileView, $entry);
-				} else if ($entry instanceof \OCP\Files\Folder) {
-					$results[] = new Directory($this->fileView, $entry);
+				$node = $this->makeSabreNode($entry);
+				if ($node) {
+					$results[] = $node;
 				}
 			}
 		}
@@ -389,6 +376,15 @@ class FilesReportPlugin extends ServerPlugin {
 		return $results;
 	}

+	private function makeSabreNode(\OCP\Files\Node $filesNode) {
+		if ($filesNode instanceof \OCP\Files\File) {
+			return new File($this->fileView, $filesNode);
+		} else if ($filesNode instanceof \OCP\Files\Folder) {
+			return new Directory($this->fileView, $filesNode);
+		}
+		throw new \Exception('Unrecognized Files API node returned, aborting');
+	}
+
 	/**
 	 * Returns whether the currently logged in user is an administrator
 	 */

--- a/apps/dav/lib/Files/Xml/FilterRequest.php
+++ b/apps/dav/lib/Files/Xml/FilterRequest.php
+<?php
+
+namespace OCA\DAV\Files\Xml;
+
+use Sabre\Xml\Element\Base;
+use Sabre\Xml\Element\KeyValue;
+use Sabre\Xml\Reader;
+use Sabre\Xml\XmlDeserializable;
+
+class FilterRequest implements XmlDeserializable {
+
+	/**
+	 * An array with requested properties.
+	 *
+	 * @var array
+	 */
+	public $properties;
+
+	/**
+	 * @var array
+	 */
+	public $filters;
+
+	/**
+	 * @var array
+	 */
+	public $search;
+
+	/**
+	 * The deserialize method is called during xml parsing.
+	 *
+	 * This method is called statically, this is because in theory this method
+	 * may be used as a type of constructor, or factory method.
+	 *
+	 * Often you want to return an instance of the current class, but you are
+	 * free to return other data as well.
+	 *
+	 * You are responsible for advancing the reader to the next element. Not
+	 * doing anything will result in a never-ending loop.
+	 *
+	 * If you just want to skip parsing for this element altogether, you can
+	 * just call $reader->next();
+	 *
+	 * $reader->parseInnerTree() will parse the entire sub-tree, and advance to
+	 * the next element.
+	 *
+	 * @param Reader $reader
+	 * @return mixed
+	 */
+	static function xmlDeserialize(Reader $reader) {
+		$elems = (array)$reader->parseInnerTree([
+			'{DAV:}prop' => KeyValue::class,
+			'{http://owncloud.org/ns}filter-rules' => Base::class,
+			'{http://owncloud.org/ns}search' => KeyValue::class,
+		]);
+
+		$newProps = [
+			'filters'    => [
+				'systemtag' => [],
+				'favorite' => null
+			],
+			'properties' => [],
+			'search' => null,
+		];
+
+		if (!is_array($elems)) {
+			$elems = [];
+		}
+
+		foreach ($elems as $elem) {
+
+			switch ($elem['name']) {
+
+				case '{DAV:}prop' :
+					$newProps['properties'] = array_keys($elem['value']);
+					break;
+				case '{http://owncloud.org/ns}filter-rules' :
+
+					foreach ($elem['value'] as $tag) {
+						if ($tag['name'] === '{http://owncloud.org/ns}systemtag') {
+							$newProps['filters']['systemtag'][] = $tag['value'];
+						}
+						if ($tag['name'] === '{http://owncloud.org/ns}favorite') {
+							$newProps['filters']['favorite'] = true;
+						}
+					}
+					break;
+				case '{http://owncloud.org/ns}search' :
+					$value = $elem['value'];
+					if (isset($value['{http://owncloud.org/ns}pattern'])) {
+						$newProps['search']['pattern'] = $value['{http://owncloud.org/ns}pattern'];
+					}
+					if (isset($value['{http://owncloud.org/ns}limit'])) {
+						$newProps['search']['limit'] = (int)$value['{http://owncloud.org/ns}limit'];
+					}
+					if (isset($value['{http://owncloud.org/ns}offset'])) {
+						$newProps['search']['offset'] = (int)$value['{http://owncloud.org/ns}offset'];
+					}
+					break;
+			}
+		}
+
+		$obj = new self();
+		foreach ($newProps as $key => $value) {
+			$obj->$key = $value;
+		}
+
+		return $obj;
+	}
+}
--- a/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php
+++ b/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php
@@ -25,6 +25,7 @@
 namespace OCA\DAV\Tests\unit\Connector\Sabre;

 use OCA\DAV\Connector\Sabre\FilesReportPlugin as FilesReportPluginImplementation;
+use OCA\DAV\Files\Xml\FilterRequest;
 use OCP\SystemTag\ISystemTagObjectMapper;
 use OC\Files\View;
 use OCP\Files\Folder;
@@ -32,6 +33,8 @@ use OCP\IGroupManager;
 use OCP\SystemTag\ISystemTagManager;
 use OCP\ITags;
 use OCP\Files\FileInfo;
+use OCP\IRequest;
+use OCP\IConfig;

 class FilesReportPluginTest extends \Test\TestCase {
 	/** @var \Sabre\DAV\Server|\PHPUnit_Framework_MockObject_MockObject */
@@ -70,13 +73,11 @@ class FilesReportPluginTest extends \Test\TestCase {
 			->disableOriginalConstructor()
 			->getMock();

-		$this->view = $this->getMockBuilder('\OC\Files\View')
-			->disableOriginalConstructor()
-			->getMock();
+		$this->view = new View();

 		$this->server = $this->getMockBuilder('\Sabre\DAV\Server')
 			->setConstructorArgs([$this->tree])
-			->setMethods(['getRequestUri', 'getBaseUri'])
+			->setMethods(['getRequestUri', 'getBaseUri', 'generateMultiStatus'])
 			->getMock();

 		$this->server->expects($this->any())
@@ -109,6 +110,15 @@ class FilesReportPluginTest extends \Test\TestCase {
 			->method('getUser')
 			->will($this->returnValue($user));

+		// add FilesPlugin to test more properties
+		$this->server->addPlugin(
+			new \OCA\DAV\Connector\Sabre\FilesPlugin(
+				$this->tree,
+				$this->createMock(IConfig::class),
+				$this->createMock(IRequest::class)
+			)
+		);
+
 		$this->plugin = new FilesReportPluginImplementation(
 			$this->tree,
 			$this->view,
@@ -156,21 +166,16 @@ class FilesReportPluginTest extends \Test\TestCase {
 	public function testOnReport() {
 		$path = 'test';

-		$parameters = [
-			[
-				'name'  => '{DAV:}prop',
-				'value' => [
-					['name' => '{DAV:}getcontentlength', 'value' => ''],
-					['name' => '{http://owncloud.org/ns}size', 'value' => ''],
-				],
-			],
-			[
-				'name'  => '{http://owncloud.org/ns}filter-rules',
-				'value' => [
-					['name' => '{http://owncloud.org/ns}systemtag', 'value' => '123'],
-					['name' => '{http://owncloud.org/ns}systemtag', 'value' => '456'],
-				],
-			],
+		$parameters = new FilterRequest();
+		$parameters->properties = [
+			'{DAV:}getcontentlength',
+			'{http://owncloud.org/ns}size',
+			'{http://owncloud.org/ns}fileid',
+			'{DAV:}resourcetype',
+		];
+		$parameters->filters = [
+			'systemtag' => [123, 456],
+			'favorite' => null
 		];

 		$this->groupManager->expects($this->any())
@@ -186,14 +191,8 @@ class FilesReportPluginTest extends \Test\TestCase {
 			->with('456', 'files')
 			->will($this->returnValue(['111', '222', '333']));

-		$reportTargetNode = $this->getMockBuilder('\OCA\DAV\Connector\Sabre\Directory')
-			->disableOriginalConstructor()
-			->getMock();
-
-		$response = $this->getMockBuilder('Sabre\HTTP\ResponseInterface')
-			->disableOriginalConstructor()
-			->getMock();
-
+		$reportTargetNode = $this->createMock(\OCA\DAV\Connector\Sabre\Directory::class);
+		$response = $this->createMock(\Sabre\HTTP\ResponseInterface::class);
 		$response->expects($this->once())
 			->method('setHeader')
 			->with('Content-Type', 'application/xml; charset=utf-8');
@@ -210,12 +209,16 @@ class FilesReportPluginTest extends \Test\TestCase {
 			->with('/' . $path)
 			->will($this->returnValue($reportTargetNode));

-		$filesNode1 = $this->getMockBuilder('\OCP\Files\Folder')
-			->disableOriginalConstructor()
-			->getMock();
-		$filesNode2 = $this->getMockBuilder('\OCP\Files\File')
-			->disableOriginalConstructor()
-			->getMock();
+		$filesNode1 = $this->createMock(\OCP\Files\Folder::class);
+		$filesNode1->method('getId')->willReturn(111);
+		$filesNode1->method('getPath')->willReturn('/node1');
+		$filesNode1->method('isReadable')->willReturn(true);
+		$filesNode1->method('getSize')->willReturn(2048);
+		$filesNode2 = $this->createMock(\OCP\Files\File::class);
+		$filesNode2->method('getId')->willReturn(222);
+		$filesNode2->method('getPath')->willReturn('/sub/node2');
+		$filesNode2->method('getSize')->willReturn(1024);
+		$filesNode2->method('isReadable')->willReturn(true);

 		$this->userFolder->expects($this->at(0))
 			->method('getById')
@@ -232,7 +235,110 @@ class FilesReportPluginTest extends \Test\TestCase {
 		$this->server->httpResponse = $response;
 		$this->plugin->initialize($this->server);

+		$responses = null;
+		$this->server->expects($this->once())
+			->method('generateMultiStatus')
+			->will($this->returnCallback(function($responsesArg) use (&$responses) {
+				$responses = $responsesArg;
+			})
+		);
+
 		$this->assertFalse($this->plugin->onReport(FilesReportPluginImplementation::REPORT_NAME, $parameters, '/' . $path));
+
+		$this->assertCount(2, $responses);
+
+		$this->assertTrue(isset($responses[0][200]));
+		$this->assertTrue(isset($responses[1][200]));
+
+		$this->assertEquals('/test/node1', $responses[0]['href']);
+		$this->assertEquals('/test/sub/node2', $responses[1]['href']);
+
+		$props1 = $responses[0];
+		$this->assertEquals('111', $props1[200]['{http://owncloud.org/ns}fileid']);
+		$this->assertNull($props1[404]['{DAV:}getcontentlength']);
+		$this->assertInstanceOf('\Sabre\DAV\Xml\Property\ResourceType', $props1[200]['{DAV:}resourcetype']);
+		$resourceType1 = $props1[200]['{DAV:}resourcetype']->getValue();
+		$this->assertEquals('{DAV:}collection', $resourceType1[0]);
+
+		$props2 = $responses[1];
+		$this->assertEquals('1024', $props2[200]['{DAV:}getcontentlength']);
+		$this->assertEquals('222', $props2[200]['{http://owncloud.org/ns}fileid']);
+		$this->assertInstanceOf('\Sabre\DAV\Xml\Property\ResourceType', $props2[200]['{DAV:}resourcetype']);
+		$this->assertCount(0, $props2[200]['{DAV:}resourcetype']->getValue());
+	}
+
+	public function testOnReportPaginationFiltered() {
+		$path = 'test';
+
+		$parameters = new FilterRequest();
+		$parameters->properties = [
+			'{DAV:}getcontentlength',
+		];
+		$parameters->filters = [
+			'systemtag' => [],
+			'favorite' => true
+		];
+		$parameters->search = [
+			'offset' => 2,
+			'limit' => 3,
+		];
+
+		$filesNodes = [];
+		for ($i = 0; $i < 20; $i++) {
+			$filesNode = $this->createMock(\OCP\Files\File::class);
+			$filesNode->method('getId')->willReturn(1000 + $i);
+			$filesNode->method('getPath')->willReturn('/nodes/node' . $i);
+			$filesNode->method('isReadable')->willReturn(true);
+			$filesNodes[$filesNode->getId()] = $filesNode;
+		}
+
+		// return all above nodes as favorites
+		$this->privateTags->expects($this->once())
+			->method('getFavorites')
+			->will($this->returnValue(array_keys($filesNodes)));
+
+		$reportTargetNode = $this->createMock(\OCA\DAV\Connector\Sabre\Directory::class);
+
+		$this->tree->expects($this->any())
+			->method('getNodeForPath')
+			->with('/' . $path)
+			->will($this->returnValue($reportTargetNode));
+
+		// getById must only be called for the required nodes
+		$this->userFolder->expects($this->at(0))
+			->method('getById')
+			->with(1002)
+			->willReturn([$filesNodes[1002]]);
+		$this->userFolder->expects($this->at(1))
+			->method('getById')
+			->with(1003)
+			->willReturn([$filesNodes[1003]]);
+		$this->userFolder->expects($this->at(2))
+			->method('getById')
+			->with(1004)
+			->willReturn([$filesNodes[1004]]);
+
+		$this->server->expects($this->any())
+			->method('getRequestUri')
+			->will($this->returnValue($path));
+
+		$this->plugin->initialize($this->server);
+
+		$responses = null;
+		$this->server->expects($this->once())
+			->method('generateMultiStatus')
+			->will($this->returnCallback(function($responsesArg) use (&$responses) {
+				$responses = $responsesArg;
+			})
+		);
+
+		$this->assertFalse($this->plugin->onReport(FilesReportPluginImplementation::REPORT_NAME, $parameters, '/' . $path));
+
+		$this->assertCount(3, $responses);
+
+		$this->assertEquals('/test/nodes/node2', $responses[0]['href']);
+		$this->assertEquals('/test/nodes/node3', $responses[1]['href']);
+		$this->assertEquals('/test/nodes/node4', $responses[2]['href']);
 	}

 	public function testFindNodesByFileIdsRoot() {
@@ -327,71 +433,6 @@ class FilesReportPluginTest extends \Test\TestCase {
 		$this->assertEquals('second node', $result[1]->getName());
 	}

-	public function testPrepareResponses() {
-		$requestedProps = ['{DAV:}getcontentlength', '{http://owncloud.org/ns}fileid', '{DAV:}resourcetype'];
-
-		$fileInfo = $this->createMock(FileInfo::class);
-		$fileInfo->method('isReadable')->willReturn(true);
-
-		$node1 = $this->getMockBuilder('\OCA\DAV\Connector\Sabre\Directory')
-			->disableOriginalConstructor()
-			->getMock();
-		$node2 = $this->getMockBuilder('\OCA\DAV\Connector\Sabre\File')
-			->disableOriginalConstructor()
-			->getMock();
-
-		$node1->expects($this->once())
-			->method('getInternalFileId')
-			->will($this->returnValue('111'));
-		$node1->expects($this->any())
-			->method('getPath')
-			->will($this->returnValue('/node1'));
-		$node1->method('getFileInfo')->willReturn($fileInfo);
-		$node2->expects($this->once())
-			->method('getInternalFileId')
-			->will($this->returnValue('222'));
-		$node2->expects($this->once())
-			->method('getSize')
-			->will($this->returnValue(1024));
-		$node2->expects($this->any())
-			->method('getPath')
-			->will($this->returnValue('/sub/node2'));
-		$node2->method('getFileInfo')->willReturn($fileInfo);
-
-		$config = $this->createMock('\OCP\IConfig');
-
-		$this->server->addPlugin(
-			new \OCA\DAV\Connector\Sabre\FilesPlugin(
-				$this->tree,
-				$config,
-				$this->createMock('\OCP\IRequest')
-			)
-		);
-		$this->plugin->initialize($this->server);
-		$responses = $this->plugin->prepareResponses('/files/username', $requestedProps, [$node1, $node2]);
-
-		$this->assertCount(2, $responses);
-
-		$this->assertEquals(200, $responses[0]->getHttpStatus());
-		$this->assertEquals(200, $responses[1]->getHttpStatus());
-
-		$this->assertEquals('http://example.com/owncloud/remote.php/dav/files/username/node1', $responses[0]->getHref());
-		$this->assertEquals('http://example.com/owncloud/remote.php/dav/files/username/sub/node2', $responses[1]->getHref());
-
-		$props1 = $responses[0]->getResponseProperties();
-		$this->assertEquals('111', $props1[200]['{http://owncloud.org/ns}fileid']);
-		$this->assertNull($props1[404]['{DAV:}getcontentlength']);
-		$this->assertInstanceOf('\Sabre\DAV\Xml\Property\ResourceType', $props1[200]['{DAV:}resourcetype']);
-		$resourceType1 = $props1[200]['{DAV:}resourcetype']->getValue();
-		$this->assertEquals('{DAV:}collection', $resourceType1[0]);
-