Skip to content

[stable21] Handle limit offset and sorting in files search #26257

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
MorrisJobke merged 10 commits into from
Mar 24, 2021
Merged

[stable21] Handle limit offset and sorting in files search #26257

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
680e21b
use "newer" node search api directly in unified search
icewind1991 Mar 18, 2021
d224573
unify handling of Folder::search methods into "new" query objects
icewind1991 Mar 18, 2021
2500d29
only require user to be set in a query that handles tags
icewind1991 Mar 18, 2021
423e158
handle limit and offset in folder file search
icewind1991 Mar 18, 2021
c867988
format
icewind1991 Mar 18, 2021
277c003
cleanup fileinfo creation
icewind1991 Mar 19, 2021
8551ee0
folder filtering in sql
icewind1991 Mar 19, 2021
d4381c0
handle ordering in folder search
icewind1991 Mar 19, 2021
2c565d6
pass order from unified search to file search
icewind1991 Mar 19, 2021
5a1ae2c
use node search api for legacy file search endpoint
icewind1991 Mar 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
cleanup fileinfo creation 
Signed-off-by: Robin Appelman <[email protected]>
  • Loading branch information
@icewind1991 @backportbot
icewind1991 authored and backportbot[bot] committed Mar 23, 2021
commit 277c0039f64ccfa32f0a20ea66e20adc36c17773
25 changes: 16 additions & 9 deletions lib/private/Files/Node/Folder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,14 @@
namespace OC\Files\Node;

use OC\DB\QueryBuilder\Literal;
use OC\Files\Mount\MountPoint;
use OC\Files\Search\SearchComparison;
use OC\Files\Search\SearchQuery;
use OC\Files\Storage\Wrapper\Jail;
use OC\Files\Storage\Storage;
use OCA\Files_Sharing\SharedStorage;
use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\Files\Cache\ICacheEntry;
use OCP\Files\Config\ICachedMountInfo;
use OCP\Files\FileInfo;
use OCP\Files\Mount\IMountPoint;
Expand Down Expand Up @@ -227,6 +230,8 @@ public function search($query) {
$query = $this->queryFromOperator(new SearchComparison(ISearchComparison::COMPARE_LIKE, 'name', '%' . $query . '%'));
}

// Limit+offset for queries without ordering
//
// assume a setup where the root mount matches 15 items,
// sub mount1 matches 7 items and mount2 matches 1 item
// a search with (0..10) returns 10 results from root with internal offset 0 and limit 10
Expand All @@ -240,6 +245,8 @@ public function search($query) {
// (we don't know how many results the previous sub-query has skipped with it's own offset)
// we instead discard the offset for the sub-queries and filter it afterwards and add the offset to limit.
// this is sub-optimal but shouldn't hurt to much since large offsets are uncommon in practice
//
// All of this is only possible for queries without ordering

$limitToHome = $query->limitToHome();
if ($limitToHome && count(explode('/', $this->path)) !== 3) {
Expand Down Expand Up @@ -277,10 +284,7 @@ public function search($query) {

foreach ($results as $result) {
if ($internalRootLength === 0 or substr($result['path'], 0, $internalRootLength) === $internalPath) {
$result['internalPath'] = $result['path'];
$result['path'] = substr($result['path'], $internalRootLength);
$result['storage'] = $storage;
$files[] = new \OC\Files\FileInfo($this->path . '/' . $result['path'], $storage, $result['internalPath'], $result, $mount);
$files[] = $this->cacheEntryToFileInfo($mount, '', $internalPath, $result);
}
}

Expand Down Expand Up @@ -312,11 +316,7 @@ public function search($query) {
$subQueryLimit -= $count;

foreach ($results as $result) {
$result['internalPath'] = $result['path'];
$result['path'] = $relativeMountPoint . $result['path'];
$result['storage'] = $storage;
$files[] = new \OC\Files\FileInfo($this->path . '/' . $result['path'], $storage,
$result['internalPath'], $result, $mount);
$files[] = $this->cacheEntryToFileInfo($mount, $relativeMountPoint, '', $result);
}
}
}
Expand All @@ -327,6 +327,13 @@ public function search($query) {
}, $files);
}

private function cacheEntryToFileInfo(IMountPoint $mount, string $appendRoot, string $trimRoot, ICacheEntry $cacheEntry): FileInfo {
$trimLength = strlen($trimRoot);
$cacheEntry['internalPath'] = $cacheEntry['path'];
$cacheEntry['path'] = $appendRoot . substr($cacheEntry['path'], $trimLength);
return new \OC\Files\FileInfo($this->path . '/' . $cacheEntry['path'], $mount->getStorage(), $cacheEntry['internalPath'], $cacheEntry, $mount);
}

