Skip to content
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

Jump to bottom

Extend accounts table for custom search attributes #27832

Closed
wants to merge 8 commits into from
Closed

Extend accounts table for custom search attributes #27832

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3422d3d
Add find methods to user manager and group manager
tomneedham May 8, 2017
8e00ed3
Add indexes for improved search performance and use lower uid in query
tomneedham May 11, 2017
9d75572
Add search attribute to user interface
tomneedham May 11, 2017
29fcb72
Add search attribute to user interface
tomneedham May 11, 2017
25b9ce4
Update sharee unit tests for custom search attribute support
tomneedham May 12, 2017
5ebc7bc
Renamed search attribute backend interface
tomneedham May 12, 2017
db21e77
Add tests for group|user manager find methods
tomneedham May 12, 2017
e010774
Update account after changing search attributes string
tomneedham May 12, 2017
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
18 changes: 9 additions & 9 deletions apps/files_sharing/lib/Controller/ShareesController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,17 +141,17 @@ protected function getUsers($search) {
// Search in all the groups this user is part of
$userGroups = $this->groupManager->getUserGroupIds($this->userSession->getUser());
foreach ($userGroups as $userGroup) {
$usersTmp = $this->groupManager->displayNamesInGroup($userGroup, $search, $this->limit, $this->offset);
foreach ($usersTmp as $uid => $userDisplayName) {
$users[$uid] = $userDisplayName;
$usersTmp = $this->groupManager->findUsersInGroup($userGroup, $search, $this->limit, $this->offset);
foreach ($usersTmp as $uid => $user) {
$users[$uid] = $user;
}
}
} else {
// Search in all users
$usersTmp = $this->userManager->searchDisplayName($search, $this->limit, $this->offset);
$usersTmp = $this->userManager->find($search, $this->limit, $this->offset);

foreach ($usersTmp as $user) {
$users[$user->getUID()] = $user->getDisplayName();
$users[$user->getUID()] = $user;
}
}

Expand All @@ -161,21 +161,21 @@ protected function getUsers($search) {

$foundUserById = false;
$lowerSearch = strtolower($search);
foreach ($users as $uid => $userDisplayName) {
if (strtolower($uid) === $lowerSearch || strtolower($userDisplayName) === $lowerSearch) {
foreach ($users as $uid => $user) {
if (strtolower($uid) === $lowerSearch || strtolower($user->getDisplayName()) === $lowerSearch || strtolower($user->getEmailAddress())) {
if (strtolower($uid) === $lowerSearch) {
$foundUserById = true;
}
$this->result['exact']['users'][] = [
'label' => $userDisplayName,
'label' => $user->getDisplayName(),
'value' => [
'shareType' => Share::SHARE_TYPE_USER,
'shareWith' => $uid,
],
];
} else {
$this->result['users'][] = [
'label' => $userDisplayName,
'label' => $user->getDisplayName(),
'value' => [
'shareType' => Share::SHARE_TYPE_USER,
'shareWith' => $uid,
Expand Down
32 changes: 16 additions & 16 deletions apps/files_sharing/tests/API/ShareesTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ public function dataGetUsers() {
true,
['abc', 'xyz'],
[
['abc', 'test', 2, 0, ['test1' => 'Test One']],
['abc', 'test', 2, 0, ['test1' => $this->getUserMock('test1', 'Test One')]],
['xyz', 'test', 2, 0, []],
],
[],
Expand All @@ -320,7 +320,7 @@ public function dataGetUsers() {
false,
['abc', 'xyz'],
[
['abc', 'test', 2, 0, ['test1' => 'Test One']],
['abc', 'test', 2, 0, ['test1' => $this->getUserMock('test1', 'Test One')]],
['xyz', 'test', 2, 0, []],
],
[],
Expand All @@ -335,12 +335,12 @@ public function dataGetUsers() {
['abc', 'xyz'],
[
['abc', 'test', 2, 0, [
'test1' => 'Test One',
'test2' => 'Test Two',
'test1' => $this->getUserMock('test1', 'Test One'),
'test2' => $this->getUserMock('test2', 'Test Two'),
]],
['xyz', 'test', 2, 0, [
'test1' => 'Test One',
'test2' => 'Test Two',
'test1' => $this->getUserMock('test1', 'Test One'),
'test2' => $this->getUserMock('test2', 'Test Two'),
]],
],
[],
Expand All @@ -358,12 +358,12 @@ public function dataGetUsers() {
['abc', 'xyz'],
[
['abc', 'test', 2, 0, [
'test1' => 'Test One',
'test2' => 'Test Two',
'test1' => $this->getUserMock('test1', 'Test One'),
'test2' => $this->getUserMock('test2', 'Test Two'),
]],
['xyz', 'test', 2, 0, [
'test1' => 'Test One',
'test2' => 'Test Two',
'test1' => $this->getUserMock('test1', 'Test One'),
'test2' => $this->getUserMock('test2', 'Test Two'),
]],
],
[],
Expand All @@ -378,10 +378,10 @@ public function dataGetUsers() {
['abc', 'xyz'],
[
['abc', 'test', 2, 0, [
'test' => 'Test One',
'test' => $this->getUserMock('test1', 'Test One'),
]],
['xyz', 'test', 2, 0, [
'test2' => 'Test Two',
'test2' => $this->getUserMock('test2', 'Test Two'),
]],
],
[
Expand All @@ -400,10 +400,10 @@ public function dataGetUsers() {
['abc', 'xyz'],
[
['abc', 'test', 2, 0, [
'test' => 'Test One',
'test' => $this->getUserMock('test1', 'Test One'),
]],
['xyz', 'test', 2, 0, [
'test2' => 'Test Two',
'test2' => $this->getUserMock('test2', 'Test Two'),
]],
],
[
Expand Down Expand Up @@ -442,7 +442,7 @@ public function testGetUsers($searchTerm, $shareWithGroupOnly, $shareeEnumeratio

if (!$shareWithGroupOnly) {
$this->userManager->expects($this->once())
->method('searchDisplayName')
->method('find')
->with($searchTerm, $this->invokePrivate($this->sharees, 'limit'), $this->invokePrivate($this->sharees, 'offset'))
->willReturn($userResponse);
} else {
Expand All @@ -462,7 +462,7 @@ public function testGetUsers($searchTerm, $shareWithGroupOnly, $shareeEnumeratio
}

$this->groupManager->expects($this->exactly(sizeof($groupResponse)))
->method('displayNamesInGroup')
->method('findUsersInGroup')
->with($this->anything(), $searchTerm, $this->invokePrivate($this->sharees, 'limit'), $this->invokePrivate($this->sharees, 'offset'))
->willReturnMap($userResponse);
}
Expand Down
55 changes: 55 additions & 0 deletions core/Migrations/Version20170510143952.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
<?php
/**
* @author Tom Needham <[email protected]>
*
* @copyright Copyright (c) 2017, ownCloud GmbH
* @license AGPL-3.0
*
* This code is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License, version 3,
* as published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License, version 3,
* along with this program. If not, see <http://www.gnu.org/licenses/>
*
*/
namespace OC\Migrations;

use Doctrine\DBAL\Schema\Schema;
use OCP\Migration\ISchemaMigration;

/**
* Adds a string column to the accounts table that can contain search
* attributes provided by user backends
*/
class Version20170510143952 implements ISchemaMigration {

public function changeSchema(Schema $schema, array $options) {
$prefix = $options['tablePrefix'];
$table = $schema->getTable("{$prefix}accounts");

// Add column to hold additional search attributes
$table->addColumn('search_attributes', 'string', [
'notnull' => false,
'length' => 185, // Max index length
]);

// Add index for search attributes
$table->addIndex(['search_attributes'], 'search_attributes_index');
Copy link
Member

Choose a reason for hiding this comment

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

we need a combined index if one query searches in multiple columns

Copy link
Contributor

Choose a reason for hiding this comment

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

@tomneedham any update ?

Copy link
Member

@butonic butonic May 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

nope. we need an index per or part of a query:

-- with combined index
CREATE INDEX find_index ON oc_accounts (lower_user_id, display_name, email);
sqlite> EXPLAIN QUERY PLAN select * from oc_accounts WHERE lower_user_id LIKE '%test%' and display_name LIKE '%test%' and email LIKE '%test%';
0|0|0|SCAN TABLE oc_accounts
sqlite> EXPLAIN QUERY PLAN select * from oc_accounts WHERE lower_user_id LIKE '%test%' or display_name LIKE '%test%' or email LIKE '%test%';
0|0|0|SCAN TABLE oc_accounts
sqlite> EXPLAIN QUERY PLAN select * from oc_accounts WHERE lower_user_id LIKE '%test%' or display_name LIKE '%test%' or email LIKE '%test%';
0|0|0|SCAN TABLE oc_accounts
sqlite> EXPLAIN QUERY PLAN select * from oc_accounts WHERE lower_user_id = '%test%' or display_name = '%test%' or email = '%test%';
0|0|0|SCAN TABLE oc_accounts
sqlite> EXPLAIN QUERY PLAN select * from oc_accounts WHERE lower_user_id = '%test%' and display_name = '%test%' and email = '%test%';
0|0|0|SEARCH TABLE oc_accounts USING INDEX find_index (lower_user_id=? AND display_name=? AND email=?)
sqlite> 

...
-- with individual indexes
CREATE UNIQUE INDEX lower_user_id_index ON oc_accounts (lower_user_id);
CREATE INDEX display_name_index ON oc_accounts (display_name);
CREATE INDEX email_index ON oc_accounts (email);
sqlite> EXPLAIN QUERY PLAN select * from oc_accounts WHERE lower_user_id = '%test%' and display_name = '%test%' and email = '%test%';
0|0|0|SEARCH TABLE oc_accounts USING INDEX lower_user_id_index (lower_user_id=?)
sqlite> EXPLAIN QUERY PLAN select * from oc_accounts WHERE lower_user_id = '%test%' or display_name = '%test%' or email = '%test%';
0|0|0|SEARCH TABLE oc_accounts USING INDEX lower_user_id_index (lower_user_id=?)
0|0|0|SEARCH TABLE oc_accounts USING INDEX display_name_index (display_name=?)
0|0|0|SEARCH TABLE oc_accounts USING INDEX email_index (email=?)
sqlite>

then again like kills the indexes on sqlite anyway:

EXPLAIN QUERY PLAN select * from oc_accounts WHERE lower_user_id like '%test%' or display_name like '%test%' or email like '%test%';
0|0|0|SCAN TABLE oc_accounts

Copy link
Member

Choose a reason for hiding this comment

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

oh well. the %test% like comparison kills the index on all dbms. If we use test% it is much better:

mysql> explain select * from oc_accounts WHERE lower_user_id like '%test%' or display_name like '%test%' or email like '%test%';
+----+-------------+-------------+------------+------+---------------+------+---------+------+------+----------+-------------+
| id | select_type | table       | partitions | type | possible_keys | key  | key_len | ref  | rows | filtered | Extra       |
+----+-------------+-------------+------------+------+---------------+------+---------+------+------+----------+-------------+
|  1 | SIMPLE      | oc_accounts | NULL       | ALL  | NULL          | NULL | NULL    | NULL |    1 |   100.00 | Using where |
+----+-------------+-------------+------------+------+---------------+------+---------+------+------+----------+-------------+

mysql> explain select * from oc_accounts WHERE lower_user_id like 'test%' or display_name like 'test%' or email like 'test%';
+----+-------------+-------------+------------+------+----------------------------------------------------+------+---------+------+------+----------+-------------+
| id | select_type | table       | partitions | type | possible_keys                                      | key  | key_len | ref  | rows | filtered | Extra       |
+----+-------------+-------------+------------+------+----------------------------------------------------+------+---------+------+------+----------+-------------+
|  1 | SIMPLE      | oc_accounts | NULL       | ALL  | lower_user_id_index,display_name_index,email_index | NULL | NULL    | NULL |    1 |   100.00 | Using where |
+----+-------------+-------------+------------+------+----------------------------------------------------+------+---------+------+------+----------+-------------+

sqlite never seems to use an index for like
mysql can use separate indexes as well as a combined index ... if we don't do medial searches.

for ldap we have 'user_ldap.enable_medial_search' in config.php to allow medial searches. we could do the same for sql. say ... 'accounts.enable_medial_search'?

Copy link
Member

Choose a reason for hiding this comment

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

also see http://use-the-index-luke.com/de/sql/where/bereiche/like-filter


// Index to improve search performance of display_name column
$table->addIndex(['display_name'], 'display_name_index');

// Index to improve search performance of email column
$table->addIndex(['email'], 'email_index');

// Index to improve search performance of lower_user_id column
$table->addUniqueIndex(['lower_user_id'], 'lower_user_id_index');

}
}
52 changes: 52 additions & 0 deletions lib/private/Group/Manager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,58 @@ public function getUserGroupIds($user, $scope = null) {
}, array_keys($this->getUserGroups($user, $scope)));
}

/**
* Finds users in a group
* @param string $gid
* @param string $search
* @param int $limit
* @param int $offset
* @return array of user objects
*/
public function findUsersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
$group = $this->get($gid);
if(is_null($group)) {
return [];
}

$search = trim($search);
$groupUsers = [];

if(!empty($search)) {
// only user backends have the capability to do a complex search for users
$searchOffset = 0;
$searchLimit = $limit * 100;
if($limit === -1) {
$searchLimit = 500;
}

do {
$filteredUsers = $this->userManager->find($search, $searchLimit, $searchOffset);
foreach($filteredUsers as $filteredUser) {
if($group->inGroup($filteredUser)) {
$groupUsers[]= $filteredUser;
}
}
$searchOffset += $searchLimit;
} while(count($groupUsers) < $searchLimit+$offset && count($filteredUsers) >= $searchLimit);

if($limit === -1) {
$groupUsers = array_slice($groupUsers, $offset);
} else {
$groupUsers = array_slice($groupUsers, $offset, $limit);
}
} else {
$groupUsers = $group->searchUsers('', $limit, $offset);
}

$matchingUsers = [];
foreach($groupUsers as $groupUser) {
$matchingUsers[$groupUser->getUID()] = $groupUser;
}

return $matchingUsers;
}

/**
* get a list of all display names in a group
* @param string $gid
Expand Down
3 changes: 3 additions & 0 deletions lib/private/User/Account.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@
* @method void setQuota(string $quota)
* @method string getHome()
* @method void setHome(string $home)
* @method void setSearchAttributes(string $searchAttributes)
* @method string getSearchAttributes()
*
* @package OC\User
*/
Expand All @@ -64,6 +66,7 @@ class Account extends Entity {
protected $backend;
protected $state;
protected $home;
protected $searchAttributes;

public function __construct() {
$this->addType('state', 'integer');
Expand Down
19 changes: 19 additions & 0 deletions lib/private/User/AccountMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,25 @@ public function search($fieldName, $pattern, $limit, $offset) {
return $this->findEntities($qb->getSQL(), $qb->getParameters(), $limit, $offset);
}

/**
* @param string $pattern
* @param integer $limit
* @param integer $offset
* @return Account[]
*/
public function find($pattern, $limit, $offset) {
$qb = $this->db->getQueryBuilder();
$qb->select('*')
->from($this->getTableName())
->where($qb->expr()->Like('user_id', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter(strtolower($pattern)) . '%')))
->orWhere($qb->expr()->iLike('display_name', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%')))
Copy link
Member

Choose a reason for hiding this comment

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

Quoting comment from http://stackoverflow.com/a/34029944:

It is in PostgreSQL the keyword ILIKE can be used instead of LIKE to make the match case-insensitive according to the active locale. This is not in the SQL standard but is a PostgreSQL extension.

Copy link
Member

@butonic butonic May 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

What is the question here? We use ILIKE from postgres and have adapters for mysql, sqlite and oracle.

On oracle it is currently problematic when the search string contains [ or ] see #26672

Copy link
Member

Choose a reason for hiding this comment

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

if we have hidden magic then it's ok.

->orWhere($qb->expr()->iLike('email', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%')))
->orWhere($qb->expr()->iLike('search_attributes', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%')))
->orderBy('display_name');

return $this->findEntities($qb->getSQL(), $qb->getParameters(), $limit, $offset);
}

public function getUserCountPerBackend($hasLoggedIn) {
$qb = $this->db->getQueryBuilder();
$qb->select(['backend', $qb->createFunction('count(*) as `count`')])
Expand Down
26 changes: 26 additions & 0 deletions lib/private/User/Manager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@
use OCP\IUser;
use OCP\IUserManager;
use OCP\IConfig;
use OCP\User\IProvidesExtendedSearchBackend;
use OCP\User\IProvidesEMailBackend;
use OCP\User\IProvidesQuotaBackend;
use OCP\UserInterface;

/**
Expand Down Expand Up @@ -240,6 +242,24 @@ public function search($pattern, $limit = null, $offset = null) {
return $users;
}

/**
* find a user account by checking user_id, display name and email fields
*
* @param string $pattern
* @param int $limit
* @param int $offset
* @return \OC\User\User[]
*/
public function find($pattern, $limit = null, $offset = null) {
$accounts = $this->accountMapper->find($pattern, $limit, $offset);
$users = [];
foreach ($accounts as $account) {
$user = $this->getUserObject($account);
$users[$user->getUID()] = $user;
}
return $users;
}

/**
* search by displayName
*
Expand Down Expand Up @@ -405,6 +425,12 @@ private function newAccount($uid, $backend) {
$account->setQuota($quota);
}
}
if ($backend instanceof IProvidesExtendedSearchBackend) {
$searchString = $backend->getSearchAttributes($uid);
if ($searchString !== null) {
$account->setSearchAttributes($searchString);
}
}
$home = false;
if ($backend->implementsActions(Backend::GET_HOME)) {
$home = $backend->getHome($uid);
Expand Down
13 changes: 13 additions & 0 deletions lib/private/User/RemoteUser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,4 +197,17 @@ public function getQuota() {
public function setQuota($quota) {
}

/**
* @inheritdoc
*/
public function getSearchAttributes() {
return '';
}

/**
* @inheritdoc
*/
public function setSearchAttributes($searchString) {
}

}
8 changes: 5 additions & 3 deletions lib/private/User/SyncService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,12 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>
*
*/


namespace OC\User;


use OCP\AppFramework\Db\DoesNotExistException;
use OCP\IConfig;
use OCP\ILogger;
use OCP\User\IProvidesExtendedSearchBackend;
use OCP\UserInterface;

/**
Expand Down Expand Up @@ -161,6 +159,10 @@ public function setupAccount(Account $a, $uid) {
if ($this->backend->implementsActions(\OC_User_Backend::GET_DISPLAYNAME)) {
$a->setDisplayName($this->backend->getDisplayName($uid));
}
// Check if backend supplies an additional search string
if ($this->backend instanceof IProvidesExtendedSearchBackend) {
$a->setSearchAttributes($this->backend->getSearchAttributes($uid));
}
return $a;
}

Expand Down
Loading