From ec144281efd2d5fec3d4af9fa50e0ecdfed97b26 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Wed, 24 Jan 2018 14:05:03 +0100 Subject: [PATCH 1/4] Respect limit and order in webdav search Signed-off-by: Roeland Jago Douma --- apps/dav/lib/Files/FileSearchBackend.php | 101 ++++++++++++++++++++++- 1 file changed, 98 insertions(+), 3 deletions(-) diff --git a/apps/dav/lib/Files/FileSearchBackend.php b/apps/dav/lib/Files/FileSearchBackend.php index 9616d21b88..2aa2ccba21 100644 --- a/apps/dav/lib/Files/FileSearchBackend.php +++ b/apps/dav/lib/Files/FileSearchBackend.php @@ -157,7 +157,8 @@ class FileSearchBackend implements ISearchBackend { /** @var Folder $folder $results */ $results = $folder->search($query); - return array_map(function (Node $node) { + /** @var SearchResult[] $nodes */ + $nodes = array_map(function (Node $node) { if ($node instanceof Folder) { $davNode = new \OCA\DAV\Connector\Sabre\Directory($this->view, $node, $this->tree, $this->shareManager); } else { @@ -167,6 +168,99 @@ class FileSearchBackend implements ISearchBackend { $this->tree->cacheNode($davNode, $path); return new SearchResult($davNode, $path); }, $results); + + // Sort again, since the result from multiple storages is appended and not sorted + usort($nodes, function(SearchResult $a, SearchResult $b) use ($search) { + return $this->sort($a, $b, $search->orderBy); + }); + + // If a limit is provided use only return that number of files + if ($search->limit->maxResults !== 0) { + $nodes = \array_slice($nodes, 0, $search->limit->maxResults); + } + + return $nodes; + } + + private function sort(SearchResult $a, SearchResult $b, array $orders) { + /** @var Order $oder */ + foreach ($orders as $order) { + $v1 = $this->getSearchResultProperty($a, $order->property); + $v2 = $this->getSearchResultProperty($b, $order->property); + + + if ($v1 === null && $v2 === null) { + continue; + } + if ($v1 === null) { + return $order->order === Order::ASC ? 1 : -1; + } + if ($v2 === null) { + return $order->order === Order::ASC ? -1 : 1; + } + + $s = $this->compareProperties($v1, $v2, $order); + if ($s === 0) { + continue; + } + + if ($order->order === Order::DESC) { + $s = -$s; + } + return $s; + } + + return 0; + } + + private function compareProperties($a, $b, $order) { + $allProps = $this->getPropertyDefinitionsForScope('', ''); + + foreach ($allProps as $prop) { + if ($prop->name === $order->property) { + $dataType = $prop->dataType; + switch ($dataType) { + case SearchPropertyDefinition::DATATYPE_STRING: + return strcmp($a, $b); + case SearchPropertyDefinition::DATATYPE_BOOLEAN: + if ($a === $b) { + return 0; + } + if ($a === false) { + return -1; + } + return 1; + default: + if ($a === $b) { + return 0; + } + if ($a < $b) { + return -1; + } + if ($a > $b) { + return 1; + } + } + } + } + } + + private function getSearchResultProperty(SearchResult $result, $propertyName) { + /** @var \OCA\DAV\Connector\Sabre\Node $node */ + $node = $result->node; + + switch ($propertyName) { + case '{DAV:}displayname': + return $node->getName(); + case '{DAV:}getlastmodified': + return $node->getLastModified(); + case FilesPlugin::SIZE_PROPERTYNAME: + return $node->getSize(); + case FilesPlugin::INTERNAL_FILEID_PROPERTYNAME: + return $node->getInternalFileId(); + default: + return null; + } } /** @@ -183,9 +277,10 @@ class FileSearchBackend implements ISearchBackend { * @return ISearchQuery */ private function transformQuery(BasicSearch $query) { - // TODO offset, limit + // TODO offset + $limit = $query->limit; $orders = array_map([$this, 'mapSearchOrder'], $query->orderBy); - return new SearchQuery($this->transformSearchOperation($query->where), 0, 0, $orders, $this->user); + return new SearchQuery($this->transformSearchOperation($query->where), $limit->maxResults, 0, $orders, $this->user); } /** From 6b5419ddf09aaa8fcbd2e9e31ec8e445d7da19b6 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 25 Jan 2018 15:55:08 +0100 Subject: [PATCH 2/4] Adjust to updated searchdav library Signed-off-by: Robin Appelman --- 3rdparty | 2 +- apps/dav/lib/Files/FileSearchBackend.php | 118 ++++++++++------------- 2 files changed, 52 insertions(+), 68 deletions(-) diff --git a/3rdparty b/3rdparty index 546b952d0e..e42a129bf8 160000 --- a/3rdparty +++ b/3rdparty @@ -1 +1 @@ -Subproject commit 546b952d0ec6c53e2e42ecd99ca0d94e35b7912d +Subproject commit e42a129bf8c10cf9e519cc9e4a0e6978bbcd7e59 diff --git a/apps/dav/lib/Files/FileSearchBackend.php b/apps/dav/lib/Files/FileSearchBackend.php index 2aa2ccba21..965176ddd0 100644 --- a/apps/dav/lib/Files/FileSearchBackend.php +++ b/apps/dav/lib/Files/FileSearchBackend.php @@ -45,10 +45,10 @@ use Sabre\DAV\Exception\NotFound; use SearchDAV\Backend\ISearchBackend; use SearchDAV\Backend\SearchPropertyDefinition; use SearchDAV\Backend\SearchResult; -use SearchDAV\XML\BasicSearch; -use SearchDAV\XML\Literal; -use SearchDAV\XML\Operator; -use SearchDAV\XML\Order; +use SearchDAV\Query\Query; +use SearchDAV\Query\Literal; +use SearchDAV\Query\Operator; +use SearchDAV\Query\Order; class FileSearchBackend implements ISearchBackend { /** @var CachingTree */ @@ -135,10 +135,10 @@ class FileSearchBackend implements ISearchBackend { } /** - * @param BasicSearch $search + * @param Query $search * @return SearchResult[] */ - public function search(BasicSearch $search) { + public function search(Query $search) { if (count($search->from) !== 1) { throw new \InvalidArgumentException('Searching more than one folder is not supported'); } @@ -170,7 +170,7 @@ class FileSearchBackend implements ISearchBackend { }, $results); // Sort again, since the result from multiple storages is appended and not sorted - usort($nodes, function(SearchResult $a, SearchResult $b) use ($search) { + usort($nodes, function (SearchResult $a, SearchResult $b) use ($search) { return $this->sort($a, $b, $search->orderBy); }); @@ -183,7 +183,7 @@ class FileSearchBackend implements ISearchBackend { } private function sort(SearchResult $a, SearchResult $b, array $orders) { - /** @var Order $oder */ + /** @var Order $order */ foreach ($orders as $order) { $v1 = $this->getSearchResultProperty($a, $order->property); $v2 = $this->getSearchResultProperty($b, $order->property); @@ -213,43 +213,34 @@ class FileSearchBackend implements ISearchBackend { return 0; } - private function compareProperties($a, $b, $order) { - $allProps = $this->getPropertyDefinitionsForScope('', ''); - - foreach ($allProps as $prop) { - if ($prop->name === $order->property) { - $dataType = $prop->dataType; - switch ($dataType) { - case SearchPropertyDefinition::DATATYPE_STRING: - return strcmp($a, $b); - case SearchPropertyDefinition::DATATYPE_BOOLEAN: - if ($a === $b) { - return 0; - } - if ($a === false) { - return -1; - } - return 1; - default: - if ($a === $b) { - return 0; - } - if ($a < $b) { - return -1; - } - if ($a > $b) { - return 1; - } + private function compareProperties($a, $b, Order $order) { + switch ($order->property->dataType) { + case SearchPropertyDefinition::DATATYPE_STRING: + return strcmp($a, $b); + case SearchPropertyDefinition::DATATYPE_BOOLEAN: + if ($a === $b) { + return 0; } - } + if ($a === false) { + return -1; + } + return 1; + default: + if ($a === $b) { + return 0; + } + if ($a < $b) { + return -1; + } + return 1; } } - private function getSearchResultProperty(SearchResult $result, $propertyName) { + private function getSearchResultProperty(SearchResult $result, SearchPropertyDefinition $property) { /** @var \OCA\DAV\Connector\Sabre\Node $node */ $node = $result->node; - switch ($propertyName) { + switch ($property->name) { case '{DAV:}displayname': return $node->getName(); case '{DAV:}getlastmodified': @@ -273,10 +264,10 @@ class FileSearchBackend implements ISearchBackend { } /** - * @param BasicSearch $query + * @param Query $query * @return ISearchQuery */ - private function transformQuery(BasicSearch $query) { + private function transformQuery(Query $query) { // TODO offset $limit = $query->limit; $orders = array_map([$this, 'mapSearchOrder'], $query->orderBy); @@ -312,7 +303,7 @@ class FileSearchBackend implements ISearchBackend { if (count($operator->arguments) !== 2) { throw new \InvalidArgumentException('Invalid number of arguments for ' . $trimmedType . ' operation'); } - if (!is_string($operator->arguments[0])) { + if (!($operator->arguments[0] instanceof SearchPropertyDefinition)) { throw new \InvalidArgumentException('Invalid argument 1 for ' . $trimmedType . ' operation, expected property'); } if (!($operator->arguments[1] instanceof Literal)) { @@ -327,11 +318,11 @@ class FileSearchBackend implements ISearchBackend { } /** - * @param string $propertyName + * @param SearchPropertyDefinition $property * @return string */ - private function mapPropertyNameToColumn($propertyName) { - switch ($propertyName) { + private function mapPropertyNameToColumn(SearchPropertyDefinition $property) { + switch ($property->name) { case '{DAV:}displayname': return 'name'; case '{DAV:}getcontenttype': @@ -347,33 +338,26 @@ class FileSearchBackend implements ISearchBackend { case FilesPlugin::INTERNAL_FILEID_PROPERTYNAME: return 'fileid'; default: - throw new \InvalidArgumentException('Unsupported property for search or order: ' . $propertyName); + throw new \InvalidArgumentException('Unsupported property for search or order: ' . $property->name); } } - private function castValue($propertyName, $value) { - $allProps = $this->getPropertyDefinitionsForScope('', ''); - foreach ($allProps as $prop) { - if ($prop->name === $propertyName) { - $dataType = $prop->dataType; - switch ($dataType) { - case SearchPropertyDefinition::DATATYPE_BOOLEAN: - return $value === 'yes'; - case SearchPropertyDefinition::DATATYPE_DECIMAL: - case SearchPropertyDefinition::DATATYPE_INTEGER: - case SearchPropertyDefinition::DATATYPE_NONNEGATIVE_INTEGER: - return 0 + $value; - case SearchPropertyDefinition::DATATYPE_DATETIME: - if (is_numeric($value)) { - return 0 + $value; - } - $date = \DateTime::createFromFormat(\DateTime::ATOM, $value); - return ($date instanceof \DateTime) ? $date->getTimestamp() : 0; - default: - return $value; + private function castValue(SearchPropertyDefinition $property, $value) { + switch ($property->dataType) { + case SearchPropertyDefinition::DATATYPE_BOOLEAN: + return $value === 'yes'; + case SearchPropertyDefinition::DATATYPE_DECIMAL: + case SearchPropertyDefinition::DATATYPE_INTEGER: + case SearchPropertyDefinition::DATATYPE_NONNEGATIVE_INTEGER: + return 0 + $value; + case SearchPropertyDefinition::DATATYPE_DATETIME: + if (is_numeric($value)) { + return 0 + $value; } - } + $date = \DateTime::createFromFormat(\DateTime::ATOM, $value); + return ($date instanceof \DateTime) ? $date->getTimestamp() : 0; + default: + return $value; } - return $value; } } From c1ff12e2340a6a1ea2a9032848778af7085ff150 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Wed, 7 Feb 2018 09:35:55 +0100 Subject: [PATCH 3/4] CacheJail should apply limit and offset after searching Else the results might not be correct. Signed-off-by: Roeland Jago Douma --- apps/dav/lib/Files/FileSearchBackend.php | 2 +- lib/private/Files/Cache/Wrapper/CacheJail.php | 11 +++++++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/apps/dav/lib/Files/FileSearchBackend.php b/apps/dav/lib/Files/FileSearchBackend.php index 965176ddd0..275d3dc8a4 100644 --- a/apps/dav/lib/Files/FileSearchBackend.php +++ b/apps/dav/lib/Files/FileSearchBackend.php @@ -271,7 +271,7 @@ class FileSearchBackend implements ISearchBackend { // TODO offset $limit = $query->limit; $orders = array_map([$this, 'mapSearchOrder'], $query->orderBy); - return new SearchQuery($this->transformSearchOperation($query->where), $limit->maxResults, 0, $orders, $this->user); + return new SearchQuery($this->transformSearchOperation($query->where), (int)$limit->maxResults, 0, $orders, $this->user); } /** diff --git a/lib/private/Files/Cache/Wrapper/CacheJail.php b/lib/private/Files/Cache/Wrapper/CacheJail.php index 1ad00ba44c..75df45e257 100644 --- a/lib/private/Files/Cache/Wrapper/CacheJail.php +++ b/lib/private/Files/Cache/Wrapper/CacheJail.php @@ -29,6 +29,7 @@ namespace OC\Files\Cache\Wrapper; use OC\Files\Cache\Cache; +use OC\Files\Search\SearchQuery; use OCP\Files\Cache\ICacheEntry; use OCP\Files\Search\ISearchQuery; @@ -236,8 +237,14 @@ class CacheJail extends CacheWrapper { } public function searchQuery(ISearchQuery $query) { - $results = $this->getCache()->searchQuery($query); - return $this->formatSearchResults($results); + $simpleQuery = new SearchQuery($query->getSearchOperation(), 0, 0, $query->getOrder(), $query->getUser()); + $results = $this->getCache()->searchQuery($simpleQuery); + $results = $this->formatSearchResults($results); + + $limit = $query->getLimit() === 0 ? NULL : $query->getLimit(); + $results = array_slice($results, $query->getOffset(), $limit); + + return $results; } /** From e4129b0dc76ce1dd7fd84c6297911174f0f92de7 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 7 Feb 2018 14:55:50 +0100 Subject: [PATCH 4/4] adjust tests Signed-off-by: Robin Appelman --- .../unit/Files/FileSearchBackendTest.php | 38 ++++++++++--------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/apps/dav/tests/unit/Files/FileSearchBackendTest.php b/apps/dav/tests/unit/Files/FileSearchBackendTest.php index 14df99a278..20a7566c2f 100644 --- a/apps/dav/tests/unit/Files/FileSearchBackendTest.php +++ b/apps/dav/tests/unit/Files/FileSearchBackendTest.php @@ -38,6 +38,9 @@ use OCP\Files\IRootFolder; use OCP\Files\Search\ISearchComparison; use OCP\IUser; use OCP\Share\IManager; +use SearchDAV\Backend\SearchPropertyDefinition; +use SearchDAV\Query\Limit; +use SearchDAV\Query\Query; use SearchDAV\XML\BasicSearch; use SearchDAV\XML\Literal; use SearchDAV\XML\Operator; @@ -132,7 +135,7 @@ class FileSearchBackendTest extends TestCase { new \OC\Files\Node\Folder($this->rootFolder, $this->view, '/test/path') ])); - $query = $this->getBasicQuery(Operator::OPERATION_EQUAL, '{DAV:}displayname', 'foo'); + $query = $this->getBasicQuery(\SearchDAV\Query\Operator::OPERATION_EQUAL, '{DAV:}displayname', 'foo'); $result = $this->search->search($query); $this->assertCount(1, $result); @@ -161,7 +164,7 @@ class FileSearchBackendTest extends TestCase { new \OC\Files\Node\Folder($this->rootFolder, $this->view, '/test/path') ])); - $query = $this->getBasicQuery(Operator::OPERATION_EQUAL, '{DAV:}getcontenttype', 'foo'); + $query = $this->getBasicQuery(\SearchDAV\Query\Operator::OPERATION_EQUAL, '{DAV:}getcontenttype', 'foo'); $result = $this->search->search($query); $this->assertCount(1, $result); @@ -190,7 +193,7 @@ class FileSearchBackendTest extends TestCase { new \OC\Files\Node\Folder($this->rootFolder, $this->view, '/test/path') ])); - $query = $this->getBasicQuery(Operator::OPERATION_GREATER_THAN, FilesPlugin::SIZE_PROPERTYNAME, 10); + $query = $this->getBasicQuery(\SearchDAV\Query\Operator::OPERATION_GREATER_THAN, FilesPlugin::SIZE_PROPERTYNAME, 10); $result = $this->search->search($query); $this->assertCount(1, $result); @@ -219,7 +222,7 @@ class FileSearchBackendTest extends TestCase { new \OC\Files\Node\Folder($this->rootFolder, $this->view, '/test/path') ])); - $query = $this->getBasicQuery(Operator::OPERATION_GREATER_THAN, '{DAV:}getlastmodified', 10); + $query = $this->getBasicQuery(\SearchDAV\Query\Operator::OPERATION_GREATER_THAN, '{DAV:}getlastmodified', 10); $result = $this->search->search($query); $this->assertCount(1, $result); @@ -248,7 +251,7 @@ class FileSearchBackendTest extends TestCase { new \OC\Files\Node\Folder($this->rootFolder, $this->view, '/test/path') ])); - $query = $this->getBasicQuery(Operator::OPERATION_IS_COLLECTION, 'yes'); + $query = $this->getBasicQuery(\SearchDAV\Query\Operator::OPERATION_IS_COLLECTION, 'yes'); $result = $this->search->search($query); $this->assertCount(1, $result); @@ -266,29 +269,30 @@ class FileSearchBackendTest extends TestCase { $this->searchFolder->expects($this->never()) ->method('search'); - $query = $this->getBasicQuery(Operator::OPERATION_EQUAL, '{DAV:}getetag', 'foo'); + $query = $this->getBasicQuery(\SearchDAV\Query\Operator::OPERATION_EQUAL, '{DAV:}getetag', 'foo'); $this->search->search($query); } private function getBasicQuery($type, $property, $value = null) { - $query = new BasicSearch(); - $scope = new Scope('/', 'infinite'); + $scope = new \SearchDAV\Query\Scope('/', 'infinite'); $scope->path = '/'; - $query->from = [$scope]; - $query->orderBy = []; - $query->select = []; + $from = [$scope]; + $orderBy = []; + $select = []; if (is_null($value)) { - $query->where = new Operator( + $where = new \SearchDAV\Query\Operator( $type, - [new Literal($property)] + [new \SearchDAV\Query\Literal($property)] ); } else { - $query->where = new Operator( + $where = new \SearchDAV\Query\Operator( $type, - [$property, new Literal($value)] + [new SearchPropertyDefinition($property, true, true, true), new \SearchDAV\Query\Literal($value)] ); } - return $query; + $limit = new Limit(); + + return new Query($select, $from, $where, $orderBy, $limit); } /** @@ -301,7 +305,7 @@ class FileSearchBackendTest extends TestCase { ->method('getNodeForPath') ->willReturn($davNode); - $query = $this->getBasicQuery(Operator::OPERATION_EQUAL, '{DAV:}displayname', 'foo'); + $query = $this->getBasicQuery(\SearchDAV\Query\Operator::OPERATION_EQUAL, '{DAV:}displayname', 'foo'); $this->search->search($query); } }