Skip to content

Commit 26f40db

Browse files
Vincent Petryicewind1991
Vincent Petry
authored and
icewind1991
committed
Adjust REPORT search query, perf and unit tests
- disable REPORT without filter Because we need to use Node::search($pattern) to find all matching nodes in all the subfolder recursively, but the result nodes contain incomplete information like owner. - remove unneeded trailing slash when buildling response href This got obsoleted when switching to generateMultiStatus() - add integration pagination test for favorites REPORT - throw exception for unknown node types in FilesReportPlugin Signed-off-by: Vincent Petry <pvince81@owncloud.com>
1 parent 2f2cc99 commit 26f40db

5 files changed

Lines changed: 240 additions & 53 deletions

File tree

‎apps/dav/lib/Connector/Sabre/FilesReportPlugin.php‎

Lines changed: 33 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -179,27 +179,31 @@ public function onReport($reportName, $report, $uri) {
179179
$requestedProps = $report->properties;
180180
$filterRules = $report->filters;
181181

182+
// "systemtag" is always an array of tags, favorite a string/int/null
182183
if (empty($filterRules['systemtag']) && is_null($filterRules['favorite'])) {
183-
// load all
184-
$results = $reportTargetNode->getChildren();
184+
// FIXME: search currently not possible because results are missing properties!
185+
throw new BadRequest('No filter criteria specified');
185186
} else {
187+
if (isset($report->search['pattern'])) {
188+
// TODO: implement this at some point...
189+
throw new BadRequest('Search pattern cannot be combined with filter');
190+
}
191+
186192
// gather all file ids matching filter
187193
try {
188194
$resultFileIds = $this->processFilterRules($filterRules);
189195
} catch (TagNotFoundException $e) {
190196
throw new PreconditionFailed('Cannot filter by non-existing tag', 0, $e);
191197
}
192198

199+
// pre-slice the results if needed for pagination to not waste
200+
// time resolving nodes that will not be returned anyway
201+
$resultFileIds = $this->slice($resultFileIds, $report);
202+
193203
// find sabre nodes by file id, restricted to the root node path
194204
$results = $this->findNodesByFileIds($reportTargetNode, $resultFileIds);
195205
}
196206

197-
if (!is_null($report->limit)) {
198-
$length = $report->limit['size'];
199-
$offset = $report->limit['page'] * $length;
200-
$results = array_slice($results, $offset, $length);
201-
}
202-
203207
$filesUri = $this->getFilesBaseUri($uri, $reportTargetNode->getPath());
204208
$results = $this->prepareResponses($filesUri, $requestedProps, $results);
205209

@@ -212,6 +216,15 @@ public function onReport($reportName, $report, $uri) {
212216
return false;
213217
}
214218

219+
private function slice($results, $report) {
220+
if (!is_null($report->search)) {
221+
$length = $report->search['limit'];
222+
$offset = $report->search['offset'];
223+
$results = array_slice($results, $offset, $length);
224+
}
225+
return $results;
226+
}
227+
215228
/**
216229
* Returns the base uri of the files root by removing
217230
* the subpath from the URI
@@ -330,11 +343,6 @@ public function prepareResponses($filesUri, $requestedProps, $nodes) {
330343
$result = $propFind->getResultForMultiStatus();
331344
$result['href'] = $propFind->getPath();
332345

333-
$resourceType = $this->server->getResourceTypeForNode($node);
334-
if (in_array('{DAV:}collection', $resourceType) || in_array('{DAV:}principal', $resourceType)) {
335-
$result['href'] .= '/';
336-
}
337-
338346
$results[] = $result;
339347
}
340348
return $results;
@@ -358,17 +366,25 @@ public function findNodesByFileIds($rootNode, $fileIds) {
358366
$entry = $folder->getById($fileId);
359367
if ($entry) {
360368
$entry = current($entry);
361-
if ($entry instanceof \OCP\Files\File) {
362-
$results[] = new File($this->fileView, $entry);
363-
} else if ($entry instanceof \OCP\Files\Folder) {
364-
$results[] = new Directory($this->fileView, $entry);
369+
$node = $this->makeSabreNode($entry);
370+
if ($node) {
371+
$results[] = $node;
365372
}
366373
}
367374
}
368375

369376
return $results;
370377
}
371378

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

‎apps/dav/lib/Files/Xml/FilterRequest.php‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ class FilterRequest implements XmlDeserializable {
2424
/**
2525
* @var array
2626
*/
27-
public $limit;
27+
public $search;
2828

2929
/**
3030
* The deserialize method is called during xml parsing.
@@ -51,7 +51,7 @@ static function xmlDeserialize(Reader $reader) {
5151
$elems = (array)$reader->parseInnerTree([
5252
'{DAV:}prop' => KeyValue::class,
5353
'{http://owncloud.org/ns}filter-rules' => Base::class,
54-
'{http://owncloud.org/ns}limit' => Base::class
54+
'{http://owncloud.org/ns}search' => KeyValue::class,
5555
]);
5656

5757
$newProps = [
@@ -60,7 +60,7 @@ static function xmlDeserialize(Reader $reader) {
6060
'favorite' => null
6161
],
6262
'properties' => [],
63-
'limit' => null,
63+
'search' => null,
6464
];
6565

6666
if (!is_array($elems)) {
@@ -85,13 +85,19 @@ static function xmlDeserialize(Reader $reader) {
8585
}
8686
}
8787
break;
88-
case '{http://owncloud.org/ns}limit' :
89-
// TODO verify page and size
90-
$newProps['limit'] = $elem['attributes'];
88+
case '{http://owncloud.org/ns}search' :
89+
$value = $elem['value'];
90+
if (isset($value['{http://owncloud.org/ns}pattern'])) {
91+
$newProps['search']['pattern'] = $value['{http://owncloud.org/ns}pattern'];
92+
}
93+
if (isset($value['{http://owncloud.org/ns}limit'])) {
94+
$newProps['search']['limit'] = (int)$value['{http://owncloud.org/ns}limit'];
95+
}
96+
if (isset($value['{http://owncloud.org/ns}offset'])) {
97+
$newProps['search']['offset'] = (int)$value['{http://owncloud.org/ns}offset'];
98+
}
9199
break;
92-
93100
}
94-
95101
}
96102

97103
$obj = new self();

‎apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php‎

Lines changed: 135 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,9 @@
3535
use OCP\IGroupManager;
3636
use OCP\SystemTag\ISystemTagManager;
3737
use OCP\ITags;
38+
use OCP\Files\FileInfo;
39+
use OCP\IRequest;
40+
use OCP\IConfig;
3841

3942
class FilesReportPluginTest extends \Test\TestCase {
4043
/** @var \Sabre\DAV\Server|\PHPUnit_Framework_MockObject_MockObject */
@@ -76,13 +79,11 @@ public function setUp() {
7679
->disableOriginalConstructor()
7780
->getMock();
7881

79-
$this->view = $this->getMockBuilder('\OC\Files\View')
80-
->disableOriginalConstructor()
81-
->getMock();
82+
$this->view = new View();
8283

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

8889
$this->server->expects($this->any())
@@ -121,6 +122,15 @@ public function setUp() {
121122
->method('getUser')
122123
->will($this->returnValue($user));
123124

125+
// add FilesPlugin to test more properties
126+
$this->server->addPlugin(
127+
new \OCA\DAV\Connector\Sabre\FilesPlugin(
128+
$this->tree,
129+
$this->createMock(IConfig::class),
130+
$this->createMock(IRequest::class)
131+
)
132+
);
133+
124134
$this->plugin = new FilesReportPluginImplementation(
125135
$this->tree,
126136
$this->view,
@@ -177,7 +187,12 @@ public function testOnReport() {
177187
$path = 'test';
178188

179189
$parameters = new FilterRequest();
180-
$parameters->properties = ['{DAV:}getcontentlength', '{http://owncloud.org/ns}size'];
190+
$parameters->properties = [
191+
'{DAV:}getcontentlength',
192+
'{http://owncloud.org/ns}size',
193+
'{http://owncloud.org/ns}fileid',
194+
'{DAV:}resourcetype',
195+
];
181196
$parameters->filters = [
182197
'systemtag' => [123, 456],
183198
'favorite' => null
@@ -196,14 +211,8 @@ public function testOnReport() {
196211
->with('456', 'files')
197212
->will($this->returnValue(['111', '222', '333']));
198213

199-
$reportTargetNode = $this->getMockBuilder('\OCA\DAV\Connector\Sabre\Directory')
200-
->disableOriginalConstructor()
201-
->getMock();
202-
203-
$response = $this->getMockBuilder('Sabre\HTTP\ResponseInterface')
204-
->disableOriginalConstructor()
205-
->getMock();
206-
214+
$reportTargetNode = $this->createMock(\OCA\DAV\Connector\Sabre\Directory::class);
215+
$response = $this->createMock(\Sabre\HTTP\ResponseInterface::class);
207216
$response->expects($this->once())
208217
->method('setHeader')
209218
->with('Content-Type', 'application/xml; charset=utf-8');
@@ -220,12 +229,16 @@ public function testOnReport() {
220229
->with('/' . $path)
221230
->will($this->returnValue($reportTargetNode));
222231

223-
$filesNode1 = $this->getMockBuilder('\OCP\Files\Folder')
224-
->disableOriginalConstructor()
225-
->getMock();
226-
$filesNode2 = $this->getMockBuilder('\OCP\Files\File')
227-
->disableOriginalConstructor()
228-
->getMock();
232+
$filesNode1 = $this->createMock(\OCP\Files\Folder::class);
233+
$filesNode1->method('getId')->willReturn(111);
234+
$filesNode1->method('getPath')->willReturn('/node1');
235+
$filesNode1->method('isReadable')->willReturn(true);
236+
$filesNode1->method('getSize')->willReturn(2048);
237+
$filesNode2 = $this->createMock(\OCP\Files\File::class);
238+
$filesNode2->method('getId')->willReturn(222);
239+
$filesNode2->method('getPath')->willReturn('/sub/node2');
240+
$filesNode2->method('getSize')->willReturn(1024);
241+
$filesNode2->method('isReadable')->willReturn(true);
229242

230243
$this->userFolder->expects($this->at(0))
231244
->method('getById')
@@ -242,7 +255,110 @@ public function testOnReport() {
242255
$this->server->httpResponse = $response;
243256
$this->plugin->initialize($this->server);
244257

258+
$responses = null;
259+
$this->server->expects($this->once())
260+
->method('generateMultiStatus')
261+
->will($this->returnCallback(function($responsesArg) use (&$responses) {
262+
$responses = $responsesArg;
263+
})
264+
);
265+
266+
$this->assertFalse($this->plugin->onReport(FilesReportPluginImplementation::REPORT_NAME, $parameters, '/' . $path));
267+
268+
$this->assertCount(2, $responses);
269+
270+
$this->assertTrue(isset($responses[0][200]));
271+
$this->assertTrue(isset($responses[1][200]));
272+
273+
$this->assertEquals('/test/node1', $responses[0]['href']);
274+
$this->assertEquals('/test/sub/node2', $responses[1]['href']);
275+
276+
$props1 = $responses[0];
277+
$this->assertEquals('111', $props1[200]['{http://owncloud.org/ns}fileid']);
278+
$this->assertNull($props1[404]['{DAV:}getcontentlength']);
279+
$this->assertInstanceOf('\Sabre\DAV\Xml\Property\ResourceType', $props1[200]['{DAV:}resourcetype']);
280+
$resourceType1 = $props1[200]['{DAV:}resourcetype']->getValue();
281+
$this->assertEquals('{DAV:}collection', $resourceType1[0]);
282+
283+
$props2 = $responses[1];
284+
$this->assertEquals('1024', $props2[200]['{DAV:}getcontentlength']);
285+
$this->assertEquals('222', $props2[200]['{http://owncloud.org/ns}fileid']);
286+
$this->assertInstanceOf('\Sabre\DAV\Xml\Property\ResourceType', $props2[200]['{DAV:}resourcetype']);
287+
$this->assertCount(0, $props2[200]['{DAV:}resourcetype']->getValue());
288+
}
289+
290+
public function testOnReportPaginationFiltered() {
291+
$path = 'test';
292+
293+
$parameters = new FilterRequest();
294+
$parameters->properties = [
295+
'{DAV:}getcontentlength',
296+
];
297+
$parameters->filters = [
298+
'systemtag' => [],
299+
'favorite' => true
300+
];
301+
$parameters->search = [
302+
'offset' => 2,
303+
'limit' => 3,
304+
];
305+
306+
$filesNodes = [];
307+
for ($i = 0; $i < 20; $i++) {
308+
$filesNode = $this->createMock(\OCP\Files\File::class);
309+
$filesNode->method('getId')->willReturn(1000 + $i);
310+
$filesNode->method('getPath')->willReturn('/nodes/node' . $i);
311+
$filesNode->method('isReadable')->willReturn(true);
312+
$filesNodes[$filesNode->getId()] = $filesNode;
313+
}
314+
315+
// return all above nodes as favorites
316+
$this->privateTags->expects($this->once())
317+
->method('getFavorites')
318+
->will($this->returnValue(array_keys($filesNodes)));
319+
320+
$reportTargetNode = $this->createMock(\OCA\DAV\Connector\Sabre\Directory::class);
321+
322+
$this->tree->expects($this->any())
323+
->method('getNodeForPath')
324+
->with('/' . $path)
325+
->will($this->returnValue($reportTargetNode));
326+
327+
// getById must only be called for the required nodes
328+
$this->userFolder->expects($this->at(0))
329+
->method('getById')
330+
->with(1002)
331+
->willReturn([$filesNodes[1002]]);
332+
$this->userFolder->expects($this->at(1))
333+
->method('getById')
334+
->with(1003)
335+
->willReturn([$filesNodes[1003]]);
336+
$this->userFolder->expects($this->at(2))
337+
->method('getById')
338+
->with(1004)
339+
->willReturn([$filesNodes[1004]]);
340+
341+
$this->server->expects($this->any())
342+
->method('getRequestUri')
343+
->will($this->returnValue($path));
344+
345+
$this->plugin->initialize($this->server);
346+
347+
$responses = null;
348+
$this->server->expects($this->once())
349+
->method('generateMultiStatus')
350+
->will($this->returnCallback(function($responsesArg) use (&$responses) {
351+
$responses = $responsesArg;
352+
})
353+
);
354+
245355
$this->assertFalse($this->plugin->onReport(FilesReportPluginImplementation::REPORT_NAME, $parameters, '/' . $path));
356+
357+
$this->assertCount(3, $responses);
358+
359+
$this->assertEquals('/test/nodes/node2', $responses[0]['href']);
360+
$this->assertEquals('/test/nodes/node3', $responses[1]['href']);
361+
$this->assertEquals('/test/nodes/node4', $responses[2]['href']);
246362
}
247363

248364
public function testFindNodesByFileIdsRoot() {

0 commit comments

Comments
 (0)