/**
* search for files by mimetype
*
Expand Down
90 changes: 30 additions & 60 deletions tests/lib/Files/Node/FolderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,38 +290,31 @@ public function testSearch() {
$root = $this->getMockBuilder(Root::class)
->setConstructorArgs([$manager, $view, $this->user, $this->userMountCache, $this->logger, $this->userManager])
->getMock();
$root->expects($this->any())
->method('getUser')
$root->method('getUser')
->willReturn($this->user);
$storage = $this->createMock(Storage::class);
$storage->method('getId')->willReturn('');
$cache = $this->getMockBuilder(Cache::class)->setConstructorArgs([$storage])->getMock();

$storage->expects($this->once())
->method('getCache')
$storage->method('getCache')
->willReturn($cache);

$mount = $this->createMock(IMountPoint::class);
$mount->expects($this->once())
->method('getStorage')
$mount->method('getStorage')
->willReturn($storage);
$mount->expects($this->once())
->method('getInternalPath')
$mount->method('getInternalPath')
->willReturn('foo');

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

$root->expects($this->once())
->method('getMountsIn')
$root->method('getMountsIn')
->with('/bar/foo')
->willReturn([]);

$root->expects($this->once())
->method('getMount')
$root->method('getMount')
->with('/bar/foo')
->willReturn($mount);

Expand Down Expand Up @@ -350,31 +343,25 @@ public function testSearchInRoot() {
$cache = $this->getMockBuilder(Cache::class)->setConstructorArgs([$storage])->getMock();

$mount = $this->createMock(IMountPoint::class);
$mount->expects($this->once())
->method('getStorage')
$mount->method('getStorage')
->willReturn($storage);
$mount->expects($this->once())
->method('getInternalPath')
$mount->method('getInternalPath')
->willReturn('files');

$storage->expects($this->once())
->method('getCache')
$storage->method('getCache')
->willReturn($cache);

$cache->expects($this->once())
->method('searchQuery')
$cache->method('searchQuery')
->willReturn([
new CacheEntry(['fileid' => 3, 'path' => 'files/foo', 'name' => 'qwerty', 'size' => 200, 'mtime' => 55, 'mimetype' => 'text/plain']),
new CacheEntry(['fileid' => 3, 'path' => 'files_trashbin/foo2.d12345', 'name' => 'foo2.d12345', 'size' => 200, 'mtime' => 55, 'mimetype' => 'text/plain']),
]);

$root->expects($this->once())
->method('getMountsIn')
$root->method('getMountsIn')
->with('')
->willReturn([]);

$root->expects($this->once())
->method('getMount')
$root->method('getMount')
->with('')
->willReturn($mount);

Expand All @@ -392,38 +379,31 @@ public function testSearchInStorageRoot() {
$root = $this->getMockBuilder(Root::class)
->setConstructorArgs([$manager, $view, $this->user, $this->userMountCache, $this->logger, $this->userManager])
->getMock();
$root->expects($this->any())
->method('getUser')
$root->method('getUser')
->willReturn($this->user);
$storage = $this->createMock(Storage::class);
$storage->method('getId')->willReturn('');
$cache = $this->getMockBuilder(Cache::class)->setConstructorArgs([$storage])->getMock();

$mount = $this->createMock(IMountPoint::class);
$mount->expects($this->once())
->method('getStorage')
$mount->method('getStorage')
->willReturn($storage);
$mount->expects($this->once())
->method('getInternalPath')
$mount->method('getInternalPath')
->willReturn('');

$storage->expects($this->once())
->method('getCache')
$storage->method('getCache')
->willReturn($cache);

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

$root->expects($this->once())
->method('getMountsIn')
$root->method('getMountsIn')
->with('/bar')
->willReturn([]);

$root->expects($this->once())
->method('getMount')
$root->method('getMount')
->with('/bar')
->willReturn($mount);

Expand Down Expand Up @@ -453,48 +433,38 @@ public function testSearchSubStorages() {
$subMount = $this->getMockBuilder(MountPoint::class)->setConstructorArgs([null, ''])->getMock();

$mount = $this->createMock(IMountPoint::class);
$mount->expects($this->once())
->method('getStorage')
$mount->method('getStorage')
->willReturn($storage);
$mount->expects($this->once())
->method('getInternalPath')
$mount->method('getInternalPath')
->willReturn('foo');

$subMount->expects($this->once())
->method('getStorage')
$subMount->method('getStorage')
->willReturn($subStorage);

$subMount->expects($this->once())
->method('getMountPoint')
$subMount->method('getMountPoint')
->willReturn('/bar/foo/bar/');

$storage->expects($this->once())
->method('getCache')
$storage->method('getCache')
->willReturn($cache);

$subStorage->expects($this->once())
->method('getCache')
$subStorage->method('getCache')
->willReturn($subCache);

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

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

$root->expects($this->once())
->method('getMountsIn')
$root->method('getMountsIn')
->with('/bar/foo')
->willReturn([$subMount]);

$root->expects($this->once())
->method('getMount')
$root->method('getMount')
->with('/bar/foo')
->willReturn($mount);

Expand Down