Skip to content

Allow filtering the search results to the users home storage #17941

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
rullzer merged 3 commits into from
Dec 5, 2019
Merged

Allow filtering the search results to the users home storage #17941

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c62637d
Allow filtering the search results to the users home storage
icewind1991 Nov 8, 2019
f9e5b33
remove old 'owner' filter
icewind1991 Dec 3, 2019
bf50342
add @since to limitToHome
icewind1991 Dec 3, 2019
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
82 changes: 75 additions & 7 deletions apps/dav/lib/Files/FileSearchBackend.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,14 +119,14 @@ public function getPropertyDefinitionsForScope($href, $path) {
new SearchPropertyDefinition(FilesPlugin::SIZE_PROPERTYNAME, true, true, true, SearchPropertyDefinition::DATATYPE_NONNEGATIVE_INTEGER),
new SearchPropertyDefinition(TagsPlugin::FAVORITE_PROPERTYNAME, true, true, true, SearchPropertyDefinition::DATATYPE_BOOLEAN),
new SearchPropertyDefinition(FilesPlugin::INTERNAL_FILEID_PROPERTYNAME, true, true, false, SearchPropertyDefinition::DATATYPE_NONNEGATIVE_INTEGER),
new SearchPropertyDefinition(FilesPlugin::OWNER_ID_PROPERTYNAME, true, true, false),

// select only properties
new SearchPropertyDefinition('{DAV:}resourcetype', false, true, false),
new SearchPropertyDefinition('{DAV:}getcontentlength', false, true, false),
new SearchPropertyDefinition(FilesPlugin::CHECKSUMS_PROPERTYNAME, false, true, false),
new SearchPropertyDefinition(FilesPlugin::PERMISSIONS_PROPERTYNAME, false, true, false),
new SearchPropertyDefinition(FilesPlugin::GETETAG_PROPERTYNAME, false, true, false),
new SearchPropertyDefinition(FilesPlugin::OWNER_ID_PROPERTYNAME, false, true, false),
new SearchPropertyDefinition(FilesPlugin::OWNER_DISPLAY_NAME_PROPERTYNAME, false, true, false),
new SearchPropertyDefinition(FilesPlugin::DATA_FINGERPRINT_PROPERTYNAME, false, true, false),
new SearchPropertyDefinition(FilesPlugin::HAS_PREVIEW_PROPERTYNAME, false, true, false, SearchPropertyDefinition::DATATYPE_BOOLEAN),
Expand Down Expand Up @@ -169,10 +169,12 @@ public function search(Query $search) {
return new SearchResult($davNode, $path);
}, $results);

// Sort again, since the result from multiple storages is appended and not sorted
usort($nodes, function (SearchResult $a, SearchResult $b) use ($search) {
return $this->sort($a, $b, $search->orderBy);
});
if (!$query->limitToHome()) {
// Sort again, since the result from multiple storages is appended and not sorted
usort($nodes, function (SearchResult $a, SearchResult $b) use ($search) {
return $this->sort($a, $b, $search->orderBy);
});
}

