Skip to content

Commit 0f50fe1

Browse files
icewind1991icewind1991
committed
update tests and fix some edge cases around new search
Signed-off-by: Robin Appelman <[email protected]>
1 parent d334e81 commit 0f50fe1

File tree

3 files changed

+96
-83
lines changed

3 files changed

+96
-83
lines changed

‎lib/private/Files/Cache/QuerySearchHelper.php‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -243,7 +243,11 @@ private function getParameterForValue(IQueryBuilder $builder, $value) {
243243
*/
244244
public function addSearchOrdersToQuery(IQueryBuilder $query, array $orders) {
245245
foreach ($orders as $order) {
246-
$query->addOrderBy($order->getField(), $order->getDirection());
246+
$field = $order->getField();
247+
if ($field === 'fileid') {
248+
$field = 'file.fileid';
249+
}
250+
$query->addOrderBy($field, $order->getDirection());
247251
}
248252
}
249253

‎lib/private/Files/Node/Folder.php‎

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,6 @@
3434
use OC\DB\QueryBuilder\Literal;
3535
use OC\Files\Cache\QuerySearchHelper;
3636
use OC\Files\Cache\Wrapper\CacheJail;
37-
use OC\Files\Search\SearchBinaryOperator;
3837
use OC\Files\Search\SearchComparison;
3938
use OC\Files\Search\SearchQuery;
4039
use OC\Files\Storage\Wrapper\Jail;
@@ -47,7 +46,6 @@
4746
use OCP\Files\Mount\IMountPoint;
4847
use OCP\Files\NotFoundException;
4948
use OCP\Files\NotPermittedException;
50-
use OCP\Files\Search\ISearchBinaryOperator;
5149
use OCP\Files\Search\ISearchComparison;
5250
use OCP\Files\Search\ISearchOperator;
5351
use OCP\Files\Search\ISearchQuery;
@@ -252,7 +250,12 @@ public function search($query) {
252250
// collect all caches for this folder, indexed by their mountpoint relative to this folder
253251
// and save the mount which is needed later to construct the FileInfo objects
254252

255-
$caches = ['' => new CacheJail($storage->getCache(''), $internalPath)]; // a temporary CacheJail is used to handle filtering down the results to within this folder
253+
if ($internalPath !== '') {
254+
// a temporary CacheJail is used to handle filtering down the results to within this folder
255+
$caches = ['' => new CacheJail($storage->getCache(''), $internalPath)];
256+
} else {
257+
$caches = ['' => $storage->getCache('')];
258+
}
256259
$mountByMountPoint = ['' => $mount];
257260

258261
if (!$limitToHome) {
@@ -272,13 +275,27 @@ public function search($query) {
272275
$resultsPerCache = $searchHelper->searchInCaches($query, $caches);
273276

274277
// loop trough all results per-cache, constructing the FileInfo object from the CacheEntry and merge them all
275-
$files = array_merge(...array_map(function(array $results, $relativeMountPoint) use ($mountByMountPoint) {
278+
$files = array_merge(...array_map(function (array $results, $relativeMountPoint) use ($mountByMountPoint) {
276279
$mount = $mountByMountPoint[$relativeMountPoint];
277-
return array_map(function(ICacheEntry $result) use ($relativeMountPoint, $mount) {
280+
return array_map(function (ICacheEntry $result) use ($relativeMountPoint, $mount) {
278281
return $this->cacheEntryToFileInfo($mount, $relativeMountPoint, $result);
279282
}, $results);
280283
}, array_values($resultsPerCache), array_keys($resultsPerCache)));
281284

285+
// since results were returned per-cache, they are no longer fully sorted
286+
$order = $query->getOrder();
287+
if ($order) {
288+
usort($files, function (FileInfo $a, FileInfo $b) use ($order) {
289+
foreach ($order as $orderField) {
290+
$cmp = $orderField->sortFileInfo($a, $b);
291+
if ($cmp !== 0) {
292+
return $cmp;
293+
}
294+
}
295+
return 0;
296+
});
297+
}
298+
282299
return array_map(function (FileInfo $file) {
283300
return $this->createNode($file->getPath(), $file);
284301
}, $files);

‎tests/lib/Files/Node/FolderTest.php‎

Lines changed: 69 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,11 @@
2323
use OC\Files\Storage\Temporary;
2424
use OC\Files\Storage\Wrapper\Jail;
2525
use OC\Files\View;
26+
use OCP\Files\Cache\ICacheEntry;
2627
use OCP\Files\Mount\IMountPoint;
2728
use OCP\Files\NotFoundException;
2829
use OCP\Files\Search\ISearchComparison;
2930
use OCP\Files\Search\ISearchOrder;
30-
use OCP\Files\Search\ISearchQuery;
3131
use OCP\Files\Storage;
3232

3333
/**
@@ -294,9 +294,10 @@ public function testSearch() {
294294
->getMock();
295295
$root->method('getUser')
296296
->willReturn($this->user);
297-
$storage = $this->createMock(Storage::class);
298-
$storage->method('getId')->willReturn('');
299-
$cache = $this->getMockBuilder(Cache::class)->setConstructorArgs([$storage])->getMock();
297+
/** @var Storage\IStorage $storage */
298+
$storage = $this->createMock(Storage\IStorage::class);
299+
$storage->method('getId')->willReturn('test::1');
300+
$cache = new Cache($storage);
300301

301302
$storage->method('getCache')
302303
->willReturn($cache);
@@ -307,10 +308,8 @@ public function testSearch() {
307308
$mount->method('getInternalPath')
308309
->willReturn('foo');
309310

310-
$cache->method('searchQuery')
311-
->willReturn([
312-
new CacheEntry(['fileid' => 3, 'path' => 'foo/qwerty', 'name' => 'qwerty', 'size' => 200, 'mtime' => 55, 'mimetype' => 'text/plain']),
313-
]);
311+
$cache->insert('foo', ['size' => 200, 'mtime' => 55, 'mimetype' => ICacheEntry::DIRECTORY_MIMETYPE]);
312+
$cache->insert('foo/qwerty', ['size' => 200, 'mtime' => 55, 'mimetype' => 'text/plain']);
314313

315314
$root->method('getMountsIn')
316315
->with('/bar/foo')
@@ -322,6 +321,7 @@ public function testSearch() {
322321

323322
$node = new Folder($root, $view, '/bar/foo');
324323
$result = $node->search('qw');
324+
$cache->clear();
325325
$this->assertEquals(1, count($result));
326326
$this->assertEquals('/bar/foo/qwerty', $result[0]->getPath());
327327
}
@@ -341,8 +341,8 @@ public function testSearchInRoot() {
341341
->willReturn($this->user);
342342
/** @var \PHPUnit\Framework\MockObject\MockObject|Storage $storage */
343343
$storage = $this->createMock(Storage::class);
344-
$storage->method('getId')->willReturn('');
345-
$cache = $this->getMockBuilder(Cache::class)->setConstructorArgs([$storage])->getMock();
344+
$storage->method('getId')->willReturn('test::2');
345+
$cache = new Cache($storage);
346346

347347
$mount = $this->createMock(IMountPoint::class);
348348
$mount->method('getStorage')
@@ -353,10 +353,8 @@ public function testSearchInRoot() {
353353
$storage->method('getCache')
354354
->willReturn($cache);
355355

356-
$cache->method('searchQuery')
357-
->willReturn([
358-
new CacheEntry(['fileid' => 3, 'path' => 'files/foo', 'name' => 'qwerty', 'size' => 200, 'mtime' => 55, 'mimetype' => 'text/plain']),
359-
]);
356+
$cache->insert('files', ['size' => 200, 'mtime' => 55, 'mimetype' => ICacheEntry::DIRECTORY_MIMETYPE]);
357+
$cache->insert('files/foo', ['size' => 200, 'mtime' => 55, 'mimetype' => 'text/plain']);
360358

361359
$root->method('getMountsIn')
362360
->with('')
@@ -366,7 +364,8 @@ public function testSearchInRoot() {
366364
->with('')
367365
->willReturn($mount);
368366

369-
$result = $root->search('qw');
367+
$result = $root->search('foo');
368+
$cache->clear();
370369
$this->assertEquals(1, count($result));
371370
$this->assertEquals('/foo', $result[0]->getPath());
372371
}
@@ -383,8 +382,8 @@ public function testSearchInStorageRoot() {
383382
$root->method('getUser')
384383
->willReturn($this->user);
385384
$storage = $this->createMock(Storage::class);
386-
$storage->method('getId')->willReturn('');
387-
$cache = $this->getMockBuilder(Cache::class)->setConstructorArgs([$storage])->getMock();
385+
$storage->method('getId')->willReturn('test::1');
386+
$cache = new Cache($storage);
388387

389388
$mount = $this->createMock(IMountPoint::class);
390389
$mount->method('getStorage')
@@ -395,10 +394,9 @@ public function testSearchInStorageRoot() {
395394
$storage->method('getCache')
396395
->willReturn($cache);
397396

398-
$cache->method('searchQuery')
399-
->willReturn([
400-
new CacheEntry(['fileid' => 3, 'path' => 'foo/qwerty', 'name' => 'qwerty', 'size' => 200, 'mtime' => 55, 'mimetype' => 'text/plain']),
401-
]);
397+
$cache->insert('foo', ['size' => 200, 'mtime' => 55, 'mimetype' => ICacheEntry::DIRECTORY_MIMETYPE]);
398+
$cache->insert('foo/qwerty', ['size' => 200, 'mtime' => 55, 'mimetype' => 'text/plain']);
399+
402400

403401
$root->method('getMountsIn')
404402
->with('/bar')
@@ -410,6 +408,7 @@ public function testSearchInStorageRoot() {
410408

411409
$node = new Folder($root, $view, '/bar');
412410
$result = $node->search('qw');
411+
$cache->clear();
413412
$this->assertEquals(1, count($result));
414413
$this->assertEquals('/bar/foo/qwerty', $result[0]->getPath());
415414
}
@@ -427,10 +426,11 @@ public function testSearchSubStorages() {
427426
->method('getUser')
428427
->willReturn($this->user);
429428
$storage = $this->createMock(Storage::class);
430-
$storage->method('getId')->willReturn('');
431-
$cache = $this->getMockBuilder(Cache::class)->setConstructorArgs([$storage])->getMock();
432-
$subCache = $this->getMockBuilder(Cache::class)->setConstructorArgs([$storage])->getMock();
429+
$storage->method('getId')->willReturn('test::1');
430+
$cache = new Cache($storage);
433431
$subStorage = $this->createMock(Storage::class);
432+
$subStorage->method('getId')->willReturn('test::2');
433+
$subCache = new Cache($subStorage);
434434
$subMount = $this->getMockBuilder(MountPoint::class)->setConstructorArgs([null, ''])->getMock();
435435

436436
$mount = $this->createMock(IMountPoint::class);
@@ -451,15 +451,12 @@ public function testSearchSubStorages() {
451451
$subStorage->method('getCache')
452452
->willReturn($subCache);
453453

454-
$cache->method('searchQuery')
455-
->willReturn([
456-
new CacheEntry(['fileid' => 3, 'path' => 'foo/qwerty', 'name' => 'qwerty', 'size' => 200, 'mtime' => 55, 'mimetype' => 'text/plain']),
457-
]);
454+
$cache->insert('foo', ['size' => 200, 'mtime' => 55, 'mimetype' => ICacheEntry::DIRECTORY_MIMETYPE]);
455+
$cache->insert('foo/qwerty', ['size' => 200, 'mtime' => 55, 'mimetype' => 'text/plain']);
456+
457+
$subCache->insert('asd', ['size' => 200, 'mtime' => 55, 'mimetype' => ICacheEntry::DIRECTORY_MIMETYPE]);
458+
$subCache->insert('asd/qwerty', ['size' => 200, 'mtime' => 55, 'mimetype' => 'text/plain']);
458459

459-
$subCache->method('searchQuery')
460-
->willReturn([
461-
new CacheEntry(['fileid' => 4, 'path' => 'asd/qweasd', 'name' => 'qweasd', 'size' => 200, 'mtime' => 55, 'mimetype' => 'text/plain']),
462-
]);
463460

464461
$root->method('getMountsIn')
465462
->with('/bar/foo')
@@ -472,6 +469,8 @@ public function testSearchSubStorages() {
472469

473470
$node = new Folder($root, $view, '/bar/foo');
474471
$result = $node->search('qw');
472+
$cache->clear();
473+
$subCache->clear();
475474
$this->assertEquals(2, count($result));
476475
}
477476

@@ -931,18 +930,18 @@ public function testRecentJail() {
931930

932931
public function offsetLimitProvider() {
933932
return [
934-
[0, 10, [10, 11, 12, 13, 14, 15, 16, 17], []],
935-
[0, 5, [10, 11, 12, 13, 14], []],
936-
[0, 2, [10, 11], []],
937-
[3, 2, [13, 14], []],
938-
[3, 5, [13, 14, 15, 16, 17], []],
939-
[5, 2, [15, 16], []],
940-
[6, 2, [16, 17], []],
941-
[7, 2, [17], []],
933+
[0, 10, ['/bar/foo/foo1', '/bar/foo/foo2', '/bar/foo/foo3', '/bar/foo/foo4', '/bar/foo/sub1/foo5', '/bar/foo/sub1/foo6', '/bar/foo/sub2/foo7', '/bar/foo/sub2/foo8'], []],
934+
[0, 5, ['/bar/foo/foo1', '/bar/foo/foo2', '/bar/foo/foo3', '/bar/foo/foo4', '/bar/foo/sub1/foo5'], []],
935+
[0, 2, ['/bar/foo/foo1', '/bar/foo/foo2'], []],
936+
[3, 2, ['/bar/foo/foo4', '/bar/foo/sub1/foo5'], []],
937+
[3, 5, ['/bar/foo/foo4', '/bar/foo/sub1/foo5', '/bar/foo/sub1/foo6', '/bar/foo/sub2/foo7', '/bar/foo/sub2/foo8'], []],
938+
[5, 2, ['/bar/foo/sub1/foo6', '/bar/foo/sub2/foo7'], []],
939+
[6, 2, ['/bar/foo/sub2/foo7', '/bar/foo/sub2/foo8'], []],
940+
[7, 2, ['/bar/foo/sub2/foo8'], []],
942941
[10, 2, [], []],
943-
[0, 5, [16, 10, 14, 11, 12], [new SearchOrder(ISearchOrder::DIRECTION_ASCENDING, 'mtime')]],
944-
[3, 2, [11, 12], [new SearchOrder(ISearchOrder::DIRECTION_ASCENDING, 'mtime')]],
945-
[0, 5, [14, 15, 16, 10, 11], [
942+
[0, 5, ['/bar/foo/sub2/foo7', '/bar/foo/foo1', '/bar/foo/sub1/foo5', '/bar/foo/foo2', '/bar/foo/foo3'], [new SearchOrder(ISearchOrder::DIRECTION_ASCENDING, 'mtime')]],
943+
[3, 2, ['/bar/foo/foo2', '/bar/foo/foo3'], [new SearchOrder(ISearchOrder::DIRECTION_ASCENDING, 'mtime')]],
944+
[0, 5, ['/bar/foo/sub1/foo5', '/bar/foo/sub1/foo6', '/bar/foo/sub2/foo7', '/bar/foo/foo1', '/bar/foo/foo2'], [
946945
new SearchOrder(ISearchOrder::DIRECTION_DESCENDING, 'size'),
947946
new SearchOrder(ISearchOrder::DIRECTION_ASCENDING, 'mtime')
948947
]],
@@ -953,12 +952,16 @@ public function offsetLimitProvider() {
953952
* @dataProvider offsetLimitProvider
954953
* @param int $offset
955954
* @param int $limit
956-
* @param int[] $expectedIds
955+
* @param string[] $expectedPaths
957956
* @param ISearchOrder[] $ordering
958957
* @throws NotFoundException
959958
* @throws \OCP\Files\InvalidPathException
960959
*/
961-
public function testSearchSubStoragesLimitOffset(int $offset, int $limit, array $expectedIds, array $ordering) {
960+
public function testSearchSubStoragesLimitOffset(int $offset, int $limit, array $expectedPaths, array $ordering) {
961+
if (!$ordering) {
962+
$ordering = [new SearchOrder(ISearchOrder::DIRECTION_ASCENDING, 'fileid')];
963+
}
964+
962965
$manager = $this->createMock(Manager::class);
963966
/**
964967
* @var \OC\Files\View | \PHPUnit\Framework\MockObject\MockObject $view
@@ -971,13 +974,15 @@ public function testSearchSubStoragesLimitOffset(int $offset, int $limit, array
971974
->method('getUser')
972975
->willReturn($this->user);
973976
$storage = $this->createMock(Storage::class);
974-
$storage->method('getId')->willReturn('');
975-
$cache = $this->getMockBuilder(Cache::class)->setConstructorArgs([$storage])->getMock();
976-
$subCache1 = $this->getMockBuilder(Cache::class)->setConstructorArgs([$storage])->getMock();
977+
$storage->method('getId')->willReturn('test::1');
978+
$cache = new Cache($storage);
977979
$subStorage1 = $this->createMock(Storage::class);
980+
$subStorage1->method('getId')->willReturn('test::2');
981+
$subCache1 = new Cache($subStorage1);
978982
$subMount1 = $this->getMockBuilder(MountPoint::class)->setConstructorArgs([null, ''])->getMock();
979-
$subCache2 = $this->getMockBuilder(Cache::class)->setConstructorArgs([$storage])->getMock();
980983
$subStorage2 = $this->createMock(Storage::class);
984+
$subStorage2->method('getId')->willReturn('test::3');
985+
$subCache2 = new Cache($subStorage2);
981986
$subMount2 = $this->getMockBuilder(MountPoint::class)->setConstructorArgs([null, ''])->getMock();
982987

983988
$mount = $this->createMock(IMountPoint::class);
@@ -990,7 +995,7 @@ public function testSearchSubStoragesLimitOffset(int $offset, int $limit, array
990995
->willReturn($subStorage1);
991996

992997
$subMount1->method('getMountPoint')
993-
->willReturn('/bar/foo/bar/');
998+
->willReturn('/bar/foo/sub1/');
994999

9951000
$storage->method('getCache')
9961001
->willReturn($cache);
@@ -1002,36 +1007,21 @@ public function testSearchSubStoragesLimitOffset(int $offset, int $limit, array
10021007
->willReturn($subStorage2);
10031008

10041009
$subMount2->method('getMountPoint')
1005-
->willReturn('/bar/foo/bar2/');
1010+
->willReturn('/bar/foo/sub2/');
10061011

10071012
$subStorage2->method('getCache')
10081013
->willReturn($subCache2);
10091014

1010-
$cache->method('searchQuery')
1011-
->willReturnCallback(function (ISearchQuery $query) {
1012-
return array_slice([
1013-
new CacheEntry(['fileid' => 10, 'path' => 'foo/qwerty', 'name' => 'qwerty', 'size' => 200, 'mtime' => 10, 'mimetype' => 'text/plain']),
1014-
new CacheEntry(['fileid' => 11, 'path' => 'foo/qwerty', 'name' => 'qwerty', 'size' => 200, 'mtime' => 20, 'mimetype' => 'text/plain']),
1015-
new CacheEntry(['fileid' => 12, 'path' => 'foo/qwerty', 'name' => 'qwerty', 'size' => 200, 'mtime' => 30, 'mimetype' => 'text/plain']),
1016-
new CacheEntry(['fileid' => 13, 'path' => 'foo/qwerty', 'name' => 'qwerty', 'size' => 200, 'mtime' => 40, 'mimetype' => 'text/plain']),
1017-
], $query->getOffset(), $query->getOffset() + $query->getLimit());
1018-
});
1015+
$cache->insert('foo/foo1', ['size' => 200, 'mtime' => 10, 'mimetype' => 'text/plain']);
1016+
$cache->insert('foo/foo2', ['size' => 200, 'mtime' => 20, 'mimetype' => 'text/plain']);
1017+
$cache->insert('foo/foo3', ['size' => 200, 'mtime' => 30, 'mimetype' => 'text/plain']);
1018+
$cache->insert('foo/foo4', ['size' => 200, 'mtime' => 40, 'mimetype' => 'text/plain']);
10191019

1020-
$subCache1->method('searchQuery')
1021-
->willReturnCallback(function (ISearchQuery $query) {
1022-
return array_slice([
1023-
new CacheEntry(['fileid' => 14, 'path' => 'foo/qwerty', 'name' => 'qwerty', 'size' => 300, 'mtime' => 15, 'mimetype' => 'text/plain']),
1024-
new CacheEntry(['fileid' => 15, 'path' => 'foo/qwerty', 'name' => 'qwerty', 'size' => 300, 'mtime' => 50, 'mimetype' => 'text/plain']),
1025-
], $query->getOffset(), $query->getOffset() + $query->getLimit());
1026-
});
1020+
$subCache1->insert('foo5', ['size' => 300, 'mtime' => 15, 'mimetype' => 'text/plain']);
1021+
$subCache1->insert('foo6', ['size' => 300, 'mtime' => 50, 'mimetype' => 'text/plain']);
10271022

1028-
$subCache2->method('searchQuery')
1029-
->willReturnCallback(function (ISearchQuery $query) {
1030-
return array_slice([
1031-
new CacheEntry(['fileid' => 16, 'path' => 'foo/qwerty', 'name' => 'qwerty', 'size' => 200, 'mtime' => 5, 'mimetype' => 'text/plain']),
1032-
new CacheEntry(['fileid' => 17, 'path' => 'foo/qwerty', 'name' => 'qwerty', 'size' => 200, 'mtime' => 60, 'mimetype' => 'text/plain']),
1033-
], $query->getOffset(), $query->getOffset() + $query->getLimit());
1034-
});
1023+
$subCache2->insert('foo7', ['size' => 200, 'mtime' => 5, 'mimetype' => 'text/plain']);
1024+
$subCache2->insert('foo8', ['size' => 200, 'mtime' => 60, 'mimetype' => 'text/plain']);
10351025

10361026
$root->method('getMountsIn')
10371027
->with('/bar/foo')
@@ -1041,14 +1031,16 @@ public function testSearchSubStoragesLimitOffset(int $offset, int $limit, array
10411031
->with('/bar/foo')
10421032
->willReturn($mount);
10431033

1044-
10451034
$node = new Folder($root, $view, '/bar/foo');
10461035
$comparison = new SearchComparison(ISearchComparison::COMPARE_LIKE, 'name', '%foo%');
10471036
$query = new SearchQuery($comparison, $limit, $offset, $ordering);
10481037
$result = $node->search($query);
1038+
$cache->clear();
1039+
$subCache1->clear();
1040+
$subCache2->clear();
10491041
$ids = array_map(function (Node $info) {
1050-
return $info->getId();
1042+
return $info->getPath();
10511043
}, $result);
1052-
$this->assertEquals($expectedIds, $ids);
1044+
$this->assertEquals($expectedPaths, $ids);
10531045
}
10541046
}

0 commit comments

Comments
 (0)