Skip to content

Commit 139fac9

Browse files
icewind1991icewind1991
committed
update tests and fix some edge cases around new search
Signed-off-by: Robin Appelman <robin@icewind.nl>
1 parent 056996d commit 139fac9

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
@@ -31,11 +31,9 @@
3131

3232
namespace OC\Files\Node;
3333

34-
use OC\DB\QueryBuilder\Literal;
3534
use OC\Files\Cache\QuerySearchHelper;
3635
use OC\Files\Search\SearchBinaryOperator;
3736
use OC\Files\Cache\Wrapper\CacheJail;
38-
use OC\Files\Search\SearchBinaryOperator;
3937
use OC\Files\Search\SearchComparison;
4038
use OC\Files\Search\SearchOrder;
4139
use OC\Files\Search\SearchQuery;
@@ -251,7 +249,12 @@ public function search($query) {
251249
// collect all caches for this folder, indexed by their mountpoint relative to this folder
252250
// and save the mount which is needed later to construct the FileInfo objects
253251

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

257260
if (!$limitToHome) {
@@ -271,13 +274,27 @@ public function search($query) {
271274
$resultsPerCache = $searchHelper->searchInCaches($query, $caches);
272275

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

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

@@ -944,18 +943,18 @@ public function testRecentJail() {
944943

945944
public function offsetLimitProvider() {
946945
return [
947-
[0, 10, [10, 11, 12, 13, 14, 15, 16, 17], []],
948-
[0, 5, [10, 11, 12, 13, 14], []],
949-
[0, 2, [10, 11], []],
950-
[3, 2, [13, 14], []],
951-
[3, 5, [13, 14, 15, 16, 17], []],
952-
[5, 2, [15, 16], []],
953-
[6, 2, [16, 17], []],
954-
[7, 2, [17], []],
946+
[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'], []],
947+
[0, 5, ['/bar/foo/foo1', '/bar/foo/foo2', '/bar/foo/foo3', '/bar/foo/foo4', '/bar/foo/sub1/foo5'], []],
948+
[0, 2, ['/bar/foo/foo1', '/bar/foo/foo2'], []],
949+
[3, 2, ['/bar/foo/foo4', '/bar/foo/sub1/foo5'], []],
950+
[3, 5, ['/bar/foo/foo4', '/bar/foo/sub1/foo5', '/bar/foo/sub1/foo6', '/bar/foo/sub2/foo7', '/bar/foo/sub2/foo8'], []],
951+
[5, 2, ['/bar/foo/sub1/foo6', '/bar/foo/sub2/foo7'], []],
952+
[6, 2, ['/bar/foo/sub2/foo7', '/bar/foo/sub2/foo8'], []],
953+
[7, 2, ['/bar/foo/sub2/foo8'], []],
955954
[10, 2, [], []],
956-
[0, 5, [16, 10, 14, 11, 12], [new SearchOrder(ISearchOrder::DIRECTION_ASCENDING, 'mtime')]],
957-
[3, 2, [11, 12], [new SearchOrder(ISearchOrder::DIRECTION_ASCENDING, 'mtime')]],
958-
[0, 5, [14, 15, 16, 10, 11], [
955+
[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')]],
956+
[3, 2, ['/bar/foo/foo2', '/bar/foo/foo3'], [new SearchOrder(ISearchOrder::DIRECTION_ASCENDING, 'mtime')]],
957+
[0, 5, ['/bar/foo/sub1/foo5', '/bar/foo/sub1/foo6', '/bar/foo/sub2/foo7', '/bar/foo/foo1', '/bar/foo/foo2'], [
959958
new SearchOrder(ISearchOrder::DIRECTION_DESCENDING, 'size'),
960959
new SearchOrder(ISearchOrder::DIRECTION_ASCENDING, 'mtime')
961960
]],
@@ -966,12 +965,16 @@ public function offsetLimitProvider() {
966965
* @dataProvider offsetLimitProvider
967966
* @param int $offset
968967
* @param int $limit
969-
* @param int[] $expectedIds
968+
* @param string[] $expectedPaths
970969
* @param ISearchOrder[] $ordering
971970
* @throws NotFoundException
972971
* @throws \OCP\Files\InvalidPathException
973972
*/
974-
public function testSearchSubStoragesLimitOffset(int $offset, int $limit, array $expectedIds, array $ordering) {
973+
public function testSearchSubStoragesLimitOffset(int $offset, int $limit, array $expectedPaths, array $ordering) {
974+
if (!$ordering) {
975+
$ordering = [new SearchOrder(ISearchOrder::DIRECTION_ASCENDING, 'fileid')];
976+
}
977+
975978
$manager = $this->createMock(Manager::class);
976979
/**
977980
* @var \OC\Files\View | \PHPUnit\Framework\MockObject\MockObject $view
@@ -984,13 +987,15 @@ public function testSearchSubStoragesLimitOffset(int $offset, int $limit, array
984987
->method('getUser')
985988
->willReturn($this->user);
986989
$storage = $this->createMock(Storage::class);
987-
$storage->method('getId')->willReturn('');
988-
$cache = $this->getMockBuilder(Cache::class)->setConstructorArgs([$storage])->getMock();
989-
$subCache1 = $this->getMockBuilder(Cache::class)->setConstructorArgs([$storage])->getMock();
990+
$storage->method('getId')->willReturn('test::1');
991+
$cache = new Cache($storage);
990992
$subStorage1 = $this->createMock(Storage::class);
993+
$subStorage1->method('getId')->willReturn('test::2');
994+
$subCache1 = new Cache($subStorage1);
991995
$subMount1 = $this->getMockBuilder(MountPoint::class)->setConstructorArgs([null, ''])->getMock();
992-
$subCache2 = $this->getMockBuilder(Cache::class)->setConstructorArgs([$storage])->getMock();
993996
$subStorage2 = $this->createMock(Storage::class);
997+
$subStorage2->method('getId')->willReturn('test::3');
998+
$subCache2 = new Cache($subStorage2);
994999
$subMount2 = $this->getMockBuilder(MountPoint::class)->setConstructorArgs([null, ''])->getMock();
9951000

9961001
$mount = $this->createMock(IMountPoint::class);
@@ -1003,7 +1008,7 @@ public function testSearchSubStoragesLimitOffset(int $offset, int $limit, array
10031008
->willReturn($subStorage1);
10041009

10051010
$subMount1->method('getMountPoint')
1006-
->willReturn('/bar/foo/bar/');
1011+
->willReturn('/bar/foo/sub1/');
10071012

10081013
$storage->method('getCache')
10091014
->willReturn($cache);
@@ -1015,36 +1020,21 @@ public function testSearchSubStoragesLimitOffset(int $offset, int $limit, array
10151020
->willReturn($subStorage2);
10161021

10171022
$subMount2->method('getMountPoint')
1018-
->willReturn('/bar/foo/bar2/');
1023+
->willReturn('/bar/foo/sub2/');
10191024

10201025
$subStorage2->method('getCache')
10211026
->willReturn($subCache2);
10221027

1023-
$cache->method('searchQuery')
1024-
->willReturnCallback(function (ISearchQuery $query) {
1025-
return array_slice([
1026-
new CacheEntry(['fileid' => 10, 'path' => 'foo/qwerty', 'name' => 'qwerty', 'size' => 200, 'mtime' => 10, 'mimetype' => 'text/plain']),
1027-
new CacheEntry(['fileid' => 11, 'path' => 'foo/qwerty', 'name' => 'qwerty', 'size' => 200, 'mtime' => 20, 'mimetype' => 'text/plain']),
1028-
new CacheEntry(['fileid' => 12, 'path' => 'foo/qwerty', 'name' => 'qwerty', 'size' => 200, 'mtime' => 30, 'mimetype' => 'text/plain']),
1029-
new CacheEntry(['fileid' => 13, 'path' => 'foo/qwerty', 'name' => 'qwerty', 'size' => 200, 'mtime' => 40, 'mimetype' => 'text/plain']),
1030-
], $query->getOffset(), $query->getOffset() + $query->getLimit());
1031-
});
1028+
$cache->insert('foo/foo1', ['size' => 200, 'mtime' => 10, 'mimetype' => 'text/plain']);
1029+
$cache->insert('foo/foo2', ['size' => 200, 'mtime' => 20, 'mimetype' => 'text/plain']);
1030+
$cache->insert('foo/foo3', ['size' => 200, 'mtime' => 30, 'mimetype' => 'text/plain']);
1031+
$cache->insert('foo/foo4', ['size' => 200, 'mtime' => 40, 'mimetype' => 'text/plain']);
10321032

1033-
$subCache1->method('searchQuery')
1034-
->willReturnCallback(function (ISearchQuery $query) {
1035-
return array_slice([
1036-
new CacheEntry(['fileid' => 14, 'path' => 'foo/qwerty', 'name' => 'qwerty', 'size' => 300, 'mtime' => 15, 'mimetype' => 'text/plain']),
1037-
new CacheEntry(['fileid' => 15, 'path' => 'foo/qwerty', 'name' => 'qwerty', 'size' => 300, 'mtime' => 50, 'mimetype' => 'text/plain']),
1038-
], $query->getOffset(), $query->getOffset() + $query->getLimit());
1039-
});
1033+
$subCache1->insert('foo5', ['size' => 300, 'mtime' => 15, 'mimetype' => 'text/plain']);
1034+
$subCache1->insert('foo6', ['size' => 300, 'mtime' => 50, 'mimetype' => 'text/plain']);
10401035

1041-
$subCache2->method('searchQuery')
1042-
->willReturnCallback(function (ISearchQuery $query) {
1043-
return array_slice([
1044-
new CacheEntry(['fileid' => 16, 'path' => 'foo/qwerty', 'name' => 'qwerty', 'size' => 200, 'mtime' => 5, 'mimetype' => 'text/plain']),
1045-
new CacheEntry(['fileid' => 17, 'path' => 'foo/qwerty', 'name' => 'qwerty', 'size' => 200, 'mtime' => 60, 'mimetype' => 'text/plain']),
1046-
], $query->getOffset(), $query->getOffset() + $query->getLimit());
1047-
});
1036+
$subCache2->insert('foo7', ['size' => 200, 'mtime' => 5, 'mimetype' => 'text/plain']);
1037+
$subCache2->insert('foo8', ['size' => 200, 'mtime' => 60, 'mimetype' => 'text/plain']);
10481038

10491039
$root->method('getMountsIn')
10501040
->with('/bar/foo')
@@ -1054,14 +1044,16 @@ public function testSearchSubStoragesLimitOffset(int $offset, int $limit, array
10541044
->with('/bar/foo')
10551045
->willReturn($mount);
10561046

1057-
10581047
$node = new Folder($root, $view, '/bar/foo');
10591048
$comparison = new SearchComparison(ISearchComparison::COMPARE_LIKE, 'name', '%foo%');
10601049
$query = new SearchQuery($comparison, $limit, $offset, $ordering);
10611050
$result = $node->search($query);
1051+
$cache->clear();
1052+
$subCache1->clear();
1053+
$subCache2->clear();
10621054
$ids = array_map(function (Node $info) {
1063-
return $info->getId();
1055+
return $info->getPath();
10641056
}, $result);
1065-
$this->assertEquals($expectedIds, $ids);
1057+
$this->assertEquals($expectedPaths, $ids);
10661058
}
10671059
}

0 commit comments

Comments
 (0)