// If a limit is provided use only return that number of files
if ($search->limit->maxResults !== 0) {
Expand Down Expand Up @@ -267,11 +269,29 @@ private function getHrefForNode(Node $node) {
* @param Query $query
* @return ISearchQuery
*/
private function transformQuery(Query $query) {
private function transformQuery(Query $query): ISearchQuery {
// TODO offset
$limit = $query->limit;
$orders = array_map([$this, 'mapSearchOrder'], $query->orderBy);
return new SearchQuery($this->transformSearchOperation($query->where), (int)$limit->maxResults, 0, $orders, $this->user);

$limitHome = false;
$ownerProp = $this->extractWhereValue($query->where, FilesPlugin::OWNER_ID_PROPERTYNAME, Operator::OPERATION_EQUAL);
if ($ownerProp !== null) {
if ($ownerProp === $this->user->getUID()) {
$limitHome = true;
} else {
throw new \InvalidArgumentException("Invalid search value for '{http://owncloud.org/ns}owner-id', only the current user id is allowed");
}
}

return new SearchQuery(
$this->transformSearchOperation($query->where),
(int)$limit->maxResults,
0,
$orders,
$this->user,
$limitHome
);
}

/**
Expand Down Expand Up @@ -360,4 +380,52 @@ private function castValue(SearchPropertyDefinition $property, $value) {
return $value;
}
}

/**
* Get a specific property from the were clause
*/
private function extractWhereValue(Operator &$operator, string $propertyName, string $comparison, bool $acceptableLocation = true): ?string {
switch ($operator->type) {
case Operator::OPERATION_AND:
case Operator::OPERATION_OR:
case Operator::OPERATION_NOT:
foreach ($operator->arguments as &$argument) {
$value = $this->extractWhereValue($argument, $propertyName, $comparison, $acceptableLocation && $operator->type === Operator::OPERATION_AND);
if ($value !== null) {
return $value;
}
}
return null;
case Operator::OPERATION_EQUAL:
case Operator::OPERATION_GREATER_OR_EQUAL_THAN:
case Operator::OPERATION_GREATER_THAN:
case Operator::OPERATION_LESS_OR_EQUAL_THAN:
case Operator::OPERATION_LESS_THAN:
case Operator::OPERATION_IS_LIKE:
if ($operator->arguments[0]->name === $propertyName) {
if ($operator->type === $comparison) {
if ($acceptableLocation) {
if ($operator->arguments[1] instanceof Literal) {
$value = $operator->arguments[1]->value;

// to remove the comparison from the query, we replace it with an empty AND
$operator = new Operator(Operator::OPERATION_AND);
Copy link
Contributor

@kesselb kesselb Nov 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan. Could you consider to keep it? Sure we need owner as property at mapPropertyNameToColumn and still need to move $comparison->getField() === 'owner' before validateComparison. I think this is less hacky ;)

Copy link
Member Author

@icewind1991 icewind1991 Dec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My original plan was something like that but you'll still need to remove the comparison from the query since there is no column related to the owner field, this way the logic is confirmed to a single place

Copy link
Contributor

@kesselb kesselb Dec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me 👍 I see your point keeping the logic together. searchComparisonToDBExpr is more or less a generic method and adding "do nothing if owner field" there is bad. The fake operator is also bad. Guess it does not matter which bad we pick ;)


return $value;
} else {
throw new \InvalidArgumentException("searching by '$propertyName' is only allowed with a literal value");
}
} else{
throw new \InvalidArgumentException("searching by '$propertyName' is not allowed inside a '{DAV:}or' or '{DAV:}not'");
}
} else {
throw new \InvalidArgumentException("searching by '$propertyName' is only allowed inside a '$comparison'");
}
} else {
return null;
}
default:
return null;
}
}
}
79 changes: 79 additions & 0 deletions apps/dav/tests/unit/Files/FileSearchBackendTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@
use OCP\Files\FileInfo;
use OCP\Files\Folder;
use OCP\Files\IRootFolder;
use OCP\Files\Search\ISearchBinaryOperator;
use OCP\Files\Search\ISearchComparison;
use OCP\Files\Search\ISearchQuery;
use OCP\IUser;
use OCP\Share\IManager;
use SearchDAV\Backend\SearchPropertyDefinition;
Expand Down Expand Up @@ -308,4 +310,81 @@ public function testSearchNonFolder() {
$query = $this->getBasicQuery(\SearchDAV\Query\Operator::OPERATION_EQUAL, '{DAV:}displayname', 'foo');
$this->search->search($query);
}

