Skip to content

Commit

Permalink
update tests and fix some edge cases around new search
Browse files Browse the repository at this point in the history 
Signed-off-by: Robin Appelman <robin@icewind.nl>
  • Loading branch information
@icewind1991
icewind1991 committed May 7, 2021
1 parent d334e81 commit 0f50fe1
Show file tree
Hide file tree
Showing 3 changed files with 96 additions and 83 deletions.
6 changes: 5 additions & 1 deletion lib/private/Files/Cache/QuerySearchHelper.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,11 @@ private function getParameterForValue(IQueryBuilder $builder, $value) {
*/
public function addSearchOrdersToQuery(IQueryBuilder $query, array $orders) {
foreach ($orders as $order) {
$query->addOrderBy($order->getField(), $order->getDirection());
$field = $order->getField();
if ($field === 'fileid') {
$field = 'file.fileid';
}
$query->addOrderBy($field, $order->getDirection());
}
}

Expand Down
27 changes: 22 additions & 5 deletions lib/private/Files/Node/Folder.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
use OC\DB\QueryBuilder\Literal;
use OC\Files\Cache\QuerySearchHelper;
use OC\Files\Cache\Wrapper\CacheJail;
use OC\Files\Search\SearchBinaryOperator;
use OC\Files\Search\SearchComparison;
use OC\Files\Search\SearchQuery;
use OC\Files\Storage\Wrapper\Jail;
Expand All @@ -47,7 +46,6 @@
use OCP\Files\Mount\IMountPoint;
use OCP\Files\NotFoundException;
use OCP\Files\NotPermittedException;
use OCP\Files\Search\ISearchBinaryOperator;
use OCP\Files\Search\ISearchComparison;
use OCP\Files\Search\ISearchOperator;
use OCP\Files\Search\ISearchQuery;
Expand Down Expand Up @@ -252,7 +250,12 @@ public function search($query) {
// collect all caches for this folder, indexed by their mountpoint relative to this folder
// and save the mount which is needed later to construct the FileInfo objects

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

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

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

// since results were returned per-cache, they are no longer fully sorted
$order = $query->getOrder();
if ($order) {
usort($files, function (FileInfo $a, FileInfo $b) use ($order) {
foreach ($order as $orderField) {
$cmp = $orderField->sortFileInfo($a, $b);
if ($cmp !== 0) {
return $cmp;
}
}
return 0;
});
}

return array_map(function (FileInfo $file) {
return $this->createNode($file->getPath(), $file);
}, $files);
Expand Down
146 changes: 69 additions & 77 deletions tests/lib/Files/Node/FolderTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,11 @@
use OC\Files\Storage\Temporary;
use OC\Files\Storage\Wrapper\Jail;
use OC\Files\View;
use OCP\Files\Cache\ICacheEntry;
use OCP\Files\Mount\IMountPoint;
use OCP\Files\NotFoundException;
use OCP\Files\Search\ISearchComparison;
use OCP\Files\Search\ISearchOrder;
use OCP\Files\Search\ISearchQuery;
use OCP\Files\Storage;

/**
Expand Down Expand Up @@ -294,9 +294,10 @@ public function testSearch() {
->getMock();
$root->method('getUser')
->willReturn($this->user);
$storage = $this->createMock(Storage::class);
$storage->method('getId')->willReturn('');
$cache = $this->getMockBuilder(Cache::class)->setConstructorArgs([$storage])->getMock();
/** @var Storage\IStorage $storage */
$storage = $this->createMock(Storage\IStorage::class);
$storage->method('getId')->willReturn('test::1');
$cache = new Cache($storage);

$storage->method('getCache')
->willReturn($cache);
Expand All @@ -307,10 +308,8 @@ public function testSearch() {
$mount->method('getInternalPath')
->willReturn('foo');

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

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

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

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

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

$root->method('getMountsIn')
->with('')
Expand All @@ -366,7 +364,8 @@ public function testSearchInRoot() {
->with('')
->willReturn($mount);

$result = $root->search('qw');
$result = $root->search('foo');
$cache->clear();
$this->assertEquals(1, count($result));
$this->assertEquals('/foo', $result[0]->getPath());
}
Expand All @@ -383,8 +382,8 @@ public function testSearchInStorageRoot() {
$root->method('getUser')
->willReturn($this->user);
$storage = $this->createMock(Storage::class);
$storage->method('getId')->willReturn('');
$cache = $this->getMockBuilder(Cache::class)->setConstructorArgs([$storage])->getMock();
$storage->method('getId')->willReturn('test::1');
$cache = new Cache($storage);

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

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


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

$node = new Folder($root, $view, '/bar');
$result = $node->search('qw');
$cache->clear();
$this->assertEquals(1, count($result));
$this->assertEquals('/bar/foo/qwerty', $result[0]->getPath());
}
Expand All @@ -427,10 +426,11 @@ public function testSearchSubStorages() {
->method('getUser')
->willReturn($this->user);
$storage = $this->createMock(Storage::class);
$storage->method('getId')->willReturn('');
$cache = $this->getMockBuilder(Cache::class)->setConstructorArgs([$storage])->getMock();
$subCache = $this->getMockBuilder(Cache::class)->setConstructorArgs([$storage])->getMock();
$storage->method('getId')->willReturn('test::1');
$cache = new Cache($storage);
$subStorage = $this->createMock(Storage::class);
$subStorage->method('getId')->willReturn('test::2');
$subCache = new Cache($subStorage);
$subMount = $this->getMockBuilder(MountPoint::class)->setConstructorArgs([null, ''])->getMock();

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

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

$subCache->insert('asd', ['size' => 200, 'mtime' => 55, 'mimetype' => ICacheEntry::DIRECTORY_MIMETYPE]);
$subCache->insert('asd/qwerty', ['size' => 200, 'mtime' => 55, 'mimetype' => 'text/plain']);

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

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

$node = new Folder($root, $view, '/bar/foo');
$result = $node->search('qw');
$cache->clear();
$subCache->clear();
$this->assertEquals(2, count($result));
}

Expand Down Expand Up @@ -931,18 +930,18 @@ public function testRecentJail() {

public function offsetLimitProvider() {
return [
[0, 10, [10, 11, 12, 13, 14, 15, 16, 17], []],
[0, 5, [10, 11, 12, 13, 14], []],
[0, 2, [10, 11], []],
[3, 2, [13, 14], []],
[3, 5, [13, 14, 15, 16, 17], []],
[5, 2, [15, 16], []],
[6, 2, [16, 17], []],
[7, 2, [17], []],
[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'], []],
[0, 5, ['/bar/foo/foo1', '/bar/foo/foo2', '/bar/foo/foo3', '/bar/foo/foo4', '/bar/foo/sub1/foo5'], []],
[0, 2, ['/bar/foo/foo1', '/bar/foo/foo2'], []],
[3, 2, ['/bar/foo/foo4', '/bar/foo/sub1/foo5'], []],
[3, 5, ['/bar/foo/foo4', '/bar/foo/sub1/foo5', '/bar/foo/sub1/foo6', '/bar/foo/sub2/foo7', '/bar/foo/sub2/foo8'], []],
[5, 2, ['/bar/foo/sub1/foo6', '/bar/foo/sub2/foo7'], []],
[6, 2, ['/bar/foo/sub2/foo7', '/bar/foo/sub2/foo8'], []],
[7, 2, ['/bar/foo/sub2/foo8'], []],
[10, 2, [], []],
[0, 5, [16, 10, 14, 11, 12], [new SearchOrder(ISearchOrder::DIRECTION_ASCENDING, 'mtime')]],
[3, 2, [11, 12], [new SearchOrder(ISearchOrder::DIRECTION_ASCENDING, 'mtime')]],
[0, 5, [14, 15, 16, 10, 11], [
[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')]],
[3, 2, ['/bar/foo/foo2', '/bar/foo/foo3'], [new SearchOrder(ISearchOrder::DIRECTION_ASCENDING, 'mtime')]],
[0, 5, ['/bar/foo/sub1/foo5', '/bar/foo/sub1/foo6', '/bar/foo/sub2/foo7', '/bar/foo/foo1', '/bar/foo/foo2'], [
new SearchOrder(ISearchOrder::DIRECTION_DESCENDING, 'size'),
new SearchOrder(ISearchOrder::DIRECTION_ASCENDING, 'mtime')
]],
Expand All @@ -953,12 +952,16 @@ public function offsetLimitProvider() {
* @dataProvider offsetLimitProvider
* @param int $offset
* @param int $limit
* @param int[] $expectedIds
* @param string[] $expectedPaths
* @param ISearchOrder[] $ordering
* @throws NotFoundException
* @throws \OCP\Files\InvalidPathException
*/
public function testSearchSubStoragesLimitOffset(int $offset, int $limit, array $expectedIds, array $ordering) {
public function testSearchSubStoragesLimitOffset(int $offset, int $limit, array $expectedPaths, array $ordering) {
if (!$ordering) {
$ordering = [new SearchOrder(ISearchOrder::DIRECTION_ASCENDING, 'fileid')];
}

$manager = $this->createMock(Manager::class);
/**
* @var \OC\Files\View | \PHPUnit\Framework\MockObject\MockObject $view
Expand All @@ -971,13 +974,15 @@ public function testSearchSubStoragesLimitOffset(int $offset, int $limit, array
->method('getUser')
->willReturn($this->user);
$storage = $this->createMock(Storage::class);
$storage->method('getId')->willReturn('');
$cache = $this->getMockBuilder(Cache::class)->setConstructorArgs([$storage])->getMock();
$subCache1 = $this->getMockBuilder(Cache::class)->setConstructorArgs([$storage])->getMock();
$storage->method('getId')->willReturn('test::1');
$cache = new Cache($storage);
$subStorage1 = $this->createMock(Storage::class);
$subStorage1->method('getId')->willReturn('test::2');
$subCache1 = new Cache($subStorage1);
$subMount1 = $this->getMockBuilder(MountPoint::class)->setConstructorArgs([null, ''])->getMock();
$subCache2 = $this->getMockBuilder(Cache::class)->setConstructorArgs([$storage])->getMock();
$subStorage2 = $this->createMock(Storage::class);
$subStorage2->method('getId')->willReturn('test::3');
$subCache2 = new Cache($subStorage2);
$subMount2 = $this->getMockBuilder(MountPoint::class)->setConstructorArgs([null, ''])->getMock();

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

$subMount1->method('getMountPoint')
->willReturn('/bar/foo/bar/');
->willReturn('/bar/foo/sub1/');

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

$subMount2->method('getMountPoint')
->willReturn('/bar/foo/bar2/');
->willReturn('/bar/foo/sub2/');

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

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

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

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

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


$node = new Folder($root, $view, '/bar/foo');
$comparison = new SearchComparison(ISearchComparison::COMPARE_LIKE, 'name', '%foo%');
$query = new SearchQuery($comparison, $limit, $offset, $ordering);
$result = $node->search($query);
$cache->clear();
$subCache1->clear();
$subCache2->clear();
$ids = array_map(function (Node $info) {
return $info->getId();
return $info->getPath();
}, $result);
$this->assertEquals($expectedIds, $ids);
$this->assertEquals($expectedPaths, $ids);
}
}

0 comments on commit 0f50fe1

Please sign in to comment.