public function testSearchLimitOwnerBasic() {
$this->tree->expects($this->any())
->method('getNodeForPath')
->willReturn($this->davFolder);

/** @var ISearchQuery|null $receivedQuery */
$receivedQuery = null;
$this->searchFolder
->method('search')
->will($this->returnCallback(function ($query) use (&$receivedQuery) {
$receivedQuery = $query;
return [
new \OC\Files\Node\Folder($this->rootFolder, $this->view, '/test/path')
];
}));

$query = $this->getBasicQuery(\SearchDAV\Query\Operator::OPERATION_EQUAL, FilesPlugin::OWNER_ID_PROPERTYNAME, $this->user->getUID());
$this->search->search($query);

$this->assertNotNull($receivedQuery);
$this->assertTrue($receivedQuery->limitToHome());

/** @var ISearchBinaryOperator $operator */
$operator = $receivedQuery->getSearchOperation();
$this->assertInstanceOf(ISearchBinaryOperator::class, $operator);
$this->assertEquals(ISearchBinaryOperator::OPERATOR_AND, $operator->getType());
$this->assertEmpty($operator->getArguments());
}

public function testSearchLimitOwnerNested() {
$this->tree->expects($this->any())
->method('getNodeForPath')
->willReturn($this->davFolder);

/** @var ISearchQuery|null $receivedQuery */
$receivedQuery = null;
$this->searchFolder
->method('search')
->will($this->returnCallback(function ($query) use (&$receivedQuery) {
$receivedQuery = $query;
return [
new \OC\Files\Node\Folder($this->rootFolder, $this->view, '/test/path')
];
}));

$query = $this->getBasicQuery(\SearchDAV\Query\Operator::OPERATION_EQUAL, FilesPlugin::OWNER_ID_PROPERTYNAME, $this->user->getUID());
$query->where = new \SearchDAV\Query\Operator(
\SearchDAV\Query\Operator::OPERATION_AND,
[
new \SearchDAV\Query\Operator(
\SearchDAV\Query\Operator::OPERATION_EQUAL,
[new SearchPropertyDefinition('{DAV:}getcontenttype', true, true, true), new \SearchDAV\Query\Literal('image/png')]
),
new \SearchDAV\Query\Operator(
\SearchDAV\Query\Operator::OPERATION_EQUAL,
[new SearchPropertyDefinition(FilesPlugin::OWNER_ID_PROPERTYNAME, true, true, true), new \SearchDAV\Query\Literal($this->user->getUID())]
)
]
);
$this->search->search($query);

$this->assertNotNull($receivedQuery);
$this->assertTrue($receivedQuery->limitToHome());

/** @var ISearchBinaryOperator $operator */
$operator = $receivedQuery->getSearchOperation();
$this->assertInstanceOf(ISearchBinaryOperator::class, $operator);
$this->assertEquals(ISearchBinaryOperator::OPERATOR_AND, $operator->getType());
$this->assertCount(2, $operator->getArguments());

/** @var ISearchBinaryOperator $operator */
$operator = $operator->getArguments()[1];
$this->assertInstanceOf(ISearchBinaryOperator::class, $operator);
$this->assertEquals(ISearchBinaryOperator::OPERATOR_AND, $operator->getType());
$this->assertEmpty($operator->getArguments());
}
}
5 changes: 4 additions & 1 deletion lib/private/Files/Cache/Cache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -793,7 +793,10 @@ public function searchQuery(ISearchQuery $searchQuery) {
->andWhere($builder->expr()->eq('tag.uid', $builder->createNamedParameter($searchQuery->getUser()->getUID())));
}

$query->andWhere($this->querySearchHelper->searchOperatorToDBExpr($builder, $searchQuery->getSearchOperation()));
$searchExpr = $this->querySearchHelper->searchOperatorToDBExpr($builder, $searchQuery->getSearchOperation());
if ($searchExpr) {
$query->andWhere($searchExpr);
}

$this->querySearchHelper->addSearchOrdersToQuery($query, $searchQuery->getOrder());

Expand Down
8 changes: 6 additions & 2 deletions lib/private/Files/Cache/QuerySearchHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,14 +88,18 @@ public function shouldJoinTags(ISearchOperator $operator) {
* @param ISearchOperator $operator
*/
public function searchOperatorArrayToDBExprArray(IQueryBuilder $builder, array $operators) {
return array_map(function ($operator) use ($builder) {
return array_filter(array_map(function ($operator) use ($builder) {
return $this->searchOperatorToDBExpr($builder, $operator);
}, $operators);
}, $operators));
}

public function searchOperatorToDBExpr(IQueryBuilder $builder, ISearchOperator $operator) {
$expr = $builder->expr();
if ($operator instanceof ISearchBinaryOperator) {
if (count($operator->getArguments()) === 0) {
return null;
}

switch ($operator->getType()) {
case ISearchBinaryOperator::OPERATOR_NOT:
$negativeOperator = $operator->getArguments()[0];
Expand Down
38 changes: 23 additions & 15 deletions lib/private/Files/Node/Folder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
use OCP\Files\Mount\IMountPoint;
use OCP\Files\NotFoundException;
use OCP\Files\NotPermittedException;
use OCP\Files\Search\ISearchOperator;
use OCP\Files\Search\ISearchQuery;

class Folder extends Node implements \OCP\Files\Folder {
/**
Expand Down Expand Up @@ -191,7 +191,7 @@ public function newFile($path) {
/**
* search for files with the name matching $query
*
* @param string|ISearchOperator $query
* @param string|ISearchQuery $query
* @return \OC\Files\Node\Node[]
*/
public function search($query) {
Expand Down Expand Up @@ -229,6 +229,11 @@ public function searchByTag($tag, $userId) {
* @return \OC\Files\Node\Node[]
*/
private function searchCommon($method, $args) {
$limitToHome = ($method === 'searchQuery')? $args[0]->limitToHome(): false;
if ($limitToHome && count(explode('/', $this->path)) !== 3) {
throw new \InvalidArgumentException('searching by owner is only allows on the users home folder');
}

$files = array();
$rootLength = strlen($this->path);
$mount = $this->root->getMount($this->path);
Expand All @@ -252,19 +257,22 @@ private function searchCommon($method, $args) {
}
}

$mounts = $this->root->getMountsIn($this->path);
foreach ($mounts as $mount) {
$storage = $mount->getStorage();
if ($storage) {
$cache = $storage->getCache('');

$relativeMountPoint = ltrim(substr($mount->getMountPoint(), $rootLength), '/');
$results = call_user_func_array(array($cache, $method), $args);
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);
if (!$limitToHome) {
$mounts = $this->root->getMountsIn($this->path);
foreach ($mounts as $mount) {
$storage = $mount->getStorage();
if ($storage) {
$cache = $storage->getCache('');

$relativeMountPoint = ltrim(substr($mount->getMountPoint(), $rootLength), '/');
$results = call_user_func_array([$cache, $method], $args);
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);
}
}
}
}
Expand Down
16 changes: 15 additions & 1 deletion lib/private/Files/Search/SearchQuery.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ class SearchQuery implements ISearchQuery {
private $order;
/** @var IUser */
private $user;
private $limitToHome;

/**
* SearchQuery constructor.
Expand All @@ -48,13 +49,22 @@ class SearchQuery implements ISearchQuery {
* @param int $offset
* @param array $order
* @param IUser $user
* @param bool $limitToHome
*/
public function __construct(ISearchOperator $searchOperation, $limit, $offset, array $order, IUser $user) {
public function __construct(
ISearchOperator $searchOperation,
int $limit,
int $offset,
array $order,
IUser $user,
bool $limitToHome = false
) {
$this->searchOperation = $searchOperation;
$this->limit = $limit;
$this->offset = $offset;
$this->order = $order;
$this->user = $user;
$this->limitToHome = $limitToHome;
}

/**
Expand Down Expand Up @@ -91,4 +101,8 @@ public function getOrder() {
public function getUser() {
return $this->user;
}

public function limitToHome(): bool {
return $this->limitToHome;
}
}
8 changes: 8 additions & 0 deletions lib/public/Files/Search/ISearchQuery.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,4 +66,12 @@ public function getOrder();
* @since 12.0.0
*/
public function getUser();

/**
* Whether or not the search should be limited to the users home storage
*
* @return bool
* @since 18.0.0
*/
public function limitToHome(): bool;
}